fix(sandbox): gate /tmp test assertions on GOOS, not path existence

[euxaristia]

Update after rebase: main picked up the same functional guard fix via #378 while this PR was open, so this now only adds the explanatory comment for why the guard needs the GOOS check. Merge for the comment or close as superseded, either is fine.

What

Adds a comment to the /tmp assertions in TestPermissionProfileFromPolicyIncludesDefaultTempWriteRoots and TestNewScopeNormalizesAndValidatesExtraRoots explaining why runtime.GOOS != "windows" is required in addition to pathExists("/tmp"): Go resolves the bare path /tmp against the current drive on Windows, so a stray C:\tmp used to satisfy the old guard while the product code (correctly, see defaultTempWriteRootCandidatesForGOOS in internal/sandbox/scope.go) never adds /tmp as a Windows default temp root. The comment keeps the check from being "simplified" away later.

Testing

Full internal/sandbox suite passes on Windows 11 after the rebase. POSIX behavior unchanged.

🤖 Generated with Claude Code

[coderabbitai]

Walkthrough

Two sandbox tests now treat /tmp as a default temp write root only on non-Windows platforms, with comments explaining the Windows path-resolution behavior.

Changes

Sandbox test /tmp gating

Layer / File(s)	Summary
Windows-safe /tmp assertions internal/sandbox/manager_test.go, internal/sandbox/scope_test.go	Adds POSIX-only comments, imports runtime, and gates the /tmp root expectation behind a non-Windows check in the sandbox tests.

Estimated code review effort: 1 (Trivial) | ~3 minutes

Suggested reviewers: Vasanthdev2004

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the test-only fix to gate /tmp assertions on GOOS instead of path existence.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[kevincodex1]

LGTM

[kevincodex1]

hello @euxaristia kindly rebase to main and fix conflicts

[euxaristia]

Rebased onto main. One note: main already picked up the functional guard fix via #378 while this was open, so after the rebase this PR only adds the explanatory comment for why the GOOS check is needed (Go resolves the bare path /tmp against the current drive on Windows, so a stray C:\tmp made the old pathExists guard pass). Merge it for the comment or close as superseded by #378, either is fine. Sandbox suite passes on Windows 11 after the rebase.

[Vasanthdev2004]

Worth merging: marginal — I'd lean close-as-superseded, but your call

The functional part of this PR — the runtime.GOOS != "windows" guard on the /tmp test assertions — already landed on main via #378 (confirmed at manager_test.go:75 and scope_test.go:132). So the current diff is comment-only (+6/−0): two identical 3-line comments explaining why the guard needs the GOOS check.

The comments are accurate — I verified the claim against internal/sandbox/scope.go (defaultTempWriteRootCandidatesForGOOS returns only TEMP/TMP on Windows and adds /tmp only on POSIX), and the gotcha is real (Go resolves the bare /tmp against the current drive on Windows, so a stray C:\tmp could satisfy a naive pathExists-only guard). Given #414 broke a Windows test by simplifying something subtle, a "don't simplify this away" comment is cheap regression insurance.

That said — applying our usefulness-first bar: this is documentation churn on a superseded PR. I'd lean toward closing it as superseded to keep the history clean, but it's harmless enough that merging the comment is defensible too. You flagged this yourself (@euxaristia): "merge for the comment or close as superseded, either is fine." Your call.

CI note: the red Smoke (windows-latest) is not this PR — it's the known TestLoadOrCreateSecretConcurrentConverges concurrency flake in internal/oauth, a package this PR doesn't touch. Everything else is green.

[gnanam1990]

VERDICT: approve

REGRESSION RISK: none identified

The PR adds explanatory comments to two existing test assertions in internal/sandbox/manager_test.go and internal/sandbox/scope_test.go. No production code, test logic, or assertions are changed. The functional guard (runtime.GOOS != "windows") already landed on main via #378; this PR only documents why the guard is necessary.

BUILD / TEST

• go build ./... — pass (worktree)
• go vet ./internal/sandbox/... — pass
• gofmt -l on changed files — clean
• go test ./internal/sandbox/... -count=1 — pass
• CI: Windows Smoke failed, but all test packages (including internal/sandbox) show ok in the log. The failure is in a post-test smoke step, not the test suite. The PR is comment-only and cannot cause a test failure. This is an environmental/pre-existing issue on the Windows runner.
• All other CI jobs green (macOS, Ubuntu, Performance Smoke, Security, Zero Review).

CONTRIBUTING

No linked issue. The author is a community contributor (euxaristia). Per CONTRIBUTING.md, community PRs require an approved parent issue with the issue-approved label. No such issue is linked. However, the change is comment-only with zero functional impact — the process violation is technical. Maintainer's call whether to merge or close as superseded (Vasanthdev2004 already noted the functional fix landed via #378).

FINDINGS

None. The diff is two comment additions to test files.

EXISTING REVIEWS

• Vasanthdev2004 (COMMENTED): Correctly identified that the functional guard already landed via #378, making this comment-only. Suggested close-as-superseded as a valid option. Valid and accurate on the current head. Nothing missed.
• kevincodex1 (APPROVED): "LGTM" — agree for the comment-only nature. Nothing missed.
• coderabbitai (APPROVED): No issues found. Nothing missed.

All reviews are on the current head (23120e34). No stale reviews.

BOTTOM LINE

Comment-only PR with zero regression risk; the functional fix it documents already landed via #378, so merge for the documentation value or close as superseded.