Repository navigation
refactor: share scan result preparation between the SDK and plugin - #1114
Conversation
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. |
|
@codex review |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It relocates core scan-finalization semantics across package boundaries, and the full runtime suites were not rerun on this head.
Review effort: Balanced
Findings: None
What changed in this PR
Shares scan semantics and draft preparation between the TypeScript SDK and MCP artifact tooling.
Changes:
- Generates semantic scan types from the checked-in schema.
- Moves draft preparation and evidence-preservation logic into the SDK.
- Derives candidate validation from schema and updates CI dependencies.
| File | Description |
|---|---|
.github/workflows/node-ci.yml |
Installs shared SDK dependencies in plugin jobs. |
plugins/codex-security/mcp-app/src/artifact-candidate.ts |
Adds schema-derived candidate validation. |
plugins/codex-security/mcp-app/src/artifact-discovery.ts |
Uses the shared candidate schema. |
plugins/codex-security/mcp-app/src/artifact-scan-draft.ts |
Delegates draft preparation to the SDK. |
plugins/codex-security/mcp-app/tests/test_artifact_discovery.mjs |
Tests strict and passthrough candidate validation. |
sdk/typescript/scripts/check-package.mjs |
Verifies new SDK modules are packaged. |
sdk/typescript/scripts/generate-models.cjs |
Generates semantic models from plugin schemas. |
sdk/typescript/src/scan-semantics.ts |
Adds shared semantic preparation and preservation logic. |
sdk/typescript/src/semantic-models.ts |
Adds generated semantic scan types. |
sdk/typescript/tests-ts/helpers/semantic-scan.ts |
Provides semantic scan test fixtures. |
sdk/typescript/tests-ts/runtime.test.ts |
Tests shared finding identity preparation. |
sdk/typescript/tests-ts/scan-semantics-preservation.test.ts |
Tests lossless evidence preservation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b447135361
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| validateCoverageSemantics, | ||
| validateFindingSemantics, | ||
| type SemanticScan, | ||
| } from "../../../../sdk/typescript/src/scan-semantics.js"; |
There was a problem hiding this comment.
When the plugin is checked out or distributed by itself, this relative import points outside the available tree, so the build_mcp_app.mjs typecheck/bundle step cannot resolve it. That remains a documented workflow in plugins/codex-security/native/README.md:66-74 and was previously verified by the plugin-host sparse checkout; expanding that workflow's checkout to include sdk/typescript only masks the regression. Keep the shared implementation within the standalone plugin boundary or package it as an explicit dependency.
Useful? React with 👍 / 👎.
mldangelo-oai
changed the title
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed b44713536176c7b3fc9ea831e1e5802aa452ec41 against mdangelo/codex/pr939-stack-08-scan-completion at 54d8f2b680016dd2a40dc14c8fa74baa730fa40e, scoped to this PR's diff and touched files.
Reviewed draft preparation, schema/model generation, evidence preservation, packaging, and changed tests. No additional distinct findings beyond the existing standalone-plugin build comment. An isolated base/head comparison passed 340 synthetic semantic-draft and evidence-preservation cases.
Review included simplification/deletion opportunities. Full repository and platform test suites were not run.
b447135 to
3399fc0
Compare
1a70e3c to
dc1f6e8
Compare
0f9bc43 to
9ee3131
Compare
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
faizan-oai
left a comment
faizan-oai
left a comment
There was a problem hiding this comment.
Reviewed shared finding/coverage preparation, identity handling, and the SDK/plugin call sites. Reusing the same preparation path avoids two implementations of these semantics. No issue found in this layer. Semantic-preservation and merge tests passed at the integrated stack tip.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed the exact base-to-head diff with three independent review passes and root verification. Shared semantic construction preserves the previous target/scope, finding and coverage behavior, and the preservation optimization retains exact JSON equality and ordering. No actionable findings remain.
Validation: 8 semantic preservation tests, the artifact discovery suite, both standalone host build tests, and generated-model freshness passed. The current documentation and CI explicitly require SDK source when building the plugin; the resulting standalone host bundle built and loaded successfully without a separate runtime SDK installation.
9ee3131 to
e03af65
Compare
3ad3023 to
a9b405b
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.
a9b405b to
af0944b
Compare
e03af65 to
6d755bd
Compare
There was a problem hiding this comment.
Re-reviewed the real PR diff at 6d755bdf3f0f350046f3d60c64d9909c75bad32d. 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. Latest workflow runs for this unchanged head were verified successful at 2026-10-02 01:26 UTC. Earlier failed or canceled job records are superseded; this CI status correction does not alter the review decision.
af0944b to
e89bf60
Compare
6d755bd to
b5a0ca9
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 b5a0ca93efbe.
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.
b5a0ca9 to
d9a2b21
Compare
e89bf60 to
71822c8
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 c63ff821d7f5.
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.
c63ff82 to
474de61
Compare
d655625 to
c3eeedb
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed 474de6118a38f13e842106c2468e470043747757 against c3eeedb245c6c0270cd52131c0aff7d06522d271 with exactly three independent full-diff passes, including all changed tests, and independent root verification. No serious introduced findings remain.
Local macOS arm64 validation: 183 SDK tests passed, 22 skipped; fresh native host build, standard plugin build, SDK build, types, and formatting checks passed. The complete artifact-discovery MCP script, Ruff 0.16.9, portable source compatibility checks, and all 9 source-check tests also passed.
No model execution or local Windows run. This approval covers this exact PR contribution and does not establish validation of the whole stack with current main.
At the prepublication check, current-head CI was still running with no reported failures. Approval is not a claim of completed CI or merge readiness.
c3eeedb to
d354fe0
Compare
474de61 to
34cba87
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed 34cba879e215aaf646f97326cbd8f6698ae88958 against d354fe0480492fb72e042399ebe78d705d269e4e with exactly three fresh independent full-diff passes, including every test change, and root verification. No serious introduced findings remain.
Local macOS arm64 validation: 183 SDK tests passed, 22 skipped. The complete artifact-discovery MCP test script, nine portable source-check tests, Ruff 0.16.9, source compatibility, fresh native host/plugin/SDK builds, types and formatting passed.
No model execution, full installed-package smoke or local Windows run. This approval covers this exact contribution and does not establish whole-stack validation with current main.
At the last check, current-head hosted CI was still running with no failed latest-attempt jobs. This approval does not assert completed CI or merge readiness.
d354fe0 to
8eed8fd
Compare
34cba87 to
5a447dc
Compare
There was a problem hiding this comment.
The contribution, all touched blobs and the complete head tree are identical to the previously reviewed version. Root verified that identity and retained the exactly three prior independent full-diff reviews, including tests. No serious introduced findings remain.
Retained validation: 183 focused SDK tests passed, 22 skipped; the complete artifact-discovery MCP script, native host/plugin/SDK builds, types, formatting, Ruff, compatibility checks and nine source-check tests passed. No duplicate review or test runs were needed.
Current-head hosted CI was still running with no failed latest-attempt jobs at the prepublication check. This approval covers the reviewed source and does not assert completed CI or whole-stack integration with current main. No model execution, installed-package smoke or local Windows run.
CI follow-up: the later observation found two primary failures in job111303672085 (tests / macos-latest / shard-3, run37157110125 attempt1). The deep and standard attribution-concurrency tests expected delegated attribution observed but received EACCES while inspecting the bundled plugin scripts/pycache directory. The relevant source blobs match base/prior revisions, and the complete head tree matches the previously reviewed head. The cause is not established; this is not proof of flakiness or an introduced source regression. The source approval remains, but CI is not clear and merge readiness is blocked pending investigation or a successful hosted rerun. The job used Bun1.3.14 and reported 968 passed, five skipped and two failed tests. No local test rerun or source changes were made for this follow-up.
8eed8fd to
063e279
Compare
5a447dc to
64e7b79
Compare
There was a problem hiding this comment.
Reviewed exact head 64e7b791ed7385306aaba3c12717fbeb232ef4af against 063e279c2e0e28dde8846af3828c04b5aa847d02.
Completed exactly three fresh independent HIGH reasoning full-diff reviews, including every test diff, followed by root verification. The Python helper now disables bytecode writes with -B, and the restrictive-umask test uses an isolated plugin fixture and checks that no Python cache is created. No serious introduced finding remains. Fresh native host, plugin and SDK builds, types, formatting, 249 SDK tests (22 skipped), the complete artifact-discovery MCP script, Ruff, compatibility checks and all 9 portable source tests passed.
Hosted CI was still pending at the 23:28 UTC check without failed jobs on this head. This review does not establish merge readiness or whole-stack integration. Validation was local macOS arm64 without model execution, source edits, a local Windows run, or a full installed-package smoke test.
CI update after source approval: Windows Node 22 installed-package job 111316613272 in run 37161518740 attempt 1 failed with PluginPythonUnavailableError during CodexSecurity.run session preparation (package-behavior.mjs:165). CPython 3.12.10 setup had succeeded, but the log does not expose interpreter probe diagnostics, so the reason for rejection and source attribution remain unestablished. Job metadata is tied to this PR head; the checkout and package artifact use merge commit 8a97b33. At that observation this first-attempt failure blocked merge readiness; preserve it separately from later attempt results. Please investigate the hosted Python resolution failure or rerun the failed job with useful probe diagnostics.
https://github.com/openai/codex-security/actions/runs/37161518740/job/111316613272
CI follow-up, 2026-10-04 00:13 UTC: the latest metadata for attempt 2 has a different primary failure. Windows baseline job111323832533 logged EBUSY while removing a test temporary directory and marked “launches long Windows executable paths through the Node runtime” failed (3580 passed, 98 skipped, 1 failed). Inventory job111323834595 then rejected the mismatch between that failed baseline outcome and passed outcomes in other modes; it is a comparison consequence, not a second runtime cause. The test body and cleanup registration match the base. The log does not identify the lock owner or precise removal caller, so source attribution and cause remain unestablished; this is not a flakiness determination. Both jobs checked out merge8a97b3304cfdb96c829a6906ae16f83a3afe42e1. Latest-attempt metadata retains execution beginning on October3, including the EBUSY at23:47:38UTC; no claim that every job reran at this observation. Please investigate the locked temporary directory or rerun with cleanup/process diagnostics. At that observation, source approval remained but hosted CI was incomplete/failing and merge readiness was not established.
https://github.com/openai/codex-security/actions/runs/37161518740/job/111323832533
CI clearance, 2026-10-04 01:08 UTC: run 37161518740 attempt 3 completed successfully with 66 passed and 1 skipped unique latest-attempt jobs; the last job completed at 2026-10-04T01:03:15Z. The exact head, base, actual GitHub diff, three prior HIGH review reports and touched source snapshots are unchanged and verified. The CI-only hold is cleared. No new code review pass, local test run or log download was needed. The earlier Python-resolution and EBUSY causes remain unexplained; a successful later attempt does not establish flakiness or a source fix. Latest-attempt metadata includes retained earlier executions, so these counts do not mean all jobs reran freshly. This is hosted CI evidence for this PR head, not successful whole-stack/current-main validation.
https://github.com/openai/codex-security/actions/runs/37161518740/attempts/3
063e279 to
cbe37d3
Compare
64e7b79 to
89dabf3
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Source review approved for 89dabf30c8b74b29424f6f1f422374c200478555 against cbe37d38913e39e5065e679b935b41a08f791d95. Completed exactly three fresh independent HIGH full actual-PR-diff passes, including every changed test and fixture, followed by root verification. No supported introduced source finding remains in this contribution.
Fresh focused validation passed 249 SDK tests with 22 skips, the complete artifact-discovery MCP script, 9 portable source tests, builds, source type checks, formatting and the production archive check. Initial portable invocations used the wrong Ruff version and lacked Git context; corrected once with the required Ruff 0.16.9 and an exact-head disposable index, both Ruff checks and source compatibility passed. Original invocation failures remain recorded.
CI remains pending at the 2026-10-04T06:25:55.396371+00:00 audit; this is source approval only. Source approval and individual CI do not establish whole-stack/current-main successful validation or merge readiness. No model execution, local Windows run, native rebuild or clean installed-package smoke is claimed.
89dabf3 to
bea6c64
Compare
cbe37d3 to
43ea131
Compare
There was a problem hiding this comment.
Source approval for bea6c64a79e5465ad6911f3f082bfa754dc50ec5 against 43ea1314b3a540345632c78a3323557589954513. The ordered actual GitHub contribution is unchanged. All three prior independent HIGH full-diff reviews, including tests and fixtures, are retained after touched own/base modes and bytes were reconciled; inherited touched-file changes match the previously reviewed main changes on both sides. Root synthesis found no new supported finding.
Prior validation retains its original head and execution time; no duplicate local test or build was run for this unchanged contribution.
The prefix contains main at 634532d, while main has separately advanced to 1fb0e5f. This source approval does not establish successful whole-stack integration. Exact-head CI was pending in the 2026-10-04 09:26 UTC receipt and is tracked separately. No clean install, local Windows run or model execution is claimed.
CI update, 2026-10-04 09:39 UTC: current-head Windows Node22 test shard2 failed; this remains source-approved but CI-held. The separate Windows test-quality shard1 log records three inherited provider-fixture failures on the new empty-stderr assertion introduced in #1111; its overall workflow was still pending in this receipt. The source finding is #1111 (comment). Original execution times and source-review results remain separate; no local Windows/model run or CI retry was performed.
bea6c64 to
e947be8
Compare
43ea131 to
e93a32d
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Source approval for e947be8371781fe626b2768101c342300b0bce91 against e93a32d999332a89eec45f104076e4f561067785. The actual GitHub diff, ordered contribution and touched own/base file modes, OIDs and bytes match the prior reviewed contribution. Exactly three prior independent HIGH full-diff reports, including tests and fixtures, are retained after reconciliation; there is no inherited touched residual and root synthesis found no new supported finding.
Historical validation keeps its original head and execution time. No unchanged local source review, test or build was repeated for this contribution.
Exact-head hosted CI remained pending in the 2026-10-04 10:32–10:33 UTC observation; historical failures at older heads are separate.
The prefix contains main 1fb0e5fbf8684c926cac2a36d08389b212d28d5a. Successful whole-stack integration remains unestablished. No clean installation, fresh native compilation, local Windows run or model execution is claimed.
e93a32d to
57f13aa
Compare
57f13aa to
cf737e9
Compare
e947be8 to
8b1558b
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed head 8b1558b4bbc48098a8a41d969c804366d7acb350 against base cf737e9a6e02e36f524452f85a03f8066ffe0b12 using three fresh independent HIGH full reviews of the actual GitHub PR diff, including every changed test and fixture, followed by root synthesis. Actual three-dot and own-base diffs match.
Shared semantic draft construction, finding identity/evidence preservation, candidate schema extraction and generated models have no supported introduced finding. Three paths dropped from this actual contribution because they are already present identically in base/head and match the separately reviewed dependency. All remaining touched head bytes match the earlier reviewed head; no unchanged product tests were rerun.
Hosted CI is still pending at the latest saved observation, with no failed jobs observed for this head. Source approval is separate from CI and integration. The reviewed prefix contains main 1fb0e5f, but successful whole-stack integration remains unestablished. No product models, clean installation, local Windows execution or new native compilation were performed.
Consolidate approved Deep Scan changes into the parent topic branch.
Part 9 of 46. Previous: #1113 · Next: #1115 · Stack index
Summary
Move finding identity, target, coverage, and evidence-preservation logic into a shared SDK module so the plugin and later Deep Scan merge use the same result format.
Changes
Testing
Earlier validation of the policy, export, and attribution suites passed 72 tests with 384 assertions, including the artifact-cache regression now introduced in #1103. SDK types, the SDK CI build, fresh plugin packaging, formatting, and diff checks passed. Shared semantic integration also passed SDK/MCP and generated-model checks, Ruff check/format, portable source compatibility, and all nine compatibility tests.
After an earlier rebase onto main, 134 grouping, package-build, workflow, release, and report checks passed with 800 assertions; the SDK CI build and fresh plugin bundle passed. See Checks for hosted CI on the current commit.
Risk and rollout
Plugin draft saving uses the shared implementation immediately. Building the plugin from source now requires a checkout containing the SDK; the packaged plugin remains self-contained. The existing scan engine stays in use.
Public disclosure review