fullsend-ai · guyoron1 · Aug 25, 2026
@@ -68,5 +68,6 @@ script-test:
 	$(call run-timed,python3 scripts/process-fix-result-test.py)
 	$(call run-timed,bash eval/scripts/scrub-eval-results-test.sh)
 	$(call run-timed,bash .github/scripts/check-rollup-result-test.sh)
+	$(call run-timed,bash scripts/filter-review-diff-test.sh)
 
 test: script-test
@@ -0,0 +1,279 @@
+#!/usr/bin/env bash
+# filter-review-diff-test.sh — Tests for
+# skills/pr-review/scripts/filter-review-diff.sh
+#
+# Run from the repo root:
+#   bash scripts/filter-review-diff-test.sh
+
+set -euo pipefail
+
+REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
+FILTER="${REPO_ROOT}/skills/pr-review/scripts/filter-review-diff.sh"
+
+FAILURES=0
+TMPDIR=$(mktemp -d)
+trap 'rm -rf "${TMPDIR}"' EXIT
+
+# --- Test helpers ---
+
+run_test() {
+  local test_name="$1"
+  local expected="$2"
+  local actual="$3"
+
+  if [[ "${actual}" == "${expected}" ]]; then
+    echo "PASS: ${test_name}"
+  else
+    echo "FAIL: ${test_name}"
+    echo "  expected: '${expected}'"
+    echo "  actual:   '${actual}'"
+    FAILURES=$((FAILURES + 1))
+  fi
+}
+
+run_test_contains() {
+  local test_name="$1"
+  local needle="$2"
+  local haystack="$3"
+
+  if [[ "${haystack}" == *"${needle}"* ]]; then
+    echo "PASS: ${test_name}"
+  else
+    echo "FAIL: ${test_name}"
+    echo "  expected to contain: '${needle}'"
+    echo "  actual:               '${haystack}'"
+    FAILURES=$((FAILURES + 1))
+  fi
+}
+
+run_test_not_contains() {
+  local test_name="$1"
+  local needle="$2"
+  local haystack="$3"
+
+  if [[ "${haystack}" != *"${needle}"* ]]; then
+    echo "PASS: ${test_name}"
+  else
+    echo "FAIL: ${test_name}"
+    echo "  expected NOT to contain: '${needle}'"
+    echo "  actual:                   '${haystack}'"
+    FAILURES=$((FAILURES + 1))
+  fi
+}
+
+# --- Fixtures ---
+
+NORMAL_DIFF='diff --git a/src/main.go b/src/main.go
+index 1111111..2222222 100644
+--- a/src/main.go
++++ b/src/main.go
+@@ -1,4 +1,5 @@
+ package main
+
++import "fmt"
+ func main() {
+ }'
+
+LOCKFILE_SECTION='diff --git a/package-lock.json b/package-lock.json
+index aaaaaaa..bbbbbbb 100644
+--- a/package-lock.json
++++ b/package-lock.json
+@@ -1,3 +1,3 @@
+ {
+-  "version": "1.0.0"
++  "version": "1.0.1"
+ }'
+
+CODE_SECTION='diff --git a/src/app.js b/src/app.js
+index ccccccc..ddddddd 100644
+--- a/src/app.js
++++ b/src/app.js
+@@ -1,2 +1,3 @@
+ function app() {
++  console.log("hi");
+ }'
+
+GENERATED_ADDED='diff --git a/gen/models.go b/gen/models.go
+index 1111111..2222222 100644
+--- a/gen/models.go
++++ b/gen/models.go
+@@ -0,0 +1,3 @@
++// Code generated by protoc-gen-go. DO NOT EDIT.
++// @generated
++package gen'
+
+GENERATED_REMOVED_ONLY='diff --git a/gen/legacy.go b/gen/legacy.go
+index 1111111..2222222 100644
+--- a/gen/legacy.go
++++ b/gen/legacy.go
+@@ -1,2 +1,2 @@
+-// @generated marker
+-package gen
++package gen
++// no marker here'
+
+MIGRATION_GENERATED='diff --git a/db/migrations/0001_init.sql b/db/migrations/0001_init.sql
+index 1111111..2222222 100644
+--- a/db/migrations/0001_init.sql
++++ b/db/migrations/0001_init.sql
+@@ -0,0 +1,2 @@
++-- @generated by some tool
++CREATE TABLE foo (id INT);'
+
+# shellcheck disable=SC2034 # read indirectly via ${!fixture_var} in the loop below
+MINJS='diff --git a/dist/bundle.min.js b/dist/bundle.min.js
+index 1111111..2222222 100644
+--- a/dist/bundle.min.js
++++ b/dist/bundle.min.js
+@@ -1 +1 @@
+-old
++new'
+
+# shellcheck disable=SC2034 # read indirectly via ${!fixture_var} in the loop below
+SOURCEMAP='diff --git a/dist/bundle.js.map b/dist/bundle.js.map
+index 1111111..2222222 100644
+--- a/dist/bundle.js.map
++++ b/dist/bundle.js.map
+@@ -1 +1 @@
+-old
++new'
+
+# shellcheck disable=SC2034 # read indirectly via ${!fixture_var} in the loop below
+VENDORED='diff --git a/vendor/lib/thing.go b/vendor/lib/thing.go
+index 1111111..2222222 100644
+--- a/vendor/lib/thing.go
++++ b/vendor/lib/thing.go
+@@ -1 +1 @@
+-old
++new'
+
+MALFORMED='This is not a diff.
+Just some random text.
+No file markers here.'
+
+# --- 1. Normal-code diff passes through byte-identical (the critical
+#        transparency property: cmp, not just string equality). ---
+
+printf '%s\n' "${NORMAL_DIFF}" > "${TMPDIR}/normal.in"
+"${FILTER}" < "${TMPDIR}/normal.in" > "${TMPDIR}/normal.out"
+if cmp -s "${TMPDIR}/normal.in" "${TMPDIR}/normal.out"; then
+  echo "PASS: normal-diff-byte-identical"
+else
+  echo "FAIL: normal-diff-byte-identical"
+  diff -u "${TMPDIR}/normal.in" "${TMPDIR}/normal.out" || true
+  FAILURES=$((FAILURES + 1))
+fi
+
+# --- 2. Lockfile section stripped; mixed diff keeps the code section. ---
+
+printf '%s\n%s\n' "${LOCKFILE_SECTION}" "${CODE_SECTION}" > "${TMPDIR}/mixed.in"
+MIXED_OUT=$("${FILTER}" "${TMPDIR}/mixed.summary" < "${TMPDIR}/mixed.in")
+
+run_test_not_contains "mixed-lockfile-not-in-stdout" "package-lock.json" "${MIXED_OUT}"
+run_test_contains "mixed-code-section-kept" "console.log" "${MIXED_OUT}"
+printf '%s\n' "${CODE_SECTION}" > "${TMPDIR}/mixed-code-expected"
+CODE_ONLY=$(printf '%s\n' "${MIXED_OUT}")
+if [[ "${CODE_ONLY}" == "${CODE_SECTION}" ]]; then
+  echo "PASS: mixed-code-section-byte-identical"
+else
+  echo "FAIL: mixed-code-section-byte-identical"
+  echo "  expected: ${CODE_SECTION}"
+  echo "  actual:   ${CODE_ONLY}"
+  FAILURES=$((FAILURES + 1))
+fi
+
+# --- 3. @generated in added lines strips; marker only in REMOVED lines
+#        does not. ---
+
+printf '%s\n' "${GENERATED_ADDED}" > "${TMPDIR}/gen-added.in"
+GEN_ADDED_OUT=$("${FILTER}" "${TMPDIR}/gen-added.summary" < "${TMPDIR}/gen-added.in")
+run_test "generated-added-strips-stdout" "" "${GEN_ADDED_OUT}"
+run_test_contains "generated-added-in-summary" "gen/models.go" "$(/bin/cat "${TMPDIR}/gen-added.summary")"
+run_test_contains "generated-added-reason" "generated-marker" "$(/bin/cat "${TMPDIR}/gen-added.summary")"
+
+printf '%s\n' "${GENERATED_REMOVED_ONLY}" > "${TMPDIR}/gen-removed.in"
+"${FILTER}" "${TMPDIR}/gen-removed.summary" < "${TMPDIR}/gen-removed.in" > "${TMPDIR}/gen-removed.out"
+if cmp -s "${TMPDIR}/gen-removed.in" "${TMPDIR}/gen-removed.out"; then
+  echo "PASS: generated-marker-in-removed-line-does-not-strip"
+else
+  echo "FAIL: generated-marker-in-removed-line-does-not-strip"
+  diff -u "${TMPDIR}/gen-removed.in" "${TMPDIR}/gen-removed.out" || true
+  FAILURES=$((FAILURES + 1))
+fi
+run_test "generated-removed-empty-summary" "" "$(/bin/cat "${TMPDIR}/gen-removed.summary" 2>/dev/null || true)"
+
+# --- 4. Migration file with @generated is KEPT (exemption beats every
+#        strip rule, including the generated-marker rule). ---
+
+printf '%s\n' "${MIGRATION_GENERATED}" > "${TMPDIR}/migration.in"
+"${FILTER}" "${TMPDIR}/migration.summary" < "${TMPDIR}/migration.in" > "${TMPDIR}/migration.out"
+if cmp -s "${TMPDIR}/migration.in" "${TMPDIR}/migration.out"; then
+  echo "PASS: migration-with-generated-marker-kept"
+else
+  echo "FAIL: migration-with-generated-marker-kept"
+  diff -u "${TMPDIR}/migration.in" "${TMPDIR}/migration.out" || true
+  FAILURES=$((FAILURES + 1))
+fi
+run_test "migration-not-in-summary" "" "$(/bin/cat "${TMPDIR}/migration.summary" 2>/dev/null || true)"
+
+# --- 5. *.min.js, *.map, vendor/ stripped. ---
+
+for case in "MINJS:minjs:bundle.min.js:minified" "SOURCEMAP:sourcemap:bundle.js.map:sourcemap" "VENDORED:vendored:vendor/lib/thing.go:vendored"; do
+  fixture_var="${case%%:*}"
+  rest="${case#*:}"
+  label="${rest%%:*}"
+  rest="${rest#*:}"
+  path_hint="${rest%%:*}"
+  reason="${rest#*:}"
+  fixture="${!fixture_var}"
+
+  printf '%s\n' "${fixture}" > "${TMPDIR}/${label}.in"
+  OUT=$("${FILTER}" "${TMPDIR}/${label}.summary" < "${TMPDIR}/${label}.in")
+  run_test "${label}-stdout-empty" "" "${OUT}"
+  run_test_contains "${label}-in-summary" "${path_hint}" "$(/bin/cat "${TMPDIR}/${label}.summary")"
+  run_test_contains "${label}-reason-recorded" "${reason}" "$(/bin/cat "${TMPDIR}/${label}.summary")"
+done
+
+# --- 6. Summary lines correct (path, counts, reason); summary never on
+#        stdout. ---
+
+SUMMARY_LINE=$(/bin/cat "${TMPDIR}/mixed.summary")
+run_test "summary-line-format" "package-lock.json  +1/-1  lockfile" "${SUMMARY_LINE}"
+run_test_not_contains "summary-never-on-stdout" "lockfile" "${MIXED_OUT}"
+
+# --- 7. Empty result (everything stripped) produces empty stdout + full
+#        summary, exit 0. ---
+
+printf '%s\n' "${LOCKFILE_SECTION}" > "${TMPDIR}/all-stripped.in"
+set +e
+ALL_STRIPPED_OUT=$("${FILTER}" "${TMPDIR}/all-stripped.summary" < "${TMPDIR}/all-stripped.in")
+ALL_STRIPPED_EXIT=$?
+set -e
+run_test "all-stripped-exit-0" "0" "${ALL_STRIPPED_EXIT}"
+run_test "all-stripped-stdout-empty" "" "${ALL_STRIPPED_OUT}"
+run_test_contains "all-stripped-summary-full" "package-lock.json" "$(/bin/cat "${TMPDIR}/all-stripped.summary")"
+
+# --- 8. Malformed input (not a diff) passes through unchanged, exit 0. ---
+
+printf '%s\n' "${MALFORMED}" > "${TMPDIR}/malformed.in"
+set +e
+"${FILTER}" < "${TMPDIR}/malformed.in" > "${TMPDIR}/malformed.out"
+MALFORMED_EXIT=$?
+set -e
+run_test "malformed-exit-0" "0" "${MALFORMED_EXIT}"
+if cmp -s "${TMPDIR}/malformed.in" "${TMPDIR}/malformed.out"; then
+  echo "PASS: malformed-input-passthrough"
+else
+  echo "FAIL: malformed-input-passthrough"
+  diff -u "${TMPDIR}/malformed.in" "${TMPDIR}/malformed.out" || true
+  FAILURES=$((FAILURES + 1))
+fi
+
+# --- Wrap up ---
+
+echo ""
+if [ "${FAILURES}" -gt 0 ]; then
+  echo "${FAILURES} test(s) failed"
+  exit 1
+fi
+echo "All tests passed"
@@ -117,15 +117,44 @@ skill commands:
   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
+unfiltered file-stats response, and used as-is for the routing decision
+below. Nothing in this step recomputes them from post-filter output —
+triage must see the true size of the change, not its post-filter size.
+
 From there use FILE_COUNT and LINE_COUNT to decide how to proceed
 
-1. FILE_COUNT<50, LINE_COUNT<3000: small PR — fetch the full unified diff
+1. FILE_COUNT<50, LINE_COUNT<3000: small PR — fetch the full unified
+   diff, then pipe it through
+   `skills/pr-review/scripts/filter-review-diff.sh <summary-file>`
+   before it enters any context package (step 3d). The script
+   deterministically strips lockfiles, `*.min.js`/`*.min.css`,
+   sourcemaps, and vendored paths (`vendor/`, `node_modules/`,
+   `third_party/`), and files carrying an `@generated` marker in their
+   added lines — migrations are exempt from every one of those rules.
+   See the script's header comment for the exact classification. Read
+   the exclusion-summary file it writes (never emitted on stdout):
+   - fold it into the orchestrator's own context — it is not part of
+     the diff sub-agents receive, they only ever see the filtered output
+   - if it is non-empty, add an `excluded-content` info-level finding at
+     step 7 (same mechanism as the `provenance-warning` finding below —
+     not a footer; SKILL.md step 7 explicitly forbids appending one):
+     "N generated/lockfile file(s) changed but not reviewed
+     line-by-line: <list>" — a stripped lockfile must still be visible
+     to whoever reads the review, even though no model read its
+     contents.
 2. FILE_COUNT~=50-200, LINE_COUNT~=3000-10000: large PR — switch to per-file
    mode
 
    - Extract file paths from PR_STATS
-   - Filter out generated files (lockfiles, vendor/, protobuf, etc.)
    - Produce per-file diffs via `git diff <merge-base>..HEAD -- <file>`
+   - Filter out generated files: pipe each per-file diff through the
+     same `skills/pr-review/scripts/filter-review-diff.sh` the small-PR
+     path uses above — one deterministic definition of "generated" for
+     both paths, instead of a separate prompt-level list here. A file
+     whose filtered output is empty is dropped from the concatenation;
+     collect its exclusion-summary line the same way the small-PR path
+     does.
    - Concatenate per-file diffs into a single blob per sub-agent (see
      step 3d for the format)
 
@@ -516,12 +545,16 @@ incident.
 For each selected sub-agent, assemble a context package containing:
 
 - `diff`: For small PRs (< 50 files, < 3000 lines), the full unified PR
-  diff (fetched via the forge-specific review skill). For large PRs (step 2 criteria), a concatenation
-  of per-file diffs, each produced by
-  `git diff <merge-base>..HEAD -- <file>`. Each per-file diff is preceded
-  by a `### File: <relative-path>` header so sub-agents can identify file
-  boundaries. Generated files (lockfiles, vendor/, protobuf output) are
-  excluded from the concatenation.
+  diff (fetched via the forge-specific review skill), filtered through
+  `filter-review-diff.sh` (step 2). For large PRs (step 2 criteria), a
+  concatenation of per-file diffs, each produced by
+  `git diff <merge-base>..HEAD -- <file>` and filtered the same way.
+  Each per-file diff is preceded by a `### File: <relative-path>` header
+  so sub-agents can identify file boundaries. Both paths share the same
+  script and the same definition of "generated" — lockfiles, minified/
+  sourcemap output, and vendored paths are excluded from the
+  concatenation, migrations are exempt — see step 2 for the exclusion-
+  summary handling.
 - `source_files`: full contents of changed files at the PR head revision,
   fetched by the orchestrator in step 2b. Each file is preceded by a
   `#### <relative-path>` header and wrapped in a fenced code block with
@@ -1172,6 +1205,15 @@ info-level finding in the review output:
   provenance validation failed (`PRIOR_REVIEW_PROVENANCE` value).
   This review treats all findings as first-time assessments.
 
+If step 2's diff filtering produced a non-empty exclusion summary,
+include an info-level finding in the review output (this is a
+disclosure, not a footer — it goes through the same findings/severity
+structure as everything else in this section):
+
+- **[excluded-content]** — N generated/lockfile file(s) changed but not
+  reviewed line-by-line: `<path>` (`<reason>`), ... — listing every
+  path and reason from the exclusion summary.
+
 Map the outcome to an action value. `action`, `pr_number`, and `repo`
 are always required (see the agent definition for the full schema).
 The table below lists the **additional** required fields per action: