Skip to content

fix: say "Connected" even when there is no terminal to spin on - #377

Merged
leggetter merged 4 commits into
mainfrom
fix/listen-readiness-without-tty
Sep 10, 2026
Merged

fix: say "Connected" even when there is no terminal to spin on#377
leggetter merged 4 commits into
mainfrom
fix/listen-readiness-without-tty

Conversation

@leggetter

@leggetter leggetter commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Closes #376.

What was wrong

hookdeck listen connected, forwarded events, and never said it was ready.

SimpleRenderer.OnConnected kept its whole body — the readiness line and the active-filters notice — behind if r.spinner != nil. ansi.StartNewSpinner returns nil when the writer is not a TTY or colors are disabled, so Connected. Waiting for events... was dropped whenever output was piped or --color off was passed. README documents that line for both compact and quiet, so callers waiting on it were right to wait; they just had no way to stop waiting.

The writer is log.StandardLogger().Outstderr. Nothing calls SetOutput, so whether a line reached stdout was decided by whether stderr was a terminal. A wrapper that gave stdout a PTY and piped stderr lost the line too. #333 downgrades --output interactive to compact without a terminal, which steers non-interactive callers straight onto this renderer.

Reported from the field as a websocket failure. The websocket was healthy.

What changed

  • ansi.CanSpin(w) — makes "can a spinner be drawn here?" a question you can ask, so the next caller doesn't have to read nil as "nothing to say". StartNewSpinner now uses it, same behaviour.
  • SimpleRenderer.showStatus / stopStatus — spinner on a terminal exactly as before; a plain stdout line otherwise.
  • OnConnected prints unconditionally.
  • OnDisconnected had the same shape (message to stderr, bare newline to stdout) and is fixed the same way.

Net effect: readiness, reconnection and events all land on stdout, so one stream carries the whole state machine.

Verification

New tests in renderer_simple_test.go exercise the no-spinner path directly — readiness in compact and quiet, readiness under --color off, the drop notice reaching stdout, silence when the first attempt fails, and no duplicate notice across a reconnect. All five assertions fail against the unfixed renderer:

Error: "" does not contain "Connected. Waiting for events..."
Error: "\n" does not contain "Connection lost, reconnecting..."

End-to-end, a binary built from the v2.5.0 tag was run against a local stand-in for api.hookdeck.com / ws.hookdeck.com with a real PTY, then rebuilt from this branch:

Case v2.5.0 This branch
--color off, PTY no readiness line (websocket up, 101 + pings flowing) Connected. Waiting for events...
stdout+stderr piped, no --color no readiness line Connected. Waiting for events..., stderr now empty
TTY with colors spinner then readiness unchanged; line printed exactly once

go test ./... green.

Release

Targets main, shipping in v2.6.0 (a v2.5.1 patch was considered and dropped). KNOWN_ISSUES.md names v2.6.0 as the version to upgrade to. release/v3.0.0 carries the identical bug and already contains all of main, so it should take this via a merge of main rather than a cherry-pick.

Also here: saying why a connection failed

Added after the patch release was dropped in favour of v2.6.0 - "too broad for a patch" was the only reason it was held back, and that reasoning expired with the patch.

listen gave up with Could not connect. Terminating after 10 failed attempts to establish a connection. and no reason. The dial error existed in connect() but was logged at debug, so at the default log level the user learned the CLI had stopped trying and nothing about whether it was DNS, a refused connection, a proxy, or a rejected session.

That is the other half of the same report. The readiness line says when the tunnel came up; this says why when it did not. A caller who saw neither could not tell a healthy tunnel from a broken one, which is how a working connection got filed as a failure in the first place.

Could not connect. Terminating after 10 failed attempts to establish a
connection. Last error: dial tcp [::1]:39999: connect: connection refused

Verified by rebuilding and re-running the dead-endpoint reproduction that produced the reasonless message. TestLastConnectErr covers the client side.

leggetter and others added 4 commits September 9, 2026 17:43
`listen` connected, forwarded events, and never said it was ready. The
readiness line — documented in README for both compact and quiet — lived
inside `if r.spinner != nil`, and ansi.StartNewSpinner returns nil whenever
the writer is not a TTY or colors are off. So every piped, redirected, CI or
`--color off` run went silent at exactly the moment it succeeded, and callers
waiting for the documented signal could only time out.

The writer is the log stream, i.e. stderr. Nothing sets it, so whether a line
reached *stdout* was decided by whether *stderr* was a terminal — a harness
that gave stdout a PTY and piped stderr lost the line too. #333 then steers
non-interactive callers onto this very renderer.

Connection state is not decoration. It now prints unconditionally, and it
prints to stdout, next to the banner and the event log, so one stream carries
the whole state machine. With a terminal the spinner behaves exactly as
before. OnDisconnected had the same shape — the message went to stderr while
stdout got a bare newline — and is fixed the same way.

ansi.CanSpin makes the condition askable, so the next caller does not have to
read nil as "nothing to say".

Tests cover the no-spinner path directly: readiness in compact and quiet,
readiness under `--color off`, the drop notice reaching stdout, silence when
the very first attempt fails, and no duplicate notice across a reconnect.
All five fail on the unfixed renderer.

Closes #376

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
The entry named v2.5.1 because this was scoped as a patch release. It now
ships in v2.6.0, and a workaround telling people to upgrade to a version that
will never be published is worse than no workaround at all.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
`listen` gave up with "Could not connect. Terminating after 10 failed attempts
to establish a connection." and no reason. The reason existed - the dial error
is right there in connect() - but it was logged at debug, so at the default log
level the user was told the CLI had stopped trying and nothing about whether it
was DNS, a refused connection, a proxy, or a rejected session.

That is the other half of the report this branch already addresses. The
readiness line tells you when the tunnel came up; this tells you why when it
did not. A caller who saw neither had no way to tell a healthy tunnel from a
broken one, which is exactly how a working connection got filed as a failure.

The websocket client now keeps the last connect error and the proxy appends it
to the terminating message:

  Could not connect. Terminating after 10 failed attempts to establish a
  connection. Last error: dial tcp [::1]:39999: connect: connection refused

Held back earlier as too broad for a patch release. That reasoning expired when
the patch was dropped in favour of shipping in v2.6.0.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
@leggetter
leggetter merged commit 0486884 into main Sep 10, 2026
13 checks passed
@leggetter
leggetter deleted the fix/listen-readiness-without-tty branch September 10, 2026 11:13
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.

listen connects but never prints "Connected": readiness line is dropped when output is piped or --color off

1 participant