fix: escape shell metacharacters in expanded env files

[ralphbean]

Summary

• Replace os.ExpandEnv with a shell-safe expander that escapes \, ", $, and ` — the four characters special inside double-quoted shell strings
• Env file templates (export FOO="${FOO}") stay unchanged; the expander ensures substituted values can't break out of the double-quote context
• Adds 21 unit tests and 28 shell roundtrip tests (source expanded file in real sh, read value back, assert exact match)

Problem

HUMAN_INSTRUCTION and other user-authored env vars are expanded into shell env files via os.ExpandEnv, which does naive text substitution. When the value contains ", (, $, or backticks, the expanded file has invalid shell syntax:

# Template:
export HUMAN_INSTRUCTION="${HUMAN_INSTRUCTION}"

# After os.ExpandEnv with value containing double quotes and parens:
export HUMAN_INSTRUCTION="revise "Grant all roles" (for example, with IAM policy)"
#                               ^ closes the quote   ^ syntax error

This caused the pre-agent security scan to flag it as a critical finding, blocking the fix agent entirely.

Failing run: https://github.com/fullsend-ai/.fullsend/actions/runs/26232380969/job/77196067764

Fix

shellSafeExpandEnv uses os.Expand with a custom mapper that calls escapeForDoubleQuotes on each substituted value. Only the four characters that have special meaning inside double quotes need escaping:

Character	Escaped	Why
\	\\	Escape character (must be first to avoid double-escaping)
"	\"	Would close the surrounding double quotes
$	\$	Would trigger variable/command expansion
`	\`	Would trigger command substitution

Characters like (, ), ||, &&, ;, > are not special inside double quotes and don't need escaping.

Test plan

• 21 unit tests verify escaped output for each special char, edge cases, injection attempts, and real-world values from Fix agent: HUMAN_INSTRUCTION should not pass through bash expand #408/Fix agent fails when HUMAN_INSTRUCTION contains shell metacharacters (|| true, $() expansion) #615
• 28 shell roundtrip tests write expanded env files, source them in sh, and assert the value matches the original — covering injection attempts ("; rm -rf /; echo "), command substitution ($(evil)), and the exact values from the failing runs
• go test ./internal/cli/ passes
• go vet ./internal/cli/ clean
• make lint clean

Fixes #408
Fixes #615

[github-actions]

Site preview

Preview: https://290e0d47-site.fullsend-ai.workers.dev

Commit: f2c64ba2e0ef2c850bc84f23c66406a3cc94efc3

[fullsend-ai-review]

Review

Findings

No findings.

Previous run

Review

Findings

No findings.

The change correctly replaces os.ExpandEnv with a shell-safe expander that escapes the four characters special inside double-quoted shell strings (\, ", $, `). The escape order is correct (backslash first to prevent double-escaping). The implementation uses os.Expand with a custom mapper, which preserves the existing ${VAR} expansion behavior while adding safety.

The test suite is thorough: 21 unit tests cover individual escape cases, edge cases, and injection attempts; 28 shell roundtrip tests source the expanded file in a real sh process and verify exact value preservation. The roundtrip tests are the definitive correctness proof — if values survive a real shell source-and-read cycle, the escaping is sound.

The remaining os.ExpandEnv calls in the codebase are appropriate: line 938 in run.go expands file paths (not shell-sourced content), and sandbox.go uses it for credential/config values passed as CLI args (not sourced as shell).

[waynesun09]

Review Squad (10 agents) — 1 HIGH, 2 MEDIUM findings

Well-executed security fix. The escaping logic is POSIX-correct (all four double-quote special characters handled in the right order) and the shell roundtrip tests are exemplary. The HIGH finding — 9 env template lines across 5 files use unquoted ${VAR} patterns where the escaping assumptions don't hold — is not exploitable today (all values are machine-generated), but the templates should be standardized to "${VAR}" in a follow-up to eliminate the risk class.

[waynesun09]

HIGH — Unquoted env templates are not protected by this escaping

The doc comment says "Templates use the standard export FOO="${FOO}" pattern" but 9 template lines across 5 files use unquoted ${VAR}:

• code-agent.env: export ISSUE_NUMBER=${ISSUE_NUMBER}, GITHUB_ISSUE_URL, GH_TOKEN
• review.env, triage.env, prioritize.env: export GH_TOKEN=${GH_TOKEN}
• gcp-vertex.env: ANTHROPIC_VERTEX_PROJECT_ID, CLOUD_ML_REGION, GOOGLE_CLOUD_PROJECT

In an unquoted context, characters like spaces, ;, |, &&, > are shell operators — the escaper does NOT protect against them. Currently safe because these are machine-generated values (tokens, integers, project IDs), but the function name shellSafeExpandEnv implies broader safety than it provides.

Suggestion (follow-up PR): Standardize all env templates to export VAR="${VAR}" — a one-line change per template, zero regression risk (double-quoting simple values is a no-op in shell), eliminates an entire class of future bugs.

7/10 review agents flagged this (strong consensus)

[waynesun09]

MEDIUM — Misleading test name

This test uses FOO=bar (no special characters) but the name "unquoted template still safe" implies the escaper handles unquoted contexts safely for arbitrary values. It does not — e.g., FOO="hello world" would expand to export FOO=hello world, and sh would interpret world as a separate command.

Suggest renaming to "unquoted template with simple value" to remove the safety implication, and optionally adding a comment documenting that unquoted templates are only safe for values without whitespace or shell operators.

5/10 review agents flagged this

[waynesun09]

MEDIUM — Missing test for $VAR (no braces) expansion

All test templates use ${VAR} (with braces), but os.Expand also handles $VAR (without braces). Adding one test case with template export FOO="$FOO" would confirm the brace-less path also gets escaped values and document this behavior.

3/10 review agents flagged this