Fix shelltool readExitCode to wait on the live stdout signal so it is not gated on the 100ms poll

[PratikDhanave]

What

In tool/shelltool/session.go, readExitCode captured signal := s.stdoutSignal and then immediately reassigned s.stdoutSignal = newSignal(). The producer readLoop only closes the current s.stdoutSignal (prev := s.stdoutSignal; s.stdoutSignal = newSignal(); close(prev)). Because readExitCode had already swapped in a replacement, readLoop closed that replacement on the next stdout chunk and never the old channel value readExitCode was selecting on.

That made the <-signal case dead: readExitCode always waited out the time.After(100ms) fallback poll to notice the exit-code line whenever it arrived in a separate read from the sentinel — stalling each command by up to ~100ms.

The fix drops the reassignment so readExitCode selects on the exact channel readLoop closes, mirroring waitForSentinel (which grabs signal := s.stdoutSignal without reassigning and is therefore woken immediately). The pre-command fresh signal is already installed by snapshotOffsets, so no reset is lost.

Why

This matches the intended producer/consumer signalling contract already used by waitForSentinel, and keeps command latency tight against the .NET ShellSession protocol semantics the Go port follows — the exit-code line delimiting each command should be observed as soon as it is read, not on a coarse poll.

Testing

Added TestReadExitCodeWakesOnLiveStdoutSignal (white-box, in the package's existing internal test file) that drives a persistentSession readLoop with a scripted stdout reader delivering the sentinel and the exit-code line in two separate reads ~5ms apart, then asserts waitForSentinel/readExitCode returns the correct rc in well under 100ms.

• Before the fix: the test fails, elapsed ~106ms (gated on the 100ms poll).
• After the fix: it returns in ~10ms.

go build ./..., go vet ./tool/shelltool/..., and go test -race ./tool/shelltool/... all pass.

[Copilot]

Pull request overview

This PR fixes a subtle producer/consumer signaling bug in the persistent shell session that caused readExitCode to miss the live stdoutSignal close and fall back to the 100ms polling path, adding avoidable latency to each command.

Changes:

• Stop resetting s.stdoutSignal inside readExitCode, so it always waits on the exact signal that readLoop will close for the next stdout chunk.
• Add a regression test that splits the sentinel and exit-code suffix across two stdout reads and asserts the exit code is observed promptly (not gated by the 100ms poll).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
tool/shelltool/session.go	Fixes readExitCode to wait on the live stdoutSignal channel closed by readLoop, avoiding the 100ms fallback poll delay.
tool/shelltool/localshell_internal_test.go	Adds a regression test covering the “sentinel and exit code arrive in separate reads” case to ensure prompt wake-up behavior.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[github-actions]

Parity Review — No Issues Found

This PR fixes a channel-signalling race in tool/shelltool/session.go (readExitCode). All changed symbols are unexported (persistentSession, readExitCode, readLoop); no public Go API surface is added, removed, or changed.

Cross-repo parity: Not applicable — this is a Go-internal concurrency fix with no semantic equivalent in the upstream .NET or Python implementations.

Label status: parity-approved is correct; public-api-change is not needed.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• awmgmcpg

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "awmgmcpg"

See Network Configuration for more information.

Generated by Go API Consistency Review Agent · 20.7 AIC · ⌖ 3.9 AIC · ⊞ 5.9K · ◷