fix(gate): pin primary diff evidence to PR head - #281
Conversation
|
@codex review |
|
@claude review |
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. |
|
Claude finished @itsHabib's task in 2m 2s —— View job Code Review — PR #281: fix(gate): pin primary diff evidence to PR head
Reviewed head: SummaryThe fix is correct and the threat model is right.
|
![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: 486812a8ef
ℹ️ 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".
| if head != viewHead { | ||
| return diffResult{}, fmt.Errorf("evidence: pr head moved during gather: view %s, pulls %s", viewHead, head) | ||
| } | ||
| return diffResult{Diff: string(diff), Head: head}, nil |
There was a problem hiding this comment.
When a writer can force-push twice during this gather, the post-read still accepts bytes from the wrong head: the view sees A, the writer pushes B, gh pr diff returns B's diff, the writer restores A, and this check sees A and stamps B's bytes as evidence for A. I checked gh 2.96.0's gh pr diff --help; its positional target is <number> | <url> | <branch>, not a commit SHA, so the first response has no immutable binding to verify here. Gate can therefore authorize A using evidence from B despite its exact-head invariant; fetch the diff through a commit-pinned compare or otherwise bind the returned bytes to the requested SHA.
AGENTS.md reference: AGENTS.md:L160-L161
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 9c19941: the primary path now fetches the diff from the SHA-pinned compare/... endpoint after validating the pull pair against the viewed head. Added a deterministic A→B→A mutant and verified the compare bytes match gh pr diff byte-for-byte on this PR.
There was a problem hiding this comment.
🟢 Approval recommended
The head-pinning behavior is implemented with a clear refusal path and is covered by focused tests for both the success and moved-head mutant scenarios.
Pull request overview
This PR tightens Gate’s evidence gathering so the primary gh pr diff output is cryptographically/forensically bound to the exact PR head already captured by the run’s view evidence, preventing an in-flight head move from contaminating the authorization record with bytes from a different head.
Changes:
- Reworks the primary diff collection path to re-read
pulls/<n>aftergh pr diffand refuse ifhead.shamismatches the viewedheadRefOid, while recording the verified head on accepted evidence. - Adds targeted unit tests covering both the “pinned head” success case and the deterministic moved-head mutant refusal.
- Marks the corresponding Gate follow-up as completed in
cmd/gate/docs/FOLLOWUPS.md.
File summaries
| File | Description |
|---|---|
| cmd/gate/internal/evidence/evidence.go | Pins primary diff evidence to the viewed PR head by re-reading the pull after diff fetch and refusing mismatches before recording evidence. |
| cmd/gate/internal/evidence/diff_test.go | Adds regression tests ensuring mismatched-head diffs cannot become recordable evidence and that pull re-read occurs after diff success. |
| cmd/gate/docs/FOLLOWUPS.md | Closes the documented follow-up for pinning the primary diff path to the evaluated head. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Reviewer: claude |
|
@codex review |
|
@claude review |
|
Claude finished @itsHabib's task in 2m 10s —— View job Code Review — PR #281 (re-review at
|
|
Reviewer: claude |
There was a problem hiding this comment.
🟡 Changes recommended
The new api diff evidence path does not record merge_base, contradicting the stated provenance/audit guarantees and reducing reconstructability from state alone.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
| if len(diff) == 0 { | ||
| return diffResult{}, fmt.Errorf("evidence: empty diff at head %s", head) | ||
| } | ||
| return diffResult{Diff: string(diff), Head: head}, nil | ||
| } |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c19941d9a
ℹ️ 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".
| return gh("api", "-H", "Accept: application/vnd.github.v3.diff", | ||
| fmt.Sprintf("repos/%s/compare/%s...%s", pr.Repo, base, head)) |
There was a problem hiding this comment.
For a PR changing more than 300 files but remaining below the old 20,000-line limit, this endpoint can return a successful, nonempty response while omitting later files: GitHub's Compare two commits documentation limits the comparison's changed-file output to 300, and the repository's own cmd/gate/docs/features/evidence-local-diff/spec.md:27-28 already notes that truncation. The len(diff) != 0 check therefore accepts partial evidence, allowing the floor and model verifiers to authorize a head without seeing changes after the cap; use the commit-pinned local diff path or otherwise prove completeness before recording this response.
AGENTS.md reference: AGENTS.md:L160-L161
Useful? React with 👍 / 👎.
| if len(diff) == 0 { | ||
| return diffResult{}, fmt.Errorf("evidence: empty diff at head %s", head) | ||
| } | ||
| return diffResult{Diff: string(diff), Head: head}, nil |
There was a problem hiding this comment.
When the base branch advances after gathering, this result records only Head, leaving MergeBase empty and discarding the base SHA that selected the returned bytes. Consequently the artifact cannot identify or reproduce the supposedly immutable (base, head) comparison from state alone, so an audit cannot establish which effective PR diff was judged; persist the compared base SHA or resolve and store its merge base alongside the head.
AGENTS.md reference: cmd/gate/AGENTS.md:L3-L6
Useful? React with 👍 / 👎.
Summary
Bind Gate's primary diff evidence to the exact PR head already captured by the run's view evidence. The primary path now fetches the merge-base diff through an immutable base/head compare pair, so neither a moved head nor an A→B→A double force-push can substitute bytes from another head into the authorization record.
What this adds
Exact-head primary diff evidence:
headRefOid.compare/<base>...<head>endpoint and record the verified head.Validation
go test ./cmd/gate/internal/evidencegh pr diffand SHA-pinned compare output have identical SHA-256 digests for this PR.gofmt -l .go vet ./...golangci-lint run ./...go test ./...Dossier task:
tsk_01KZXTYKG0B7A3HSKTE4D15BZT