fix(ssh): reject NUL-containing command environments

[steipete]

The stdin-based SSH environment transport accepted NUL-containing strings from constructed/profile/preset environment values. Native process environments cannot represent NUL, so accepting the staged shell file can silently change the intended value.

Reject these values with exit code 2 before opening an SSH upload, and report only the validated variable name. Add regression coverage for early rejection with no SSH activity or value disclosure, plus an approximately 80 KiB multiline round trip to preserve valid large payloads.

Follow-up to #2556.

Validation: TestSSHCommandEnvRejectsNULBeforeUpload failed before the fix because staging returned no error. The focused env transport/allowlist race shard now passes, including the large value, cancellation/failure cleanup, PowerShell/WSL fixtures, and real OpenSSH stdin separation. Gofmt, go vet ./..., CLI build/invocation, and both docs checks passed. Codex autoreview is scoped-clean through P2. No cloud resources were created; native Windows/WSL2 live execution was unavailable and the coordinator owns the full remote suite.

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

[clawsweeper]

Codex review: blocked before merge. Reviewed September 26, 2026, 12:24 AM ET / 04:24 UTC (Revision 2).

ClawSweeper review

What this changes

The branch rejects NUL-containing SSH command environment values with a variable-name-only exit-2 error, adds rejection and large-value tests, and updates the documentation and changelog.

Merge readiness

⛔ Blocked before merge - 4 items remain

Keep open. Current main still accepts NUL-containing values in the shared SSH command-environment staging path, and this focused fix is distinct from the earlier transport PR.

Priority: P2
Reviewed head: ba1f939e0a6c8e42d8f79df28310c57c8f5a95a6
Owner decision: Required. See Decision needed.

Review scores

Measure	Result	What it means
Overall readiness	🦐 gold shrimp (3/6)	The focused patch and regression coverage are useful; a saved-configuration CLI observation and the unresolved compatibility choice limit readiness confidence.
Proof confidence	🦐 gold shrimp (3/6)	Not applicable: The Repository State classifies the author as MEMBER, so the external-contributor proof gate does not apply. The changed production owner is SSH command-environment staging; its direct fake-SSH test checks early rejection, but no saved-configuration CLI after-fix trace is supplied. No stored-data contract changes.
Patch quality	🐚 platinum hermit (4/6)	1 actionable review finding remain.

Verification

Check	Result	Evidence
Real behavior	Not applicable	Not applicable: The Repository State classifies the author as MEMBER, so the external-contributor proof gate does not apply. The changed production owner is SSH command-environment staging; its direct fake-SSH test checks early rejection, but no saved-configuration CLI after-fix trace is supplied. No stored-data contract changes.
Evidence reviewed	10 items	Introduced validation: The patch checks valid environment values for NUL before formatting the private file or invoking SSH. Current main still needs the fix: The pinned main revision copies selected values into the staging map without a NUL check. Shared caller: Normal runs pass selected inline and capability values through this staging function; preflight and cache warm also call it.
Findings	1 actionable finding	[P3] Add the full PR URL to the changelog bullet
Security	None	None.

How this fits together

Crabbox selects environment values from run configuration and allowlists. The CLI stages command-scoped values through SSH stdin into a private file that the remote session loads before running a workload.

flowchart LR
A[Selected command values] --> B[CLI SSH staging]
B --> C{Value contains NUL?}
C -->|Yes| D[Exit 2 with variable name]
C -->|No| E[Private stdin upload]
E --> F[Remote environment file]
F --> G[Workload]

Loading

Decision needed

Question	Recommendation
Should runs with existing saved profile or preset command-environment values containing NUL now fail with exit code 2?	Accept explicit rejection: Approve the exit-2 error for unsupported saved values and document the configuration correction.

Why: A native process cannot receive the intended value, but the new failure changes the behavior of existing saved configurations.

Before merge

• Add the full PR URL to the changelog bullet (P3) - The new user-visible bullet lacks the full PR URL required by AGENTS.md. Add fix(ssh): reject NUL-containing command environments #2573 so the release note remains traceable.
• Resolve merge risk (P1) - Existing saved profile or preset command-environment values containing NUL will now stop an SSH run with exit code 2 after lease setup, where the run previously continued with an unrepresentable value; this upgrade behavior needs explicit maintainer acceptance.
• Complete next step (P2) - Add the full PR URL to the changelog bullet and obtain explicit maintainer acceptance of exit 2 for existing saved NUL-containing command values.
• Resolve maintainer decision - Resolve the maintainer decision shown above before merge.

Findings

• [P3] Add the full PR URL to the changelog bullet — CHANGELOG.md:6

Agent review details

Security

None.

Review metrics

None.

Merge-risk options

Maintainer options:

1. Accept exit 2 for invalid values (recommended)
   Approve fail-closed behavior because continuing cannot preserve a NUL-containing native environment value.
2. Move validation before lease setup
   Pause this PR if avoiding remote setup for an invalid saved value is required for upgrade safety.

Technical review

Best possible solution:

Keep validation at the shared command-staging boundary and document how to remove an invalid saved value when the name-only error appears.

Do we have a high-confidence way to reproduce the issue?

Yes, from source: a saved profile or preset value can reach the staging loop that lacks a NUL check on current main. The added direct regression test supplies that value; this read-only review did not execute it.

Is this the best way to solve the issue?

Yes for command-staged SSH values: checking the shared staging path before upload is narrow and preserves valid values. A saved-configuration CLI observation would better establish the user-facing failure.

Full review comments:

• [P3] Add the full PR URL to the changelog bullet — CHANGELOG.md:6
  The new user-visible bullet lacks the full PR URL required by AGENTS.md. Add fix(ssh): reject NUL-containing command environments #2573 so the release note remains traceable.
  Confidence: 0.96

Overall correctness: patch is correct
Overall confidence: 0.89

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 596d907ba514.

Labels

Label changes:

No label changes.

Label justifications:

• P2: This is a focused SSH command-environment correctness fix with limited blast radius.
• merge-risk: 🚨 compatibility: A saved NUL-containing command value that previously proceeded with corruption will now stop the run with exit code 2.
• rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
• status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The Repository State classifies the author as MEMBER, so the external-contributor proof gate does not apply. The changed production owner is SSH command-environment staging; its direct fake-SSH test checks early rejection, but no saved-configuration CLI after-fix trace is supplied. No stored-data contract changes.

Evidence

Acceptance criteria:

• [P1] git diff --check.
• [P1] rg -n 'fix(ssh): reject NUL-containing command environments #2573' CHANGELOG.md.

What I checked:

• Introduced validation: The patch checks valid environment values for NUL before formatting the private file or invoking SSH. (internal/cli/run_env_transport.go:25, ba1f939e0a6c)
• Current main still needs the fix: The pinned main revision copies selected values into the staging map without a NUL check. (internal/cli/run_env_transport.go:25, 596d907ba514)
• Shared caller: Normal runs pass selected inline and capability values through this staging function; preflight and cache warm also call it. (internal/cli/run.go:2853, ba1f939e0a6c)
• Saved profile and preset values: Profile and preset expansion is merged into the inline selection that reaches SSH staging. (internal/cli/run.go:857, ba1f939e0a6c)
• Separate profile-file boundary: Values supplied through --env-from-profile use a separate upload path before command staging; this patch's guard applies to command-staged values. (internal/cli/run.go:2774, ba1f939e0a6c)
• Focused regression coverage: The new test checks exit code 2, no SSH invocation, and no value disclosure. A transport test adds a large valid multiline value. (internal/cli/run_env_nul_test.go:26, ba1f939e0a6c)

Likely related people:

• Peter Steinberger: Raw commit fc3cb7d adds internal/cli/run_env_transport.go:18 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: fc3cb7d44ef1; files: internal/cli/run_env_transport.go)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

• Show a CLI run with a saved profile or preset env value containing NUL that exits 2, names only the variable, and makes no command-environment upload.

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (1 earlier review cycle)

• reviewed 2026-09-26T02:32:31.291Z sha 0d8cb9d :: blocked before merge. :: [P3] Add the full PR link to the changelog bullet