fix(session): exec claude on resume so the agent leads the pane - #1786
fix(session): exec claude on resume so the agent leads the pane#1786paskal wants to merge 1 commit into
Conversation
|
👋 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 gate marker read: ai= |
📝 WalkthroughWalkthroughClaude session command builders now prefix supported invocations with ChangesClaude execution handoff
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/session/claude_exec_pane_leader_test.go (1)
33-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the conversation-history
--resumebranch.This fresh temporary instance has no conversation data, so this case takes the
--session-idfallback. Add a fixture recognized bysessionHasConversationDataand assert both the start and restart--resumepaths retain the finalexec.🤖 Prompt for 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. In `@internal/session/claude_exec_pane_leader_test.go` around lines 33 - 39, Update the “resume by id” test setup to add conversation-history fixture data recognized by sessionHasConversationData, ensuring buildClaudeCommand selects the --resume branch instead of the --session-id fallback. Assert that both the initial start and restart command paths preserve the final exec invocation.
🤖 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.
Nitpick comments:
In `@internal/session/claude_exec_pane_leader_test.go`:
- Around line 33-39: Update the “resume by id” test setup to add
conversation-history fixture data recognized by sessionHasConversationData,
ensuring buildClaudeCommand selects the --resume branch instead of the
--session-id fallback. Assert that both the initial start and restart command
paths preserve the final exec invocation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 688c9031-9800-4bcb-8c28-1ef52e55a062
📒 Files selected for processing (2)
internal/session/claude_exec_pane_leader_test.gointernal/session/instance.go
The fresh-start path already exec's claude, so the agent replaces the wrapper
shell and becomes the pane's process-group leader. buildClaudeResumeCommand and
the resume/continue branches of buildClaudeCommand did not, so every restarted
session left bash leading the group with claude as its child:
fg pgid = 14053 /bin/bash
claude = 14058 (pgid 14053)
That makes a session look different depending on whether it was started or
restarted. tmux reports #{pane_current_command} as `bash` rather than the agent,
and anything comparing a pid against the tty's foreground pgid concludes the
agent is not running. ccgram hits both: it drops the session's hooks as
"nested" and types the pane as a shell, so a restarted session goes silent
while a freshly started one works.
Nothing follows the agent invocation in these command strings, so exec is free
here, and it makes the two spawn paths agree.
d1de439 to
991759b
Compare
github-actions
Bot
added
intake:clean
1 participant
What problem does this solve?
A session behaves differently depending on whether it was started or restarted. On a fresh start agent-deck
execs the agent, so claude is the pane's process-group leader.buildClaudeResumeCommandand theresume/continuebranches ofbuildClaudeCommanddo not, so after any restart the pane is led by bash with claude as its child.Anything inspecting the pane then reads it as a shell rather than an agent:
#{pane_current_command}reportsbash, and any check comparing a pid against the tty's foreground pgid concludes no agent is running.Why this change
execis what the fresh-start path already does, and its existing comment gives the reason ("exec replaces the current shell with claude for proper job control"). Adding it to the remaining four paths makes the two spawn shapes agree rather than introducing a new convention.It is also the same class of problem as #1567 / #1580, where the fix was to make tmux
execvp()bash directly so the tool is the pane leader instead of being wrapped by the server default-shell. That fixed the outer layer; these command strings reintroduce it at the inner one, because bash's implicit tail-exec optimisation does not apply to an&&chain.Nothing follows the agent invocation in these strings, so
execcosts nothing here.Considered and rejected: leaving it and having consumers special-case the wrapper. That pushes an agent-deck implementation detail onto every tool that reads a pane, and does not fix the start/restart asymmetry.
User impact
Restarted claude sessions now present the same way as freshly started ones:
#{pane_current_command}reports the agent, and no wrapper shell lingers in the process tree. Anything that reads panes to detect a running agent (status tooling, tmux-driven integrations, external bridges) starts working across restarts.No user-visible agent-deck behaviour changes otherwise: same flags, same session ids, same resume semantics.
Evidence
Same session, restarted through the built binary.
Before, on the pane's tty:
After:
The wrapper shell is gone and claude is the group leader (pid == pgid).
Downstream effect that led me here: with ccgram driving the same panes, a restarted session was silent because its hooks were discarded as coming from a "nested" claude (pid != foreground pgid) and the pane was typed as a shell. After the change the same session bound and relayed normally, confirmed end to end (agent reply delivered to its Telegram topic).
Sandboxed suite, exactly as CONTRIBUTING specifies:
All packages pass except
TestIssue1421_CleanStaleSSHSockets, which fails identically on a pristineorigin/mainworktree on this machine, so it is not from this change:That looks environmental (macOS
$TMPDIRpath plus@/:in the socket name) rather than a real regression; happy to open a separate issue if it is not already known.self-check.shreports 9 PASS, includingrevert-check("tests fail without the change, as they should") anddiff-size +81/-6.AI disclosure
Model(s), if AI helped: claude-opus-5
What actually bothered you
My human set ccgram up so he could answer his agent-deck sessions from his phone, and outbound never worked. In his words: "I see in telegram message I sent to you in this chat but not the reply." Typing from Telegram reached the agent, but nothing the agent said ever came back.
It took a long time to find because it is invisible from the outside: a freshly started session worked and the same session after a restart did not, with no error anywhere. The one session that behaved had simply never been restarted.
Checklist
HOME=$(mktemp -d) XDG_CONFIG_HOME= XDG_DATA_HOME= XDG_CACHE_HOME= go test ./...— everything passes exceptTestIssue1421_CleanStaleSSHSockets, which fails the same way on a pristineorigin/mainworktree here (see Evidence). Not ticking this since your pipeline runsinternal/sessionand will see the same failure; leaving it visible rather than claiming a clean run.execto a command string built once per spawn, with no extra work at runtime and nothing added tolist,statusorsession output. Say the word if you want numbers anyway.