Repository navigation
build(plugin): bundle SDK document reader dependencies - #1149
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The shipped bundle’s PDF extraction path lacks an integration test verifying that the injected worker functions at runtime.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Bundles the SDK document reader and PDF worker into the plugin runtime in preparation for the later SDK entrypoint.
Changes:
- Adds PDF worker injection and shared bundle options.
- Expands the packaged runtime chunks and test fixtures.
- Supports plugin-root discovery from standalone MCP bundles.
| File | Description |
|---|---|
sdk/typescript/tests-ts/runtime.test.ts |
Tests standalone plugin-root discovery. |
sdk/typescript/tests-ts/plugin-root.ts |
Loads the expanded runtime fixture. |
sdk/typescript/tests-ts/completed-scan-handoff.test.ts |
Updates generated bundle identifiers. |
sdk/typescript/src/runtime.ts |
Adds the standalone bundle root candidate. |
plugins/codex-security/plugin-files.json |
Declares three additional runtime chunks. |
plugins/codex-security/mcp-app/scripts/test_host_build.mjs |
Renames the host-build test. |
plugins/codex-security/mcp-app/scripts/build_mcp_app.mjs |
Bundles the PDF worker. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| inject: | ||
| name === "server" | ||
| ? [sdkRequire.resolve("pdfjs-dist/legacy/build/pdf.worker.mjs")] | ||
| : [], |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
mldangelo-oai
changed the title
faizan-oai
left a comment
faizan-oai
left a comment
There was a problem hiding this comment.
Reviewed the document-reader bundling changes at e32fbde. The integrated plugin builds, and the native document-reader test passes. Current-head CI passed. Local packaging reused prebuilt native payloads whose source is unchanged.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed the packaging change at e32fbdeab2a73b5cd43268b99db817d3dff8a781 with three independent passes and root verification. No actionable findings. The exact plugin build and both standalone host build tests pass. Runtime/handoff tests passed except four existing ambient-executable assumptions; all four pass with CODEX_CLI_PATH unset (175 passing tests across the runs, 22 skipped).
A separate probe loaded the emitted PDF worker and extracted synthetic PDF text using the matching reader. Full document reading through the shipped server remains an activation-boundary check, since that reader is not yet included in this staged server entrypoint.
1533a29 to
b1c2502
Compare
e32fbde to
a1fb609
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
The previously completed three independent passes carry forward after verifying the patch is identical; root reconciled the current base and discussion.
No serious outstanding finding was identified in this change. The scoped prior review and current diff remain consistent.
Approval is for this change. Integrated-tip validation passed 1,357 SDK tests (31 skipped), 105 MCP tests (2 skipped), portable checks, types/builds and installed-package smoke. Other stack findings and known CI fixture failures still prevent the stack from being mergeable.
b1c2502 to
485bce5
Compare
a1fb609 to
20da3e0
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Re-reviewed the real PR diff at 20da3e0c406fba80e9ddb6140ea04170eebf09ae. The patch is unchanged from the prior three-pass review. I reconciled the inherited changes in touched files and retained the prior verified findings and tests.
No serious outstanding finding identified in this patch; prior conclusions remain consistent with the unchanged change or the current re-review.
Validation across touched paths at branch tip 06e75cfe: 1,356 SDK tests, 867 Python tests plus 100 subtests, and 108 MCP tests passed; builds, types, portable checks and installed-package checks passed. These are integration checks at the branch tip, not a claim that every intermediate PR or its CI is clean.
485bce5 to
85da8b2
Compare
20da3e0 to
55662ed
Compare
85da8b2 to
94b5226
Compare
55662ed to
7c33edf
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Retained the three independent passes for the identical patch and reconciled inherited touched-file changes at 7c33edf31fa4.
No actionable introduced finding survives the independent reviews and root reconciliation of this exact patch, touched-file changes, and current integration checks.
Current tip 355ec321 passed 75 report-projection tests, portable source checks, SDK build:ci and plugin build. The immediately preceding tip fec36f1d passed 1,360 SDK tests, 873 Python tests plus 100 subtests, 109 MCP tests, builds/types and the clean 600-entry installed-package check. Only report_projection.py and its test changed between those tips; SDK/MCP runtime files are byte-identical. Both descend from main 008a8b4d. These are integration checks, separate from own-head probes. No native Windows local run.
No serious outstanding finding introduced by this PR. Code review approval does not establish CI or merge readiness.
7c33edf to
8d37974
Compare
94b5226 to
7226902
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Retained the three independent passes for the verified identical patch and reconciled inherited touched-file changes at 8d37974a8657.
No actionable introduced finding survives the independent reviews and root reconciliation of this exact patch and inherited changes in touched files.
Integrated tip 6ef8cbb8, based on 008a8b4d, passed 1,388 SDK tests, 879 Python tests plus 100 subtests, 109 MCP tests, portable checks, builds and the clean 600-entry installed-package check. These are branch-tip integration checks, separate from own-head probes; no local Windows execution was performed.
No serious outstanding finding introduced by this PR. Code approval is separate from CI and merge readiness.
7226902 to
b364974
Compare
8d37974 to
5cf010f
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Retained three prior independent passes for the verified identical patch and reconciled inherited touched-file changes at 5cf010f9725e.
No actionable introduced finding survives the independent reviews and root reconciliation of this exact patch and inherited changes in touched files.
Branch tip 3e0083d9, based on 008a8b4d, passed 1,392 SDK tests, 881 Python tests plus 100 subtests, 109 MCP tests, builds, portable source checks and the 600-entry installed-package check. These are tip checks, separate from own-head probes. No local Windows run was performed. The tip excludes the dependency update merged separately in #1184; combined validation remains outstanding.
No serious outstanding finding introduced by this PR. Code approval is separate from CI and merge readiness.
Consolidate approved Deep Scan changes into the parent topic branch.
2da950f
into
mdangelo/codex/pr939-split-25-codex-executable
Part 26 of 46. Previous: #1148 · Next: #1150 · Stack index
Summary
Prepare the plugin bundle to load the SDK document reader, including the PDF worker, before the SDK-backed plugin entrypoint is enabled.
Changes
Testing
Checks against the complete stack passed: SDK and MCP typechecks, formatting, Ruff, the SDK build, portable plugin compatibility and its tests, focused recovery/accounting tests, and local-artifact package smoke using exact cached dependencies. The package check was not a clean registry install. See this PR’s Checks tab for CI results for its current commit.
Risk and rollout
This changes the shipped bundle and plugin-root discovery. The plugin scan entrypoint switches to the SDK in part 40; packaging and source-level document-reader checks cover different parts of that rollout.
Public disclosure review