[Vasanthdev2004]

What changed

TestExecCommandTTYSkipsInteractiveBlock now stops its session in a cleanup and builds its tool on execTestRoot(t) instead of a bare t.TempDir(), the way the other exec tests that leave a session running already do. Test-only.

Why

The test runs ssh host.example.com with tty: true and a 250 ms yield. When ssh is still resolving the host after 250 ms, the tool returns with the session running in the test's directory, and nothing ever stops it. On Windows, t.TempDir's cleanup then fails:

--- FAIL: TestExecCommandTTYSkipsInteractiveBlock (1.54s)
    testing.go:1464: TempDir RemoveAll cleanup: unlinkat ...\TestExecCommandTTYSkipsInteractiveBlock693704757\001: The process cannot access the file because it is being used by another process.

That is what failed Smoke (windows-latest) on #1174, which only touches the READMEs, and on #1167 before it.

StopAll terminates the session and waits for it. execTestRoot then retries the removal for the moment Windows still holds the directory after the stop, which is what it is there for.

Linked issue

None. Maintainer fix for a recurring Windows CI flake, opened through the internal development process.

Verification

On Windows:

• A copy of the test whose session is sure to outlive the yield (the package's long-sleep helper under tty: true) fails on a bare t.TempDir() with the same sharing-violation error, and passes with this change.
• Each half on its own: execTestRoot without the stop passes but waits about 6 s for the sleeping process; the stop without execTestRoot passes here but leaves the release window execTestRoot exists for. Both together pass in under a second.
• The test passes 5 of 5.
• go vet ./... for windows, linux and darwin, gofmt, git diff --check, go test ./..., the release build and smoke, and govulncheck for linux and windows all pass. The pinned golangci-lint run over internal/tools reports only the existing web_fetch.go:315 QF1008 note.

Summary by CodeRabbit

• Tests
  • Updated automated checks for interactive command execution to account for SSH connection timing and clean up active sessions after testing.
  • These changes improve test consistency without changing user-facing behavior.

[github-actions]

Zero automated PR review

Verdict: No blockers found

Blockers

• None found.

Validation

• [pass] Diff hygiene: git diff --check
• [pass] Tests: go test ./...
• [pass] Build: go run ./cmd/zero-release build
• [pass] Smoke build: go run ./cmd/zero-release smoke

Scope

Head: a4578dacc0c8
Changed files (1): internal/tools/exec_command_test.go

This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality.

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: Twigpine/zero
• Branch: test/tty-exec-test-root
• File: .coderabbit.yaml
• Commit: a4578da
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Essentials
• Run ID: a29b90c2-aea5-4a6e-9d2d-1dfe17e15044

Commits

Reviewing files that changed from the base of the PR and between a0b036a and a4578da.

Files selected for processing: 1

• internal/tools/exec_command_test.go

Included review availability: This review used your included allowance. 2 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour.

Walkthrough

The TTY test now uses execTestRoot and a named session manager, stops sessions during cleanup, and accounts for SSH still resolving after the initial yield.

Changes

Exec command test

Layer / File(s)	Summary
Update TTY test setup internal/tools/exec_command_test.go	The test uses execTestRoot and a named session manager, and registers cleanup to stop sessions. Its comment allows for SSH to still be resolving after the initial yield.

Priority: Low

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

Change: Bug fix

Suggested reviewers: anandh8x

Merge Risk: Minimal · up to a4578

No actionable merge-blocking risk is identified in this test-only change.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within survi…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly describes the main test fix: stop the TTY session before removing its temporary root.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 1 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches: 💡 1

📝 Generate docstrings: 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Warning

Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption.

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

[jatmn]

🟢 Thanks for the contribution. I do not see any actionable issues from my review.

Reviewed head a4578dacc0c805a700e9483d361f9d2484257c0f against merge base 164150633a8b2e6ae864ae02fd7105b201eeb68e and live main at b837448dd4fbf2915b65a471ea4c63d50512cc89.

Merge readiness

GitHub reports MERGEABLE, CLEAN, and APPROVED; all 13 checks pass, including Windows/macOS/Linux smoke, race detection, lint, security, and CodeQL. Main's two intervening commits only change README content and assets; there is no overlapping code or release-metadata drift. The author's verified collaborator status qualifies for the internal contribution process. No duplicate or superseding open repair was identified; merged #1110 addresses a different foreground-server startup race.

Changed contract

Surface	Verified behavior
Session ownership	The tool and cleanup use the same test-local manager.
Lifecycle and ordering	Create root → launch/register session → check interactive guard → stop sessions → remove root. Cleanup LIFO preserves this order after both ordinary return and assertion failure.
Completion and failure	Empty/already completed sessions are safe; a retained session receives tree termination and the existing bounded wait before root removal retries.
Siblings and claims	Only this test changes. Its SSH input and interactive-guard assertion are preserved; no operator-facing surface, production API, persistence, dependency, or workflow changes.

Validation

• Focused Linux race tests covering this test, the paired interactive guard, session StopAll, and PTY input passed with -count=5.
• A review-only overlay used the existing long-sleep helper to guarantee a retained process and independently check that teardown leaves no live session and removes the root. It passed five times under the race detector. Removing the stop callback caused the intended live-session assertion to fail; the probe then cleaned up its process.
• Formatting and diff hygiene passed. The current Windows smoke log confirms the tools package, release build, and smoke passed. The earlier README-only PR's failed Windows attempt independently confirms this exact test's directory-sharing failure.
• Local Windows/macOS execution was unavailable; platform validation comes from the current CI runs. Full-repository build, test, vet, and security results come from CI. The reviewed checkout was unchanged.