Skip to content

fix(tmux): pass -u on the remaining attach paths - #1789

Open
Jonesxq wants to merge 2 commits into
asheshgoplani:mainfrom
Jonesxq:fix/tmux-utf8-attach-paths
Open

fix(tmux): pass -u on the remaining attach paths#1789
Jonesxq wants to merge 2 commits into
asheshgoplani:mainfrom
Jonesxq:fix/tmux-utf8-attach-paths

Conversation

@Jonesxq

@Jonesxq Jonesxq commented Jul 30, 2026

Copy link
Copy Markdown

What problem does this solve?

When agent-deck starts under a supervisor without a UTF-8 locale, four tmux attach clients outside the web terminal bridge can still start in non-UTF-8 mode and replace Unicode glyphs with _. Fixes #1781 and completes the follow-up to #1779.

Why this change

Pass tmux's global -u option before attach-session in the normal and read-only PTY attaches, ControlPipe, and KeySender. This makes each attach client use UTF-8 independently of its inherited locale. An AST regression test now rejects any production attach spawn that omits -u or places it after the subcommand; the existing exact-argv tests and comments were updated with the same contract.

User impact

Terminal output on the remaining PTY and control-mode attach paths preserves Unicode when agent-deck is launched by locale-light environments such as launchd or systemd. Environments that already provide a UTF-8 locale keep the same behavior. The change adds no process, handshake, poll, or output-processing step.

Evidence

The merged #1779 records real output for the same tmux mechanism: without -u, raw bytes for ⏵⏵ were 5f 5f (__); with -u, they were e2 8f b5 e2 8f b5 (⏵⏵). Issue #1781 identifies the four remaining attach sites that share that locale exposure.

Test-first baseline / revert-check:

$ go test internal/tmux/attach_utf8_argv_lint_test.go internal/tmux/tmux_exec_lint_helpers_test.go internal/tmux/tmux_exec_lint_test.go -run '^TestTmuxAttachSpawnsForceUTF8$' -count=1 -v
4 tmux attach spawn(s) do not force UTF-8:
  internal/tmux/controlpipe.go:113
  internal/tmux/keysender.go:101
  internal/tmux/pty.go:733
  internal/tmux/pty.go:737
FAIL

After adding -u, the same command passes.

Final checks:

$ go test internal/tmux/attach_utf8_argv_lint_test.go internal/tmux/control_mode_argv_lint_test.go internal/tmux/tmux_exec_lint_helpers_test.go internal/tmux/tmux_exec_lint_test.go -count=1
ok      command-line-arguments

$ GOOS=linux GOARCH=amd64 CGO_ENABLED=0 go test -c -o <temporary-path>/tmux.test ./internal/tmux
exit 0

$ GOOS=linux GOARCH=amd64 CGO_ENABLED=0 go vet ./internal/tmux
exit 0

$ gofmt -l internal/tmux/attach_utf8_argv_lint_test.go internal/tmux/control_mode_argv_lint_test.go internal/tmux/controlpipe.go internal/tmux/keysender.go internal/tmux/pty.go internal/tmux/pty_socket_test.go
(no output)

$ git diff --check
exit 0

The full sandboxed go test ./... suite, runtime tmux reproduction, and before/after performance timing were not run: the local WSL environment has no Go toolchain, and runtime WSL execution was unavailable for this run. The performance-sensitive scope is limited to one extra global option on existing one-time attach process launches.

AI disclosure

  • Human-written
  • AI-assisted (I directed and reviewed it)
  • AI-authored (a model wrote most of it)

Model(s), if AI helped: gpt-5-codex

Prompt / session log (optional): Not provided.

What actually bothered you

My human asked: "完成一下 #1781" ("Complete issue #1781.")

Checklist

  • Targeted diff: one problem, no unrelated changes
  • Tests added or updated for new behavior
  • Test suite passes sandboxed: HOME=$(mktemp -d) XDG_CONFIG_HOME= XDG_DATA_HOME= XDG_CACHE_HOME= go test ./... - not run locally; see Evidence
  • If this touches a hot path (list, status, session output, startup, tmux layer): before/after timing evidence included - not measured; this only adds a global option to existing attach launches
  • CHANGELOG.md untouched (entries are added at landing)
  • AI-assisted? Disclosed above, with validation evidence, and I can answer questions about the code
  • "Allow edits from maintainers" is enabled

Summary by CodeRabbit

  • Bug Fixes

    • Improved tmux session and control-mode attachments to consistently use UTF-8 mode.
    • Ensured headless tmux control behavior avoids opening /dev/tty for detached callers.
    • Updated attach command construction for both interactive and read-only modes for more reliable non-ASCII handling.
  • Tests

    • Added lint-style checks to fail if required tmux attach/control flags or arguments are missing.
    • Updated tmux command-argument expectations in socket-related tests to include UTF-8 mode.

@Jonesxq
Jonesxq requested a review from asheshgoplani as a code owner July 30, 2026 08:38
@github-actions github-actions Bot added intake:clean PR/issue passed the intake contract ai-authored Primarily authored by an AI agent labels Jul 30, 2026
@github-actions

Copy link
Copy Markdown

👋 Thanks for the contribution — intake looks complete.

Your PR body carries everything the maintainer's validation pipeline reads first: the problem, the reasoning, the human intent behind it, and an AI-disclosure. It will be applied, built, and tested against main, and you'll get a structured result within about a day. Merges are always human.

gate marker read: ai=authored model=gpt-5-codex intent=yes

@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 2469d04c-4f3b-4691-a8f5-8d593afb6428

📥 Commits

Reviewing files that changed from the base of the PR and between 1a524fe and 0bb8d4e.

📒 Files selected for processing (3)
  • internal/tmux/controlpipe.go
  • internal/tmux/keysender.go
  • internal/tmux/pty.go
🚧 Files skipped from review as they are similar to previous changes (3)
  • internal/tmux/keysender.go
  • internal/tmux/controlpipe.go
  • internal/tmux/pty.go

📝 Walkthrough

Walkthrough

tmux session and control-client attachment commands now pass the global -u flag before attach-session. Socket argv tests and AST-based lint coverage enforce the command shape across the remaining attach paths.

Changes

tmux UTF-8 attachment handling

Layer / File(s) Summary
Session attach argv updates
internal/tmux/pty.go, internal/tmux/pty_socket_test.go
Read-write and read-only session attachments include -u, with socket and empty-socket argv expectations updated.
Control-client attach argv updates
internal/tmux/controlpipe.go, internal/tmux/keysender.go
ControlPipe and KeySender use -C -u attach-session, with matching headless-process documentation and error text.
Attach command validation
internal/tmux/attach_utf8_argv_lint_test.go, internal/tmux/control_mode_argv_lint_test.go
Repository linting detects attach-session spawns without -u, and control-mode guidance documents the required UTF-8 attach form.

Estimated code review effort: 2 (Simple) | ~15 minutes

Possibly related PRs

Suggested reviewers: asheshgoplani

🚥 Pre-merge checks | ✅ 7
✅ Passed checks (7 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title uses conventional commits format and accurately describes the tmux attach-path UTF-8 fix.
Linked Issues check ✅ Passed The PR updates all remaining attach paths listed in #1781 and adds regression coverage for the UTF-8 flag.
Out of Scope Changes check ✅ Passed The test and comment updates are directly tied to enforcing -u on tmux attach paths and are in scope.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 70.00%.
Remote_parity ✅ Passed Not applicable: the PR only changes internal/tmux files; no internal/ui/ or cmd/agent-deck code was modified, so RemoteSession parity isn’t in scope.
Test_coverage_per_surface ✅ Passed Affected surfaces are covered: pty, controlpipe, and keysender now have regression/argv tests, and web already has explicit UTF-8 attach tests. No remote change.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/tmux/pty.go`:
- Around line 732-737: Document the controlling-TTY contracts near the
attach-client symbols: in internal/tmux/pty.go lines 732-737, explain that the
normal PTY attach requires a controlling terminal and describe its failure
behavior per `#1114`; in internal/tmux/controlpipe.go line 36, document the
control-pipe client’s distinct controlling-TTY behavior; and in
internal/tmux/keysender.go line 46, document the KeySender client’s
controlling-TTY behavior. Update comments only, preserving the existing command
implementations.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 4ccb7d2f-ae5f-4bcd-81c4-9d34ea4f50d9

📥 Commits

Reviewing files that changed from the base of the PR and between 580e772 and 1a524fe.

📒 Files selected for processing (6)
  • internal/tmux/attach_utf8_argv_lint_test.go
  • internal/tmux/control_mode_argv_lint_test.go
  • internal/tmux/controlpipe.go
  • internal/tmux/keysender.go
  • internal/tmux/pty.go
  • internal/tmux/pty_socket_test.go

Comment thread internal/tmux/pty.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-authored Primarily authored by an AI agent intake:clean PR/issue passed the intake contract

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(tmux): pass -u on the remaining attach paths (same locale exposure as #1779)

1 participant