Repository navigation
fix(coverage): bind Standard completion to file review receipts - #829
soyeon-oai wants to merge 3 commits into
Conversation
mldangelo-oai
left a comment
There was a problem hiding this comment.
AI-assisted review, reviewed by me. Most feedback is suggestions; concrete behavior issues are called out where applicable.
Reviewed 77cc515cc04ea1b1f4e3fde07dba0556fc8c57b7. Freezing the Standard inventory and deriving completion from content-bound file receipts gives the completion gate a concrete source of truth. Transactional receipt batches, duplicate handling, and the distinction between progress display and authoritative completion are valuable parts of this design. One introduced filename-transport issue is inline.
The change's explicit compatibility boundary is reasonable: an older in-flight Standard scan lacks the frozen inventory needed by this contract and must restart, while completed history remains readable. Keep that restart guidance close to the actual recovery error. The gate records review completion against the frozen bytes; it should continue to describe that contract without implying it establishes the quality of model reasoning.
One simplification suggestion is to remove the new 512-receipt batch cap unless a concrete transport or storage limit requires it. The transactional implementation already provides the important integrity behavior; an unexplained count adds coordination work for the caller. Retain the atomic rejection, duplicate/idempotent receipt, and source-drift tests while simplifying that surface.
Validation passed 39 focused Python tests, three source/shipped MCP test scripts, 58 SDK receipt/completion/recovery tests, type/build checks, Ruff, and the historical portable-source checker and its nine tests. The filename issue was reproduced through the shipped stdio MCP server and real Python helper/database; the base completed the same synthetic target, and the equals-form argument closed its receipt. Deep parent and worker completion paths were traced, including resumed workers. No live model, external service, rendered desktop UI, or native Windows execution was used.
| ...optionalNumberArg("--phase-items-completed", derivedPhaseItemsCompleted), | ||
| ...optionalArg("--phase-progress-unit", derivedPhaseProgressUnit), | ||
| ...(serializedPreflightIssues ? ["--preflight-issues-json-stdin"] : []), | ||
| ...(reviewedFiles ?? []).flatMap((path) => ["--reviewed-file", path]), |
There was a problem hiding this comment.
[P2] Preserve leading-dash filenames when forwarding receipts
For an inventoried top-level file named -source.py, this emits separate arguments --reviewed-file and -source.py. Python argparse treats the filename as another option and reports that --reviewed-file needs an argument. Through the shipped MCP server, the receipt stays at 0/1 and the new mandatory gate prevents validation and completion; prefixing ./ is rejected by canonical-path validation. The same synthetic target completes at the base revision. Forward each value as --reviewed-file=<path>: that control closes the receipt without changing its canonical identity. Add a shipped MCP/helper regression so a valid repository filename cannot make Standard completion impossible.
Summary
Standard scans could finish with completed findings while their persisted review count remained zero. Bind review progress to the files selected at scan start, and require every file's review receipt before leaving discovery or sealing the result.
Changes
reviewedFilesand the workbench helper's repeatable--reviewed-fileargument. Validate their content, persist receipts transactionally, and derive coverage totals from those rows. Standard scans reject aggregate review counters.Testing
1/1/1coverage locally. Fresh npm installation remains locally blocked: registry requests returned HTTP 403, followed by npm'sExit handler never called!error.Risk and rollout
In-flight Standard scans created before this migration need to restart because they have no frozen per-file inventory. Existing completed scans remain readable. Receipts record declared review completion for frozen file content; they do not attest to the quality of model reasoning.
Native Desktop and live-model CLI scans of this OSS revision remain follow-up QA. This PR does not publish a release.
Public disclosure review
Change impact
The workbench freezes the file list, validates declared reviews against those bytes, and blocks real scan completion while receipts remain open.
flowchart LR subgraph column_0["Inputs"] direction TB node_0["Frozen file inventory"] node_1["MCP reviewedFiles"] end subgraph column_1["Persisted reviews"] direction TB node_2["Content-bound receipts"] end subgraph column_2["Completion rule"] direction TB node_3["All file reviews closed"] end subgraph column_3["Consumers"] direction TB node_4["Leave discovery"] node_5["Prepare or complete scan"] end node_0 -->|"binds paths and hashes"| node_2 node_1 -->|"records paths"| node_2 node_2 -->|"supplies closed rows"| node_3 node_3 -->|"permits transition"| node_4 node_3 -->|"permits finalization"| node_5 class node_0 changed class node_1 changed class node_2 changed class node_3 changed class node_4 affected class node_5 affected classDef changed fill:#d7f5e5,stroke:#237a4b,color:#111 classDef affected fill:#e6f0ff,stroke:#3569a8,color:#111 classDef context fill:#f2f3f5,stroke:#6e7781,color:#111Source evidence (6)
plugins/codex-security/scripts/workbench_scan_start.py:L217-L249— Startup freezes unique paths and SHA-256 digests, preserving direct file scopes and deriving the denominator from those rows.plugins/codex-security/mcp-app/server.ts:L1090-L1107— The progress tool forwards reviewedFiles to the workbench as repeated --reviewed-file arguments.plugins/codex-security/scripts/workbench_progress.py:L62-L116— The receipt writer checks persisted membership, canonical scope, containment, and content digest, then counts stored rows and closed timestamps.plugins/codex-security/scripts/workbench_progress.py:L25-L59— The shared gate requires a complete inventory with every receipt closed. Only an explicit persisted mock launch recipe skips real review completion.plugins/codex-security/scripts/workbench_progress.py:L400-L411— Forward transitions beyond discovery call the receipt gate before updating the scan phase.plugins/codex-security/scripts/workbench_db.py:L1467-L1477— The shared completion implementation checks receipts before finalization, including SDK preparation.