Skip to content

feat(review-pr): auto-filter low-risk files before chunking#215

Open
derekmisler wants to merge 2 commits into
mainfrom
feat/auto-filter-large-diffs
Open

feat(review-pr): auto-filter low-risk files before chunking#215
derekmisler wants to merge 2 commits into
mainfrom
feat/auto-filter-large-diffs

Conversation

@derekmisler
Copy link
Copy Markdown
Contributor

Summary

Fixes the root cause of large-PR review timeouts by making the risk-scoring pipeline automatically filter out low-risk files before diff chunking, without requiring per-repo exclude-paths configuration.

Root causes addressed

  1. Risk scoring happened after chunking — scores were computed but never used to filter the diff. Every file (including test files, generated code, etc.) got chunked and processed by the full multi-agent pipeline.

  2. TEST_FILE_RE in score-risk.ts missed Rust/Ruby test files — Rust test/bench/spec files and Ruby spec files scored non-zero and were included in chunks unnecessarily.

Changes

Change 1 — Extend TEST_FILE_RE in src/score-risk/score-risk.ts

Extended the regex to also match:

  • Rust suffix patterns: _test.rs, _bench.rs, _spec.rs
  • Ruby spec files: _spec.rb
  • Any file under a tests/, test/, benches/, __tests__/, or specs/ directory component

Added 9 new Vitest tests covering these new patterns.

Change 2 — New module src/auto-filter-diff/

Pure TypeScript module with two-phase filtering:

  • Phase 1 (auto-exclude): Removes all score-0 files. Unknown files (not in riskScores) are kept as they need review.
  • Phase 2 (progressive cap): If remaining diff > maxDiffLines (default 3000), removes lowest-risk files first until it fits. Always keeps at least 1 file.

Includes 18 Vitest unit tests covering all phases and edge cases.

Change 3 — Wire up in review-pr/action.yml

  • Added max-diff-lines input (default "3000", set "0" to disable)
  • Moved "Score file risk" step to run before "Split diff into chunks"
  • Added new "Auto-filter low-risk files" step between scoring and chunking

New step order: Filter excluded paths → Score file risk → Auto-filter → Split into chunks → Generate file history

Change 4 — Propagate through .github/workflows/review-pr.yml

Added max-diff-lines as a workflow-level number input (default 3000) and passed it through to the review-pr action call.

Change 5 — tsup.config.ts

Added 'src/auto-filter-diff/index.ts' to the entry map so tsup bundles it as dist/auto-filter-diff.js.

Test results

Test Files  20 passed (20)
     Tests  358 passed (358)

Biome formatting and TypeScript type-check pass cleanly. The pre-existing actionlint warning about the node24 runner in test-e2e.yml is unrelated to this PR (present on main before these changes).

Copy link
Copy Markdown
Contributor

@docker-agent docker-agent left a comment

Choose a reason for hiding this comment

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

Assessment: 🟡 NEEDS ATTENTION

This PR correctly implements a two-phase auto-filter mechanism and moves risk scoring before diff chunking. The new TEST_FILE_RE additions for Rust/Ruby patterns look correct, and the tsup.config.ts entry is properly wired. One medium-severity logic issue was found in Phase 2 of autoFilterDiff.

Comment thread src/auto-filter-diff/auto-filter-diff.ts Outdated
Copy link
Copy Markdown
Contributor Author

@derekmisler derekmisler left a comment

Choose a reason for hiding this comment

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

Assessment: 🟡 NEEDS ATTENTION

No new findings from this review pass the quality gate. One confirmed bug is already flagged in an existing inline comment (line 164 of src/auto-filter-diff/auto-filter-diff.ts): the Phase 2 progressive cap uses slice(0, -1) to protect the last file in the ascending-risk sort, but when multiple unscored files exist they all sort to Infinity and only the very last is protected — the others remain removable, violating the "unknown = needs review" invariant.

All other items examined were verified clean: missing set -euo pipefail in the auto-filter step (single-command step, no functional impact), remainingLines preamble accounting (no production trigger path), TEST_FILE_RE regex (works correctly), step ordering (correct: filter → score → auto-filter → chunk), lock-check guard asymmetry (harmless: pr.diff is never created when skip=true), and extractFilePath edge cases (correct for normal paths).

@derekmisler derekmisler force-pushed the feat/auto-filter-large-diffs branch 4 times, most recently from 77bdd24 to 0b9207e Compare May 22, 2026 16:02
Copy link
Copy Markdown
Contributor

@docker-agent docker-agent left a comment

Choose a reason for hiding this comment

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

Assessment: 🟢 APPROVE

The auto-filter-diff pipeline is well-structured and correct. The two-phase filtering logic in src/auto-filter-diff/auto-filter-diff.ts handles edge cases properly:

  • Phase 1 (auto-exclude): Correctly removes score-0 known files while preserving unknown files for review.
  • Phase 2 (progressive cap): Correctly sorts by risk score ascending and removes lowest-risk scored files first. Unknown files are intentionally exempt from removal (documented in code comments). The slice(0, -1) guard ensures at least one file is always kept among scored files.
  • remainingLines calculation: Accurate — parseSections partitions the split('\n') array into non-overlapping slices, so sum(s.lines.length) equals the assembled diff's line count.
  • TEST_FILE_RE extension: Rust and Ruby test patterns are correctly scoped to file suffixes and directory components.
  • review-pr/action.yml wiring: Steps are correctly ordered (score → auto-filter → chunk), max-diff-lines input is properly threaded through.
  • tsup.config.ts entry map: auto-filter-diff/index.ts is correctly added since it backs a direct CLI step in action.yml.

18 unit tests cover all phases and edge cases including empty diffs, single-file protection, and unknown-file preservation. No bugs found in the changed code.

Signed-off-by: Derek Misler <derek.misler@docker.com>
@derekmisler derekmisler force-pushed the feat/auto-filter-large-diffs branch from fbaf8f1 to d246250 Compare May 26, 2026 14:34
…events by structure

GitHub Actions does not allow overriding GITHUB_* runner-injected variables
via the env: block on a uses: composite action step. As a result, the
test-mention-reply-toplevel and test-mention-reply-inline jobs were reading
the real pull_request event file (which has no 'issue' field), causing
parseEventContext() to throw 'Cannot read properties of undefined (reading number)'.

Fix 1 — src/mention-reply/index.ts:
Add a structural fallback in parseEventContext(): treat the event as a
pull_request_review_comment if raw.pull_request exists AND comment.diff_hunk
is present (a combination unique to inline review comment payloads). This
makes the handler work correctly even when GITHUB_EVENT_NAME cannot be
overridden and stays as 'pull_request'.

Fix 2 — .github/workflows/test-e2e.yml:
Write synthetic event payloads directly to $GITHUB_EVENT_PATH (the real
runner-provided path, which is writable) instead of /tmp/test-event-*.json.
Remove the non-working env: overrides for GITHUB_EVENT_PATH and
GITHUB_EVENT_NAME from both 'Run mention-reply handler' steps — overriding
GITHUB_* variables this way has no effect on composite actions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants