Skip to content

feat(review-pr): add exclude-paths input to skip generated files from review #188

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
derekmisler merged 6 commits into from
May 7, 2026
Merged

feat(review-pr): add exclude-paths input to skip generated files from review #188

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
eadbd00
feat(review-pr): add exclude-paths input to skip generated files from…
docker-agent May 7, 2026
300ac1c
fix(review-pr): fix awk diff-filter for deleted files, pure renames, …
docker-agent May 7, 2026
80545fa
refactor(filter-diff): rewrite awk diff-filter as TypeScript with tests
docker-agent May 7, 2026
d8b3ea2
refactor(score-risk): rewrite bash/awk scoring block as TypeScript wi…
docker-agent May 7, 2026
3701812
docs(AGENTS.md): add score-risk to repo layout section
docker-agent May 7, 2026
690ad4e
fix(score-risk): add set -euo pipefail and extractFilePath regression…
docker-agent May 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .github/workflows/review-pr.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ on:
required: false
type: string
default: ""
exclude-paths:
description: 'Newline-separated list of path prefixes to strip from the diff before chunking. Files matching these prefixes are excluded from review entirely.'
required: false
type: string
default: ''
trigger-run-id:
description: "Workflow run ID from pr-review-trigger.yml — used to download the event context artifact. When set, resolve-context job downloads the artifact and routes to the appropriate job."
required: false
Expand Down Expand Up @@ -353,6 +358,7 @@ jobs:
comment-id: ${{ inputs.comment-id || github.event.comment.id }}
additional-prompt: ${{ inputs.additional-prompt }}
add-prompt-files: ${{ inputs.add-prompt-files }}
exclude-paths: ${{ inputs.exclude-paths }}
model: ${{ inputs.model }}
github-token: ${{ env.GITHUB_APP_TOKEN || github.token }}
anthropic-api-key: ${{ env.ANTHROPIC_API_KEY_FROM_SSM || secrets.ANTHROPIC_API_KEY }}
Expand Down
9 changes: 9 additions & 0 deletions AGENTS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,14 @@ Anything else here (workflows under `.github/workflows/`, scripts, tests) exists
│ │ ├── aws-credentials.ts
│ │ ├── github-app.ts # Reads docker-agent-action/github-app from Secrets Manager; exports GITHUB_APP_TOKEN (a PAT) + ORG_MEMBERSHIP_TOKEN.
│ │ └── __tests__/
│ ├── filter-diff/ # Strips excluded-path sections from a unified diff.
│ │ ├── index.ts # CLI entry → bundled to dist/filter-diff.js
│ │ ├── filter-diff.ts # Core filterDiff() pure function + applyFilter() I/O wrapper.
│ │ └── __tests__/
Copy link
Copy Markdown
Contributor Author

@docker-agent docker-agent May 7, 2026

Choose a reason for hiding this comment

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

[LOW] score-risk/ module missing from AGENTS.md repo layout

AGENTS.md was updated to add the filter-diff/ section (lines 42–45), but src/score-risk/ — the other new module introduced by this PR — is absent from the layout. The layout section is the canonical map of what lives in src/ for future contributors and agents reading this file.

The missing entry should appear after filter-diff/ (alphabetically between filter-diff/ and get-pr-meta/):

│   ├── score-risk/                  # Per-file risk scoring for the PR review pipeline.
│   │   ├── index.ts                 # CLI entry → bundled to dist/score-risk.js
│   │   ├── score-risk.ts            # Core scoreFiles() pure function.
│   │   └── __tests__/

Copy link
Copy Markdown
Contributor Author

@docker-agent docker-agent May 7, 2026

Choose a reason for hiding this comment

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

Fixed in 06b6243 — added score-risk/ between filter-diff/ and get-pr-meta/ in the repo layout section of AGENTS.md.

Copy link
Copy Markdown
Contributor Author

@docker-agent docker-agent May 7, 2026

Choose a reason for hiding this comment

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

Already fixed in 06b6243 (the commit immediately before this round's changes). score-risk/ is now present between filter-diff/ and get-pr-meta/ in the repo layout section of AGENTS.md.

│ ├── score-risk/ # Per-file risk scoring for the PR review pipeline.
│ │ ├── index.ts # CLI entry → bundled to dist/score-risk.js
│ │ ├── score-risk.ts # Core scoreFiles() pure function.
│ │ └── __tests__/
│ ├── get-pr-meta/ # Fetches PR metadata (title, body, author, base branch) used by review-pr.
│ │ ├── index.ts # Entry → bundled to dist/get-pr-meta.js
│ │ └── __tests__/
Expand Down Expand Up @@ -109,6 +117,7 @@ Anything else here (workflows under `.github/workflows/`, scripts, tests) exists
### TypeScript / `src` rules

- Only `src/<name>/index.ts` files listed in the explicit `entry` map in `tsup.config.ts` are bundled to `dist/<name>.js`. To add a new action entrypoint, create `src/<name>/index.ts` **and** add it to the `entry` map in `tsup.config.ts`. Pure library modules that are only imported by other actions (e.g. `add-reaction`, `check-org-membership`, `get-pr-meta`, `post-comment`) should **not** be added to the entry map — they get bundled into their consumer automatically.
- **New logic in composite actions must be implemented as TypeScript in `src/` with Vitest unit tests — not as inline bash, awk, or other scripting languages embedded in YAML files.** Shell steps in action YAML files should only orchestrate calls to `dist/*.js` tools (e.g. `node "$ACTION_PATH/dist/filter-diff.js" pr.diff "$EXCLUDE_PATHS"`). This keeps business logic testable, type-safe, and auditable outside the YAML layer.
- `tsup` runs with `noExternal: [/.*/]` — **all npm dependencies are bundled in**. Do not assume `node_modules` exists at runtime.
- Target is `node24`, ESM only, Node platform (so AWS SDK uses the Node export, not browser).
- Sourcemaps are intentionally disabled (consumers clone `dist/`; sourcemaps would bloat every checkout).
Expand Down
72 changes: 16 additions & 56 deletions review-pr/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,10 @@ inputs:
description: "Skip the built-in authorization check (caller already verified auth)"
required: false
default: "false"
exclude-paths:
description: 'Newline-separated list of path prefixes to strip from the diff before chunking. Files matching these prefixes are excluded from review entirely.'
required: false
default: ''

outputs:
exit-code:
Expand Down Expand Up @@ -233,6 +237,14 @@ runs:
fi
fi

- name: Filter excluded paths from diff
if: hashFiles('pr.diff') != '' && inputs.exclude-paths != ''
shell: bash
env:
EXCLUDE_PATHS: ${{ inputs.exclude-paths }}
run: |
node "$ACTION_PATH/dist/filter-diff.js" pr.diff "$EXCLUDE_PATHS"

- name: Split diff into chunks
if: hashFiles('pr.diff') != ''
id: chunk-diff
Expand Down Expand Up @@ -307,63 +319,11 @@ runs:
- name: Score file risk
if: hashFiles('pr.diff') != ''
shell: bash
env:
EXCLUDE_PATHS: ${{ inputs.exclude-paths }}
run: |
# Extract per-file diff stats (added lines, hunk count)
awk '
/^diff --git/ {
if (file != "") print file, added, hunks
file = $0; sub(/.*b\//, "", file)
added = 0; hunks = 0
}
/^@@/ { hunks++ }
/^\+[^+]/ { added++ }
END { if (file != "") print file, added, hunks }
' pr.diff > /tmp/file_diff_stats.txt

# Build final scores
jq -n '{}' > /tmp/file_risk_scores.json
while read -r file added hunks; do
SCORE=0

# Security-sensitive paths: +2
if echo "$file" | grep -qiE 'auth|security|crypto|session|secret|token|password|credential'; then
SCORE=$((SCORE + 2))
fi

# Large change (>100 added lines): +2
if [ "$added" -gt 100 ]; then
SCORE=$((SCORE + 2))
fi

# Many hunks (>3): +1
if [ "$hunks" -gt 3 ]; then
SCORE=$((SCORE + 1))
fi

# Test/doc/config files: score = 0
if echo "$file" | grep -qiE '_test\.go$|\.test\.[tj]sx?$|\.spec\.[tj]sx?$|test_.*\.py$|\.md$|\.ya?ml$|\.json$|\.toml$'; then
SCORE=0
fi

# Error handling patterns in diff for this file: +1
ERROR_LINES=$(awk -v f="$file" '
/^diff --git/ { cur=$0; sub(/.*b\//, "", cur) }
cur == f && /^\+[^+]/ && /catch|rescue|except|recover|error|panic/ { count++ }
END { print count+0 }
' pr.diff)
if [ "$ERROR_LINES" -gt 0 ]; then
SCORE=$((SCORE + 1))
fi

if jq --arg f "$file" --argjson s "$SCORE" '.[$f] = $s' /tmp/file_risk_scores.json > /tmp/file_risk_scores.tmp; then
mv /tmp/file_risk_scores.tmp /tmp/file_risk_scores.json
else
echo "⚠️ Failed to score $file, skipping"
rm -f /tmp/file_risk_scores.tmp
fi
done < /tmp/file_diff_stats.txt

rm -f /tmp/file_diff_stats.txt
set -euo pipefail
node "$ACTION_PATH/dist/score-risk.js" pr.diff "$EXCLUDE_PATHS"
Copy link
Copy Markdown
Contributor Author

@docker-agent docker-agent May 7, 2026

Choose a reason for hiding this comment

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

[MEDIUM] score-risk.js failure is silently masked by the trailing echo command

The Score file risk step has two commands with no set -euo pipefail:

node "$ACTION_PATH/dist/score-risk.js" pr.diff "$EXCLUDE_PATHS"
echo "✅ File risk scores: $(jq -c . /tmp/file_risk_scores.json)"

If score-risk.js exits non-zero (binary not found, parse error, write error), bash continues to the echo line. The echo exits 0 even if the embedded jq fails, so the step reports success while /tmp/file_risk_scores.json is absent or stale. The pr-review.yaml agent reads this file directly and will silently operate on wrong/missing risk data, potentially misrouting the review.

Compare: filter-diff is a single-command step — its node failure propagates correctly.

Fix: add set -euo pipefail before the node call, or restructure as a single node ... && echo ... chain:

set -euo pipefail
node "$ACTION_PATH/dist/score-risk.js" pr.diff "$EXCLUDE_PATHS"
echo "✅ File risk scores: $(jq -c . /tmp/file_risk_scores.json)"

Copy link
Copy Markdown
Contributor Author

@docker-agent docker-agent May 7, 2026

Choose a reason for hiding this comment

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

Fixed in 4bc6182. The Score file risk step now begins with set -euo pipefail:

run: |
  set -euo pipefail
  node "$ACTION_PATH/dist/score-risk.js" pr.diff "$EXCLUDE_PATHS"
  echo "✅ File risk scores: $(jq -c . /tmp/file_risk_scores.json)"

If score-risk.js exits non-zero the step now fails immediately rather than continuing to the echo and masking the error.

echo "✅ File risk scores: $(jq -c . /tmp/file_risk_scores.json)"

- name: Generate file history
Expand Down
Loading
Loading