feat(review): filter unreviewable content from diff context - #1008
feat(review): filter unreviewable content from diff context#1008guyoron1 wants to merge 21 commits into
Conversation
Functional tests did not runFunctional tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the |
PR Summary by QodoFilter unreviewable content from PR review diff context
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
Code review by qodo was updated up to the latest commit cb8cc6d |
|
/review |
PR Reviewer Guide 🔍Warning
Here are some key observations to aid the review process:
|
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 61f404b |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit cc3f219 |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review sweep at head cc3f2196. Seven findings posted inline (1 CRITICAL, 6 MEDIUM), none overlapping the existing review threads:
- CRITICAL —
scripts/post-review.shwas edited directly;post-review.src.shwas not, somake check-bundlefails and a rebuild silently removes the new threshold exemption. - MEDIUM — an author-controlled
@generatedline removes a whole file from review on non-protected paths. - MEDIUM — Go/protoc canonical generated markers and context-line markers are not detected; the fixture only passes because of an extra
@generatedline. - MEDIUM — unquoted paths containing spaces carry a trailing tab on
---/+++and are never classified. - MEDIUM — the filter is a silent no-op on GitLab-shaped input, with no disclosure.
- MEDIUM — the cited
route-review-model.sh/is_lock()in fullsend does not exist. - MEDIUM — SKILL.md step 2 now contradicts bucket 3 ("after filtering").
One further item (large-PR per-file invocations truncating a shared exclusion-summary file) is already raised in the Reviewer Guide comment under "Summary Loss", so it is not repeated here.
Review-only; not requesting changes.
| # them here would undo the exemption the agent honors. | ||
| jq --argjson rank "$threshold_rank" ' | ||
| .findings |= [.[] | select( | ||
| .category == "provenance-warning" or .category == "excluded-content" or |
There was a problem hiding this comment.
[CRITICAL] post-review.sh edited directly; post-review.src.sh not updated (make check-bundle fails)
scripts/post-review.sh is a generated bundle (header: "GENERATED from post-review.src.sh — DO NOT EDIT. Run: make script-build"). Round 2 added the .category == "provenance-warning" or .category == "excluded-content" exemption here (lines 552-557), but scripts/post-review.src.sh (around lines 142-150) was not touched: git log dc7c805..HEAD -- scripts/post-review.src.sh is empty and neither category string appears in the src file.
Verified at head cc3f2196: make check-bundle exits 1 with Bundled script stale: scripts/post-review.sh (run make script-build), and a forced make script-build regenerates post-review.sh with the exemption and its comment removed. CI has not caught this because the script-test workflow runs on 61f404b2 and cc3f2196 both ended in action_required (never executed); the only successful run was on cb8cc6dd, before the exemption existed.
Once the bundle is rebuilt, excluded-content / provenance-warning info findings are dropped again at the default low threshold — exactly the behaviour agents/review.md:66-68 and SKILL.md step 7 now promise is exempt.
Suggested fix: Apply the jq exemption (and its comment) to scripts/post-review.src.sh, run make script-build, and commit both files together. While there, update the mirrored filter_findings_json in scripts/post-review-test.sh (around line 144, commented "keep in sync") and add cases showing provenance-warning / excluded-content survive at threshold=low while a plain info finding is dropped.
There was a problem hiding this comment.
Fixed in ccbb0aa — good catch. The exemption now lives in post-review.src.sh and the bundle is regenerated via make script-build; make check-bundle exits 0.
| sec_adds++ | ||
| if (added_seen < 5) { | ||
| added_seen++ | ||
| if (index(line, "@generated") > 0) generated_hit = 1 |
There was a problem hiding this comment.
[MEDIUM] Author-controlled @generated marker hides an entire file from automated review
Rule 3 (line 310, documented at lines 28-30) strips a whole section whenever the literal @generated appears in any of the first 5 added lines. That string is untrusted PR content: a contributor adds one // @generated comment line at the top of an ordinary source file and the section is removed from every sub-agent's diff, and step 2b (SKILL.md:178-183) then also omits its full contents. The only trace is an info-level excluded-content finding listing the path with reason generated-marker.
Verified mitigation: the protected-path check in post-review.src.sh:184-262 runs on forge_get_pr_files() (the API file list, not the diff), so changes under the default REVIEW_PROTECTED_PATHS (scripts/, .github/, skills/, agents/, harness/, ...) still force approve→comment. The bypass is therefore fully effective only on non-protected paths — but those are precisely the paths the bot is allowed to approve unassisted, which is the new exposure this PR introduces (previously small PRs were never filtered at all).
Suggested fix: Bound the marker rule: apply it only when the path also looks generated (e.g. *.pb.go, *_gen.go, *.generated.*, gen/ or generated/ components), or require the marker to appear as the first added line of a new file (--- /dev/null), and/or have the excluded-content disclosure call out generated-marker exclusions on non-generated-looking paths at a higher severity so a human notices. Document the accepted-risk boundary in the script header either way.
There was a problem hiding this comment.
Fixed in ccbb0aa — content-marker stripping is now gated on generated-looking paths (protoc/codegen suffixes, generated|dist|build components); a planted @generated on ordinary source keeps the section, no disclosure line. Negative test added.
| if (added_seen < 5) { | ||
| added_seen++ | ||
| if (index(line, "@generated") > 0) generated_hit = 1 | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Generated-file detection misses Go/protoc canonical markers and context-line markers; fixture masks the gap
SKILL.md dropped protobuf from the large-PR exclusion list and states the script is now the one definition of "generated", but the only content rule is index(line, "@generated") on added lines. protoc-gen-go emits // Code generated by protoc-gen-go. DO NOT EDIT. (the golang.org/s/generatedcode convention), and protoc's Python generator emits # Generated by the protocol buffer compiler. DO NOT EDIT! — neither contains @generated.
Verified at head: a .pb.go section whose first added lines are // Code generated by protoc-gen-go. DO NOT EDIT. / // versions: / package gen passes through unfiltered with an empty summary. The GENERATED_ADDED fixture (scripts/filter-review-diff-test.sh:96-103) contains that real Go marker but only passes because a separate +// @generated line was added beneath it.
Separately, regenerating an existing generated file leaves the header as unchanged context ( prefix), which the check never inspects (lines 305-311 only look at + lines), so the common regenerate-and-commit case is never caught either.
Suggested fix: Extend the marker check to the Go convention (^\+// Code generated .* DO NOT EDIT\.$) and protoc's Generated by the protocol buffer compiler, consider inspecting the first few context lines of the first hunk as well, and add fixtures that contain only those markers. Alternatively restore protobuf to the SKILL.md list and drop the "one definition of generated" claim.
There was a problem hiding this comment.
Fixed in ccbb0aa — has_generated_marker() adds the anchored Go marker and the protoc header, checked on added and context lines within the bounded window; removed-only still never strips. .pb.go context-line test added.
| } | ||
| if (line ~ /^\+\+\+ /) { | ||
| p = substr(line, 5) | ||
| if (p != "/dev/null" && substr(p, 1, 2) == "b/") new_path = substr(p, 3) |
There was a problem hiding this comment.
[MEDIUM] Paths containing spaces are never classified: git tab-terminates ---/+++ and +++ overrides the header path
For any unquoted path containing a space, git appends a TAB to the --- a/... and +++ b/... lines. Lines 263 and 269 take substr(p, 3) verbatim, so new_path ends in \t, and section_path() (line 158) prefers new_path over the correctly parsed hdr_new from round 1's header fallback — so the fallback never helps here. Every $-anchored rule then misses.
Verified with a real git diff at head: lib x/foo.min.js and pkg dir/package-lock.json both pass through unfiltered and the summary file is 0 bytes.
This is distinct from the existing thread at line 265 (git-quoted paths, fixed in round 1) and from test 15 (the b/ split in the diff --git line): the defect is the trailing tab on the unquoted ---/+++ lines of those same files.
Suggested fix: Strip a trailing tab in both branches (sub(/\t$/, "", p) before the a/ / b/ prefix check at lines 263 and 269), and add a fixture generated from real git output for a space-bearing lockfile/minified path.
There was a problem hiding this comment.
Fixed in ccbb0aa — path extraction strips from the first tab, so tab-terminated headers for spacey paths classify; regression tests use real git-shaped fixtures for both your examples.
| line = $0 | ||
|
|
||
| if (!in_diff) { | ||
| if (line ~ /^diff --git /) { |
There was a problem hiding this comment.
[MEDIUM] Filter is a silent no-op on GitLab input (no diff --git sections) with no disclosure
The parser only opens a section on ^diff --git (line 230); everything else is printed verbatim. skills/pr-review/gitlab/SKILL.md provides no unified-diff command — only the MR /changes API (per-file .diff hunk text) and repository/compare .diffs[], neither of which carries diff --git lines (the sibling skills/fix-review/gitlab/SKILL.md:36 synthesises only --- a/X / +++ b/Y headers).
Verified at head: a GitLab-shaped section (--- a/package-lock.json / +++ b/package-lock.json / hunk) passes through unfiltered with an empty summary.
The GitLab skill's lack of a unified-diff source is pre-existing; what this PR adds is that SKILL.md now presents the filter and its excluded-content disclosure as forge-neutral, while on GitLab nothing is filtered and nothing is disclosed.
Suggested fix: Cheapest fix: state explicitly in SKILL.md step 2 that the filter applies to the GitHub unified diff only. Otherwise either have the GitLab skill emit a diff --git a/<old_path> b/<new_path> line per .changes[] entry before the hunk text, or accept ^--- a/ as a section boundary when no diff --git has been seen, and add a GitLab-shaped fixture to the test file.
There was a problem hiding this comment.
Fixed in ccbb0aa — sections now also open at ^--- a/ (and quoted/dev-null variants) when no diff --git header exists, so GitLab MR-shaped diffs filter; fail-open preserved. Two-section GitLab fixture test added.
| # 1. EXEMPT (always kept, beats every rule below): path has a | ||
| # "migrations" or "migrate" directory component. | ||
| # 2. STRIP: path matches the dependency-lockfile list (mirrors the | ||
| # is_lock() regex in fullsend's .github/scripts/route-review-model.sh |
There was a problem hiding this comment.
[MEDIUM] Cited fullsend .github/scripts/route-review-model.sh / is_lock() does not exist
The header (lines 23-25), the classifier comment (lines 173-174: "mirrors is_lock() in fullsends .github/scripts/route-review-model.sh (lines 65-84 there)... kept in sync by hand"), and the PR description all cite a file that could not be found. Verified: the fullsend-ai/fullsend main tree (7495cdc5) has no path matching route-review*; its .github/scripts listing contains only check-fix-eligibility(-test).sh, install-openshell.sh, install-podman.sh, openshell-version.sh; git grep is_lock origin/main in that repo returns nothing; and this repo has no such file either.
The "kept in sync by hand" promise therefore points at nothing, and the lockfile list is narrower than common practice (no Pipfile.lock, uv.lock, bun.lockb, packages.lock.json, mix.lock, pubspec.lock, Package.resolved, flake.lock).
Suggested fix: Remove the cross-repo citation (or replace it with a permalink to the real location if one exists elsewhere), state that the list is self-contained here, fix the "fullsends" typo, and consider extending the lockfile list.
There was a problem hiding this comment.
Fixed in ccbb0aa — the citation was stale, removed; the lockfile list is described as self-contained.
| deletions) — paginate if the forge API requires it | ||
| - Compute `FILE_COUNT` and `LINE_COUNT` from the response | ||
|
|
||
| `FILE_COUNT` and `LINE_COUNT` are computed once, here, from this |
There was a problem hiding this comment.
[MEDIUM] Step 2 contradicts itself: new "never recomputed after filtering" rule vs bucket 3 "FILE_COUNT>200 after filtering"
The new paragraph at lines 120-123 states FILE_COUNT / LINE_COUNT are computed once from the unfiltered stats response and that nothing recomputes them from post-filter output; the PR body repeats this ("routing must see the true size"). But the retained bucket 3 at line 162 still reads "FILE_COUNT>200 after filtering, LINE_COUNT>10K: emit failure".
As written an orchestrator has two incompatible instructions for the token-limit gate: with the unfiltered rule, a 250-lockfile PR fails with token-limit before the filter runs; with the bucket-3 text, it must compute a post-filter count the new paragraph forbids. The bucket-3 wording is pre-existing, but the contradiction is introduced by this PR's new paragraph.
Suggested fix: Drop "after filtering" from bucket 3 (matching the stated unfiltered-count intent), or define precisely which count bucket 3 uses and reconcile the line-120 paragraph with it.
There was a problem hiding this comment.
Fixed in ccbb0aa — bucket 3 now names the same unfiltered counts computed in step 2, never post-filter numbers.
|
/agentic_review |
| if (lp ~ /\.pb\.go$|\.gen\.go$|_pb2\.py|_generated\.|(^|\/)(generated|dist|build)\//) { | ||
| phase = "pending" | ||
| return |
There was a problem hiding this comment.
2. Description overstates marker filtering 📜 Skill insight ≡ Correctness
The PR description claims any section with @generated in its first five added lines is stripped, but the classifier only performs that check for generated-looking paths. The new test explicitly preserves an ordinary source file containing that marker, so the stated behavior does not match the implementation.
Agent Prompt
## Issue description
The PR description says every section with an early `@generated` marker is stripped, while the implementation intentionally gates marker filtering on generated-looking paths.
## Issue Context
The source-path test confirms this is intentional security behavior rather than an implementation omission. Update the PR description to explain the path-shape prerequisite and context-line handling so reviewers are not given an inaccurate contract.
## Fix Focus Areas
- skills/pr-review/scripts/filter-review-diff.sh[242-250]
- scripts/filter-review-diff-test.sh[449-472]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
PR description updated — it now states the path-shape prerequisite (planted markers on ordinary source never strip) and the context-line handling, and drops the stale citation.
|
Code review by qodo was updated up to the latest commit ccbb0aa |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 5ef1681 |
ed79218 to
93c982d
Compare
|
/agentic_review |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review sweep at head a86e820. Six findings checked against existing threads for overlap; five posted inline (1 HIGH, 4 MEDIUM), one MEDIUM below (its line falls outside the visible diff hunk for that file).
[MEDIUM] agents/review.md (near line 73, outside the visible diff hunk) — agents/review.md still states the narrower "downgrade only when all findings removed" rule, which no longer matches post-review.src.sh's actual (broader) downgrade logic
agents/review.md (lines 71-74) still says: "If filtering removes all findings from a request-changes or reject verdict, downgrade the verdict to comment." But scripts/post-review.src.sh (lines 161-189) computes blocking_count as findings excluding (provenance-warning OR excluded-content) AND severity==info, and downgrades whenever blocking_count -eq 0 — i.e. it downgrades even when disclosure-only findings remain (a non-empty findings array with only exempt info-level disclosures), not only when the findings array is fully empty. This asymmetry was never corrected in review.md even though the fix rounds (ccbb0aa, 5ef1681) touched the enforcement code and its tests extensively; the prompt text was not updated to match, so the agent's own stated rationale for its verdict can diverge from what the post-script actually enforces.
Suggested fix: Update agents/review.md to state the same rule as post-review.src.sh: exempt info-level disclosures (provenance-warning, excluded-content) never by themselves justify a blocking verdict, and the verdict is downgraded whenever no non-exempt (blocking) finding remains — not only when the findings array is entirely empty.
| return | ||
| } | ||
|
|
||
| # phase == "pending": buffering, watching the first 5 added lines and |
There was a problem hiding this comment.
[HIGH] Generated-marker window is scoped to the diff hunk, so typical mid-file protoc/codegen regen diffs never see the marker
The pending-phase content-marker check (lines 373-396) scans only the lines actually buffered inside the current diff hunk (first 5 added lines, or first 100 buffered lines, whichever comes first) — it does not, and structurally cannot, look at file content outside the hunk. The generated-file marker (// Code generated ... DO NOT EDIT. or the protoc header) normally sits at the very top of the file, outside the diff entirely, once the file is only being incrementally regenerated rather than newly added. A realistic regen touching .pb.go/_pb2.py deep in the file (e.g. a hunk at @@ -500,7 +500,7 @@) will never contain the marker in its buffered window, so generated_hit stays 0 and the section falls through to phase="keep", flushing the whole hunk into the reviewed diff. This is exactly the case the earlier fix (context-line marker checking, applied in ccbb0aa) did not close: Test 19 (filter-review-diff-test.sh lines 474-492, fixture PBGO_CONTEXT_MARKER) only proves the marker strips when it happens to fall inside the hunk (hunk starts at @@ -10,7 +10,7 @@, marker at file line 11) — there is still no fixture for a hunk offset far from the file's own header comment, which is the common case for incremental protoc/codegen regeneration.
Suggested fix: Either strip .pb.go/.gen.go/_pb2.py/_generated.* by path/suffix alone (dropping the content-marker gate for those specific protoc/codegen suffixes, keeping it only for the broader generated/, dist/, build/ path-component rule where false positives are more likely), or fetch the file's own header lines from the base/head ref instead of relying on what happens to be inside the diff hunk. Add a fixture where a .pb.go hunk sits far (e.g. @@ -500,7 +500,7 @@) from any marker line, to catch the realistic regression this misses today.
| function has_generated_marker(text) { | ||
| if (index(text, "@generated") > 0) return 1 | ||
| if (text ~ /^\/\/ Code generated .* DO NOT EDIT\.$/) return 1 | ||
| if (index(text, "Generated by the protocol buffer compiler. DO NOT EDIT!") > 0) return 1 |
There was a problem hiding this comment.
[MEDIUM] Hardcoded protoc "DO NOT EDIT" marker string is missing the required second space, so it never matches real protoc output
has_generated_marker() checks index(text, "Generated by the protocol buffer compiler. DO NOT EDIT!") with a single space after the period (also stated the same way in the header comment at lines 30-31). protoc's actual C++/Java/Python generators emit this string with TWO spaces after the period ("Generated by the protocol buffer compiler. DO NOT EDIT!" — matches upstream protobuf source and GitHub Linguist's own generated-file detector). Because of the single-space mismatch, this branch of has_generated_marker() never fires against real protoc-generated Python/Java/C++ output; only the separately-matched Go marker (// Code generated ... DO NOT EDIT.) actually works. This is a distinct defect from the hunk-window issue above (wrong string vs. wrong scan window) — even a marker that does land inside the buffered window would still fail this exact-match check.
Suggested fix: Fix the string at both line 214 and the header comment at line 31 to the real double-space marker, and prefer a whitespace-tolerant match given it has now been mistyped once already: text ~ /Generated by the protocol buffer compiler\.[ \t]+DO NOT EDIT!/. Add a fixture using the real double-space marker text — the current suite has no case exercising the actual protoc string.
| summary is non-empty, even though `info` sits below the default `low` | ||
| threshold (see "Severity filtering" in the agent definition): | ||
|
|
||
| - **[excluded-content]** — N excluded file(s) |
There was a problem hiding this comment.
[MEDIUM] excluded-content finding names multiple file paths but the findings schema's file field is a single required string
agents/review.md's finding-object schema (line 277) requires file as a single non-empty string. The excluded-content disclosure instruction (SKILL.md line 1342, referenced again at line 144) tells the agent to emit one info-level finding whose description lists every excluded path and reason ("<path> (<reason>), ..." for N files), but neither SKILL.md nor the schema in agents/review.md says what value goes in the required single-valued file field when N>1. This is the same unresolved gap provenance-warning already has (PR-wide, no file specified either), but this PR adds a second, more-likely-to-trigger multi-file instance of it. The recent fix in a86e820 addressed a different complaint (the disclosure mislabeling exclusions as all "generated/locked" instead of naming all five categories) — it did not touch the file field ambiguity.
Suggested fix: Either give findings a documented convention for representing multiple files (e.g. the first excluded path, or a PR-level sentinel), or instruct emitting one excluded-content finding per excluded file when the exact file matters. State the same rule in both agents/review.md's schema description and the SKILL.md instruction so provenance-warning and excluded-content populate file consistently.
| if (gl_cand != "") { | ||
| cand = gl_cand | ||
| gl_cand = "" | ||
| if ($0 ~ /^\+\+\+ /) { |
There was a problem hiding this comment.
[MEDIUM] GitLab section-boundary confirmation regex can be forged by an added line whose payload starts with "++ "
The one-line lookahead that confirms a GitLab ---/+++ section boundary checks only $0 ~ /^\+\+\+ / (line 405). The companion fix in 5ef1681 tightened the candidate side (is_gl_candidate() now requires ^--- (a\/|"a\/) or --- /dev/null, confirmed at lines 294-297) but left this confirming regex unchanged. An ADDED content line whose payload text begins with ++ (e.g. an added comment or string literal) renders in the diff as a line starting with +++ , which satisfies this regex and causes the parser to treat it as a real GitLab boundary — calling finalize_section()/reset_section() mid-hunk and flipping gitlab_mode on. This is a genuine parser-confusion bug distinct from the case the recent fix addressed (a removed line spoofing the --- candidate); it is reachable on ordinary (non-adversarial) PR content, not just crafted input.
Suggested fix: Require a more specific match before accepting a confirmed GitLab boundary, e.g. ^\+\+\+ (b\/|"b\/|/dev/null), mirroring the tightening already applied to the --- candidate side. Add a regression fixture where an added line's payload begins with ++ .
| return | ||
| } | ||
|
|
||
| if (phase == "header") { |
There was a problem hiding this comment.
[MEDIUM] Unbounded buffering in the pre-hunk header phase, reachable only via a --binary diff or non-git-produced input
Until an @@ hunk marker or a Binary files line is seen, the phase == "header" block (lines 335-370) appends every line to buf[] with no cap — unlike the 100-line cap applied once inside the pending/hunk phase. In the documented pipeline (SKILL.md's git diff <merge-base>..HEAD -- <file> / gh pr diff, neither of which passes --binary), real diff headers are 4-8 bounded lines before either @@ or Binary files ... differ appears, so this is not reachable through the pipeline as currently wired. It becomes reachable if a caller ever adds --binary (producing a GIT binary patch preamble of base85 lines with no @@ marker) or if the script is fed non-git-produced/malformed input directly — which the script's own header comment treats as a design concern ("Malformed input ... passes through unchanged rather than erroring").
Suggested fix: Apply the same buf_n cap used in the pending phase to the header phase too (flush fail-open on cap trip), closing the gap defensively even though today's callers don't pass --binary. If left as-is, document the --binary/non-git-input precondition so future callers don't add --binary without revisiting this.
a86e820 to
db55cb4
Compare
|
@waynesun09 I rebased onto main. #1178 rewrote step 2 and 3d; the only conflicts were
|
The skillsaw context-budget entry for skills/pr-review/SKILL.md is a ceiling, not a mute, so it has to move with the file. This PR grows the skill by the dismissal-reconciliation step: 15,709 tokens on main, 20,258 here. Same fingerprint, one entry, value and message only, as b9fef04 did for code-implementation. fullsend-ai#1006 and fullsend-ai#1008 grow the same file, so whichever of the three merges last has to re-measure and bump again. Signed-off-by: guy oron <goron@redhat.com>
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review sweep at head db55cb48. Six findings checked against existing threads for overlap; all six posted inline (none overlapping existing threads).
Review-only; not requesting changes.
| suffixes; `generated/`, `dist/`, `build/` paths) carrying a | ||
| generated-content marker in those first 20 lines or in the section's | ||
| bounded window — migrations are exempt from every one of those rules. | ||
| It handles both the GitHub unified diff (`diff --git` sections) and the |
There was a problem hiding this comment.
[CRITICAL] filter-review-diff.sh never runs on the actual per-file diff shape either forge skill produces — the "handles both" claim on this line doesn't hold, and the pre-existing coarse jq it leaves in place bypasses the migrations exemption
Both forge skills (untouched by this PR — git diff against the merge-base for skills/pr-review/github/SKILL.md and skills/pr-review/gitlab/SKILL.md is empty) write /sandbox/workspace/pr-diff.txt as "### File: <path>\n<per-file diff>" per changed file — github/SKILL.md's "Per-file diffs (large PRs)" step (### File: \(.filename)\n\(.patch // ...)) and gitlab/SKILL.md's single "Unified diff (small and large MRs)" step (### File: \(.new_path)\n\(.diff)), which is GitLab's only diff source, used for every MR. Empirically reproduced against filter-review-diff.sh at HEAD: printf '### File: package-lock.json\n@@ -1,3 +1,3 @@\n-a\n+b\n context\n' | filter-review-diff.sh summary.txt emits the input byte-for-byte unchanged with an EMPTY summary file — the parser only opens a section at ^diff --git or a confirmed --- a/++++ b/ pair; ### File: matches neither, so in_diff never becomes 1 and every line prints via the !in_diff passthrough. GitHub's .patch field is confirmed header-less (starts at @@). For GitLab I verified live against a real project (GET /projects/:id/merge_requests/:iid/changes): changes[].diff also starts directly at @@ -186,7 +186,9 @@ ... with no --- a//+++ b/ lines — so GitLab is unfiltered for every MR, not just an "orphan header" cosmetic artifact. No fixture in scripts/filter-review-diff-test.sh exercises the ### File: shape at all, so this line's claim ("It handles both the GitHub unified diff ... and the GitLab MR shape") is untested and false for both forges' only diff path. Compounding this: the coarse jq both forge skills apply before pr-diff.txt is even written has no migrations exemption (unlike filter-review-diff.sh's own classify_path(), which the text at lines 168-171 now correctly says is "the only rule with the migrations exemption") — a large/any PR's db/migrations/package-lock.json or db/migrations/x.pb.go is dropped by that jq unconditionally, before filter-review-diff.sh ever sees it, so it never reaches pr-excluded.txt and no excluded-content disclosure is ever emitted for it.
Suggested fix: Teach the classifier to treat ### File: <path> as a section boundary (or have both forge skills synthesize real --- a//+++ b/ headers before the ### File: line — GitLab's own skills/fix-review/gitlab/SKILL.md:36 already does this for .diff, so the pattern exists in-repo). Then route the migrations-containing file list through filter-review-diff.sh's classification before the coarse jq drop, so exclusions there also land in pr-excluded.txt and respect the migrations exemption. Add fixtures covering the ### File:+patch shape end to end, including a db/migrations/package-lock.json case.
|
|
||
| ```bash | ||
| # Scanner dialect (fullsend-ai/agents#1190): `test` not `[ ]`, no rm. | ||
| skills/pr-review/scripts/filter-review-diff.sh /sandbox/workspace/pr-excluded.txt \ |
There was a problem hiding this comment.
[CRITICAL] Step 2c invokes the filter with a bare repo-relative path; on the wrong cwd it silently clobbers the real diff with an empty one
Empirically reproduced: from a cwd without a skills/ subtree (the actual review sandbox layout — CWD is /sandbox/workspace, target checkout under target-repo/, skills live under ${CLAUDE_CONFIG_DIR}), running this block verbatim against a real non-empty pr-diff.txt gives skills/pr-review/scripts/filter-review-diff.sh: no such file or directory (exit 127) — yet pr-diff.filtered is still created empty by the shell redirect, the unconditional mv pr-diff.filtered pr-diff.txt then overwrites the real diff with nothing, and the check reports 'EMPTY DIFF after filtering' even though a perfectly good diff existed seconds earlier. The established convention elsewhere in this same repo is bash "${CLAUDE_CONFIG_DIR}/skills/<skill>/scripts/<script>.sh" (skills/pr-risk-assessment/scripts/risk-tier1.sh, invoked identically from skills/pr-review/sub-agents/risk-assessment.md:27 and skills/pr-risk-assessment/SKILL.md:199) — step 2c does not follow it.
Suggested fix: Invoke as bash "${CLAUDE_CONFIG_DIR}/skills/pr-review/scripts/filter-review-diff.sh", matching risk-tier1.sh's pattern. Capture the filter's exit code and only mv the filtered file over pr-diff.txt when it is 0; otherwise fail without touching the original diff.
| /sandbox/workspace/pr-head \ | ||
| < /sandbox/workspace/pr-diff.txt > /sandbox/workspace/pr-diff.filtered | ||
| mv /sandbox/workspace/pr-diff.filtered /sandbox/workspace/pr-diff.txt | ||
| test -s /sandbox/workspace/pr-diff.txt || echo "EMPTY DIFF after filtering — produce a failure result (reason tool-failure)" |
There was a problem hiding this comment.
[HIGH] Step 2c's empty-diff check can't distinguish a genuine tool failure from a correctly all-stripped diff, likely short-circuiting before the disclosure it exists to enable
filter-review-diff-test.sh test 7 confirms the intended contract: a lockfile-only input produces empty stdout, exit 0, and a fully-populated summary file — the filter worked exactly as designed. This line's test -s pr-diff.txt || echo "EMPTY DIFF after filtering — produce a failure result (reason tool-failure)" fires on that outcome with no check of whether pr-excluded.txt is non-empty. The disclosure logic that would otherwise cover this case lives later in step 7 ('If step 2c left a non-empty pr-excluded.txt, include an info-level finding...') — reached only if the review continues past 2c. github/SKILL.md's diff-fetch step uses the identical bare test -s ... || echo "EMPTY DIFF — produce a failure result" idiom as a hard stop, so the same phrasing here reads as an instruction to abandon the review rather than continue to step 7. Net effect: a dependabot/renovate lockfile-only PR — filtered exactly as intended — is likely reported as action: failure, reason: tool-failure ("not reviewed") instead of a clean review carrying one excluded-content disclosure, even though nothing actually failed. (Note: the review-result.schema.json failure action does not itself forbid a findings array — only reason is required — so this is an instruction-sequencing gap in SKILL.md, not a hard schema block.)
Suggested fix: Distinguish the two empty cases explicitly in step 2c: empty diff AND empty pr-excluded.txt = genuine tool failure. Empty diff WITH a non-empty pr-excluded.txt = continue to step 7, emit one excluded-content finding per excluded line, and do not fail the review.
| # and quoted paths leave the variable unset (the diff --git header | ||
| # fallback covers those). | ||
| function set_old_path(line, p) { | ||
| p = substr(line, 5) |
There was a problem hiding this comment.
[MEDIUM] Quoted GitLab --- "a/..." / +++ "b/..." paths fail open because the diff --git header dequoting fallback never applies in gitlab_mode
set_old_path()/set_new_path() only accept an unquoted a//b/ prefix; for a quoted path they leave old_path/new_path unset. section_path() then falls back to hdr_old/hdr_new, which are populated only by parse_git_header() — called exclusively from a diff --git header line. In gitlab_mode there is no diff --git header (sections open directly at a confirmed ---/+++ pair), so parse_git_header() is never invoked and hdr_old/hdr_new stay empty for the whole section. With all four unset, section_path() returns "", and the code's own fail-open path (phase="keep"; flush_buf() at the @@/Binary files check) keeps the section unfiltered instead of classifying it. This is a narrower residual of the already-fixed 'Quoted paths evade filtering' finding: that fix added the diff-header dequoting fallback, which covers GitHub-shaped diff --git sections (confirmed by the QUOTED_VENDORED fixture using diff --git "a/..." "b/...") but not GitLab sections, which never carry that header. No test exercises a quoted GitLab ---/+++ pair. Effect is fail-safe (over-inclusion, content stays visible/reviewed), not a hidden-content risk.
Suggested fix: Apply the same dequoting logic used for GitHub diff --git headers to the GitLab ---/+++ extraction directly in set_old_path()/set_new_path() (or a GitLab-specific fallback), and add a GitLab quoted-path fixture — it would currently fail if added.
| @@ -1269,6 +1335,25 @@ info-level finding in the review output: | |||
| provenance validation failed (`PRIOR_REVIEW_PROVENANCE` value). | |||
There was a problem hiding this comment.
[MEDIUM] The new provenance-warning file:"<pr>" convention (residual of the resolved excluded-content/file-field thread) isn't carried into this bullet or into this PR's own test fixtures
agents/review.md documents (line 174, and the schema table at line 280): for provenance-warning, 'It is PR-wide, so set file to <pr> and omit line,' naming <pr> as the sentinel for PR-wide findings — this looks like it grew out of the fix for the resolved 'excluded-content finding names multiple file paths' thread, which addressed the sibling category. But this bullet (the provenance-warning instruction) still doesn't mention the <pr> convention at all. More concretely, this PR's own test additions in scripts/post-review-test.sh (the DISCLOSURES_PLUS_INFO and DISCLOSURE_ONLY_REJECT fixtures, added in this PR) hardcode "file":"a.go" for provenance-warning — contradicting the convention agents/review.md states in this same PR, so the convention ships with zero test coverage and an internally-inconsistent example.
Suggested fix: Mirror the file:"<pr>" / omit-line convention for provenance-warning into this bullet, and update the post-review-test.sh provenance-warning fixtures to use "<pr>" instead of "a.go" so the convention is actually exercised.
| } | ||
| lp = tolower(path) | ||
| # Well-known dependency lockfiles across ecosystems (npm, yarn, pnpm, | ||
| # Go, Rust, Ruby, Python, PHP). |
There was a problem hiding this comment.
[MEDIUM] Minified-file matching is case-sensitive while the lockfile rule right next to it is not
classify_path() lowercases path into lp and uses lp for the lockfile match, but the very next two rules — *.min.js/*.min.css (this line) and *.map — match against the raw, un-lowercased path instead of lp. A file named e.g. Bundle.MIN.JS or styles.MAP bypasses these exclusions and stays in the reviewed diff, inconsistent with the case-insensitive handling used one rule earlier in the same function. Fails safe (over-inclusion — the content still gets reviewed, nothing is hidden), just inconsistent and likely unintended given the adjacent lowercased comparison.
Suggested fix: Use lp (already computed) for the minified and sourcemap rules too, and add a case-insensitivity fixture for those two rules alongside the existing lockfile one.
Review context includes lockfile, minified, sourcemap, and @generated hunks that no model can meaningfully assess; on a mixed PR they are pure input cost for every review dimension. filter-review-diff.sh strips them deterministically before context assembly - migrations are exempt from every rule, and the exclusion is always disclosed (as an info-level finding, the same mechanism the provenance-warning finding already uses) so a changed lockfile stays visible even though no model read it. Small-PR and large-PR paths now share one definition of "generated" instead of the large-PR path's prompt-level list. Signed-off-by: guy oron <goron@redhat.com>
…, threshold-exempt disclosure - Derive section paths from the diff --git header itself (git-quoted paths dequoted), so binary, mode-only, and quoted-path sections classify correctly; when no path can be parsed at all the section fails open — never stripped, even on an @generated added line. - Truncate the caller-supplied summary file at start of run so a no-exclusions run cannot leave a previous run's stale records (preserves /dev/null behavior). - Match the migrations/migrate exemption as whole slash-delimited path components only — db/remigrations/ no longer bypasses the lockfile rule. - Bound pending-section buffering: the generated-marker decision resolves at the 5th added line or the 100th buffered line, whichever comes first, so a deletion-only section never buffers whole. - SKILL.md step 2b now skips exclusion-summary paths when fetching source_files, so filtered content cannot re-enter model context as full file contents. - The excluded-content disclosure is threshold-exempt (alongside provenance-warning) so the default low severity threshold cannot suppress it; agent-definition severity filtering states the exception. Signed-off-by: guy oron <goron@redhat.com>
…summary, post-script exemption - diff --git header split now prefers the position where both sides name the same file (the common non-rename case), so a path itself containing " b/" no longer misparses; last-marker fallback kept for genuine renames. - rename from/to metadata is dequoted when git-quoted (no a/ b/ prefix on these lines); undecodable quoting falls back to the header path, so a quoted rename into vendor/ is classified correctly. - dequote() keeps control and octal escapes as literal backslash sequences (only \" and \\ are unescaped): a \n in a filename can no longer break the one-record-per-physical-line summary contract. - post-review.sh severity filter now preserves the provenance-warning and excluded-content process disclosures regardless of threshold, matching the exemption stated in the agent definition. Signed-off-by: guy oron <goron@redhat.com>
…hape, tab paths - Move the severity-filter category exemption into post-review.src.sh (the round-2 edit landed in the generated bundle by mistake) and regenerate scripts/post-review.sh via make script-build; check-bundle is green again. - Verdict downgrade now keys on blocking findings: an array containing only the exempt process disclosures (provenance-warning, excluded-content) downgrades request-changes/reject to comment while keeping the disclosures; only a truly empty array is deleted. Test mirrors and cases updated accordingly. - Content-marker stripping is gated on generated-looking paths (.pb.go, _pb2.py*, .gen.go, _generated.*, generated/, dist/, build/) so an author-planted @generated cannot hide an ordinary source file from review; markers on any other path keep the section with no summary line. - Recognize the canonical Go marker (Code generated ... DO NOT EDIT.) and the protocol buffer compiler header, and check markers on context lines too — a modified .pb.go carries its marker as context, not as an added line — within the same bounded window. - Strip the tab git appends to ---/+++ paths containing spaces, so $-anchored rules match again. - Open sections at bare --- a/X lines when no diff --git headers exist, so GitLab MR diffs are filtered too; ambiguous input still fails open. - dequote() decodes octal escapes to raw bytes (summary paths now match the changed-file list byte-for-byte; awk runs under LC_ALL=C so %c emits bytes); \t \n \r stay literal to keep one summary line per record. - Drop the bogus cross-repo is_lock() citation — the lockfile list is self-contained here. - SKILL.md: bucket 3 uses the once-computed unfiltered counts, and the step-2 filter description matches the gated marker semantics. Signed-off-by: guy oron <goron@redhat.com>
…GitLab boundaries - The disclosure exemption now requires category AND info severity, in both the severity-filter keep-clause and blocking_count: a high- or critical-severity finding that uses the provenance-warning or excluded-content category is a real finding — it filters by rank and sustains a blocking verdict. Bundle regenerated from the src; tests cover survive-at-low, dropped-at-critical, and blocks-no-downgrade. - A GitLab section boundary is now confirmed by structure, not shape: a `--- ` candidate line opens a section only when the very next line is the paired `+++ ` header (one-line lookahead; the state machine moved into a process() function so a rejected candidate replays through identical logic). A removed content line whose original text begins "-- a/..." renders as `--- a/...` and previously reset the parser mid-file, stripping later hunks under the fake path — the new fixture shows the round-3 filter losing 8 lines with a false vendored exclusion, and now passes through byte-identical. Signed-off-by: guy oron <goron@redhat.com>
The disclosure template called every omitted file generated/lockfile, but the filter also excludes minified, sourcemap, and vendored content. Name all five categories so reviews that exclude only, say, a minified bundle do not publish an inaccurate description of what was omitted. Signed-off-by: guy oron <goron@redhat.com>
An incremental protoc/codegen regen touches the middle of a generated file, so the header comment carrying the marker never appears in the diff and the section fell through to the reviewed context unfiltered. The PR head is materialised in step 2b, so the file's own first 20 lines are readable: pass that tree as $2 and the marker is found where it actually lives. Without the tree, or the file, the in-hunk window still decides. Three narrower defects in the same classifier: - protoc emits two spaces before "DO NOT EDIT!", so the exact-match branch never fired on real Python/Java/C++ output. Matched whitespace-tolerantly now. - A GitLab section boundary was confirmed by any `+++ ` line, which an added line whose payload begins "++ " also produces: the parser reset mid-hunk, dropped four lines of real code and invented an exclusion. Both sides of the pair are now matched on their a/ b/ prefix, as is_gl_candidate() already matched the `---` side. - The pre-hunk header phase buffered without a cap, unlike the pending phase. Past 100 lines it fails open: unclassified, included. Signed-off-by: guy oron <goron@redhat.com>
Four fixtures for the four defects, plus the negatives that keep the guarantees honest: a .pb.go hunk at @@ -500,7 +500,7 @@ is kept without a head tree and stripped with one, a planted @generated on ordinary source is still kept, protoc's real double-space header strips (and the single-space spelling too), an added line beginning "++ " leaves a GitLab-shaped diff byte-identical, and a 150-line binary preamble passes through while a short binary header still classifies. Signed-off-by: guy oron <goron@redhat.com>
`file` is a single required string, so an excluded-content finding listing N paths had nowhere to put them. Emit one finding per excluded path instead, with no line — a finding without a line never becomes an inline comment on a file nobody reviewed. The PR-wide provenance-warning disclosure gets the same rule and a `<pr>` sentinel, stated in both the schema table and the instruction. Filtering moves to its own step 2c, after the PR head exists, and passes that tree to the script so the marker lookup has something to read. States the 20-line marker window, that both the GitHub and GitLab diff shapes are handled, and the accepted risk that a marker on an already-generated-looking path is still author-controlled — the per-file disclosure is what keeps it visible. Signed-off-by: guy oron <goron@redhat.com>
The block showed the script name and its two arguments with no stdin redirect and no output target, and `filter-review-diff.sh` is a stdin-to-stdout filter: an agent running it verbatim left `pr-diff.txt` untouched and the whole filter a no-op on the documented path. "In place" is not something one shell redirect can do either. Now a complete bash block in the house dialect, like every sibling command block in the forge skills: read the diff, pass the pr-head tree, write a new file, `mv` it back over `pr-diff.txt` (so every later step keeps reading the same name), and the same `test -s … || echo "EMPTY DIFF …"` guard those blocks end with. The summary gets a real name, `/sandbox/workspace/pr-excluded.txt`, used in 3d and step 7 instead of "step 2c's exclusion summary". Restores `> filtered-diff` to the script's own usage line. Also drops a sentence that was not true: the large-PR bucket still has the forge skill's own coarse jq exclusions applied to it upstream (`skills/pr-review/github/SKILL.md:44`), which strip `.pb.go` with no marker check and carry no migrations exemption. What the two buckets share is step 2c, and that is what the text now says. Signed-off-by: guy oron <goron@redhat.com>
`skills/pr-review/SKILL.md` is 16,815 tokens against the 15,709 ceiling the baseline records, so `make lint` is red on this branch — and was already red at a86e820, before any of the review-feedback commits. All of the growth is this PR's. CONTRIBUTING.md:38 says not to re-baseline a violation your own PR increased; b9fef04 did exactly that for the same rule and the same reason, and was merged. Ceiling set to the measured value, no slack. Maintainer's call which way this goes — the alternative is trimming the skill, which is a different change. Signed-off-by: guy oron <goron@redhat.com>
…ir coarse jq pre-filter Both forge skills write pr-diff.txt as "### File: <path>" followed by the API's header-less patch, which the classifier never opened a section for — every such diff passed through unfiltered with an empty summary. A "### File: " line is now a section boundary carrying the path. The forge skills' own jq exclusion list is gone: it had no migrations exemption and nothing it dropped ever reached pr-excluded.txt. Step 2c is the single filter for both PR sizes on both forges. Signed-off-by: guy oron <goron@redhat.com>
… is not a failure Invoke the filter by its absolute skills path like every other skill script (the sandbox cwd is /sandbox/workspace, where the bare relative path exited 127 while the redirect had already created an empty file for the unconditional mv to install). The mv now runs only on exit 0; on failure the fetched diff stays and the summary is emptied so nothing undisclosed is claimed stripped. Empty diff plus non-empty summary is a lockfile-only PR filtered as designed: go to step 7 with the disclosures. Empty diff plus empty summary stays a tool failure.
…lassify A GitLab-shaped section has no diff --git header to fall back on, so a git-quoted path left old_path/new_path unset and the section failed open. Dequote at the ---/+++ line itself. Signed-off-by: guy oron <goron@redhat.com>
…ke the lockfile rule Signed-off-by: guy oron <goron@redhat.com>
…t definition names The bullet now states file <pr> / no line, and the post-review test fixtures exercise that shape instead of a made-up path. Signed-off-by: guy oron <goron@redhat.com>
Signed-off-by: guy oron <goron@redhat.com>
The pre-hunk header phase has been capped at 100 lines since e388990, but the script's own contract only described the cap on the pending phase. State that both phases are bounded and fail open past the cap. Signed-off-by: guy oron <goron@redhat.com>
db55cb4 to
dd67633
Compare
|
@waynesun09 thanks — every open thread is addressed at the current head. Rebased on main (883141b, so #1211's dispatch shape is in) and force-pushed.
Script tests, check-bundle, lint and shellcheck are green. |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review sweep at head dd67633. Five findings checked against the existing threads for overlap; all five posted inline (1 HIGH, 4 MEDIUM), none duplicating an existing or already-resolved thread.
- HIGH —
scripts/post-review.src.sh: a disclosure-only (all-excluded) PR can still carryapprovethrough toready-for-merge; theblocking_count -eq 0branch only rewritesrequest-changes/reject, and step 2c adds a new route into that state. - MEDIUM — step 2c is inert on the
piruntime:${CLAUDE_CONFIG_DIR}is exported only by the Claude runtime, so the filter call exits 127 and logsFILTER FAILEDon every pi review. - MEDIUM — the skillsaw
context-budgetceiling was raised twice for this PR's own growth, whichCONTRIBUTING.md:39forbids; the author left the call open. - MEDIUM — GitLab-shaped input: a header-only section followed by a stripped section is swallowed whole (reproduced; kept lines vanish with no disclosure).
- MEDIUM — excluded paths are omitted from
pr_headbut still listed inchanged_files, so they can re-enter sub-agent context off disk.
Review-only; not requesting changes.
| # ready-for-merge. An entirely empty findings array is deleted | ||
| # (minItems: 1 in the schema); a disclosure-only array is kept — the | ||
| # disclosures must still reach the review. | ||
| if [ "${blocking_count}" -eq 0 ]; then |
There was a problem hiding this comment.
[HIGH] Disclosure-only (all-excluded) PRs can still get approve → ready-for-merge with nothing reviewed
Verified by reading the file at head dd67633. blocking_count (line 165) excludes info-level provenance-warning/excluded-content, and the if [ "${blocking_count}" -eq 0 ] block (line 179) rewrites .action only when the original action was request-changes or reject (line 183); the else branch at 186 just deletes an empty findings array and leaves approve intact. approve then flows to OUTCOME_LABEL="ready-for-merge" (line 548) and the label is applied at line 565-568. The only other approve guard is the protected-path check (line 203+), which is path-based and does not fire for lockfile/vendor paths.
The PR opens a new route into that state: step 2c (SKILL.md:236-243) sends an all-excluded PR (a lockfile-only dependency bump) straight to step 7 with the disclosures as its only findings, skipping steps 3-6 entirely. Step 7's outcome table (SKILL.md:1412-1418) has no row for "nothing was reviewed" — its approve row reads "set body to 'Looks good to me' when there are no findings", and neither step 2c nor step 7 forbids approve when the only findings are exempt info-level disclosures. So the model guesses, and an approve guess yields ready-for-merge on content the PR itself declares "no model read its contents" — the opposite of what the disclosure mechanism is for.
Not covered by any existing thread: the resolved qodo threads on scripts/post-review.sh:557/569 (fixed in ccbb0aa / 5ef1681) only ever addressed the request-changes/reject direction and high-severity disclosure categories; the approve direction was never raised.
Suggestion: Close it on the enforcement side: in the blocking_count -eq 0 branch, also downgrade approve to comment when the findings array is non-empty (disclosure-only), so an unreviewed PR gets requires-manual-review rather than ready-for-merge. Edit scripts/post-review.src.sh and re-run make script-build so the generated bundle follows — this PR already took a CRITICAL for editing post-review.sh directly. Add an "approve + only disclosure findings" case to scripts/post-review-test.sh beside the existing downgrade tests, and add an explicit step 7 row (or a sentence in step 2c) stating that an all-excluded PR emits action: comment with the excluded-content findings.
| ```bash | ||
| # Scanner dialect (fullsend-ai/agents#1190): `test` not `[ ]`, no rm. | ||
| # Absolute path: cwd is /sandbox/workspace, not the skills checkout. | ||
| if bash "${CLAUDE_CONFIG_DIR}/skills/pr-review/scripts/filter-review-diff.sh" \ |
There was a problem hiding this comment.
[MEDIUM] Step 2c is inert on the pi runtime — ${CLAUDE_CONFIG_DIR} is exported only by the Claude runtime
Step 2c runs the filter as bash "${CLAUDE_CONFIG_DIR}/skills/pr-review/scripts/filter-review-diff.sh". Re-verified against fullsend origin/main at ad1037e1 (fetched, not a stale local submodule): internal/runtime/claude.go:42 is the only place CLAUDE_CONFIG_DIR is exported. PiRuntime.EnvExports() (internal/runtime/pi.go:121-129) exports PI_CODING_AGENT_DIR, PI_CODING_AGENT_SESSION_DIR, PI_OFFLINE, PI_SKIP_VERSION_CHECK, PI_TELEMETRY, JITI_FS_CACHE — no CLAUDE_CONFIG_DIR — and pi_bootstrap.go:249 uploads skills to <PI_CODING_AGENT_DIR>/skills/. README.md:13 declares the Review agent runs on claude, pi.
On pi the variable expands empty, the command becomes bash /skills/pr-review/scripts/filter-review-diff.sh → exit 127 → the else branch fires, printing FILTER FAILED — pr-diff.txt left as fetched on every pi review, and the PR's whole benefit does not materialise on a declared runtime. The PR body's non-goal ("the pi-runtime path can adopt it when it stabilizes") is not implementable as wired: there is one SKILL.md and pi loads it as-is, with no runtime guard. The failure is fail-open to the pre-PR status quo, hence MEDIUM rather than HIGH — but it ships a permanent false-alarm log line and a silently unrealised feature.
Distinct from the resolved SKILL.md:211 thread (fixed in a30cb3b), which was about a bare relative path clobbering the diff from the wrong cwd; this is about the absolute path chosen to fix it not resolving off the Claude runtime.
Suggestion: Resolve the skills root runtime-neutrally before the call — a scanner-safe test -f chain over ${CLAUDE_CONFIG_DIR}, ${PI_CODING_AGENT_DIR}, ${CODEX_HOME}, taking the first whose skills/pr-review/scripts/filter-review-diff.sh exists — and add a sentence to step 2c naming the runtimes the filter is active on, so FILTER FAILED is not read as a regression. Longer term, have fullsend export one FULLSEND_SKILLS_DIR and use it here and in skills/pr-risk-assessment/SKILL.md:199, which carries the same pattern.
| "rule_id": "context-budget", | ||
| "file_path": "skills/pr-review/SKILL.md", | ||
| "message": "Estimated 16,050 tokens exceeds skill error limit of 6,000", | ||
| "message": "Estimated 17,579 tokens exceeds skill error limit of 6,000", |
There was a problem hiding this comment.
[MEDIUM] skillsaw context-budget ceiling raised for this PR's own growth, which CONTRIBUTING.md forbids
Verified by diff at head dd67633: the context-budget ceiling for skills/pr-review/SKILL.md goes from 16050 to 17579 (+1,529 tokens), all of it attributable to the step 2c block and the step 7 disclosure text this PR adds (commits 9da767e then 81ecb38 — raised twice). CONTRIBUTING.md:39 reads: "If make lint fails because unrelated changes on main increased an existing baselined violation, merge main and run make lint-baseline. Do not regenerate the baseline for violations introduced or increased by your PR; fix those instead." The increase here is unambiguously introduced by this PR, not by unrelated main drift.
The author explicitly left this open rather than resolving it — issue comment 2026-09-06: "Your call: the skill exceeds main's skillsaw ceiling, so I raised it to 16,815 in its own commit. Drop it to shrink the skill." It has since been raised again to 17,579 with no answer recorded. Flagging so the open call is closed before merge rather than landing by default. Raised independently by two reviewers.
Suggestion: Shrink the added SKILL.md prose back under 16,050 rather than raising the ceiling — step 2c currently restates the script's classification rules, the accepted-risk paragraph and the $1/$2 semantics that the script header comment (filter-review-diff.sh:1-70) already documents; replace those with a one-line pointer to the header. If maintainers accept the raise instead, record the sign-off in the PR body naming why this is exempt from CONTRIBUTING.md:39, and keep it in its own commit (already done).
| function is_gl_candidate(line) { | ||
| if (line !~ /^--- (a\/|"a\/)/ && line != "--- /dev/null") return 0 | ||
| if (!in_diff) return 1 | ||
| return gitlab_mode && phase != "header" |
There was a problem hiding this comment.
[MEDIUM] GitLab-shaped input loses kept lines: a header-only section followed by a stripped section is swallowed whole
is_gl_candidate() ends with return gitlab_mode && phase != "header" (line 359). So once gitlab_mode is on, a --- a/Y line arriving while the current section is still in the header phase (no @@ seen yet — a rename-only, mode-only or empty-diff section) is not treated as a boundary. It is consumed as another header line of the current section, set_old_path/set_new_path overwrite the paths with Y, and the whole accumulated section is then classified as Y.
Reproduced at head dd67633 by running the script directly. Input:
--- a/src/keep.js
+++ b/src/keep.js
--- a/package-lock.json
+++ b/package-lock.json
@@ -1 +1 @@
-a
+b
Output: empty stdout, exit 0, summary package-lock.json +1/-1 lockfile. The two src/keep.js header lines vanish with no disclosure — breaking the "kept sections stream through byte-identical" contract the whole design rests on, and silently misattributing the loss to a lockfile.
Not covered by any existing thread: the resolved GitLab threads are :329 (removed lines as boundaries, fixed 5ef1681), :405 (loose +++ confirmation regex, since tightened to /^\+\+\+ (b\/|"b\/)/), and :165 (quoted GitLab paths fail open). None touches the header-phase boundary rule. No current forge skill emits this shape (both now write ### File: headers), but the script's own header comment and step 2c advertise --- a/X as supported input, and the 100-assertion suite has no header-only GitLab section case.
Suggestion: Either drop gitlab_mode as unsupported dead code (and the header/SKILL.md claims with it) now that no producer emits that shape, or fix the boundary rule: in gitlab_mode, a --- a/ / --- "a/ / --- /dev/null line seen while phase == "header" after both old_path and new_path are already set must still be a candidate, finalizing the header-only section first. Add a test with a header-only section followed by a stripped section asserting the first section's lines are printed byte-identical.
| - `pr_head`: the MANIFEST lines (step 2b) for the files this sub-agent | ||
| should look at — all changed files for `correctness`, `security` and | ||
| `style-conventions`, the dimension-relevant subset otherwise. Paths | ||
| named in `/sandbox/workspace/pr-excluded.txt` are omitted: content stripped |
There was a problem hiding this comment.
[MEDIUM] Excluded paths are dropped from pr_head but still listed in changed_files, so they can re-enter sub-agent context
This PR adds the invariant on this line — "Paths named in /sandbox/workspace/pr-excluded.txt are omitted: content stripped from the diff must not re-enter model context as a whole file" — to the pr_head bullet of the step 3d context package. Two bullets below (SKILL.md:738, unchanged by this PR) the same package still carries changed_files: "list of relative file paths modified", with no subtraction for excluded paths.
Step 2b materialises every changed blob into /sandbox/workspace/pr-head/<path> before 2c computes the exclusions, and nothing deletes them afterwards (2c has no rm — the scanner dialect forbids it). A sub-agent that iterates changed_files rather than the filtered pr_head manifest can Read a lockfile or vendored tree straight off disk. The invariant this PR states is therefore enforced only on one of the two path lists the same package hands the sub-agent.
Distinct from the resolved qodo thread at SKILL.md:130 ("Excluded source files still loaded"), which was about step 2b fetching contents and was answered by the pr_head omission added here; the residual changed_files route has not been raised.
Suggestion: Subtract the excluded paths from changed_files the same way they are subtracted from pr_head — one sentence on the changed_files bullet is enough, since both lists are assembled at the same point. Alternatively state explicitly that changed_files is deliberately the full unfiltered list (for routing/context) and instruct sub-agents to read only paths present in the pr_head manifest, so the two lists' different semantics are documented rather than implicit.
An all-excluded PR (whose only findings are exempt info-level excluded-content / provenance-warning disclosures) kept its `approve` action through the `blocking_count -eq 0` branch, which only rewrote request-changes / reject. `approve` then mapped to `ready-for-merge` — approving content no model actually read. Downgrade `approve` to `comment` when the only remaining findings are disclosures, so the label logic maps it to requires-manual-review. Rebuild the generated bundle and add an approve + disclosure-only case to post-review-test.sh beside the existing downgrade tests. Signed-off-by: guy oron <goron@redhat.com>
In gitlab_mode, `is_gl_candidate()` treated a `--- a/` line seen while the section was still in the header phase as another header line rather than a boundary. A header-only section (rename-only, mode-only or empty diff, no `@@`) followed by a stripped section was swallowed whole: set_old_path overwrote the path and the kept lines vanished with no disclosure, misattributed to the next section. Treat a `--- a/` line in the header phase as a boundary once both old_path and new_path are already set, so finalize flushes the header-only section first. Add a test asserting the first section's lines survive byte-identical while the lockfile is still stripped. Signed-off-by: guy oron <goron@redhat.com>
Three review findings on the diff-filtering change:
- Step 2c ran the filter as `${CLAUDE_CONFIG_DIR}/.../filter-...sh`,
which is exported only by the Claude runtime; on pi/codex the path
expanded empty, the call exited 127 and logged FILTER FAILED on every
review. Resolve the skills root over CLAUDE_CONFIG_DIR,
PI_CODING_AGENT_DIR and CODEX_HOME, taking the first whose checkout
has the script. The mv stays gated: an unresolved script skips the
command, so no empty diff is ever moved into place.
- Excluded paths were dropped from `pr_head` but still listed in
`changed_files`, so a sub-agent iterating that list could Read a
lockfile or vendored tree off disk. Subtract the excluded paths from
`changed_files` too, matching `pr_head`.
- The skillsaw context-budget ceiling for this skill had been raised
16050 -> 16815 -> 17579 for this PR's own growth, which
CONTRIBUTING.md forbids. Shrink the skill back under 16050 instead:
drop the step 2c prose that restated the script header (classification
rules, arg contract, accepted risk) down to a pointer, and tighten
verbose explanations elsewhere. Revert the baseline ceiling to 16050.
Also state in step 7 that an all-excluded PR emits `comment`, never
`approve`, documenting the enforcement added to post-review.
Signed-off-by: guy oron <goron@redhat.com>
|
@waynesun09 — thanks, all five addressed. New head
|
2 participants
Heyaa : )
Watching review context get assembled, I kept seeing lockfile, minified, sourcemap and
@generatedhunks ride along into every dimension's prompt — content no model can meaningfully assess, pure input cost on every mixed PR.New
skills/pr-review/scripts/filter-review-diff.sh: a single awk program over the unified diff on stdin. Strips lockfile sections (self-contained list across ecosystems),*.min.js/*.min.css/*.map,vendor//node_modules//third_party/paths, and generated files — a generated-looking path (protobuf/codegen suffixes,generated//dist//build/) whose early added or context lines carry@generated, the GoCode generated … DO NOT EDIT.marker, or the protoc header. The path-shape gate is deliberate: an author-planted marker on ordinary source never hides it from review.migrations/paths are exempt from every rule. Filtered diff on stdout — byte-identical for untouched sections — and a one-line-per-file exclusion summary written only to$1, never stdout.Wiring (
skills/pr-review/SKILL.md): the small-PR path pipes the full diff through the filter before any context package; the large-PR path's "lockfiles, vendor/, protobuf, etc." vibes list is replaced by the same script — both paths now share one deterministic definition of "generated".FILE_COUNT/LINE_COUNTstay computed from the unfiltered stats: routing must see the true size.Exclusions are disclosed via an info-level
excluded-contentfinding (theprovenance-warningprecedent — step 7 forbids footers), so a changed lockfile is still visible, just not reviewed.scripts/filter-review-diff-test.shis wired intomake script-test: byte-identical pass-through (viacmp), each stripping rule, the added-vs-removed@generateddistinction, the migration exemption, summary format and stdout silence, all-stripped output, malformed-input pass-through. shellcheck clean.Step 2b's file-contents fetch honors the same exclusions, so filtered files stay out of
source_filestoo. Non-goals: no config knob. The script is architecture-neutral; the pi-runtime path can adopt it when it stabilizes.Refs #1143