fix(contract): adopt the registered target kind when sealing a draft - #238
Open
rohanpoudel2 wants to merge 3 commits into
Open
fix(contract): adopt the registered target kind when sealing a draft#238rohanpoudel2 wants to merge 3 commits into
rohanpoudel2 wants to merge 3 commits into
Conversation
The scan prompt tells the agent to copy CODEX_SECURITY_TARGET_KIND verbatim instead of inferring scan.target.kind from the checkout, because only the workbench knows how the target was registered. A draft that inferred it anyway failed _validate_completion_binding with "scan.target.kind: must match the workbench target", which discarded a scan whose analysis had already finished and whose findings, coverage, and manifest were complete on disk. A clean worktree is the ordinary way to reach this. The workbench registers it as git_revision, since a worktree with no uncommitted changes is content- identical to its revision, while the checkout still looks like a worktree to the agent. Bulk-scan checkouts are always clean, so any campaign could hit it. scan.target.kind carries no draft-owned information when the registration allows a single kind, so take the registered kind during draft population rather than rejecting the scan. This runs only for unsealed drafts, next to the coordinate replacement that already treats workbench-owned target fields as authoritative, so the pruning there now sees the registered kind and drops coordinates that kind must not carry. Sealed scans keep verifying unchanged. Fixes openai#62 Fixes openai#50
Collaborator
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a6525bd13
ℹ️ 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".
|
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. |
Adopting the registered target kind also reinterprets the coordinates that kind requires, and the pruning beside it keeps a coordinate the kind requires even when the completion binding does not carry one. The draft's own value then survives under the new kind's meaning. Commit and range diffs are the reachable case. The workbench registers them as git_diff and authors only base and head revisions, never a snapshotDigest, so a draft that labelled itself git_worktree or directory_snapshot had the digest it computed for whole-worktree or directory contents sealed as the diff digest instead of the mismatch being reported. Take the registered kind only when the binding supplies every coordinate that kind requires. That still normalizes the clean worktree this change was written for, where the binding owns the revision behind git_revision, along with dirty worktrees, directory snapshots, and working-tree diffs, whose registrations all author the snapshot digest. Every other kind change keeps the draft value so _validate_completion_binding reports the mismatch it already describes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #62
Fixes #50
Problem
scan.target.kindis a workbench-owned field that the agent transcribes.api.tsderives it from the registration contract and passes it in the environment:and the scan prompt is explicit about it:
Nothing enforces that instruction. A draft that infers the kind from the checkout instead reaches
_validate_completion_bindinginfinalize_scan_contract.py:That rejection lands at the seal step — after the analysis is finished. Both reports describe the same outcome:
findings.json,coverage.jsonandscan-manifest.jsonall complete on disk, ~13 minutes of analysis in #50, and the scan discarded, left unsealed, and recorded asfailed.A clean worktree is the ordinary way to reach it.
expected_target_kindsinworkbench_db.pytreats a worktree with no uncommitted changes as its revision, which is content-identical:clean_worktree_content_digest()is a sentinel — the digest of an empty tracked-diff — so this branch means exactly "nothing uncommitted". The workbench therefore registersgit_revision, while the checkout still looks like a worktree to the agent. #62 reaches it through a bulk-scan checkout, which is clean by construction (git init+git fetch --depth=1+git checkout --detach), so any campaign can hit it. #50 reaches the same branch through a path-scoped scan of a clean worktree.Change
Take the registered kind during draft population instead of rejecting the scan.
scan.target.kindcarries no draft-owned information when the registration allows a single kind, so there is exactly one correct value and the workbench is holding it.This goes next to the coordinate replacement that already treats workbench-owned target fields as authoritative, and reuses its ordering: the kind is adopted first, so the existing pruning sees the registered kind and drops coordinates that kind must not carry. In the clean-worktree case that means a
snapshotDigestthe draft invented for its inferredgit_worktreeis removed, andrevisionfrom the binding is applied.It runs only under the existing
if not was_sealed:guard in_prepare_scan_finalization, so already-sealed scans keep verifying byte-for-byte and this cannot rewrite sealed content.Why not widen the allowed kinds, as #62 suggests
#62 proposes returning both kinds from
expected_target_kinds:By inspection that breaks registration before the scan starts.
api.tsderives the kind it hands the agent from a single-element list and treats anything else as unusable:→
throw new CodexSecurityError("The Codex Security workbench returned an invalid scan registration.")So a two-element list turns a scan that currently fails at the seal into one that fails at registration. Making that path work needs a matching
api.tschange, and it would still leave the kind to the agent's discretion — a draft could name a third kind and be rejected the same way. Adopting the registered value removes the failure mode instead of narrowing it.verify_manifest_bindinginworkbench_db.pyalso gates on the same list, and the digest check two branches below it keys offkind in {"directory_snapshot", "git_worktree"}, so widening the list would additionally require the clean sentinel to be threaded through that check. Adopting the kind needs none of that.Why normalize rather than reject
finalize_scan_contract.pyalready reconciles agent-authored fields against workbench expectations on the unsealed path rather than failing —_normalize_unsealed_deep_repository_inventory_strategyrewrites a stale coverage label,_normalize_unsealed_open_questionsdrops unusable rows, and_populate_unsealed_target_bindingreplaces target coordinates outright. This follows that established convention for the one target field it had been left out of.What still fails is unchanged: a draft whose
revisionorsnapshotDigestdisagrees with the workbench is still rejected by the coordinate comparison in_validate_completion_binding,_validate_targetstill enforces the coordinate the adopted kind requires, and a genuinely wrong draft now fails on the coordinate rather than the kind — a more specific message, not a weaker check.Impact, stated plainly
This does not change what a sealed scan asserts. It changes which of two truthful descriptions of the same target ends up in the manifest, and it does so by preferring the one the registration recorded over the one the agent guessed. The recovered value is that a completed analysis is no longer thrown away over a field the agent was never meant to decide.
Scans that already seal today are unaffected: when the draft kind already equals the registered kind the helper returns immediately.
Verification
Added a test to
scan-recovery.test.tsthat drives the reported scenario through the real workbench — a clean-worktree draft whose manifest claimsgit_worktreewith an inventedsnapshotDigest— and asserts it seals asgit_revision, carries the trueHEADrevision, and has the stale digest pruned.With the fix reverted, that test fails with the exact message from both issues:
With the fix,
scan-recovery.test.tsis 17 pass / 0 fail.Full suite on this base: 727 pass / 5 skip / 0 fail.
pnpm run typesandpnpm run formatare clean, andfinalize_scan_contract.pycompiles underpython3 -m py_compile.