feat(#568): make protected paths configurable via env var

[ralphbean]

Summary

• Adds REVIEW_PROTECTED_PATHS environment variable to override the hardcoded protected-path list in post-review.sh. Comma-separated path prefixes, whitespace-trimmed.
• REVIEW_FINDING_SEVERITY_THRESHOLD's wiring into harness/review.yaml's runner/sandbox env predates this branch. This PR's only change to that file is adding the two REVIEW_PROTECTED_PATHS entries.
• Scope increase: the original design had a separate env/default-review-protected-paths.txt file that post-review.sh, run-fullsend.sh, and SKILL.md each had to independently resolve via a three-way (set / set-empty / unset) ladder. That's now replaced with a single literal default declared directly in harness/review.yaml's env.runner/env.sandbox stanzas (matching the existing constant-value pattern already used for things like MAX_RETRIES in harness/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.
  • Deleted 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.
  • Updated SKILL.md, docs/review.md, and the 003-protected-path-downgrade eval case's annotations to match.
• Updates skills/pr-review/SKILL.md and docs/review.md to document the variable.

Closes #568

Test plan

• All 69 tests pass (bash scripts/post-review-test.sh)
• Schema validation tests pass (bash scripts/validate-output-schema-test.sh)
• pre-commit (shellcheck, yaml, secrets) clean on all changed files

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Make protected review paths configurable via REVIEW_PROTECTED_PATHS

✨ Enhancement 🧪 Tests 📝 Documentation 🕐 20-40 Minutes

AI Description

• Add REVIEW_PROTECTED_PATHS env var to override the default protected-path list.
• Preserve existing defaults when the variable is unset; trim whitespace in overrides.
• Add integration tests and docs covering override behavior and examples.

Diagram

graph TD
  A["CI env: REVIEW_PROTECTED_PATHS"] --> B["scripts/post-review.sh"] --> C["Protected path match"] --> D["Downgrade approve to comment"]
  B --> E["gh pr view --json files"] --> F["PR file list"] --> C
  T["scripts/post-review-test.sh"] --> B
  G["Docs (review.md, SKILL.md)"] --> A

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Additive override (extend defaults)

• ➕ Keeps baseline protections while allowing repos to add extra paths safely
• ➕ Avoids accidental weakening by replacing the list entirely
• ➖ Harder to express 'remove one default' without additional syntax
• ➖ Slightly more complex UX and parsing logic

2. Config file in repo (e.g., .review/protected-paths)

• ➕ Versioned, reviewable changes to protections
• ➕ More discoverable than CI env configuration
• ➖ Protected-path logic becomes self-referential (changing config could be protected)
• ➖ More moving parts for CI/setup across repos

3. Support glob patterns (not just prefixes)

• ➕ More expressive matching (e.g., **/workflows/*.yml)
• ➕ Can reduce false positives/negatives for edge cases
• ➖ More complex matching semantics and escaping rules in bash
• ➖ Higher risk of misconfiguration compared to simple prefix rules

Recommendation: The PR’s env-var replacement approach is the simplest operationally for CI and keeps default behavior unchanged when unset. If teams are likely to want to add protections more often than replace them, consider a follow-up to support an additive mode (e.g., REVIEW_PROTECTED_PATHS_MODE=extend) while keeping the current replacement behavior as the explicit override.

Files changed (4) +114 / -9

Enhancement (1) +12 / -2

post-review.shMake protected paths configurable via REVIEW_PROTECTED_PATHS +12/-2

Make protected paths configurable via REVIEW_PROTECTED_PATHS

• Renames the hardcoded array to 'DEFAULT_PROTECTED_PATHS' and introduces runtime selection of 'PROTECTED_PATHS'. When 'REVIEW_PROTECTED_PATHS' is set, parses it as a comma-separated list and trims whitespace before matching; otherwise uses defaults.

scripts/post-review.sh

Tests (1) +92 / -2

post-review-test.shAdd integration tests for protected-path overrides +92/-2

Add integration tests for protected-path overrides

• Allows the mocked 'gh pr view --json files' response to be set via 'MOCK_PR_FILES'. Adds a helper and five integration tests verifying default behavior, override replacement semantics, whitespace trimming, and non-matching behavior.

scripts/post-review-test.sh

Documentation (2) +10 / -5

review.mdDocument REVIEW_PROTECTED_PATHS in the variables table +4/-3

Document REVIEW_PROTECTED_PATHS in the variables table

• Adds 'REVIEW_PROTECTED_PATHS' to the documented CI variables, describing comma-separated prefix semantics and default behavior. Tweaks surrounding wording to reflect multiple variables and clarifies severity-filtering downgrade text.

docs/review.md

SKILL.mdExplain how to override the protected paths list +6/-2

Explain how to override the protected paths list

• Updates the protected-paths section to describe the default list as such and documents the 'REVIEW_PROTECTED_PATHS' env var override behavior. Adjusts guidance text to reference the active protected paths list rather than only defaults.

skills/pr-review/SKILL.md

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 4:59 PM UTC · Ended 5:07 PM UTC
Commit: c8f60ed · View workflow run →

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. scripts/ and skills/ modified ✓ Resolved 📜 Skill insight § Compliance

Description

This PR changes files under protected governance/infrastructure paths (scripts/, skills/). Per
policy, PRs touching protected paths must not be auto-approved and require human approval.

Code

scripts/post-review.sh[R188-196]

+if [[ -n "${REVIEW_PROTECTED_PATHS:-}" ]]; then
+  IFS=',' read -ra PROTECTED_PATHS <<< "${REVIEW_PROTECTED_PATHS}"
+  # Trim leading/trailing whitespace from each entry.
+  for i in "${!PROTECTED_PATHS[@]}"; do
+    PROTECTED_PATHS[i]="$(echo "${PROTECTED_PATHS[i]}" | sed 's/^[[:space:]]*//;s/[[:space:]]*$//')"
+  done
+else
+  PROTECTED_PATHS=("${DEFAULT_PROTECTED_PATHS[@]}")
+fi

Relevance

●●● Strong

Protected-path governance is core review policy; team documents/maintains manual-review labeling for
such cases.

PR-#389

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1538392 requires that PRs modifying protected paths (including scripts/ and
skills/) must not be auto-approved and must raise a finding. The diff includes changes to
scripts/post-review.sh, scripts/post-review-test.sh, and skills/pr-review/SKILL.md, which are
within the protected path set.

scripts/post-review.sh[161-196]
scripts/post-review-test.sh[945-1034]
skills/pr-review/SKILL.md[986-1019]
Skill: pr-review

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
This PR modifies protected governance/infrastructure paths (e.g., `scripts/`, `skills/`), which must not be auto-approved.

## Issue Context
The compliance requirement mandates raising a protected-path finding and ensuring a human reviewer explicitly approves the PR when protected paths are touched.

## Fix Focus Areas
- scripts/post-review.sh[188-196]
- scripts/post-review-test.sh[945-1034]
- skills/pr-review/SKILL.md[986-1019]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Empty prefix matches all ✓ Resolved 🐞 Bug ☼ Reliability

Description

If REVIEW_PROTECTED_PATHS contains an empty/whitespace-only entry (e.g., ",deploy/" or "deploy/,
,manifests/"), the trim step leaves an empty string in PROTECTED_PATHS. The later check `[[ "$file"
== "$pattern"* ]] then matches every file when pattern is empty, so all approve` actions are
downgraded regardless of what the PR touches.

Code

scripts/post-review.sh[R188-193]

+if [[ -n "${REVIEW_PROTECTED_PATHS:-}" ]]; then
+  IFS=',' read -ra PROTECTED_PATHS <<< "${REVIEW_PROTECTED_PATHS}"
+  # Trim leading/trailing whitespace from each entry.
+  for i in "${!PROTECTED_PATHS[@]}"; do
+    PROTECTED_PATHS[i]="$(echo "${PROTECTED_PATHS[i]}" | sed 's/^[[:space:]]*//;s/[[:space:]]*$//')"
+  done

Relevance

●●● Strong

Deterministic bug: empty trimmed entry becomes empty prefix and matches all; team usually accepts
hardening fixes.

PR-#284
PR-#38

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The script trims each comma-split entry but never filters entries that become empty; later it
performs a Bash prefix/glob match against each entry, and an empty prefix matches any path, causing
protected matches for all files.

scripts/post-review.sh[188-196]
scripts/post-review.sh[206-214]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`REVIEW_PROTECTED_PATHS` is split and whitespace-trimmed, but empty entries are not removed. Any empty entry (from consecutive commas, leading comma, or whitespace-only segment) makes the protected-path match treat *all* files as protected.

## Issue Context
This is configuration-dependent and fail-closed (it downgrades approvals), but it can unexpectedly disable automated approvals across the repo.

## Fix Focus Areas
- scripts/post-review.sh[188-196]
- scripts/post-review.sh[206-214]

## Suggested fix
- After trimming, rebuild `PROTECTED_PATHS` with only non-empty entries.
- If the resulting list is empty, fail closed in a predictable way (e.g., log an error and fall back to `DEFAULT_PROTECTED_PATHS`, or exit non-zero), to avoid accidentally disabling protection.
- Add a regression test for malformed inputs like `,deploy/` and `deploy/, ,manifests/`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context used

✅ Compliance rules (platform): 55 rules

✅ Skills: 4 invoked

  code-review

  code-implementation

  pr-review

  docs-review

Tip of the day

💡 Did you know, you can reply 'qodo' on any finding to push back, ask questions, or dig deeper

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 5:08 PM UTC · Ended 5:13 PM UTC
Commit: df953d2 · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 5:15 PM UTC · Ended 5:23 PM UTC
Commit: d9888a5 · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 5:24 PM UTC · Ended 5:47 PM UTC
Commit: 9278aab · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:48 PM UTC · Completed 6:08 PM UTC
Commit: bcf097f · View workflow run →

[fullsend-ai-review]

Review

Findings

Medium

• [protected-path] harness/review.yaml, scripts/post-review.sh, scripts/post-review-test.sh, skills/pr-review/SKILL.md, skills/pr-review/sub-agents/security-triage.md, agents/code.md, agents/fix.md — PR modifies files under protected paths: agents/, harness/, scripts/, skills/. PR links to issue Review agent: make protected paths configurable via environment variable #568 and provides context for the changes. Human approval is always required for protected-path changes regardless of context.

• [runtime-mechanism] skills/pr-review/sub-agents/security-triage.md:40 — 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. The SKILL.md (step 3c-1) describes the injection procedure, but the security-triage sub-agent has no fallback or detection mechanism if the section is missing or malformed. If the orchestrator omits this section (prompt assembly bug), the sub-agent silently loses all governance-path classification capability. The actual enforcement in post-review.sh is unaffected (it reads the env var directly), so this is a defense-in-depth gap, not a safety-critical hole.
  Remediation: Add a fallback in security-triage.md: if the "Active governance paths" section is absent, classify all infrastructure-looking paths as security-critical.

Low

• [stale-hardcoded-list] agents/fix.md:85 — 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 to REVIEW_PROTECTED_PATHS in harness/review.yaml will silently diverge from this hardcoded list. The fix agent uses this list as a "do not modify" guardrail, so a divergence means the fix agent might modify files that post-review.sh considers protected (or vice versa).

• [fail-open-risk] harness/review.yaml:51 — 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. The distinction between unset (fail-closed abort), empty string (deliberate opt-out), and comma-noise (fail-closed abort) is well-documented and tested.

• [scope-exceeded] eval/scripts/run-fullsend.sh:210 — Adds REVIEW_FINDING_SEVERITY_THRESHOLD passthrough, a separate concern from the protected-path configurability requested in issue Review agent: make protected paths configurable via environment variable #568. Practically necessary for the new eval case to work.

Previous run

Review

Findings

Medium

• [protected-path] harness/review.yaml, scripts/post-review.sh, scripts/post-review-test.sh, skills/pr-review/SKILL.md, skills/pr-review/sub-agents/security-triage.md — PR modifies files under protected paths: harness/, scripts/, skills/. PR links to issue Review agent: make protected paths configurable via environment variable #568 and provides context for the changes. Human approval is always required for protected-path changes regardless of context.

• [edge-case] harness/review.yaml:57 — 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. The PR body acknowledges "Scope increase" for this change, but it is a behavioral change: any PR touching env/ files will now trigger a protected-path downgrade where it previously would not. See also: [unauthorized-change] finding at this location.
  Remediation: If adding env/ is intentional (to protect configuration env files like env/review.env), document the rationale explicitly. If unintentional, remove env/ from the default value in harness/review.yaml.

• [unauthorized-change] harness/review.yaml — Issue Review agent: make protected paths configurable via environment variable #568 requests making protected paths configurable but does not authorize adding env/ as a new entry to the default list. The PR body acknowledges this as a "Scope increase." See also: [edge-case] finding at this location.
  Remediation: Remove env/ from the default list or obtain explicit authorization for adding it.

• [stale-reference] agents/code.md:76 — States "Protected paths are defined in post-review.sh." After this PR, the default protected-path list is defined in harness/review.yaml, with post-review.sh serving as the enforcement point.
  Remediation: Update to: "Protected paths are configured in harness/review.yaml and enforced by post-review.sh."

• [runtime-mechanism] skills/pr-review/sub-agents/security-triage.md:40 — 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 or error detection.
  Remediation: Add a defensive instruction to security-triage.md with a minimal fallback list (e.g., .claude/, .github/, agents/, scripts/, harness/, skills/) to use when no Active governance paths section is present.

Low

• [GHA-workflow-command-injection] scripts/post-review.sh:210 — The ::error:: workflow command at the degenerate-paths abort sanitizes the interpolated REVIEW_PROTECTED_PATHS value using :: → : collapse, which is not idempotent (::: → ::). The same file's REVIEW_FINDING_SEVERITY_THRESHOLD sanitization (lines ~108–111) uses the more robust per-character stripping pattern (//%/ and //:/). Exploitability is limited (requires CI config write access).
  Remediation: Use per-character stripping matching the existing pattern in the same file.

• [fail-open-risk] harness/review.yaml:49 — 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.

• [stale-hardcoded-list] agents/fix.md:85 — Contains a hardcoded protected paths list (lines 85–102) that will diverge from the canonical list in harness/review.yaml because the harness list now includes env/. Impact is limited to LLM guidance accuracy.
  Remediation: Update agents/fix.md lines 85–102 to add the missing env/ entry.

• [stale-reference] agents/fix.md:105 — States "Protected-path enforcement lives in post-review.sh" without noting that the path list configuration source has changed. The statement remains true but is incomplete.
  Remediation: Clarify that paths are now configured via the REVIEW_PROTECTED_PATHS env var (defaults in harness/review.yaml).

• [scope-exceeded] eval/scripts/run-fullsend.sh — Adds REVIEW_FINDING_SEVERITY_THRESHOLD passthrough, a separate concern from the protected-path configurability requested in issue Review agent: make protected paths configurable via environment variable #568. Minor supporting change for the new eval case.

• [design-direction] scripts/post-review.sh — The implementation introduces an explicit "disable all protection" mode (empty string) that did not exist in the hardcoded implementation. The behavior is documented in the PR's docs/review.md update and tested.

• [variable-naming-consistency] scripts/post-review.sh — New temporary variables use leading underscores (_trimmed, _entry, _sanitized_paths) while existing codebase convention uses lowercase without leading underscores (stale_label, file, threshold_rank).

Previous run (2)

Review

Findings

Medium

• [protected-path] harness/review.yaml, scripts/post-review.sh, scripts/post-review-test.sh, skills/pr-review/SKILL.md, skills/pr-review/sub-agents/security-triage.md — PR modifies files under protected paths: harness/, scripts/, skills/. PR links to issue Review agent: make protected paths configurable via environment variable #568 and provides context for the changes. Human approval is always required for protected-path changes regardless of context.

• [edge-case] harness/review.yaml:57 — 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. The PR body acknowledges "Scope increase" for this change, but it is a behavioral change: any PR touching env/ files will now trigger a protected-path downgrade where it previously would not.
  Remediation: If adding env/ is intentional (to protect configuration env files like env/review.env), document it explicitly in the PR description. If unintentional, remove env/ from the default value in harness/review.yaml.

Low

• [GHA-workflow-command-injection] scripts/post-review.sh:210 — The ::error:: workflow command at the degenerate-paths abort sanitizes the interpolated REVIEW_PROTECTED_PATHS value for literal newlines, carriage returns, and :: sequences, but does not strip %0A/%0D URL-encoded newlines, which GitHub Actions decodes in workflow command parameters. Exploitability is limited (requires CI config write access to inject a crafted value).
  Remediation: Add %0A and %0D stripping (case-insensitive variants %0a/%0d too) to the sanitization block.

• [fail-open-risk] harness/review.yaml:56 — 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. The risk is low because the value is a literal in harness/review.yaml, not user-supplied input — accidental clearing requires an intentional harness composition override.

• [stale-reference] agents/code.md:76 — States "Protected paths are defined in post-review.sh." After this PR, the default protected-path list is defined in harness/review.yaml, with post-review.sh serving as the enforcement point. The reference is partially stale.
  Remediation: Update to: "Protected paths are configured in harness/review.yaml and enforced by post-review.sh."

• [stale-hardcoded-list] agents/fix.md:85 — agents/fix.md contains a hardcoded protected paths list (lines 85–102) that will diverge from the canonical list in harness/review.yaml because the harness list now includes env/. Impact is limited to LLM guidance accuracy.
  Remediation: Update agents/fix.md lines 85–102 to add the missing env/ entry, matching the canonical list in harness/review.yaml.

Previous run (3)

Review

Findings

Medium

• [protected-path] harness/review.yaml, scripts/post-review.sh, scripts/post-review-test.sh, skills/pr-review/SKILL.md, skills/pr-review/sub-agents/security-triage.md — PR modifies files under protected paths: harness/, scripts/, skills/. PR links to issue Review agent: make protected paths configurable via environment variable #568 and provides context for the changes. Human approval is always required for protected-path changes regardless of context.

Low

• [GHA-workflow-command-injection] scripts/post-review.sh:210 — The ::error:: workflow command at the degenerate-paths abort sanitizes the interpolated REVIEW_PROTECTED_PATHS value for literal newlines, carriage returns, and :: sequences, but does not strip %0A/%0D URL-encoded newlines, which GitHub Actions decodes in workflow command parameters. Exploitability is limited (requires CI config write access to inject a crafted value).
  Remediation: Add %0A and %0D stripping (case-insensitive variants %0a/%0d too) to the sanitization block.

• [stale-reference] agents/code.md:76 — States "Protected paths are defined in post-review.sh." After this PR, the default protected-path list is defined as a literal value in harness/review.yaml, with post-review.sh serving as the enforcement point. The statement is misleading.
  Remediation: Update to: "Protected paths are configured in harness/review.yaml and enforced by post-review.sh."

• [stale-hardcoded-list] agents/fix.md:85 — agents/fix.md contains a hardcoded protected paths list (lines 85–102) that is now stale. The canonical list moved to harness/review.yaml. The harness list includes env/ which is absent from agents/fix.md. Impact is limited to LLM guidance accuracy.
  Remediation: Update agents/fix.md lines 85–102 to add the missing env/ entry, matching the canonical list in harness/review.yaml.

• [architectural-conflict] harness/review.yaml:173 — REVIEW_PROTECTED_PATHS uses a literal default baked into harness YAML, while REVIEW_FINDING_SEVERITY_THRESHOLD uses a ${VAR} passthrough with a script-side default in post-review.sh. The inconsistency is intentional (the paths list must always be set; the threshold has a trivial scalar fallback), but the two configuration patterns are undocumented.
  Remediation: Add a comment in harness/review.yaml or docs/review.md explaining why the two variables use different default-provision patterns.

• [scope-creep] env/default-review-protected-paths.txt — The PR deletes env/default-review-protected-paths.txt and replaces it with a literal baked into harness/review.yaml. Issue Review agent: make protected paths configurable via environment variable #568 requested making protected paths configurable, not changing the default-provision mechanism. The PR body acknowledges this as "Scope increase."

• [documentation-consistency] docs/review.md — The new REVIEW_PROTECTED_PATHS row in the Variables table has a significantly longer description than the existing REVIEW_FINDING_SEVERITY_THRESHOLD row, breaking visual consistency.
  Remediation: Move detailed parsing behavior to a separate paragraph below the table.

Previous run (4)

Review

Findings

Medium

• [protected-path] harness/review.yaml, scripts/post-review.sh, scripts/post-review-test.sh, skills/pr-review/SKILL.md, skills/pr-review/sub-agents/security-triage.md, env/default-review-protected-paths.txt — PR modifies files under protected paths: env/, harness/, scripts/, skills/. PR links to issue Review agent: make protected paths configurable via environment variable #568 and provides context for the changes. Human approval is always required for protected-path changes regardless of context.

Low

• [fail-open] eval/scripts/run-fullsend.sh:226 — When the default protected paths file exists but contains only comments or blank lines, _defaults remains empty and emit_env "REVIEW_PROTECTED_PATHS" "" is called. post-review.sh interprets empty-but-set REVIEW_PROTECTED_PATHS as a deliberate opt-out, silently disabling protected-path enforcement in the eval context.
  Remediation: After the while loop, add a check: if [[ -z "${_defaults}" ]]; then echo "ERROR: default protected paths file yielded no entries" >&2; exit 1; fi

• [GHA-workflow-command-injection] scripts/post-review.sh:198 — The ::error:: workflow command sanitizes the REVIEW_PROTECTED_PATHS value for newlines/CR and :: sequences, but does not strip %0A/%0D URL-encoded newlines, which GitHub Actions decodes in workflow command parameters. Exploitability is limited (requires CI config write access).
  Remediation: Add %0A and %0D stripping to the sanitization block.

• [stale-hardcoded-list] agents/fix.md:85 — agents/fix.md contains a hardcoded protected paths list (lines 85–102) that is now stale. This PR moves the canonical list to env/default-review-protected-paths.txt. The new default list includes env/ which is absent from agents/fix.md. The list is advisory (enforcement lives in post-review.sh), so impact is limited to LLM guidance accuracy.
  Remediation: Update agents/fix.md to reference env/default-review-protected-paths.txt or update the list to match the new defaults.

• [stale-reference] agents/code.md:76 — States "Protected paths are defined in post-review.sh." After this PR, protected paths are defined in env/default-review-protected-paths.txt (or via REVIEW_PROTECTED_PATHS env var), with post-review.sh as the enforcement point.
  Remediation: Update the reference to clarify the source of the protected paths definition.

Previous run (5)

Review

Findings

Medium

• [fail-open] eval/scripts/run-fullsend.sh:229 — When the eval runner's default protected paths file is missing, it falls back to emit_env "REVIEW_PROTECTED_PATHS" "", which post-review.sh interprets as a deliberate opt-out, silently disabling protected-path enforcement. This converts a missing-config condition into permissive behavior rather than failing closed (as post-review.sh does at its own missing-file guard, lines 214–216).
  Remediation: When the defaults file is missing in run-fullsend.sh, either fail the eval run (exit 1) or leave REVIEW_PROTECTED_PATHS unset so post-review.sh's own missing-file guard can fire.

• [stale-hardcoded-list] agents/fix.md:85 — agents/fix.md contains a hardcoded protected paths list (lines 85–102) that duplicates the old list formerly in post-review.sh. This PR moves the canonical list to env/default-review-protected-paths.txt, making the fix.md copy stale. The new default list includes env/ which is absent from agents/fix.md.
  Remediation: Replace the hardcoded list in agents/fix.md with a reference to the runtime-resolved protected paths list, or update the list to match the new defaults file including env/.

• [protected-path] harness/review.yaml, scripts/post-review.sh, scripts/post-review-test.sh, skills/pr-review/SKILL.md, skills/pr-review/sub-agents/security-triage.md, env/default-review-protected-paths.txt — PR modifies files under protected paths: env/, harness/, scripts/, skills/. PR links to issue Review agent: make protected paths configurable via environment variable #568 and provides context for the changes. Human approval is always required for protected-path changes regardless of context.

Low

• [fail-open] scripts/post-review.sh:173 — The three-way resolution distinguishes between REVIEW_PROTECTED_PATHS being "set and empty" (disables enforcement) vs "unset" (reads defaults file). In harness/review.yaml, the variable is wired through as "${REVIEW_PROTECTED_PATHS}". If the fullsend templating engine resolves an unset outer variable to an empty string, the runner receives an empty value, triggering the disable path instead of the file-fallback path. The distinction between "unset" and "empty string" is fragile across template engines.

• [GHA-workflow-command-injection] scripts/post-review.sh:198 — The ::error:: workflow command sanitizes the REVIEW_PROTECTED_PATHS value for newlines/CR and :: sequences, but does not strip %0A/%0D URL-encoded newlines, which GitHub Actions decodes in workflow command parameters. Exploitability is limited (requires CI config write access).
  Remediation: Add %0A and %0D stripping to the sanitization block.

• [edge-case] scripts/post-review-test.sh:971 — The run_protected_paths_test helper exports REVIEW_PROTECTED_PATHS only when the protected_paths argument is non-empty. When it's empty (file-fallback tests), the subshell does not unset the variable. If the test runs in an environment where REVIEW_PROTECTED_PATHS is already set (e.g., CI), file-fallback tests silently exercise the wrong code path.
  Remediation: Add unset REVIEW_PROTECTED_PATHS in the else branch of the subshell.

• [stale-reference] agents/code.md:76 — States "Protected paths are defined in post-review.sh." After this PR, protected paths are defined in env/default-review-protected-paths.txt (or via REVIEW_PROTECTED_PATHS env var), with post-review.sh as the enforcement point.

---

Labels: PR modifies review agent infrastructure (post-review.sh, harness config, skill definitions, eval runner)

---

Next steps:

• /fs-fix — agent addresses review findings automatically
• /fs-fix <your instruction> — agent fixes with your specific guidance
• Push commits directly — review re-runs automatically on push
• /fs-fix-stop — disable automatic fix runs for this PR

Previous run (6)

Review

Findings

Medium

• [runtime-mechanism] skills/pr-review/SKILL.md — SKILL.md instructs the sandbox agent to fall back to reading env/default-review-protected-paths.txt when REVIEW_PROTECTED_PATHS is not set or empty. This file is not mounted into the sandbox via host_files in harness/review.yaml, so the fallback cannot function inside the sandbox. While this path is currently unreachable in production (run-fullsend.sh always populates the env var, and review.yaml passes it via env.sandbox), the instruction describes a fallback that cannot work. Additionally, SKILL.md says "not set or empty" falls through to file reading, but post-review.sh (runner side) treats set-but-empty as disabling enforcement entirely — inconsistent semantics for the empty case.
  Remediation: Either mount env/default-review-protected-paths.txt into the sandbox via host_files, or remove the file-fallback instruction from SKILL.md and note that REVIEW_PROTECTED_PATHS is always provided by the harness. Align empty-string semantics between SKILL.md and post-review.sh.

• [fail-open] scripts/post-review.sh — Setting REVIEW_PROTECTED_PATHS="" (explicitly empty) disables protected-path enforcement entirely. The harness YAML passes REVIEW_PROTECTED_PATHS: "${REVIEW_PROTECTED_PATHS}" — if the outer variable is unset, the fullsend templating engine may resolve this to an empty string, silently triggering the disable path. Only a ::notice:: annotation signals when protection is disabled.
  Remediation: Verify how the fullsend harness resolves ${REVIEW_PROTECTED_PATHS} when the outer variable is unset. Consider using a sentinel value (e.g., REVIEW_PROTECTED_PATHS=NONE) for the explicit-disable path instead of overloading empty string.

• [stale-hardcoded-list] agents/fix.md — agents/fix.md contains a hardcoded protected paths list (lines ~85–102) that duplicates the old list formerly in post-review.sh. This PR moves the canonical list to env/default-review-protected-paths.txt, making the fix.md copy stale. The new default list includes env/ which is absent from agents/fix.md.
  Remediation: Replace the hardcoded list in agents/fix.md with a reference to the runtime-resolved protected paths list, or update the list to match the new defaults file including env/.

• [protected-path] harness/review.yaml, scripts/post-review.sh, scripts/post-review-test.sh, skills/pr-review/SKILL.md, skills/pr-review/sub-agents/security-triage.md — PR modifies files under protected paths: harness/, scripts/, skills/. These are governance and infrastructure files that require human approval. PR links to issue Review agent: make protected paths configurable via environment variable #568 and provides context for the changes. Human approval is always required for protected-path changes regardless of context.

Low

• [GHA-workflow-command-injection] scripts/post-review.sh — The ::error:: workflow command interpolates the raw REVIEW_PROTECTED_PATHS value unsanitized. While exploitability is limited (requires repo write access to CI configuration), applying the same sanitization pattern used for label actions (lines ~279–284) would be good defense-in-depth.

• [edge-case] eval/scripts/run-fullsend.sh — When REVIEW_PROTECTED_PATHS is not set and the defaults file is missing, the eval runner emits REVIEW_PROTECTED_PATHS="". On the PR-head post-review.sh, a set-but-empty variable disables protected-path enforcement — the opposite of fail-closed intent in eval context.

• [defense-in-depth] harness/review.yaml — REVIEW_PROTECTED_PATHS is passed into the sandbox environment. This is now intentional and documented (SKILL.md step 3c-1 and step 6e reference the variable for governance-path resolution). The enforcement point (post-review.sh) runs independently on the runner and cannot be influenced by the sandbox agent.

• [scope-creep-minor] env/default-review-protected-paths.txt — The defaults file adds env/ as a new protected path not present in the original hardcoded list. Defensible as self-protection of the new configuration mechanism.

• [stale-reference] agents/code.md — States "Protected paths are defined in post-review.sh." After this PR, protected paths are defined in env/default-review-protected-paths.txt (or via REVIEW_PROTECTED_PATHS env var), with post-review.sh as the enforcement point.

• [stale-reference] agents/fix.md — States "Protected-path enforcement lives in post-review.sh" without noting that the path list is now sourced externally from the env var or defaults file.

• [consistency] harness/review.yaml — The env: block lists sandbox: before runner:, but the convention in other harness files (triage.yaml, fix.yaml, prioritize.yaml) is runner: first.

Previous run (7)

Review

Findings

Medium

• [runtime-mechanism] skills/pr-review/SKILL.md — SKILL.md instructs the sandbox agent to fall back to reading env/default-review-protected-paths.txt when REVIEW_PROTECTED_PATHS is "not set or empty". This file is not mounted into the sandbox via host_files in harness/review.yaml. While this path is currently unreachable in production (run-fullsend.sh always populates the env var, and review.yaml passes it via env.sandbox), the instruction describes a fallback that cannot function inside the sandbox. Additionally, SKILL.md says "not set or empty" falls through to file reading, but post-review.sh (runner side) treats empty as a hard abort — the two components have inconsistent semantics for the same condition.
  Remediation: Either mount env/default-review-protected-paths.txt into the sandbox via host_files (making the fallback functional), or update SKILL.md to note that the env var is always pre-populated by the harness and the file-read fallback is not available in the sandbox.

• [protected-path] harness/review.yaml, scripts/post-review.sh, scripts/post-review-test.sh, skills/pr-review/SKILL.md, skills/pr-review/sub-agents/security-triage.md — PR modifies files under protected paths: harness/, scripts/, skills/. These are governance and infrastructure files that require human approval. PR links to issue Review agent: make protected paths configurable via environment variable #568 and provides context for the changes. Human approval is always required for protected-path changes regardless of context.

Low

• [edge-case] eval/scripts/run-fullsend.sh:434 — When REVIEW_PROTECTED_PATHS is not set and the defaults file is missing, the script emits an empty string. post-review.sh then aborts with "set but empty after parsing" — fail-closed but the error message obscures the real cause (missing defaults file).

• [logic-error] scripts/post-review.sh — The guard if [[ ${#PROTECTED_PATHS[@]} -gt 0 ]] is always true at this point. All preceding branches either populate a non-empty array or abort with exit 1. Dead code that adds unnecessary nesting.

• [defense-in-depth] harness/review.yaml — REVIEW_PROTECTED_PATHS is passed into the sandbox environment. This is now intentional and documented (SKILL.md step 3c-1 and step 6e reference the variable for governance-path resolution). The enforcement point (post-review.sh) runs independently on the runner and cannot be influenced by the sandbox agent.

• [scope-creep-minor] env/default-review-protected-paths.txt — The defaults file adds env/ as a new protected path not present in the original hardcoded list. Defensible as self-protection of the new configuration mechanism.

Previous run (8)

Review

Findings

Medium

• [logic-error] docs/review.md:82 — The documentation claims "When unset or empty, defaults are read from env/default-review-protected-paths.txt. Setting to an empty string is treated the same as unset (fail-closed)." This is incorrect. In post-review.sh, when REVIEW_PROTECTED_PATHS is set to an empty string, the code enters the ${REVIEW_PROTECTED_PATHS+set} branch, parses zero entries, and aborts with exit 1. It does NOT fall through to read the defaults file. "Unset" (reads from file) and "empty string" (aborts) behave differently, contradicting the documentation.
  Remediation: Change the documentation to: "When unset, defaults are read from env/default-review-protected-paths.txt. When set to an empty string (or a value that parses to no valid paths), the script aborts (fail-closed)."

• [runtime-mechanism] skills/pr-review/SKILL.md — SKILL.md instructs the agent in two places (step 3c-1 step 2 and step 6e Protected paths) to fall back to reading env/default-review-protected-paths.txt when REVIEW_PROTECTED_PATHS is not set. This file exists in the agents repo but is not mounted into the sandbox via host_files in harness/review.yaml. The sandbox contains the target repository, not the agents repo. Currently masked because run-fullsend.sh always populates the env var, but the fallback path described in SKILL.md is non-functional in the sandbox.
  Remediation: Either mount the file via host_files in review.yaml, or remove the file-fallback instruction from SKILL.md (the env var will always be provided by the harness).

• [protected-path] harness/review.yaml, scripts/post-review.sh, scripts/post-review-test.sh, skills/pr-review/SKILL.md, skills/pr-review/sub-agents/security-triage.md, env/default-review-protected-paths.txt — PR modifies files under protected paths: harness/, scripts/, skills/, env/. These are governance and infrastructure files that require human approval. PR links to issue Review agent: make protected paths configurable via environment variable #568 and provides context for the changes. Human approval is always required for protected-path changes regardless of context.

Low

• [defense-in-depth] harness/review.yaml — REVIEW_PROTECTED_PATHS is passed into the sandbox environment. This is now intentional and documented (SKILL.md step 3c-1 and step 6e reference the variable for governance-path resolution). The enforcement point (post-review.sh) runs independently on the runner and cannot be influenced by the sandbox agent.

• [edge-case] eval/scripts/run-fullsend.sh:456 — When REVIEW_PROTECTED_PATHS is not set and the defaults file is missing, the script emits an empty string. post-review.sh then aborts with "set but empty after parsing" — fail-closed but the error message obscures the real cause (missing defaults file).

• [logic-error] scripts/post-review.sh:792 — The guard if [[ ${#PROTECTED_PATHS[@]} -gt 0 ]] is always true at this point. All preceding branches either populate a non-empty array or abort with exit 1. Dead code that adds unnecessary nesting.

• [scope-creep-minor] env/default-review-protected-paths.txt — The defaults file adds env/ as a new protected path not present in the original hardcoded list. Defensible as self-protection of the new configuration mechanism.

• [external-dependency] env/default-review-protected-paths.txt — New external configuration file creates a deployment dependency. Downstream repos that fork the review harness must include this file or explicitly set REVIEW_PROTECTED_PATHS. post-review.sh aborts (fail-closed) if neither exists.

Previous run (9)

Review

Findings

High

• [fail-open] scripts/post-review.sh:173 — Setting REVIEW_PROTECTED_PATHS to an empty string explicitly disables all protected-path checking (PROTECTED_PATHS=()), allowing the review agent to approve PRs that touch governance files without downgrading to comment. This removes the "sole enforcement point" for protected-path governance. While this is a deliberate design choice (the code comment says "Explicitly empty — protection disabled"), empty-string assignment can occur through CI misconfiguration (e.g., REVIEW_PROTECTED_PATHS= with no value), and the disable happens silently with no log output. Issue Review agent: make protected paths configurable via environment variable #568 did not authorize a "disable all protection" mode — it requested "override or extend."
  Remediation: Remove the empty-string disable code path. If disabling protection is a legitimate need, require a distinct opt-out mechanism (e.g., REVIEW_PROTECTED_PATHS_DISABLE=true) that cannot be triggered by accidental empty-string assignment, and log when protection is disabled.

Medium

• [runtime-mechanism] skills/pr-review/SKILL.md — SKILL.md instructs the LLM agent to check if REVIEW_PROTECTED_PATHS "is set" and use it for governance path resolution. However, eval/scripts/run-fullsend.sh unconditionally emits REVIEW_PROTECTED_PATHS as an empty string when unset (${REVIEW_PROTECTED_PATHS:-}), and review.yaml passes ${REVIEW_PROTECTED_PATHS} to the sandbox. The env var is always defined (as an empty string) in the eval environment. An LLM following the instruction may interpret an empty-but-defined env var as "set", split the empty string, and proceed with zero governance paths — affecting both the security-triage spawn prompt (Part 2 will have an empty governance paths list) and the step 6e protected-paths check. The post-review.sh enforcement correctly distinguishes set-but-empty from unset, so this is a defense-in-depth gap rather than an enforcement bypass.
  Remediation: Change SKILL.md to say "if set and non-empty" to match the semantics in post-review.sh, or have run-fullsend.sh only emit REVIEW_PROTECTED_PATHS when the caller explicitly provides a value.

• [runtime-mechanism] skills/pr-review/SKILL.md — Both the triage procedure (step 2) and the protected-paths section (step 6e) instruct the agent to read env/default-review-protected-paths.txt as fallback when the env var is not set. This file exists in the agents repo but is not mounted into the sandbox via host_files in harness/review.yaml. The sandbox contains the target repository being reviewed, not the agents repo, so the file will not exist at the expected path. Currently masked by the always-set env var, but represents a latent bug that would surface if the empty-string issue is resolved.
  Remediation: Either mount env/default-review-protected-paths.txt into the sandbox via host_files in harness/review.yaml, or have the pre-script inject the file contents into the env var when unset.

• [fail-open] scripts/post-review.sh — When the defaults file (env/default-review-protected-paths.txt) contains only blank lines and comments (all lines filtered out), the resulting PROTECTED_PATHS array is empty. The code reaches if [[ ${#PROTECTED_PATHS[@]} -gt 0 ]] and skips the entire protected-path check — silently disabling protection. This is inconsistent with the fail-closed behavior for the env var path, which explicitly aborts when parsing yields zero entries.
  Remediation: After reading the defaults file, add the same zero-length check: if [[ ${#PROTECTED_PATHS[@]} -eq 0 ]]; then echo "::error::..." >&2; exit 1; fi.

• [fail-open] env/default-review-protected-paths.txt — The default protected paths list does not include env/ itself. A PR modifying env/default-review-protected-paths.txt (e.g., removing entries to weaken protection) would not be flagged as touching a protected path on the next run. Note: this is a pre-existing gap — the old hardcoded list also did not include env/.
  Remediation: Add env/ to the default protected paths list.

• [privilege-escalation] harness/review.yaml — REVIEW_PROTECTED_PATHS is passed into the sandbox environment where the review agent runs. While the agent needs to know protected paths for its own findings (SKILL.md step 6e), exposing the mutable configuration to the sandbox is a defense-in-depth concern. The base-branch SKILL.md already hardcodes the list, so no new information asymmetry is created, but making the list operator-configurable inside the sandbox widens the surface.
  Remediation: Consider whether the sandbox truly needs this variable. If the agent's protected-path findings are defense-in-depth (with post-review.sh as the sole enforcement point), the SKILL.md could embed defaults and the env var could be limited to env.runner.

• [protected-path] harness/review.yaml, scripts/post-review.sh, scripts/post-review-test.sh, skills/pr-review/SKILL.md, skills/pr-review/sub-agents/security-triage.md — PR modifies files under protected paths: harness/, scripts/, skills/. These are governance and infrastructure files that require human approval. PR links to issue Review agent: make protected paths configurable via environment variable #568 and provides context for the changes. Human approval is always required for protected-path changes regardless of context.

Low

• [edge-case] harness/review.yaml — review.yaml now references ${REVIEW_PROTECTED_PATHS} and ${REVIEW_FINDING_SEVERITY_THRESHOLD} in both sandbox and runner env sections. The production CI workflow (in fullsend-ai/fullsend) must emit these env vars before merging this PR, or fullsend may fail to resolve the variable references at startup.

• [logic-error] skills/pr-review/sub-agents/security-triage.md:40 — The hardcoded governance path list (16 concrete paths) was removed from the security-triage sub-agent definition and replaced with a reference to the orchestrator-provided "Active governance paths" list. The sub-agent now has zero governance path patterns of its own, making it entirely dependent on the orchestrator correctly resolving and injecting the paths via the spawn prompt (SKILL.md step 3c-1). If the orchestrator fails to inject them (e.g., due to the env var ambiguity above), the sub-agent has no fallback.

• [scope-authorization-interpretation] scripts/post-review.sh — Issue Review agent: make protected paths configurable via environment variable #568 contains contradictory guidance: the body says "override or extend" (suggesting append capability), but the recommended approach says "replaces the default list entirely" (override-only). The implementation follows the recommended approach.

• [breaking-schema] harness/review.yaml:48 — The runner_env key under forge.github was changed to env with sandbox and runner sub-keys. Three other harness files on main already use the new env format (fix.yaml, triage.yaml, prioritize.yaml), confirming the fullsend CLI parser supports both schemas.

---

Next steps:

• /fs-fix — agent addresses review findings automatically
• /fs-fix <your instruction> — agent fixes with your specific guidance
• Push commits directly — review re-runs automatically on push
• /fs-fix-stop — disable automatic fix runs for this PR

Previous run (10)

Review

Findings

Medium

• [runtime-mechanism] skills/pr-review/SKILL.md — SKILL.md instructs the LLM agent to check if REVIEW_PROTECTED_PATHS "is set" and use it for governance path resolution. However, run-fullsend.sh unconditionally emits REVIEW_PROTECTED_PATHS as an empty string when unset (${REVIEW_PROTECTED_PATHS:-}), and review.yaml passes ${REVIEW_PROTECTED_PATHS} to the sandbox. The env var is always defined (as an empty string) in the sandbox. An LLM following the natural-language instruction may interpret an empty-but-defined env var as "set", split the empty string, and proceed with zero governance paths — affecting both the security-triage spawn prompt (Part 2 will have an empty governance paths list) and the step 6e protected-paths check. The post-review.sh enforcement (the "sole enforcement point" per the code comment) handles empty strings correctly with its -n test and fail-closed guard, so this is a defense-in-depth gap rather than an enforcement bypass.
  Remediation: Change SKILL.md to say "if set and non-empty" to match the -n semantics in post-review.sh, or have run-fullsend.sh populate REVIEW_PROTECTED_PATHS with the contents of env/default-review-protected-paths.txt when the caller does not provide a value.

• [runtime-mechanism] skills/pr-review/SKILL.md — Both the triage procedure (new step 2) and the protected-paths section (step 6e) instruct the agent to read env/default-review-protected-paths.txt as fallback when the env var is not set. This file is added to the repo by this PR but is not mounted into the sandbox via host_files in harness/review.yaml. The sandbox contains the target repository, not the agents repo, so the file will not exist at the expected path. Currently masked by the always-set env var (finding above), but represents a latent bug that would surface if the empty-string issue is resolved by not emitting the variable.
  Remediation: Either mount env/default-review-protected-paths.txt into the sandbox via host_files in harness/review.yaml, or ensure the default paths are always passed via the env var so the file-reading fallback is never needed.

• [protected-path] harness/review.yaml — PR modifies files under protected paths: harness/review.yaml, scripts/post-review.sh, scripts/post-review-test.sh, skills/pr-review/SKILL.md, skills/pr-review/sub-agents/security-triage.md. These are governance and infrastructure files that require human approval. PR links to issue Review agent: make protected paths configurable via environment variable #568 and provides context for the changes. Human approval is always required for protected-path changes regardless of context.

Low

• [breaking-schema] harness/review.yaml:48 — The runner_env key under forge.github was changed to env with sandbox and runner sub-keys. Three other harness files on main already use the new env format (fix.yaml, triage.yaml, prioritize.yaml), confirming the fullsend CLI parser supports both schemas. This aligns review.yaml with the newer convention rather than introducing a breaking change.

• [scope-authorization-interpretation] scripts/post-review.sh — Issue Review agent: make protected paths configurable via environment variable #568 contains contradictory guidance: the body says "override or extend" (suggesting append capability), but the recommended approach says "replaces the default list entirely" (override-only). The implementation follows the recommended approach. If repository owners expect append semantics based on the issue body's phrasing, they may be surprised.

• [variable-naming] scripts/post-review.sh:170 — PROTECTED_PATHS uses a generic name without namespace qualifier. The codebase convention in this file (REVIEW_CONTROL_LABELS at line 238) is to prefix global variables with REVIEW_. The old REVIEW_PROTECTED_PATHS array followed this pattern; the new internal array does not.

• [edge-case] harness/review.yaml — review.yaml now references ${REVIEW_PROTECTED_PATHS} and ${REVIEW_FINDING_SEVERITY_THRESHOLD} in both sandbox and runner env sections. The production CI workflow (in fullsend-ai/fullsend) must emit these env vars before merging this PR, or fullsend may fail to resolve the variable references at startup.

Previous run (11)

Review

Findings

Medium

• [runtime-mechanism] skills/pr-review/SKILL.md — SKILL.md instructs the LLM agent to check if REVIEW_PROTECTED_PATHS "is set" and use it for governance path resolution. However, run-fullsend.sh unconditionally emits REVIEW_PROTECTED_PATHS as an empty string when unset (${REVIEW_PROTECTED_PATHS:-}), and review.yaml passes ${REVIEW_PROTECTED_PATHS} to the sandbox. The env var is always defined (as an empty string) in the sandbox. An LLM following the natural-language instruction may interpret an empty-but-defined env var as "set", split the empty string, and proceed with zero governance paths — affecting both the security-triage spawn prompt (Part 2 will have an empty governance paths list) and the step 6e protected-paths check. The post-review.sh enforcement (the "sole enforcement point" per the code comment) handles empty strings correctly with its -n test and fail-closed guard, so this is a defense-in-depth gap rather than an enforcement bypass.
  Remediation: Change SKILL.md to say "if set and non-empty" to match the -n semantics in post-review.sh, or have run-fullsend.sh populate REVIEW_PROTECTED_PATHS with the contents of env/default-review-protected-paths.txt when the caller does not provide a value.

• [runtime-mechanism] skills/pr-review/SKILL.md — Both the triage procedure (new step 2) and the protected-paths section (step 6e) instruct the agent to read env/default-review-protected-paths.txt as fallback when the env var is not set. This file is added to the repo by this PR but is not mounted into the sandbox via host_files in harness/review.yaml. The sandbox contains the target repository, not the agents repo, so the file will not exist at the expected path. Currently masked by the always-set env var (finding above), but represents a latent bug that would surface if the empty-string issue is resolved by not emitting the variable.
  Remediation: Either mount env/default-review-protected-paths.txt into the sandbox via host_files in harness/review.yaml, or ensure the default paths are always passed via the env var so the file-reading fallback is never needed.

• [protected-path] harness/review.yaml — PR modifies files under protected paths: harness/review.yaml, scripts/post-review.sh, scripts/post-review-test.sh, skills/pr-review/SKILL.md, skills/pr-review/sub-agents/security-triage.md. These are governance and infrastructure files that require human approval. PR links to issue Review agent: make protected paths configurable via environment variable #568 and provides context for the changes. Human approval is always required for protected-path changes regardless of context.

Low

• [breaking-schema] harness/review.yaml:48 — The runner_env key under forge.github was changed to env with sandbox and runner sub-keys. Three other harness files on main already use the new env format (fix.yaml, triage.yaml, prioritize.yaml), confirming the fullsend CLI parser supports both schemas. Two others (code.yaml, retro.yaml) still use runner_env. This aligns review.yaml with the newer convention rather than introducing a breaking change.

• [scope-authorization-interpretation] scripts/post-review.sh — Issue Review agent: make protected paths configurable via environment variable #568 contains contradictory guidance: the body says "override or extend" (suggesting append capability), but the recommended approach says "replaces the default list entirely" (override-only). The implementation follows the recommended approach. If repository owners expect append semantics based on the issue body's phrasing, they may be surprised.

• [variable-naming] scripts/post-review.sh:170 — PROTECTED_PATHS uses a generic name without namespace qualifier. The codebase convention in this file (REVIEW_CONTROL_LABELS at line 238) is to prefix global variables with REVIEW_. The old REVIEW_PROTECTED_PATHS array followed this pattern; the new internal array does not.

• [edge-case] harness/review.yaml — review.yaml now references ${REVIEW_PROTECTED_PATHS} and ${REVIEW_FINDING_SEVERITY_THRESHOLD} in both sandbox and runner env sections. The production CI workflow (in fullsend-ai/fullsend) must emit these env vars before merging this PR, or fullsend may fail to resolve the variable references at startup.

Previous run (12)

Review

Findings

High

• [runtime-mechanism] skills/pr-review/sub-agents/security-triage.md:40 — The hardcoded governance path list (16 concrete paths like .claude/**, .github/**, scripts/**, etc.) was removed and replaced with a reference to the REVIEW_PROTECTED_PATHS environment variable. The security-triage sub-agent is a Haiku model spawned with a composed prompt — it has no mechanism to read environment variables at runtime. When REVIEW_PROTECTED_PATHS is not set (the default case), the sub-agent receives zero concrete governance paths for classification, only seeing “Any path listed in the REVIEW_PROTECTED_PATHS environment variable is a governance or infrastructure path.” This degrades the triage classifier’s ability to identify governance files as security-critical in large PRs. See also: [security-control-weakening] finding at this location.
  Remediation: Either (a) keep the hardcoded governance path list in security-triage.md as the default classification criteria, or (b) update the orchestrator’s spawn prompt composition (SKILL.md step 3c-1) to resolve the protected paths list and include it in the triage prompt.

• [breaking-schema] harness/review.yaml:48 — The runner_env key under forge.github was renamed to env with sandbox and runner sub-keys. The fullsend harness (in fullsend-ai/fullsend) consumes this YAML. The repo has mixed migration state: code.yaml and retro.yaml still use runner_env, while fix.yaml, triage.yaml, and prioritize.yaml already use the new env structure. If the fullsend CLI requires a minimum version for the new schema, this should be documented.
  Remediation: Verify whether the fullsend harness parser supports both schemas. If only the new schema is supported, update code.yaml and retro.yaml in the same PR for consistency. If this is a breaking change for older CLI versions, mark the commit with ! suffix per conventional commits.

Medium

• [fail-open] scripts/post-review.sh:189 — After parsing REVIEW_PROTECTED_PATHS (env var or file), no check verifies that the resulting PROTECTED_PATHS array is non-empty. If the env var is set to a degenerate value that trims to nothing (e.g., only commas or whitespace), the array is empty and the protected-path enforcement loop becomes a no-op — an approve action for a PR touching sensitive paths would never be downgraded. The else branch correctly aborts when neither source is available, but the env-var and file-reading branches can produce empty arrays without aborting.

• [runtime-mechanism] skills/pr-review/SKILL.md — The updated SKILL.md instructs the review agent: “If the variable is not set, read the default list from env/default-review-protected-paths.txt.” However, the review agent runs in a sandbox against the target repository, not the fullsend-ai/agents repo. The file env/default-review-protected-paths.txt exists only in the agents repo and is inaccessible from the sandbox. This instruction is unimplementable in the default case. Post-review.sh enforcement on the runner handles this correctly, but the agent cannot emit protected-path findings without knowing the path list.

• [security-control-weakening] skills/pr-review/sub-agents/security-triage.md:40 — Same code location as the [runtime-mechanism] finding above, evaluated from the security dimension. Removing the hardcoded governance path list from the triage classifier weakens a security control: governance files (.github/, scripts/, CODEOWNERS, etc.) may no longer be classified as security-critical in large PRs, causing them to receive standard rather than prioritized review attention.

• [protected-path] harness/review.yaml — PR modifies files under protected paths: harness/review.yaml, scripts/post-review.sh, scripts/post-review-test.sh, skills/pr-review/SKILL.md, skills/pr-review/sub-agents/security-triage.md. These are governance and infrastructure files that require human approval. PR links to issue Review agent: make protected paths configurable via environment variable #568 and provides context for the changes. Human approval is always required for protected-path changes regardless of context.

Low

• [edge-case] scripts/post-review.sh — When REVIEW_PROTECTED_PATHS is not set in CI, the harness passes an empty string to the sandbox. The agent may interpret a set-but-empty env var as “no protected paths” and skip emitting protected-path findings. Post-review.sh handles this correctly (empty string is falsy for -n), but the agent-side behavior is ambiguous.

• [scope-authorization-interpretation] scripts/post-review.sh — Issue Review agent: make protected paths configurable via environment variable #568 authorizes “override or extend” semantics, but the implementation provides override-only. When the env var is set, it fully replaces the defaults with no mechanism to append.

• [variable-naming] scripts/post-review.sh — The internal array PROTECTED_PATHS uses a generic name. The codebase convention is namespace-qualified names (e.g., REVIEW_CONTROL_LABELS). Consider REVIEW_ACTIVE_PROTECTED_PATHS or similar.

• [stale-reference] docs/code.md:44 — References runner_env in harness/code.yaml. While code.yaml still uses runner_env, this reference will become stale when the migration completes.

• [configuration-file-location] scripts/post-review.sh — The ../env/ relative path pattern for the default file is a new convention in the codebase. Other scripts use absolute paths from env vars or paths within the same directory.

---

Labels: PR modifies review agent infrastructure (post-review.sh, SKILL.md, security-triage.md, harness config)

---

Next steps:

• /fs-fix — agent addresses review findings automatically
• /fs-fix <your instruction> — agent fixes with your specific guidance
• Push commits directly — review re-runs automatically on push
• /fs-fix-stop — disable automatic fix runs for this PR

[fullsend-ai-coder]

🤖 Finished Fix · ❌ Failure · Started 6:10 PM UTC · Completed 6:19 PM UTC
Commit: bcf097f · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 6:11 PM UTC · Ended 6:21 PM UTC
Commit: edf335f · View workflow run →

[fullsend-ai-coder]

⚠️ Post-fix script failed — Push rejected (exit code 1)

The fix agent completed, but the post-fix script failed before finishing.

Workflow run: https://github.com/fullsend-ai/.fullsend/actions/runs/30569103309

Details:
To https://github.com/fullsend-ai/agents.git
! [rejected] agent/568-configurable-protected-paths -> agent/568-configurable-protected-paths (fetch first)
error: failed to push some refs to 'https://github.com/fullsend-ai/agents.git'
hint: Updates were rejected because the remote contains work that you do not
hint: have locally. This is usually caused by another repository pushing to
hint: the same ref. If you want to integrate the remote changes, use
hint: 'git pull' before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.
To https://github.com/fullsend-ai/agents.git
! [rejected] agent/568-configurable-protected-paths -> agent/568-configurable-protected-paths (stale info)
error: failed to push some refs to 'https://github.com/fullsend-ai/agents.git'
Please check the workflow logs for full details and retry with /fs-fix if appropriate.

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 6:23 PM UTC · Ended 6:32 PM UTC
Commit: f3398a4 · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 6:33 PM UTC · Ended 6:45 PM UTC
Commit: e961179 · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 6:47 PM UTC · Ended 7:01 PM UTC
Commit: 1ceb993 · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 7:03 PM UTC · Ended 7:20 PM UTC
Commit: d6c38e9 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:21 PM UTC · Completed 7:42 PM UTC
Commit: 2c33629 · View workflow run →

Superseded by updated review

[waynesun09]

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.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:09 PM UTC · Completed 9:30 PM UTC
Commit: 90f829f · View workflow run →

[fullsend-ai-coder]

🤖 Finished Fix · ✅ Success · Started 9:31 PM UTC · Completed 9:43 PM UTC
Commit: 90f829f · View workflow run →