feat(openshell): upgrade to 0.0.54, fix sandbox integration

[rh-hemartin]

build(openshell): upgrade to 0.0.54, fix sandbox integration

• Install via deb package (CI runner default)
• Fix sandbox download cmd broken by 0.0.54 API change
• Set working/config dir to /sandbox/ for direct artifact download
• Add UploadFile: works around upload-in-git-repo bug that placed
  schema at wrong sandbox path; self-corrects on path mismatch
• Move credentials to /tmp/ (outside workspace, reduces confusion)
• Fix review agent hardcoded /tmp/workspace path
• Rename variables that caused implementation confusion

• Tested on rh-hemartin-fullsendai/standalone-fullsend with triage, code and review agents.

Closes #1763, closes #1767, closes #1768, closes #1765, closes #1766, closes #878, closes #1830

[github-actions]

Site preview

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

Commit: 228e036703ecb3f36e1ab62666f7c6924f3b45d7

[fullsend-ai-review]

Review

Findings

Medium

• [logic-error] internal/sandbox/sandbox.go:417 — UploadFile uses a fixed temporary path /tmp/<basename> inside the sandbox for the mv workaround. If remotePath has the same basename as a file already at /tmp/<basename>, the mv will silently overwrite it. After the workaround moves the file to /tmp/<basename>, it does rm -r <remotePath> then mv /tmp/<basename> <remotePath>. If rm -r succeeds but the final mv fails, both the original file and the temp copy are lost — there is no rollback. The blast radius is contained (only operates on CLI-uploaded files with known basenames on a fresh sandbox), but the pattern is fragile.
  Remediation: Use a unique temp path inside the sandbox, e.g., include a random suffix or use mktemp via Exec.

• [stale-reference] docs/ADRs/0024-harness-definitions.md:450 — Contains 4 references to /tmp/workspace paths in harness YAML examples that are not updated by this PR (e.g., dest: /tmp/workspace/.env.d/gcp-vertex.env). These should now be /sandbox/workspace paths.

• [stale-reference] docs/ADRs/0032-safe-push-wrapper-for-sandboxed-agents.md:73 — Contains 3 references to /tmp/workspace/ paths that are not updated by this PR.

• [stale-reference] docs/superpowers/plans/2026-05-04-retro-agent.md:667 — Contains 4 references to /tmp/workspace paths in harness YAML examples not updated by this PR.

Low

• [edge-case] internal/sandbox/sandbox.go:425 — UploadFile workaround uses 1-second timeouts for all sandbox Exec operations (test -f, mv, rm -r). While these are trivial filesystem operations unlikely to time out, increasing to 5-10 seconds would match the codebase convention.

• [error-handling] internal/sandbox/sandbox.go:413 — Error message checking for file: %s only mentions wrongPath when the file is not found at either path, not remotePath. Slightly misleading for debugging.

• [error-handling] internal/sandbox/sandbox.go:422 — Error messages fixing UploadFile path: %s, %s are identical for three different operations (mv to tmp, rm -r, mv back), making it harder to determine which step failed.

• [architectural-coherence] internal/scaffold/fullsend-repo/env/gcp-vertex.env:4 — Credentials path changed from /tmp/workspace/.gcp-credentials.json to /tmp/.gcp-credentials.json. This is security-positive (credentials outside workspace), but the rationale should be documented. See also: [credential-placement] info finding.

• [scope-creep] internal/sandbox/sandbox.go:890 — 54-line UploadFile workaround for OpenShell bug is directly necessitated by the upgrade and well-commented with upstream issue link. Not actual scope creep, but the workaround's complexity is worth noting.

• [supply-chain] action.yml:117 — OpenShell install uses curl -LsSf | sh with SHA-pinned URL. No checksum verification of downloaded script content. The SHA pin provides strong integrity guarantees in practice.

• [documentation] internal/sandbox/sandbox.go:890 — UploadFile doc comment could be more explicit about the symptom (upload places file in wrong directory), though the upstream issue link provides context.

• [architectural-alignment] action.yml:72 — Removes explicit gateway health check logic without documenting how readiness is verified in the new OpenShell install method.

Info

• [naming] internal/cli/run.go:387 — Variable rename repoSrc → hostRepositoryDir and repoDir → remoteRepositoryDir improves clarity by disambiguating host vs. sandbox paths, directly relevant to the path migration.

• [test-adequacy] internal/sandbox/sandbox_test.go:378 — Only test for UploadFile verifies failure when openshell not in PATH. Workaround logic (multi-step mv/rm/mv) is untested, though testing requires a running sandbox environment.

• [network-exposure] action.yml:83 — OPENSHELL_BIND_ADDRESS=0.0.0.0 binds gateway to all interfaces. Pre-existing behavior, not a regression.

• [credential-placement] internal/scaffold/fullsend-repo/harness/code.yaml — GCP credentials moved from workspace to /tmp/. Security-positive change: credentials no longer co-located with agent's working directory.

• [stale-doc] docs/plans/agent-execution-environment.md:49 — References OPENSHELL_VERSION=0.0.37-dev. Historical planning document.

• [documentation-drift] docs/ADRs/0036-agent-execution-sandbox.md:186 — ADR documents 'OpenShell 0.0.37-dev+ with Podman support'. The 0.0.37-dev+ notation encompasses 0.0.54, but is stale.

• [tier-alignment] PR title uses feat(openshell): but body says build(openshell):. Minor inconsistency in commit type classification.

Previous run

Review

Findings

Medium

• [stale-reference] docs/guides/dev/cli-internals.md:273 — The ASCII diagram line "Upload to /tmp/workspace:" was not updated to /sandbox/workspace, despite adjacent lines in the same diagram (PATH at line 283 and CLAUDE_CONFIG_DIR at line 284) being updated by this PR. Incomplete migration within the same file.
  Remediation: Update line 273 to Upload to /sandbox/workspace:.

• [stale-reference] internal/harness/harness_test.go — Test fixtures contain 9 references to /tmp/workspace paths (lines 88, 92, 104, 110, 118, 172–174, 209) that are now inconsistent with the production harness YAML files updated in this PR. While the harness parser doesn't validate paths (so tests still pass), the fixtures should match production values for maintainability and to catch regressions from future path-dependent logic.
  Remediation: Update test YAML fixtures and assertions to use /sandbox/workspace (or /tmp/.gcp-credentials.json for credential paths) to match the updated harness files.

Low

• [error-handling] internal/sandbox/sandbox.go:~409 — UploadFile error messages are vague and inconsistent with the package's established patterns. When the file is not found at either path, fmt.Errorf("checking for file: %s", wrongPath) only mentions the fallback path, not the intended remotePath. The three "fixing UploadFile path: %s, %s" messages are identical and don't indicate which step failed. The codebase convention is "<verb> to/from sandbox %q <detail>: %w".
  Remediation: Use fmt.Errorf("file not found at %s or fallback %s", remotePath, wrongPath) for the not-found case, and distinct messages for each mv/rm step (e.g., "moving uploaded file to temp", "removing incorrect directory", "moving file to final path").

• [edge-case] internal/sandbox/sandbox.go:~413 — UploadFile workaround uses 1-second timeouts for mv and rm -r operations (lines 413, 421, 429) while the test -f checks use 10 seconds. The codebase consistently uses 10-second timeouts for sandbox Exec operations. On a loaded sandbox, rm -r of a directory could time out, leaving the sandbox in an inconsistent state. Additionally, the intermediate path /tmp/<basename> could collide if two sequential UploadFile calls target files with the same base name.
  Remediation: Use 10*time.Second for all Exec operations in the workaround. Consider using a unique temp path (e.g., include a random suffix).

• [supply-chain] action.yml:117 — OpenShell install uses curl -LsSf https://raw.githubusercontent.com/NVIDIA/OpenShell/${OPENSHELL_SHA}/install.sh | sh. The SHA pin prevents tag mutability and -f prevents partial error-page execution, but there is no checksum verification of the downloaded script content. The previous uv-based install had package-manager integrity verification.

• [stale-doc] docs/ADRs/0024-harness-definitions.md:450 — Example YAML snippets at lines 450, 453, 508, 511 reference /tmp/workspace paths. As an accepted ADR, these may intentionally reflect the state at acceptance time, but the path references no longer match runtime behavior.

• [stale-doc] docs/superpowers/plans/2026-05-04-retro-agent.md:667 — Harness YAML examples at lines 667–675 reference /tmp/workspace and /tmp/workspace/.gcp-credentials.json paths, now stale after the path migration.

• [stale-doc] docs/ADRs/0032-safe-push-wrapper-for-sandboxed-agents.md:73 — References /tmp/workspace/ paths at lines 73, 101, 145 in explanatory text and audit log paths.

Info

• [network-exposure] action.yml:83 — OPENSHELL_BIND_ADDRESS=0.0.0.0 binds the gateway to all interfaces, but this is pre-existing behavior (the old code also used --bind-address 0.0.0.0).

• [credential-placement] internal/scaffold/fullsend-repo/harness/*.yaml — GCP credentials moved from /sandbox/workspace/.gcp-credentials.json to /tmp/.gcp-credentials.json. This is a security-positive change: credentials are now outside the git working tree, reducing risk of accidental commit or exfiltration via git operations.

Previous run (2)

Review

Findings

Medium

• [stale-reference] docs/guides/dev/cli-internals.md:262 — The ASCII diagram line "Upload to /tmp/workspace:" was not updated to /sandbox/workspace, despite lines 272–273 in the same diagram being updated. Incomplete migration within the same file.
  Remediation: Update line 262 to Upload to /sandbox/workspace:.

• [stale-reference] internal/harness/harness_test.go — Test fixtures contain 9 references to /tmp/workspace paths (lines 88, 92, 104, 110, 118, 172–174, 209) that are now inconsistent with the production harness YAML files updated in this PR. While the harness parser doesn't validate paths (so tests still pass), the fixtures should match production values for maintainability.
  Remediation: Update test YAML fixtures and assertions to use /sandbox/workspace (or /tmp/.gcp-credentials.json for credential paths) to match the updated harness files.

Low

• [error-handling] internal/sandbox/sandbox.go:~409 — UploadFile error message when file is not found at either the intended path or the workaround path only mentions the wrong path. The message checking for file: <wrongPath> doesn't indicate that remotePath was also checked, making diagnosis harder.
  Remediation: fmt.Errorf("UploadFile: file not found at %s or expected fallback %s", remotePath, wrongPath).

• [edge-case] internal/sandbox/sandbox.go:~414 — UploadFile workaround uses 1-second timeouts for mv and rm -r operations, while the test -f check uses 10 seconds. If the OpenShell bug creates a non-trivial directory structure at remotePath, rm -r could time out, leaving the sandbox in an inconsistent state (file in /tmp/ but original directory partially removed).
  Remediation: Use 10-second timeouts for the mv/rm -r operations to match the test -f timeout.

• [supply-chain] action.yml:117 — OpenShell install uses curl -LsSf | sh with a SHA-pinned URL. While the SHA pin prevents tag mutability and -f prevents partial error-page execution, there is no checksum verification of the downloaded script content. The previous uv-based install had package-manager integrity verification.

• [stale-doc] docs/ADRs/0024-harness-definitions.md — Example YAML snippets at lines 450, 453, 508, 511 reference /tmp/workspace paths. As accepted ADRs these may intentionally reflect the state at acceptance time, but the path references no longer match runtime behavior.

• [stale-doc] docs/superpowers/plans/2026-05-04-retro-agent.md — Harness YAML examples at lines 667–675 reference /tmp/workspace and /tmp/workspace/.gcp-credentials.json paths. These are now stale after the path migration.

Info

• [stale-doc] docs/ADRs/0032-safe-push-wrapper-for-sandboxed-agents.md — References /tmp/workspace/ paths at lines 73, 101, 145 in explanatory text and audit log paths.

• [network-exposure] action.yml:83 — OPENSHELL_BIND_ADDRESS=0.0.0.0 binds the gateway to all interfaces, but this is pre-existing behavior (the old code also used --bind-address 0.0.0.0).

Previous run (3)

Review

Findings

High

• [supply-chain] action.yml:117 — The OpenShell install step pipes a remote script directly into sh via curl -LsSf https://raw.githubusercontent.com/NVIDIA/OpenShell/${OPENSHELL_SHA}/install.sh | sh. While the URL is pinned to a commit SHA (OPENSHELL_SHA), there is no checksum verification of the downloaded script before execution. The OPENSHELL_SHA value is written to GITHUB_ENV in a prior step (line 74), meaning any earlier composite action step that can append to the GITHUB_ENV file could override the SHA to point to an attacker-controlled commit. The previous install method (uv tool install from PyPI) provided package-manager-level integrity verification that this approach lacks.
  Remediation: Download the script to a temporary file, verify its SHA-256 checksum against a hardcoded expected value in action.yml, and only then execute it. Alternatively, move OPENSHELL_SHA and OPENSHELL_VERSION into the same step that consumes them as local shell variables instead of GITHUB_ENV, to eliminate the step-injection vector.

Medium

• [command-injection] internal/sandbox/sandbox.go:402 — The new shellQuote helper is correctly applied in UploadFile, but pre-existing UploadDir does not use it. At line 402, remotePath and remoteTar are interpolated into a shell command string (mkdir -p %s && tar -xzf %s -C %s && rm %s) without quoting. The introduction of shellQuote in this PR creates an inconsistency where some Exec calls are properly quoted and others are not, increasing the risk that a future caller passes user-influenced paths to the unquoted functions.
  Remediation: Apply shellQuote() to all path arguments interpolated into shell command strings in UploadDir (line 402) for consistency.

Low

• [incomplete-migration] docs/guides/dev/cli-internals.md:273 — The diff updates PATH=/tmp/workspace/bin to PATH=/sandbox/workspace/bin on the adjacent line but leaves CLAUDE_CONFIG_DIR=/tmp/claude-config unchanged. Since SandboxClaudeConfig is changing to /sandbox/claude-config in this PR, this documentation line is now stale.

• [test-inadequate] internal/sandbox/sandbox_test.go:383 — The only test for UploadFile (TestUploadFile_OpenshellNotInPath) verifies the error path when openshell is not in PATH. The core path-correction logic (detect wrong path, move to /tmp, remove directory, move back) has no test coverage.

• [command-injection] internal/cli/run.go:415 — The excludeCmd constructs a shell command using remoteRepositoryDir without shell quoting. While remoteRepositoryDir is built from SandboxWorkspace constant + filepath.Base(hostRepositoryDir), the new shellQuote function exists and should be used here for defense in depth.

• [error-handling-idiom] internal/sandbox/sandbox.go:820 — Error messages in UploadFile use "checking for file: %s" and "fixing UploadFile path: %s, %s" which do not follow the established sandbox package pattern of "<verb> to/from sandbox %q <detail>: %w".

• [stale-doc] docs/ADRs/0024-harness-definitions.md — Contains harness YAML examples with old /tmp/workspace/ paths. As an ADR (historical record), these may be left as-is per project convention, but will be inaccurate post-merge.

• [stale-doc] docs/ADRs/0032-safe-push-wrapper-for-sandboxed-agents.md — References old /tmp/workspace/.safe-push-audit.jsonl and other old paths.

• [stale-doc] docs/superpowers/plans/2026-05-04-retro-agent.md — Contains harness config examples with old /tmp/workspace/ paths.

Info

• [resolved] Prior critical findings resolved: SandboxHooksDir in hooks.go now uses sandbox.SandboxWorkspace + "/.claude/hooks" instead of the hardcoded path, and all 8 Python security hook scripts now use /sandbox/workspace/.security/findings.jsonl. Both align with bootstrapCommon's directory creation.

Previous run (4)

Review

Findings

Critical

• [stale-reference] internal/security/hooks.go:51 — SandboxHooksDir is hardcoded to /tmp/workspace/.claude/hooks but SandboxWorkspace changed to /sandbox/workspace. The comment on line 50 explicitly states "Must match sandbox.SandboxWorkspace + /.claude/hooks" — and it no longer does. GenerateClaudeSettings() builds settings.json with hook commands pointing to this stale path, but claude.go installs the actual scripts at the new path via sandbox.SandboxWorkspace. All 8 security hooks (tirith, ssrf, canary, tool_allowlist, context_suppress, unicode, secret_redact, canary_posttool) will silently fail at runtime because Claude Code will try to execute scripts at a non-existent path.
  Remediation: Update the constant to "/sandbox/workspace/.claude/hooks" or derive it from sandbox.SandboxWorkspace.

• [stale-reference] internal/security/hooks/*.py — All 8 Python security hook scripts hardcode FINDINGS_PATH = "/tmp/workspace/.security/findings.jsonl". The .security directory is created at /sandbox/workspace/.security/ (via sandbox.SandboxWorkspace), and findings extraction in run.go reads from sandbox.SandboxWorkspace + "/.security/". These hooks will write findings to a path that doesn't exist, silently breaking the security audit trail. Affected files: tirith_check.py, ssrf_pretool.py, canary_pretool.py, tool_allowlist_pretool.py, context_suppress_posttool.py, unicode_posttool.py, secret_redact_posttool.py, canary_posttool.py.
  Remediation: Update FINDINGS_PATH in all 8 hook scripts to /sandbox/workspace/.security/findings.jsonl.

High

• [supply-chain] action.yml:117 — The new OpenShell install step pipes a remote script directly into sh via curl -LsSf ... | sh. While the URL is pinned to a commit SHA (OPENSHELL_SHA), there is no checksum verification of the downloaded script before execution. The SHA is written to GITHUB_ENV in a prior step, which means a step-injection attack modifying GITHUB_ENV could alter it. The previous install method (uv tool install from PyPI) provided package-manager-level integrity verification.
  Remediation: Download the install script to a file first, verify its SHA256 checksum against a known-good hash hardcoded in action.yml, then execute it. Alternatively, install from the .deb package with checksum verification.

Medium

• [command-injection] internal/sandbox/sandbox.go:723 — The new UploadFile function constructs shell commands using fmt.Sprintf with remotePath and filepath.Base(localPath) interpolated directly into strings passed to Exec() without shell-quoting. Current callers pass controlled paths, but the function is exported. This is a pre-existing pattern in the codebase but the new function adds more instances.

• [stale-reference] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md:151 — Still references /tmp/workspace/prior-review.txt but the review harness now places that file at /sandbox/workspace/prior-review.txt.

• [stale-doc] docs/architecture.md — Contains stale references to /tmp/workspace and /tmp/claude-config sandbox paths.

• [stale-doc] docs/plans/universal-harness-access.md — Contains multiple YAML examples with old /tmp/workspace/ paths.

• [stale-doc] docs/ADRs/0024-harness-definitions.md — Contains harness YAML examples with old /tmp/workspace/ paths throughout.

• [stale-doc] docs/superpowers/plans/2026-05-04-retro-agent.md — Contains harness config examples with old /tmp/workspace/ paths.

• [stale-doc] docs/ADRs/0032-safe-push-wrapper-for-sandboxed-agents.md — References old audit log path /tmp/workspace/.safe-push-audit.jsonl.

Low

• [dead-code] internal/sandbox/sandbox.go:406 — In UploadFile, the if exitCode == 0 check after a return on exitCode != 0 is always true and redundant.

• [test-inadequate] internal/sandbox/sandbox_test.go:381 — The only test for UploadFile verifies failure when openshell is not in PATH. The core path-correction logic has no test coverage.

• [incomplete-migration] internal/cli/run.go:923 — The .env file upload still uses sandbox.Upload() rather than sandbox.UploadFile(). This call happens before the git repo is populated so may not be affected by the OpenShell bug, but the inconsistency is worth noting.

Previous run (5)

Review

Findings

High

• [incomplete-scope] internal/security/hooks.go:51 — SandboxHooksDir is hardcoded to "/tmp/workspace/.claude/hooks" but this PR changes SandboxWorkspace to "/sandbox/workspace". The comment on line 50 says "Must match sandbox.SandboxWorkspace + '/.claude/hooks'", confirming these must stay in sync. Hook scripts are installed at sandbox.SandboxWorkspace + "/.claude/hooks" (runtime/claude.go:223,242), so after this PR they will be placed at /sandbox/workspace/.claude/hooks/, but the settings.json will reference /tmp/workspace/.claude/hooks/. Additionally, all 8 Python hook scripts in internal/security/hooks/ hardcode FINDINGS_PATH = "/tmp/workspace/.security/findings.jsonl". This will silently disable all security hooks (injection detection, SSRF protection, secret redaction, canary detection, Unicode filtering, context suppression, tool allowlisting, and Tirith policy checks).
  Remediation: Update SandboxHooksDir to sandbox.SandboxWorkspace + "/.claude/hooks" (or "/sandbox/workspace/.claude/hooks"), and update FINDINGS_PATH in all 8 Python hook scripts to /sandbox/workspace/.security/findings.jsonl.

Medium

• [command-injection] internal/sandbox/sandbox.go (UploadFile) — Constructs shell commands via fmt.Sprintf with unsanitized remotePath and filepath.Base(localPath) values, passed to Exec which runs sh -c. Commands include test -f %s, mv %s %s, and rm -r %s. The function is exported and does not enforce that paths are shell-safe. While current callers pass controlled paths built from the SandboxWorkspace constant, this replicates the pre-existing pattern from UploadDir rather than the safer exec.Command approach used by Upload. See also: [logic-error] finding at this location.
  Remediation: Shell-quote interpolated path values (e.g., single-quote wrapping with internal escaping), or pass paths as positional arguments to a script (sh -c '...' _ "$1" "$2").

• [supply-chain] action.yml — Replaces pinned uv tool install openshell==${OPENSHELL_VERSION} (PyPI package with version pin) with curl -LsSf https://raw.githubusercontent.com/NVIDIA/OpenShell/v${OPENSHELL_VERSION}/install.sh | sh, fetching an install script from raw.githubusercontent.com by version tag. Version tags can be force-pushed, and no checksum verification is performed on the script before execution.
  Remediation: Pin by commit SHA rather than version tag (e.g., raw.githubusercontent.com/NVIDIA/OpenShell/<full-sha>/install.sh), or download the script first and verify a checksum before execution.

• [logic-error] internal/sandbox/sandbox.go (UploadFile) — After the if exitCode != 0 { return ... } check for the wrongPath test, the subsequent if exitCode == 0 guard is always true (dead code). More importantly, the three-step mv/rm/mv sequence has no rollback: if the rm -r remotePath succeeds but the final mv tmpPath remotePath fails, the file is stranded at tmpPath and the original remotePath directory is deleted. See also: [command-injection] finding at this location.
  Remediation: Remove the redundant if exitCode == 0 check. Chain the three operations in a single sh -c command with && so partial failure leaves a deterministic state, or add cleanup logic.

• [stale-doc] docs/architecture.md:559-562,588 — Sandbox filesystem diagram shows old paths (/tmp/claude-config/agents/, /tmp/claude-config/skills/, /tmp/workspace/target-repo/, /tmp/workspace/output/) that are now incorrect after the sandbox path migration.
  Remediation: Update the diagram to use /sandbox/claude-config/ and /sandbox/workspace/ prefixes.

• [stale-doc] docs/plans/universal-harness-access.md:38 — References old path /tmp/workspace/.env.d/gcp-vertex.env which is now incorrect.
  Remediation: Update the dest path to /sandbox/workspace/.env.d/gcp-vertex.env.

• [stale-doc] docs/superpowers/plans/2026-05-04-retro-agent.md — Four harness configuration references to old /tmp/workspace/ paths (lines 667, 670, 673, 675) that will be incorrect after this PR.
  Remediation: Update dest paths to use /sandbox/workspace/ prefix. The credentials path (line 673) should use /tmp/ per the new convention.

Low

• [race-condition] internal/sandbox/sandbox.go (UploadFile) — Uses tmpPath := fmt.Sprintf("/tmp/%s", filepath.Base(remotePath)) as an intermediate location. If two concurrent UploadFile calls target the same sandbox with files sharing the same filepath.Base(remotePath), they will collide on the shared intermediate path.
  Remediation: Use a unique temporary path (e.g., include a random suffix or use mktemp inside the sandbox).

• [test-adequacy] internal/sandbox/sandbox_test.go — TestUploadFile_OpenshellNotInPath only verifies the error path when openshell is not in PATH. The core fix-up logic (the mv/rm/mv sequence, correct behavior when openshell places the file at the wrong path) is untested.
  Remediation: Add tests that verify: (1) success when file lands at remotePath, (2) correction when file lands at remotePath/basename, (3) error when file is at neither location.

• [error-handling-idiom] internal/sandbox/sandbox.go (UploadFile) — Error messages use "fixing UploadFile path: %s, %s" and "checking for file: %s" which do not follow the established sandbox package pattern of "<verb> to/from sandbox %q <detail>: %w".
  Remediation: Align error messages with the package convention.

• [stale-doc] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md:151 — References /tmp/workspace/prior-review.txt which should be /sandbox/workspace/prior-review.txt.

• [stale-doc] docs/ADRs/0032-safe-push-wrapper-for-sandboxed-agents.md — Three references to /tmp/workspace/ (lines 73, 101, 145). As an ADR (historical record), these may be left as-is per project convention, but will be inaccurate post-merge.

Previous run (6)

Review

Findings

Medium

• [command-injection] internal/sandbox/sandbox.go (UploadFile) — Constructs shell commands via fmt.Sprintf with unsanitized remotePath and filepath.Base(localPath) values, passed to Exec which runs sh -c. Commands include test -f %s, mv %s %s, and rm -r %s. The function is exported and does not enforce that paths are shell-safe. While current callers pass controlled paths built from the SandboxWorkspace constant and the commands execute inside a sandbox, this replicates the pre-existing pattern from UploadDir rather than the safer exec.Command approach used by Upload.
  Remediation: Shell-quote interpolated path values (e.g., single-quote wrapping with internal escaping), or pass paths as positional arguments to a script (sh -c '...' _ "$1" "$2").

• [supply-chain] action.yml — Replaces pinned uv tool install openshell==${OPENSHELL_VERSION} (PyPI package with version pin) with curl -LsSf https://raw.githubusercontent.com/NVIDIA/OpenShell/v${OPENSHELL_VERSION}/install.sh | sh, fetching an install script from raw.githubusercontent.com by version tag. Version tags can be force-pushed, and no checksum verification is performed on the script before execution.
  Remediation: Pin by commit SHA rather than version tag (e.g., raw.githubusercontent.com/NVIDIA/OpenShell/<full-sha>/install.sh), or download the script first and verify a checksum before execution.

• [stale-doc] docs/plans/universal-harness-access.md:38 — References old path /tmp/workspace/.env.d/gcp-vertex.env which is now incorrect after this PR changes the sandbox workspace to /sandbox/workspace.
  Remediation: Update the dest path to /sandbox/workspace/.env.d/gcp-vertex.env.

• [stale-doc] docs/superpowers/plans/2026-05-04-retro-agent.md — Four harness configuration references to old /tmp/workspace/ paths (lines 667, 670, 673, 675) that will be incorrect after this PR.
  Remediation: Update dest paths to use /sandbox/workspace/ prefix. The credentials path (line 673) should use /tmp/ per the new convention.

Low

• [race-condition] internal/sandbox/sandbox.go (UploadFile) — Uses tmpPath := fmt.Sprintf("/tmp/%s", filepath.Base(remotePath)) as an intermediate location. If two concurrent UploadFile calls target the same sandbox with files sharing the same filepath.Base(remotePath), they will collide on the shared intermediate path.
  Remediation: Use a unique temporary path (e.g., include a random suffix or use mktemp inside the sandbox).

• [test-adequacy] internal/sandbox/sandbox_test.go — TestUploadFile_OpenshellNotInPath only verifies the error path when openshell is not in PATH. The core fix-up logic (the mv/rm/mv sequence, correct behavior when openshell places the file at the wrong path) is untested.
  Remediation: Add tests that verify: (1) success when file lands at remotePath, (2) correction when file lands at remotePath/basename, (3) error when file is at neither location.

• [code-organization] internal/sandbox/sandbox.go (UploadFile) — Contains a commented-out debug line (// fmt.Fprintf(os.Stderr, "DEBUG: Uploading %s to %s\n", ...)). The codebase does not leave commented debug code in production.
  Remediation: Remove the commented-out debug line.

• [error-handling-idiom] internal/sandbox/sandbox.go (UploadFile) — Error messages use "fixing UploadFile path: %s, %s" which does not follow the established sandbox package pattern of "<verb> to/from sandbox %q <detail>: %w".
  Remediation: Align error messages with the package convention.

• [stale-doc] docs/ADRs/0032-safe-push-wrapper-for-sandboxed-agents.md — Three references to /tmp/workspace/ (lines 73, 101, 145). As an ADR (historical record), these may be left as-is per project convention, but will be inaccurate post-merge.

• [stale-doc] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md:151 — References /tmp/workspace/prior-review.txt which should be /sandbox/workspace/prior-review.txt.

Previous run (7)

Review

Findings

Medium

• [command-injection] internal/sandbox/sandbox.go (UploadFile) — Constructs shell commands via fmt.Sprintf with unsanitized remotePath and filepath.Base(localPath) values, passed to Exec which runs sh -c. Commands include test -f %s, mv %s %s, and rm -r %s. The function is exported and does not enforce that paths are shell-safe. While current callers pass controlled paths built from the SandboxWorkspace constant and the commands execute inside a sandbox, this replicates the pre-existing pattern from UploadDir rather than the safer exec.Command approach used by Upload.
  Remediation: Shell-quote interpolated path values (e.g., single-quote wrapping with internal escaping), or pass paths as positional arguments to a script (sh -c '...' _ "$1" "$2").

• [supply-chain] action.yml — Replaces pinned uv tool install openshell==${OPENSHELL_VERSION} (PyPI package with version pin) with curl -LsSf https://raw.githubusercontent.com/NVIDIA/OpenShell/v${OPENSHELL_VERSION}/install.sh | sh, fetching an install script from raw.githubusercontent.com by version tag. Version tags can be force-pushed, and no checksum verification is performed on the script before execution.
  Remediation: Pin by commit SHA rather than version tag (e.g., raw.githubusercontent.com/NVIDIA/OpenShell/<full-sha>/install.sh), or download the script first and verify a checksum before execution.

• [stale-doc] docs/plans/universal-harness-access.md:38 — References old path /tmp/workspace/.env.d/gcp-vertex.env which is now incorrect after this PR changes the sandbox workspace to /sandbox/workspace.
  Remediation: Update the dest path to /sandbox/workspace/.env.d/gcp-vertex.env.

• [stale-doc] docs/superpowers/plans/2026-05-04-retro-agent.md — Four harness configuration references to old /tmp/workspace/ paths (lines 667, 670, 673, 675) that will be incorrect after this PR.
  Remediation: Update dest paths to use /sandbox/workspace/ prefix. The credentials path (line 673) should use /tmp/ per the new convention.

Low

• [race-condition] internal/sandbox/sandbox.go (UploadFile) — Uses tmpPath := fmt.Sprintf("/tmp/%s", filepath.Base(remotePath)) as an intermediate location. If two concurrent UploadFile calls target the same sandbox with files sharing the same filepath.Base(remotePath), they will collide on the shared intermediate path.
  Remediation: Use a unique temporary path (e.g., include a random suffix or use mktemp inside the sandbox).

• [test-adequacy] internal/sandbox/sandbox_test.go — TestUploadFile_OpenshellNotInPath only verifies the error path when openshell is not in PATH. The core fix-up logic (the mv/rm/mv sequence, correct behavior when openshell places the file at the wrong path) is untested.
  Remediation: Add tests that verify: (1) success when file lands at remotePath, (2) correction when file lands at remotePath/basename, (3) error when file is at neither location.

• [code-organization] internal/sandbox/sandbox.go (UploadFile) — Contains a commented-out debug line (// fmt.Fprintf(os.Stderr, "DEBUG: Uploading %s to %s\n", ...)). The codebase does not leave commented debug code in production.
  Remediation: Remove the commented-out debug line.

• [error-handling-idiom] internal/sandbox/sandbox.go (UploadFile) — Error messages use "fixing UploadFile path: %s, %s" which does not follow the established sandbox package pattern of "<verb> to/from sandbox %q <detail>: %w".
  Remediation: Align error messages with the package convention.

• [stale-doc] docs/ADRs/0032-safe-push-wrapper-for-sandboxed-agents.md — Three references to /tmp/workspace/ (lines 73, 101, 145). As an ADR (historical record), these may be left as-is per project convention, but will be inaccurate post-merge.

• [stale-doc] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md:151 — References /tmp/workspace/prior-review.txt which should be /sandbox/workspace/prior-review.txt.

Previous run (8)

Review

Findings

High

• [test-integrity] internal/sandbox/sandbox_test.go:24 — TestConstants asserts SandboxWorkspace == "/tmp/workspace" and SandboxClaudeConfig == "/tmp/claude-config", but this PR changes those constants to /sandbox/workspace and /sandbox/claude-config. The test is not updated and will fail in CI.
  Remediation: Update TestConstants to assert the new values: assert.Equal(t, "/sandbox/workspace", SandboxWorkspace) and assert.Equal(t, "/sandbox/claude-config", SandboxClaudeConfig).

Medium

• [edge-case] internal/sandbox/sandbox.go (UploadFile function) — UploadFile returns nil (success) when the uploaded file is found at neither remotePath nor remotePath/filepath.Base(localPath) after Upload succeeds. The caller will believe the upload succeeded when the file may be missing. See also: [command-injection] finding at this location.
  Remediation: Return an error when the file cannot be found at either expected location after a successful Upload, e.g., return fmt.Errorf("UploadFile: file not found at %s or %s after upload", remotePath, wrongPath).

• [command-injection] internal/sandbox/sandbox.go (UploadFile function) — UploadFile constructs shell commands via fmt.Sprintf with unsanitized remotePath and filepath.Base(localPath) values, passed to Exec which runs sh -c <command>. Commands include test -f %s, mv %s %s, and rm -r %s. The function is exported and does not document or enforce that paths must be shell-safe. While current callers pass controlled paths (from the SandboxWorkspace constant) and the commands execute inside a sandbox, this replicates a pre-existing pattern from UploadDir rather than following the safer exec.Command approach used by Upload. See also: [edge-case] finding at this location.
  Remediation: Shell-quote interpolated path values (e.g., single-quote wrapping with internal escaping), or pass paths as positional arguments to a script (sh -c '...' _ "$1" "$2").

• [supply-chain] action.yml — Replaces pinned uv tool install openshell==${OPENSHELL_VERSION} (PyPI package with version pin) with curl -LsSf https://raw.githubusercontent.com/NVIDIA/OpenShell/v${OPENSHELL_VERSION}/install.sh | sh, fetching an install script from raw.githubusercontent.com by version tag. Version tags can be force-pushed, and no checksum verification is performed on the script before execution.
  Remediation: Pin by commit SHA rather than version tag (e.g., raw.githubusercontent.com/NVIDIA/OpenShell/<full-sha>/install.sh), or download the script first and verify a checksum before execution.

• [stale-doc] docs/plans/universal-harness-access.md:38 — Harness configuration example references /tmp/workspace/.env.d/gcp-vertex.env which is now incorrect after this PR changes the sandbox workspace to /sandbox/workspace.
  Remediation: Update the dest path to /sandbox/workspace/.env.d/gcp-vertex.env.

• [stale-doc] docs/superpowers/plans/2026-05-04-retro-agent.md — Four harness configuration references to old /tmp/workspace/ paths (lines 667, 670, 673, 675) that will be incorrect after this PR.
  Remediation: Update dest paths to use /sandbox/workspace/ prefix. Note: the credentials path should use /tmp/ per the new convention (line 673).

Low

• [race-condition] internal/sandbox/sandbox.go (UploadFile function) — The fix-up sequence uses tmpPath := fmt.Sprintf("/tmp/%s", filepath.Base(remotePath)) as an intermediate location. If two concurrent UploadFile calls target the same sandbox with files that have the same filepath.Base(remotePath), they will collide on the shared intermediate path.
  Remediation: Use a unique temporary path (e.g., include a random suffix or use mktemp inside the sandbox).

• [test-adequacy] internal/sandbox/sandbox_test.go — The new TestUploadFile_OpenshellNotInPath only verifies that UploadFile returns an error when openshell is not in PATH. The core fix-up logic (the mv/rm/mv sequence, the silent nil return, correct behavior when openshell places the file at the wrong path) is untested.
  Remediation: Add tests that verify: (1) success when file lands at remotePath, (2) correction when file lands at remotePath/basename, (3) error when file is at neither location.

• [code-organization] internal/sandbox/sandbox.go (UploadFile function) — Contains a commented-out debug line (// fmt.Fprintf(os.Stderr, "DEBUG: Uploading %s to %s\n", ...)). The codebase does not leave commented debug code in production.
  Remediation: Remove the commented-out debug line.

• [error-handling-idiom] internal/sandbox/sandbox.go (UploadFile function) — Error messages use "error while fixing UploadFile path: %s, %s" which does not follow the established sandbox package pattern of "<verb> to/from sandbox %q <detail>: %w".
  Remediation: Align error messages with the package convention, e.g., "moving file in sandbox %q: %s %s".

• [stale-doc] docs/ADRs/0032-safe-push-wrapper-for-sandboxed-agents.md — Three references to /tmp/workspace/ (lines 73, 101, 145). As an ADR (historical record), these may be left as-is per project convention, but will be inaccurate post-merge.

• [stale-doc] internal/scaffold/fullsend-repo/skills/fix-review/SKILL.md:139 — References REVIEW_BODY_FILE="/tmp/workspace/review-body.txt" which should be /sandbox/workspace/review-body.txt.

• [stale-doc] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md:151 — References /tmp/workspace/prior-review.txt which should be /sandbox/workspace/prior-review.txt.

Previous run (9)

Review

Findings

High

• [test-integrity] internal/sandbox/sandbox_test.go:23 — TestConstants asserts SandboxWorkspace == "/tmp/workspace" and SandboxClaudeConfig == "/tmp/claude-config", but this PR changes those constants to /sandbox/workspace and /sandbox/claude-config. The test is not updated in this PR and will fail in CI.
  Remediation: Update TestConstants to assert the new values: assert.Equal(t, "/sandbox/workspace", SandboxWorkspace) and assert.Equal(t, "/sandbox/claude-config", SandboxClaudeConfig).

• [stale-doc] docs/guides/user/running-agents-locally.md — Six references to OpenShell 0.0.38 (lines 14, 56, 61, 65, 135, 299) that will be incorrect after this PR upgrades to 0.0.54. Users following this guide will install the wrong version and use outdated setup instructions.
  Remediation: Update all version references to 0.0.54 and revise the installation instructions to match the new deb-based install method. The v0.0.38-specific troubleshooting note at line 299 should be verified against 0.0.54 behavior.

Medium

• [command-injection] internal/sandbox/sandbox.go (UploadFile function) — The new UploadFile constructs shell commands via fmt.Sprintf with unsanitized remotePath and filepath.Base(localPath) values, passed to Exec which runs sh -c <command>. While current callers pass controlled paths (from the SandboxWorkspace constant), the function is exported and its contract does not restrict inputs. In contrast, the existing Upload function uses exec.Command with separate argv entries (safe). See also: [edge-case] finding at this location.
  Remediation: Shell-quote interpolated path values (e.g., single-quote wrapping with internal escaping), or restructure fix-up commands to avoid sh -c string interpolation.

• [supply-chain] action.yml — Replaces the pinned uv tool install openshell==${OPENSHELL_VERSION} with curl -LsSf ... | sh, fetching an install script from raw.githubusercontent.com by version tag. Version tags can be force-pushed, and no checksum verification is performed on the script before execution.
  Remediation: Pin by commit SHA rather than version tag, or verify a checksum/signature of the downloaded script before piping to sh.

• [stale-doc] docs/guides/user/customizing-agents.md, docs/guides/dev/cli-internals.md, docs/architecture.md — Multiple user-facing documentation files reference /tmp/workspace and /tmp/claude-config paths that this PR changes to /sandbox/workspace and /sandbox/claude-config. These docs will actively mislead readers after this change (e.g., customizing-agents.md shows harness YAML examples with old dest: paths; cli-internals.md documents the old constant values explicitly).
  Remediation: Update path references in these documentation files. ADR references (0024, 0032) may be left as-is since they are historical records.

Low

• [edge-case] internal/sandbox/sandbox.go (UploadFile function) — UploadFile returns nil (success) if the uploaded file is found at neither remotePath nor remotePath/filepath.Base(localPath) after Upload succeeds. The caller will believe the upload succeeded when the file may be missing. See also: [command-injection] finding at this location.
  Remediation: Return an error when the file cannot be found at either the expected or the known-wrong location after a successful Upload call.

• [code-organization] internal/sandbox/sandbox.go (UploadFile function) — Contains a commented-out debug line (// fmt.Fprintf(os.Stderr, "DEBUG: Uploading %s to %s\n", ...)). The codebase does not leave commented debug code in production; debug output is gated behind environment variables or flags.
  Remediation: Remove the commented-out debug line.

• [error-handling-idiom] internal/sandbox/sandbox.go (UploadFile function) — Error messages use the format "error while fixing UploadFile path: %s, %s" which does not follow the established sandbox package pattern of "<verb> to sandbox %q <detail>: %w".
  Remediation: Align error messages with the existing pattern, e.g., "fixing upload path in sandbox %q: %s %s".

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:05 PM UTC · Completed 3:15 PM UTC
Commit: d0ac11b · View workflow run →

[waynesun09]

Review Squad Report — Round 2 (7 agents, MEDIUM+ inline)

Round 1 fixes confirmed: SandboxHooksDir, Python hooks FINDINGS_PATH, shell injection, dead code, SHA pinning, pr-review SKILL.md, test updates, architecture diagrams — all resolved.

Remaining findings (10 MEDIUM+):

Sev	Finding	Agents
CRITICAL	UploadFile tmpPath collision when remotePath under /tmp/ — breaks GCP credential upload	5/7
HIGH	requirements table still shows 0.0.38	4/7
HIGH	$OPENSHELL_VERSION undefined in user guide — 404s	5/7
MEDIUM	harness_test.go 9 stale /tmp/workspace paths	2/7
MEDIUM	cli-internals.md diagram stale path + broken ASCII alignment	3/7
MEDIUM	Stale troubleshooting refs to removed "step 5" and 0.0.38	3/7
MEDIUM	UploadFile mv/rm timeouts 1s — too tight for CI	3/7
MEDIUM	UploadFile fix-up logic has no test coverage	5/7
MEDIUM	Env file comments reference stale /tmp/workspace/bin	2/7
MEDIUM	sandbox_test.go UploadDir test uses old path	2/7

False positive confirmed: Heredoc indentation in action.yml is NOT an issue — YAML | block scalar strips common indentation before shell execution (verified by 2 agents).

The tmpPath collision is the only merge-blocker — it silently breaks credential injection for all 6 default agents when OpenShell #1740 triggers.

[waynesun09]

Correction on the CRITICAL tmpPath collision finding:

After tracing the execution order more carefully, the tmpPath collision for /tmp/.gcp-credentials.json is a false positive in the current code:

1. OpenShell bug fix(#1738): skip dispatch for offboard when FULLSEND_MINT_URL is unset #1740 only triggers "in a git environment" — when a git repo exists at the upload destination in the sandbox
2. bootstrapEnv (which uploads GCP creds to /tmp/.gcp-credentials.json) runs before step 8 (UploadDir copies the git repo into the sandbox)
3. At credential upload time, there's no git repo in the sandbox → bug fix(#1738): skip dispatch for offboard when FULLSEND_MINT_URL is unset #1740 doesn't fire → the fix-up path is never entered → no collision

The UploadFile workaround matters for files uploaded after the git repo exists (AGENTS.md injection, OIDC refresh, schema upload) — those all target /sandbox/workspace/, where tmpPath would be /tmp/<basename> with no collision.

Downgrading to LOW — the collision is theoretically possible if a future caller passes a /tmp/ path to UploadFile in a post-repo-copy context, but it doesn't happen today. A defensive fix (unique tmpPath) is still nice-to-have but not a merge blocker.

The remaining actionable items are the docs issues (version table, undefined $OPENSHELL_VERSION, stale troubleshooting).

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:23 AM UTC · Completed 6:36 AM UTC
Commit: d0ac11b · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ❌ Failure · Started 6:42 AM UTC · Completed 6:43 AM UTC
Commit: d0ac11b · View workflow run →

[rh-hemartin]

/fs-retro

[fullsend-ai-retro]

🤖 Finished Retro · ❌ Failure · Started 6:49 AM UTC · Completed 6:49 AM UTC
Commit: 9bd7234 · View workflow run →

[rh-hemartin]

/fs-retro

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 6:56 AM UTC · Completed 7:04 AM UTC
Commit: ba204cb · View workflow run →

[fullsend-ai-retro]

Retro Analysis: PR #1887 — OpenShell 0.0.54 Upgrade

Timeline

This was a large (35 files, 244+/284-) human-authored infrastructure PR by rh-hemartin upgrading OpenShell to 0.0.54 and fixing sandbox integration. It took 5 days from open to merge (Jun 4–9), with significant review friction.

• 5 CHANGES_REQUESTED verdicts from the review bot across multiple review rounds
• CRITICAL false positive: Both the review bot and 5/7 Review Squad agents flagged a tmpPath collision in UploadFile as CRITICAL. Human reviewer waynesun09 traced the execution order and determined it was a false positive — bootstrapEnv runs before the sandbox git copy, so the collision scenario is impossible.
• Explicit author dismissals ignored: rh-hemartin responded "Not going to fix" to several findings (test coverage for UploadFile, command injection in pre-existing UploadDir, shell quoting in run.go), but subsequent review rounds re-raised similar findings.
• Retro agent failed on its first two runs due to missing pull_requests:write permission (tracked in Retro GitHub App missing pull_requests:write — all PR retros failing since Jun 4 #1950).

Review Quality Assessment

Positives: The review bot caught real issues early (hardcoded /tmp/workspace in SandboxHooksDir, Python hooks FINDINGS_PATH not updated) that were fixed. Human reviewers (maruiz93, waynesun09) added valuable context — maruiz93 identified that heredoc indentation was a non-issue, waynesun09 correctly downgraded the CRITICAL to LOW.

Negatives: The review bot generated excessive noise — 5 CHANGES_REQUESTED rounds on a single-commit PR where the author was actively engaging. The CRITICAL false positive (tmpPath collision) required human intervention to resolve. The bot re-raised findings the author had explicitly deferred.

Existing Issue Coverage

All identified friction points are well-covered by existing open issues:

Problem	Existing Issues
Author dismissals not honored	#1672
Redundant review iterations	#1370, #1582
Dedup findings across iterations	#1013
COMMENT verdict for low-severity re-reviews	#2029
Severity capping for unverifiable assumptions	#1928
Incorporating human reviews	#664
Verify facts before asserting	#1835
Retro agent permission failure	#1950

No new proposals are warranted — implementing the existing backlog (particularly #1672, #2029, and #1013) would have substantially reduced the friction observed on this PR.