Skip to content

Preserve source reviews and referenced code in Gate judgment evidence - #290

Merged
itsHabib merged 4 commits into
mainfrom
codex/gate-review-evidence
Sep 9, 2026
Merged

Preserve source reviews and referenced code in Gate judgment evidence#290
itsHabib merged 4 commits into
mainfrom
codex/gate-review-evidence

Conversation

@itsHabib

@itsHabib itsHabib commented Sep 9, 2026

Copy link
Copy Markdown
Owner

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

  • Source comments retain their recorded metadata and body inside the existing untrusted-data boundary. A separate 64 KiB budget keeps whole comments by latest known GitHub source activity across endpoints. Unknown legacy timestamps remain explicit and last; omissions are reported.
  • Backtick-quoted file references, including extensionless/configuration names and custom extensions, select only matching files from the already-recorded diff. Unique suffixes support short filenames; ambiguous and absent paths are reported. Structured loci and explicit prose line references select bounded code windows; remaining referenced hunks precede the rest of the diff within its 48 KiB budget. Oversized hunks do not abandon later requested files. Path diagnostics have a separate 8 KiB cap and explicit omissions.
  • Regression coverage for source attribution, stale/resolved metadata, marker escaping, absent/ambiguous paths, budget omissions, exact extensions, alias deduplication, malformed evidence, and source code behind large generated diffs.

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

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.

@itsHabib

itsHabib commented Sep 9, 2026

Copy link
Copy Markdown
Owner Author

@claude review

@itsHabib

itsHabib commented Sep 9, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-09T06:25:17.282273Z dfe195c Manual request
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@claude

claude Bot commented Sep 9, 2026

Copy link
Copy Markdown

Claude finished @itsHabib's task in 3m 55s —— View job


Code Review — PR #290

  • Gather context and read changed files
  • Review judgereviews.go (new file)
  • Review judgereviews_test.go (new file)
  • Review judgediff.go changes
  • Review judge.go changes
  • Review FOLLOWUPS.md changes
  • Post final review

Head reviewed: 006b28f · Branch: codex/gate-review-evidence

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 well

Budget separation is correct. reviewContextCap (64 KiB for comments) is independent of judgeDiffCap (48 KiB for the diff). They don't compete, and neither can starve the other.

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. "absence is not resolution" in the budget-exhaustion line is the key invariant. A missing comment must not look like a resolved thread to a judge reading the truncated context.

TestMalformedRecordedCommentCannotDisappear is the right test. A malformed comment that silently disappears is worse than an error, because the judge would never know it was absent. Propagating the decode error all the way up to judgeContext is correct.

Security boundary is consistently maintained. Every path through to the builder calls scrub()c.raw in writeRecordedReviews, the path list, the missing-path reason strings, and the rendered diff in writeReviewDiffSection. The artifactsBegin/artifactsEnd markers cannot be forged by comment content.


Issues

P2 — writeRecordedReviews skip-not-stop produces non-contiguous context (judgereviews.go:63–75)

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 — writeReviewDiffSection silently drops the diff on unmarshal failure (judgereviews.go:133)

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 writeDiffSection's behaviour (the prior code did the same), but it's the wrong pattern for a section the judge is supposed to see. A decode error here means the judge gets the review comments section but no diff — which could look like "no diff was provided" rather than "the diff artifact was unreadable." A note to the builder (fmt.Fprintf(b, "[recorded diff unavailable: decode error]\n\n")) would keep the failure visible in the judge's context. Fix this →

P2 — reviewPathPattern extension list is hardcoded and narrow (judgereviews.go:78)

var reviewPathPattern = regexp.MustCompile("`([A-Za-z0-9_./-]+\\.(?:py|mjs|js|ts|tsx|go|html|css|json|ya?ml|md))(?:[^`\\n]*)`")

.rs, .sh, .rb, .java, .c, .cpp, .sql, and others are absent. A review comment that says `migrations/schema.sql` silently produces no path hint. This is probably intentional for now — a narrow allowlist avoids matching arbitrary backtick-quoted non-path strings — but the FOLLOWUPS closure says "unambiguous file references," and this list is more "file references from a specific language subset." A follow-up note in FOLLOWUPS.md would be appropriate if the intent is to expand coverage later.

P3 — Multiple evidence artifacts produce multiple path-request headers

writeReviewDiffSection is called once per KindEvidence artifact. If a gate run records two evidence artifacts (e.g., a diff artifact and a comments artifact, as the test at judgereviews_test.go:45 constructs), the "Review-referenced files requested within the diff budget" line appears once per diff-carrying artifact. The comments artifact's call returns early (evidence.Diff == ""), so in practice there's at most one real output. But if the gate ever records two separate diff artifacts, a judge would see two separate "Review-referenced files" headers. This is low-risk given the current artifact model but worth noting.


Test coverage notes

TestJudgeShowsIssueReviewCodeBehindGeneratedBundle correctly reproduces the original failure mode — renderJudgeDiff(diff, nil) losing the tail code — then shows the fix restores it. The fixture closely mirrors the real Ivy #109 scenario described in the PR.

TestNewestReviewPathsPrecedeOlderOversizedContext would catch a regression where the older review-path hint displaced the newer one. The review ordering in reviewPathHints (reverse iteration, newest first) is important for priority, and this test pins it.

One gap: there is no test for a gate run with multiple evidence artifacts (one with a diff, one with only comments). The existing TestJudgeShowsIssueReviewCodeBehindGeneratedBundle constructs exactly this at line 45 ([]state.Artifact{diffEvidence, commentsEvidence}), so the path is exercised. But the test does not assert that writeReviewDiffSection is NOT called with the comments-only artifact producing a diff section — it only checks that the right content appears. A minor coverage gap, not a correctness bug.


Nits

  • writeRecordedReviews (judgereviews.go:65): allocates the full formatted entry (including the raw JSON body) before checking whether it fits the remaining budget. For a comment that is e.g. 512 KiB, this is a 512 KiB allocation just to discard. In practice comment sizes are bounded and this is on a non-hot path, so it is a style note only. Checking len(c.raw) + estimatedHeaderSize > remaining first would avoid the allocation.

  • resolveReviewPath (judgereviews.go:116): the strings.HasSuffix(f.path, "/"+hint) guard correctly adds a separator so model.go does not match alt_model.go. Worth a short comment because the / is load-bearing and easy to miss.


Summary

The 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 (scrub, untrusted-data markers) is consistently applied; and the judge prompt is updated to tell the model how to read the new section. The P2s above are genuine but none of them are correctness blockers for the primary goal — getting the review body and referenced code in front of the judge.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment thread cmd/gate/internal/verify/judgereviews.go
Comment thread cmd/gate/internal/verify/judgereviews.go
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

Reviewer: claude
Reviewed commit: 006b28fba70e81f16505c122610cff133f77db1a

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread cmd/gate/internal/verify/judgediff.go Outdated
@itsHabib

itsHabib commented Sep 9, 2026

Copy link
Copy Markdown
Owner Author

Fix round 1, head 2fd8479e701eb3f08b8b371fdd7dcf02187377b1.

Consolidated Claude, Codex, Copilot and the independent adversarial review of 006b28f:

  • Fixed exact extension matching, rejected arbitrary suffixes, and expanded the explicit source-extension set.
  • Fixed duplicate resolved paths, preserving unique suffix resolution without guessing ambiguous paths.
  • Oversized requested hunks no longer abort later requested files. Explicit prose line references now select bounded windows, including a guard at line 2001 inside an oversized hunk.
  • Capped review-path diagnostic output separately at 8 KiB with an explicit omitted-entry count. A 20,000-missing-path fixture cannot crowd out substantive code.
  • Valid non-object evidence does not break comment decoding; malformed comments still error. Unreadable diff fields now emit an explicit unavailable marker.
  • Deferred Claude's optional omitted-comment identity summary in FOLLOWUPS.md: retained entries already carry artifact ID and original index, selection order and omission count are explicit, and absence is not resolution. Whole-entry first-fit retains useful smaller reviews rather than abandoning them after one oversized comment. Multiple diff artifacts remain independently attributed; repeated headers are intentional per-artifact context.

Validation: go test -race ./cmd/gate/..., go vet ./cmd/gate/..., golangci-lint run ./cmd/gate/... (0 issues), and git diff --check passed. Offline replay of the recorded Ivy #109 and #102 artifacts shows the actual allocation and authored-lecture overwrite guards in rendered diff evidence (113775 and 112025 bytes respectively). No judge was invoked, Gate state was not edited, and the installed binary remains unchanged.

Risk remains T3 (gate-machinery); the operator's Ivy grant is not Workbench merge authority. Prior Ivy BLOCK judgments remain settled history.

@itsHabib

itsHabib commented Sep 9, 2026

Copy link
Copy Markdown
Owner Author

@claude review

@itsHabib

itsHabib commented Sep 9, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@itsHabib
itsHabib requested a lite review from Copilot September 9, 2026 05:45
@claude

claude Bot commented Sep 9, 2026

Copy link
Copy Markdown

Claude finished @itsHabib's task in 4m 41s —— View job


Code Review — PR #290 (Round 2)

Head reviewed: 2fd8479 · Branch: codex/gate-review-evidence

All verified P2 findings from the initial review (006b28f) are addressed in this fix round. The core logic is correct. Below is the full accounting.


Prior findings — disposition

P2 — Decode error now explicit
writeReviewDiffSection (judgereviews.go:141–144) now emits [recorded diff unavailable (%s): decode error] instead of silently returning. TestUnreadableDiffIsExplicit pins it.

P2 — Extension list expanded
reviewPathPattern (judgereviews.go:82) now includes .rs|sh|rb|java|c|cpp|sql. The updated TestReviewPathTokensAreExactAndDeduplicated asserts exact extension matching (.backup rejected, .tsx vs .ts disambiguated via unique-suffix resolution, course.json vs course.js disambiguated correctly).

P2 — Duplicate resolved paths ✅ (Copilot finding)
The resolved map (judgereviews.go:99–103) deduplicates by the resolved diff path, not the textual hint. A file referenced both as foo.go and dir/foo.go now appears once.

P2 — Non-object evidence handled ✅ (Copilot finding)
recordedReviewComments (judgereviews.go:28–29) skips valid JSON that doesn't begin with {, so arrays, null, and string evidence bodies are silently bypassed. Invalid JSON still errors — malformed comments cannot disappear. TestNonObjectEvidenceDoesNotBreakReviewDecoding covers both the skip and the error paths.

P2 — Oversized hunks no longer abort later files ✅ (Codex finding)
emitReviewHunks (judgediff.go:411–421) uses continue where the original had return, so a hunk that exceeds the budget does not prevent later review-referenced files from being emitted. Budget is not decremented on a failed emit, so a smaller subsequent file can still fit. TestOversizedRequestedHunkDoesNotHideLaterGuard pins this.

P2 — Explicit line references now window oversized hunks ✅ (Codex finding — guard.py:2001 case)
reviewLineHints (judgereviews.go:181–206) extracts explicit line references from review comment prose and converts them to locusRef entries. These are appended to the structured-findings loci list in writeReviewDiffSection (judgereviews.go:151) and flow through the first tranche of renderJudgeDiffWithPaths as windowed views rather than full-hunk renders. TestExplicitReviewLineWindowsOversizedHunk confirms the line 2001 code is visible and the elision marker is present.

P2 — Skip-not-stop non-contiguous context → Deferred, documented
FOLLOWUPS.md (line 215) records the bounded omitted-comment identity summary as a deferred follow-up. The justification is solid: whole-entry first-fit retains useful smaller reviews from any position rather than abandoning everything after the first oversized comment. Retained entries carry artifact ID and original index; the omission count is explicit; the omission marker's wording (absence is not resolution) is load-bearing.

P3 — Multiple path-request headers per diff artifact → Documented as intentional
Each diff artifact produces its own per-artifact context header. The fix author's framing ("independently attributed") is reasonable — the P3 was never a correctness issue.


New finding

P3 — writeReviewDiffSection emits a misleading "decode error" for valid non-object evidence (judgereviews.go:138–154)

recordedReviewComments correctly skips valid-but-non-object evidence bodies ([], null, "string"). writeReviewDiffSection does not apply the same guard. For a KindEvidence artifact with body [], json.Unmarshal into the struct fails and the function emits:

[recorded diff unavailable (id): decode error]

This is technically a false error — the artifact is valid JSON, just not a diff-carrying object. null bodies do silently skip (unmarshal into a struct succeeds and leaves Diff=""), so the inconsistency is between []/string bodies and null bodies. TestNonObjectEvidenceDoesNotBreakReviewDecoding tests recordedReviewComments but not the writeReviewDiffSection path for these inputs.

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)

  • writeRecordedReviews (judgereviews.go:69): allocates the full scrubbed entry including the raw JSON body before the budget check. Nit on a non-hot path.
  • resolveReviewPath (judgereviews.go:125): the / in "/"+hint is load-bearing (prevents model.go matching alt_model.go) but still has no comment.
  • writeReviewDiffSection calls parseUnifiedDiff once for path/loci extraction and renderJudgeDiffWithPaths calls it again internally for the same diff string. Redundant but not incorrect.

Test coverage

The new tests added in fix round 1 are well-chosen:

  • TestReviewPathTokensAreExactAndDeduplicated pins the exact extension boundary and deduplication contract
  • TestOversizedRequestedHunkDoesNotHideLaterGuard and TestExplicitReviewLineWindowsOversizedHunk together close the two halves of the Codex finding
  • TestReviewPathDiagnosticsStayBounded closes the 20,000-path fixture concern

One coverage gap carried forward: TestNonObjectEvidenceDoesNotBreakReviewDecoding does not assert that judgeContext (via writeReviewDiffSection) handles a [] or "string" evidence body without emitting a decode-error marker.


Summary

All P2 findings from the initial review are addressed or properly deferred in fix round 1. The security boundary (scrub, artifactsBegin/artifactsEnd) is consistently applied. The skip-not-stop deferral is recorded with justification. The one new finding is P3 with theoretical impact. The code is correct at the P2 bar and ready for judgment.

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

Reviewer: claude
Reviewed commit: 2fd8479e701eb3f08b8b371fdd7dcf02187377b1

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

Comment thread cmd/gate/internal/verify/judgereviews.go
Comment thread cmd/gate/internal/verify/judgereviews.go Outdated
@itsHabib

itsHabib commented Sep 9, 2026

Copy link
Copy Markdown
Owner Author

Correction-head review disposition at 2fd8479e701eb3f08b8b371fdd7dcf02187377b1:

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:

  • Claude P3: valid non-object evidence such as arrays can render a diff decode-error marker even though they carry no diff. Current production diff/comment artifacts are objects; the marker is conservative unavailable-context text, never fabricated code or permission. Keep this as a consistency follow-up.
  • Copilot comments3965068714/3965068740: a temporary string conversion and repeated unified-diff parse add allocations. These are real optimization opportunities but do not change evidence selection, attribution, budget bounds or authorization. Both real blocked Ivy artifact replays together finish the rendering test in approximately 0.01 seconds on this machine. No measured performance failure justifies extending this correctness repair. Retain as profiling-led follow-ups rather than another code/review cycle.
  • The previously documented bounded omitted-comment identity summary remains in FOLLOWUPS.md.

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.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread cmd/gate/internal/verify/judgereviews.go
Comment thread cmd/gate/internal/verify/judgereviews.go Outdated
@itsHabib

itsHabib commented Sep 9, 2026

Copy link
Copy Markdown
Owner Author

Final panel reconciliation for 2fd8479e701eb3f08b8b371fdd7dcf02187377b1: Claude, Codex and Copilot have completed. CI check/fuzz/hygiene pass. Independent adversarial correction review passed. One fix round used.

Codex's final two P2 findings are valid limitations, proposed for explicit judge acceptance:

  1. Comment order is endpoint-grouped, not chronological. The rendered heading deliberately says reverse recorded order; timestamps are not retained in the existing Comment artifact, so this patch must not invent chronology. The raw review budget can omit comments, and the output explicitly reports incompleteness and says absence is not resolution. Structured finding loci still select code independently. A future artifact change should preserve source timestamps and define ordering for older timestamp-free records. No claim that all latest comments are retained is made.
  2. The path grammar is an explicit source-extension subset. Configuration names such as Dockerfile/go.mod and other extensions are not currently selected from prose; structured loci remain supported. This is a real generalization gap, but the bounded production consumers motivating this repair use .mjs/.py/.md references, and actual recorded Ivy109/102 replay asserts their guard code is present. Resolving general quoted tokens against recorded paths belongs in a follow-up with ambiguity/absent-token tests.

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.

@itsHabib

itsHabib commented Sep 9, 2026

Copy link
Copy Markdown
Owner Author

Final fix round (2/2), exact head dfe195c49248bcdfb7cc892e210bf5461ec062e8.

Gate BLOCK run_a405c64f55bca7e3 rejected the proposed deferrals on chronology and extension coverage. That decision remains settled; both concerns are now fixed in code:

  • GitHub created_at, updated_at and submitted_at propagate through inline, issue and review-body evidence. Stable source-activity sorting places newest known activity first across endpoints. Missing/invalid legacy timestamps remain explicitly unknown, last in reverse recorded order; no historical timestamps are invented.
  • Full quoted path tokens resolve against the recorded diff without an extension allowlist. Dockerfile, Makefile, go.mod, go.sum, Cargo.toml, .proto, .tf, custom extensions and paths with spaces are covered behind an oversized generated diff. Missing and ambiguous tokens are explicitly reported within the diagnostic cap. Explicit line references still select windows.
  • Also addressed the minor non-object diff diagnostic and allocation observations: byte-based object detection, consistent non-object skipping, and one shared parsed diff.

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.

@itsHabib

itsHabib commented Sep 9, 2026

Copy link
Copy Markdown
Owner Author

@claude review

@itsHabib

itsHabib commented Sep 9, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@itsHabib
itsHabib requested a lite review from Copilot September 9, 2026 06:19
@claude

claude Bot commented Sep 9, 2026

Copy link
Copy Markdown

Claude finished @itsHabib's task in 4m 54s —— View job


Code Review — PR #290 (Round 3 / Final Fix dfe195c)

Head reviewed: dfe195c · Branch: codex/gate-review-evidence

All prior verified findings are addressed. This review covers the changes from 2fd8479dfe195c (the final fix round) that addressed Gate's own BLOCK run rejection of the two Codex P2 deferrals.


Prior findings — disposition

Codex P2 — Chronological ordering

Correctly implemented across all three layers:

  • evidence.go: Comment struct gains CreatedAt, UpdatedAt, SubmittedAt string fields; all three fetchComments paths (inline, issue, review bodies) propagate them.
  • judgereviews.go: recordedReview gains timestamp time.Time; decodeReviewComments populates it via latestReviewTime; sort.SliceStable sorts ascending (oldest first, zero-time/unknown last); writeRecordedReviews and reviewPathHints both iterate in reverse so newest gets priority.
  • Section header updated to "newest known source activity first; unknown timestamps last in reverse recorded order" — correct description of the invariant.

The latestReviewTime helper picking max of created/updated/submitted is the right reduction: an edited issue comment should sort by its edit time, not creation time. The choice of updated_at as the authority for issue comments correctly models "long-running reviewer edits."

Codex P2 — Extension allowlist removed

reviewPathPattern changed from \\.(?:py|mjs|...) to [^:\r\n]+— resolves quoted tokens against the recorded diff rather than a hardcoded allowlist.TestReviewReferencesResolveWithoutExtensionRestrictionsverifiesDockerfile, go.mod, go.sum, Cargo.toml, Makefile, .proto, .tf, and paths with spaces all survive a 2000-line generated diff. The colon exclusion in [^:\r\n] correctly drops URL-like tokens (e.g. `https://x`).

Copilot — Allocation (bytes vs string conversion)

Both non-object skip guards now use bytes.HasPrefix(bytes.TrimSpace(a.Body), []byte("{")) instead of converting to string first. recordedReviewComments was updated in the prior round; writeReviewDiffSection receives the same guard in this round.

Copilot — Double diff parse

renderParsedJudgeDiff is split out from renderJudgeDiffWithPaths. writeReviewDiffSection calls parseUnifiedDiff once and passes the result to both reviewDiffPaths (via resolveReviewPath) and renderParsedJudgeDiff. The old call-through renderJudgeDiffWithPaths(evidence.Diff, ...) is gone.

Claude Round 2 P3 — Non-object evidence in writeReviewDiffSection

writeReviewDiffSection now carries the same bytes.HasPrefix guard at its top, consistent with recordedReviewComments. A []-body artifact no longer emits a misleading decode-error marker.


New observation

P3 — Broad pattern produces diagnostic entries for common inline code tokens

reviewPathPattern = \([^ + "" + :\r\n]+)\`` matches any backtick-quoted token without a colon. Review prose routinely uses backticks for identifiers: `nil`, `err`, `false`, `context`. None of these appear in the diff, so each produces a "context unavailable: nil: absent from recorded diff" entry in the 8 KiB diagnostic section.

In a verbose review comment, the diagnostic cap can be exhausted quickly and then emit "N entries omitted by byte budget" without the judge seeing what was actually resolved vs. absent. The 8 KiB cap bounds the blast radius; absent entries never fabricate code or authority. The tradeoff is correct — silently dropping Dockerfile is worse than noisy diagnostics about nil. Worth noting but not a correctness blocker.

If noise becomes a problem in practice, a post-resolution filter could suppress tokens that match nothing in the diff without reporting them as missing (since "nil not found in diff" is expected and not meaningful). That's a follow-up, not a fix here.


Tests

TestReviewChronologyOverridesEndpointGrouping correctly exercises the core scenario: a newest inline comment (created_at: 2026-09-09T03:00:00Z) competes with an oversized old issue comment (created_at: 2026-09-08T01:00:00Z). After sorting and reverse iteration, the newest comment's path hint (guard.py) is prioritized and the comment body appears before the old review body. The oversized issue comment is omitted (fitting budget arithmetic), and the omission count is present.

TestReviewActivityUsesUpdatesAndPreservesUnknown pins two invariants: (1) a comment with updated_at=03:00 sorts after created_at=02:00 (edit time wins), reaching index 2 in the ascending sort; (2) the comment with an invalid created_at retains a zero timestamp at index 0.

TestReviewReferencesResolveWithoutExtensionRestrictions closes the configuration-path gap from the Codex finding. Paths with spaces (docs/a file.custom) work because [^ + "" + :\r\n]includes space. The absentmissing.config` is explicitly reported.

TestReviewBodiesRetainSourceActivity (reviews_test.go) is the unit pin for reviewBodies — the path that was most likely to be missed since review-body evidence has no Path/Line and the timestamp fields are new.

TestReviewPathTokensAreExactAndDeduplicated is correctly updated: guard.py.backup was previously silently dropped by the extension allowlist; now it's extracted as a hint and reported as "absent from recorded diff" in missing. The reviewPathHints assertion inverts to confirm the token IS extracted.


Summary

The final fix round addresses all outstanding verified findings. Chronological ordering is correctly implemented with latestReviewTime + stable sort + reverse iteration; evidence.go propagates timestamps through all three comment-fetching paths. The extension allowlist is correctly replaced with diff-backed resolution — arbitrary config paths are covered, with absence and ambiguity explicitly reported. The shared parsed diff eliminates the double-parse. Security boundary (scrub, artifactsBegin/artifactsEnd) is consistently applied. No grant, reducer, panel authority, judgment uniqueness, or merge execution behavior is changed.

The one new P3 (diagnostic noise from identifier tokens) is bounded, non-fabricating, and best addressed as a follow-up if noise is observed in production. No unresolved P1 or authorization-invariant finding is known.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

Reviewer: claude
Reviewed commit: dfe195c49248bcdfb7cc892e210bf5461ec062e8

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread cmd/gate/internal/verify/judgereviews.go Outdated
Comment thread cmd/gate/internal/verify/judgediff.go
@itsHabib

itsHabib commented Sep 9, 2026

Copy link
Copy Markdown
Owner Author

Final panel complete at dfe195c49248bcdfb7cc892e210bf5461ec062e8: Claude confirms prior blocking issues fixed; Copilot recommends approval with zero new findings; Codex completed with two P2 residuals below. CI is green, and independent adversarial review of this correction found no remaining issue. Both permitted fix rounds are consumed; no fourth panel is requested.

Residuals proposed for explicit Gate judgment, not marked fixed:

  • Codex3965274642: wide prose line ranges retain the first endpoint only. A range whose far endpoint lies outside the window can lose that portion when the whole hunk exceeds budget. Existing structured finding loci remain separate; diff elision/truncation remains visible. This needs endpoint coverage as a follow-up. The real Ivy109/102 copied-artifact replays assert the precise allocation/overwrite guards remain present; this does not prove arbitrary wide ranges complete.
  • Codex3965274658: review-referenced mode-only/binary/rename-only files have no hunks, so their prefaces currently appear only in the ordinary remainder pass and can be crowded out. This concerns file metadata context; the current Ivy consumers depend on textual source guards. Follow-up should emit the preface in the referenced tranche. No claim of binary contents being inspected is made.
  • Claude P3: generic quoted identifiers may generate noisy missing-token diagnostics; the 8 KiB diagnostic cap and omission count bound this without consuming the 48 KiB code budget.
  • Optional omitted-comment identity summary remains documented in FOLLOWUPS.md.

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.

@itsHabib

itsHabib commented Sep 9, 2026

Copy link
Copy Markdown
Owner Author

Operator-approved bounded correction at exact head 96cac26299ea011318c54fecd25edc2339b69644.

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.

  • Wide line ranges now capture and expand both endpoints before existing deduplication and diff-window selection. Regressions cover hyphen and en-dash ranges with distant endpoints inside an oversized hunk.
  • Referenced hunkless files now emit their preface in the prioritized tranche. Binary, mode-only and rename-only metadata regressions place these files behind an oversized unreferenced source hunk.

Full go test -race ./cmd/gate/..., go vet ./cmd/gate/..., golangci-lint run ./cmd/gate/... (0 issues), and git diff --check pass. An independent read-only review of the exact three-file correction found no verified issue and confirmed endpoint windowing and hunkless-file budget/truncation behavior. The configured panel last reviewed dfe195c; its final two findings are verified-addressed at this new head, not claimed retracted by a new panel. No further panel was requested.

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.

@itsHabib
itsHabib merged commit dfae08a into main Sep 9, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants