[waynesun09]

What

The post-script that fullsend agent new generates no longer echoes a rejected status raw. It flattens CR/LF to spaces, caps the value at 40 characters, and writes it with printf '%s\n'.

Dry-run preview: the dry-run branch previews the model's comment on stdout, and under GitHub Actions a line starting with :: is a workflow command. The runner trims leading spaces before checking. The preview is now wrapped in ::stop-commands::<token> … ::<token>::, where the token is 16 bytes from /dev/urandom in hex. It's guarded to exactly 32 characters, and the script exits 1 before printing anything without one. This is the documented Actions mechanism. It works whatever the line breaks are, unlike prefixing each line.

Guide example: the example post-script in docs/guides/user/building-custom-agents.md echoed a raw STATUS the same way. It now flattens, caps and prints it with printf. docs/cli/agent.md shows the new dry-run output, captured from the generated script.

Why

status is model output. jq -r turns JSON \n/\r escapes into real line breaks, so a crafted value could split the runner's log line and start a new one with a :: workflow-command prefix. echo also expands a literal \n under xpg_echo, which printf '%s' never does. summary already had this check; status did not.

Tests

internal/agentnew/postscript_status_test.go:

• A status of bogus + LF, CR or CRLF + ::error::injected is rejected on a single log line.

• With bash -O xpg_echo, a literal backslash-n stays on one line.

• A dry-run findings comment containing ::error::injected, ::add-mask::x (after a CR) and an indented ::warning:: stays inside a stop-commands block. The block opens on the first line and closes on the last with the same 32-hex token, and the token differs between runs.

• With an od that yields no token, the dry run exits 1 and prints nothing.

These two came from the review summary on e64be8f.

Each of these mutations fails a test: removing the flattening, flattening LF only, switching printf back to echo, removing the 40-character cap, dropping the stop-commands wrap, fixing the token, and dropping the token-length guard.

The goldens are regenerated; the diff is this hunk only.

go test -race ./internal/agentnew/

Split out of #7243. Part of #6966.

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Prevent log-line injection from rejected agent statuses

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

AI Description

• Flatten and cap rejected model-generated statuses before logging them, preventing forged runner
 log lines.
• Use printf so literal backslash escapes cannot become line breaks under Bash’s xpg_echo
 option.
• Add execution tests for LF, CR, CRLF, backslash escapes, and the 40-character cap.

Diagram

graph TD
  R["Agent result"] --> J["jq extraction"] --> V{"Valid status?"} -->|No| S["Flatten and cap"] --> L["Single-line stderr"]
  V -->|Yes| C["Comment path"]

Loading

High-Level Assessment

Sanitizing at the generated script’s rejection point is the narrowest effective fix: it protects the runner log without changing how valid statuses are handled. Rejecting the value earlier would not by itself make the rejection message safe to log.

Files changed (4) +135 / -3

Bug fix (1) +10 / -1

post.sh.tmplSanitize invalid status before writing to stderr +10/-1

Sanitize invalid status before writing to stderr

• Replaces CR/LF with spaces, limits the displayed value to 40 characters, and uses 'printf' instead of 'echo' when reporting an invalid status.

internal/agentnew/templates/post.sh.tmpl

Tests (3) +125 / -2

postscript_status_test.goTest safe logging of rejected statuses +105/-0

Test safe logging of rejected statuses

• Executes the generated script with LF, CR, and CRLF injection attempts and checks that rejection stays on one log line. Also verifies literal backslash-n under 'xpg_echo' and the 40-character display cap.

internal/agentnew/postscript_status_test.go

triage.txtRefresh triage generated-script golden +10/-1

Refresh triage generated-script golden

• Records the sanitized invalid-status rejection branch in the generated triage fixture.

internal/agentnew/testdata/golden/triage.txt

validation-loop.txtRefresh validation-loop generated-script golden +10/-1

Refresh validation-loop generated-script golden

• Records the same invalid-status rejection branch in the validation-loop fixture.

internal/agentnew/testdata/golden/validation-loop.txt

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Tip of the day

💡 Did you know, you can keep summaries lean with Findings visible per group, which tucks the rest behind a View link

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 2:51 PM UTC · Ended 3:37 PM UTC

Commit: e64be8f · View workflow run →

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure (agent timed out after 45m0s without completing (timeout: 45m0s)) · Started 2:51 PM UTC · Completed 3:37 PM UTC

Commit: e64be8f · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $2.21

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:43 PM UTC · Completed 4:24 PM UTC

Commit: e64be8f · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $3.35

[fullsend-ai-review]

Risk Assessment: moderate (2/5)

Details

Small, well-tested security hardening fix touching no protected paths or workflows, with moderate recent churn in generator templates and an easily revertible change surface; consistent with the prior assessment, as only a doc wording fix changed since.

Previous run

Risk Assessment: moderate (2/5)

Details

Moderate risk (score 2) reflecting a scoped hardening fix to generated script templates and docs with strong test coverage, no CI or dependency changes, and moderate git churn driven primarily by active documentation files.

Previous run (2)

Risk Assessment: moderate (2/5)

Details

Small, well-tested security fix touching no protected paths or workflows, with moderate recent churn in agent generator templates and an easily revertible change surface.

[fullsend-ai-review]

Looks good to me

Previous run

Review

Findings

Low

• [incorrect-doc] docs/cli/agent.md:334 — The new explanation of the ::stop-commands:: wrapping claims "a GitHub Actions log masks it as ***." This is not accurate: GitHub Actions only masks values explicitly registered as secrets or passed through ::add-mask::, and internal/agentnew/templates/post.sh.tmpl never emits ::add-mask:: for the stop-commands token — it's a non-secret random delimiter, not something that needs hiding. The claim also contradicts the example transcript on the same page: lines 320 and 327 show the identical 32-hex token printed in full, unmasked.
  Remediation: Drop the masking clause and keep the real security property instead, e.g. "The token is random on every run so model output cannot predict the closing delimiter." If masked logs are genuinely wanted, emit printf '::add-mask::%s\n' "${stop_token}" before the stop-commands block so the runner redacts it.

---

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 (2)

Review

Findings

High

• [sub-agent-failure] N/A — The correctness sub-agent did not return findings after two dispatch attempts (each timed out at 900s). The orchestrator independently verified several correctness-adjacent facts by direct execution: the ${status//[$'\r\n']/ } flattening correctly collapses LF, CR, and CRLF to spaces; printf '%s\n' does not expand a literal \n even under bash -O xpg_echo; the 40-character cap applies after flattening; and the two golden fixtures (triage.txt, validation-loop.txt) regenerate to byte-for-byte match the new template (verified by diffing a reconstructed post-fix golden against the template with __AGENT_NAME__/__DRY_RUN_VAR__ substituted). However, a dedicated correctness pass — test design adequacy, whether the new postscript_status_test.go helpers are reused correctly from postscript_exec_test.go without duplication, and a full read of internal/agentnew/render.go for consumer completeness — did not complete. Per review policy, a skipped correctness review on a safety-relevant change is treated as a high-severity gap rather than silently approved.
  Remediation: Re-run a correctness-focused review of this diff (a human reviewer or a follow-up automated pass) before merge, focusing on test adequacy and whether render.go needs no corresponding change.

Medium

• [incorrect-doc] docs/guides/user/building-custom-agents.md:449 — The hand-written custom-agent guide's example post-script still echoes untrusted model output raw: echo "ERROR: Unknown or missing status '${STATUS}'". This PR's own commit message establishes that status is model output and must never be echoed raw, because jq -r turns JSON \n/\r escapes into real line breaks that can split a runner's log line and inject a ::-prefixed GitHub Actions workflow command, and because echo expands a literal \n under xpg_echo while printf '%s' does not. The doc's example teaches exactly the pattern this PR just classified as unsafe, for users hand-writing a post-script outside the generator.
  Remediation: Update the example's rejection branch to flatten CR/LF, cap the length, and log via printf '%s\n', matching the pattern now used in internal/agentnew/templates/post.sh.tmpl:

    *)
      shown="${STATUS//[$'\r\n']/ }"
      printf '%s\n' "ERROR: Unknown or missing status '${shown:0:40}'" >&2
      exit 1
      ;;

Low

• [workflow-command-injection] internal/agentnew/templates/post.sh.tmpl:130 — In the dry-run branch (if [[ "${__DRY_RUN_VAR__:-}" == "1" ]]; then printf '**%s**\n\n%s\n' "${summary}" "${comment}"), the model-controlled comment field is written to stdout with its embedded newlines intact — comment is the one field this script does not reject for multi-line content. A comment body containing a line starting with :: (e.g., ::error::injected or ::add-mask::...) becomes its own log line whenever this dry-run path executes under a GitHub Actions runner, which parses both stdout and stderr for workflow commands. This is the same injection class this PR closes for status, left open on an adjacent, pre-existing path not touched by this diff. Not introduced by this PR, and the production (non-dry-run) path pipes comment to fullsend issues post-comment via stdin rather than a log stream, so the blast radius is limited to dry-run invocations running on an actual Actions runner.
  Remediation: Before the dry-run printf, neutralize any line in comment whose first non-empty characters are :: (and flatten bare \r), or route the dry-run preview somewhere other than the runner's parsed stdout/stderr.

• [sub-agent-failure] N/A — The adversarial challenger pass was skipped: fewer than 600s remained in the time budget when steps 6a–6c completed. The pre-challenger finding set above was used as-is, without adversarial deduplication or false-positive removal.

Note

The PR's core claim was independently verified: the new case branch in post.sh.tmpl correctly flattens CR/LF to spaces, caps the value at 40 characters, and uses printf '%s\n' (not echo), closing the specific workflow-command-injection vector described for the status field. The new test file's three scenarios (LF/CR/CRLF flattening, xpg_echo non-expansion, 40-char cap) were reproduced manually against the exact bash snippet and all passed as described. Both regenerated golden fixtures match the template change exactly, with no unrelated drift.

---

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-review]

🤖 Review · ❌ Terminated · Started 4:48 PM UTC · Ended 5:06 PM UTC

Commit: c057eb8 · View workflow run →

[github-actions]

Site preview

Preview: https://2b6dcc6d-site.fullsend-ai.workers.dev

Commit: 3664e90f6e60a2ea7657d995540e63569b914eb3

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 4:48 PM UTC · Completed 5:05 PM UTC

Commit: c057eb8 · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $4.67

Outdated: its findings (dry-run workflow-command injection, building-custom-agents.md raw echo) are fixed in c057eb8.

[waynesun09]

@ggallen when you have a moment, could you confirm that the reply on your post.sh.tmpl:88 thread covers the CR/LF concern? ${status//[$'\r\n']/ } replaces both characters on bash 3.2 and 5.3 (outputs in the thread), and CI tests LF, CR-only and CRLF.

Since then, c057eb8 also wraps the dry-run preview in a random-token ::stop-commands:: block, so model output there can't issue workflow commands either.

[ggallen]

@waynesun09 I re-tested the exact ${status//[$'\r\n']/ } expression under Bash 5.3; it does replace LF, CR, and CRLF as intended. The latest ::stop-commands:: wrapper also closes the adjacent dry-run workflow-command path, so the original concern is resolved.

[ggallen]

The CR/LF concern is resolved in the latest head. I left one minor documentation comment about the token-masking claim.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:30 PM UTC · Completed 7:43 PM UTC

Commit: 3664e90 · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $2.57