Replace SSH/SCP/rsync with OpenShell native CLI commands

[maruiz93]

Summary

• Replace all exec.Command SSH/SCP/rsync wrappers in internal/sandbox/ with OpenShell native CLI commands (sandbox exec, sandbox upload, sandbox download) that use gRPC internally
• Add os.Root kernel-enforced path containment for local writes in ExtractTranscripts and ExtractOutputFiles, replacing manual filepath.Clean + HasPrefix checks
• Add sanitizeDownload post-download cleanup (symlink removal + .git/hooks/ deletion) to replace rsync --no-links and --exclude flags
• Remove sshConfigPath parameter from all function signatures — no more SSH config temp file creation/cleanup
• Add DownloadFile helper for single-file downloads (openshell always treats destination as directory)
• Use POSIX . instead of bash source since openshell exec runs via sh -c
• Remove dead ExecStream function (unused since progress tracking replaced it)

Closes #261

Test plan

• Unit tests pass (make go-test)
• go vet clean (make go-vet)
• Lint passes (make lint)
• Integration tested with fullsend run hello-world against live sandbox — bootstrap, agent execution with streaming, output extraction, transcript extraction, and repo extraction all verified working
• Validation passes on iteration 2 (iteration 1 is harness deliberate-retry test)

🤖 Generated with Claude Code

[github-actions]

fullsend review is working on this — view logs

[github-actions]

Site preview

Preview: https://f1805264-site.fullsend-ai.workers.dev

Commit: 79b476b5d0c56c778042a2af729be7712f680efc

[fullsend-ai-review]

Review: #761

Head SHA: 79b476b
Timestamp: 2026-05-08T00:00:00Z
Outcome: comment-only

Summary

This PR replaces all SSH/SCP/rsync exec.Command wrappers in the sandbox package with OpenShell native CLI commands (sandbox exec, sandbox upload, sandbox download), adds os.Root kernel-enforced path containment for local writes, and introduces a sanitizeDownload post-download cleanup function. The change is well-structured, aligns with issue #261, and the security improvements (os.Root, sanitizeDownload) are genuine upgrades over the previous filepath.Clean + HasPrefix pattern. The findings below are non-blocking observations about subtle behavior changes and minor robustness concerns.

Findings

Medium

• [Correctness] internal/sandbox/sandbox.go:Exec — The Exec function removes the Go-context-based timeout error detection that the old SSH function had (if runErr != nil && ctx.Err() != nil). The new code relies solely on openshell's --timeout flag returning exit code 124. If the Go context fires as a safety net (timeout+10s), cmd.Run() returns an error but ProcessState is non-nil (process was killed), so the function falls through to return ..., nil — silently returning a nil error despite the process being killed by context cancellation. Callers that only check err != nil (not exitCode) would miss this failure. In practice most callers also check exitCode, so the impact is limited, but the old behavior was more defensive.
  Remediation: Consider re-adding a ctx.Err() check after the exit-code-124 check, e.g., if runErr != nil && ctx.Err() != nil { return ..., fmt.Errorf("openshell exec timed out (context deadline): %w", ctx.Err()) }.

• [Correctness] internal/sandbox/sandbox.go:DownloadFile — DownloadFile calls os.Remove(downloadedPath) before downloading, presumably to clean up stale files. However, if the download itself fails partway, any pre-existing file at that path is already deleted. This is a minor data-loss risk during partial failures. Also, there's no check that the download actually produced downloadedPath — if openshell creates the file with a different name than filepath.Base(remotePath), the rename will fail with a confusing error.
  Remediation: Consider verifying downloadedPath exists after download before attempting rename.

Low

• [Correctness] internal/sandbox/sandbox.go:ExtractTranscripts and ExtractOutputFiles — Both functions use a "probe file" pattern: create a file via root.Create() to validate the path, immediately close it, then os.Remove() it, then DownloadFile() the actual content. This introduces a TOCTOU window between the os.Remove and the DownloadFile. The window is very small and unlikely to be exploited in practice (sandbox extraction runs in a controlled environment), but it's worth noting since the stated goal of os.Root is to eliminate TOCTOU. A cleaner pattern would be to download to a temp file and then use os.Root to place it.
  Remediation: Acceptable as-is given the controlled execution environment, but document the TOCTOU limitation.

• [Style/conventions] internal/sandbox/sandbox.go:sanitizeDownload — The function checks .git/hooks by matching d.Name() == "hooks" && filepath.Base(filepath.Dir(path)) == ".git". The plan document uses rel == filepath.Join(".git", "hooks") which is more explicit but would miss submodule hooks. The implementation's approach also matches submodule hooks (e.g., vendor/dep/.git/hooks/), which is good and tested. This is actually an improvement over the plan — just noting the divergence.

• [Correctness] internal/sandbox/sandbox.go:ExecStreamReader — This function sets both a Go context timeout AND passes --timeout to openshell. Unlike Exec (which adds a 10s buffer), ExecStreamReader uses the same timeout value for both. If openshell's internal timeout and Go's context fire at roughly the same time, the behavior is race-y. The caller (runAgentWithProgress) handles this by checking cmd.ProcessState and exit codes, so it's likely fine in practice.
  Remediation: Consider adding the same 10s buffer as Exec uses for consistency.

Info

• [Intent alignment] The PR body claims "Remove dead ExecStream function" — confirmed: the old SSHStream function had zero callers in the codebase. Its removal is correct.

• [Intent alignment] The source to . change in buildClaudeCommand and buildScanContextCommand is a correct POSIX compatibility fix since openshell exec runs commands via sh -c, and source is a bash-ism not available in POSIX sh.

• [Intent alignment] Scope matches issue Replace exec.Command SSH/SCP wrappers with Go-native libraries #261 authorization. The PR adds ~1100 lines of documentation (plan + spec) which is appropriate for a change of this complexity.

Footer

Outcome: comment-only
This review applies to SHA 79b476b5d0c56c778042a2af729be7712f680efc. Any push to the PR head clears this review and requires a new evaluation.

Previous run

Review: #761

Head SHA: 70d79fd
Timestamp: 2026-05-08T00:00:00Z
Outcome: approve

Summary

Clean transport-layer migration from SSH/SCP/rsync exec.Command wrappers to OpenShell native CLI commands. The change is well-structured: security posture improves with os.Root kernel-enforced path containment (replacing TOCTOU-vulnerable filepath.Clean + HasPrefix), sanitizeDownload provides defense-in-depth for symlinks and .git/hooks/, and SSH config temp files are eliminated entirely. All call sites migrated consistently, dead code removed, tests are thorough. Two minor timeout edge cases noted below.

Findings

Medium

• [merge-readiness] internal/cli/run.go — The main branch contains OIDC token refresh code (refreshOIDCToken, runOIDCRefresh) added after this branch diverged. That code calls sandbox.SCP() which this PR removes. Merging will either produce a git conflict or, if auto-resolved, a build break. The OIDC functions must be migrated to sandbox.Upload() during merge conflict resolution.

• [correctness] internal/sandbox/sandbox.go:188-221 — Exec() sets a Go context timeout of timeout+10s and relies on openshell's --timeout flag (exit code 124) for timeout detection. If openshell's timeout mechanism fails and the Go context fires instead, cmd.Run() returns an error but ProcessState is set (process was signal-killed), so the "failed to start" check is skipped. Exit code is -1 (not 124), so the timeout check is also skipped. The function returns nil error with exit code -1 — a silent timeout. The old SSH() explicitly checked ctx.Err(). Consider adding a fallback: if runErr != nil && ctx.Err() != nil { return ..., fmt.Errorf("command timed out...") }.

Low

• [correctness] internal/sandbox/sandbox.go:226-227 — ExecStreamReader() sets ctx.WithTimeout(timeout) without the +10s buffer that Exec() uses. The Go context and openshell --timeout could race. Low severity because this path handles the main agent execution with long timeouts (30-60 min).

Info

• [style] sanitizeDownload implementation improved on the plan spec — the plan checks rel == filepath.Join(".git", "hooks") while the implementation uses d.Name() == "hooks" && filepath.Base(filepath.Dir(path)) == ".git", which correctly catches submodule .git/hooks/ directories. TestSanitizeDownload_RemovesSubmoduleGitHooks validates this.

• [test-coverage] Tests are well-structured: 5 sanitizeDownload cases (symlinks, git hooks, nested, submodules, empty), os.Root containment validation, and Exec error path. Integration validation appropriately deferred to e2e tests.

Footer

Outcome: approve
This review applies to SHA 70d79fdd9f1b5b34e127c47b7cc9f9568e7f0b45. Any push to the PR head clears this review and requires a new evaluation.

Previous run (2)

Review: #761

Head SHA: 36aa646
Timestamp: 2026-05-08T00:00:00Z
Outcome: comment-only

Summary

This PR cleanly replaces SSH/SCP/rsync subprocess wrappers with OpenShell native CLI commands and upgrades path traversal containment from manual filepath.Clean+HasPrefix checks to kernel-enforced os.Root. The migration is well-executed with consistent API renaming, comprehensive test coverage for sanitizeDownload, and appropriate os.Root containment tests. Several medium/low findings are noted below — none blocking, but worth considering for defense-in-depth.

Findings

Medium

• [Correctness] internal/sandbox/sandbox.go — Exec() lacks client-side timeout
  The old SSH() used exec.CommandContext(ctx, ...) with context.WithTimeout as a Go-side safety net. The new Exec() uses bare exec.Command (no context) and relies solely on openshell's --timeout flag. If the openshell process itself hangs (e.g., gRPC connection stalls before the server-side timeout can engage), the Go process blocks indefinitely. ExecStreamReader, Upload, and Download all correctly preserve client-side timeouts via exec.CommandContext. Exec() is the only function that dropped this defense-in-depth layer.
  Remediation: Wrap Exec() with exec.CommandContext using the same timeout duration, keeping the --timeout flag as the primary mechanism and the context as a fallback.

• [Correctness] internal/sandbox/sandbox.go — sanitizeDownload only matches .git/hooks at repo root
  The check rel == filepath.Join(".git", "hooks") only matches .git/hooks relative to the download root. The old rsync --exclude .git/hooks/ matched at any depth, including in submodules (e.g., vendor/dep/.git/hooks/). While submodule hooks are an unlikely attack vector for this use case, the behavioral difference narrows the safety surface.
  Remediation: Consider using filepath.Base(rel) == "hooks" && filepath.Dir(rel) ends with .git or walk-based pattern matching to catch nested .git/hooks/ directories.

Low

• [Correctness] internal/sandbox/sandbox.go — sanitizeDownload silently ignores .git/hooks/ removal error
  os.RemoveAll(path) error is discarded. If removal fails (e.g., permission denied), the hooks directory survives but the walk skips it via filepath.SkipDir, so neither the caller nor any log sees the failure.
  Remediation: Return the error or log it: if err := os.RemoveAll(path); err != nil { return fmt.Errorf("removing .git/hooks: %w", err) }

Info

• [Style] docs/superpowers/plans/2026-05-06-openshell-native-sandbox-transport.md — Plan document contains developer-specific local paths
  Multiple references to /home/manon/Workspace/fullsend/fullsend--261-go-native-ssh leak the author's username and workspace layout. These are benign but could be parameterized or use generic placeholders.

• [Style] internal/sandbox/sandbox.go — os.Root.Create() + immediate os.Remove() pattern is non-obvious
  In ExtractTranscripts and ExtractOutputFiles, files are created via root.Create() for path validation, immediately closed, then removed, then re-created by DownloadFile. A brief comment explaining this is a validation-only step (not actual file creation) would help future readers.

Footer

Outcome: comment-only
This review applies to SHA 36aa646774a9f2c02c54204a206b251ee9d12e86. Any push to the PR head clears this review and requires a new evaluation.

[rh-hemartin]

Tested locally and LGTM

[github-actions]

fullsend review is working on this — view logs

[github-actions]

fullsend review is working on this — view logs

[github-actions]

fullsend review is working on this — view logs

[ralphbean]

Clean, thorough transport migration. Security properties are preserved or improved (os.Root kernel-enforced containment is strictly stronger than filepath.Clean+HasPrefix). One minor inconsistency noted inline — not blocking.

[ralphbean]

[moderate] Exec() adds +10*time.Second to the Go context timeout as defense-in-depth against openshell process hangs, but ExecStreamReader() uses the raw timeout with no buffer. Since ExecStreamReader is used for the main long-running agent session, the same defense-in-depth rationale applies — if openshell's --timeout fails to kill the child, the Go context fires at the exact same moment.

Non-blocking, but worth aligning in a follow-up:

ctx, cancel := context.WithTimeout(context.Background(), timeout+10*time.Second)