refactor: move severity threshold default to harness env

[ralphbean]

Summary

• Moves the REVIEW_FINDING_SEVERITY_THRESHOLD default (low) from agents/review.md and scripts/post-review.sh into harness/review.yaml under env.runner and env.sandbox
• Post-review script now fails on invalid values instead of silently falling back, since the harness guarantees a valid default
• Updates docs and tests to match

Aligns the review agent with the configuration pattern recommended in #566.

Test plan

• bash scripts/post-review-test.sh — all tests pass
• shellcheck clean on both changed shell scripts

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Move severity threshold default to harness env and fail fast on invalid values

⚙️ Configuration changes 📝 Documentation 🧪 Tests 🕐 20-40 Minutes

AI Description

• Set REVIEW_FINDING_SEVERITY_THRESHOLD default via harness env for runner and sandbox.
• Make post-review.sh exit with an error on invalid threshold values.
• Update docs and post-review tests to match the new defaulting behavior.

Diagram

graph TD
  A["CI runner"] --> B[/"harness/review.yaml"/] --> C["Runner/Sandbox env"] --> D["Review agent prompt"] --> E[("agent-result.json")] --> F(["scripts/post-review.sh"]) --> G{{"GitHub PR"}}
  H["docs/review.md"] -. "documents" .-> B
  subgraph Legend
    direction LR
    _cfg[/"Config"/] ~~~ _proc(["Script/Process"]) ~~~ _data[("Data file")] ~~~ _ext{{"External"}}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Keep defaulting logic in post-review.sh

• ➕ No dependency on harness config to guarantee a valid default
• ➕ Single place to understand post-review behavior in isolation
• ➖ Duplicates configuration defaults across harness/docs/scripts
• ➖ Encourages silent fallback behavior that can hide misconfiguration

2. Centralize defaults in a shared env file (e.g., env/review.env) sourced by both contexts

• ➕ Keeps defaults in one env-focused artifact that can be reused elsewhere
• ➕ Easy to override via standard dotenv patterns
• ➖ Requires ensuring both runner and sandbox reliably source the file
• ➖ Can be less explicit than harness YAML about per-context injection

Recommendation: The PR’s approach (setting the default in harness/review.yaml for both runner and sandbox, then failing fast on invalid overrides) is the best fit for the stated configuration pattern: it removes duplicated defaults, makes the runtime contract explicit, and turns misconfiguration into an actionable CI error instead of a silent behavior change.

Files changed (5) +22 / -13

Bug fix (1) +2 / -4

post-review.shFail hard on invalid severity threshold (no silent fallback) +2/-4

Fail hard on invalid severity threshold (no silent fallback)

• Removes the shell-level defaulting to low and changes validation to emit a GitHub Actions error and exit non-zero on invalid values. Assumes the harness provides a valid default and treats invalid overrides as configuration errors.

scripts/post-review.sh

Tests (1) +8 / -0

post-review-test.shEnsure tests set REVIEW_FINDING_SEVERITY_THRESHOLD explicitly +8/-0

Ensure tests set REVIEW_FINDING_SEVERITY_THRESHOLD explicitly

• Exports REVIEW_FINDING_SEVERITY_THRESHOLD="low" in multiple test helpers to satisfy the stricter post-review validation. Keeps test execution consistent with the harness-provided default.

scripts/post-review-test.sh

Documentation (2) +6 / -9

review.mdRemove implied defaulting language for severity threshold +4/-7

Remove implied defaulting language for severity threshold

• Updates the severity filtering section to treat REVIEW_FINDING_SEVERITY_THRESHOLD as an explicit input rather than defaulting to low in documentation. Clarifies that findings below the threshold are suppressed.

agents/review.md

review.mdDocument harness-provided default for REVIEW_FINDING_SEVERITY_THRESHOLD +2/-2

Document harness-provided default for REVIEW_FINDING_SEVERITY_THRESHOLD

• Updates the configuration table to state that the low default is defined in harness/review.yaml (runner and sandbox). Adjusts guidance to emphasize overriding via CI env rather than setting from scratch.

docs/review.md

Other (1) +6 / -0

review.yamlAdd runner and sandbox env defaults for severity threshold +6/-0

Add runner and sandbox env defaults for severity threshold

• Introduces an env section that sets REVIEW_FINDING_SEVERITY_THRESHOLD to "low" for both runner and sandbox contexts. This centralizes the default in the harness configuration.

harness/review.yaml

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:34 PM UTC · Completed 6:48 PM UTC
Commit: 34695be · View workflow run →

[qodo-code-review]

Code Review by Qodo

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

Context used

✅ Compliance rules (platform): 55 rules

✅ Skills: 4 invoked

  code-review

  code-implementation

  pr-review

  docs-review

1. Protected paths modified 📜 Skill insight § Compliance

Description

This PR changes protected governance/infrastructure paths (agents/, harness/, and scripts/),
which must be surfaced for human review and must not be auto-approved. Protected-path modifications
require an explicit compliance finding per policy.

Code

harness/review.yaml[R44-48]

+env:
+  runner:
+    REVIEW_FINDING_SEVERITY_THRESHOLD: "low"
+  sandbox:
+    REVIEW_FINDING_SEVERITY_THRESHOLD: "low"

Relevance

●●● Strong

Compliance policy: protected-path changes must be explicitly surfaced for human review.

PR-#326

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The compliance checklist designates agents/, harness/, and scripts/ as protected paths; this
PR modifies files in those directories (e.g., harness/review.yaml adds an env block). Therefore,
a protected-path finding is required.

harness/review.yaml[44-48]
Skill: pr-review

---

2. Unsanitized ::error interpolation ✓ Resolved 📜 Skill insight ⛨ Security

Description

scripts/post-review.sh emits a GitHub Actions workflow command using the raw
REVIEW_FINDING_SEVERITY_THRESHOLD value, which can allow workflow-command injection via ::,
newlines, or control characters. All interpolated values in workflow commands must be sanitized
individually.

Code

scripts/post-review.sh[R97-98]

+  *) echo "::error::REVIEW_FINDING_SEVERITY_THRESHOLD='${REVIEW_FINDING_SEVERITY_THRESHOLD}' is invalid (expected info|low|medium|high|critical)"
+     exit 1 ;;

Relevance

●●● Strong

Repo has accepted sanitizing ::/workflow-command content to prevent GHA command injection.

PR-#184

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The checklist requires sanitizing every interpolated variable used inside GitHub Actions workflow
commands. The new ::error::... line interpolates REVIEW_FINDING_SEVERITY_THRESHOLD directly,
without any sanitization step.

scripts/post-review.sh[95-99]
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
`scripts/post-review.sh` prints a GitHub Actions workflow command (`::error::...`) that interpolates `REVIEW_FINDING_SEVERITY_THRESHOLD` without sanitization, which violates the workflow-command sanitization requirement.

## Issue Context
The problematic line is the newly-added invalid-threshold failure path. The interpolated env var must be sanitized (individually) before being placed into the workflow command payload.

## Fix Focus Areas
- scripts/post-review.sh[95-99]

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

---

3. Unset threshold crashes script ✓ Resolved 🐞 Bug ☼ Reliability

Description

scripts/post-review.sh runs with set -u and now expands REVIEW_FINDING_SEVERITY_THRESHOLD
without a presence check after removing the prior defaulting assignment, so an unset variable will
terminate the script before the intended validation error can be printed. This can break older
harnesses or any local/standalone invocation that doesn’t set the variable, even though
harness/review.yaml now provides a default for the configured harness path.

Code

scripts/post-review.sh[95]

-REVIEW_FINDING_SEVERITY_THRESHOLD="${REVIEW_FINDING_SEVERITY_THRESHOLD:-low}"

Relevance

●●● Strong

With set -u, removing ${VAR:-default} risks hard failure; team often adds unset-guard patterns.

PR-#415

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The script enables nounset and then expands the variable directly; without the removed default
assignment, an unset variable will abort the script at expansion time. The harness file does set a
default, but only for the harness-managed environment.

scripts/post-review.sh[19-99]
harness/review.yaml[37-58]

Agent prompt

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

### Issue description
`scripts/post-review.sh` uses `set -euo pipefail` and expands `$REVIEW_FINDING_SEVERITY_THRESHOLD` directly in a `case` statement. Since this PR removed the `:-low` default assignment, an unset variable will cause an immediate `nounset` failure before the script can emit the intended `::error::...invalid...` message.

### Issue Context
The harness now sets a default in `harness/review.yaml`, but the script should still fail *cleanly* (with a clear error) when invoked outside that path (older harness versions, local debugging, ad-hoc runs).

### Fix Focus Areas
- scripts/post-review.sh[19-99]

### Proposed fix
Make the expansion nounset-safe and add an explicit required check, e.g.:

```bash
# Nounset-safe read
REVIEW_FINDING_SEVERITY_THRESHOLD="${REVIEW_FINDING_SEVERITY_THRESHOLD:-}"
: "${REVIEW_FINDING_SEVERITY_THRESHOLD:?REVIEW_FINDING_SEVERITY_THRESHOLD is required (info|low|medium|high|critical)}"

case "${REVIEW_FINDING_SEVERITY_THRESHOLD}" in
 info|low|medium|high|critical) ;;
 *) echo "::error::REVIEW_FINDING_SEVERITY_THRESHOLD='${REVIEW_FINDING_SEVERITY_THRESHOLD}' is invalid (expected info|low|medium|high|critical)"; exit 1 ;;
esac
```

This preserves the new “fail closed” behavior while avoiding an unhelpful unbound-variable crash.

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

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[fullsend-ai-review]

Review

Findings

Medium

• [protected-path] agents/review.md, harness/review.yaml, scripts/post-review.sh, scripts/post-review-test.sh — This PR modifies files under protected paths (agents/, harness/, scripts/). The PR description explains the rationale (moving severity threshold default to harness config, aligning with docs: add configuration checklist for agent feature work #566). Human approval is always required for protected-path changes, regardless of context.

Low

• [pattern-inconsistency] harness/review.yaml — The PR moves review.yaml from runner_env to env.runner under forge.github, aligning with the pattern used in fix.yaml, prioritize.yaml, and triage.yaml. Two files (code.yaml and retro.yaml) still use the older runner_env key. A follow-up PR to migrate them would complete the standardization.

• [stale-doc] docs/review.md — Documentation references ADR 0045, ADR 0055, ADR 0080, and ADR 0081 as authoritative design decisions, but these ADR files do not exist in the repository. Without accessible ADR files, readers cannot verify the claims about harness composition behavior or the rationale for preferring harness-level configuration over CI workflow environment variables.

Previous run

Review

Findings

Medium

• [protected-path] agents/review.md, harness/review.yaml, scripts/post-review.sh, scripts/post-review-test.sh — This PR modifies files under protected paths (agents/, harness/, scripts/). The PR description explains the rationale (moving severity threshold default to harness config, aligning with docs: add configuration checklist for agent feature work #566). Human approval is always required for protected-path changes, regardless of context.

Low

• [pattern-inconsistency] harness/review.yaml — The PR moves review.yaml from runner_env to env.runner under forge.github, aligning with the pattern used in fix.yaml, prioritize.yaml, and triage.yaml. Two files (code.yaml and retro.yaml) still use the older runner_env key. A follow-up PR to migrate them would complete the standardization.

Previous run (2)

Review

Findings

Medium

• [protected-path] agents/review.md, harness/review.yaml, scripts/post-review.sh, scripts/post-review-test.sh — This PR modifies files under protected paths (agents/, harness/, scripts/). The PR description explains the rationale (moving severity threshold default to harness config, aligning with docs: add configuration checklist for agent feature work #566). Human approval is always required for protected-path changes, regardless of context.

Low

• [api-contract-violation] harness/review.yaml — The forge.github section uses env.runner instead of runner_env. Three other harness files (fix.yaml, prioritize.yaml, triage.yaml) already use this pattern in production, confirming harness CLI support. Two files (code.yaml, retro.yaml) still use the old runner_env key — a minor inconsistency worth noting but not a functional risk.

• [test-adequacy] scripts/post-review-test.sh — The new sanitization tests cover colon collapse, %0A, and %0d injection bypass scenarios but do not test the empty/unset REVIEW_FINDING_SEVERITY_THRESHOLD case. The default was changed from :-low to :- (empty), making empty/unset a new failure path. The harness guarantees the variable is set, but an explicit test would improve defense-in-depth coverage.

• [pattern-inconsistency] harness/review.yaml — The PR moves review.yaml from runner_env to env.runner under forge.github, changing the split from 3-3 to 4-2 in favor of env.runner. No ADR exists to establish the canonical pattern. The change is internally consistent with the new top-level env block, but a follow-up PR to migrate code.yaml and retro.yaml would complete the standardization.

Previous run (3)

Review

Findings

Medium

• [protected-path] agents/review.md, harness/review.yaml, scripts/post-review.sh, scripts/post-review-test.sh — This PR modifies files under protected paths (agents/, harness/, scripts/). The PR description explains the rationale (moving severity threshold default to harness config, aligning with docs: add configuration checklist for agent feature work #566). Human approval is always required for protected-path changes, regardless of context.

Low

• [test-adequacy] scripts/post-review-test.sh — The PR changes post-review.sh from gracefully defaulting REVIEW_FINDING_SEVERITY_THRESHOLD to low on invalid/empty values to hard-failing with exit 1. This new error path has no direct test coverage. The 8 test helpers that gained export REVIEW_FINDING_SEVERITY_THRESHOLD="low" ensure existing tests pass, but there is no test that omits or invalidates the variable to confirm the script exits non-zero with the expected ::error:: message. The harness now guarantees the variable is always set, making this a defense-in-depth guard with low risk.

• [GHA workflow command injection] scripts/post-review.sh — The new sanitization strips raw newlines and collapses :: sequences but does not strip URL-encoded newlines (%0A, %0D). GitHub Actions interprets these as literal newlines in workflow command parameters. Exploitability is limited since the variable is defaulted by the harness config, requiring config-level access to override.

• [scope-creep] harness/review.yaml — The PR restructures forge.github.runner_env to forge.github.env.runner in addition to moving the severity threshold default. The PR body mentions env.runner and env.sandbox but does not explicitly call out the runner_env-to-env.runner restructuring or its potential impact on downstream harness consumers. The majority of harness files (fix.yaml, scribe.yaml, triage.yaml, prioritize.yaml) already use the env.runner pattern, so this aligns review.yaml with the dominant convention.

• [unauthorized-design-change] harness/review.yaml — The PR references issue docs: add configuration checklist for agent feature work #566 as authorization for the configuration pattern, but docs: add configuration checklist for agent feature work #566 is a documentation PR (still open) that does not establish architectural recommendations. However, the env.runner/env.sandbox pattern is already the dominant convention in the repository (4 of 7 harness files), so this migration reduces rather than increases pattern inconsistency.

Previous run (4)

Review

Findings

Medium

• [protected-path] agents/review.md, harness/review.yaml, scripts/post-review.sh, scripts/post-review-test.sh — This PR modifies files under protected paths (agents/, harness/, scripts/). The PR description explains the rationale (moving severity threshold default to harness config, aligning with docs: add configuration checklist for agent feature work #566). Human approval is always required for protected-path changes, regardless of context.

Low

• [test-adequacy] scripts/post-review-test.sh — The PR changes post-review.sh from gracefully defaulting REVIEW_FINDING_SEVERITY_THRESHOLD to low on invalid/empty values to hard-failing with exit 1. This behavioral change has no test coverage. The 8 test helpers that gained export REVIEW_FINDING_SEVERITY_THRESHOLD="low" ensure existing tests pass, but there is no test that omits or invalidates the variable to confirm the script exits non-zero with the expected ::error:: message. The harness now guarantees the variable is always set, making this a defense-in-depth guard with low risk.

Previous run (5)

Review

Findings

Medium

• [protected-path] agents/review.md, harness/review.yaml, scripts/post-review.sh, scripts/post-review-test.sh — This PR modifies files under protected paths (agents/, harness/, scripts/). The PR description explains the rationale (moving severity threshold default to harness config, aligning with docs: add configuration checklist for agent feature work #566). Human approval is always required for protected-path changes, regardless of context.

• [test-adequacy] scripts/post-review-test.sh — The PR changes post-review.sh from gracefully defaulting REVIEW_FINDING_SEVERITY_THRESHOLD to low on invalid/empty values to hard-failing with exit 1. This behavioral change has no test coverage. The 8 test helpers that gained export REVIEW_FINDING_SEVERITY_THRESHOLD="low" ensure existing tests pass, but there is no test that omits or invalidates the variable to confirm the script exits non-zero with the expected ::error:: message.

Low

• [code-organization] harness/review.yaml — The env: block is placed after timeout_minutes, but in both other harness files (fix.yaml, scribe.yaml), env: appears before timeout_minutes. This breaks the established key-ordering convention across harness YAML files.

---

Labels: PR modifies review agent infrastructure (agents/review.md, harness/review.yaml, scripts/post-review.sh).

Previous run (6)

Review

Findings

Medium

• [protected-path] agents/review.md, harness/review.yaml, scripts/post-review.sh, scripts/post-review-test.sh — This PR modifies files under protected paths (agents/, harness/, scripts/). The PR links to docs: add configuration checklist for agent feature work #566 and explains the rationale for the changes (moving the REVIEW_FINDING_SEVERITY_THRESHOLD default from script fallback to harness-level configuration). Human approval is always required for protected-path changes, regardless of context.

Low

• [error-handling] scripts/post-review.sh — With set -euo pipefail active and the ${REVIEW_FINDING_SEVERITY_THRESHOLD:-low} default removed, if the variable is ever unset (e.g., someone invokes the script outside the harness), bash will abort with an opaque "unbound variable" error before reaching the case statement's clear ::error:: message. Using ${REVIEW_FINDING_SEVERITY_THRESHOLD:-} in the case statement would let the * branch fire with its descriptive error message instead.

• [dependency-ordering] The PR description references docs: add configuration checklist for agent feature work #566 (still open) as the source of the configuration pattern being adopted. The implementation is independently valid but precedes the documented standard.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:00 PM UTC · Completed 7:16 PM UTC
Commit: 6d8ff96 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:24 PM UTC · Completed 7:38 PM UTC
Commit: 846deeb · View workflow run →

[waynesun09]

Automated review pass — 5 findings below (1 critical, 1 high, 3 medium). None overlap with the existing qodo/fullsend-ai-review bot findings already addressed on this PR (injection sanitization, unset-variable crash, env-block ordering).

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:50 PM UTC · Completed 9:09 PM UTC
Commit: ddc57a8 · View workflow run →

[waynesun09]

Automated review pass — 2 findings below (1 high, 1 medium). Skipped 1 duplicate (missing test coverage for the invalid-threshold hard-failure path was already flagged by fullsend-ai-review's earlier review comment on this PR).

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:16 PM UTC · Completed 2:30 PM UTC
Commit: 3919f26 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:27 PM UTC · Completed 6:42 PM UTC
Commit: 6d18709 · View workflow run →

[waynesun09]

Automated review pass — 3 findings (1 high, 2 medium). None overlap with existing qodo/fullsend-ai-review bot findings or prior human review threads on this PR (all prior discussion was scoped to REVIEW_FINDING_SEVERITY_THRESHOLD sanitization/validation, not the pre-existing label_actions code path or the env/review.env host-file interaction).

Two of the findings below reference lines outside this PR's diff hunks, so GitHub won't accept them as inline comments — included here in the body instead.

---

[HIGH] scripts/post-review.sh:303 — label_actions sanitizer (LA_LABEL) still uses the same non-idempotent :: collapse the PR's own tests prove is bypassable

This PR replaced the single-pass ${var//::/:} collapse for REVIEW_FINDING_SEVERITY_THRESHOLD with a full character-strip specifically because the collapse is non-idempotent and bypassable (proven live: s=':::error:::injected'; s="${s//::/:}" yields ::error::injected, a fully-formed GHA workflow-command delimiter). A few lines later in the very same file, the pre-existing label-action handling still uses the exact vulnerable pattern: LA_LABEL="${LA_LABEL//::/:}" (line 303, alongside LA_ACTION at line 300 — though LA_ACTION is schema-enum-constrained to add/remove and not exploitable).

LA_LABEL comes from the agent's JSON output (label_actions.actions[].label), whose schema pattern ^[a-zA-Z0-9._/: +-]+$ (schemas/review-result.schema.json) explicitly permits colons and passes a value like :::error:::injected straight through both the regex check (post-review.sh's own [[ ! "${LA_LABEL}" =~ ^[a-zA-Z0-9._/:\ +\-]+$ ]] guard) and the vulnerable sanitizer, after which it is echoed into ::warning::...'${LA_LABEL}'... workflow-command lines in the label-validation loop.

Confirmed not in this PR's diff (pre-existing code, untouched by the diff), but it is the exact bug class this PR fixes elsewhere in the same file and was never raised in any of the review rounds on this PR.

Suggestion: Apply the same full-character-strip approach used for REVIEW_FINDING_SEVERITY_THRESHOLD (strip newlines/CR, then strip every : outright — or extract a shared sanitize_for_workflow_command() helper) to LA_LABEL (and LA_ACTION for consistency) instead of the single-pass ${var//::/:} replace, since this PR already establishes that pattern is unsafe.

---

[MEDIUM] docs/review.md:81 — asserts an unverified harness base: extension mechanism (including deep-merge semantics) as settled fact

See inline comment on docs/review.md line 81 for full detail.

---

[MEDIUM] env/review.env:7 — re-exports REVIEW_FINDING_SEVERITY_THRESHOLD from the runner-side value, risking clobber of a partial env.sandbox override

harness/review.yaml's host_files entry loads this file (untouched by this PR's diff) into the sandbox at /sandbox/workspace/.env.d/review.env with expand: true. Line 7 (export REVIEW_FINDING_SEVERITY_THRESHOLD="${REVIEW_FINDING_SEVERITY_THRESHOLD}") is expanded against the runner-side environment (i.e., env.runner.REVIEW_FINDING_SEVERITY_THRESHOLD) at host_files-expansion time, then written into the sandbox as a dotenv file.

Today this is benign because env.runner and env.sandbox both hardcode the same literal "low". But this PR's own new docs/review.md guidance (line 81) tells downstream repos to override severity by setting env.runner / env.sandbox independently via a base: extension — and harness/review.yaml's own new comment explicitly acknowledges "they're allowed to differ." If a downstream repo overrides only env.sandbox (e.g. to "high") without touching env.runner, and if .env.d/review.env is sourced inside the sandbox after the harness's own env.sandbox injection (plausible, since dotenv files are typically sourced late in shell init), this line would silently overwrite the sandbox override back to the stale env.runner value — defeating the override the new docs just told the user to make. This interaction between the new env.runner/env.sandbox mechanism and the pre-existing env/review.env host_file was not discussed anywhere in this PR's extensive review history.

Suggestion: Confirm (with the harness engine authors, or empirically via a dry run) the precedence order between env.sandbox injection and .env.d/*.env sourcing inside the sandbox for this variable. If .env.d/review.env can clobber a harness-supplied env.sandbox override, either remove the REVIEW_FINDING_SEVERITY_THRESHOLD line from env/review.env (since env.sandbox now owns it) or update it to source from the sandbox-side value instead of the runner-side one.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:58 PM UTC · Completed 6:11 PM UTC
Commit: ec7f6c6 · View workflow run →

[waynesun09]

Reviewed the fix at 6d18709 (strip every :/% character outright rather than collapsing substrings) — this closes the GHA workflow-command injection class correctly, since removing all instances of a single character can never reassemble into that character. Confirmed no regressions in the follow-up ADR-0080/0081 doc-citation commit. CI green, all threads resolved.

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 6:51 PM UTC · Completed 7:02 PM UTC
Commit: ec7f6c6 · View workflow run →

[fullsend-ai-retro]

Retro: PR #573 — move severity threshold default to harness env

What happened

Human-authored PR by ralphbean moving REVIEW_FINDING_SEVERITY_THRESHOLD from agent/script defaults into harness/review.yaml. The PR went through 4 rounds of human review by waynesun09 over 4 days (Jul 30–Aug 3), producing 7 commits before merge.

Review agent performance

Both review bots (fullsend-ai-review and qodo-code-review) correctly identified the category of vulnerability — GHA workflow-command injection via unsanitized ::error:: interpolation. The fullsend-ai-review bot even suggested %0A/%0D stripping in its inline comment. However, all agent findings were rated low severity, while the human reviewer treated the same class as high.

The human reviewer went deeper than the agents in three critical ways:

1. Idempotency testing — Demonstrated that :::error:::injected survives single-pass :: collapse to become the valid workflow command ::error::injected (second-pass review).
2. Reassembly testing — Showed that %0%0aA survives %0A stripping and reassembles into a live %0A injection (third-pass review).
3. Adjacent-code scanning — Found the same vulnerable :: collapse pattern in the label_actions sanitizer (LA_LABEL) in the same file, outside the PR diff (fourth-pass review).

Evidence for existing issues

• Security sub-agent should verify shell script security controls against bypass vectors #282 (Security sub-agent should verify shell script security controls against bypass vectors): This PR provides direct evidence — the security sub-agent identified the injection category but did not adversarially test whether the proposed sanitization was bypass-resistant. The human reviewer constructed specific bypass strings that the agent did not attempt.
• Review correctness sub-agent should verify cited mitigations before downgrading security-sensitive findings #432 (Review correctness sub-agent should verify cited mitigations before downgrading security-sensitive findings): The review agent rated the injection finding LOW partly because the harness provides a default value. It did not verify that this mitigation is sufficient to prevent exploitation in all execution contexts.
• Correctness sub-agent should verify fix completeness across similar code sites during re-review #587 (Correctness sub-agent should verify fix completeness across similar code sites during re-review): The human reviewer's fourth-pass finding of the same vulnerability in label_actions is a concrete example of what this issue proposes. Note that Correctness sub-agent should verify fix completeness across similar code sites during re-review #587 is scoped to correctness/re-review; proposal 2 below addresses the security/initial-review gap.
• Harden pre-review.sh and pre-fix.sh against the same GHA workflow-command injection class as pre-code #598 (Harden pre-review.sh and pre-fix.sh against GHA workflow-command injection): Tracks the same injection class in other scripts but explicitly excludes post-review.sh.

Workflow cost

43 fullsend.yaml runs were triggered across the PR lifecycle (5 cancelled). The review agent ran 7 times, once per commit. This volume is proportional to the 4-round review cycle and is not anomalous for a PR with this many iterations.

Proposals filed

• Harden label_actions sanitizer in post-review.sh against GHA workflow-command injection (in fullsend-ai/agents)
• Security sub-agent should cross-reference vulnerability patterns in adjacent code when reviewing fixes (in fullsend-ai/agents)