Repository navigation
Judge each rule on the PRs it cites, with full upstream diffs - #237
Conversation
The L2 judge saw each merged PR as an 8 KB diff excerpt, and saw the whole
batch's evidence for every rule (about 105 KB for 10 PRs). In the first
live run, "the supplied diff ends before ..." was the most common reason
for an unsure or failed verdict.
- New kb_service/evidence.for_rule narrows the evidence to the PRs a rule
cites (^[PR #N]) and the PRs its lifecycle footer names as evidence (a
retirement's justification). When the rule cites none we hold, it keeps
all of them.
- It replaces each upstream merged-PR excerpt with the full per-file diffs
of the merged commit, read from the service's own upstream mirror (new
MirrorObserver.file_diff).
- Order: files the rule names first, then source, then tests, then docs
and configuration.
- Limits: 24 KB per file and 48 KB per rule. A cut is marked, and files
past the budget are listed in diffs_omitted.
- Anything the mirror cannot answer keeps its recorded excerpt. Only
upstream merged-PR evidence (with merged_at) is expanded.
- merged_pr events now record merge_commit_sha. Older events look it up
through pull().
- run_gate(evidence_for=...) is wired from gate_and_stage. The facts check
and the evidence expansion share one observer, so the mirror syncs once.
Measured with the real judge (codex gpt-6-sol, medium) on the 7 rules
disputed in the first live run:
recorded evidence: 0 pass, 3 fail, 4 unsure (3 of them "diff truncated")
full diffs: 1 pass, 5 fail, 1 unsure (a specific gap, no truncation)
- The 3 rules with real errors (EXEC-17a, SERV-SLP-1a, MOSSTTS-5a) fail
for the same reason in both arms.
- COSYVOICE3-1d and MOSSTTS-4c now fail on concrete scope errors that the
judge could see only in the full diff.
- SENSENOVA-1d passes, verified against the pipeline diff.
- SERV-VD-1 stays unsure for a specific, stated reason.
- Evidence per rule: about 11-57 KB instead of 105 KB.
A first version ordered files as GitHub lists them, so docs and tests used
up the budget before the adapter code (MOSSTTS-4c). Ranking source first
fixed that. Calibration cases carry no upstream evidence and are
unaffected.
Tests (test_kb_judge_evidence.py): only the cited PRs are shown; full
diffs replace the excerpt, named files first; the byte limits hold and say
so; source comes before tests and docs; an unreadable mirror keeps the
excerpt; older evidence asks for its merge SHA; run-lesson evidence is left
alone; each rule's judge prompt carries only its own PR; recorded evidence
carries merge_commit_sha.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3087322b44
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not number or "merged_at" not in item or budget <= 0: | ||
| out.append(item) # not an upstream merged PR (or no room left): as recorded |
There was a problem hiding this comment.
When the selected PRs consume PER_RULE, every remaining cited PR is appended with its original diff excerpt. A rule citing several large PRs can therefore include 48 KB of expanded diffs plus another 8 KB per remaining PR, defeating the stated per-rule limit and potentially recreating the oversized judge prompts this change is intended to prevent. Preserve only metadata and an omission marker once the budget is exhausted.
Useful? React with 👍 / 👎.
| sha = str(item.get("merge_commit_sha") or observer.pull(int(number.group(1))).get("merge_commit_sha") or "") | ||
| if not sha: | ||
| raise FactsError("no merge commit") | ||
| files = list(item.get("changed_files") or []) |
There was a problem hiding this comment.
For PRs changing more than 50 files, this list comes from PullRequest.evidence(), which truncates changed_files to MAX_CHANGED_PATHS. If a rule names a file after that cutoff, the new expansion cannot prioritize or retrieve its diff; once any earlier file produces a diff, the recorded excerpt is also removed, so the judge receives unrelated files while the relevant file is not even reported in diffs_omitted. Retain or obtain the complete changed-path list for expansion.
Useful? React with 👍 / 👎.
A path-filtered `git diff base head -- path` turns rename detection off. A renamed and edited file then showed up as all-new content, and the lines it removed were lost, so L2 could miss a behaviour removal. That is worse than the excerpt it replaces. MirrorObserver.pr_diff now lists the PR's files once with NUL-separated `--name-status -M -z` (renames paired; spaces and non-ASCII names come through exactly, no quote parsing), then diffs each file with both its old and new path in the pathspec, which keeps rename detection on. Test: with real git, a PR that renames a file and drops a function keeps "rename from", keeps the removed lines, does not show the file as all new, and is found under both names. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Follow-up commit 0ca0489: the first commit's review hit its iteration cap with REVISE (rename detection lost by path-filtered diffs). Fixed here: per-file PR diffs now keep rename history and exact spaced/non-ASCII paths (NUL-separated name-status + both paths in the pathspec). Hook review on this commit: APPROVE. |
The L2 judge saw each merged PR as an 8 KB diff excerpt, and saw the whole
batch's evidence for every rule (about 105 KB for 10 PRs). In the first
live run, "the supplied diff ends before ..." was the most common reason
for an unsure or failed verdict.
cites (^[PR #N]) and the PRs its lifecycle footer names as evidence (a
retirement's justification). When the rule cites none we hold, it keeps
all of them.
of the merged commit, read from the service's own upstream mirror (new
MirrorObserver.file_diff).
and configuration.
past the budget are listed in diffs_omitted.
upstream merged-PR evidence (with merged_at) is expanded.
through pull().
and the evidence expansion share one observer, so the mirror syncs once.
Measured with the real judge (codex gpt-6-sol, medium) on the 7 rules
disputed in the first live run:
recorded evidence: 0 pass, 3 fail, 4 unsure (3 of them "diff truncated")
full diffs: 1 pass, 5 fail, 1 unsure (a specific gap, no truncation)
for the same reason in both arms.
judge could see only in the full diff.
A first version ordered files as GitHub lists them, so docs and tests used
up the budget before the adapter code (MOSSTTS-4c). Ranking source first
fixed that. Calibration cases carry no upstream evidence and are
unaffected.
Tests (test_kb_judge_evidence.py): only the cited PRs are shown; full
diffs replace the excerpt, named files first; the byte limits hold and say
so; source comes before tests and docs; an unreadable mirror keeps the
excerpt; older evidence asks for its merge SHA; run-lesson evidence is left
alone; each rule's judge prompt carries only its own PR; recorded evidence
carries merge_commit_sha.
Commit hook (gpt-6-astra-high): APPROVE at iteration 3 (iterations 1-2 found the rebase-merge range and the dropped retirement evidence; both fixed and tested).
🤖 Generated with Claude Code