Repository navigation
release: v4.260404 — Claude Agent SDK executor + Omni bridge - #1024
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds comprehensive Claude SDK support and an Omni messaging system: new SDK types, providers, executors (tmux + in-process SDK), an OmniBridge with NATS routing and reply publishing, frontmatter/agent-directory SDK sync, CLI flags/commands, tests, migrations, manifest bumps, and removals of several brain docs/scripts. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant NATS as "NATS Server"
participant Bridge as "OmniBridge\n(src/services/omni-bridge)"
participant Exec as "Executor\n(ClaudeCode | ClaudeSdk)"
participant SDK as "Claude Agent SDK\n(in-process)"
participant TMUX as "tmux / Claude Code\npane & inbox"
participant Reply as "omni-reply\npublisher"
User->>NATS: publish omni.message.{instance}.{chat} (JSON)
NATS->>Bridge: deliver message
Bridge->>Bridge: lookup session by agent:chatId
alt session exists & alive
Bridge->>Exec: deliver(session, OmniMessage)
alt Exec using tmux
Exec->>TMUX: write to inbox file / tmux pane
Exec->>Reply: publish via omni-reply (NATS) if configured
else Exec using SDK
Exec->>SDK: runQuery / stream messages
SDK-->>Exec: SDKMessage stream
Exec->>Reply: publish reply (NATS) when produced
end
else session missing
Bridge->>Bridge: if under maxConcurrent spawn
Bridge->>Exec: spawn(agent, chatId, env)
Exec-->>Bridge: return OmniSession
Bridge->>Exec: deliver(session, OmniMessage)
end
Note over Bridge,Exec: periodic checks -> isAlive / shutdown idle sessions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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}
GitHub API unavailable (no gh CLI / mcp__github__ tools in session). README not updated per policy — no fresh data available. https://claude.ai/code/session_019VijAgAxeWy3ivemv3wysp
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.genie/agents/metrics-updater/runs.jsonl:
- Line 18: The metrics-updater agent is failing due to missing environment
dependencies (errors "gh CLI not available" and "GitHub MCP tools not present in
session") so restore the required GitHub tooling and session bindings for the
metrics-updater service: ensure the gh CLI is installed and on PATH, reinstall
or enable the GitHub MCP tools used by the agent runtime, and verify the
metrics-updater startup/init sequence (the agent that writes runs.jsonl) can
locate and authenticate to those tools before it attempts work; after restoring
tools, rerun the agent and confirm runs in runs.jsonl resume producing metrics
rather than immediate failures.
🪄 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: 3a0ab9e2-459d-4592-9f4c-bda2e655a66c
📒 Files selected for processing (2)
.genie/agents/metrics-updater/runs.jsonl.genie/agents/metrics-updater/state.json
| {"timestamp":"2026-03-31T00:00:00Z","duration_ms":5002,"api_calls":2,"tools_generated":4,"errors":0,"metrics":{"releases_24h":0,"merged_prs_7d":35,"avg_merge_time_h":0.3,"ship_rate_pct":85}} | ||
| {"timestamp":"2026-04-01T00:00:00Z","duration_ms":8000,"api_calls":0,"tools_generated":0,"errors":["gh CLI unavailable","GitHub MCP tools not loaded in session"],"status":"failed","fallback":false,"metrics":null} | ||
| {"timestamp":"2026-04-02T00:00:00.000Z","duration_ms":15000,"api_calls":0,"tools_generated":0,"errors":["gh CLI not available","GitHub MCP tools not present in session"],"status":"failed","fallback":false,"metrics":null} | ||
| {"timestamp":"2026-04-03T00:00:00.000Z","duration_ms":3200,"api_calls":0,"tools_generated":0,"errors":["gh CLI not available","GitHub MCP tools not present in session"],"status":"failed","fallback":false,"metrics":null} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if gh CLI is available and configured in the environment
which gh || echo "gh CLI not found in PATH"
# Check recent workflow runs for metrics-updater if it's a GitHub Action
gh run list --workflow=metrics-updater --limit 5 2>/dev/null || echo "Cannot access workflow runs"
# Check if there are any recent successful metrics updates
rg -n '"status":"success"' .genie/agents/metrics-updater/runs.jsonl | tail -5Repository: automagik-dev/genie
Length of output: 498
🏁 Script executed:
cat -n .genie/agents/metrics-updater/runs.jsonl | tail -10Repository: automagik-dev/genie
Length of output: 3307
Metrics-updater blocked by missing GitHub infrastructure for three consecutive days.
The service fails to run since 2026-04-01 (lines 16-18) due to unavailable gh CLI and GitHub MCP tools. No metrics collected since 2026-03-31. The failing runs complete faster as duration drops from 15s to 3s, indicating rapid failure before attempting work.
This is an environment configuration issue preventing the metrics agent from executing, not a code defect. Infrastructure dependencies must be restored to resume metrics collection.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.genie/agents/metrics-updater/runs.jsonl at line 18, The metrics-updater
agent is failing due to missing environment dependencies (errors "gh CLI not
available" and "GitHub MCP tools not present in session") so restore the
required GitHub tooling and session bindings for the metrics-updater service:
ensure the gh CLI is installed and on PATH, reinstall or enable the GitHub MCP
tools used by the agent runtime, and verify the metrics-updater startup/init
sequence (the agent that writes runs.jsonl) can locate and authenticate to those
tools before it attempts work; after restoring tools, rerun the agent and
confirm runs in runs.jsonl resume producing metrics rather than immediate
failures.
Replace filesystem-based Omni integration with native NATS pub/sub transport. Genie subscribes to omni.message.> and routes inbound WhatsApp messages to per-chat tmux windows via executor interface. - IExecutor interface (spawn/deliver/shutdown/isAlive) for pluggable backends - ClaudeCodeOmniExecutor: first implementation using tmux windows - OmniBridge: NATS subscriber + router with idle timeout (15min) and max concurrency (20) with auto-reply queuing - omni-reply publisher for agent → NATS → Omni reply routing - genie omni start/stop/status CLI commands - nats dependency added to package.json Closes #1025
feat(omni-bridge): NATS integration for Omni ↔ Genie messaging
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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/services/executors/claude-code.ts`:
- Around line 209-214: The current PAYLOAD construction escapes only
quotes/newlines and can produce invalid JSON when MSG contains backslashes,
tabs, CRs, etc.; replace the hand-rolled printf-based builder with a proper JSON
encoder to build PAYLOAD atomically (use a tool like jq/python/node to
json-encode MSG, GENIE_OMNI_AGENT, GENIE_OMNI_CHAT_ID and timestamp) so that the
variables MSG, GENIE_OMNI_AGENT, and GENIE_OMNI_CHAT_ID are passed through a
JSON serializer rather than manual escaping, ensuring all special characters are
correctly escaped and the resulting PAYLOAD is valid JSON.
- Around line 158-163: The isAlive method in claude-code.ts currently returns
the tmux pane existence via isPaneAlive(session.paneId), but that doesn't detect
if the Claude process inside the pane has exited and left a shell; change
isAlive(OmniSession) to verify the actual Claude process is running rather than
just the pane: obtain the pane PID (session.paneId / use tmux helper that
returns pane PID), then check that the process tree for that PID contains the
Claude binary/process name or the expected agent command (or add a new helper
like isClaudeProcessAlive(panePid) in src/lib/tmux.ts that returns true only if
a matching process is present); if the process check fails, return false so the
bridge can respawn the session. Ensure you reference the existing isPaneAlive
usage and replace/augment it to check the underlying process for the Claude
agent.
- Around line 28-30: The fallback reply script path returned by
getReplyScriptPath() points to ~/.genie/bin/omni-reply but the complementary
TypeScript helper ("../src/services/omni-reply.ts") is never installed there, so
machines without nats/natscli cannot publish replies; update the implementation
so it either (A) resolves and invokes a real installed package entrypoint (use
require.resolve or the package's bin entry) instead of assuming ~/.genie/src/...
exists, or (B) ensure the helper file is shipped alongside the installed script
under ~/.genie (copy the compiled helper into
~/.genie/src/services/omni-reply.js during install); apply the same change to
the other occurrences referenced around lines 223-226 to keep paths consistent.
- Around line 22-25: sanitizeWindowName currently strips and truncates chatId
which can produce collisions shared by spawn() and deliver(); change it to
append a short deterministic hash of the full chatId so IDs remain unique.
Implement sanitizeWindowName to (1) create a filename-safe prefix from chatId as
now, (2) compute a stable hash (e.g., SHA-256 or SHA-1) of the entire chatId and
take a short hex substring (e.g., first 8–12 chars), (3) combine prefix + "-" +
hash, ensuring the final string uses only tmux/filename-safe chars and still
respects length limits. Update any callers (spawn() and deliver()) to keep using
sanitizeWindowName unchanged so they get the collision-proof value.
In `@src/services/omni-bridge.ts`:
- Around line 320-323: The catch block that currently deletes the session
placeholder (using this.sessions.delete(key)) bypasses the common session
removal logic that drains the backlog, so queued chats can remain stuck; update
the spawn-failure catch and any dead-session cleanup paths (the blocks around
the current this.sessions.delete(key) and the cleanup that calls
removeSession()) to call the shared removal routine (removeSession(key) or
whatever central session-teardown method is used) instead of deleting directly,
and ensure removeSession() contains the backlog-draining logic so all removal
paths (idle-timeout, spawn failure, dead-session cleanup) trigger backlog
draining consistently.
- Around line 276-278: The concurrency checks currently compute activeCount by
filtering out entries with e.spawning, which ignores reserved "spawning"
placeholders and allows bursts to oversubscribe; change the logic in the checks
that reference this.sessions (the activeCount calculation near the current check
and the similar one later) to include spawning entries when comparing against
this.maxConcurrent so that spawning placeholders count toward capacity (i.e.,
stop filtering by !e.spawning or explicitly treat e.spawning as an occupied
slot), and keep the variable name (activeCount) and properties (this.sessions,
this.maxConcurrent, e.spawning) so the updated checks are easy to locate and
consistent.
- Line 83: messageQueue currently holds raw OmniMessage items which allows
multiple backlog entries for the same chat to repeatedly call spawnSession() and
overwrite live sessions; change messageQueue into a per-chat queue (e.g.,
Map<string, OmniMessage[]> or Record<string, OmniMessage[]>) keyed by the
session/chat key, update enqueue logic that pushes into messageQueue to add to
the appropriate per-chat array (and apply a per-chat cap to avoid unbounded
growth), change drainQueue() to iterate per-chat queues and deliver batched
messages into an existing session when one exists (without calling
spawnSession() that replaces this.sessions[key]) and only call spawnSession()
when no live session exists, and modify spawnSession() so it does not blindly
overwrite this.sessions[key] if a session is already active (use a check like if
(!this.sessions[key]) then create/assign).
- Around line 107-135: Replace all console.log calls in this module with the
project logging abstraction (e.g., processLogger or the module's existing
logger) — do not leave any console usage in src/**/*.ts; find the console.log
calls around the NATS connect sequence and elsewhere (the block that references
this.natsUrl, this.nc, this.sub, processSubscription,
idleCheckTimer/checkIdleSessions, IDLE_CHECK_INTERVAL_MS, bridgeInstance,
this.maxConcurrent and this.idleTimeoutMs) and change them to appropriate logger
methods (info/debug/error) with the same message text and interpolated values;
ensure you import or reference the existing logger used elsewhere in the repo
and use it consistently for the other console.log ranges noted (lines ~143-175
and ~282-368).
- Around line 141-175: The stop() method currently clears timers and drains NATS
but never shuts down per-session executors, leaving tmux windows running; update
stop() to iterate this.sessions (use the same map referenced in stop), for each
session entry call and await its executor.shutdown() (or
session.executor.shutdown()) inside a try/catch to ensure failures don't block
shutdown, clear each entry.idleTimer and any other resources, and only then
proceed to unsubscribe/drain/set this.nc and bridgeInstance to null so all
spawned agents are properly terminated before the bridge reports "Stopped".
In `@src/term-commands/omni.ts`:
- Around line 45-69: The stop/status commands currently rely solely on the
process-local getBridge() in src/services/omni-bridge.ts which only returns an
in-memory singleton, so separate CLI invocations can't control a bridge started
in another terminal; change the behavior so getBridge() (or a new helper like
getRemoteBridgeClient) first attempts to contact a daemon boundary (e.g., check
a PID file and connect to a Unix domain socket or TCP control port) and only
falls back to the in-process singleton, and update the omni.command('stop') and
omni.command('status') handlers to use that remote client when getBridge()
returns null (calling remote stop/status RPC or sending the appropriate signal)
so genie omni stop/status work across processes rather than only in-process.
- Line 31: Replace all occurrences of console.log in this file (e.g., the call
that prints "[genie omni] Bridge running. Press Ctrl+C to stop." and the other
console.log calls around lines referenced 52-53 and 68-95) with the project's
logging/output abstraction (use the existing logger or output utility used
elsewhere in the repo, e.g., logger.info or output.write) so these messages go
through centralized logging; keep the same message text and log level context
(informational) and preserve any dynamic data or newlines. Locate each
console.log call by searching for the exact string "[genie omni]" and replace it
with the appropriate logger method, ensuring imports for the logging utility are
added if missing and remove the console import/usages. Ensure formatting and
return behavior remain unchanged.
🪄 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: 4a8933c7-ab57-4087-a201-1beba7c974d6
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
knip.jsonpackage.jsonsrc/genie.tssrc/services/executor.tssrc/services/executors/claude-code.tssrc/services/omni-bridge.tssrc/services/omni-reply.tssrc/term-commands/omni.ts
Comment on lines
+22
to
+25
| /** Sanitize a string for use as a tmux window name. */ | ||
| function sanitizeWindowName(chatId: string): string { | ||
| return chatId.replace(/[^a-zA-Z0-9_-]/g, '').slice(0, 40) || 'chat'; | ||
| } |
There was a problem hiding this comment.
Make the chat-derived key collision-proof.
This helper strips characters and truncates length, so distinct chat IDs can collapse to the same tmux window and inbox filename. Because spawn() and deliver() both reuse it, two conversations can end up sharing one session and leaking replies across chats.
Suggested fix
-import { randomUUID } from 'node:crypto';
+import { createHash, randomUUID } from 'node:crypto';
@@
function sanitizeWindowName(chatId: string): string {
- return chatId.replace(/[^a-zA-Z0-9_-]/g, '').slice(0, 40) || 'chat';
+ const prefix = chatId.replace(/[^a-zA-Z0-9_-]/g, '').slice(0, 28) || 'chat';
+ const suffix = createHash('sha256').update(chatId).digest('hex').slice(0, 8);
+ return `${prefix}-${suffix}`;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** Sanitize a string for use as a tmux window name. */ | |
| function sanitizeWindowName(chatId: string): string { | |
| return chatId.replace(/[^a-zA-Z0-9_-]/g, '').slice(0, 40) || 'chat'; | |
| } | |
| /** Sanitize a string for use as a tmux window name. */ | |
| function sanitizeWindowName(chatId: string): string { | |
| const prefix = chatId.replace(/[^a-zA-Z0-9_-]/g, '').slice(0, 28) || 'chat'; | |
| const suffix = createHash('sha256').update(chatId).digest('hex').slice(0, 8); | |
| return `${prefix}-${suffix}`; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/executors/claude-code.ts` around lines 22 - 25,
sanitizeWindowName currently strips and truncates chatId which can produce
collisions shared by spawn() and deliver(); change it to append a short
deterministic hash of the full chatId so IDs remain unique. Implement
sanitizeWindowName to (1) create a filename-safe prefix from chatId as now, (2)
compute a stable hash (e.g., SHA-256 or SHA-1) of the entire chatId and take a
short hex substring (e.g., first 8–12 chars), (3) combine prefix + "-" + hash,
ensuring the final string uses only tmux/filename-safe chars and still respects
length limits. Update any callers (spawn() and deliver()) to keep using
sanitizeWindowName unchanged so they get the collision-proof value.
Comment on lines
+28
to
+30
| function getReplyScriptPath(): string { | ||
| return join(homedir(), '.genie', 'bin', 'omni-reply'); | ||
| } |
There was a problem hiding this comment.
The TypeScript fallback target is not installed under ~/.genie.
getReplyScriptPath() writes this script to ~/.genie/bin/omni-reply, so "$SCRIPT_DIR/../src/services/omni-reply.ts" resolves to ~/.genie/src/services/omni-reply.ts. This PR never installs that file there, so machines without nats / natscli cannot publish replies at all. Ship a real helper beside the script or invoke an installed package entrypoint.
Also applies to: 223-226
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/executors/claude-code.ts` around lines 28 - 30, The fallback
reply script path returned by getReplyScriptPath() points to
~/.genie/bin/omni-reply but the complementary TypeScript helper
("../src/services/omni-reply.ts") is never installed there, so machines without
nats/natscli cannot publish replies; update the implementation so it either (A)
resolves and invokes a real installed package entrypoint (use require.resolve or
the package's bin entry) instead of assuming ~/.genie/src/... exists, or (B)
ensure the helper file is shipped alongside the installed script under ~/.genie
(copy the compiled helper into ~/.genie/src/services/omni-reply.js during
install); apply the same change to the other occurrences referenced around lines
223-226 to keep paths consistent.
Comment on lines
+158
to
+163
| async isAlive(session: OmniSession): Promise<boolean> { | ||
| try { | ||
| return await isPaneAlive(session.paneId); | ||
| } catch { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
isAlive() is checking the pane, not the Claude process.
The check delegates to isPaneAlive() in src/lib/tmux.ts, Lines 548-570, which only tells you whether the tmux pane still exists. If Claude exits back to a shell prompt, this still returns true, so the bridge keeps routing messages to a session with no agent consuming them and never respawns it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/executors/claude-code.ts` around lines 158 - 163, The isAlive
method in claude-code.ts currently returns the tmux pane existence via
isPaneAlive(session.paneId), but that doesn't detect if the Claude process
inside the pane has exited and left a shell; change isAlive(OmniSession) to
verify the actual Claude process is running rather than just the pane: obtain
the pane PID (session.paneId / use tmux helper that returns pane PID), then
check that the process tree for that PID contains the Claude binary/process name
or the expected agent command (or add a new helper like
isClaudeProcessAlive(panePid) in src/lib/tmux.ts that returns true only if a
matching process is present); if the process check fails, return false so the
bridge can respawn the session. Ensure you reference the existing isPaneAlive
usage and replace/augment it to check the underlying process for the Claude
agent.
Comment on lines
+209
to
+214
| # Build JSON payload | ||
| PAYLOAD=$(printf '{"content":"%s","agent":"%s","chat_id":"%s","timestamp":"%s"}' \\ | ||
| "$(echo "$MSG" | sed 's/"/\\\\"/g' | tr '\\n' ' ')" \\ | ||
| "\${GENIE_OMNI_AGENT:-unknown}" \\ | ||
| "\${GENIE_OMNI_CHAT_ID:-unknown}" \\ | ||
| "$(date -u +%Y-%m-%dT%H:%M:%S.%3NZ)") |
There was a problem hiding this comment.
The shell payload builder can emit invalid JSON.
Only quotes and newlines are escaped before printf. Replies containing backslashes, tabs, or carriage returns will produce malformed JSON and fail to publish. Use a real JSON encoder instead of hand-rolled escaping.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/executors/claude-code.ts` around lines 209 - 214, The current
PAYLOAD construction escapes only quotes/newlines and can produce invalid JSON
when MSG contains backslashes, tabs, CRs, etc.; replace the hand-rolled
printf-based builder with a proper JSON encoder to build PAYLOAD atomically (use
a tool like jq/python/node to json-encode MSG, GENIE_OMNI_AGENT,
GENIE_OMNI_CHAT_ID and timestamp) so that the variables MSG, GENIE_OMNI_AGENT,
and GENIE_OMNI_CHAT_ID are passed through a JSON serializer rather than manual
escaping, ensuring all special characters are correctly escaped and the
resulting PAYLOAD is valid JSON.
| private sub: Subscription | null = null; | ||
| private executor: IExecutor; | ||
| private sessions = new Map<string, SessionEntry>(); | ||
| private messageQueue: OmniMessage[] = []; |
There was a problem hiding this comment.
Queue by chat, not by raw message.
messageQueue stores individual messages, and drainQueue() later feeds each one straight into spawnSession(). Since spawnSession() blindly replaces this.sessions[key] with a new placeholder at Lines 294-295, repeated backlog entries for the same chat can overwrite a live session with a fresh spawn instead of delivering into it. This also leaves backlog growth unbounded.
Also applies to: 280-283, 287-295, 396-397
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/omni-bridge.ts` at line 83, messageQueue currently holds raw
OmniMessage items which allows multiple backlog entries for the same chat to
repeatedly call spawnSession() and overwrite live sessions; change messageQueue
into a per-chat queue (e.g., Map<string, OmniMessage[]> or Record<string,
OmniMessage[]>) keyed by the session/chat key, update enqueue logic that pushes
into messageQueue to add to the appropriate per-chat array (and apply a per-chat
cap to avoid unbounded growth), change drainQueue() to iterate per-chat queues
and deliver batched messages into an existing session when one exists (without
calling spawnSession() that replaces this.sessions[key]) and only call
spawnSession() when no live session exists, and modify spawnSession() so it does
not blindly overwrite this.sessions[key] if a session is already active (use a
check like if (!this.sessions[key]) then create/assign).
Comment on lines
+141
to
+175
| async stop(): Promise<void> { | ||
| if (!this.nc) { | ||
| console.log('[omni-bridge] Not running'); | ||
| return; | ||
| } | ||
|
|
||
| console.log('[omni-bridge] Shutting down...'); | ||
|
|
||
| // Stop idle checker | ||
| if (this.idleCheckTimer) { | ||
| clearInterval(this.idleCheckTimer); | ||
| this.idleCheckTimer = null; | ||
| } | ||
|
|
||
| // Clear all idle timers | ||
| for (const entry of this.sessions.values()) { | ||
| if (entry.idleTimer) clearTimeout(entry.idleTimer); | ||
| } | ||
|
|
||
| // Unsubscribe | ||
| if (this.sub) { | ||
| this.sub.unsubscribe(); | ||
| this.sub = null; | ||
| } | ||
|
|
||
| // Drain and close | ||
| try { | ||
| await this.nc.drain(); | ||
| } catch { | ||
| // Connection may already be closed | ||
| } | ||
| this.nc = null; | ||
| bridgeInstance = null; | ||
|
|
||
| console.log('[omni-bridge] Stopped'); |
There was a problem hiding this comment.
stop() disconnects the bridge but leaves every spawned agent running.
This clears timers and drains NATS, but it never calls executor.shutdown() for live sessions. The tmux windows keep running after the bridge reports "Stopped", and nothing will reap them because the idle checker is already gone.
Suggested fix
// Clear all idle timers
for (const entry of this.sessions.values()) {
if (entry.idleTimer) clearTimeout(entry.idleTimer);
+ if (!entry.spawning) {
+ await this.executor.shutdown(entry.session).catch(() => {});
+ }
}
+ this.sessions.clear();
+ this.messageQueue = [];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async stop(): Promise<void> { | |
| if (!this.nc) { | |
| console.log('[omni-bridge] Not running'); | |
| return; | |
| } | |
| console.log('[omni-bridge] Shutting down...'); | |
| // Stop idle checker | |
| if (this.idleCheckTimer) { | |
| clearInterval(this.idleCheckTimer); | |
| this.idleCheckTimer = null; | |
| } | |
| // Clear all idle timers | |
| for (const entry of this.sessions.values()) { | |
| if (entry.idleTimer) clearTimeout(entry.idleTimer); | |
| } | |
| // Unsubscribe | |
| if (this.sub) { | |
| this.sub.unsubscribe(); | |
| this.sub = null; | |
| } | |
| // Drain and close | |
| try { | |
| await this.nc.drain(); | |
| } catch { | |
| // Connection may already be closed | |
| } | |
| this.nc = null; | |
| bridgeInstance = null; | |
| console.log('[omni-bridge] Stopped'); | |
| async stop(): Promise<void> { | |
| if (!this.nc) { | |
| console.log('[omni-bridge] Not running'); | |
| return; | |
| } | |
| console.log('[omni-bridge] Shutting down...'); | |
| // Stop idle checker | |
| if (this.idleCheckTimer) { | |
| clearInterval(this.idleCheckTimer); | |
| this.idleCheckTimer = null; | |
| } | |
| // Clear all idle timers | |
| for (const entry of this.sessions.values()) { | |
| if (entry.idleTimer) clearTimeout(entry.idleTimer); | |
| if (!entry.spawning) { | |
| await this.executor.shutdown(entry.session).catch(() => {}); | |
| } | |
| } | |
| this.sessions.clear(); | |
| this.messageQueue = []; | |
| // Unsubscribe | |
| if (this.sub) { | |
| this.sub.unsubscribe(); | |
| this.sub = null; | |
| } | |
| // Drain and close | |
| try { | |
| await this.nc.drain(); | |
| } catch { | |
| // Connection may already be closed | |
| } | |
| this.nc = null; | |
| bridgeInstance = null; | |
| console.log('[omni-bridge] Stopped'); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/omni-bridge.ts` around lines 141 - 175, The stop() method
currently clears timers and drains NATS but never shuts down per-session
executors, leaving tmux windows running; update stop() to iterate this.sessions
(use the same map referenced in stop), for each session entry call and await its
executor.shutdown() (or session.executor.shutdown()) inside a try/catch to
ensure failures don't block shutdown, clear each entry.idleTimer and any other
resources, and only then proceed to unsubscribe/drain/set this.nc and
bridgeInstance to null so all spawned agents are properly terminated before the
bridge reports "Stopped".
Comment on lines
+276
to
+278
| // Check concurrency limit | ||
| const activeCount = Array.from(this.sessions.values()).filter((e) => !e.spawning).length; | ||
| if (activeCount >= this.maxConcurrent) { |
There was a problem hiding this comment.
Count spawning placeholders toward maxConcurrent.
Both capacity checks ignore spawning entries. A burst of new chats can reserve far more than maxConcurrent slots while they are still starting, then all of them finish and oversubscribe the bridge.
Also applies to: 392-394
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/omni-bridge.ts` around lines 276 - 278, The concurrency checks
currently compute activeCount by filtering out entries with e.spawning, which
ignores reserved "spawning" placeholders and allows bursts to oversubscribe;
change the logic in the checks that reference this.sessions (the activeCount
calculation near the current check and the similar one later) to include
spawning entries when comparing against this.maxConcurrent so that spawning
placeholders count toward capacity (i.e., stop filtering by !e.spawning or
explicitly treat e.spawning as an occupied slot), and keep the variable name
(activeCount) and properties (this.sessions, this.maxConcurrent, e.spawning) so
the updated checks are easy to locate and consistent.
Comment on lines
+320
to
+323
| } catch (err) { | ||
| console.error(`[omni-bridge] Failed to spawn session for ${key}:`, err); | ||
| this.sessions.delete(key); | ||
| } |
There was a problem hiding this comment.
Freeing a slot does not consistently wake the backlog.
The idle-timeout path is the only one that drains queued work. A spawn failure deletes the placeholder directly, and dead-session cleanup just calls removeSession(), so queued chats can stay stuck even after capacity is available. Move backlog draining into the common session-removal path.
Also applies to: 357-362, 382-385
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/omni-bridge.ts` around lines 320 - 323, The catch block that
currently deletes the session placeholder (using this.sessions.delete(key))
bypasses the common session removal logic that drains the backlog, so queued
chats can remain stuck; update the spawn-failure catch and any dead-session
cleanup paths (the blocks around the current this.sessions.delete(key) and the
cleanup that calls removeSession()) to call the shared removal routine
(removeSession(key) or whatever central session-teardown method is used) instead
of deleting directly, and ensure removeSession() contains the backlog-draining
logic so all removal paths (idle-timeout, spawn failure, dead-session cleanup)
trigger backlog draining consistently.
| await bridge.start(); | ||
|
|
||
| // Keep the process alive | ||
| console.log('[genie omni] Bridge running. Press Ctrl+C to stop.'); |
There was a problem hiding this comment.
Replace the new console.log calls in this source file.
This adds console.log usage under src/**, which violates the repo rule and will keep the file out of policy until those writes go through the existing logging/output abstraction.
As per coding guidelines, src/**/*.ts: No console.log in source code (enforced by biome, relaxed in tests).
Also applies to: 52-53, 68-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/term-commands/omni.ts` at line 31, Replace all occurrences of console.log
in this file (e.g., the call that prints "[genie omni] Bridge running. Press
Ctrl+C to stop." and the other console.log calls around lines referenced 52-53
and 68-95) with the project's logging/output abstraction (use the existing
logger or output utility used elsewhere in the repo, e.g., logger.info or
output.write) so these messages go through centralized logging; keep the same
message text and log level context (informational) and preserve any dynamic data
or newlines. Locate each console.log call by searching for the exact string
"[genie omni]" and replace it with the appropriate logger method, ensuring
imports for the logging utility are added if missing and remove the console
import/usages. Ensure formatting and return behavior remain unchanged.
Comment on lines
+45
to
+69
| omni | ||
| .command('stop') | ||
| .description('Stop the running NATS bridge') | ||
| .action(async () => { | ||
| const { getBridge } = await import('../services/omni-bridge.js'); | ||
| const bridge = getBridge(); | ||
| if (!bridge) { | ||
| console.log('No running bridge found in this process.'); | ||
| console.log('If the bridge is running in another terminal, use Ctrl+C to stop it.'); | ||
| return; | ||
| } | ||
| await bridge.stop(); | ||
| }); | ||
|
|
||
| omni | ||
| .command('status') | ||
| .description('Show bridge status: active sessions, queue depth, idle timers') | ||
| .option('--json', 'Output as JSON') | ||
| .action(async (options) => { | ||
| const { getBridge } = await import('../services/omni-bridge.js'); | ||
| const bridge = getBridge(); | ||
|
|
||
| if (!bridge) { | ||
| console.log('Bridge is not running in this process.'); | ||
| return; |
There was a problem hiding this comment.
omni stop / omni status only work inside the same process as omni start.
Both commands call getBridge(), but the helper in src/services/omni-bridge.ts, Lines 70-72, only returns an in-memory singleton for the current process. A fresh genie omni stop or genie omni status invocation will always report "not running" even while the bridge is active in another terminal. This needs a pid/socket/daemon boundary instead of process-local state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/term-commands/omni.ts` around lines 45 - 69, The stop/status commands
currently rely solely on the process-local getBridge() in
src/services/omni-bridge.ts which only returns an in-memory singleton, so
separate CLI invocations can't control a bridge started in another terminal;
change the behavior so getBridge() (or a new helper like getRemoteBridgeClient)
first attempts to contact a daemon boundary (e.g., check a PID file and connect
to a Unix domain socket or TCP control port) and only falls back to the
in-process singleton, and update the omni.command('stop') and
omni.command('status') handlers to use that remote client when getBridge()
returns null (calling remote stop/status RPC or sending the appropriate signal)
so genie omni stop/status work across processes rather than only in-process.
The monolithic /brain skill (284 lines) is replaced by 9 focused skills that install from the genie-brain npm package into ~/.claude/skills/: /brain — router (classifies intent, delegates to sub-skills) /brain-search — confidence-scored search with strategy selection /brain-ingest — multimodal content processing pipeline /brain-health — 7-dimension quality audit /brain-learn — agent learning memory management /brain-build — end-to-end KB builder /brain-init — brain setup guide /brain-observe — gap analysis and strategy tuning /brain-analyze — deep reasoning via rlmx Skills auto-install via `genie brain install-skills` or postinstall hook. No code in genie CLI references skills/brain/ — zero breaking change.
chore: remove /brain skill — now ships with @automagik/genie-brain
… enforcement Add new `claude-sdk` executor provider using `@anthropic-ai/claude-agent-sdk` for programmatic agent execution with per-agent tool permission control via `canUseTool` callback — 3-layer defense in depth (tools visibility, disallowedTools blacklist, canUseTool runtime gate). New files: - providers/claude-sdk.ts: ExecutorProvider using SDK query() in-process - providers/claude-sdk-permissions.ts: permission gate factory + 3 presets - executors/claude-sdk.ts: IExecutor for omni-bridge with session resume - 44 tests across 3 test files Modified: - provider-adapters.ts: added 'claude-sdk' to ProviderName - agent-directory.ts: added permissions field to DirectoryEntry - agents.ts: wired launchSdkSpawn() flow for --provider claude-sdk - omni-bridge.ts: --executor sdk flag for SDK-based sessions - registry.ts: registered ClaudeSdkProvider - package.json: added SDK dep + external build flag Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…rontmatter + sync pipeline Extend AGENTS.md frontmatter to support the `sdk:` configuration block as a permissive record, and wire it through the agent-sync pipeline to persist in PG metadata. Adds GIN index for efficient SDK-specific queries.
…persistence Add SdkDirectoryConfig types covering all serializable Claude Agent SDK Options, and wire the new `sdk` field through buildMetadata/roleToEntry so it round-trips through PG JSONB storage.
Route all 24 SDKMessage types to structured audit events via fire-and-forget calls in the SDK provider's message stream.
Extract per-category detail builders and use lookup maps instead of nested switch statements to satisfy linter complexity threshold.
Keep SdkMcpStdioServerConfig, SdkMcpSSEServerConfig, and SdkMcpHttpServerConfig as file-scoped interfaces since they are only used within the SdkMcpServerConfig union type. Fixes knip dead-code.
Add streaming output pipeline for claude-sdk provider with text, JSON, and NDJSON format support. When --stream is passed, messages are written to stdout as they arrive with includePartialMessages enabled for live typing effect.
TypeScript parser rejects split import() type assertions across lines. Extract a local type alias instead.
…ration Wire all SdkDirectoryConfig options as CLI flags on genie dir add and genie dir edit, enabling users to configure SDK agents entirely from the command line. Includes buildSdkConfig parser, printSdkConfig display, and comprehensive tests for parsing (thinking, MCP servers, subagents).
The runQuery call in agents.ts passes 5 arguments but the method only accepted 4. Add the optional 5th parameter to unblock typecheck.
Remove sdkConfig and runtimeExtraOptions params from runSdkQuery and launchSdkSpawn to avoid dependency on uncommitted claude-sdk.ts changes from another group. The streaming pipeline only needs the 4-arg runQuery signature that is currently committed.
…tion - Add sdkConfig + runtimeExtraOptions params to runSdkQuery and launchSdkSpawn - Add --sdk-max-turns, --sdk-max-budget, --sdk-stream, --sdk-effort to SpawnOptions - Wire spawn call site: directory sdkConfig forwarded to provider, CLI flags as runtime overrides - Provider uses translateSdkConfig() for proper type translation, mergeHooks() for hook composition - Priority layering: directory config (base) → translated SDK → runtime extras (highest)
…DK flags - Add sdkConfig + runtimeExtraOptions params to runSdkQuery and launchSdkSpawn - Add sdkMaxTurns, sdkMaxBudget, sdkStream, sdkEffort to SpawnOptions interface - Wire handleWorkerSpawn: directory sdkConfig forwarded to provider, CLI flags as runtime overrides - Add --sdk-max-turns, --sdk-max-budget, --sdk-stream, --sdk-effort flags to agent spawn command - Priority layering: directory config (base) → translated SDK → runtime extras (highest)
The SDK-specific flags (--sdk-max-turns, --sdk-max-budget, --sdk-stream, --sdk-effort) were added to agent/spawn.ts but missing from the top-level genie spawn command in genie.ts.
…line 7 scenarios: config roundtrip, provider runQuery with layered options, event routing completeness (all 24+ SDKMessage types), stream formatting (text/json/ndjson), permission gate, frontmatter-to-Options pipeline, and config priority layering (extraOptions > sdkConfig).
- Create frontmatter-writer.ts with writeFrontmatter() for merging YAML updates into AGENTS.md while preserving unknown fields and markdown body - Add serializeSdkConfig() to convert SDK config to clean YAML objects - Update agent-directory.ts edit() to sync frontmatter-relevant fields back to AGENTS.md on disk after PG update - Add genie dir export <name> command to print full AGENTS.md frontmatter from PG state as YAML - Add comprehensive test suite (8 tests) covering write, roundtrip, unknown field preservation, and SDK serialization
Move syncFrontmatterToDisk() call from agent-directory edit() to the dir.ts handleEdit handler. This preserves AGENTS.md as source-of-truth during agent-sync while enabling bidirectional sync for CLI dir edits. Export syncFrontmatterToDisk() so dir.ts can call it after edit().
…s test files bun's mock.module persists globally — incomplete audit mocks caused getActor/queryAuditEvents/etc to be undefined in audit.test.ts when it ran after our SDK tests. Adding all exported functions to the mock prevents the leak.
… files
bun's mock.module and spyOn persist globally within a test process,
causing audit.test.ts to fail when run alongside SDK tests. Removed
all audit mocking from events and integration tests — routeSdkMessage
is fire-and-forget (.catch(() => {})) so audit errors never propagate.
context-enrichment test calls `which rlmx` + potentially runs a query, db-backup test calls pg_dump which waits on TCP connection. Both exceed default 5s/15s timeouts in environments without rlmx/pgserve. Bumped to 30s to prevent false CI failures.
Covers: quick start, AGENTS.md config reference, CLI commands, config priority layering, permission model, streaming output, event routing, architecture overview, SDK coverage report with gap analysis vs Claude Agent SDK v0.2.92, troubleshooting, and 4 recipe examples (analyzer, researcher, budget-constrained, JSON schema).
bun pre-analyzes require() calls in ESM modules, which caused
cascade module resolution failures in CI ('contractPath not found'
in genie-config.ts). Convert to top-level ESM imports of
writeFrontmatter and serializeSdkConfig from frontmatter-writer.
feat(sdk): full Claude Agent SDK executor with config pipeline
Previous version 4.260403.4 already published. Bump to trigger @next release with SDK executor changes from PR #1033.
chore: bump version to 4.260404.1
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/frontmatter.ts (1)
22-35:⚠️ Potential issue | 🟠 Major
claude-sdkis accepted here, but not fully handled in all spawn routers.This schema change enables
provider: claude-sdk, butsrc/lib/protocol-router-spawn.tsstill routes non-codexproviders totmux. That can sendclaude-sdkagents down the wrong executor transport path in protocol routing flows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/frontmatter.ts` around lines 22 - 35, The AgentFrontmatterSchema now accepts provider value "claude-sdk" (providerValues and AgentFrontmatterSchema), but the spawn routing logic in protocol-router-spawn (the router that currently treats non-"codex" providers as tmux) doesn't handle "claude-sdk" and can route those agents incorrectly; update the routing logic in src/lib/protocol-router-spawn.ts to explicitly handle "claude-sdk" (either treat it like "codex" if it should use the same executor transport, or add a new transport branch for "claude-sdk"), and ensure any switch/case or conditional that checks provider names includes "claude-sdk" so agents using AgentFrontmatterSchema.provider are routed to the correct executor.
♻️ Duplicate comments (3)
src/term-commands/omni.ts (2)
51-71:⚠️ Potential issue | 🟠 Major
omni stop/statusare process-local and fail across CLI invocations.Both commands only inspect in-memory
getBridge()state. A bridge started in another process will be reported as not running, so operational control is broken from normal CLI usage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/term-commands/omni.ts` around lines 51 - 71, The stop/status commands currently only check in-process state via getBridge() (so omni.command('stop') and the 'status' action only work if the bridge is in the same process); change them to first attempt an external control path (e.g., connect to the bridge's IPC endpoint: Unix domain socket, TCP/HTTP control port, or read/write a PID/lock file and send a signal/RPC) to query status and request shutdown, and only fall back to calling getBridge() and bridge.stop() when no external control endpoint is available; update the actions that reference getBridge(), bridge.stop(), and the status output to perform the external RPC/IPC handshake (with timeout and clear error messages) and then use the in-process code path as a fallback.
33-33:⚠️ Potential issue | 🟠 MajorReplace
console.logcalls in source command code.These writes violate the source logging policy and should go through the project’s logger/output abstraction.
As per coding guidelines,
src/**/*.ts: Noconsole.login source code (enforced by biome, relaxed in tests).Also applies to: 54-56, 70-71, 77-77, 81-87, 90-90, 94-95, 97-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/term-commands/omni.ts` at line 33, Replace all direct console.* usages in src/term-commands/omni.ts (e.g., the "Bridge running. Press Ctrl+C to stop." console.log and the other console calls at the listed ranges) with the project's logger/output abstraction: import the shared logger or output module used elsewhere in the project and call the appropriate level (e.g., logger.info, logger.warn, logger.error) instead of console.log/console.error; remove or convert any console.debug/console.trace to the matching logger method so no console.* remains in omni.ts.src/services/omni-bridge.ts (1)
172-192:⚠️ Potential issue | 🟠 Major
stop()still leaves active sessions running.This path tears down timers and NATS, but it never calls
executor.shutdown()for the entries still tracked inthis.sessions. The bridge can reportStoppedwhile panes/SDK deliveries keep running in the background.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/omni-bridge.ts` around lines 172 - 192, The stop() implementation tears down timers and NATS but doesn't shut down active executors, leaving background deliveries running; iterate this.sessions (the Map entries used earlier), and for each entry if entry.executor exists call await entry.executor.shutdown() (handle/await promises and catch errors per entry), then clear entry references (idleTimer, executor) before unsubscribing/draining so all session executors are cleanly stopped prior to setting this.nc = null and bridgeInstance = null.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/sdk-executor-guide.md`:
- Around line 514-515: The diagram contains an encoding artifact "���
parseFrontmatter()" which breaks readability; replace the corrupted characters
with a normal arrow or label (for example "→ parseFrontmatter()" or "->
parseFrontmatter()") so the flow remains readable and keep the following node
text "AgentFrontmatter { sdk: Record<string, unknown> }" intact; update the
diagram text where "parseFrontmatter()" appears.
- Around line 327-330: The multiline shell example uses backslashes followed by
inline comments/spaces which breaks line continuation; update the example so
each continuation backslash is the last character on the line (remove trailing
comments/spaces) or move comments to separate lines above/below the flags;
specifically fix the lines containing --sdk-max-turns, --sdk-max-budget,
--sdk-effort, and --sdk-stream so the backslash remains the final character (or
remove backslashes and present the flags on one line) to ensure the command
copies and runs correctly.
In `@src/lib/agent-sync.ts`:
- Around line 267-270: The JSON.stringify-based comparison for sdkChanged
(comparing existing.sdk and sdkConfig) can produce false positives due to
key-order differences; replace it with an order-insensitive deep equality check
(e.g., use a canonical/stable stringify or a deepEqual utility such as
lodash.isEqual or fast-deep-equal) so sdkChanged reflects true structural
differences, and update the sdkChanged assignment in agent-sync.ts (the
existing.sdk vs sdkConfig comparison) to use that deep-equal/canonicalization
approach before computing needsUpdate.
In `@src/lib/frontmatter-writer.ts`:
- Around line 96-117: The splitFrontmatter function's regex requires a newline
before the closing '---' and can miss frontmatter blocks that have the closing
delimiter immediately after YAML content; update the regex used in
splitFrontmatter to be more permissive (use multiline mode and allow optional
newline/CR before the closing '---' and optional trailing newline after it) so
it matches cases like '---\\n<yaml>---\\n' and Windows line endings; adjust the
pattern invocation to include the 'm' flag and make the newline before the
closing delimiter optional while still capturing the YAML block into yamlStr and
the rest into body.
- Around line 19-29: WritableFrontmatter includes a mutable name field which
risks accidental agent identity corruption; either remove name from the
WritableFrontmatter interface or enforce validation in writeFrontmatter() to
reject/ignore any updates to name. Locate the WritableFrontmatter type and
remove the name?: string declaration, or alternatively update writeFrontmatter()
to explicitly strip/validate incoming payloads and throw or ignore attempts to
change name (mirroring the exclusion logic in syncFrontmatterToDisk's
fmUpdates); ensure tests and callers that rely on name being immutable are
updated to use the agent's canonical identity source instead.
In `@src/lib/provider-adapters.ts`:
- Line 21: ProviderName includes 'app-pty' but spawnParamsSchema and its runtime
validation/error message do not accept or list 'app-pty', causing type-safe
callers to fail at runtime; update the spawnParamsSchema (and any
validation/error message logic that builds the allowed-provider list) to
explicitly accept 'app-pty' (or remove it from ProviderName if intentionally
unsupported) and ensure the error message lists all values from ProviderName;
update any exported type usages referencing ProviderName to avoid breaking
changes.
In `@src/lib/providers/__tests__/claude-sdk-events.test.ts`:
- Around line 174-176: Remove the no-op beforeEach in the 'claude-sdk-events'
test suite: delete the empty beforeEach(() => {}) inside the
describe('claude-sdk-events', ...) block (or replace it with a one-line comment
if it was intended as a placeholder) so there are no empty lifecycle hooks in
the test file.
In `@src/lib/providers/claude-sdk-stream.ts`:
- Around line 85-105: In formatResult, calling success.total_cost_usd.toFixed(4)
can produce NaN if total_cost_usd is undefined; update formatResult to coerce or
default total_cost_usd to 0 (e.g., use (success.total_cost_usd ?? 0) or
Number(...) fallback) before calling toFixed, and ensure usage fields are
accessed safely (usage?.input_tokens / usage?.output_tokens) to avoid undefined
token output in the Turns/Cost/Tokens line.
In `@src/lib/providers/claude-sdk.ts`:
- Around line 64-72: The function translateSdkConfig is dropping explicit false
values because it uses truthy checks; update the assignments for
enableFileCheckpointing, includePartialMessages, promptSuggestions, and
agentProgressSummaries (and any other boolean flags) to test for undefined (e.g.
if (typeof sdkConfig.enableFileCheckpointing !== 'undefined') or use the "in"
operator) before copying into opts so false is preserved; locate these fields in
translateSdkConfig (symbols: enableFileCheckpointing, includePartialMessages,
promptSuggestions, agentProgressSummaries) and replace the truthy checks with
existence checks that allow false.
In `@src/lib/sdk-directory-types.ts`:
- Around line 27-32: The inline JSDoc for the "enabled" variant is awkwardly
placed inside the union type; move the comment(s) out of the trailing inline
position and place them immediately above the variant and above the budgetTokens
property for clarity—i.e., update the union variant that currently reads { /**
Fixed thinking token budget (older models). */ type: 'enabled' ...
budgetTokens?: number; } so the JSDoc for the variant sits on its own line above
the object and the JSDoc for budgetTokens (if needed) sits directly above
budgetTokens?: number; to improve readability in sdk-directory-types.ts.
- Around line 42-71: The three MCP server config interfaces
SdkMcpStdioServerConfig, SdkMcpSSEServerConfig, and SdkMcpHttpServerConfig are
currently unexported but are referenced by exported types (e.g.,
SdkSubagentConfig); export them so consumers can explicitly type variables.
Update the declarations for SdkMcpStdioServerConfig, SdkMcpSSEServerConfig, and
SdkMcpHttpServerConfig to be exported (export interface ...) so tests/builders
and external code can import and reuse these exact types.
In `@src/services/omni-bridge.ts`:
- Around line 263-268: When entry.spawning is true and you hit the
MAX_BUFFER_PER_CHAT cap in the block that currently pushes to entry.buffer, do
not drop the incoming message; instead detect the full-buffer condition
(entry.buffer.length >= MAX_BUFFER_PER_CHAT) and either (a) immediately send an
overload/system reply back to the sender indicating the session is busy (use the
same message delivery path that sends user/system replies), or (b) push the
message to a global overflow/retry queue to be re-attempted once spawning
finishes; update the logic around entry.spawning, entry.buffer,
MAX_BUFFER_PER_CHAT and the message handling path so every incoming message is
either buffered, queued for retry, or acknowledged with an overload response
rather than being silently discarded.
In `@src/term-commands/agent/spawn.ts`:
- Around line 29-30: The CLI currently uses Number as the parser for
--sdk-max-turns and --sdk-max-budget but only null-checks before passing values
to the SDK, allowing NaN through; update the action handler in spawn.ts to
validate these parsed numeric options (sdkMaxTurns and sdkMaxBudget) with
Number.isFinite (or isFinite after coercion) and only forward them when valid
numbers, otherwise treat them as undefined or throw a clear CLI error; locate
the CLI options parsing and the action handler that reads options.sdkMaxTurns /
options.sdkMaxBudget and add the finite-number guard before constructing the
runtime/SDK config.
In `@src/term-commands/agents.ts`:
- Around line 975-1011: Wrap the SDK streaming and non-streaming
message-processing loops in a single try/catch/finally so cleanup always runs:
move the worker/executor cleanup logic currently executed only on successful
completion (from launchSdkSpawn/related cleanup code) into the finally block,
and in the catch set the executor/worker state to "error" before rethrowing the
error so failures don't leave a spawning worker behind; apply the same pattern
for both the streaming branch (where formatSdkMessage/messages are iterated) and
the non-streaming branch (where you iterate messages and inspect
message.type/result).
- Around line 1003-1005: Replace the new console.log calls (e.g., in the handler
checking message.type === 'result' && message.subtype === 'success' and the
other occurrences around the message handling at 1032-1045) with the project's
CLI/output abstraction rather than using console.log; locate the existing output
helper used elsewhere in this file (or the repo) and call that helper with
message.result (or the formatted text) so all output goes through the
established CLI/output layer and avoids direct console.log usage.
- Around line 1021-1030: The code registers SDK workers with a hardcoded pane id
'sdk', which breaks later liveness checks that use isPaneAlive(w.paneId); change
registerSpawnWorker so it receives a real, unique pane id tied to the
executor/process instead of the constant 'sdk' (for example use ctx.executorId
or the actual tmux pane id created when spawning the worker), and ensure the
paneId stored on the executor (created via createAndLinkExecutor) matches the
value passed to registerSpawnWorker so isPaneAlive(paneId) can correctly detect
live SDK-backed workers and prevent duplicate spawns.
In `@src/term-commands/dir.ts`:
- Around line 556-561: parseThinkingConfig currently converts the budget with
Number(value.slice('enabled:'.length)) which yields NaN for invalid input like
"enabled:abc"; validate the parsed budget before returning by parsing the slice
(e.g., parseInt or Number), and check Number.isFinite(parsed) &&
!Number.isNaN(parsed) (and optionally that it's >= 0) and throw the same Invalid
--sdk-thinking Error if invalid; update the branch that returns { type:
'enabled', budgetTokens: budget } to only do so after this validation so
parseThinkingConfig (and the local value variable) never returns NaN
budgetTokens.
- Around line 489-490: options.sdkMaxTurns and options.sdkMaxBudget are being
converted with Number(...) which yields NaN for non-numeric input and silently
corrupts config.maxTurns and config.maxBudgetUsd; update the handling in the
block that sets config.maxTurns and config.maxBudgetUsd to validate the inputs
(use parseInt/parseFloat or Number), check isNaN on the result, and either
reject/throw a clear error or skip assignment (and log a user-facing message)
when the value is not a valid number; reference options.sdkMaxTurns,
options.sdkMaxBudget, config.maxTurns and config.maxBudgetUsd when making the
change so the validation is applied exactly where these assignments occur.
In `@src/term-commands/omni.ts`:
- Around line 25-27: Validate CLI numeric options before passing them into
OmniBridge: parse and sanitize options.maxConcurrent and options.idleTimeout
(e.g., use Number(...) then check Number.isFinite / Number.isInteger and
non-negative) and if invalid replace with the intended defaults instead of
letting NaN through; then pass the sanitized values to the OmniBridge
constructor (fields maxConcurrent and idleTimeoutMs). Also ensure the
executorType cast (options.executor) is validated to one of 'tmux'|'sdk' before
casting. Reference: options.maxConcurrent, options.idleTimeout, maxConcurrent,
idleTimeoutMs, OmniBridge, and executorType.
---
Outside diff comments:
In `@src/lib/frontmatter.ts`:
- Around line 22-35: The AgentFrontmatterSchema now accepts provider value
"claude-sdk" (providerValues and AgentFrontmatterSchema), but the spawn routing
logic in protocol-router-spawn (the router that currently treats non-"codex"
providers as tmux) doesn't handle "claude-sdk" and can route those agents
incorrectly; update the routing logic in src/lib/protocol-router-spawn.ts to
explicitly handle "claude-sdk" (either treat it like "codex" if it should use
the same executor transport, or add a new transport branch for "claude-sdk"),
and ensure any switch/case or conditional that checks provider names includes
"claude-sdk" so agents using AgentFrontmatterSchema.provider are routed to the
correct executor.
---
Duplicate comments:
In `@src/services/omni-bridge.ts`:
- Around line 172-192: The stop() implementation tears down timers and NATS but
doesn't shut down active executors, leaving background deliveries running;
iterate this.sessions (the Map entries used earlier), and for each entry if
entry.executor exists call await entry.executor.shutdown() (handle/await
promises and catch errors per entry), then clear entry references (idleTimer,
executor) before unsubscribing/draining so all session executors are cleanly
stopped prior to setting this.nc = null and bridgeInstance = null.
In `@src/term-commands/omni.ts`:
- Around line 51-71: The stop/status commands currently only check in-process
state via getBridge() (so omni.command('stop') and the 'status' action only work
if the bridge is in the same process); change them to first attempt an external
control path (e.g., connect to the bridge's IPC endpoint: Unix domain socket,
TCP/HTTP control port, or read/write a PID/lock file and send a signal/RPC) to
query status and request shutdown, and only fall back to calling getBridge() and
bridge.stop() when no external control endpoint is available; update the actions
that reference getBridge(), bridge.stop(), and the status output to perform the
external RPC/IPC handshake (with timeout and clear error messages) and then use
the in-process code path as a fallback.
- Line 33: Replace all direct console.* usages in src/term-commands/omni.ts
(e.g., the "Bridge running. Press Ctrl+C to stop." console.log and the other
console calls at the listed ranges) with the project's logger/output
abstraction: import the shared logger or output module used elsewhere in the
project and call the appropriate level (e.g., logger.info, logger.warn,
logger.error) instead of console.log/console.error; remove or convert any
console.debug/console.trace to the matching logger method so no console.*
remains in omni.ts.
🪄 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: bcb421a4-3cf4-4f64-b53b-204cd3b8931e
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (34)
docs/sdk-executor-guide.mdknip.jsonpackage.jsonsrc/__tests__/sdk-integration.test.tssrc/db/migrations/025_sdk_metadata_index.sqlsrc/genie.tssrc/lib/agent-directory.test.tssrc/lib/agent-directory.tssrc/lib/agent-sync.tssrc/lib/context-enrichment.test.tssrc/lib/db-backup.test.tssrc/lib/frontmatter-writer.test.tssrc/lib/frontmatter-writer.tssrc/lib/frontmatter.test.tssrc/lib/frontmatter.tssrc/lib/provider-adapters.tssrc/lib/providers/__tests__/claude-sdk-events.test.tssrc/lib/providers/__tests__/claude-sdk-permissions.test.tssrc/lib/providers/__tests__/claude-sdk-stream.test.tssrc/lib/providers/__tests__/claude-sdk.test.tssrc/lib/providers/claude-sdk-events.tssrc/lib/providers/claude-sdk-permissions.tssrc/lib/providers/claude-sdk-stream.tssrc/lib/providers/claude-sdk.tssrc/lib/providers/registry.tssrc/lib/sdk-directory-types.tssrc/services/executors/__tests__/claude-sdk.test.tssrc/services/executors/claude-sdk.tssrc/services/omni-bridge.tssrc/term-commands/agent/spawn.tssrc/term-commands/agents.tssrc/term-commands/dir.test.tssrc/term-commands/dir.tssrc/term-commands/omni.ts
Comment on lines
+327
to
+330
| --sdk-max-turns 10 \ # Override directory maxTurns | ||
| --sdk-max-budget 2.0 \ # Override directory maxBudgetUsd | ||
| --sdk-effort medium \ # Override directory effort | ||
| --sdk-stream # Enable streaming output |
There was a problem hiding this comment.
Broken multiline shell example will fail when copied.
The backslashes are followed by inline comments/spaces, so newline continuation is broken and the command won’t execute as intended.
Proposed doc fix
genie spawn my-agent \
- --sdk-max-turns 10 \ # Override directory maxTurns
- --sdk-max-budget 2.0 \ # Override directory maxBudgetUsd
- --sdk-effort medium \ # Override directory effort
- --sdk-stream # Enable streaming output
+ --sdk-max-turns 10 \
+ --sdk-max-budget 2.0 \
+ --sdk-effort medium \
+ --sdk-stream
+# Overrides: maxTurns, maxBudgetUsd, effort; enables streaming📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| --sdk-max-turns 10 \ # Override directory maxTurns | |
| --sdk-max-budget 2.0 \ # Override directory maxBudgetUsd | |
| --sdk-effort medium \ # Override directory effort | |
| --sdk-stream # Enable streaming output | |
| genie spawn my-agent \ | |
| --sdk-max-turns 10 \ | |
| --sdk-max-budget 2.0 \ | |
| --sdk-effort medium \ | |
| --sdk-stream | |
| # Overrides: maxTurns, maxBudgetUsd, effort; enables streaming |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/sdk-executor-guide.md` around lines 327 - 330, The multiline shell
example uses backslashes followed by inline comments/spaces which breaks line
continuation; update the example so each continuation backslash is the last
character on the line (remove trailing comments/spaces) or move comments to
separate lines above/below the flags; specifically fix the lines containing
--sdk-max-turns, --sdk-max-budget, --sdk-effort, and --sdk-stream so the
backslash remains the final character (or remove backslashes and present the
flags on one line) to ensure the command copies and runs correctly.
Comment on lines
+514
to
+515
| ��� parseFrontmatter() | ||
| AgentFrontmatter { sdk: Record<string, unknown> } |
There was a problem hiding this comment.
Corrupted character in architecture diagram.
��� parseFrontmatter() appears to be an encoding artifact and should be replaced with a normal arrow/label so the flow stays readable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/sdk-executor-guide.md` around lines 514 - 515, The diagram contains an
encoding artifact "��� parseFrontmatter()" which breaks readability; replace the
corrupted characters with a normal arrow or label (for example "→
parseFrontmatter()" or "-> parseFrontmatter()") so the flow remains readable and
keep the following node text "AgentFrontmatter { sdk: Record<string, unknown> }"
intact; update the diagram text where "parseFrontmatter()" appears.
Comment on lines
+267
to
+270
| // Check if any field actually changed (deep compare sdk via JSON serialization) | ||
| const sdkChanged = JSON.stringify(existing.sdk) !== JSON.stringify(sdkConfig); | ||
| const needsUpdate = | ||
| sdkChanged || |
There was a problem hiding this comment.
JSON.stringify compare can cause false-positive SDK updates.
Object key order differences (especially after JSONB round-trips) can mark unchanged configs as changed, causing repeated unnecessary edits.
Proposed fix
import { execSync } from 'node:child_process';
import { existsSync, watch as fsWatch, readFileSync, readdirSync, realpathSync } from 'node:fs';
import { join } from 'node:path';
+import { isDeepStrictEqual } from 'node:util';
@@
- const sdkChanged = JSON.stringify(existing.sdk) !== JSON.stringify(sdkConfig);
+ const sdkChanged = !isDeepStrictEqual(existing.sdk, sdkConfig);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Check if any field actually changed (deep compare sdk via JSON serialization) | |
| const sdkChanged = JSON.stringify(existing.sdk) !== JSON.stringify(sdkConfig); | |
| const needsUpdate = | |
| sdkChanged || | |
| // Check if any field actually changed (deep compare sdk via JSON serialization) | |
| const sdkChanged = !isDeepStrictEqual(existing.sdk, sdkConfig); | |
| const needsUpdate = | |
| sdkChanged || |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/agent-sync.ts` around lines 267 - 270, The JSON.stringify-based
comparison for sdkChanged (comparing existing.sdk and sdkConfig) can produce
false positives due to key-order differences; replace it with an
order-insensitive deep equality check (e.g., use a canonical/stable stringify or
a deepEqual utility such as lodash.isEqual or fast-deep-equal) so sdkChanged
reflects true structural differences, and update the sdkChanged assignment in
agent-sync.ts (the existing.sdk vs sdkConfig comparison) to use that
deep-equal/canonicalization approach before computing needsUpdate.
Comment on lines
+19
to
+29
| interface WritableFrontmatter { | ||
| name?: string; | ||
| description?: string; | ||
| model?: string; | ||
| color?: string; | ||
| promptMode?: string; | ||
| provider?: string; | ||
| tools?: string[]; | ||
| permissionMode?: string; | ||
| sdk?: Record<string, unknown>; | ||
| } |
There was a problem hiding this comment.
name in WritableFrontmatter creates risk of accidental identity corruption.
The interface includes name as a writable field, but overwriting name in frontmatter could corrupt agent identity. While syncFrontmatterToDisk in agent-directory.ts explicitly excludes name when building fmUpdates, direct calls to writeFrontmatter() bypass that safety. Consider removing name from the interface or adding validation that rejects name updates.
Proposed fix
interface WritableFrontmatter {
- name?: string;
description?: string;
model?: string;
color?: string;
promptMode?: string;
provider?: string;
tools?: string[];
permissionMode?: string;
sdk?: Record<string, unknown>;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| interface WritableFrontmatter { | |
| name?: string; | |
| description?: string; | |
| model?: string; | |
| color?: string; | |
| promptMode?: string; | |
| provider?: string; | |
| tools?: string[]; | |
| permissionMode?: string; | |
| sdk?: Record<string, unknown>; | |
| } | |
| interface WritableFrontmatter { | |
| description?: string; | |
| model?: string; | |
| color?: string; | |
| promptMode?: string; | |
| provider?: string; | |
| tools?: string[]; | |
| permissionMode?: string; | |
| sdk?: Record<string, unknown>; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/frontmatter-writer.ts` around lines 19 - 29, WritableFrontmatter
includes a mutable name field which risks accidental agent identity corruption;
either remove name from the WritableFrontmatter interface or enforce validation
in writeFrontmatter() to reject/ignore any updates to name. Locate the
WritableFrontmatter type and remove the name?: string declaration, or
alternatively update writeFrontmatter() to explicitly strip/validate incoming
payloads and throw or ignore attempts to change name (mirroring the exclusion
logic in syncFrontmatterToDisk's fmUpdates); ensure tests and callers that rely
on name being immutable are updated to use the agent's canonical identity source
instead.
Comment on lines
+96
to
+117
| function splitFrontmatter(content: string): { yamlObj: Record<string, unknown>; body: string } { | ||
| const match = content.match(/^---\n([\s\S]*?)\n---\n?([\s\S]*)$/); | ||
| if (!match) { | ||
| // No frontmatter — entire content becomes body | ||
| return { yamlObj: {}, body: content }; | ||
| } | ||
|
|
||
| const yamlStr = match[1]; | ||
| const body = match[2]; | ||
|
|
||
| let yamlObj: Record<string, unknown> = {}; | ||
| try { | ||
| const parsed = yaml.load(yamlStr); | ||
| if (typeof parsed === 'object' && parsed !== null) { | ||
| yamlObj = parsed as Record<string, unknown>; | ||
| } | ||
| } catch { | ||
| // Malformed YAML — start fresh | ||
| } | ||
|
|
||
| return { yamlObj, body }; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Frontmatter regex may miss edge cases.
The regex ^---\n([\s\S]*?)\n---\n?([\s\S]*)$ requires a newline before the closing ---. Files with ---\n<yaml>---\n (no trailing newline before closing delimiter) won't match. This is likely fine for most YAML generators, but worth noting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/frontmatter-writer.ts` around lines 96 - 117, The splitFrontmatter
function's regex requires a newline before the closing '---' and can miss
frontmatter blocks that have the closing delimiter immediately after YAML
content; update the regex used in splitFrontmatter to be more permissive (use
multiline mode and allow optional newline/CR before the closing '---' and
optional trailing newline after it) so it matches cases like
'---\\n<yaml>---\\n' and Windows line endings; adjust the pattern invocation to
include the 'm' flag and make the newline before the closing delimiter optional
while still capturing the YAML block into yamlStr and the rest into body.
Comment on lines
+1003
to
+1005
| if (message.type === 'result' && message.subtype === 'success' && message.result) { | ||
| console.log(message.result); | ||
| } |
There was a problem hiding this comment.
Avoid the new console.log calls in src/**.
These additions violate the repo rule for source files and are likely to trip biome on this path. Route them through the CLI/output abstraction instead. As per coding guidelines, src/**/*.ts: No console.log in source code (enforced by biome, relaxed in tests).
Also applies to: 1032-1045
🤖 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 1003 - 1005, Replace the new
console.log calls (e.g., in the handler checking message.type === 'result' &&
message.subtype === 'success' and the other occurrences around the message
handling at 1032-1045) with the project's CLI/output abstraction rather than
using console.log; locate the existing output helper used elsewhere in this file
(or the repo) and call that helper with message.result (or the formatted text)
so all output goes through the established CLI/output layer and avoids direct
console.log usage.
Comment on lines
+1021
to
+1030
| if (ctx.agentIdentityId && ctx.executorId) { | ||
| await createAndLinkExecutor(ctx.agentIdentityId, 'claude-sdk' as ProviderName, 'process', { | ||
| id: ctx.executorId, | ||
| claudeSessionId: null, | ||
| state: 'spawning', | ||
| repoPath: ctx.cwd, | ||
| }); | ||
| } | ||
|
|
||
| await registerSpawnWorker(ctx, 'sdk'); |
There was a problem hiding this comment.
A fake 'sdk' pane ID gives later liveness checks nothing real to probe.
Later in this file, duplicate prevention and status reporting decide whether a worker is live via isPaneAlive(w.paneId). Registering SDK workers with the constant 'sdk' means those tmux-based checks can misclassify a live process-backed worker, which can allow duplicate spawns and underreport active SDK sessions.
🤖 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 1021 - 1030, The code registers SDK
workers with a hardcoded pane id 'sdk', which breaks later liveness checks that
use isPaneAlive(w.paneId); change registerSpawnWorker so it receives a real,
unique pane id tied to the executor/process instead of the constant 'sdk' (for
example use ctx.executorId or the actual tmux pane id created when spawning the
worker), and ensure the paneId stored on the executor (created via
createAndLinkExecutor) matches the value passed to registerSpawnWorker so
isPaneAlive(paneId) can correctly detect live SDK-backed workers and prevent
duplicate spawns.
Comment on lines
+489
to
+490
| if (options.sdkMaxTurns !== undefined) config.maxTurns = Number(options.sdkMaxTurns); | ||
| if (options.sdkMaxBudget !== undefined) config.maxBudgetUsd = Number(options.sdkMaxBudget); |
There was a problem hiding this comment.
Number() on non-numeric strings produces NaN.
If the user passes --sdk-max-turns abc, Number('abc') returns NaN, which silently corrupts the config. Consider validation.
Proposed fix
- if (options.sdkMaxTurns !== undefined) config.maxTurns = Number(options.sdkMaxTurns);
- if (options.sdkMaxBudget !== undefined) config.maxBudgetUsd = Number(options.sdkMaxBudget);
+ if (options.sdkMaxTurns !== undefined) {
+ const n = Number(options.sdkMaxTurns);
+ if (Number.isNaN(n)) throw new Error(`Invalid --sdk-max-turns value: "${options.sdkMaxTurns}"`);
+ config.maxTurns = n;
+ }
+ if (options.sdkMaxBudget !== undefined) {
+ const n = Number(options.sdkMaxBudget);
+ if (Number.isNaN(n)) throw new Error(`Invalid --sdk-max-budget value: "${options.sdkMaxBudget}"`);
+ config.maxBudgetUsd = n;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/term-commands/dir.ts` around lines 489 - 490, options.sdkMaxTurns and
options.sdkMaxBudget are being converted with Number(...) which yields NaN for
non-numeric input and silently corrupts config.maxTurns and config.maxBudgetUsd;
update the handling in the block that sets config.maxTurns and
config.maxBudgetUsd to validate the inputs (use parseInt/parseFloat or Number),
check isNaN on the result, and either reject/throw a clear error or skip
assignment (and log a user-facing message) when the value is not a valid number;
reference options.sdkMaxTurns, options.sdkMaxBudget, config.maxTurns and
config.maxBudgetUsd when making the change so the validation is applied exactly
where these assignments occur.
Comment on lines
+556
to
+561
| if (value.startsWith('enabled:')) { | ||
| const budget = Number(value.slice('enabled:'.length)); | ||
| return { type: 'enabled', budgetTokens: budget }; | ||
| } | ||
| throw new Error(`Invalid --sdk-thinking value: "${value}". Expected adaptive|disabled|enabled[:budgetTokens].`); | ||
| } |
There was a problem hiding this comment.
Same NaN risk in parseThinkingConfig budget parsing.
Number(value.slice('enabled:'.length)) on enabled:abc produces NaN.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/term-commands/dir.ts` around lines 556 - 561, parseThinkingConfig
currently converts the budget with Number(value.slice('enabled:'.length)) which
yields NaN for invalid input like "enabled:abc"; validate the parsed budget
before returning by parsing the slice (e.g., parseInt or Number), and check
Number.isFinite(parsed) && !Number.isNaN(parsed) (and optionally that it's >= 0)
and throw the same Invalid --sdk-thinking Error if invalid; update the branch
that returns { type: 'enabled', budgetTokens: budget } to only do so after this
validation so parseThinkingConfig (and the local value variable) never returns
NaN budgetTokens.
Comment on lines
+25
to
+27
| maxConcurrent: Number(options.maxConcurrent), | ||
| idleTimeoutMs: Number(options.idleTimeout), | ||
| executorType: options.executor as 'tmux' | 'sdk', |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify parsing/validation flow for maxConcurrent and idleTimeoutMs.
rg -n -C3 'Number\(options\.maxConcurrent\)|Number\(options\.idleTimeout\)|maxConcurrent|idleTimeoutMs' src/term-commands/omni.ts src/services/omni-bridge.tsRepository: automagik-dev/genie
Length of output: 6562
Validate numeric CLI options before constructing OmniBridge.
Number(options.maxConcurrent) and Number(options.idleTimeout) accept invalid input as NaN. Since NaN is truthy, it bypasses the fallback defaults in the OmniBridge constructor. When NaN is used in comparisons (line 295: activeCount >= this.maxConcurrent, line 384: idleMs > this.idleTimeoutMs), all results are false, silently disabling concurrency limits and idle session cleanup.
Proposed validation
const bridge = new OmniBridge({
natsUrl: options.natsUrl,
- maxConcurrent: Number(options.maxConcurrent),
- idleTimeoutMs: Number(options.idleTimeout),
+ maxConcurrent: (() => {
+ const n = Number(options.maxConcurrent);
+ if (!Number.isFinite(n) || n <= 0) throw new Error('--max-concurrent must be a positive number');
+ return n;
+ })(),
+ idleTimeoutMs: (() => {
+ const n = Number(options.idleTimeout);
+ if (!Number.isFinite(n) || n < 0) throw new Error('--idle-timeout must be a non-negative number');
+ return n;
+ })(),
executorType: options.executor as 'tmux' | 'sdk',
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| maxConcurrent: Number(options.maxConcurrent), | |
| idleTimeoutMs: Number(options.idleTimeout), | |
| executorType: options.executor as 'tmux' | 'sdk', | |
| maxConcurrent: (() => { | |
| const n = Number(options.maxConcurrent); | |
| if (!Number.isFinite(n) || n <= 0) throw new Error('--max-concurrent must be a positive number'); | |
| return n; | |
| })(), | |
| idleTimeoutMs: (() => { | |
| const n = Number(options.idleTimeout); | |
| if (!Number.isFinite(n) || n < 0) throw new Error('--idle-timeout must be a non-negative number'); | |
| return n; | |
| })(), | |
| executorType: options.executor as 'tmux' | 'sdk', |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/term-commands/omni.ts` around lines 25 - 27, Validate CLI numeric options
before passing them into OmniBridge: parse and sanitize options.maxConcurrent
and options.idleTimeout (e.g., use Number(...) then check Number.isFinite /
Number.isInteger and non-negative) and if invalid replace with the intended
defaults instead of letting NaN through; then pass the sanitized values to the
OmniBridge constructor (fields maxConcurrent and idleTimeoutMs). Also ensure the
executorType cast (options.executor) is validated to one of 'tmux'|'sdk' before
casting. Reference: options.maxConcurrent, options.idleTimeout, maxConcurrent,
idleTimeoutMs, OmniBridge, and executorType.
namastex888
changed the title
Contributor
Author
|
@codex review this pr |
![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: c97de69f9a
ℹ️ 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".
Comment on lines
+225
to
+226
| const sdk = buildSdkConfig(options); | ||
| if (sdk) updates.sdk = sdk; |
There was a problem hiding this comment.
buildSdkConfig() only contains flags passed on the current command, but this assignment writes that partial object as the entire sdk payload. Because directory.edit() persists metadata.sdk as a full object, running genie dir edit with a single --sdk-* flag drops previously saved SDK fields (e.g., MCP servers, plugins, subagents). This causes silent config loss on normal incremental edits.
Useful? React with 👍 / 👎.
Comment on lines
+224
to
+225
| if [ -f "$SCRIPT_DIR/../src/services/omni-reply.ts" ]; then | ||
| echo "$PAYLOAD" | bun run "$SCRIPT_DIR/../src/services/omni-reply.ts" |
There was a problem hiding this comment.
The generated script is installed at ~/.genie/bin/omni-reply, but this fallback checks ../src/services/omni-reply.ts relative to that directory, which resolves to ~/.genie/src/services/omni-reply.ts and does not exist in normal installs. In environments without nats/natscli, every reply publish will fail with “no fallback available”, so Omni chats stop receiving agent responses.
Useful? React with 👍 / 👎.
Comment on lines
+337
to
+340
| } catch (err) { | ||
| console.error(`[omni-bridge] Failed to spawn session for ${key}:`, err); | ||
| this.sessions.delete(key); | ||
| } |
There was a problem hiding this comment.
On spawn failure, the catch block only deletes the placeholder session and returns. The triggering message is stored in placeholder.buffer, so deleting the entry here drops that inbound chat message with no retry, requeue, or auto-reply. Any transient spawn error (tmux/NATS/agent resolution) causes permanent message loss for that user message.
Useful? React with 👍 / 👎.
Comment on lines
+377
to
+379
| console.log(`[omni-bridge] Dead session detected: ${key}`); | ||
| this.removeSession(key); | ||
| continue; |
There was a problem hiding this comment.
When checkIdleSessions() removes a dead session, it frees capacity but does not call drainQueue(). Queued messages created by the max-concurrency gate can remain stuck even though slots are available, until some other code path happens to trigger queue draining. This can delay or starve chat delivery under crash/restart conditions.
Useful? React with 👍 / 👎.
This was referenced Apr 4, 2026
Closed
Closed
This was referenced Apr 4, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release v4.260404
Highlights
Claude Agent SDK Executor — Run agents in-process via
@anthropic-ai/claude-agent-sdkinstead of tmux. Full config pipeline from AGENTS.md frontmatter through PG to runtime.Omni Bridge — NATS-backed multi-agent chat routing with per-chat sessions, buffering, auto-replies, and idle management.
What's New
Claude Agent SDK (
claude-sdkprovider)provider: claude-sdkin AGENTS.md spawns agents via SDKquery()instead of tmux subprocesses--sdk-*CLI flags forgenie dir add/editandgenie spawncode-review,full-auto, etc.)--sdk-streamflag for real-time text/json/ndjson outputsdk:block ↔ PGagents.metadata->'sdk'JSONBdocs/sdk-executor-guide.md(839 lines) with recipes and config referenceOmni Bridge
Housekeeping
/brainskill removed — now ships separately via@automagik/genie-brainFiles Changed
QA Results (v4.260404.1 @next)
bun testbun run typecheckbun run lintbunx knipVerdict: SHIP
Zero CRITICAL/HIGH gaps. Full regression pass. QA'd on fresh
@nextinstall.