fix(sandbox): use tarball upload to preserve symlinks

[rh-hemartin]

Closes #1133

Summary

• openshell sandbox upload dereferences symlinks into real directories when building the tar archive — confirmed on v0.0.36 and v0.0.42, filed upstream as NVIDIA/OpenShell#1425
• This causes git status inside the sandbox to report all git-tracked symlinks as deleted:, producing spurious noise and misleading agents into attempting to restore files that were never deleted
• Replace the previous RestoreSymlinks post-processing workaround with UploadDir: build a local tar archive (GNU tar preserves symlinks by default), upload it, and extract in the sandbox — one step, no git dependency, covers untracked symlinks too
• Also eliminates the separate .git upload step since the tarball includes everything

How it works

UploadDir creates a temp tarball with tar -czf (symlinks are preserved by default — only -h/--dereference would follow them), uploads it to /tmp/fs-upload-<sandbox>.tar.gz, then execs mkdir -p <dest> && tar -xzf ... && rm in the sandbox.

Test plan

• Reproduced bug with openshell v0.0.36: find -type l returns empty, link-dir shows as drwxr-xr-x instead of symlink
• Verified fix in live sandbox: find -type l returns /sandbox/repo/link-dir, ls -la shows lrwxrwxrwx -> real-dir, git status clean

🤖 Generated with Claude Code

[rh-hemartin]

Not sure about this approach yet, I need to test it locally.

[github-actions]

Site preview

Preview: https://74b08680-site.fullsend-ai.workers.dev

Commit: d27437f59e360dba721bcb050c74bf4786676d82

[fullsend-ai-review]

Review

Findings

Low

• [documentation-currency] docs/guides/dev/cli-internals.md:317 — The sandbox function table lists Upload() but does not include the new UploadDir() function. The "Copy source code" step in the flow diagram (line 248) also describes the upload generically without noting the tarball-based approach. Consider adding UploadDir() to the table with a description like: tar -czf + Upload + Exec extract | Copy directory preserving symlinks.
  Remediation: Add an UploadDir() row to the function table between Upload() and Download().

• [test-adequacy] internal/sandbox/sandbox_test.go:248 — TestUploadDir_OpenshellNotInPath only exercises the error path where tar fails because PATH is empty. The happy path (tarball creation, upload, extraction) is not unit-tested. This is understandable given the function shells out to tar and openshell, and the PR description confirms manual live-sandbox verification. Consider adding a test that at least verifies tarball creation (e.g., create a temp dir with a symlink, run the tar step, verify the archive contains the symlink entry).
  Remediation: Add a unit test that creates a directory with a symlink, builds the tarball, and inspects the archive to confirm symlinks are preserved.

Info

• [style] internal/sandbox/sandbox.go:295 — The extractCmd string is built with fmt.Sprintf without shell-escaping remotePath or remoteTar. This is consistent with every other Exec() call site in the codebase and the values are internally constructed (not user-supplied), so it is safe in practice. Noting for awareness — if UploadDir is ever called with user-controlled paths, this would need escaping.

Previous run

Review

Findings

Info

• [test-coverage] internal/sandbox/sandbox_test.go — The three new tests (TestRestoreSymlinks_OpenshellNotInPath, _PathWithSpaces, _PathWithQuotes) verify the function doesn't panic and returns an error when openshell is missing, but they don't exercise the actual shell command logic (awk parsing, symlink recreation). A table-driven test with a mock Exec that verifies the constructed command string for various git ls-files output patterns (multiple symlinks, nested paths, paths with special characters) would catch regressions in the shell pipeline without requiring a live sandbox.

Previous run (2)

Review

Findings

Low

• [correctness] internal/sandbox/sandbox.go:278 — The awk command uses default whitespace field splitting. git ls-files --stage output format is <mode> <sha> <stage> <path>, where a tab separates the stage number from the path. With default awk field splitting (whitespace), $4 captures only the first word of a space-containing path, causing rm -rf and ln -s to operate on a truncated path. This only affects symlinks whose paths contain spaces — uncommon but possible.
  Remediation: Use tab-aware field splitting, e.g., awk -F'\t' '/^120000/{split($1,a," "); print a[2] "\t" $2}' and IFS=$'\t' read sha path.

Info

• [test-coverage] internal/sandbox/sandbox.go:275 — RestoreSymlinks has no unit test. This is understandable since it requires a live sandbox, but documenting the expected git ls-files --stage parsing in a table-driven test with a mock Exec would catch regressions in the shell command construction.

[fullsend-ai-review]

[info] test-coverage

RestoreSymlinks has no unit test. A table-driven test with a mock Exec could validate the shell command construction without requiring a live sandbox.

[fullsend-ai-review]

Review follow-ups

Created follow-up issues for actionable non-blocking review findings:

• #1134 — The sandbox function table lists Upload() but does not include the new UploadDir() function. The flow diagram also describes the upload step generically without noting the tarball-based approach.
• #1135 — TestUploadDir_OpenshellNotInPath only exercises the error path. The happy path (tarball creation with symlink preservation) is not unit-tested. Manual live-sandbox verification was performed per the PR description.

Previous run

Review follow-ups

Created follow-up issues for actionable non-blocking review findings:

• #1089 — The three new tests verify the function does not panic and returns an error when openshell is missing, but they do not exercise the actual shell command logic (awk parsing, symlink recreation). A table-driven test with a mock Exec that verifies the constructed command string for various git ls-files output patterns would catch regressions without requiring a live sandbox.

Previous run (2)

Review follow-ups

Created follow-up issues for actionable non-blocking review findings:

• #1080 — awk default whitespace field splitting truncates symlink paths containing spaces. git ls-files --stage uses a tab before the path, but awk treats both spaces and tabs as delimiters, so $4 only captures the first word of a space-containing path.
• #1081 — RestoreSymlinks has no unit test. A table-driven test with a mock Exec could validate the shell command construction without requiring a live sandbox.

[fullsend-ai-review]

[low] test-adequacy

TestUploadDir_OpenshellNotInPath only exercises the error path. The happy path (tarball creation with symlink preservation) is not unit-tested. Manual live-sandbox verification was performed per the PR description.

Suggested fix: Add a unit test that creates a directory with a symlink, runs the tar step in isolation, and inspects the archive to confirm symlinks are preserved.

[fullsend-ai-review]

[info] style

extractCmd is built with fmt.Sprintf without shell-escaping remotePath or remoteTar. Safe in practice since values are internally constructed, consistent with all other Exec() call sites.