Skip to content

fix(hooks): detach fish hook from the tty - #297

Merged
AnnatarHe merged 1 commit into
mainfrom
fix/fish-hook-detach-tty
Aug 5, 2026
Merged

fix(hooks): detach fish hook from the tty#297
AnnatarHe merged 1 commit into
mainfrom
fix/fish-hook-detach-tty

Conversation

@AnnatarHe

Copy link
Copy Markdown
Contributor

Problem

Escape text like ^[]11;rgb:1010/1010/1010^[\^[[32;1R^[[?62;22;52c intermittently
appears on the fish command line after Ctrl-C'ing a long-running foreground job.

Those bytes are not corruption — they are the terminal's replies to queries fish
sends. fish 4.1+ probes the terminal (the query-term feature, on by default) with:

query meaning reply seen
ESC]11;?ESC\ background colour ESC]11;rgb:1010/1010/1010ESC\
ESC[6n cursor position (CPR) ESC[32;1R
ESC[0c device attributes (DA1) ESC[?62;22;52c

fish emits these at startup and on prompt redraw. The terminal answers by writing into
the tty input queue. A hook child that holds that queue can leave the replies
unconsumed, after which they are read back as ordinary keystrokes and rendered as
literal text at the prompt.

Change

model/hooks/fish.fish was the least protected of the three hooks:

  • both shelltime track calls used > /dev/nullstdout only, while
    hooks/bash.bash and hooks/zsh.zsh both use &> /dev/null
  • none of the three call sites detached stdin, so every hook child inherited the tty
  • shelltime gc runs during config load, i.e. inside fish's startup query window

All three now use < /dev/null so the hook can never sit on terminal input, and the
two track calls fold stderr away for parity with bash/zsh.

Kept in the foreground deliberately: backgrounding with & makes fish emit job-status
notifications, which is noisier than the problem being fixed.

Notes

  • ensureHookFile() (model/shell.go:24) returns early when the hook file already
    exists, so existing installs keep their old ~/.shelltime/hooks/fish.fish until it is
    updated by hand or removed and reinstalled.
  • This hardens a plausible contributor; the root-cause bisect (bare fish vs. hooks vs.
    the interrupted process tree) is still outstanding, and fish's own documented escape
    hatch remains set -Ua fish_features no-query-term.

Testing

  • go build ./..., go test ./model/... pass
  • fish --no-execute model/hooks/fish.fish passes
  • go test ./commands/... has 10 failures, all pre-existing at main (daemon
    unix-socket tests plus one requiring the open binary) — verified by re-running at HEAD

🤖 Generated with Claude Code

The fish hook ran `shelltime gc` and both `shelltime track` calls with the
terminal still attached as stdin, and redirected stdout only. fish 4.1+
probes the terminal with OSC 11 / CPR / DA1 (the `query-term` feature) at
startup and on prompt redraw; the terminal answers by writing into the tty
input queue. A hook child holding that queue can leave those replies
unconsumed, after which they are read as keystrokes and appear as literal
escape text at the prompt.

Redirect stdin from /dev/null so the hook can never sit on terminal input,
and fold stderr into /dev/null to match the bash and zsh hooks, which
already use `&> /dev/null`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@codecov

codecov Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Flag Coverage Δ
unittests 79.47% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.
see 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@AnnatarHe
AnnatarHe merged commit 3027b19 into main Aug 5, 2026
4 checks passed
@AnnatarHe
AnnatarHe deleted the fix/fish-hook-detach-tty branch August 5, 2026 18:10
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