Uh oh!
There was an error while loading. Please reload this page.
test: anti-regression tests for tarball home directory mirroring - #2466
test: anti-regression tests for tarball home directory mirroring#2466AhmedTMM wants to merge 7 commits into
Conversation
Tarballs are built with absolute /root/ paths, but GCP and AWS Lightsail SSH as a regular user whose $HOME is /home/<user>/. After extraction, binaries like `claude` end up at /root/.claude/local/bin/ but the launchCmd looks in $HOME/.claude/local/bin/ — causing "command not found". Add a post-extraction step that copies /root/ dotfiles to $HOME/ when the SSH user isn't root. This fixes `spawn claude gcp` failing with exit code 127 after tarball install. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adds 4 tests in a "non-root home directory mirroring" describe block to prevent regression of the /root/ → $HOME/ copy step for non-root SSH users (GCP, AWS Lightsail). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
louisgv
left a comment
There was a problem hiding this comment.
Security Review
Verdict: APPROVED
Commit: 9a8b908
Summary
This PR adds anti-regression tests for tarball home directory mirroring functionality. The changes are test-only additions that verify the security-critical mirroring behavior added to prevent path injection issues.
Security Analysis
agent-tarball.ts (lines 116-135):
- ✅ Non-root check properly uses
id -ucomparison (shell-safe) - ✅ Loop variable
_dis locally scoped and not user-controlled - ✅ Source paths use double-quotes with proper escaping:
"/root/$_d" - ✅ Destination paths use double-quotes:
"$HOME/$_d" - ✅ Error suppression
2>/dev/null || trueprevents failure propagation (non-fatal by design) - ✅ No command injection vectors (all paths are literal or shell-safe variables)
- ✅ Hardcoded directory list (no user input): .claude, .local, .npm-global, .cargo, .opencode, .hermes, .bun
agent-tarball.test.ts (lines 172-224):
- ✅ New test suite verifies the mirroring behavior
- ✅ Tests check for proper non-root guard (
id -u) - ✅ Tests verify hardcoded directory list
- ✅ Tests verify non-fatal failure mode (line 205-213)
- ✅ No security regressions introduced
Tests
- bun test: ✅ PASS (11/11 tests)
- biome lint: ✅ PASS (0 errors, 114 files)
- bash -n: N/A (no shell script changes)
- curl|bash: N/A (no shell script changes)
- macOS compat: N/A (no shell script changes)
Findings
No security issues found. The code properly guards against:
- Path traversal (hardcoded directory list)
- Command injection (proper quoting, no user input)
- Privilege escalation (non-root check before mirroring)
-- security/pr-reviewer
Files copied from /root/ retained root ownership, causing permission errors when agents tried to write to their config dirs. Adds chown -R after the cp -a step. Also adds a test to prevent regression. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Uh oh!
There was an error while loading. Please reload this page.
…arball Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
louisgv
left a comment
There was a problem hiding this comment.
Security Review
Verdict: APPROVED
Commit: 4df5a7b
Summary
This PR adds anti-regression tests for tarball home directory mirroring and fixes ownership of mirrored files. The implementation properly addresses file ownership issues when tarballs are extracted as root but accessed by non-root users.
Security Findings
No security issues found.
The changes:
- Add
chown -Rcommands to fix ownership of mirrored dotfiles - Use safe command substitutions (
$(id -u),$(id -g)) - Properly guard operations with error suppression (
2>/dev/null || true) - Execute only in non-root context (guarded by
if [ "$(id -u)" != "0" ]) - Use hardcoded directory list (no path traversal risk)
Tests
- bun test: ✅ PASS (12 tests, 28 assertions)
- Command injection: ✅ SAFE - Uses safe command substitutions
- Path traversal: ✅ SAFE - Hardcoded directory list
- Privilege escalation: ✅ SAFE - chown only works on user-owned files in non-root context
- Error handling: ✅ SAFE - Non-fatal failures with proper guards
Code Quality
- Follows project conventions (no try/catch, uses promise chaining)
- Comprehensive test coverage for new functionality
- Well-documented with inline comments
-- security/pr-reviewer
Replace try/catch in agent-tarball.ts with asyncTryCatch Result helpers: - Phase 3 (download/extract): asyncTryCatch → returns false on any failure - Phase 4 (mirror): asyncTryCatch → non-fatal, logs warning on failure Add chown ownership fix for non-root SSH users (GCP, AWS Lightsail): files extracted as root need ownership corrected after mirroring. Add 5 anti-regression tests for non-root home directory mirroring. Supersedes #2466. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
la14-1
commented
Mar 11, 2026
…2478) Replace try/catch in agent-tarball.ts with asyncTryCatch Result helpers: - Phase 3 (download/extract): asyncTryCatch → returns false on any failure - Phase 4 (mirror): asyncTryCatch → non-fatal, logs warning on failure Add chown ownership fix for non-root SSH users (GCP, AWS Lightsail): files extracted as root need ownership corrected after mirroring. Add 5 anti-regression tests for non-root home directory mirroring. Supersedes #2466. Co-authored-by: lab <6723574+louisgv@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
/root/→$HOME/mirroring step inagent-tarball.tsfor non-root SSH users (GCP, AWS Lightsail)describe("non-root home directory mirroring")block:/root/to$HOMEfor non-root users.spawn-tarballmarker filetrueeven when mirror step fails (non-fatal)id -u)Test plan
bunx @biomejs/biome check— 0 errorsbun test src/__tests__/agent-tarball.test.ts— 11 passbun test— 1501 pass, 0 fail🤖 Generated with Claude Code