feat(#568): make protected paths configurable via env var - #569
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
Previous run (5)ReviewFindingsMedium
Low
Labels: PR modifies review agent infrastructure (post-review.sh, harness config, skill definitions, eval runner) Next steps:
Previous run (6)ReviewFindingsMedium
Low
Previous run (7)ReviewFindingsMedium
Low
Previous run (8)ReviewFindingsMedium
Low
Previous run (9)ReviewFindingsHigh
Medium
Low
Next steps:
Previous run (10)ReviewFindingsMedium
Low
Previous run (11)ReviewFindingsMedium
Low
Previous run (12)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
…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.
- Sanitize REVIEW_PROTECTED_PATHS before GHA error interpolation by stripping every '%' and ':' character individually, matching the REVIEW_FINDING_SEVERITY_THRESHOLD sanitization above. The previous "::" -> ":" collapse was not idempotent and didn't strip %0A/%0D, which GHA decodes as literal newlines in workflow command params. - Guard the PROTECTED_PATHS array assignment after trimming so an empty _trimmed array doesn't crash with "unbound variable" under `set -u` on bash < 4.4 (e.g. bash 3.2 on macOS), which would mask the intended fail-closed misconfiguration error. - Keep the "PR has no changed files" safety-net check independent of protected-path enforcement being enabled, so it still applies when an operator explicitly disables protected-path enforcement via REVIEW_PROTECTED_PATHS="". - Default REVIEW_FINDING_SEVERITY_THRESHOLD to "low" in the eval runner to match harness/review.yaml's documented default, and correct the comment's stale claim that it's the only caller-supplied var needing a default here. - Add regression test coverage for the PR-files safety-net fix. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
The 3c-1 procedure's own numbered item 2 (governance-paths resolution) collided with the orchestrator's separate global step 2 (large-PR per-file-mode selection), which is also referenced as "step 2" earlier in the same subsection. Spell out which "step 2" each reference means. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
agents/code.md and agents/fix.md said protected paths are "defined in" post-review.sh, but the list is configured via REVIEW_PROTECTED_PATHS in harness/review.yaml and only enforced by post-review.sh. Clarify that split. Also rename post-review.sh's leading-underscore locals (_trimmed, _entry, _sanitized_paths) to match the file's existing naming convention, per review feedback on PR #569. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
Rename the internal PROTECTED_PATHS array to REVIEW_ACTIVE_PROTECTED_PATHS to follow the codebase's namespace-qualified naming convention (e.g. REVIEW_CONTROL_LABELS) and to avoid reading like the REVIEW_PROTECTED_PATHS env var it's derived from, per review feedback on PR #569. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
|
Following up on the findings here (#569 (comment)):
Still open, and these feel like your call rather than mine:
Leaving those open for now. |
REVIEW_PROTECTED_PATHS' default is now hardcoded verbatim in three places (harness/review.yaml's env.runner and env.sandbox, and this test file) with no structural source of truth since env/default-review-protected-paths.txt was removed. Add a test that compares this file's default against harness/review.yaml via yq so a future edit that updates one copy and misses another fails loudly instead of silently testing against a stale default. Skips (doesn't fail) when yq is unavailable, per review feedback on PR #569. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
env/ was not in the previous hardcoded protected-paths list in post-review.sh; adding it as a default is an unrelated scope increase that changes downgrade behavior for any PR touching env/ files. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only pass (no approve/request-changes). One additional PR-level finding not covered by a specific diff line, plus one inline finding below.
[MEDIUM] No CI has run against the current PR head (fed0477); only the DCO check has executed
Live check via gh api repos/fullsend-ai/agents/commits/fed0477.../check-runs confirms only the DCO check has run for the current head — there is no CI / Script-tests / Functional-tests run recorded for this SHA. gh pr view 569 --json mergeStateStatus currently reports BLOCKED (mergeable is MERGEABLE, so the block is not a merge conflict). The PR's test-plan checklist ("All 69 tests pass", "pre-commit clean") and every prior automated/bot review were generated against earlier commits — the last recorded workflow run predates fed0477. This isn't a code defect (local runs of scripts/post-review-test.sh pass), but the automated verification gate the team relies on has not actually executed against the SHA that would be merged, and this gap isn't flagged anywhere in the PR's existing review comments.
Suggestion: push an empty commit or otherwise re-trigger the CI/Script-tests/Functional-tests workflows against fed0477 (or whatever the final head becomes) before merging, so the required status checks reflect the actual merged content rather than a stale SHA.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Approving — mechanism is sound
Verified the enforcement end to end, not just the description:
- No regression in the default. The
REVIEW_PROTECTED_PATHSdefault inharness/review.yamlis byte-for-byte the same 18-path list previously hardcoded inpost-review.sh— nothing dropped. - Fail-closed on misconfiguration. Unset →
::error::+exit 1; comma-noise (e.g.",,,") →exit 1. The accidental case (forgetting to set it) refuses to approve rather than opening up. - Opt-out is operator-gated, not agent-reachable. Enforcement reads the runner-scoped value host-side in the post-script; the sandbox only receives its own copy and can't change what
post-review.shsees. The only way to set it empty is editing the deployed harness — andharness/is itself protected, so a PR attempting to weaken it gets downgraded. Self-protecting. - Single source of truth for the agent. The sub-agent's hardcoded list is replaced by the orchestrator-injected "Active governance paths," resolved with identical non-empty/explicit-empty semantics, and
post-review.shremains the sole enforcement point — agent misclassification can't grant approval. - Verified green:
post-review-test.sh75/75,validate-output-schema-test.shall pass,shellcheckclean onpost-review.sh. The earlier::→:non-idempotent-sanitization concern is resolved — the current code strips every%and:outright.
The REVIEW_PROTECTED_PATHS="" full opt-out is a real capability, but it's correctly gated to harness operators and is what #568 asked for. LGTM on the mechanics.
Requests before merge (non-blocking on the approval, but please address)
env/is not in the default protected list. Pre-existing (the old hardcoded list didn't include it either), but now that per-repo config lives in env-adjacent places, an agent-authored PR touchingenv/would be eligible for auto-approval. Please either addenv/to the default list or drop a one-line note inharness/review.yamlexplaining why it's intentionally excluded.- Document the two config patterns.
REVIEW_PROTECTED_PATHSuses a literal default baked into the harness (must-always-be-set, fail-closed if unset) whileREVIEW_FINDING_SEVERITY_THRESHOLDuses a${VAR}passthrough with a script-side scalar fallback. The inconsistency is defensible but currently undocumented — a short comment near the env stanza would save the next reader.
Approving on the mechanism; the two items above are worth folding in before merge (the blocked label gates that separately).
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Approving — mechanism is sound
Verified the enforcement end to end, not just the description:
- No regression in the default. The
REVIEW_PROTECTED_PATHSdefault inharness/review.yamlis byte-for-byte the same 18-path list previously hardcoded inpost-review.sh— nothing dropped. - Fail-closed on misconfiguration. Unset →
::error::+exit 1; comma-noise (e.g.",,,") →exit 1. The accidental case (forgetting to set it) refuses to approve rather than opening up. - Opt-out is operator-gated, not agent-reachable. Enforcement reads the runner-scoped value host-side in the post-script; the sandbox only receives its own copy and can't change what
post-review.shsees. The only way to set it empty is editing the deployed harness — andharness/is itself protected, so a PR attempting to weaken it gets downgraded. Self-protecting. - Single source of truth for the agent. The sub-agent's hardcoded list is replaced by the orchestrator-injected "Active governance paths," resolved with identical non-empty/explicit-empty semantics, and
post-review.shremains the sole enforcement point — agent misclassification can't grant approval. - Verified green:
post-review-test.sh75/75,validate-output-schema-test.shall pass,shellcheckclean onpost-review.sh. The earlier::→:non-idempotent-sanitization concern is resolved — the current code strips every%and:outright.
The REVIEW_PROTECTED_PATHS="" full opt-out is a real capability, but it's correctly gated to harness operators and is what #568 asked for. LGTM on the mechanics.
Requests before merge (non-blocking on the approval, but please address)
env/is not in the default protected list. Pre-existing (the old hardcoded list didn't include it either), but now that per-repo config lives in env-adjacent places, an agent-authored PR touchingenv/would be eligible for auto-approval. Please either addenv/to the default list or drop a one-line note inharness/review.yamlexplaining why it's intentionally excluded.- Document the two config patterns.
REVIEW_PROTECTED_PATHSuses a literal default baked into the harness (must-always-be-set, fail-closed if unset) whileREVIEW_FINDING_SEVERITY_THRESHOLDuses a${VAR}passthrough with a script-side scalar fallback. The inconsistency is defensible but currently undocumented — a short comment near the env stanza would save the next reader.
Approving on the mechanism; the two items above are worth folding in before merge (the blocked label gates that separately).
Duplicate — superseded by the identical approval directly below (a CLI/API double-submit). See #4884064714 for the review content.
waynesun09 pointed out the required-env-vars comment block omitted REVIEW_PROTECTED_PATHS, even though it's effectively required for any approve action. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
|
Intentional — this is the PR that changes those paths. Same as the review-thread version of this finding: #569 (comment) |
|
🤖 Finished Review · ✅ Success · Started 3:05 PM UTC · Completed 3:22 PM UTC Commit: |
There was a problem hiding this comment.
See the review comment for full details.
Note: The following inline comments could not be posted on the diff (GitHub returned 422) and are included here instead:
skills/pr-review/sub-agents/security-triage.md:40: [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. The actual enforcement in post-review.sh is unaffected, so this is a defense-in-depth gap.
Suggested fix: Add a fallback in security-triage.md: if the 'Active governance paths' section is absent, classify all infrastructure-looking paths as security-critical.
agents/fix.md(file-level): Line 85 · [low] stale-hardcoded-list
Contains a hardcoded protected paths list that is now a second copy of the canonical list in harness/review.yaml. While the lists currently match, future edits will silently diverge.
harness/review.yaml:51: [low] fail-open-risk
Setting REVIEW_PROTECTED_PATHS to an empty string disables all protected-path enforcement. This is a deliberate design decision (explicit opt-out), not an accidental bypass.
eval/scripts/run-fullsend.sh:210: [low] scope-exceeded
Adds REVIEW_FINDING_SEVERITY_THRESHOLD passthrough, a separate concern from the protected-path configurability requested in issue #568. Practically necessary for the new eval case.
| # Trim leading/trailing whitespace and drop empty entries. | ||
| trimmed=() | ||
| for entry in "${REVIEW_ACTIVE_PROTECTED_PATHS[@]}"; do | ||
| entry="$(echo "${entry}" | sed 's/^[[:space:]]*//;s/[[:space:]]*$//')" |
There was a problem hiding this comment.
[MEDIUM] Whitespace-trim loop uses echo on user-controlled entries, silently dropping dash-prefixed paths
entry="$(echo "${entry}" | sed 's/^[[:space:]]*//;s/[[:space:]]*$//')" pipes a REVIEW_PROTECTED_PATHS entry through bash's echo builtin before trimming. Reproduced directly on bash 5.3.15: entry="-n", entry="-e", and entry="-E" all produce empty output because echo interprets them as flags rather than printing them literally. After trimming, the entry is empty and is silently dropped from REVIEW_ACTIVE_PROTECTED_PATHS instead of being kept or triggering the script's fail-closed misconfiguration error.
Failure scenario: an operator configures REVIEW_PROTECTED_PATHS to include an entry like -n or a path segment matching one of echo's recognized flags; that entry is silently discarded during trimming, so files under that prefix are no longer protected and an approve action touching them is not downgraded — a silent fail-open for that specific configured prefix, inconsistent with the fail-closed design used elsewhere in this function (e.g. the explicit comma-noise/zero-entries abort a few lines below).
Suggestion: use printf '%s' "${entry}" instead of echo "${entry}" when piping into sed, e.g. entry="$(printf '%s' "${entry}" | sed 's/^[[:space:]]*//;s/[[:space:]]*$//')", to avoid echo's flag-parsing behavior on user-supplied content.
|
🤖 Finished Retro · ✅ Success · Started 3:34 PM UTC · Completed 3:49 PM UTC Commit: |
Retro: PR #569 — Make protected paths configurable via env varTimelinePR #569 (closing issue #568) made Key events:
Review Quality AssessmentThe review agent caught 7 of 22 findings that the human reviewer identified (fully or partially). 15 of 22 human findings were completely missed by the review agent. The agent performed well on surface-level code analysis (variable naming, sanitization gaps, fail-open risks) but systematically missed findings requiring:
Fix Agent AssessmentThe fix agent ran 2 iterations. Iteration 1 failed due to a push race condition (concurrent human commits made the worktree stale). Iteration 2 succeeded, correctly addressing 7/8 findings with a well-reasoned disagreement on the 8th. Post-fix, ~15 more human commits were needed for design-level decisions the fix agent appropriately did not attempt autonomously. Existing Issues — Skipped Proposals
Proposals filed
|
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_THRESHOLD's wiring intoharness/review.yaml's runner/sandbox env predates this branch. This PR's only change to that file is adding the twoREVIEW_PROTECTED_PATHSentries.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