feat(#568): make protected paths configurable via env var - #569
feat(#568): make protected paths configurable via env var#569ralphbean wants to merge 22 commits into
Conversation
PR Summary by QodoMake protected review paths configurable via REVIEW_PROTECTED_PATHS
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
|
🤖 Review · |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
🤖 Review · |
|
🤖 Review · |
|
🤖 Review · |
|
🤖 Finished Review · ✅ Success · Started 5:48 PM UTC · Completed 6:08 PM UTC |
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsMedium
Low
Previous run (3)ReviewFindingsMedium
Low
Previous run (4)ReviewFindingsMedium
Low
Labels: PR modifies review agent infrastructure (post-review.sh, harness config, skill definitions, eval runner) Next steps:
Previous run (5)ReviewFindingsMedium
Low
Previous run (6)ReviewFindingsMedium
Low
Previous run (7)ReviewFindingsMedium
Low
Previous run (8)ReviewFindingsHigh
Medium
Low
Next steps:
Previous run (9)ReviewFindingsMedium
Low
Previous run (10)ReviewFindingsMedium
Low
Previous run (11)ReviewFindingsHigh
Medium
Low
Labels: PR modifies review agent infrastructure (post-review.sh, SKILL.md, security-triage.md, harness config) Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Fix · ❌ Failure · Started 6:10 PM UTC · Completed 6:19 PM UTC |
|
🤖 Review · |
|
The fix agent completed, but the post-fix script failed before finishing. Workflow run: https://github.com/fullsend-ai/.fullsend/actions/runs/30569103309 Details: |
|
🤖 Review · |
|
🤖 Review · |
|
🤖 Review · |
|
🤖 Review · |
|
🤖 Finished Review · ✅ Success · Started 7:21 PM UTC · Completed 7:42 PM UTC |
Superseded by updated review
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
requires-manual-review
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only pass (no approve/request-changes). Two additional findings not yet covered by prior review threads or the automated review agent's summary on the current head commit.
|
🤖 Finished Review · ✅ Success · Started 9:09 PM UTC · Completed 9:30 PM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
removed
the
requires-manual-review
|
🤖 Finished Fix · ✅ Success · Started 9:31 PM UTC · Completed 9:43 PM UTC |
- Remove empty-string-disables-protection code path from post-review.sh; REVIEW_PROTECTED_PATHS="" now triggers fail-closed instead of silently disabling all protection (addresses High fail-open finding) - Add zero-length check after reading defaults file to prevent silent protection bypass when file contains only comments/blanks - Add env/ to default protected paths so the defaults file itself is protected from unreviewed modification - Update SKILL.md to say "if set and non-empty" for REVIEW_PROTECTED_PATHS to prevent LLM misinterpretation of empty-but-defined env var - Update eval runner to populate REVIEW_PROTECTED_PATHS with defaults from file when caller does not provide a value (avoids empty-string ambiguity) - Update docs/review.md to document fail-closed semantics for empty string - Replace explicit-empty-string-no-downgrade test with explicit-empty-string-aborts - Add file-fallback-comments-only-aborts test for defaults file edge case Addresses review feedback on #569
- docs/review.md: REVIEW_PROTECTED_PATHS description said empty string is "treated the same as unset (fail-closed)" but unset reads defaults while empty string aborts — these are different behaviors. Clarify. - eval case 003 annotation claimed to verify the direct file-read fallback branch in post-review.sh, but the eval harness always pre-populates REVIEW_PROTECTED_PATHS, so only the env-var branch runs. Correct the annotation to describe what's actually exercised. Addresses review feedback on #569 Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
…ECTED_PATHS REVIEW_PROTECTED_PATHS="" now disables protected-path enforcement entirely (deliberate operator opt-out), instead of failing closed. A value that parses to zero entries after trimming (e.g. stray or consecutive commas) still fails closed, since that's more likely a misconfiguration than an intentional opt-out — the abort message now includes the raw value to make that easier to diagnose. Addresses review feedback on #569. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
… tests Two review findings from PR #569: - eval/scripts/run-fullsend.sh: when the default protected-paths file is missing, emit_env'ing REVIEW_PROTECTED_PATHS="" gets interpreted by post-review.sh as a deliberate opt-out (protection disabled) rather than a fail-closed error. Exit 1 instead, matching post-review.sh's own missing-file guard. - scripts/post-review-test.sh: run_protected_paths_test only exported REVIEW_PROTECTED_PATHS when the test's protected_paths argument was non-empty, so file-fallback tests could silently inherit a stale value from the calling environment instead of exercising the defaults-file path. Unset it explicitly in the empty-argument case, with a new test that reproduces the leak. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 8:14 PM UTC · Completed 8:34 PM UTC |
| REVIEW_PROTECTED_PATHS: ".claude/,.cursor/,.gitattributes,.github/,.pre-commit-config.yaml,AGENTS.md,agents/,api-servers/,CLAUDE.md,CODEOWNERS,Containerfile,Dockerfile,env/,harness/,images/,plugins/,policies/,scripts/,skills/" | ||
|
|
||
| timeout_minutes: 20 | ||
|
|
There was a problem hiding this comment.
[medium] edge-case
The default REVIEW_PROTECTED_PATHS value adds env/ as a new protected path prefix that was NOT in the previous hardcoded list in post-review.sh. This is a behavioral change: any PR touching env/ files will now trigger a protected-path downgrade where it previously would not.
Suggested fix: If adding env/ is intentional, document the rationale explicitly. If unintentional, remove env/ from the default value in harness/review.yaml.
| - `plugins/**` — plugin definitions | ||
| - `scripts/**` — pre/post scripts (CI and deployment) | ||
| - `skills/**` — skill definitions | ||
| The orchestrator provides an **Active governance paths** list in the |
There was a problem hiding this comment.
[medium] runtime-mechanism
The hardcoded governance paths list was removed and replaced with a dependency on the orchestrator injecting an 'Active governance paths' section in the spawn prompt. If the orchestrator omits or malforms this section, the sub-agent silently loses all governance-path classification capability with no fallback.
Suggested fix: Add a defensive instruction with a minimal fallback list to use when no Active governance paths section is present.
| for _entry in "${PROTECTED_PATHS[@]}"; do | ||
| _entry="$(echo "${_entry}" | sed 's/^[[:space:]]*//;s/[[:space:]]*$//')" | ||
| [[ -n "${_entry}" ]] && _trimmed+=("${_entry}") | ||
| done |
There was a problem hiding this comment.
[low] GHA-workflow-command-injection
The ::error:: workflow command sanitizes the interpolated value using :: to : collapse, which is not idempotent (::: collapses to ::). The same file's REVIEW_FINDING_SEVERITY_THRESHOLD sanitization uses the more robust per-character stripping pattern. Exploitability is limited (requires CI config write access).
Suggested fix: Use per-character stripping (//%/ and //:/) matching the existing pattern in the same file.
| # agent's own output and the post-script's defense-in-depth filter. | ||
| # Best practice is to keep them in sync. Repos needing different values | ||
| # override these keys via harness `base:` composition. | ||
| runner: |
There was a problem hiding this comment.
[low] fail-open-risk
Setting REVIEW_PROTECTED_PATHS to an empty string disables all protected-path enforcement with a ::notice:: log message. This is a deliberate design decision (explicit opt-out), not an accidental bypass.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Automated review sweep — 3 findings verified against PR head (b6c0ad4) and deduplicated against existing review threads. All three are new (not previously raised despite the PR's extensive review history). See inline comments for details.
| _entry="$(echo "${_entry}" | sed 's/^[[:space:]]*//;s/[[:space:]]*$//')" | ||
| [[ -n "${_entry}" ]] && _trimmed+=("${_entry}") | ||
| done | ||
| PROTECTED_PATHS=("${_trimmed[@]}") |
There was a problem hiding this comment.
[HIGH] Empty-array expansion under set -u crashes on bash 3.2, masking the fail-closed error message
When REVIEW_PROTECTED_PATHS trims down to zero valid entries (e.g. ",,, ,"), this builds _trimmed=() in the loop above and then does PROTECTED_PATHS=("${_trimmed[@]}") before the ${#PROTECTED_PATHS[@]} -eq 0 check below. The script uses set -euo pipefail (line 22). Reproduced directly: /bin/bash -c 'set -u; a=(); b=("${a[@]}"); echo ok' fails with a[@]: unbound variable on bash 3.2.57 (the default /bin/bash on macOS, and still present on minimal/BusyBox-adjacent images). So expanding an empty _trimmed array crashes with a raw "unbound variable" error instead of ever reaching the intended ::error::...likely misconfigured... message a few lines down.
The degenerate-paths-aborts test in scripts/post-review-test.sh (around line 1173) exercises exactly this scenario (REVIEW_PROTECTED_PATHS=",,, ,") and greps stdout for "likely misconfigured" — that test would fail (wrong error text, though still non-zero exit) under bash <4.4.
Suggested fix: Guard the expansion, e.g. PROTECTED_PATHS=(); [[ ${#_trimmed[@]} -gt 0 ]] && PROTECTED_PATHS=("${_trimmed[@]}"), or build the array via a for-loop append that never dereferences a possibly-empty array's [@]. Consider adding a bash-3.2 (or similarly old) leg to the test matrix.
| # harness/review.yaml (not a ${VAR} passthrough), so it needs no handling | ||
| # here — only REVIEW_FINDING_SEVERITY_THRESHOLD is a real caller-supplied var. | ||
| if [[ "$AGENT" == "review" ]]; then | ||
| emit_env "REVIEW_FINDING_SEVERITY_THRESHOLD" "${REVIEW_FINDING_SEVERITY_THRESHOLD:-}" |
There was a problem hiding this comment.
[MEDIUM] Stale/incorrect rationale for asymmetric REVIEW_FINDING_SEVERITY_THRESHOLD handling causes eval runs to fail-closed unnecessarily
The comment above (lines 205-207) states "REVIEW_PROTECTED_PATHS is a literal default baked into harness/review.yaml (not a ${VAR} passthrough), so it needs no handling here — only REVIEW_FINDING_SEVERITY_THRESHOLD is a real caller-supplied var." Diffing this PR's head against the merge-base shows the REVIEW_FINDING_SEVERITY_THRESHOLD: "low" lines under both env.runner and env.sandbox in harness/review.yaml are unchanged context — they already existed as literal defaults before this PR, exactly like REVIEW_PROTECTED_PATHS now does. The asymmetric treatment is based on a false premise.
Practical consequence: post-review.sh's severity-validation block does REVIEW_FINDING_SEVERITY_THRESHOLD="${REVIEW_FINDING_SEVERITY_THRESHOLD:-}" followed by a case statement whose *) catch-all treats an empty string as invalid and posts {"action":"failure","reason":"tool-failure"} before exiting 1. Since this line unconditionally emits ${REVIEW_FINDING_SEVERITY_THRESHOLD:-} (empty when the caller hasn't exported it), any review-agent eval run that doesn't pre-export this var — including this PR's own new 003-protected-path-downgrade case, whose input.yaml never sets it — will fail with a tool-failure unrelated to the protected-paths feature under test.
Suggested fix: Default explicitly to match harness/review.yaml's documented default, e.g. emit_env "REVIEW_FINDING_SEVERITY_THRESHOLD" "${REVIEW_FINDING_SEVERITY_THRESHOLD:-low}", and correct the now-inaccurate comment about which variable needs handling here.
| fi | ||
| fi | ||
|
|
||
| if [[ ${#PROTECTED_PATHS[@]} -gt 0 ]]; then |
There was a problem hiding this comment.
[MEDIUM] Disabling protected-path enforcement silently disables the unrelated "PR has no changed files" safety check
The pre-existing gh pr view --json files fetch and its PR_FILES empty-check ("Failed to fetch PR files or PR has no changed files — refusing to approve", a few lines below) is a defensive check unrelated to protected paths, but it's now wrapped entirely inside if [[ ${#PROTECTED_PATHS[@]} -gt 0 ]]; then. When an operator sets REVIEW_PROTECTED_PATHS="" to deliberately opt out of protected-path enforcement (a documented, supported use case per docs/review.md), they also silently lose this independent safety net that refuses to approve a PR whose file list couldn't be fetched or is empty.
Suggested fix: Keep the PR_FILES fetch and empty-check outside/independent of the ${#PROTECTED_PATHS[@]} -gt 0 guard so that safety net always applies to every approve action, and only gate the protected-path pattern-matching loop itself on a non-empty PROTECTED_PATHS.
When the REVIEW_PROTECTED_PATHS environment variable is set (comma- separated list of path prefixes), it replaces the built-in default list. When unset, the existing defaults are preserved. Changes: - post-review.sh: parse REVIEW_PROTECTED_PATHS env var into the array used for protected-path matching, with whitespace trimming - post-review-test.sh: add 5 integration tests covering default behavior, custom overrides, whitespace handling, and non-matches - SKILL.md: document the override mechanism - docs/review.md: add REVIEW_PROTECTED_PATHS to the Variables table Closes #568 Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
Add env/default-review-protected-paths.txt as the single source of truth for the default protected paths list. post-review.sh reads REVIEW_PROTECTED_PATHS (comma-separated) when set, otherwise falls back to the defaults file. If neither is available, it aborts. Remove duplicated path lists from SKILL.md and security-triage.md — both now reference the env var / defaults file instead. Changes: - env/default-review-protected-paths.txt: new canonical defaults file - scripts/post-review.sh: env var → file fallback → abort - scripts/post-review-test.sh: 8 tests covering env var override, file fallback, whitespace trimming, and missing-file abort - skills/pr-review/SKILL.md: reference env var and defaults file - skills/pr-review/sub-agents/security-triage.md: reference env var - docs/review.md: add REVIEW_PROTECTED_PATHS to Variables table - scripts/post-code.sh, post-fix.sh: update comments Closes #568 Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
…rness Add eval/review/cases/001-protected-path-downgrade — the first review eval case. A PR pins actions/checkout to a full SHA in .github/workflows/ ci.yml (a default protected path). The review agent should approve the change but post-review.sh must downgrade the approval because .github/ is protected. Expected outcome: requires-manual-review label, no ready-for-merge. Also restructure harness/review.yaml: - Rename runner_env → env.runner - Add env.sandbox with REVIEW_PROTECTED_PATHS and REVIEW_FINDING_SEVERITY_THRESHOLD so the agent can read them Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
Reserve 001/002 for existing unmerged review eval cases. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
Empty entries from leading/trailing/consecutive commas in REVIEW_PROTECTED_PATHS would match all files, causing every approval to be downgraded. Filter them out after trimming whitespace. Also rebuilds bundled scripts (post-code.sh, post-fix.sh). Signed-off-by: Ralph Bean <rbean@redhat.com> Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
The setup-fixture hook pushes test content to an ephemeral GitHub repo. Pushing .github/workflows/ files requires a token with `workflow` scope, which the CI token does not have. Replace the workflow fixture with a dependabot.yml file — still under .github/ so the protected-path downgrade logic is exercised identically. Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
- Add fail-closed guard: abort if PROTECTED_PATHS is empty after parsing a degenerate REVIEW_PROTECTED_PATHS value (e.g. ",,,"). - Add test for degenerate input. - Fix security-triage sub-agent: the orchestrator now resolves governance paths and includes them in the spawn prompt (Part 2) instead of referencing REVIEW_PROTECTED_PATHS, which the Haiku sub-agent cannot read at runtime. Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
fullsend validates that all ${VAR} references in harness YAML resolve
to set host variables. REVIEW_PROTECTED_PATHS and
REVIEW_FINDING_SEVERITY_THRESHOLD are optional (post-review.sh falls
back to defaults) but the harness references them unconditionally.
Emit empty defaults in run-fullsend.sh for the review agent so
fullsend's env validation passes.
Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com>
Signed-off-by: Ralph Bean <rbean@redhat.com>
The eval fixture had no linked issue, so the agent chose request-changes for the protected-path finding. GitHub returns 422 when the same token that created the PR tries to submit a request-changes review (self-review). Add a seed issue and link it in the PR body so the agent has sufficient context to approve — which post-review.sh then downgrades to comment. Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
The review agent used 41 turns (partly due to model availability retries). Bump the budget from 30 to 50 to accommodate variance. Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
CI failed because the review agent cost $3.17, exceeding the $3.00 budget. Raise to $4.00 for headroom. Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
…only Move protected-paths resolution and fail-closed guards inside the approve conditional so non-approve actions (comment, reject, request-changes) are not blocked by degenerate REVIEW_PROTECTED_PATHS values or a missing defaults file. Also distinguish set-but-empty REVIEW_PROTECTED_PATHS="" (disables protection) from unset (falls through to defaults file). This lets repo owners explicitly opt out of protected-path enforcement. Signed-off-by: Ralph Bean <rbean@redhat.com> Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
- Remove empty-string-disables-protection code path from post-review.sh; REVIEW_PROTECTED_PATHS="" now triggers fail-closed instead of silently disabling all protection (addresses High fail-open finding) - Add zero-length check after reading defaults file to prevent silent protection bypass when file contains only comments/blanks - Add env/ to default protected paths so the defaults file itself is protected from unreviewed modification - Update SKILL.md to say "if set and non-empty" for REVIEW_PROTECTED_PATHS to prevent LLM misinterpretation of empty-but-defined env var - Update eval runner to populate REVIEW_PROTECTED_PATHS with defaults from file when caller does not provide a value (avoids empty-string ambiguity) - Update docs/review.md to document fail-closed semantics for empty string - Replace explicit-empty-string-no-downgrade test with explicit-empty-string-aborts - Add file-fallback-comments-only-aborts test for defaults file edge case Addresses review feedback on #569
- docs/review.md: REVIEW_PROTECTED_PATHS description said empty string is "treated the same as unset (fail-closed)" but unset reads defaults while empty string aborts — these are different behaviors. Clarify. - eval case 003 annotation claimed to verify the direct file-read fallback branch in post-review.sh, but the eval harness always pre-populates REVIEW_PROTECTED_PATHS, so only the env-var branch runs. Correct the annotation to describe what's actually exercised. Addresses review feedback on #569 Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
…ECTED_PATHS REVIEW_PROTECTED_PATHS="" now disables protected-path enforcement entirely (deliberate operator opt-out), instead of failing closed. A value that parses to zero entries after trimming (e.g. stray or consecutive commas) still fails closed, since that's more likely a misconfiguration than an intentional opt-out — the abort message now includes the raw value to make that easier to diagnose. Addresses review feedback on #569. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
…tion fullsend-ai-review pointed out the ::error:: workflow command interpolates the raw REVIEW_PROTECTED_PATHS value unsanitized. Apply the same newline/CR/:: stripping already used for label-actions sanitization before it hits the log. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
waynesun09 noticed the three "missing/comments-only defaults file"
tests all copied post-review.sh into distinctly-named siblings of the
same TMPDIR, so SCRIPT_DIR/../env collapsed to the same path for all
three regardless of the leaf directory name — making them silently
order-dependent. Give each test its own case-<name>/{scripts,env}
parent so the path can't collide.
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Ralph Bean <rbean@redhat.com>
…KILL.md waynesun09 found that the review agent's own instructions still said "if not set or empty, use defaults" — contradicting post-review.sh's current behavior of treating explicit-empty as a deliberate opt-out. That meant the review agent would still emit a protected-path finding (and the schema would still block approve) even when an operator had opted out via REVIEW_PROTECTED_PATHS="". Spell out the three-way resolution (set/non-empty, set/empty, unset) in both the "Protected paths" section and the security-triage governance-paths step so they match the script. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
…HS in eval harness
waynesun09 flagged two issues in run-fullsend.sh's REVIEW_PROTECTED_PATHS
handling:
- `[[ -n "${REVIEW_PROTECTED_PATHS:-}" ]]` treated explicit-empty the
same as unset, so the eval harness could never exercise the
opt-out path post-review.sh now supports. Switch to the same
`${VAR+set}` check post-review.sh uses.
- The defaults-file parsing used `sed | paste` without trimming
per-entry whitespace, diverging from post-review.sh's trim-then-join
logic. Match it so a future indented entry in the defaults file
can't desync the two parsers.
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Ralph Bean <rbean@redhat.com>
… tests Two review findings from PR #569: - eval/scripts/run-fullsend.sh: when the default protected-paths file is missing, emit_env'ing REVIEW_PROTECTED_PATHS="" gets interpreted by post-review.sh as a deliberate opt-out (protection disabled) rather than a fail-closed error. Exit 1 instead, matching post-review.sh's own missing-file guard. - scripts/post-review-test.sh: run_protected_paths_test only exported REVIEW_PROTECTED_PATHS when the test's protected_paths argument was non-empty, so file-fallback tests could silently inherit a stale value from the calling environment instead of exercising the defaults-file path. Unset it explicitly in the empty-argument case, with a new test that reproduces the leak. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
…ew.yaml
Replaces the separate env/default-review-protected-paths.txt file and its
three-way (set / set-empty / unset) resolution ladder — previously
duplicated across post-review.sh, eval/scripts/run-fullsend.sh, and
skills/pr-review/SKILL.md — with a single literal default declared
directly in harness/review.yaml. Repos needing a different list override
it via harness composition instead of an env var; unset is now a hard
misconfiguration error rather than a file-read fallback.
- harness/review.yaml: REVIEW_PROTECTED_PATHS is a literal default in
both the runner and sandbox env stanzas, not a ${VAR} passthrough.
- post-review.sh: collapsed to two cases (non-empty / explicitly-empty);
removed the defaults-file lookup entirely.
- run-fullsend.sh: removed the now-dead default-computation block.
- Deleted env/default-review-protected-paths.txt.
- Updated SKILL.md, docs/review.md, and the 003-protected-path-downgrade
eval case's annotations to match.
- post-review-test.sh: removed the file-fallback tests, simplified the
two "missing defaults" tests to plain unset-aborts tests, and exports
a module-level REVIEW_PROTECTED_PATHS default so generic integration
tests reflect that harness/review.yaml always sets it in production.
Assisted-by: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Ralph Bean <rbean@redhat.com>
…s tests main's severity-threshold refactor (merged after this branch diverged) made post-review.sh hard-fail when REVIEW_FINDING_SEVERITY_THRESHOLD is unset or invalid, rather than silently defaulting to "low". The protected-paths test helpers introduced here predate that change and didn't export it, so rebasing onto main broke them. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
b6c0ad4 to
bb20f15
Compare
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only pass (no approve/request-changes). One finding below.
| # Best practice is to keep them in sync. Repos needing different values | ||
| # override these keys via harness `base:` composition. | ||
| runner: | ||
| REVIEW_FINDING_SEVERITY_THRESHOLD: "low" |
There was a problem hiding this comment.
[MEDIUM] PR description's claim about newly wiring REVIEW_FINDING_SEVERITY_THRESHOLD is stale/false
The PR body currently states: "Wires REVIEW_FINDING_SEVERITY_THRESHOLD into harness/review.yaml's runner env. post-review.sh already read it on the runner side, but it was never plumbed through runner_env — only the sandbox got it via env/review.env. So runner-side severity filtering silently defaulted to low before this PR."
This is factually stale as of head bb20f15. The diff for harness/review.yaml shows runner: REVIEW_FINDING_SEVERITY_THRESHOLD: "low" and sandbox: REVIEW_FINDING_SEVERITY_THRESHOLD: "low" as unchanged context — the only lines this PR actually adds to this file are the two new REVIEW_PROTECTED_PATHS: ... entries. Git history shows the commit that originally added REVIEW_FINDING_SEVERITY_THRESHOLD to env.runner/env.sandbox is an ancestor of the merge-base between origin/main and this branch — i.e. that wiring already existed on main before this PR branched, contradicting the claim that this PR is the one doing the wiring. This looks like a holdover from an earlier iteration of the PR (before it was rebased onto a main that already contained that change) and from an earlier, now-resolved review thread where this explanation was added to the description in response to a since-superseded concern.
Suggestion: Correct the PR description to remove or rewrite the harness/review.yaml claim so it accurately reflects that this PR's only change to that file is adding REVIEW_PROTECTED_PATHS; REVIEW_FINDING_SEVERITY_THRESHOLD's runner-env wiring predates this branch and shouldn't be attributed to it.
2 participants
Summary
REVIEW_PROTECTED_PATHSenvironment variable to override the hardcoded protected-path list inpost-review.sh. Comma-separated path prefixes, whitespace-trimmed.REVIEW_FINDING_SEVERITY_THRESHOLDintoharness/review.yaml's runner env.post-review.shalready read it on the runner side, but it was never plumbed throughrunner_env— only the sandbox got it viaenv/review.env. So runner-side severity filtering silently defaulted tolowbefore this PR.env/default-review-protected-paths.txtfile thatpost-review.sh,run-fullsend.sh, andSKILL.mdeach had to independently resolve via a three-way (set / set-empty / unset) ladder. That's now replaced with a single literal default declared directly inharness/review.yaml'senv.runner/env.sandboxstanzas (matching the existing constant-value pattern already used for things likeMAX_RETRIESinharness/fix.yaml). Repos needing a different list override it via harness composition instead of an env var. Unset is now a hard misconfiguration error rather than a file-read fallback.env/default-review-protected-paths.txt.post-review.sh: collapsed to two cases (non-empty / explicitly-empty).run-fullsend.sh: removed the now-dead default-computation block.SKILL.md,docs/review.md, and the003-protected-path-downgradeeval case's annotations to match.skills/pr-review/SKILL.mdanddocs/review.mdto document the variable.Closes #568
Test plan
bash scripts/post-review-test.sh)bash scripts/validate-output-schema-test.sh)🤖 Generated with Claude Code