Skip to content

fix: stop dropping Ctrl-C during interactive login, and let CI see it - #381

Merged
leggetter merged 1 commit into
mainfrom
fix/signal-chan-buffer
Sep 10, 2026
Merged

fix: stop dropping Ctrl-C during interactive login, and let CI see it#381
leggetter merged 1 commit into
mainfrom
fix/signal-chan-buffer

Conversation

@leggetter

Copy link
Copy Markdown
Collaborator

Found while reviewing #378 - unrelated to that change, and already present on main.

The bug

protectTerminalState puts the terminal into raw mode for the login prompt and restores it from a goroutine when the user interrupts. The channel was unbuffered:

signalChan := make(chan os.Signal)
signal.Notify(signalChan, os.Interrupt)

signal.Notify never blocks - it does a non-blocking send and drops the signal if nobody is receiving. The goroutine starts immediately after, but a Ctrl-C landing in that window is discarded, and the terminal stays in the state the prompt put it in: no echo, until the user runs reset.

Note: this is already fixed on release/v3.0.0

It landed incidentally inside f2de696 ("fix(mcp): do not sign the user out when a reauth is never completed") and never made it back to main, so the 2.x line still has it. The change here is byte-identical to the v3 version, comment included, so the two converge rather than conflict when the branches meet.

Why CI missed it

go test runs a reduced vet suite - printf, bools, atomic and a handful more - and only over the packages under test. sigchanyzer is not in that set, and nothing else ran vet:

- name: Run unit tests
  run: go test -short ./pkg/...

The unit-test job now also runs go vet ./..., which covers test/ and tools/ as well. It is clean at this commit, so the job passes without any other change. Adding it to the existing required job rather than as a new one means no branch-protection changes are needed.

That also surfaced two no-op append(sliceLiteral) calls in the acceptance tests, fixed here - adding a vet gate while leaving known vet errors in the tree would be incoherent.

Deliberately not here

  • Tagged vet. The step runs untagged, so the 22 acceptance build tags are unchecked. -tags basic is clean after this change; the rest are unverified. A tag matrix is a bigger conversation.
  • A gofmt gate. Roughly 40 files on main are already unformatted, so enforcing it needs a repo-wide formatting pass first. Worth doing - it is the other half of why formatting regressions land unnoticed - but not as a rider on this.

Testing

No test. The race is between signal.Notify returning and a goroutine's first receive, which is not reachable from a unit test without a real TTY and precise timing; term.GetState fails outright under go test because stdin is not a terminal. go vet is the check that catches this class, which is why it is now wired into CI - that is the regression guard, rather than a test that would have to fake the very thing being fixed.

Verified: go build ./..., go vet ./..., go test -short ./pkg/..., and go test -tags basic ./test/acceptance/ all pass.

`protectTerminalState` puts the terminal into raw mode for the prompt and
restores it from a goroutine when the user interrupts. The channel it listened
on was unbuffered, and signal.Notify never blocks: a signal arriving while
nobody is receiving is discarded. The goroutine starts immediately after the
Notify call, but a Ctrl-C in that window is dropped and the terminal is left in
the state the prompt put it in - no echo, until the user types `reset`.

The same one-line fix already exists on release/v3.0.0, where it landed
incidentally inside f2de696. This is byte-identical to it, so the two lines
converge rather than conflict when the branches meet.

CI could not have caught this. `go test` runs a reduced vet suite - printf,
bools, atomic and a handful more - and only over the packages under test.
sigchanyzer is not in that set, and nothing else ran vet. So the unit-test job
now runs the full suite over ./..., which also covers test/ and tools/. It is
clean at this commit.

That also turned up two no-op `append(sliceLiteral)` calls in the acceptance
tests, fixed here: they are vet findings, and adding a vet gate while leaving
known vet errors in the tree would be incoherent.

Not addressed: vet still only runs untagged in CI, so the 22 acceptance build
tags are unchecked, and there is no gofmt gate - roughly 40 files on main are
already unformatted, so that one needs a formatting pass before it can be
enforced.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
@leggetter
leggetter merged commit b6c0914 into main Sep 10, 2026
13 checks passed
@leggetter
leggetter deleted the fix/signal-chan-buffer branch September 10, 2026 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant