Repository navigation
chore: rolling promotion dev -> main - #1020
Conversation
…1017) Add completion guard in ensureWorkerAlive() to prevent ghost executor spawns when stale messages arrive for agents that already finished. Checks last executor state before attempting auto-spawn — done and terminated executors are skipped, while error/suspended/first-spawn cases proceed normally.
…1016) Add guard in genie.ts no-args entry to detect when already inside the genie-tui tmux session (via TMUX env var) and exit with a helpful message. Modify attachTuiSession() to use switch-client instead of attach-session when already inside any tmux session. Add tests for both code paths.
…ce complexity Reduces ensureWorkerAlive cognitive complexity from 20 to 16 by extracting the done/terminated executor check into a standalone helper. Also fixes unused variable lint warnings in tests.
Prevents mock.module leak that broke spawn/exec imports in other test files when running the full suite.
bun's mock.module('node:child_process') permanently poisons the shared
module cache, breaking execSync/spawnSync in downstream test files
(wish-state, state). The TUI tmux behavior is covered by the guard in
genie.ts and the switch-client logic is trivial enough to verify manually.
…spawn Three fixes for agents/executors stuck permanently in 'spawning' state: 1. launchTmuxSpawn: after awaitAgentReadiness(), transition executor to 'running' and legacy worker to 'idle' — previously nothing triggered this transition, leaving all executors stuck in 'spawning' 2. reconcileStaleSpawns: add current_executor_id IS NULL guard so agent identity records (from findOrCreateAgent) are not falsely marked as errors — they track state through their executor, not the legacy state column 3. findOrCreateAgent: explicitly set state=NULL on identity records instead of inheriting the 'spawning' default, preventing them from being caught by any state-based reconciliation queries
fix: tmux spawn guards — nested cascade, completion guard
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughBumps package/plugin versions to 4.260403.3, adds a completion guard to avoid auto-spawning workers whose last executor is Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as genie.ts
participant Router as protocol-router
participant DB as Executor/DB
participant Scheduler as scheduler-daemon
participant Tmux as tmux.ts
User->>CLI: run genie (no-args)
CLI->>CLI: check process.env.TMUX
alt TMUX contains "genie-tui"
CLI->>CLI: log info and exit
else TMUX is set
CLI->>CLI: warn about tmux switch
end
CLI->>Router: ensureWorkerAlive(worker)
Router->>DB: getCurrentExecutor(worker)
DB-->>Router: executor(state)
alt state == done or terminated
Router-->>Router: completion guard -> skip auto-spawn, return null
else
Router->>Scheduler: attempt auto-spawn / resume
Scheduler->>DB: evaluate concurrency (includes spawning)
alt allowed
Scheduler->>Tmux: launch/attach TUI or spawn worker
Tmux->>Tmux: check process.env.TMUX
alt already in tmux
Tmux->>Tmux: run "tmux switch-client -t SESSION"
else
Tmux->>Tmux: run "tmux attach-session -t SESSION"
end
else
Scheduler-->>Router: skip due to concurrency_cap
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the version to 4.260403.3 across multiple configuration files and introduces a completion guard in the protocol router to prevent the automatic spawning of agents that have intentionally finished their tasks. It also adds logic to detect and handle nested tmux sessions. Feedback indicates that the implementation of switch-client for TUI session attachment will fail when invoked from a different tmux socket, and it is recommended to revert this change to ensure the TUI opens correctly via nesting.
| if (process.env.TMUX) { | ||
| // Already inside a tmux session — switch-client avoids nested attach | ||
| runTuiTmux(['switch-client', '-t', SESSION_NAME], 'inherit'); | ||
| } else { | ||
| runTuiTmux(['attach-session', '-t', SESSION_NAME], 'inherit'); | ||
| } |
There was a problem hiding this comment.
The implementation of switch-client here will fail in the most common scenario: when genie is invoked from a standard tmux session on a different socket (e.g., the default socket). Tmux's switch-client only works for clients already attached to the target server. Since genie-tui uses a dedicated socket (-L genie-tui), the command will return a "no current client" error, and the user will not be attached to the TUI.
Additionally, src/genie.ts (line 515) already prevents nesting when the user is on the genie-tui socket. Therefore, when this code is reached while process.env.TMUX is set, it is guaranteed to be a different socket where switch-client is inapplicable. Reverting to attach-session is necessary to ensure the TUI opens correctly (via nesting) when called from another tmux session.
runTuiTmux(['attach-session', '-t', SESSION_NAME], 'inherit');![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ac68088f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Completion guard: don't auto-spawn agents whose last executor finished | ||
| // intentionally. An agent that reached 'done' or 'terminated' shouldn't be | ||
| // auto-resurrected by stale messages — explicit dispatch handles re-spawn. | ||
| if (await isExecutorCompleted(worker)) return null; |
There was a problem hiding this comment.
This new guard blocks all auto-spawn when the current executor is terminated, but in this codebase terminated is also the state used when an agent is stopped/suspended for later resume (for example, handleWorkerStop still instructs users to “Send a message to auto-resume” in src/term-commands/agents.ts:1416). Because ensureWorkerAlive now returns early here, message-driven resume via sendMessage no longer reaches template-based respawn for stopped agents, so previously recoverable workers become permanently non-deliverable until manually restarted.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/protocol-router.ts`:
- Around line 69-74: isExecutorCompleted can throw and its call in
ensureWorkerAlive (and the similar call around lines 138-141) may bubble errors
out of sendMessage; wrap calls to isExecutorCompleted (and any direct
getCurrentExecutor usage) in a try/catch inside ensureWorkerAlive (and the other
guard site), catch DB/read errors, log or record the error, and treat the
executor as NOT completed (return false / proceed as alive) so the error does
not propagate; reference the isExecutorCompleted function, ensureWorkerAlive
function, sendMessage call site, and getCurrentExecutor helper when locating the
code to change.
In `@src/tui/tmux.ts`:
- Around line 83-88: The current logic always uses runTuiTmux(['switch-client',
...]) whenever process.env.TMUX is set, which fails if the user is inside a
different tmux server; change this to detect whether the TMUX environment points
at our tmux socket and only use 'switch-client' in that case. Parse
process.env.TMUX (take the socket path before the first comma) and compare it to
your tmux socket identifier (the same symbol used when creating the session,
e.g., the SESSION socket constant or variable used alongside SESSION_NAME); if
they match call runTuiTmux(['switch-client','-t', SESSION_NAME], 'inherit'),
otherwise call runTuiTmux(['attach-session','-t', SESSION_NAME], 'inherit').
🪄 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: ASSERTIVE
Plan: Pro
Run ID: c125ee0c-8da0-46e9-bf6e-6ee3223b4376
📒 Files selected for processing (8)
.claude-plugin/marketplace.jsonpackage.jsonplugins/genie/.claude-plugin/plugin.jsonplugins/genie/package.jsonsrc/genie.tssrc/lib/protocol-router.test.tssrc/lib/protocol-router.tssrc/tui/tmux.ts
| /** Check if a worker's last executor completed intentionally (done/terminated). */ | ||
| async function isExecutorCompleted(worker: registry.Agent | null): Promise<boolean> { | ||
| if (!worker?.currentExecutorId) return false; | ||
| const executor = await getCurrentExecutor(worker.id); | ||
| return executor != null && (executor.state === 'done' || executor.state === 'terminated'); | ||
| } |
There was a problem hiding this comment.
Handle executor lookup failures in the completion guard.
isExecutorCompleted() can throw (DB/read error), and it is called before the try/catch in ensureWorkerAlive(). That can now bubble an exception out of sendMessage instead of returning a normal undelivered result.
Suggested fix
async function isExecutorCompleted(worker: registry.Agent | null): Promise<boolean> {
if (!worker?.currentExecutorId) return false;
- const executor = await getCurrentExecutor(worker.id);
- return executor != null && (executor.state === 'done' || executor.state === 'terminated');
+ try {
+ const executor = await getCurrentExecutor(worker.id);
+ return executor != null && (executor.state === 'done' || executor.state === 'terminated');
+ } catch {
+ // Fail-open: if completion state can't be read, keep prior routing behavior.
+ return false;
+ }
}Also applies to: 138-141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/protocol-router.ts` around lines 69 - 74, isExecutorCompleted can
throw and its call in ensureWorkerAlive (and the similar call around lines
138-141) may bubble errors out of sendMessage; wrap calls to isExecutorCompleted
(and any direct getCurrentExecutor usage) in a try/catch inside
ensureWorkerAlive (and the other guard site), catch DB/read errors, log or
record the error, and treat the executor as NOT completed (return false /
proceed as alive) so the error does not propagate; reference the
isExecutorCompleted function, ensureWorkerAlive function, sendMessage call site,
and getCurrentExecutor helper when locating the code to change.
| if (process.env.TMUX) { | ||
| // Already inside a tmux session — switch-client avoids nested attach | ||
| runTuiTmux(['switch-client', '-t', SESSION_NAME], 'inherit'); | ||
| } else { | ||
| runTuiTmux(['attach-session', '-t', SESSION_NAME], 'inherit'); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In tmux, does tmux -L switch-client -t work when the current shell is attached to a different tmux server (different socket), or doesswitch-client require a current client on the same server?
💡 Result:
No, tmux -L switch-client -t does not work when the current shell is attached to a different tmux server (different socket). The switch-client command requires a current client attached to the specified server via the -L option. Without an attached client on that server, it fails with "no current client".
Citations:
- 1: https://man7.org/linux/man-pages/man1/tmux.1.html
- 2: https://man.openbsd.org/i386/tmux.1
- 3: https://github.com/tmux/tmux/blob/master/tmux.1
- 4: https://www.man7.org/linux/man-pages/man1/tmux.1.html
- 5: https://stackoverflow.com/questions/61764021/how-do-i-switch-the-active-tmux-session-inside-a-shell-script
🏁 Script executed:
# Check the full context of src/tui/tmux.ts to see TMUX_SOCKET, SESSION_NAME, and runTuiTmux
head -150 src/tui/tmux.ts | tail -100Repository: automagik-dev/genie
Length of output: 3997
🏁 Script executed:
# Find TMUX_SOCKET and SESSION_NAME definitions
rg -n 'TMUX_SOCKET|SESSION_NAME' src/tui/tmux.ts | head -20Repository: automagik-dev/genie
Length of output: 715
🏁 Script executed:
# Check the runTuiTmux function to confirm it uses -L flag
rg -A 10 'function runTuiTmux|const runTuiTmux' src/tui/tmux.tsRepository: automagik-dev/genie
Length of output: 721
switch-client should be limited to the genie tmux socket.
Using switch-client for any non-empty TMUX fails when the user is inside a different tmux server. Fall back to attach-session instead.
Suggested fix
export function attachTuiSession(): void {
- if (process.env.TMUX) {
+ const inGenieSocket = process.env.TMUX?.includes(TMUX_SOCKET) === true;
+ if (inGenieSocket) {
// Already inside a tmux session — switch-client avoids nested attach
runTuiTmux(['switch-client', '-t', SESSION_NAME], 'inherit');
} else {
runTuiTmux(['attach-session', '-t', SESSION_NAME], 'inherit');
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tui/tmux.ts` around lines 83 - 88, The current logic always uses
runTuiTmux(['switch-client', ...]) whenever process.env.TMUX is set, which fails
if the user is inside a different tmux server; change this to detect whether the
TMUX environment points at our tmux socket and only use 'switch-client' in that
case. Parse process.env.TMUX (take the socket path before the first comma) and
compare it to your tmux socket identifier (the same symbol used when creating
the session, e.g., the SESSION socket constant or variable used alongside
SESSION_NAME); if they match call runTuiTmux(['switch-client','-t',
SESSION_NAME], 'inherit'), otherwise call runTuiTmux(['attach-session','-t',
SESSION_NAME], 'inherit').
… from concurrency cap Bug 1: reconcileStaleSpawns second pass (added in prior commit) now verified by updating the integration test to expect dead-pane agents ARE cleaned up. Bug 2: The concurrency cap in attemptAgentResume counted 'spawning' agents as active, causing zombie spawning agents to block all auto-resume. Adding 'spawning' to the exclusion list fixes this — spawning agents are not yet consuming compute and should not count toward the cap.
fix(spawn): transition executor state from spawning to running after spawn
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/__tests__/zombie-spawns.test.ts`:
- Around line 119-121: The test uses an unnecessary type assertion: remove the
"as any" from the string literal 'spawning' in the zombieSpawning array creation
so it relies on the existing AgentState type; update both occurrences (the one
creating zombieSpawning and the similar instance around line 156) where
makeWorker({ id: ..., state: 'spawning' as any }) is used to simply pass state:
'spawning' to makeWorker (WorkerInfo).
In `@src/term-commands/agents.ts`:
- Around line 914-921: The code advances executor and worker states regardless
of whether awaitAgentReadiness(paneId) actually succeeded and it swallows any
update errors; change the logic so that
executorRegistry.updateExecutorState(ctx.executorId, 'running') and
registry.update(ctx.workerId, { state: 'idle' }) are only invoked when
awaitAgentReadiness(paneId) returns success (or indicates readiness), and do not
silently ignore failures—handle promise rejections from
executorRegistry.updateExecutorState and registry.update by bubbling/logging the
error instead of using .catch(()=>{}). Use the existing ctx.executorId and
ctx.workerId checks to gate updates and ensure any update failures are logged or
rethrown so state write errors are observable.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: fe0d9053-fadf-4425-a2d5-05a409f19277
📒 Files selected for processing (5)
src/lib/__tests__/zombie-spawns.test.tssrc/lib/agent-registry.test.tssrc/lib/agent-registry.tssrc/lib/scheduler-daemon.tssrc/term-commands/agents.ts
| const zombieSpawning: WorkerInfo[] = Array.from({ length: 5 }, (_, i) => | ||
| makeWorker({ id: `zombie-${i}`, state: 'spawning' as any }), | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unnecessary as any cast.
'spawning' is already a valid AgentState value (per agent-registry.ts line 25). The type assertion is redundant.
♻️ Remove unnecessary cast
- const zombieSpawning: WorkerInfo[] = Array.from({ length: 5 }, (_, i) =>
- makeWorker({ id: `zombie-${i}`, state: 'spawning' as any }),
+ const zombieSpawning: WorkerInfo[] = Array.from({ length: 5 }, (_, i) =>
+ makeWorker({ id: `zombie-${i}`, state: 'spawning' }),
);Same applies to line 156.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/__tests__/zombie-spawns.test.ts` around lines 119 - 121, The test
uses an unnecessary type assertion: remove the "as any" from the string literal
'spawning' in the zombieSpawning array creation so it relies on the existing
AgentState type; update both occurrences (the one creating zombieSpawning and
the similar instance around line 156) where makeWorker({ id: ..., state:
'spawning' as any }) is used to simply pass state: 'spawning' to makeWorker
(WorkerInfo).
| await awaitAgentReadiness(paneId); | ||
|
|
||
| // Transition executor + legacy worker from 'spawning' to 'running' | ||
| if (ctx.executorId) { | ||
| await executorRegistry.updateExecutorState(ctx.executorId, 'running').catch(() => {}); | ||
| } | ||
| await registry.update(ctx.workerId, { state: 'idle' }).catch(() => {}); | ||
|
|
There was a problem hiding this comment.
Gate 'running'/'idle' transitions on readiness and stop silently swallowing state-write failures.
Line 914 can continue after a readiness timeout, but Lines 918 and 920 still force lifecycle transitions and ignore failures. This can desync executor/worker state and undermine spawn/respawn decisions.
Suggested fix
-async function awaitAgentReadiness(paneId: string): Promise<void> {
- if (paneId === 'inline') return;
+async function awaitAgentReadiness(paneId: string): Promise<boolean> {
+ if (paneId === 'inline') return true;
const result = await waitForAgentReady(paneId);
if (result.ready) {
console.log(` ✓ Agent ready (${(result.elapsedMs / 1000).toFixed(1)}s)`);
- } else {
- console.log(` ⚠ Agent readiness timeout (${Math.round(result.elapsedMs / 1000)}s) — proceeding anyway`);
+ return true;
}
+ console.log(` ⚠ Agent readiness timeout (${Math.round(result.elapsedMs / 1000)}s) — proceeding anyway`);
+ return false;
}
@@
- await awaitAgentReadiness(paneId);
+ const ready = await awaitAgentReadiness(paneId);
- // Transition executor + legacy worker from 'spawning' to 'running'
- if (ctx.executorId) {
- await executorRegistry.updateExecutorState(ctx.executorId, 'running').catch(() => {});
- }
- await registry.update(ctx.workerId, { state: 'idle' }).catch(() => {});
+ if (ready) {
+ if (ctx.executorId) {
+ try {
+ await executorRegistry.updateExecutorState(ctx.executorId, 'running');
+ } catch (err) {
+ console.warn(`Warning: failed to update executor "${ctx.executorId}" to running: ${err}`);
+ }
+ }
+ try {
+ await registry.update(ctx.workerId, { state: 'idle' });
+ } catch (err) {
+ console.warn(`Warning: failed to update worker "${ctx.workerId}" to idle: ${err}`);
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/term-commands/agents.ts` around lines 914 - 921, The code advances
executor and worker states regardless of whether awaitAgentReadiness(paneId)
actually succeeded and it swallows any update errors; change the logic so that
executorRegistry.updateExecutorState(ctx.executorId, 'running') and
registry.update(ctx.workerId, { state: 'idle' }) are only invoked when
awaitAgentReadiness(paneId) returns success (or indicates readiness), and do not
silently ignore failures—handle promise rejections from
executorRegistry.updateExecutorState and registry.update by bubbling/logging the
error instead of using .catch(()=>{}). Use the existing ctx.executorId and
ctx.workerId checks to gate updates and ensure any update failures are logged or
rethrown so state write errors are observable.
git pull origin main while on dev merges main INTO dev, keeping the dev-prefix version (1.*). This caused genie brain update to show wrong version after update (e.g. "Updated: 0.260403.2 → 1.260403.6"). Now explicitly checks out main first.
…-main fix(brain): checkout main before pulling in brain update
Rolling Promotion PR
Auto-maintained rolling promotion PR from
devtomain.Process:
ready-to-mergeadded when all checks passSummary by CodeRabbit
Chores
Bug Fixes
Tests