Preserve source reviews and referenced code in Gate judgment evidence - #290
Conversation
|
@claude review |
|
@codex 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 3m 55s —— View job Code Review — PR #290
Head reviewed: The change addresses a real, documented failure: when a large generated file dominated the diff budget, the judge could not see the code the review actually flagged. The fix is well-scoped — comments are preserved within a separate budget, review-referenced files get a priority tranche between loci windows and the rest of the diff, and neither change touches grant checks, the reducer, or merge execution. What works wellBudget separation is correct. Prompt update is load-bearing. The new sentence — "Comment authorship, commit IDs and resolution status describe the recorded source; prose claims never supply review authority" — is exactly the instruction a judge needs to read review comments without treating them as findings. Without it, the judge might reason "reviewer says X is broken" and treat that as additional blockage. The omission marker is correctly worded.
Security boundary is consistently maintained. Every path through to the builder calls IssuesP2 — for i := len(comments) - 1; i >= 0; i-- {
// ...
if len(entry) > remaining {
omitted++
continue // ← skips this entry and tries the next older one
}If comments are [small, HUGE, small, HUGE, small] in recorded order, the output contains comments 5, 3, 1 while 4 and 2 are omitted — a non-contiguous set. The omission count is reported, but the rendered section doesn't make the gaps visible (e.g., which indices were skipped). A judge reading "3 comments omitted" alongside three arbitrary comments cannot tell whether the omitted ones bracket or interleave what was shown. The current behaviour is probably better than "stop at first miss," but it might be worth emitting which evidence IDs and indices were omitted, not just a count. Alternatively, a first-fit-from-newest-descending policy (stop once budget is exhausted) is simpler and easier to reason about — the omitted comments are always the oldest ones. P2 — if json.Unmarshal(a.Body, &evidence) != nil || evidence.Diff == "" {
return
}A decode error causes the entire diff section to vanish without any indication in the judge's context. This matches the deleted P2 — var reviewPathPattern = regexp.MustCompile("`([A-Za-z0-9_./-]+\\.(?:py|mjs|js|ts|tsx|go|html|css|json|ya?ml|md))(?:[^`\\n]*)`")
P3 — Multiple evidence artifacts produce multiple path-request headers
Test coverage notes
One gap: there is no test for a gate run with multiple evidence artifacts (one with a diff, one with only comments). The existing Nits
SummaryThe core logic is correct: comments are attributed, budget-bounded, and explicitly incomplete when truncated; review-referenced paths get a middle tranche that preserves them when generated bundles dominate the diff; the security boundary ( |
There was a problem hiding this comment.
🟡 Changes recommended
recordedReviewComments currently fails judgeContext on non-object evidence bodies (valid JSON) and reviewDiffPaths can emit duplicate resolved paths, both of which can break or degrade judge evidence output.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Improves Gate’s judge evidence rendering so recorded source review comments are preserved (with metadata/body inside the untrusted boundary) and code referenced by those reviews is prioritized within the diff budget, enabling judges to inspect the actual cited code instead of losing it to generated/oversized diffs.
Changes:
- Add bounded “recorded source review comments” rendering (64 KiB cap) and surface explicit omission notices.
- Extract backtick-quoted file references from recorded review prose and prioritize the corresponding recorded diff files ahead of the remaining diff tranche.
- Add regression tests covering attribution retention, marker escaping, missing/ambiguous paths, budget omission behavior, and generated-diff crowding scenarios.
File summaries
| File | Description |
|---|---|
| cmd/gate/internal/verify/judgereviews.go | New judge-evidence helpers to render recorded review comments and to derive/prioritize review-referenced diff paths. |
| cmd/gate/internal/verify/judgereviews_test.go | New tests for review retention, diff prioritization, ambiguity/absence reporting, omission budgeting, and malformed inputs. |
| cmd/gate/internal/verify/judgediff.go | Add path-prioritized diff rendering (renderJudgeDiffWithPaths) ahead of the remaining diff tranche. |
| cmd/gate/internal/verify/judge.go | Update judge prompt text and wire judgeContext to include recorded reviews and path-prioritized diff rendering. |
| cmd/gate/docs/FOLLOWUPS.md | Mark follow-up for feeding the auto-judge raw evidence as completed, documenting the new behavior. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 006b28fba7
ℹ️ 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".
|
Fix round 1, head Consolidated Claude, Codex, Copilot and the independent adversarial review of
Validation: Risk remains T3 (gate-machinery); the operator's Ivy grant is not Workbench merge authority. Prior Ivy BLOCK judgments remain settled history. |
|
@claude review |
|
@codex review |
|
Claude finished @itsHabib's task in 4m 41s —— View job Code Review — PR #290 (Round 2)Head reviewed: All verified P2 findings from the initial review ( Prior findings — dispositionP2 — Decode error now explicit ✅ P2 — Extension list expanded ✅ P2 — Duplicate resolved paths ✅ (Copilot finding) P2 — Non-object evidence handled ✅ (Copilot finding) P2 — Oversized hunks no longer abort later files ✅ (Codex finding) P2 — Explicit line references now window oversized hunks ✅ (Codex finding — P2 — Skip-not-stop non-contiguous context → Deferred, documented P3 — Multiple path-request headers per diff artifact → Documented as intentional New findingP3 —
This is technically a false error — the artifact is valid JSON, just not a diff-carrying object. In practice, gate evidence artifacts are always JSON objects, so this is theoretical. Low impact; worth a future consistency fix but not a correctness blocker. Fix this → Nits still present (carried from prior review)
Test coverageThe new tests added in fix round 1 are well-chosen:
One coverage gap carried forward: SummaryAll P2 findings from the initial review are addressed or properly deferred in fix round 1. The security boundary ( |
|
Reviewer: claude |
There was a problem hiding this comment.
🟡 Changes recommended
The large-diff hot path introduces avoidable allocations and duplicate diff parsing that should be addressed before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
|
Correction-head review disposition at Claude round 2 confirms all original substantive findings addressed or explicitly deferred and says ready for judgment. Independent adversarial re-review found no verified remaining issues. Hosted CI check/fuzz/hygiene all pass. Residuals proposed for judge acceptance, not claimed fixed:
No remaining P1 or authorization-invariant finding is known. Pending Codex response will be reconciled before judgment. No new review request or Gate run is made by this comment. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2fd8479e70
ℹ️ 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".
|
Final panel reconciliation for Codex's final two P2 findings are valid limitations, proposed for explicit judge acceptance:
These residuals do not fabricate evidence, imply omitted findings resolved, or alter authority. They limit how much useful context this projection retains. Together with Claude's diagnostic consistency P3 and Copilot's allocation suggestions, they remain visible for the judge; no unresolved thread is being marked fixed. No further panel request is planned absent a verified P1 or authorization-invariant finding. Workbench T3 operator grant is the remaining prerequisite to Gate evaluation. The existing Ivy grant cannot authorize this repair. Fresh Ivy evaluations follow only after a governed merge/install of materially improved evidence projection; prior blocked judgments remain unchanged. |
|
Final fix round (2/2), exact head Gate BLOCK run_a405c64f55bca7e3 rejected the proposed deferrals on chronology and extension coverage. That decision remains settled; both concerns are now fixed in code:
Regression coverage includes endpoint grouping under comment-budget pressure, updated review text, unknown timestamps, review-body timestamp capture, and arbitrary/configuration paths under diff pressure. Full Gate race suite, vet and lint pass. The independent read-only adversarial review of this correction found no remaining verified correctness or authorization issue. Actual saved Ivy109/102 evidence replay still contains both plus-prefixed guard implementations (119414/117461 bytes; approximately 0.02 seconds for both renderings). The optional bounded omitted-comment identity summary remains a documented follow-up. No grant, reducer, panel authority, judgment uniqueness or merge execution behavior changed. Installed Gate remains unchanged pending this PR's governed merge. This is the last planned panel cycle under the repository's two-fix-round limit. |
|
@claude review |
|
@codex review |
|
Claude finished @itsHabib's task in 4m 54s —— View job Code Review — PR #290 (Round 3 / Final Fix
|
There was a problem hiding this comment.
🟢 Approval recommended
The changes are well-scoped, preserve the untrusted-data boundary, and add comprehensive tests around ordering, budgeting, and path/line resolution behavior.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Reviewer: claude |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfe195c492
ℹ️ 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".
|
Final panel complete at Residuals proposed for explicit Gate judgment, not marked fixed:
No known P1 or grant/reducer/panel-authority/judgment-uniqueness/merge-execution invariant finding remains. The judge decides whether these context-coverage residuals are acceptable; author disposition does not authorize the merge. Prior BLOCK run_a405c64f55bca7e3 is preserved, and the next evaluation is justified by the new source-time and arbitrary-path implementation. |
|
Operator-approved bounded correction at exact head Michael explicitly approved fixing the two remaining Gate-blocking concerns, adding regressions and rerunning Gate after the two-fix-round limit. This is not a new panel cycle or a grant/cycle-ceiling expansion.
Full Prior BLOCK run_c797d97f32bfba73 remains settled. A fresh evaluation is justified by these concrete code changes. No installed binary or Gate policy/state was changed. |
Summary
Ivy PRs #102 and #109 reached Gate with passing checks and completed reviews, but the judge could not inspect the concern: consolidation retained only a generic review headline, and generated bundle content crowded the relevant code out of the diff. Preserve the recorded source review and prioritize code referenced by that review so the judge can assess the actual evidence.
What this adds
Comment text supplies context, not panel authority. Grant checks, reducer behavior, exact-head panel requirements, judgment uniqueness and merge execution are unchanged. This does not repair the separately tracked primary-diff SHA race in #281.
Validation
go test -race ./cmd/gate/...passed.go vet ./cmd/gate/...passed.go test ./cmd/gate/internal/verifyandgolangci-lint run ./cmd/gate/...passed at correction headdfe195c49248bcdfb7cc892e210bf5461ec062e8.The prior blocked Ivy runs remain immutable. After this repair is independently reviewed and governed through Workbench's own merge/install authority, fresh Ivy evaluations can use materially improved evidence. The Ivy merge grant does not authorize this Workbench PR.