feat: unify Deep Scan and Standard scan phases - #267
Conversation
|
@codex review Please review the exact current head |
|
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". |
![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: 6140788eb3
ℹ️ 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".
| commit_deep_dedup.add_argument("--result-manifest-path", required=True) | ||
| commit_deep_dedup.add_argument("--new-findings-count", type=non_negative_int, required=True) |
There was a problem hiding this comment.
Every Deep Scan that reaches its first reducer now fails because the shipped MCP runtime still invokes commit-deep-scan-dedup with the removed --canonical-inventory-path, --canonical-finding-report-path, and other canonical-artifact arguments. The updated command's --help accepts only the four arguments retained here, so argparse exits with an unrecognized-arguments error before commit_deep_scan_dedup runs and start_codex_security_deep_scan cannot produce its terminal manifest. Update the bundled coordinator in the same release or retain compatibility with its existing arguments.
Useful? React with 👍 / 👎.
| 2. Read `references/repository-wide-scan.md` and follow its standard procedure. Call `prepare_codex_security_review_items({ scanId, handoffClaimToken? })`, read every page from `list_codex_security_review_items({ scanId, handoffClaimToken?, cursor?, limit? })`, review every returned source file, and record the complete discovered candidate set once with `record_codex_security_discovery_candidates({ scanId, candidates })`. | ||
| 3. Run `$validation` once over the candidates returned by `list_codex_security_candidates({ scanId, cursor?, limit? })` in compact standard-scan mode. Call `record_codex_security_candidate_validations({ scanId, validations })` with exactly one concise validation record per candidate. Preserve the candidate id, locations, instance, and discovery evidence. | ||
| 4. Run `$attack-path-analysis` once in compact standard-scan mode over candidates whose validation disposition is `reportable` or `deferred`. Use the threat model to establish reachability and severity, and call `record_codex_security_candidate_attack_paths({ scanId, attackPaths })` with exactly one concise attack-path record for each eligible candidate. Do not create ranking or phase queues, per-candidate subagent fan-out, receipts, or narrative phase reports. | ||
| 5. Assemble the semantic findings and coverage using `../../references/final-report.md`, and call `record_codex_security_scan_draft({ scanId, handoffClaimToken?, scope?, threatModel?, findings, coverage })`. Include candidates that survive both compact phases, map rejected, not-applicable, and deferred candidates to the corresponding coverage outcomes, and preserve the relevant code locations. |
There was a problem hiding this comment.
The packaged .mcp.json launches the bundled mcp/server.mjs runtime, but its tool registrations contain none of the newly mandated prepare_codex_security_review_items, candidate list/record, draft-recording, or completed-scan tools. Consequently every Standard scan following this workflow stops on an unavailable tool before it can complete discovery or write canonical artifacts; adding schemas and instructions alone does not register MCP tools. Ship the corresponding runtime implementations and registrations, or retain the prior artifact-based workflow.
Useful? React with 👍 / 👎.
| "required": [ | ||
| "id", | ||
| "reason" | ||
| ], |
There was a problem hiding this comment.
When a scan has deferred work and the caller follows the new final-report contract by preserving only its candidateId or omitting an identifier for the workbench to derive, this schema rejects the draft because id is unconditionally required. The rejection occurs before the writer can perform the promised derivation, so otherwise valid partial-coverage scans cannot record their canonical draft or proceed to completion. Make id optional at the input boundary and validate or derive it in the writer.
Useful? React with 👍 / 👎.
| parse_scan_cost, | ||
| require_close_reason, | ||
| path_within_scope, | ||
| require_close_note, |
There was a problem hiding this comment.
Every attempt to change a finding's triage state now raises NameError before opening the transaction: this import was renamed to require_close_note, but set_finding_triage still calls require_close_reason at line 1868. This affects both opening and closing findings, so the workbench cannot persist any triage update until the call site is renamed as well.
Useful? React with 👍 / 👎.
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
@codex review Please review the exact updated head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b890bb4592
ℹ️ 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".
| In the final response, link the generated markdown report path as the primary readable artifact. | ||
|
|
||
| Every scan mode uses the same final report pipeline. The model authors canonical JSON only; it must not author, repair, or treat an existing `report.md` as input. For an app-backed running scan, author `scan-manifest.json` as an unsealed draft and omit `scan.sealedAt` and `scan.artifacts`; finalization owns the exact workbench timestamps, seal, artifact digests, and derived finding identities. `complete-scan` invokes finalization, which validates and enriches the canonical JSON, seals the canonical JSON and evidence artifacts, then deterministically generates and validates `report.md` as an unsealed downstream projection. Missing report prose must be added to the structured canonical fields rather than recovered from a separately authored report. | ||
| Every scan mode uses the same final report pipeline. For compact Standard and Deep scans, submit canonical semantics with `record_codex_security_scan_draft({ scanId, handoffClaimToken?, scope?, threatModel?, findings, coverage })`. The workbench writes the unsealed canonical draft and supplies the authoritative target, exact workbench timestamps, and derived finding identities. Other modes retain their existing canonical JSON workflow. No mode authors, repairs, or treats an existing `report.md` as input. `complete-scan` invokes finalization, which validates and enriches the canonical JSON, seals the canonical JSON and evidence artifacts, then deterministically generates and validates `report.md` as an unsealed downstream projection. Missing report prose must be supplied in the structured canonical semantics rather than recovered from a separately authored report. |
There was a problem hiding this comment.
When two independently reportable candidates have the same ruleId and title and omit the optional identity, the bundled runtime's buildFindings gives both the same title-derived anchor. The finalizer fingerprints only the target, rule, anchor, and instance—not locations or the candidate identity retained in extensions—so it treats the second finding as a duplicate and silently keeps only the stronger one. This loses distinct route, operation, or sink instances despite the new promise that the workbench derives finding identities; derive the anchor or instance from the canonical candidate identity or other per-instance material instead.
Useful? React with 👍 / 👎.
| When `complete_codex_security_scan` is available, use it to complete the scan. In Codex CLI or another terminal/chat host without that tool, run `python <plugin_dir>/scripts/finalize_scan_contract.py --scan-dir <scan_dir> --source-root <repo_root>` after writing the completed canonical JSON. Do not mark the scan goal complete until this command succeeds and the generated markdown report exists. | ||
| For each surviving compact candidate, supply the evidence-supported lowercase vulnerability-family `ruleId`; `taxonomy: { category, cwe }` using the candidate's exact `cwe_ids`; verified locations; and `provenance.source`, using `"local_plugin"` only when this plugin actually discovered the finding. Preserve the canonical candidate identity and genuine worker or source provenance in the existing finding extensions. A candidate with no known CWE retains `cwe: []`; never invent a classification. Include optional `codeEvidence` only when its actual code is nonempty and every referenced evidence ID is present. | ||
|
|
||
| Supply semantic coverage as `{ completeness, surfaces, explicitExclusions, deferred }`, with each surface using the actual `label` and one existing `disposition`. Mark coverage `partial` when a deferred item or `needs_follow_up` surface remains; preserve its real reason and supporting context. Each deferred item needs a meaningful reason; preserve any existing `id` or `candidateId`. The workbench derives a missing ID from its candidate identity or stable deferred-work details. Open questions may be nonempty strings or `{ question, followUpPrompt? }` objects. The workbench derives target and scope metadata, scope include and exclude paths, coverage mode and inventory strategy, finding identities and fingerprints, and surface IDs. Do not put those workbench-owned values or top-level coverage receipt references into the semantic draft. |
There was a problem hiding this comment.
When a compact scan follows this contract and supplies an open question as a nonempty string, record_codex_security_scan_draft rejects it before writing because both the checked-in schema and the embedded runtime schema define every openQuestions item as an object requiring question. Either accept and normalize the documented string form or remove it from the contract so otherwise valid drafts do not consume a retry and fail with -32602.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: b890bb4592
Only the user who started this review can view the report in Codex.
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
|
|
||
| def generate_in_scope_files(repository: Path, scope: str, output: Path) -> int: | ||
| """Atomically write the exact ripgrep inventory sorted as ``LC_ALL=C``.""" | ||
| command = ["rg", "--files", "--hidden", "--glob", "!.git/**", "--", scope] |
There was a problem hiding this comment.
generate_in_scope_files.py builds the Standard scan inventory with rg --files but does not pass --no-ignore, so repository-controlled .gitignore/ignore rules can remove tracked files from the review list. The Standard workflow then reviews every returned item and can complete without accounting for the hidden tracked file, allowing an incomplete scan to be reported as complete. Add --no-ignore (while still excluding .git/**) or explicitly record ignored files as exclusions/deferred coverage.
Useful? React with 👍 / 👎.
…odex/unify-deep-scan-phases
|
@codex review Please review the exact updated head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71200b0b89
ℹ️ 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".
| const schema = JSON.parse( | ||
| await readFile( | ||
| join(PLUGIN_ROOT, "schemas", "tools", "scan-draft.schema.json"), | ||
| "utf8", |
There was a problem hiding this comment.
Fresh evidence at the updated head shows the previous deferred-ID issue remains in the shipped runtime: this regression reads only the checked-in JSON, while the decompressed server.mjs.br still embeds required: ["id", "reason"]. Because MCP validates against that embedded scanDraftInputSchema before invoking the writer, a deferred item relying on the documented derived ID still receives -32602; exercise the bundled schema here and regenerate the runtime with id optional.
Useful? React with 👍 / 👎.
| "identity": { | ||
| "$ref": "#/$defs/identity" | ||
| }, |
There was a problem hiding this comment.
For a compact Standard or Deep draft that supplies identity, this schema accepts the supposedly workbench-owned value and the updated runtime's buildFindings returns that finding unchanged. Two distinct candidates can therefore provide the same identity and still be collapsed by finalization, bypassing the new candidate-ID derivation; prohibit identity at this input boundary or always overwrite it from extensions.candidateId.
Useful? React with 👍 / 👎.
|
|
||
| def generate_in_scope_files(repository: Path, scope: str, output: Path) -> int: | ||
| """Atomically write the exact ripgrep inventory sorted as ``LC_ALL=C``.""" | ||
| command = ["rg", "--files", "--hidden", "--no-ignore", "--glob", "!.git/**", "--", scope] |
There was a problem hiding this comment.
In repositories with ignored untracked dependency, build, cache, or nested-checkout directories, this now inventories all of their contents rather than only restoring ignored tracked files. The installed rg --help states that --no-ignore stops respecting .gitignore, .ignore, and .rgignore; moreover, !.git/** excludes only the root metadata directory, so a nested vendor/repo/.git/** is also returned. This can make the authoritative review list overwhelmingly consist of generated/vendor and Git metadata files, exhausting scan limits before product sources are reviewed; union tracked files with the normally filtered inventory instead.
Useful? React with 👍 / 👎.
|
|
||
| def generate_in_scope_files(repository: Path, scope: str, output: Path) -> int: | ||
| """Atomically write the exact ripgrep inventory sorted as ``LC_ALL=C``.""" | ||
| command = ["rg", "--files", "--hidden", "--no-ignore", "--glob", "!.git/**", "--", scope] |
There was a problem hiding this comment.
On Windows, every nested review item emitted by this command uses backslashes, but the bundled repositoryPath schema rejects any path containing \, so list_codex_security_review_items fails as soon as it parses such an inventory and both Standard and Deep scans stop before discovery. The installed rg --help explicitly says --path-separator defaults to / on Unix and \ on Windows; pass --path-separator / so the inventory satisfies the repository-relative path contract on all supported platforms.
Useful? React with 👍 / 👎.
| "required": ["candidateId", "sources", "remediationSubsumption"], | ||
| "properties": { | ||
| "candidateId": { "$ref": "#/$defs/candidateId" }, | ||
| "sources": { | ||
| "type": "array", | ||
| "minItems": 1, | ||
| "items": { "$ref": "#/$defs/mergeSource" } |
There was a problem hiding this comment.
When an early reducer merges two distinct source candidates under one canonical ID and a later discovery provides evidence that they must be split, the bundled runtime's validateStableCandidateIdentities rejects the correction: every output containing either old source must retain the old ID, while canonical candidate IDs must remain unique, so only one side of the split can be represented. This makes an erroneous early merge permanent and can silently suppress a distinct vulnerability despite later Deep passes; identity stability should preserve an ID for one descendant while allowing newly split descendants to receive new IDs.
Useful? React with 👍 / 👎.
…odex/unify-deep-scan-phases
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a7f67ced5
ℹ️ 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".
| 7. Verify on disk that `scan-manifest.json`, `findings.json`, and `coverage.json` exist at the workflow-owned scan path, then complete the scan once by calling `complete_codex_security_scan({ scanId })` so the workbench validates and seals the contract, generates `report.md`, and indexes findings. Do not call completion before those files exist. | ||
| 4. Run `$codex-security:validation` once in compact standard-scan mode over the canonical merged candidates, recording every result with `record_codex_security_candidate_validations`. | ||
| 5. Run `$codex-security:attack-path-analysis` once in compact standard-scan mode over the reportable or deferred validated candidates, recording every decision with `record_codex_security_candidate_attack_paths`. | ||
| 6. Assemble complete finding and coverage semantics using `../../references/final-report.md` and `../../references/finding-detail-fields.md`, then call `record_codex_security_scan_draft({ scanId, handoffClaimToken?, scope?, threatModel?, findings, coverage })`. |
There was a problem hiding this comment.
When start_codex_security_deep_scan reaches a successful terminal state, the decompressed bundled mcp/server.mjs.br still returns deepScanTerminalResult, which instructs the model to author and verify scan-manifest.json, findings.json, and coverage.json directly before completion. That contradicts the compact workflow required here, where record_codex_security_scan_draft must perform the write and derive workbench-owned semantics; because this stale text is delivered by the blocking tool immediately before the tail phase, a Deep Scan can follow the legacy path and bypass the new draft writer. Regenerate the bundled tool description and terminal result so they direct the caller through the compact draft and completed-scan tools.
Useful? React with 👍 / 👎.
| 1. Read `$codex-security:security-scan` and preserve its repository-wide or scoped-path artifact and final-report contracts. | ||
| 2. Sanity-check that the canonical candidate inventory, canonical `finding_discovery_report.md`, deduped candidate JSONL, and per-candidate ledgers describe the same candidate set. If they disagree, report the tool failure and stop; do not repair coordinator-owned discovery artifacts, reopen discovery, or silently drop candidates. | ||
| 1. Read `$codex-security:security-scan` and use its existing repository-wide or scoped-path compact artifact and final-report contracts. | ||
| 2. Read the ordinary canonical review items and candidate set with `list_codex_security_review_items({ scanId, handoffClaimToken?, cursor?, limit? })` and `list_codex_security_candidates({ scanId, cursor?, limit? })`. Follow `nextCursor` until all pages are read. If either tool fails or returns malformed records, report the tool failure and stop; do not repair coordinator-owned discovery artifacts, reopen discovery, or silently drop candidates. |
There was a problem hiding this comment.
When an app caller uses the documented direct start_codex_security_deep_scan({ scanId, handoffClaimToken }) route without first calling get_codex_security_scan_context, discovery succeeds but this centralized tail cannot read the candidate set. The bundled start handler passes the token to begin-deep-scan but never adds it to authenticatedArtifactClaims, while list_codex_security_candidates has no handoffClaimToken input and scanContext(..., requireClaim: true) therefore rejects the claimed scan. Record the authenticated claim during the Deep start call, as the headless Standard launcher does, or allow the token on the downstream candidate tools.
Useful? React with 👍 / 👎.
| - For every reportable finding, run `$codex-security:vulnerability-writeup` with exactly one dedicated write-up sub-agent, write `findings/<slug>/<slug>.md` plus any `findings/<slug>/poc/` files, verify the report exists, and set the safe relative `writeup.reportPath`. | ||
| - After every write-up is ready, run `$codex-security:propose-security-hardening` once over the complete finding collection, write-ups, threat model, coverage, and relevant source; write `hardening/hardening.md`, `hardening/hardening.json`, and any proposals and diagrams below `hardening/`; verify the portfolio is a regular file and set `scan.hardening.portfolioPath` to `hardening/hardening.md`. Skip this step when there are no reportable findings. | ||
| 7. Verify on disk that `scan-manifest.json`, `findings.json`, and `coverage.json` exist at the workflow-owned scan path, then complete the scan once by calling `complete_codex_security_scan({ scanId })` so the workbench validates and seals the contract, generates `report.md`, and indexes findings. Do not call completion before those files exist. | ||
| 4. Run `$codex-security:validation` once in compact standard-scan mode over the canonical merged candidates, recording every result with `record_codex_security_candidate_validations`. |
There was a problem hiding this comment.
If a Standard or Deep tail skips the validation call, recordCodexSecurityCandidateAttackPaths treats every candidate as ineligible because isAttackPathEligible returns false when validation is absent, so an empty attackPaths array succeeds instead of reporting that validation is missing. record_codex_security_scan_draft and finalization do not reconcile the draft against candidate_ledger.jsonl, which means the caller can then submit complete coverage with no findings and successfully finish despite never deciding any discovered candidate. Require every ledger row to have a validation before accepting the attack-path phase or the final semantic draft.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: 0a7f67ced5
Only the user who started this review can view the report in Codex.
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
| "completeness": { | ||
| "description": "Use partial if any work is deferred or any surface needs follow-up; use complete only when no such work remains.", | ||
| "enum": [ | ||
| "complete", | ||
| "partial", | ||
| "unknown" | ||
| ] | ||
| }, | ||
| "inventoryStrategy": false, | ||
| "surfaces": { | ||
| "type": "array", | ||
| "items": { | ||
| "$ref": "#/$defs/surface" | ||
| } | ||
| }, |
There was a problem hiding this comment.
The compact Standard/Deep draft schema lets a scan submit coverage.completeness: "complete" with surfaces: [], deferred: [], and no receipt or inventory accounting. The bundled runtime only rejects complete coverage when deferred work or needs_follow_up surfaces are present, and finalization then seals the bundle as complete. A skipped or prompt-influenced scan can therefore produce a passing no-findings report without reviewing the prepared inventory. Gate complete drafts on workbench-owned inventory/progress coverage before completion.
Useful? React with 👍 / 👎.
1 participant
Summary
Stack
mdangelo/codex/repair-deep-scan-schemaso this review contains only its atomic upstream change.Verification