Skip to content

fix(gate): make recorded judgment packet coverage satisfiable - #341

Merged
itsHabib merged 4 commits into
mainfrom
codex/gate-evidence-coverage
Sep 14, 2026
Merged

itsHabib merged 4 commits into
mainfrom
codex/gate-evidence-coverage

Conversation

@itsHabib

@itsHabib itsHabib commented Sep 13, 2026

Copy link
Copy Markdown
Owner

Summary

Gate can declare a recorded judgment packet incomplete even when its reviews are already rendered, or require source that the supported collector cannot supply. Rooms #120 reproduced duplicate review accounting and a total source limit smaller than its cited files. RoxIQ #246 selected unrelated same-basename files; #252 inferred an unchanged 9.5 MB executable from a command mention.

What this adds

  • Count complete rendered reviews by evidence ID and recorded index once. Actually omitted reviews remain missing, including unresolved reviews without a known head.
  • Preserve structured anchors and precise, unambiguous source references, including explicit root paths and index precedence over a changed basename. Report ambiguous names and unchanged bare tokens as visible hints without inventing required source or resolving findings.
  • Increase the bounded total supplemental text from 256 to 512 KiB, retaining 256 KiB per file, text/blob verification, exact subject binding, atomic append, 32 paths and three supplements. The separate diff/review budgets remain unchanged.

No provider judgment, grant authority, merge or installation is added. Packet completeness describes mechanical coverage.

Validation

  • Gate tests and full Gate race suite, vet, lint (zero issues), build, whitespace and paired-guide checks.
  • Synthetic regressions cover false and actual review omission, unknown-head precise findings, ambiguous paths, explicit binary anchors, exact-path precedence, atomic overflow refusal, invalid text and oversized files.
  • Offline replays of the three exact recorded subjects: Rooms becomes complete at 417,549 total source bytes; RoxIQ feat(contracts/org): versioned canonical encoder and digest #246 at 306,607 bytes; feat(flare): page grant_needed, collapse the repeated question, stop claiming health #252 becomes complete with its existing 200,050 bytes. Source additions existed only in disposable local fixtures, verified against exact Git blob hashes.
  • First-head independent verification, Codex review, Claude review and CI passed at c86e011.
  • Final native independent verification and CI passed at c231da0e42a75fb5bf4c27b6b5afce03d98a5916. The independent reviewer repeated full Gate race/vet/lint, all three replays and exact source-byte/blob checks.
  • Claude final-head review found no new issues and confirmed all prior suggestions addressed; Actions attestation names the exact head. GitHub Codex declined the follow-up due to exhausted review capacity; its clean review remains attributed to c86e011. Copilot completion remains unverified.

See cmd/gate/docs/evidence-coverage-poc.md for the measurements and supported same-run repair flow. Private snapshots remain local. Draft only; no live Gate evaluation, supplement, judgment, custody edit, installation or merge.

@itsHabib

Copy link
Copy Markdown
Owner Author

@codex review

@itsHabib

Copy link
Copy Markdown
Owner Author

@claude review

@chatgpt-codex-connector

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 🔄 Running since 2026-09-13T23:58:10.618798Z c86e011 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 13, 2026

Copy link
Copy Markdown

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


Code Review — fix(gate): make recorded judgment packet coverage satisfiable

Reviewed at HEAD c86e011 (branch codex/gate-evidence-coverage).

  • Gather context and understand the request
  • Read core logic changes (packet.go, judge.go, judgereviews.go)
  • Read test files (packet_coverage_test.go, packet_test.go, source_test.go)
  • Read documentation changes (README.md, evidence-coverage-poc.md)
  • Analyze correctness, edge cases, and potential issues
  • Post review findings

Summary

The PR fixes three distinct bugs in JudgmentPacket that caused parked runs to be unsatisfiable. The core changes are: (1) tracking which reviews are already in the primary context so they aren't double-billed against the secondary budget, (2) giving exact paths precedence over same-basename matches, and (3) demoting bare tokens and ambiguous basenames from "required source" to visible hints. The offline replay evidence is thorough and the synthetic regression tests cover all three bug patterns well.

No authorization paths, grant authority, merge commands, or custody writes are touched. The scope is correctly limited to packet coverage mechanics.


Findings

P2 — Dead branch in matchingPacketPaths second loop

cmd/gate/internal/verify/packet.go lines 227–235

func matchingPacketPaths(hint string, known []string) []string {
    for _, name := range known {
        if name == hint {
            return []string{name}   // ← early return on exact match
        }
    }
    var result []string
    for _, name := range known {
        if name == hint || ...     // ← name == hint can never be true here

After the early-return pre-pass, the name == hint clause in the second loop is unreachable — any exact match was already returned. This is harmless, but it reads as intentional code to a future reader who has to convince themselves it's dead. Removing it also clarifies the invariant: the second loop is only a basename-suffix scan.

Fix this →

P2 — writeRequiredReviews silently mutates the caller's map

cmd/gate/internal/verify/packet.go lines 315–332

func writeRequiredReviews(b *strings.Builder, active []recordedReview, included map[string]bool) []string {
    // ...
    b.WriteString(entry)
    included[c.key()] = true  // mutates the map from judgeContextWithReviewCoverage

The mutation is intentional (marking secondary-budget reviews so they won't be re-requested), but nothing at the call site or in the signature signals it. A future change that caches or reuses included for something else could break the deduplication silently. A one-line comment at the mutation point would close this: something like // mark written so packet completeness doesn't also count it missing.

P3 — Hints emitted through writeReviewPathMetadata's "context unavailable" slot

cmd/gate/internal/verify/packet.go lines 68–69

p.SourceHints = uniquePacketStrings(refs.hints)
writeReviewPathMetadata(&b, nil, p.SourceHints)

Hints are passed as the missing parameter, so they appear in the packet context as [review-referenced context unavailable: review X/Y: scrape-audit is an unchanged bare token; ...]. The "context unavailable" label is accurate for the paths-not-found case that function was designed for, but reads oddly for a diagnostic note that's explicitly saying the item is NOT required. The hints are also already in p.SourceHints for callers who want them; the inline rendering is mainly for the judge. If the label matters at all to you, a separate emit slot (e.g., "review-path hint") would make this easier to parse in the raw context, though the current framing doesn't affect completeness correctness.


What works well

Core bug fix is correct. The old writeRequiredReviews(b, active) wrote ALL active reviews to the secondary budget without checking whether the primary budget had already included them. The new flow — writeRecordedReviews returns included, judgeContextWithReviewCoverage threads it out, writeRequiredReviews skips on included[c.key()] — eliminates the double-billing. TestPacketCountsEachRenderedReviewOnce reproduces the Rooms #120 scenario exactly (two 40 KiB reviews that together exceed the secondary budget, now each counted once in the primary context and done).

Exact-path-first in matchingPacketPaths is the right fix for RoxIQ #246. When spec.md is an exact match in the known list, returning immediately prevents the basename suffix scan from also picking up a/spec.md and b/spec.md. TestPacketExactPathDoesNotSelectSameBasenameElsewhere pins this.

Bare-token demotion is well-scoped. The !strings.Contains(match[1], "/") && match[2] == "" condition is the minimal predicate: a mention needs a path separator OR a line anchor to be treated as a file reference. A bare scrape-audit token without either remains visible in hints, not in RequiredSources. TestPacketDoesNotPromoteBareCommandOrAmbiguousIndexHint covers the RoxIQ #252 pattern.

packetReferences struct is a cleaner interface than the previous four-return-value packetRequirements. The methods on it read well.

Budget constant separation (SourceBudget = 512 KiB for collected source vs. requiredDiffBudget = 256 KiB for the diff window in-packet) removes the confusion of the prior code that reused SourceBudget for both. The error messages now compute the KiB value from the constant rather than hardcoding "256 KiB", which will stay correct if the budget changes again.

staleComment semantics confirmed correct for the unknown-head case. An unresolved review with an empty commit_id is never treated as stale (commitID != "" && headSHA != "" && commitID != headSHA requires both anchors to be present). TestPacketKeepsUnknownPreciseFindingDespiteNewCleanReview validates that a precise finding from an unknown-head review stays required even when a newer clean review exists.

TestEvidenceRepairAggregateOverflowRecordsNothingPartial correctly exercises the new 512 KiB limit: two 215 KiB files (430 KiB total) succeed, a third 100 KiB file pushes the aggregate to 530 KiB and is rejected atomically.

source_test.go additions close coverage on the decodeExactPaths comma-in-path case (the docs/a,b.md file introduced in the CLI fixture).


Minor observations

  • writeRecordedReviews iterates newest-to-oldest, but writeRequiredReviews iterates the active slice in its existing order (also newest-to-oldest via actionablePacketComments which preserves sort). The budget accounting in both sections is therefore consistent in priority, which is correct.
  • addComment is called for ALL active comments, including those with structured anchor.Path already processed in the first loop of packetRequirements. A comment that has both a structured anchor (added via the first loop) and body text references (added via addHint) will add paths from both sources. Deduplication in uniquePacketStrings later cleans this up, so there's no correctness issue.
  • The evidence-coverage-poc.md doc is a clean account of the replay methodology. The note that "private snapshots remain local" and commits contain only synthetic regression data is the right boundary to draw.

Overall this is sound. The two P2 items (dead branch and undocumented map mutation) are minor cleanups worth doing before merge, but neither is a correctness risk.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🚀

Reviewed commit: c86e011d5b

ℹ️ 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".

@github-actions

Copy link
Copy Markdown

Reviewer: claude
Reviewed commit: c86e011d5bdd2e110501974a13de67b7b8a7aca6

@itsHabib

Copy link
Copy Markdown
Owner Author

Review disposition and correction at c231da0:

Independent review passed c86e011 and reproduced all three saved cases with every active review fully present (Rooms #120: 37, RoxIQ #246: 14, RoxIQ #252: 13) and every source byte/blob checked against its exact Git object. It also reproduced a pre-existing false-completeness case at both base and c86e011: an explicit ./spec.md reference could select changed a/spec.md before considering the root file in the index. That source-selection defect is included in this PR's correction.

Explicit ./ paths now retain exact-path intent. A precise unique basename fallback consults the complete index before claiming coverage, and matching considers the combined diff/index so an exact root or known ambiguity wins over a nested changed basename. New regressions cover root references with and without the index plus ambiguity split across diff/index. All three saved replays still pass with unchanged source totals.

Claude's reported P2 dead comparison is removed; its P2 map-mutation documentation suggestion is addressed; its P3 label suggestion is addressed by a bounded source-hint slot in the existing metadata writer. Codex reported no major issues at c86e011. Gate vet, lint (zero issues), verify race tests and diff checks pass after the correction. Independent verification and fresh CI/review are being collected for c231da0.

No live Gate, custody, installation or merge action occurred. Packet completeness remains a mechanical result, not a judgment.

@itsHabib

Copy link
Copy Markdown
Owner Author

@codex review

@itsHabib

Copy link
Copy Markdown
Owner Author

@claude review

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@claude

claude Bot commented Sep 14, 2026

Copy link
Copy Markdown

Claude finished @itsHabib's task in 5m 45s —— View job


Code Review — fix(gate): make recorded judgment packet coverage satisfiable

Reviewed at HEAD c231da0 (branch codex/gate-evidence-coverage).

  • Gather context and understand the request
  • Read core logic changes (packet.go, judge.go, judgereviews.go)
  • Read test files (packet_coverage_test.go, packet_test.go, source_test.go)
  • Read documentation changes (README.md, evidence-coverage-poc.md)
  • Analyze correction commit (c231da0) changes vs prior head (c86e011)
  • Post review findings

Summary

The correction commit (c231da0) addresses all three findings from the prior review, plus a pre-existing explicit-root reference bug discovered independently. The core packet coverage logic is sound. No new issues found; notes below are observations, not blockers.


Prior findings — disposition

P2: Dead name == hint clause in matchingPacketPaths second loop — Fixed. The second loop in matchingPacketPaths now contains only strings.HasSuffix(name, "/"+hint). The name == hint clause is gone; the early-return pre-pass handles all exact matches, and the second loop is exclusively the basename-suffix scan. packet.go:234–239

P2: Undocumented map mutation in writeRequiredReviews — Addressed. The mutation site now carries // Record secondary-section coverage in the same per-packet identity set. packet.go:339. The intent is clear to a future reader.

P3: Hints emitted through writeReviewPathMetadata's "context unavailable" slot — Fixed by adding a dedicated hints parameter to writeReviewPathMetadata. Hints now emit as [review-referenced source hint: ...] rather than [review-referenced context unavailable: ...]. Both call sites updated (judgereviews.go:170, packet.go:69), verified with grep. TestPacketChecksAmbiguityAcrossDiffAndIndex explicitly asserts the label [review-referenced source hint: appears in context (packet_coverage_test.go:109).


Correction commit — explicit-root reference bug

Root cause. At c86e011, addHint stripped ./ before calling matchingPacketPaths. An explicit ./spec.md reference became bare spec.md, which the basename-suffix scan matched against a/spec.md in the diff — selecting the wrong file.

Fix. matchingPacketPaths now receives the original match[1] (with ./ intact) and derives exactOnly := strings.Contains(hint, "/") before stripping the prefix (packet.go:223–224). Any hint that carries a / is treated as an explicit path: if no exact match is found in known, the basename-suffix scan is skipped entirely (packet.go:229–231). The separate hint variable (TrimPrefix applied) is still used where a display-friendly string is needed (ambiguity messages, bare-token candidate lookup).

Tracing the two TestPacketRootReferenceWinsOverChangedBasename subtests (packet_coverage_test.go:82–99):

  • ./spec.md:50 with diff containing a/spec.md, no index: matchingPacketPaths("./spec.md", ["a/spec.md"])exactOnly=true, no exact match → nil. needsIndex=true is set. Missing gets "file index unavailable". ✓
  • spec.md:50 with the same diff: matchingPacketPaths("spec.md", ["a/spec.md"]) → basename match → ["a/spec.md"]. Then len==1 && line!=0 && matches[0] != hint ("a/spec.md" != "spec.md") → needsIndex=true. Missing gets "file index unavailable". ✓
  • Both subtests: with index ["spec.md", "a/spec.md"] added, known = changed + index. matchingPacketPaths("./spec.md", known) and matchingPacketPaths("spec.md", known) both find the exact match spec.md before the basename scan. RequiredSources = ["spec.md"]. ✓

The operator-precedence in len(matches) == 0 || len(matches) == 1 && match[2] != "" && matches[0] != hint is standard Go (&& binds tighter than ||); the second clause is the exact-basename-mismatch case and is only reachable when exactOnly=false.


Cross-diff + cross-index ambiguity detection

TestPacketChecksAmbiguityAcrossDiffAndIndex (packet_coverage_test.go:101–112): diff has a/spec.md, index has ["a/spec.md", "b/spec.md"]. spec.md:50 finds a/spec.md in the diff alone (unique), sets needsIndex, then merges known = ["a/spec.md"] + ["a/spec.md", "b/spec.md"]. matchingPacketPaths("spec.md", known) returns ["a/spec.md", "b/spec.md"] (ambiguous). The hint is emitted; RequiredSources=[]; packet is complete. This is the right behavior: when combined diff+index reveals ambiguity that diff-alone hid, no source is selected and the candidates are surfaced as hints rather than silently picking the wrong one.

uniquePacketStrings inside matchingPacketPaths deduplicates the doubled a/spec.md from known correctly.


Things that remain correct

writeRecordedReviewsincludedReviewswriteRequiredReviews threading. The included map is built by writeRecordedReviews keyed by evidence/index, threaded through judgeContextWithReviewCoverage, and passed to writeRequiredReviews which skips any already-rendered entry. TestPacketCountsEachRenderedReviewOnce reproduces Rooms #120: two 40 KiB reviews together exceed the secondary budget, but each is counted once in the primary context and not re-billed. TestPacketDoesNotHideUnrepresentedReview confirms a review too large for the primary context still registers as missing.

Bare-token branch. !strings.Contains(match[1], "/") && match[2] == "" is the minimal predicate. A bare scrape-audit with no slash and no line anchor is a hint only. ./spec.md (contains /) and spec.md:1 (has line anchor) both bypass this path correctly.

SourceBudget split. SourceBudget = 512 KiB (collected source across the run) is cleanly separated from requiredDiffBudget = 256 KiB (the packet's diff window). The checkEvidenceRepair aggregate check correctly uses verify.SourceBudget; the per-packet diff loop uses requiredDiffBudget. Error messages derive KiB values from the constant.

decodeExactPaths comma-in-path coverage (source_test.go:51): the docs/a,b.md fixture in TestExactPathsRequiresCompleteTree closes a gap that comma-delimited path parsers can hit.

writeReviewPathMetadata callers — both sites updated, grep confirms no third caller exists.


Minor observations (non-blocking)

  • In addHint, hint (the TrimPrefix-ed version) serves double duty: display string in messages AND the matches[0] != hint comparator for detecting basename vs. exact matches. This works correctly because matchingPacketPaths also strips ./ before its exact check, so matches[0] always reflects the stripped form. Subtle but correct.
  • addComment is called on all recordedReviewComments (not just active), consistent with the prior code's use of packetLineHints and reviewPathHints on the full comment set. Prose from stale or coordinator comments can still add to refs.paths/refs.loci, though they can only contribute if the path uniquely matches changed + index; structured anchors (explicit path/line fields) are gated exclusively to active. This asymmetry is a pre-existing design choice, unchanged here.
  • The correction commit's change from matchingPacketPaths(hint, changed) to matchingPacketPaths(match[1], changed) (packet.go:282) is the single-character-level load-bearing change; the rest of the commit is the consequence of that decision propagating through matchingPacketPaths and the test suite.

Overall this is solid. The three prior findings are fully addressed, the explicit-root bug is correctly diagnosed and fixed, the new tests are specific and regression-complete, and the documentation update accurately reflects the new behavior.

@github-actions

Copy link
Copy Markdown

Reviewer: claude
Reviewed commit: c231da0e42a75fb5bf4c27b6b5afce03d98a5916

@itsHabib

Copy link
Copy Markdown
Owner Author

Final evidence at c231da0:

One clarification to Claude's nonblocking observation: packetRequirements receives the already filtered active slice, and addComment iterates that parameter. Prose source selection therefore uses active reviews too; the full recorded review display remains separate. No code change is needed for that observation.

Private replay inputs remain local. A local binary is explicitly stamped with this commit. No installed runtime, live evidence supplement, judgment, custody, grant or merge was changed. This verifies packet construction and its safeguards; live provider judgment and delivery remain owner actions.

@itsHabib

itsHabib commented Sep 14, 2026

Copy link
Copy Markdown
Owner Author

Owner-reported live confirmation of the reviewed isolated candidate, revision c231da0, SHA-256 6539f9a60ff3676e3d72fa1b765d954a010909fcccacea5d393684ca938323c9:

  • Rooms feat(console): brief-led trace view — decision panel + verifier ladder #120 used the supported collector on original run run_65fd0699fc1d20da, producing evd_35e3f8aa951a46e4. The owner reports a complete packet with no missing entries. Its independent judge was running at the report.

  • RoxIQ feat(contracts/org): versioned canonical encoder and digest #246 used the supported collector on original run run_246d50b568905db4, producing evd_87dd606161f429e8. The owner reports a complete packet and an independent Codex PASS at T1 on the original head. Its merge action remains held for the separate production decision.

  • RoxIQ #252 owner receipt: original run run_c92458419c337e8a was complete without a supplement; supported Codex judgment PASS at T2 (jdg_7c579fab35eae25e) and pinned action act_e8107c8996e31fc1 were read back. No replacement run or merge occurred; separate production approval remains.

Existing artifact, source-evidence, grant and custody schemas are unchanged, so no migration is required. Owners used the isolated reviewed binary under their existing repair authorization. No shared installation or direct packet/custody edit was needed. These owner receipts extend the offline replay evidence; this source task did not run their live commands or authorize their downstream merges.

An independent exact-head review found two packet-resolution defects:

- A bare, line-less token matching a changed file by basename was re-resolved
  against the recorded file index, where an unchanged root blob of the same
  name won the exact match. Following the packet's own "run gate evidence"
  advice then required that blob; an oversized one made the run permanently
  unsatisfiable. Bare tokens now resolve only among changed files.
- A precise reference whose basename matched several changed files skipped the
  index and reported Complete with an ambiguous hint, even when an exact root
  file of that name would win. Any basename fallback now waits for the index.

Pre-existing reference-parsing gaps found by the same review are recorded in
FOLLOWUPS.md. Saved Rooms #120 and RoxIQ #246/#252 replays stay complete.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@itsHabib

Copy link
Copy Markdown
Owner Author

@claude review

@claude

claude Bot commented Sep 14, 2026

Copy link
Copy Markdown

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


Code Review — fix(gate): make recorded judgment packet coverage satisfiable

Reviewed at HEAD 6dfeed1 (branch codex/gate-evidence-coverage).

  • Gather context and understand the request
  • Read changes since last review (c231da06dfeed1)
  • Read core logic changes and test files
  • Analyze correctness, edge cases, and potential issues
  • Post review findings

Summary

The correction commit (6dfeed1) addresses two bugs introduced in c231da0's needsIndex condition and refactors bare-token handling into a dedicated function. No new issues found; the implementation is correct. Notes below are observations, not blockers.


Changes in 6dfeed1

Two bugs in c231da0's needsIndex condition fixed:

The old condition was:

if len(matches) == 0 || len(matches) == 1 && match[2] != "" && matches[0] != hint {
    refs.needsIndex = true
}

This had two gaps:

  1. A basename fallback without a line numberspec.md uniquely matching a/spec.md — would not set needsIndex (guarded by match[2] != ""). The root spec.md would be silently ignored.
  2. Multiple diff matchesREADME.md:1 matching a/README.md, b/README.md, c/README.md — would not set needsIndex, even though an exact root README.md might exist.

The new condition:

if len(matches) != 1 || matches[0] != hint {
    refs.needsIndex = true
}

Covers all cases correctly:

  • Zero matches → needs index ✓
  • Exactly one exact match (spec.md == spec.md) → no index needed ✓
  • Exactly one basename fallback (spec.mda/spec.md) → needs index ✓
  • Multiple matches → needs index ✓

Bare token extraction into addBareToken:

Previously bare tokens only bypassed the needsIndex path when len(matches) == 0. A bare token matching a single changed file (package-lock.jsonweb/package-lock.json) would fall through to the general path, set needsIndex = true (since matches[0] != hint), and then resolving against the index could find a root package-lock.json blob and make it required — even though the reviewer only mentioned the changed file.

The new addBareToken always branches before needsIndex is evaluated, resolving only against already-computed matches (changed files), never against the index:

if !strings.Contains(match[1], "/") && match[2] == "" {
    refs.addBareToken(review, hint, matches, index)
    return
}

addBareToken semantics:

  • One changed match → add to refs.paths (diff covers it)
  • Multiple changed matches → ambiguity hint
  • Zero changed matches → unchanged-blob hint (if any index candidates exist)

The integration test TestEvidenceRepairBareTokenStaysSatisfiable confirms the full repair flow: the collector's read function returns an error for the unchanged root package-lock.json; the packet completes because only the diff-covered web/package-lock.json is required.


Prior findings — disposition

All three findings from the c86e011 review were addressed at c231da0 and remain intact here:

  • P2 dead name == hint clause: still absent from matchingPacketPaths second loop (packet.go:235)
  • P2 undocumented map mutation in writeRequiredReviews: comment still present (packet.go:339)
  • P3 misleading "context unavailable" label: dedicated hints slot still in place

Findings

P3 — addBareToken parameter name changed receives pre-filtered matches, not the full changed list

cmd/gate/internal/verify/packet.go:312

func (refs *packetReferences) addBareToken(review, hint string, changed, index []string) {

The caller passes matches — the result of matchingPacketPaths(match[1], changed_files) — as the changed parameter. The name changed implies "all changed files" but the actual argument is the subset that already match this token. The doc comment says "resolves only among changed files" which is correct in intent, but a reader of this function's signature in isolation might expect changed to be the full list and wonder why the function doesn't filter it.

This is harmless — one call site, private function — but the parameter would read more accurately as matches or changedMatches.

Fix this →


What works well

TestPacketReportsAmbiguousHintWithoutRequiringEveryCandidate update is correct. The test previously expected p.Complete == true without an index when README.md:1 matched three basenames in the diff. That was c231da0's bug — multiple matches didn't set needsIndex. The test now correctly expects p.Complete == false (missing index) on the first call, then p.Complete == true after supplying an index that contains only the nested files (no root README.md). The path from "needs index" to "ambiguous → hint → complete" is the right resolution when no root file exists.

TestPacketPreciseBasenameWithSeveralChangedMatchesNeedsIndex covers the two-path case. Subtest with root spec.md in index → RequiredSources = ["spec.md"] (correct: root exists and must be covered). Subtest with only nested files in index → ambiguous → hint → complete. Both transitions are correct.

TestPacketBareTokenNeverRequiresUnchangedIndexBlob's two-phase structure. First phase (diff only): bare package-lock.json matches single web/package-lock.json → diff covers it → complete. Second phase (diff + index with root package-lock.json): addBareToken never set needsIndex, so adding the index changes nothing — web/package-lock.json remains the only required path, covered by diff. The +++ b/web/package-lock.json assertion confirms the diff section is present in context.

addBareToken's three-branch structure is clean. Single changed match / multiple changed matches / no changed match are each handled distinctly with appropriate messaging, and the index is only consulted for the "hint only" case — never to promote an unchanged blob to required source.

FOLLOWUPS.md records the pre-existing P3 gap accurately. The oversized-bare-token case (where the file exceeds both diff budget and source limit) is a genuine limitation but pre-dates this PR. Recording it in FOLLOWUPS with the correct framing ("consider demoting bare, line-less mentions to hints when their complete coverage is unsatisfiable") is the right call.

matchingPacketPaths dead branch from prior review remains absent. The second loop still contains only strings.HasSuffix(name, "/"+hint) — no name == hint clause that could never be true.


Minor observation

The addHint function constructs hint = strings.TrimPrefix(match[1], "./") for display/comparison, while passing the original match[1] to matchingPacketPaths. Inside matchingPacketPaths, ./ is also stripped before exact-match comparison. So matches[0] for a ./spec.md reference would be "spec.md" (from the known list), and hint in addHint is also "spec.md". The matches[0] != hint comparator is therefore consistent across the ./-prefix case. Subtle but correct.


Overall this is solid. The P3 parameter name is the only cleanup item; it does not affect correctness. The pre-existing gaps are correctly deferred.

@github-actions

Copy link
Copy Markdown

Reviewer: claude
Reviewed commit: 6dfeed179c3c879d3b7841b4c28a126b889230da

A re-review of 6dfeed1 found that resolving bare tokens only among changed
files traded one unsatisfiable shape for another. When a monorepo PR changes
web/package-lock.json and api/package-lock.json is unchanged, a bare mention
of `package-lock.json` kept the lone changed match required even after the
index showed the sibling. An oversized lockfile then made the run
unsatisfiable, where c231da0 reported an ambiguous hint.

A bare token now requires a changed file only when that file is the one path
in the repository with the name; any same-named file makes it a hint listing
every candidate. An unchanged blob is still never required.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@itsHabib

Copy link
Copy Markdown
Owner Author

@claude review

@claude

claude Bot commented Sep 14, 2026

Copy link
Copy Markdown

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


Code Review — fix(gate): make recorded judgment packet coverage satisfiable

Reviewed at HEAD c110746 (branch codex/gate-evidence-coverage).

  • Gather context from prior reviews
  • Read changes since last review (6dfeed1c110746)
  • Read core logic and test files
  • Analyze correctness and edge cases
  • Post review findings

Prior finding — disposition

P3: addBareToken parameter name changed received pre-filtered matches — Resolved. The call site in addHint now passes the full changed list (packet.go:284) instead of the pre-filtered matches. The parameter name changed is now semantically accurate. The function itself was rewritten to derive the filtered sets internally via the new sameNamePaths helper.


Correction commit — same-named sibling bug

Root cause. At 6dfeed1, addBareToken received matches (the pre-filtered subset of changed files that matched the bare token). A single-file diff match (web/package-lock.json) triggered len(changed) == 1 → refs.paths = ["web/package-lock.json"], making that file required — even when the file index later revealed a same-named sibling (api/package-lock.json). An oversized sibling lockfile makes this unsatisfiable.

Fix. addBareToken now receives the full changed list and uses sameNamePaths to compute two independent sets:

inDiff := sameNamePaths(hint, changed)
all := sameNamePaths(hint, append(append([]string(nil), changed...), index...))

The requirement is tightened to len(inDiff) == 1 && len(all) == 1 — a changed file is only required when it is the one path with that name anywhere in the known repository. A same-named sibling in the index demotes the mention to an ambiguity hint regardless of the diff. The doc comment precisely states the new contract: "requires a changed file only when that file is the one path in the repository with the name."

Tracing TestPacketBareTokenWithSameNamedSiblingIsHint (packet_coverage_test.go:169–192):

  • Phase 1 (no index, oversized diff): inDiff = ["web/package-lock.json"], all = ["web/package-lock.json"]len == 1 && len == 1 → required. Diff section exceeds requiredDiffBudgetRequiredSources = ["web/package-lock.json"], p.Complete == false. ✓
  • Phase 2 (index ["api/package-lock.json", "web/package-lock.json"] added): all = ["web/package-lock.json", "api/package-lock.json"] (deduped, changed-first order) → len(all) == 2 ≠ 1len(inDiff) > 0 branch → ambiguity hint → RequiredSources = [], p.Complete == true. ✓
  • Hint asserts "ambiguous package-lock.json; candidates [web/package-lock.json api/package-lock.json]". ✓

TestPacketBareTokenAmbiguousAcrossChangedFiles update (packet_coverage_test.go:143): the test already provided an index ["Makefile", "a/Makefile", "b/Makefile"]. With all = sameNamePaths("Makefile", changed + index), the root Makefile from the index now appears in the candidate list. The expected string updates from [a/Makefile b/Makefile] to [a/Makefile b/Makefile Makefile], reflecting the more complete candidate picture. ✓

TestPacketBareTokenNeverRequiresUnchangedIndexBlob (packet_coverage_test.go:113) remains correct under the new logic:

  • Phase 1 (no index, small diff): inDiff = all = ["web/package-lock.json"] → required; diff text is small → covered within requiredDiffBudgetRequiredSources = [], p.Complete == true. ✓
  • Phase 2 (index ["package-lock.json", "web/package-lock.json"]): all = ["web/package-lock.json", "package-lock.json"] → ambiguity hint → no required sources → p.Complete == true. ✓

sameNamePaths helper

func sameNamePaths(name string, known []string) []string {
    var result []string
    for _, path := range known {
        if path == name || strings.HasSuffix(path, "/"+name) {
            result = append(result, path)
        }
    }
    return uniquePacketStrings(result)
}

This is equivalent to matchingPacketPaths's second loop with the exact-match clause added, and is correct for bare tokens (which, by definition, contain no /). The deduplication handles the case where a path appears in both changed and index — the first occurrence (changed-file order) is retained, consistent with the changed-first append in addBareToken. The helper has a clear, narrow contract; it is not an attempt to replace matchingPacketPaths, which carries the additional ./-stripping and exactOnly semantics needed for precise references.


Things that remain correct

len(inDiff) > 0 branch reports all candidates, not inDiff. When inDiff = ["web/package-lock.json"] and all = ["web/package-lock.json", "api/package-lock.json"], the hint shows both, giving the judge visibility into why coverage was not claimed. This is the right information to surface.

Third branch (len(inDiff) == 0 && len(all) > 0) correctly uses all, which in this case contains only index-matched (unchanged) paths — semantically the same as the old matchingPacketPaths(hint, index) call.

No len(inDiff) > 0 && len(all) == 1 ambiguity. Since all ⊇ inDiff, len(all) == 1 can only occur when len(inDiff) == 0 or len(inDiff) == 1. The len == 1 && len == 1 branch handles the non-ambiguous single-match case before len(inDiff) > 0 is tested; the dead combination can't be reached.

FOLLOWUPS.md update narrows the pre-existing P3 description accurately: the unsatisfiable-oversized-file gap now only applies to a file with no same-named sibling, not to any uniquely-changed file. ✓

cmd/gate/README.md update accurately documents the new rule: "requires a changed file only when no other file shares that name." ✓


Findings

No new issues. All prior findings are fully addressed and the implementation is correct.


Overall this is solid. The sameNamePaths helper is a clean extraction, the uniqueness requirement is precisely specified, the new test covers the regression, and the prior tests remain valid under the changed semantics.

@github-actions

Copy link
Copy Markdown

Reviewer: claude
Reviewed commit: c110746274e8fcd582d00da86ac79956da2038cb

@itsHabib

Copy link
Copy Markdown
Owner Author

Review disposition at c110746274e8fcd582d00da86ac79956da2038cb (lane A takeover from the out-of-quota Codex constraints lane):

  • Independent exact-head review of c231da0 found two verified P1s in packet path resolution: (1) a bare, line-less token that matched a changed file was re-resolved against the recorded index to an unchanged root blob, so following the packet's own "run gate evidence" advice could make the run permanently unsatisfiable (a regression against the base); (2) a precise basename reference matching several changed files skipped the index and reported complete=true. Both fixed in 6dfeed1; each new test fails on c231da0.
  • Fresh re-review of 6dfeed1 found the bare-token fix could still strand a run when an unchanged same-named sibling exists (monorepo lockfiles). Fixed in c110746: a bare token requires a changed file only when it is the one path with that name; otherwise it is an ambiguous hint. An unchanged blob is never required.
  • Final re-review of c110746: PASS, no P0/P1. Residuals accepted as non-blocking: a missing test pinning that a uniquely named changed file stays required once an index exists (behaviour verified by probe; the test lands with fix(gate): share capacity across required evidence #343); a changed root file that shares its name with an unchanged nested file is now a hint for a bare mention (consistent with the stated rule; a bare token is not a precise reference).
  • Pre-existing gaps found by the same reviews (partial slash paths are exact-only; a non-path backtick span desynchronises the reference pattern; a uniquely named oversized changed file) are recorded in FOLLOWUPS.md.
  • Claude reviewed c110746: no new issues; attestation names this head. CI (check, fuzz, hygiene) green here. Locally: go test -race ./cmd/gate/..., vet and golangci-lint (0 issues) pass; saved Rooms feat(console): brief-led trace view — decision panel + verifier ladder #120 and RoxIQ feat(contracts/org): versioned canonical encoder and digest #246/feat(flare): page grant_needed, collapse the repeated question, stop claiming health #252 packets replay complete.

@itsHabib
itsHabib marked this pull request as ready for review September 14, 2026 04:56
@itsHabib
itsHabib merged commit c955394 into main Sep 14, 2026
3 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.

1 participant