fix(workbench): record a content digest for committed diff targets - #241
fix(workbench): record a content digest for committed diff targets#241rohanpoudel2 wants to merge 3 commits into
Conversation
The contract requires a git_diff target to carry scan.target.snapshotDigest, both in scan-manifest.schema.json and in _validate_target. The workbench only computed one for working-tree diffs: require_diff_target returned no contentDigest for commit or range kinds, and register_cli_scan set one only when the requested target was working_tree. So a committed diff had no digest to record. CODEX_SECURITY_TARGET_SNAPSHOT_DIGEST was left unset, and the scan prompt only says to copy that value when it is set, which left the agent to invent a value the contract demands. A draft that omitted it failed at the seal step with "scan.target.snapshotDigest: expected a non-empty string" after the whole analysis had run, which is 90 minutes in the report. Compute the digest for commit and range targets the same way working-tree diffs already do, over git diff base head, using the same format label and field framing as worktree_content_digest. The value then flows through registration into the completion binding, so draft population fills it in and the agent no longer decides it. Extend the completion binding and manifest verification to any diff kind that recorded a digest rather than working_tree alone, so the field is verified instead of merely present. Scans registered before this have no digest and keep omitting it. Fixes openai#53
|
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. |
![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: 0bb5d75aab
ℹ️ 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".
| "kind": kind, | ||
| "baseRevision": base, | ||
| "headRevision": head, | ||
| "contentDigest": committed_diff_content_digest(target, base, head), |
There was a problem hiding this comment.
When a saved range is revalidated at scan start, content_digest contains the digest recorded during selection, but this branch ignores it and silently returns a newly computed value. The same revisions can produce different reviewed bytes if mutable Git state changes—for example, a replace ref changes the objects Git reads, or diff.renames changes the generated patch—so the scan may review a different snapshot without asking the user to reselect it. Compare a supplied digest with the computed value and fail on mismatch, as the working-tree branch already does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Note
To use Codex here, create a Codex account and connect to github.
| "kind": kind, | ||
| "baseRevision": parent, | ||
| "headRevision": head, | ||
| "contentDigest": committed_diff_content_digest(target, parent, head), |
There was a problem hiding this comment.
When the selected commit is the root commit of a SHA-256 repository, parent is the hard-coded SHA-1 empty-tree ID 4b825d...; the new digest call then passes that nonexistent object to git diff and exits with “bad revision,” so the root commit cannot be selected for review. Derive the empty-tree object ID using the repository's object format instead of passing the SHA-1 constant into this call.
Useful? React with 👍 / 👎.
| """ | ||
|
|
||
| repository, pathspec = git_worktree_context(target) | ||
| tracked = git_bytes( |
There was a problem hiding this comment.
For a committed range containing a large changed binary, git_bytes captures the entire git diff --binary output in memory before hashing it. Binary patches can be substantially larger than the underlying files, so merely registering or starting such a scan can exhaust the workbench process's memory before review begins; stream stdout incrementally into the digest rather than materializing the full patch.
Useful? React with 👍 / 👎.
…y tree Selecting a root commit resolved its base to the hard-coded SHA-1 empty-tree id. That object does not exist in a SHA-256 repository, so the new content digest ran `git diff` against an unknown revision and failed with "Could not snapshot the selected committed changes.", leaving the root commit unselectable. Look the empty tree up by the repository's object format instead. Git before 2.29 cannot report a format and only ever wrote SHA-1, so the SHA-1 constant stays the fallback, and SHA-1 repositories keep the same base revision and byte-identical digests. Revalidating a saved commit or range also discarded the digest recorded when the selection was made and returned a freshly computed one. Commit ids pin the revisions but not the bytes Git reports for them: a replace ref substitutes the objects Git reads, and diff configuration changes the generated patch, so a scan could review content nobody selected. Compare a supplied digest against the computed one and ask for a new selection on mismatch, the way the working-tree branch already does. Selections saved before the workbench recorded digests carry none and still revalidate.
2 participants
Fixes #53
Problem
The contract requires a
git_difftarget to carrysnapshotDigest.scan-manifest.schema.json:{ "if": { "properties": { "kind": { "enum": ["git_worktree", "git_diff", "directory_snapshot"] } } }, "then": { "required": ["snapshotDigest"] } }_validate_targetenforces the same, andreferences/scan-contract.mdstates it in the table of required coordinates, adding that for diffs the digest is calculated "from a deterministic representation of the reviewed content".The workbench only ever computed one for working-tree diffs.
require_diff_targetreturns nocontentDigestfor thecommitorrangekinds:and
register_cli_scanset one only when the requested target wasworking_tree:So a committed diff had no digest anywhere.
workbench_completion_bindinggated onworking_tree,api.tsreadsdiffTarget["contentDigest"]and gotundefined, soCODEX_SECURITY_TARGET_SNAPSHOT_DIGESTwas never exported. The only instruction the agent has is conditional:With the variable unset and
git_revisionnot applicable, the agent was left to invent a value the contract demands. A draft that omitted it failed at the seal step, after the whole analysis had run — 90 minutes in the report — with:--diff origin/main --head HEADis arangetarget, so this is the ordinary committed-diff path, not an edge case.Worth noting the field was also unverifiable where it was optional.
verify_manifest_bindingcomparedsnapshotDigestonly whendiff_target_kind == "working_tree", so any pattern-valid string an agent invented for a committed range passed unchecked.Change
Compute the digest for
commitandrangetargets the same way working-tree diffs already do — overgit diff base headwith the same flags, format label and field framing asworktree_content_digest, minus the untracked-file component, which has no meaning for committed revisions.The value then flows through registration into the completion binding, so
_populate_unsealed_target_bindingfills the field in during draft population and the agent no longer decides it. That is the same pathgit_worktreedigests already take, and it means a draft that omitssnapshotDigestis corrected rather than rejected.Completion binding and manifest verification are extended to any diff kind that recorded a digest, rather than
working_treealone, so the field is now verified instead of merely present.On redundancy, stated plainly
A
base..headpair of resolved 40-character SHAs already pins the reviewed content, so this digest is strictly redundant as an integrity anchor. The alternative fix is to relax the schema sogit_diffrequiresbaseRevision/headRevisionand only working-tree diffs requiresnapshotDigest.I did not take that route because it changes a published contract at
schemaVersion1.0, and manifests produced under a relaxed rule would be rejected by any SDK that predates the change. Computing the digest keeps the contract exactly as documented and as already validated on both sides, and it converts a field the agent was guessing into one the workbench owns and checks. If you would rather relax the schema, the change is small and I am happy to redo it that way.Compatibility
diff_content_digestis already a nullable column, so no migration is involved. Scans registered before this change have no digest: the completion binding keeps omittingsnapshotDigestfor them and verification keeps skipping it, exactly as today. Only newly registered committed diffs gain the field.Cost is one
git diff base headat registration, the same shape of workworktree_content_digestalready does againstHEAD.Verification
Added a test to
scan-recovery.test.tsthat drives the real workbench end to end: it builds a two-commit repository, registers arefsdiff scan for the committed range, and drafts exactly what the report describes — agit_difftarget withbaseRevisionandheadRevisionbut nosnapshotDigest. It asserts the registration recorded a well-formed content digest, that the scan seals, and that the sealed manifest carries that digest along with the true base and head.With the fix reverted, the registration reports no digest and the test fails:
Driving the same draft through
complete-scanagainst the unfixed source reproduces the reported message directly:and with the fix the same draft seals, with
snapshotDigestpopulated from the registration:The existing working-tree diff coverage is unchanged, so the digest path that already worked is not disturbed.
Full suite on this base: 727 pass / 5 skip / 0 fail.
pnpm run typesandpnpm run formatare clean, and both changed Python modules compile underpython3 -m py_compile.