fix(sandbox): keep relative in-repo symlinks on download

[rh-hemartin]

Summary

• sanitizeDownload previously removed all symlinks unconditionally, stripping tracked relative symlinks (.claude/skills, etc.) from the working tree after download
• Post-run make lint then failed in lint-broken-symlinks because git-tracked paths were absent
• Now keeps symlinks whose relative target resolves inside the repo root; removes absolute targets and relative targets that escape via ../

Test plan

• TestSanitizeDownload_RemovesAbsoluteSymlinks — absolute symlinks still removed
• TestSanitizeDownload_KeepsRelativeSymlinksInsideRepo — relative in-repo symlinks preserved
• TestSanitizeDownload_RemovesRelativeSymlinksEscapingRepo — traversal-escape symlinks removed
• All existing TestSanitizeDownload_* tests pass

Fixes #1149.

🤖 Generated with Claude Code

[github-actions]

Site preview

Preview: https://02e7487f-site.fullsend-ai.workers.dev

Commit: 481c27e2a5c898855546aba5cbcbaa5f59b3917c

[fullsend-ai-review]

Review

Findings

No findings.

Previous run

Review

Findings

No findings.

Previous run (2)

Review

Findings

No findings.

Previous run (3)

Review

Findings

Low

• [documentation-currency] internal/sandbox/sandbox.go:346 — The SafeDownload doc comment says "sanitizes the result by removing symlinks and .git/hooks/" — this now only removes dangerous symlinks (absolute or repo-escaping). The same stale phrasing appears in internal/cli/run.go:557, docs/guides/dev/cli-internals.md:319, and docs/guides/dev/cli-internals.md:326-327. All four locations should be updated to say something like "removes dangerous symlinks (absolute or repo-escaping) and .git/hooks/".
  Remediation: Update the doc comment on SafeDownload, the inline comment in run.go, and the two references in cli-internals.md to reflect the new selective-removal behavior.

[rh-hemartin]

Tested on a run as well with the vendored binary.

[ralphbean]

See inline comment.

[rh-hemartin]

Fixed in 1f8cb15. Replaced filepath.Clean(filepath.Join(...)) with filepath.EvalSymlinks on the raw (un-cleaned) path so intermediate symlink components are followed on the filesystem rather than collapsed textually. Added TestSanitizeDownload_RemovesSymlinkChainEscape which creates exactly the chain you described (sub/dirlink -> ../real, escape -> sub/dirlink/../../etc/passwd) and verifies escape is removed.

[rh-hemartin]

Updated in 481c27e. Two additions from ralphbean's branch:

1. EvalSymlinks(absLocal) instead of filepath.Clean — on macOS where /tmp -> /private/tmp, Clean leaves the symlink component in absLocal, causing the HasPrefix comparison to incorrectly reject safe symlinks whose EvalSymlinks result starts with the real prefix (/private/tmp/...).

2. Added TestSanitizeDownload_RemovesDirSymlinkIndirection (from tmp/sanitize-download-keep-inrepo-symlinks). The d/x -> . attack wasn't covered by my earlier test — wanted both cases in the suite.

The core approach stays different from the patch on your branch: I call EvalSymlinks on the raw path (dir + "/" + target without filepath.Clean) rather than on the cleaned path. This matters when repo/secret exists as a real file inside the repo — with the cleaned path, EvalSymlinks would return repo/secret and call it safe, even though following the symlink chain on the filesystem reaches the outer secret file. The raw path approach follows the chain as the OS would and returns the physical outside path.