diff --git a/src/core/session-discovery.ts b/src/core/session-discovery.ts index 48465137b..b3b0a7ec9 100644 --- a/src/core/session-discovery.ts +++ b/src/core/session-discovery.ts @@ -837,14 +837,23 @@ function resolveAdoptableSessionForPane( // 3b. Filter by CLI type if requested if (filterCliId && match.cliId !== filterCliId) return undefined; - // npm's `codex` shim can remain as a Node launcher whose argv matches - // before generic discovery reaches the native child. Only follow that - // launcher: a process whose comm is already `codex` is the selected CLI - // itself and may legitimately have another Codex deeper in its tree. - // Other CLIs preserve legacy pid selection; their wrapper behavior is - // handled by explicit wrapperCli. - const cliPid = match.cliId === 'codex' && !match.matchedByComm - ? (findLaunchedCliPid(match.pid, 'codex') ?? match.pid) + // If the match came from an argv scan on a COMM_ARGV_LAUNCHER + // (node/ttadk/aiden/python… — matchedByComm=false), the matched pid is + // the LAUNCHER, not the CLI. The real CLI is a descendant that writes the + // session state / owns the transcript (e.g. claude keys + // ~/.claude/sessions/.json to the child, not the wrapper), so + // reading meta off the launcher pid misses it and leaves sessionId + // undefined → no bridgeJsonlPath → adopted claude's replies never reach + // Lark. findLaunchedCliPid does a comm-only BFS from the launcher's + // children to find the actual CLI binary. When none exists yet (or this + // process legitimately IS the CLI running under a launcher comm, e.g. a + // node-based CLI with no renamed process title), fall back to match.pid. + // + // matchedByComm=true means comm already named the CLI (`claude`/`codex`/…) + // — that process IS the selected CLI and may legitimately have another + // instance of the same CLI deeper in its tree, so never follow it. + const cliPid = !match.matchedByComm + ? (findLaunchedCliPid(match.pid, match.cliId) ?? match.pid) : match.pid; // 4. Read CLI working directory (Linux: /proc; macOS: lsof) diff --git a/src/core/worker-pool.ts b/src/core/worker-pool.ts index 6d5e68f3f..70631f38d 100644 --- a/src/core/worker-pool.ts +++ b/src/core/worker-pool.ts @@ -4538,7 +4538,7 @@ export function adoptSandboxBlocked( || sandboxEnabled(); } -export function forkAdoptWorker(ds: DaemonSession, opts?: { restoredFromMetadata?: boolean }): void { +export function forkAdoptWorker(ds: DaemonSession, opts?: { restoredFromMetadata?: boolean; prompt?: string; turnId?: string }): void { const cb = requireCallbacks(); const workerPath = join(__dirname, '..', 'worker.js'); const t = tag(ds); @@ -4668,15 +4668,26 @@ export function forkAdoptWorker(ds: DaemonSession, opts?: { restoredFromMetadata // from there (cursor-agent never calls `botmux send`). // Other CLIs fall back to legacy screen-capture only. const adoptedCliId = adopted.cliId ?? 'claude-code'; - if (adopted.source === 'herdr' && adoptedCliId === 'claude-code' && !adopted.sessionId) { + // Claude adopt needs a sessionId to compute bridgeJsonlPath (the worker's + // claude branch starts its transcript bridge ONLY from bridgeJsonlPath — it + // has no by-pid fallback like codex/traex/cursor). Discovery resolves the + // sessionId up front for the common case, but it can still come back empty: + // • herdr `agent list` exposes no pid, so a claude with no agent_session + // binding has nothing to key ~/.claude/sessions/.json off; + // • tmux discovery can record a launcher pid (node/ttadk/aiden wrapping + // claude) when the real-CLI-pid resolver hasn't found the child yet. + // In BOTH cases fall back to the unique claude session for this cwd. Applies + // to every adopt source (not just herdr) since the tmux path hits the same + // undefined-sessionId → no-bridge → replies-never-return failure. + if (adoptedCliId === 'claude-code' && !adopted.sessionId) { const claudeMeta = findUniqueClaudeSessionByCwd(adopted.cwd); if (claudeMeta?.sessionId) { adopted.sessionId = claudeMeta.sessionId; if (ds.session.adoptedFrom) ds.session.adoptedFrom.sessionId = claudeMeta.sessionId; sessionStore.updateSession(ds.session); - logger.info(`[${t}] Resolved Claude session for adopted herdr target by cwd`); + logger.info(`[${t}] Resolved Claude session for adopted ${adopted.source ?? 'tmux'} target by cwd`); } else { - logger.warn(`[${t}] Cannot resolve unique Claude session for adopted herdr target; final replies may be unavailable`); + logger.warn(`[${t}] Cannot resolve unique Claude session for adopted ${adopted.source ?? 'tmux'} target; final replies may be unavailable`); } } const hasCliPid = typeof adopted.originalCliPid === 'number'; @@ -4703,7 +4714,19 @@ export function forkAdoptWorker(ds: DaemonSession, opts?: { restoredFromMetadata model: botCfg.model, disableCliBypass: botCfg.disableCliBypass === true, codexRpcInput: botCfg.codexRpcInput === true || config.codexRpcInputDefault, - prompt: '', + // Adopt is normally observe-only (prompt=''), driven later by 'message' + // IPCs. But a re-fork triggered by an incoming Lark turn (worker had exited + // — crash, or the "adopted session ended" kill path) must carry that turn's + // input so it isn't dropped: the daemon's worker-null branch now routes + // adopt sessions here instead of forkWorker (which would spawn a fresh + // bmx-* CLI and lose bridge semantics). The init handler queues this prompt + // into pendingMessages and the adopt idle detector (setupAdoptIdleDetection + // → markPromptReady) flushes it to the observed pane, exactly like a + // live-worker follow-up. Content is already bridge-formatted by the caller + // (buildReforkCliInput → buildBridgeInputContent), so no + // wrapper leaks into the user's un-injected external CLI. + prompt: opts?.prompt ?? '', + turnId: opts?.turnId, resume: false, ownerOpenId: ds.ownerOpenId, webPort: ds.session.webPort, diff --git a/src/daemon.ts b/src/daemon.ts index 80932bbb6..6fbd4904c 100644 --- a/src/daemon.ts +++ b/src/daemon.ts @@ -111,6 +111,7 @@ import { initWorkerPool, setActiveSessionsRegistry, forkWorker, + forkAdoptWorker, sendWorkerInput, killWorker, reapOrphanWorkers, @@ -16732,12 +16733,26 @@ async function handleThreadReply(data: any, ctx: RoutingContext): Promise } await noteTurnReceived(ds, parsed.messageId, parsed.content, reforkSender, parsed.messageId, substituteTrigger ? SUBSTITUTE_RECEIVED_REACTION_EMOJI_TYPE : undefined); try { - forkWorker(ds, wrappedInput, { - // See `hadPriorCliInput` above — an opening on a CLI that never took any - // input cold-spawns rather than `--resume`-ing an empty session. - resume: ds.hasHistory && !(openingTurn && !hadPriorCliInput), - turnId: parsed.messageId, - }); + // Adopt sessions must re-fork via forkAdoptWorker, NOT forkWorker: the + // latter would spawn a fresh botmux-managed bmx-* CLI in the adopt cwd, + // losing the observe/bridge semantics and typing the wrapped prompt into a + // brand-new CLI instead of the user's original external pane. This branch + // is reachable whenever an adopt session's bridge worker has exited (crash, + // or the "adopted session ended" kill path) and a new Lark turn arrives. + // The turn's input rides in on the init prompt (bridge-formatted by + // buildReforkCliInput above); forkAdoptWorker queues it and the adopt idle + // detector flushes it to the observed pane. Adopt never --resumes a botmux + // session, so the openingTurn/hadPriorCliInput resume logic doesn't apply. + if (ds.adoptedFrom) { + forkAdoptWorker(ds, { prompt: wrappedInput.content, turnId: parsed.messageId }); + } else { + forkWorker(ds, wrappedInput, { + // See `hadPriorCliInput` above — an opening on a CLI that never took any + // input cold-spawns rather than `--resume`-ing an empty session. + resume: ds.hasHistory && !(openingTurn && !hadPriorCliInput), + turnId: parsed.messageId, + }); + } } catch (e) { if (openingTurn) releaseInitialUserTurn(ds); throw e; @@ -16966,7 +16981,17 @@ async function handleDocComment(ctx: DocCommentContext): Promise { await noteTurnReceived(ds, commentId, text, sender, turnId); rememberLastCliInput(ds, promptContent, wrappedInput); sessionStore.updateSession(ds.session); - forkWorker(ds, wrappedInput, { resume: ds.hasHistory, turnId }); + // Same adopt re-fork routing as handleThreadReply: an adopt session whose + // bridge worker exited must come back through forkAdoptWorker (observe + + // bridge), never forkWorker. buildDocCommentTurnInput(mode:'refork') + // delegates to buildReforkCliInput, which already bridge-formats the content + // when ds.adoptedFrom is set, so the doc-comment turn reaches the observed + // pane without a wrapper. + if (ds.adoptedFrom) { + forkAdoptWorker(ds, { prompt: wrappedInput.content, turnId }); + } else { + forkWorker(ds, wrappedInput, { resume: ds.hasHistory, turnId }); + } } return true; } catch (err) { diff --git a/test/adopt-tmux-claude-wrapper-repro.smoke.test.ts b/test/adopt-tmux-claude-wrapper-repro.smoke.test.ts new file mode 100644 index 000000000..3a381fd3a --- /dev/null +++ b/test/adopt-tmux-claude-wrapper-repro.smoke.test.ts @@ -0,0 +1,161 @@ +/** + * REPRO + regression guard for PR#293 issue #1 — tmux /adopt of a claude-code + * session that was launched under a COMM_ARGV_LAUNCHER wrapper (node / ttadk / + * aiden / python…) failed to resolve its sessionId, so the transcript bridge + * never started and the CLI's replies never returned to Feishu. + * + * Real-process smoke test (no fs/proc mocks): builds the exact process trees + * the paths need and calls the production `discoverAdoptableSessions`. + * + * Case A — WRAPPED (the bug): + * tmux pane → node (comm=node, argv has "claude") + * └── fake-claude (comm=claude — the REAL CLI) + * Claude keys ~/.claude/sessions/.json to the REAL claude child pid. + * The wrapper's argv contains the literal token "claude", so findCliProcess's + * cliIdFromCommArgv matches the WRAPPER pid by argv (matchedByComm=false). + * On buggy master, discovery only resolved the real child pid under a launcher + * for cliId==='codex', so claude kept the wrapper pid → readClaudeSessionMeta + * missed the child-keyed JSON → sessionId undefined → no bridge → no replies. + * + * Case B — DIRECT (regression guard): + * tmux pane → fake-claude (comm=claude, matchedByComm=true) + * The common, un-wrapped case. The shared-path fix must NOT disturb it: comm + * already names the CLI, so discovery keeps that pid and resolves sessionId. + * + * Skipped automatically when tmux is unavailable (e.g. CI without tmux). + */ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { execFileSync, spawnSync } from 'node:child_process'; +import { homedir, tmpdir } from 'node:os'; +import { mkdtempSync, mkdirSync, writeFileSync, rmSync, existsSync, copyFileSync, unlinkSync } from 'node:fs'; +import { join } from 'node:path'; +import { discoverAdoptableSessions } from '../src/core/session-discovery.js'; + +const SID_WRAPPED = 'adoptwr1-pr02-93aa-bbbb-ccccddddeeee'; +const SID_DIRECT = 'adoptdi1-pr02-93aa-bbbb-ccccddddeeee'; +// NOTE: session names must NOT start with `bmx-` — discovery skips bmx-* panes +// (already botmux-managed). These simulate a USER's own external tmux sessions. +const TMUX_WRAPPED = 'repro-adopt-claude-wrapped'; +const TMUX_DIRECT = 'repro-adopt-claude-direct'; + +function tmuxAvailable(): boolean { + return spawnSync('tmux', ['-V'], { stdio: 'ignore' }).status === 0; +} + +const hasTmux = tmuxAvailable(); +const nodeBin = process.execPath; + +let dir: string; +let fakeClaude: string; +let sessionsDir: string; +const writtenMetaFiles: string[] = []; +let wrappedClaudePid: number | undefined; +let directClaudePid: number | undefined; +let wrappedWorkDir: string; +let directWorkDir: string; + +function waitForPidFile(pidFile: string, deadlineMs: number): Promise { + return new Promise(async resolve => { + const deadline = Date.now() + deadlineMs; + while (Date.now() < deadline) { + if (existsSync(pidFile)) { + const raw = spawnSync('cat', [pidFile], { encoding: 'utf-8' }).stdout.trim(); + const pid = Number(raw); + if (Number.isInteger(pid) && pid > 0 && existsSync(`/proc/${pid}`)) return resolve(pid); + } + await new Promise(r => setTimeout(r, 100)); + } + resolve(undefined); + }); +} + +function writeMeta(pid: number, sessionId: string, cwd: string): void { + const metaPath = join(sessionsDir, `${pid}.json`); + writeFileSync(metaPath, JSON.stringify({ sessionId, cwd, startedAt: 1785000000000, updatedAt: 1785000009000 })); + writtenMetaFiles.push(metaPath); +} + +beforeAll(async () => { + if (!hasTmux) return; + dir = mkdtempSync(join(tmpdir(), 'bmx-adopt-repro-')); + wrappedWorkDir = join(dir, 'wrapped-work'); + directWorkDir = join(dir, 'direct-work'); + mkdirSync(wrappedWorkDir, { recursive: true }); + mkdirSync(directWorkDir, { recursive: true }); + + // A binary literally named `claude` so /proc//comm === 'claude'. + fakeClaude = join(dir, 'claude'); + const sleepBin = spawnSync('sh', ['-c', 'command -v sleep'], { encoding: 'utf-8' }).stdout.trim(); + copyFileSync(sleepBin || '/bin/sleep', fakeClaude); + execFileSync('chmod', ['+x', fakeClaude]); + + // ── Case A: wrapped (node → fake claude) ────────────────────────────────── + const wrapJs = join(dir, 'wrap.js'); + writeFileSync( + wrapJs, + `const { spawn } = require('child_process');\n` + + `const c = spawn(process.argv[2], ['600'], { stdio: 'ignore', cwd: process.argv[3] });\n` + + `require('fs').writeFileSync(process.argv[4], String(c.pid));\n` + + `process.on('SIGTERM', () => { try { c.kill('SIGKILL'); } catch {} process.exit(0); });\n` + + `setTimeout(() => {}, 600000);\n`, + ); + const wrappedPidFile = join(dir, 'wrapped-child.pid'); + // argv: node — the wrapper's argv + // thus carries the basename "claude", triggering the argv-match-on-wrapper. + execFileSync('tmux', [ + 'new-session', '-d', '-s', TMUX_WRAPPED, '-x', '200', '-y', '50', + nodeBin, wrapJs, fakeClaude, wrappedWorkDir, wrappedPidFile, + ]); + + // ── Case B: direct (fake claude straight in the pane) ───────────────────── + // Wrap in a tiny shell that records the claude pid so we can key its JSON. + const directPidFile = join(dir, 'direct-child.pid'); + execFileSync('tmux', [ + 'new-session', '-d', '-s', TMUX_DIRECT, '-x', '200', '-y', '50', + 'sh', '-c', `cd ${directWorkDir} && ${fakeClaude} 600 & echo $! > ${directPidFile}; wait`, + ]); + + wrappedClaudePid = await waitForPidFile(wrappedPidFile, 5000); + directClaudePid = await waitForPidFile(directPidFile, 5000); + + // Claude writes ~/.claude/sessions/.json — key both cases + // to their REAL claude pid exactly as claude-code does. + sessionsDir = join(homedir(), '.claude', 'sessions'); + mkdirSync(sessionsDir, { recursive: true }); + if (wrappedClaudePid) writeMeta(wrappedClaudePid, SID_WRAPPED, wrappedWorkDir); + if (directClaudePid) writeMeta(directClaudePid, SID_DIRECT, directWorkDir); +}, 30_000); + +afterAll(() => { + if (hasTmux) { + for (const s of [TMUX_WRAPPED, TMUX_DIRECT]) { + try { execFileSync('tmux', ['kill-session', '-t', s]); } catch { /* already gone */ } + } + } + for (const f of writtenMetaFiles) { + try { unlinkSync(f); } catch { /* ignore */ } + } + if (dir) rmSync(dir, { recursive: true, force: true }); +}); + +describe('tmux /adopt claude-code sessionId resolution (PR#293 issue #1)', () => { + it.skipIf(!hasTmux)('WRAPPED: resolves the child-keyed sessionId under a node launcher (the bug)', () => { + expect(wrappedClaudePid, 'wrapped fake-claude child pid should have been captured').toBeDefined(); + const sessions = discoverAdoptableSessions('claude-code'); + const mine = sessions.find(s => s.cwd === wrappedWorkDir || s.tmuxTarget?.startsWith(TMUX_WRAPPED)); + expect(mine, 'the wrapped adoptable claude pane must be discovered').toBeDefined(); + // On buggy master this was undefined: discovery read the session JSON off + // the WRAPPER pid. The fix resolves the real child pid → the sessionId. + expect(mine!.sessionId).toBe(SID_WRAPPED); + }); + + it.skipIf(!hasTmux)('DIRECT: still resolves sessionId for an un-wrapped claude (regression guard)', () => { + expect(directClaudePid, 'direct fake-claude pid should have been captured').toBeDefined(); + const sessions = discoverAdoptableSessions('claude-code'); + const mine = sessions.find(s => s.cwd === directWorkDir || s.tmuxTarget?.startsWith(TMUX_DIRECT)); + expect(mine, 'the direct adoptable claude pane must be discovered').toBeDefined(); + // comm already names the CLI (matchedByComm=true), so the shared-path fix + // must leave this pid selection untouched. + expect(mine!.sessionId).toBe(SID_DIRECT); + }); +}); diff --git a/test/session-lifecycle-start.test.ts b/test/session-lifecycle-start.test.ts index 68821457d..7ac3f9619 100644 --- a/test/session-lifecycle-start.test.ts +++ b/test/session-lifecycle-start.test.ts @@ -465,6 +465,58 @@ describe('Codex App clean-input feature gate', () => { }); }); +describe('adopt worker re-fork forwards the incoming turn (PR#293 issue #3)', () => { + // A tmux-adopted claude-code session whose bridge worker has exited. When a + // new Lark turn arrives, the daemon's worker-null branch now routes adopt + // sessions to forkAdoptWorker (not forkWorker, which would spawn a fresh + // bmx-* CLI and lose bridge semantics). forkAdoptWorker must carry that + // turn's prompt + turnId into the init so the worker delivers it to the + // observed pane instead of dropping it. + function makeAdoptDs(): DaemonSession { + return makeDs({ + adoptedFrom: { + source: 'tmux', + tmuxTarget: 'work:0.0', + originalCliPid: 4242, + sessionId: 'sess-adopt-live', + cliId: 'claude-code', + cwd: '/repo', + paneCols: 200, + paneRows: 50, + }, + }); + } + + it('forwards the re-fork prompt + turnId into the adopt init (not dropped)', () => { + const ds = makeAdoptDs(); + forkAdoptWorker(ds, { prompt: 'hello from Lark', turnId: 'om_refork_turn' }); + + const init = vi.mocked((ds.worker as any).send).mock.calls[0][0]; + expect(init).toEqual(expect.objectContaining({ + type: 'init', + adoptMode: true, + adoptSource: 'tmux', + adoptTmuxTarget: 'work:0.0', + cliId: 'claude-code', + prompt: 'hello from Lark', + turnId: 'om_refork_turn', + })); + }); + + it('defaults to an observe-only empty prompt when no turn rides along (restore path)', () => { + const ds = makeAdoptDs(); + forkAdoptWorker(ds, { restoredFromMetadata: true }); + + const init = vi.mocked((ds.worker as any).send).mock.calls[0][0]; + expect(init).toEqual(expect.objectContaining({ + type: 'init', + adoptMode: true, + prompt: '', + })); + expect(init.turnId).toBeUndefined(); + }); +}); + describe('session.start lifecycle integration', () => { it('emits session.start after forkWorker spawns a worker', () => { forkWorker(makeDs(), 'hello', false);