chore: rolling promotion dev -> main - #1116
Merged
Merged
Conversation
`genie log` now aggregates SDK agent events from the audit_events table, so SDK agents (NATS bridge / --executor sdk) get the same observability as CLI agents. - Add 'sdk' to RuntimeEventSource type - Add readSdkAuditEvents() querying audit_events by actor (agentId) - Map sdk.* event types to existing LogEventKind (no new kinds) - Wire into readAgentLog, readTeamLog, and follow mode (cursor-based poller) - Add [SDK] visual tag in log output to distinguish SDK vs CLI events - Add 12 new tests (unit + integration) for SDK event aggregation Closes #1101 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
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 (4)
📝 WalkthroughWalkthroughBumps package/plugin versions to 4.260409.8 and adds SDK audit event support: new runtime source 'sdk', DB reader/mapper for SDK audit rows, inclusion of SDK events in agent/team log readers and follow polling, plus related CLI, session persistence, test, and streaming adjustments. Changes
Sequence DiagramsequenceDiagram
participant Client
participant UnifiedLog as readAgentLog/readTeamLog
participant DB as audit_events DB
participant Mapper as sdkAuditRowToLogEvent
participant Follower as startPgFollow
participant Deduper as dedupAndEmit
Client->>UnifiedLog: request unified log (agentId/teamId)
par concurrent fetch
UnifiedLog->>UnifiedLog: fetch runtime events
and
UnifiedLog->>DB: query audit_events (entity_type='sdk_message', actor=agentId)
DB-->>UnifiedLog: audit rows
UnifiedLog->>Mapper: map rows -> LogEvent (source='sdk')
Mapper-->>UnifiedLog: SDK LogEvents
end
UnifiedLog->>Client: return combined events (sorted & filtered)
Note over Follower,DB: Background follow poller
Follower->>DB: poll new audit_events (cursor by max id)
DB-->>Follower: new rows
Follower->>Mapper: map -> LogEvent
Mapper->>Deduper: dedupAndEmit(mapped events)
Deduper-->>Client: streamed LogEvents
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 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 |
Tests for --append-system-prompt-file were reading the local genie config instead of using an explicit promptMode, causing failures when the local config has promptMode: "system". Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
feat(log): bridge SDK agent events into genie log
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/unified-log.ts`:
- Around line 240-247: The query currently uses `ORDER BY created_at ASC LIMIT
${limit}` which returns oldest rows first and breaks `filter.last`; change the
SQL in the `sql.unsafe(...)` call to `ORDER BY created_at DESC LIMIT ${limit}`
so the newest rows are selected, then reverse the resulting `rows` array before
passing it downstream (or into `applyLogFilter()`), ensuring the returned order
is ascending by created_at while keeping the source-side limit behavior.
- Around line 226-249: readSdkAuditEvents currently only filters by entity_type
and actor, which allows SDK audit rows to leak across repos/teams; update the
audit schema/payload to include a repository or team discriminator (e.g.,
repo_id or team_id) and ensure all SDK audit emits include that discriminator,
then modify readSdkAuditEvents to add a WHERE condition (append to conditions
and values) that filters by the incoming repo/team id from the LogFilter before
executing the sql. Also update any other SDK-audit read logic (the other SDK
read at the later block referenced) to apply the same repo/team filter and
ensure the AuditEventRow type and any emit/insert paths include the new
discriminator field so the DB queries can filter correctly.
- Around line 251-256: The loops in readAgentLog/readTeamLog (and the similar
loop later) drop rows when SDK_KIND_MAP[row.event_type] is falsy even though
sdkAuditRowToLogEvent already handles unknown types by mapping to "system";
remove the SDK_KIND_MAP conditional and always call sdkAuditRowToLogEvent(row)
and push its result into events (and do the same in the second occurrence) so
unmapped SDK event types fall back to system instead of being silently excluded.
In `@src/term-commands/msg.test.ts`:
- Around line 262-267: The test title says it verifies the default promptMode
path but passes promptMode:'append', so either (A) change this test to not pass
promptMode (remove promptMode from the options passed to buildTeamLeadCommand)
so it exercises the options?.promptMode ?? loadGenieConfigSync().promptMode
branch and still asserts '--append-system-prompt-file' and the file path, or (B)
add a new deterministic test that calls buildTeamLeadCommand('genie', {
systemPromptFile: '/tmp/test-agents.md' }) while mocking loadGenieConfigSync()
to return promptMode:'append' and then assert the flag and path; reference
buildTeamLeadCommand and loadGenieConfigSync when implementing the change.
🪄 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: 0f454c0e-250f-480c-9916-503739784047
📒 Files selected for processing (9)
.claude-plugin/marketplace.jsonpackage.jsonplugins/genie/.claude-plugin/plugin.jsonplugins/genie/package.jsonsrc/lib/runtime-events.tssrc/lib/unified-log.test.tssrc/lib/unified-log.tssrc/term-commands/log.tssrc/term-commands/msg.test.ts
Comment on lines
+226
to
+249
| async function readSdkAuditEvents(agentId: string, filter?: LogFilter): Promise<LogEvent[]> { | ||
| try { | ||
| if (!(await isAvailable())) return []; | ||
| const sql = await getConnection(); | ||
|
|
||
| const conditions = [`entity_type = 'sdk_message'`, 'actor = $1']; | ||
| const values: unknown[] = [agentId]; | ||
| let paramIdx = 2; | ||
|
|
||
| if (filter?.since) { | ||
| conditions.push(`created_at >= $${paramIdx++}::timestamptz`); | ||
| values.push(filter.since); | ||
| } | ||
|
|
||
| const where = `WHERE ${conditions.join(' AND ')}`; | ||
| const limit = filter?.last ?? 500; | ||
|
|
||
| const rows = (await sql.unsafe( | ||
| `SELECT id, entity_type, entity_id, event_type, actor, details, created_at | ||
| FROM audit_events ${where} | ||
| ORDER BY created_at ASC | ||
| LIMIT ${limit}`, | ||
| values, | ||
| )) as unknown as AuditEventRow[]; |
There was a problem hiding this comment.
SDK audit reads ignore repo/team scope and can leak events across workspaces.
These queries only filter on entity_type='sdk_message' and actor, so genie log for one repo/team will also pull SDK rows emitted by the same actor in other repos. Every other source in this aggregator is repo-scoped; this one is global. Please add a repo/team discriminator to the audit payload/schema and filter on it before merging or emitting.
Also applies to: 479-487
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/unified-log.ts` around lines 226 - 249, readSdkAuditEvents currently
only filters by entity_type and actor, which allows SDK audit rows to leak
across repos/teams; update the audit schema/payload to include a repository or
team discriminator (e.g., repo_id or team_id) and ensure all SDK audit emits
include that discriminator, then modify readSdkAuditEvents to add a WHERE
condition (append to conditions and values) that filters by the incoming
repo/team id from the LogFilter before executing the sql. Also update any other
SDK-audit read logic (the other SDK read at the later block referenced) to apply
the same repo/team filter and ensure the AuditEventRow type and any emit/insert
paths include the new discriminator field so the DB queries can filter
correctly.
Comment on lines
+240
to
+247
| const where = `WHERE ${conditions.join(' AND ')}`; | ||
| const limit = filter?.last ?? 500; | ||
|
|
||
| const rows = (await sql.unsafe( | ||
| `SELECT id, entity_type, entity_id, event_type, actor, details, created_at | ||
| FROM audit_events ${where} | ||
| ORDER BY created_at ASC | ||
| LIMIT ${limit}`, |
There was a problem hiding this comment.
The SDK source-side limit returns the oldest rows, not the latest ones.
ORDER BY created_at ASC LIMIT ${limit} makes filter.last wrong for SDK events, and after 500 SDK rows the recent ones disappear entirely from the aggregated log. Fetch the newest rows (DESC) and reverse them, or remove the source-side limit and let applyLogFilter() own last.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/unified-log.ts` around lines 240 - 247, The query currently uses
`ORDER BY created_at ASC LIMIT ${limit}` which returns oldest rows first and
breaks `filter.last`; change the SQL in the `sql.unsafe(...)` call to `ORDER BY
created_at DESC LIMIT ${limit}` so the newest rows are selected, then reverse
the resulting `rows` array before passing it downstream (or into
`applyLogFilter()`), ensuring the returned order is ascending by created_at
while keeping the source-side limit behavior.
Comment on lines
+251
to
+256
| const events: LogEvent[] = []; | ||
| for (const row of rows) { | ||
| // Only include event types we have a mapping for | ||
| if (SDK_KIND_MAP[row.event_type]) { | ||
| events.push(sdkAuditRowToLogEvent(row)); | ||
| } |
There was a problem hiding this comment.
Unmapped SDK event types are being dropped instead of falling back to system.
sdkAuditRowToLogEvent() already has a safe fallback for unknown event_types, but both readers still guard on SDK_KIND_MAP[row.event_type]. That means any new SDK event type is silently excluded from readAgentLog, readTeamLog, and follow mode instead of showing up as a system event.
Suggested fix
- const events: LogEvent[] = [];
- for (const row of rows) {
- // Only include event types we have a mapping for
- if (SDK_KIND_MAP[row.event_type]) {
- events.push(sdkAuditRowToLogEvent(row));
- }
- }
+ const events: LogEvent[] = rows.map((row) => sdkAuditRowToLogEvent(row));- for (const row of rows) {
- if (SDK_KIND_MAP[row.event_type]) {
- dedupAndEmit(sdkAuditRowToLogEvent(row));
- }
+ for (const row of rows) {
+ dedupAndEmit(sdkAuditRowToLogEvent(row));
sdkLastId = Math.max(sdkLastId, Number(row.id));
}Also applies to: 489-492
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/unified-log.ts` around lines 251 - 256, The loops in
readAgentLog/readTeamLog (and the similar loop later) drop rows when
SDK_KIND_MAP[row.event_type] is falsy even though sdkAuditRowToLogEvent already
handles unknown types by mapping to "system"; remove the SDK_KIND_MAP
conditional and always call sdkAuditRowToLogEvent(row) and push its result into
events (and do the same in the second occurrence) so unmapped SDK event types
fall back to system instead of being silently excluded.
Comment on lines
262
to
267
| test('includes --append-system-prompt-file when systemPromptFile provided (default promptMode)', async () => { | ||
| const { buildTeamLeadCommand } = await import('../lib/team-lead-command.js'); | ||
| const cmd = buildTeamLeadCommand('genie', { systemPromptFile: '/tmp/test-agents.md' }); | ||
| const cmd = buildTeamLeadCommand('genie', { systemPromptFile: '/tmp/test-agents.md', promptMode: 'append' }); | ||
| expect(cmd).toContain('--append-system-prompt-file'); | ||
| expect(cmd).toContain('/tmp/test-agents.md'); | ||
| }); |
There was a problem hiding this comment.
This test no longer exercises the default promptMode path.
Line 264 now forces promptMode: 'append', so the title is inaccurate and the options?.promptMode ?? loadGenieConfigSync().promptMode branch is no longer covered. Please either rename the case or add a separate deterministic test for the config-backed default.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/term-commands/msg.test.ts` around lines 262 - 267, The test title says it
verifies the default promptMode path but passes promptMode:'append', so either
(A) change this test to not pass promptMode (remove promptMode from the options
passed to buildTeamLeadCommand) so it exercises the options?.promptMode ??
loadGenieConfigSync().promptMode branch and still asserts
'--append-system-prompt-file' and the file path, or (B) add a new deterministic
test that calls buildTeamLeadCommand('genie', { systemPromptFile:
'/tmp/test-agents.md' }) while mocking loadGenieConfigSync() to return
promptMode:'append' and then assert the flag and path; reference
buildTeamLeadCommand and loadGenieConfigSync when implementing the change.
- processMessage: unified handler for streaming/non-streaming - Flat events: tool_call, tool_result, assistant as individual rows - Emits genie_session with PG session ID before streaming starts - Removed resultText accumulation (each assistant msg saved individually) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Bridge is now managed as a subprocess by genie serve: - Auto-starts after approval handler (step 6 in startup sequence) - Graceful shutdown via SIGTERM (detaches sessions, doesn't kill them) - Status shown in `genie serve status` output - `genie doctor` reports bridge health (NATS connection, sessions, PG backing) - `genie omni start` warns if bridge already managed by serve (use --standalone to force) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…sions Replace non-null assertions with narrowed locals in session ID persistence, add biome-ignore for pre-existing complexity in spawnSession and _processDelivery. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Track bridge sessions in dedicated genie_bridge_sessions table so they survive process restarts. Sessions are created on spawn, updated on message delivery, closed on removal, and orphaned on bridge startup. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Knip dead-code check flagged BridgeSessionRow and CreateSessionOpts as unused exports — they are only consumed within bridge-session-store.ts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- On startup: recover active sessions from PG, verify tmux panes alive, re-attach to surviving sessions, orphan dead ones - Graceful shutdown: SIGTERM detaches from tmux panes without killing, leaves PG rows as 'active' for recovery on restart - Duplicate guard: if two PG rows map to same agent:chat key, orphan the later one - SDK sessions (no tmux pane) are always orphaned since they can't survive restart Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extract the SDK `mock.module('@anthropic-ai/claude-agent-sdk')` registration
into `_shared-sdk-query-mock.ts` so both `sdk-integration.test.ts` and the
executor tests (`_sdk-mocks.ts`) use the same `queryMock` instance.
Previously, each file registered its own competing mock. Since bun's
`mock.module()` is process-global and first-registration-wins, whichever
file loaded first locked the global cache — making the other file's mock
assertions fail (0 calls). This caused 11 persistent test failures in the
full suite that passed individually.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
feat: flat session event recording + genie_session event
Tmux sessions are now detached (not shut down) during graceful stop so they can be recovered on restart. Updated test expectation accordingly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
feat: bridge session resilience — serve integration, PG persistence, graceful shutdown
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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/db/migrations/035_bridge_sessions.sql`:
- Around line 5-27: The schema allows multiple rows with status='active' for the
same logical session (instance_id, chat_id), which breaks recovery assumptions;
add a unique partial index to enforce at most one active row per session key by
creating a unique index on (instance_id, chat_id) WHERE status = 'active' (e.g.,
CREATE UNIQUE INDEX IF NOT EXISTS uniq_bridge_sessions_active_per_key ON
genie_bridge_sessions(instance_id, chat_id) WHERE status = 'active'); reference
the genie_bridge_sessions table and the existing status, instance_id, chat_id
columns when making this migration change.
In `@src/genie-commands/doctor.ts`:
- Around line 312-327: checkBridge currently assumes it runs inside the bridge
process by calling getBridge() which is null for the CLI; update checkBridge to
treat a null getBridge() as "not local" and perform a remote health probe
instead: keep the existing local-path logic using getBridge() to report status
when present, but when getBridge() is null call the bridge health endpoint
(e.g., HTTP GET to the configured bridge host/port or a known health path) and
base the CheckResult on that response; only return the "not running in this
process" warning if both the local getBridge() is null and the remote health
probe fails/unreachable. Ensure you reference checkBridge and getBridge in the
change and use the configured/default bridge address for the remote probe.
In `@src/services/__tests__/omni-bridge.test.ts`:
- Around line 533-535: Add a parallel assertion for the SDK shutdown path: when
invoking stop() in the test variant that takes the SDK branch, assert that
calls.shutdown was invoked (e.g., expect(calls.shutdown.length).toBe(1)) and
that the SDK-specific persisted-rows cleanup was executed (assert the mock/call
used for closing persisted rows was called). Locate the existing tmux assertion
around expect(calls.shutdown.length).toBe(0) and add a second assertion in the
SDK scenario that validates shutdown was called and the persisted-rows cleanup
mock was invoked so the new SDK cleanup path is covered.
In `@src/services/bridge-session-store.ts`:
- Around line 115-128: The list() method in BridgeSessionStore is truncating
results with a hard LIMIT 100 which breaks recoverSessions() (called with
status='active') and leaves older active rows stranded; update the list(status?:
...) implementation to return all matching rows instead of capping at 100 (or
implement pagination/cursoring if you must cap): remove the LIMIT 100 from the
SQL used when status is provided (and from the no-status branch if global
truncation is unintended), or implement a paginated loop in recoverSessions()
that repeatedly calls list() with an offset/last-id until all
genie_bridge_sessions rows (BridgeSessionRow) are processed so recoverSessions()
handles every active session.
In `@src/services/omni-bridge.ts`:
- Around line 1125-1133: Change removeSession to perform synchronous teardown by
making it async and awaiting the PG close before deleting the in-memory entry:
if entry?.idleTimer clear it, then if closeId and this.sessionStore call and
await the Promise returned by safePgCall (which should be adapted to return a
Promise) that runs new BridgeSessionStore(sql).close(closeId), and only after
that await resolve call this.sessions.delete(key). Update all callers (including
handleTurnTimeout and any reset/idle flows that currently delete the map entry
directly) to await this.removeSession(key) instead of deleting the entry,
ensuring the DB row is closed before a replacement session can be created.
In `@src/term-commands/omni.ts`:
- Around line 67-75: The current guard in the omni start action uses the
process-local getBridge() which cannot detect a bridge started by a separate
genie serve process; replace or augment that check with an inter-process
existence probe (e.g., try to connect to the bridge's known admin/health TCP
port or unix socket, or check a well-known PID/lock file that genie serve
writes) and only allow starting when that probe fails or when options.standalone
is true; update the omni start handler (the .action block that calls
getBridge()) to call a new helper like isBridgeRunningRemotely() (or reuse an
existing network/IPC connector) and use its result instead of getBridge() to
prevent starting a second bridge.
In `@src/term-commands/serve.ts`:
- Around line 614-618: The code currently fire-and-forgets OmniBridge.stop()
(when handles.omniBridge exists) which causes its async cleanup to be abandoned;
update shutdown() so it awaits handles.omniBridge.stop() and propagates/reports
errors instead of swallowing them, i.e. call await handles.omniBridge.stop() (or
Promise.resolve then await) and only then set handles.omniBridge = null; also
adjust the signal handlers that call shutdown() to await the returned promise
before calling process.exit to ensure OmniBridge.stop() finishes and its
NATS/SDK cleanup completes.
- Around line 885-899: printBridgeStatus currently calls getBridge() which only
returns a singleton in the current CLI process, so `genie serve status` can't
see a bridge running in the foreground/daemon; change printBridgeStatus to query
the running serve process over its management/status endpoint (or IPC socket)
instead of calling getBridge() locally — attempt an HTTP/IPC request to the
serve daemon's status API and parse the returned {connected, activeSessions,
queueDepth} (fall back to calling getBridge() only if the daemon endpoint is
unreachable) so that bridge.status() values reflect the actual running service
rather than the local singleton.
🪄 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: d1df89c7-4119-4fba-8884-2c8a136366bd
📒 Files selected for processing (7)
src/db/migrations/035_bridge_sessions.sqlsrc/genie-commands/doctor.tssrc/services/__tests__/omni-bridge.test.tssrc/services/bridge-session-store.tssrc/services/omni-bridge.tssrc/term-commands/omni.tssrc/term-commands/serve.ts
Comment on lines
+5
to
+27
| CREATE TABLE IF NOT EXISTS genie_bridge_sessions ( | ||
| id TEXT PRIMARY KEY DEFAULT gen_random_uuid()::text, | ||
| executor_id TEXT REFERENCES executors(id) ON DELETE SET NULL, | ||
| instance_id TEXT NOT NULL, | ||
| chat_id TEXT NOT NULL, | ||
| agent_name TEXT NOT NULL, | ||
| tmux_pane_id TEXT, | ||
| claude_session_id TEXT, | ||
| status TEXT NOT NULL DEFAULT 'active' | ||
| CHECK (status IN ('active', 'closed', 'orphaned')), | ||
| started_at TIMESTAMPTZ NOT NULL DEFAULT now(), | ||
| last_activity_at TIMESTAMPTZ NOT NULL DEFAULT now(), | ||
| closed_at TIMESTAMPTZ, | ||
| metadata JSONB DEFAULT '{}' | ||
| ); | ||
|
|
||
| CREATE INDEX IF NOT EXISTS idx_bridge_sessions_status | ||
| ON genie_bridge_sessions(status); | ||
| CREATE INDEX IF NOT EXISTS idx_bridge_sessions_instance_chat | ||
| ON genie_bridge_sessions(instance_id, chat_id); | ||
| CREATE INDEX IF NOT EXISTS idx_bridge_sessions_active | ||
| ON genie_bridge_sessions(status, last_activity_at) | ||
| WHERE status = 'active'; |
There was a problem hiding this comment.
Enforce one active row per bridge session key.
The new indexes speed lookups, but nothing here prevents two status='active' rows for the same logical session. The runtime already assumes singular active rows during recovery, so duplicate inserts will make restart recovery nondeterministic and keep stale sessions alive in PG.
Suggested migration
CREATE INDEX IF NOT EXISTS idx_bridge_sessions_active
ON genie_bridge_sessions(status, last_activity_at)
WHERE status = 'active';
+
+CREATE UNIQUE INDEX IF NOT EXISTS uniq_bridge_sessions_active_key
+ ON genie_bridge_sessions(instance_id, agent_name, chat_id)
+ WHERE status = 'active';📝 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
| CREATE TABLE IF NOT EXISTS genie_bridge_sessions ( | |
| id TEXT PRIMARY KEY DEFAULT gen_random_uuid()::text, | |
| executor_id TEXT REFERENCES executors(id) ON DELETE SET NULL, | |
| instance_id TEXT NOT NULL, | |
| chat_id TEXT NOT NULL, | |
| agent_name TEXT NOT NULL, | |
| tmux_pane_id TEXT, | |
| claude_session_id TEXT, | |
| status TEXT NOT NULL DEFAULT 'active' | |
| CHECK (status IN ('active', 'closed', 'orphaned')), | |
| started_at TIMESTAMPTZ NOT NULL DEFAULT now(), | |
| last_activity_at TIMESTAMPTZ NOT NULL DEFAULT now(), | |
| closed_at TIMESTAMPTZ, | |
| metadata JSONB DEFAULT '{}' | |
| ); | |
| CREATE INDEX IF NOT EXISTS idx_bridge_sessions_status | |
| ON genie_bridge_sessions(status); | |
| CREATE INDEX IF NOT EXISTS idx_bridge_sessions_instance_chat | |
| ON genie_bridge_sessions(instance_id, chat_id); | |
| CREATE INDEX IF NOT EXISTS idx_bridge_sessions_active | |
| ON genie_bridge_sessions(status, last_activity_at) | |
| WHERE status = 'active'; | |
| CREATE TABLE IF NOT EXISTS genie_bridge_sessions ( | |
| id TEXT PRIMARY KEY DEFAULT gen_random_uuid()::text, | |
| executor_id TEXT REFERENCES executors(id) ON DELETE SET NULL, | |
| instance_id TEXT NOT NULL, | |
| chat_id TEXT NOT NULL, | |
| agent_name TEXT NOT NULL, | |
| tmux_pane_id TEXT, | |
| claude_session_id TEXT, | |
| status TEXT NOT NULL DEFAULT 'active' | |
| CHECK (status IN ('active', 'closed', 'orphaned')), | |
| started_at TIMESTAMPTZ NOT NULL DEFAULT now(), | |
| last_activity_at TIMESTAMPTZ NOT NULL DEFAULT now(), | |
| closed_at TIMESTAMPTZ, | |
| metadata JSONB DEFAULT '{}' | |
| ); | |
| CREATE INDEX IF NOT EXISTS idx_bridge_sessions_status | |
| ON genie_bridge_sessions(status); | |
| CREATE INDEX IF NOT EXISTS idx_bridge_sessions_instance_chat | |
| ON genie_bridge_sessions(instance_id, chat_id); | |
| CREATE INDEX IF NOT EXISTS idx_bridge_sessions_active | |
| ON genie_bridge_sessions(status, last_activity_at) | |
| WHERE status = 'active'; | |
| CREATE UNIQUE INDEX IF NOT EXISTS uniq_bridge_sessions_active_key | |
| ON genie_bridge_sessions(instance_id, agent_name, chat_id) | |
| WHERE status = 'active'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/db/migrations/035_bridge_sessions.sql` around lines 5 - 27, The schema
allows multiple rows with status='active' for the same logical session
(instance_id, chat_id), which breaks recovery assumptions; add a unique partial
index to enforce at most one active row per session key by creating a unique
index on (instance_id, chat_id) WHERE status = 'active' (e.g., CREATE UNIQUE
INDEX IF NOT EXISTS uniq_bridge_sessions_active_per_key ON
genie_bridge_sessions(instance_id, chat_id) WHERE status = 'active'); reference
the genie_bridge_sessions table and the existing status, instance_id, chat_id
columns when making this migration change.
Comment on lines
+312
to
+327
| async function checkBridge(): Promise<CheckResult[]> { | ||
| const results: CheckResult[] = []; | ||
|
|
||
| try { | ||
| const { getBridge } = await import('../services/omni-bridge.js'); | ||
| const bridge = getBridge(); | ||
|
|
||
| if (!bridge) { | ||
| results.push({ | ||
| name: 'Bridge running', | ||
| status: 'warn', | ||
| message: 'not running in this process', | ||
| suggestion: 'Bridge starts automatically with: genie serve', | ||
| }); | ||
| return results; | ||
| } |
There was a problem hiding this comment.
This health check only works inside the bridge-owning process.
genie doctor runs in a separate CLI process, so getBridge() will usually be null even while genie serve has a healthy bridge running. That makes doctor report a warning on healthy installs instead of reporting real bridge state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/genie-commands/doctor.ts` around lines 312 - 327, checkBridge currently
assumes it runs inside the bridge process by calling getBridge() which is null
for the CLI; update checkBridge to treat a null getBridge() as "not local" and
perform a remote health probe instead: keep the existing local-path logic using
getBridge() to report status when present, but when getBridge() is null call the
bridge health endpoint (e.g., HTTP GET to the configured bridge host/port or a
known health path) and base the CheckResult on that response; only return the
"not running in this process" warning if both the local getBridge() is null and
the remote health probe fails/unreachable. Ensure you reference checkBridge and
getBridge in the change and use the configured/default bridge address for the
remote probe.
Comment on lines
+533
to
+535
| // Tmux sessions are detached (not shut down) during graceful stop | ||
| // so they can be recovered on restart. Shutdown is NOT called for tmux sessions. | ||
| expect(calls.shutdown.length).toBe(0); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add the SDK shutdown case alongside this tmux assertion.
This update covers the tmux-detach branch, but stop() now has separate SDK behavior and closes persisted rows only there. Without an SDK-specific test, that new cleanup path can regress unnoticed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/__tests__/omni-bridge.test.ts` around lines 533 - 535, Add a
parallel assertion for the SDK shutdown path: when invoking stop() in the test
variant that takes the SDK branch, assert that calls.shutdown was invoked (e.g.,
expect(calls.shutdown.length).toBe(1)) and that the SDK-specific persisted-rows
cleanup was executed (assert the mock/call used for closing persisted rows was
called). Locate the existing tmux assertion around
expect(calls.shutdown.length).toBe(0) and add a second assertion in the SDK
scenario that validates shutdown was called and the persisted-rows cleanup mock
was invoked so the new SDK cleanup path is covered.
Comment on lines
+115
to
+128
| async list(status?: 'active' | 'closed' | 'orphaned'): Promise<BridgeSessionRow[]> { | ||
| if (status) { | ||
| return this.sql<BridgeSessionRow[]>` | ||
| SELECT * FROM genie_bridge_sessions | ||
| WHERE status = ${status} | ||
| ORDER BY started_at DESC | ||
| LIMIT 100 | ||
| `; | ||
| } | ||
| return this.sql<BridgeSessionRow[]>` | ||
| SELECT * FROM genie_bridge_sessions | ||
| ORDER BY started_at DESC | ||
| LIMIT 100 | ||
| `; |
There was a problem hiding this comment.
list() is truncating crash recovery.
recoverSessions() uses this method for status='active', so the hard LIMIT 100 silently skips older live rows. Once active rows exceed that cap, anything past the newest 100 is neither recovered nor orphaned and stays stranded as active in PG.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/bridge-session-store.ts` around lines 115 - 128, The list()
method in BridgeSessionStore is truncating results with a hard LIMIT 100 which
breaks recoverSessions() (called with status='active') and leaves older active
rows stranded; update the list(status?: ...) implementation to return all
matching rows instead of capping at 100 (or implement pagination/cursoring if
you must cap): remove the LIMIT 100 from the SQL used when status is provided
(and from the no-status branch if global truncation is unintended), or implement
a paginated loop in recoverSessions() that repeatedly calls list() with an
offset/last-id until all genie_bridge_sessions rows (BridgeSessionRow) are
processed so recoverSessions() handles every active session.
Comment on lines
1125
to
1133
| private removeSession(key: string): void { | ||
| const entry = this.sessions.get(key); | ||
| if (entry?.idleTimer) clearTimeout(entry.idleTimer); | ||
| // Close session in PG | ||
| const closeId = entry?.pgBridgeSessionId; | ||
| if (closeId && this.sessionStore) { | ||
| this.safePgCall('session_close', (sql) => new BridgeSessionStore(sql).close(closeId), undefined); | ||
| } | ||
| this.sessions.delete(key); |
There was a problem hiding this comment.
Make persisted session teardown synchronous for callers.
This helper now owns the PG close, but it starts that write without awaiting it and then deletes the in-memory entry immediately. Reset/idle flows can spawn a replacement before the old row is closed, and handleTurnTimeout() still bypasses this helper with a raw delete on Line 905, so duplicate or permanently-active rows can remain in genie_bridge_sessions.
Suggested direction
- private removeSession(key: string): void {
+ private async removeSession(key: string): Promise<void> {
const entry = this.sessions.get(key);
if (entry?.idleTimer) clearTimeout(entry.idleTimer);
const closeId = entry?.pgBridgeSessionId;
if (closeId && this.sessionStore) {
- this.safePgCall('session_close', (sql) => new BridgeSessionStore(sql).close(closeId), undefined);
+ await this.safePgCall('session_close', (sql) => new BridgeSessionStore(sql).close(closeId), undefined);
}
this.sessions.delete(key);
}And update the callers, including the timeout path, to await this.removeSession(...) instead of deleting the map entry directly.
🤖 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 1125 - 1133, Change removeSession
to perform synchronous teardown by making it async and awaiting the PG close
before deleting the in-memory entry: if entry?.idleTimer clear it, then if
closeId and this.sessionStore call and await the Promise returned by safePgCall
(which should be adapted to return a Promise) that runs new
BridgeSessionStore(sql).close(closeId), and only after that await resolve call
this.sessions.delete(key). Update all callers (including handleTurnTimeout and
any reset/idle flows that currently delete the map entry directly) to await
this.removeSession(key) instead of deleting the entry, ensuring the DB row is
closed before a replacement session can be created.
Comment on lines
+67
to
+75
| .option('--standalone', 'Force standalone mode (skip serve check)') | ||
| .action(async (options) => { | ||
| // Check if serve is already managing a bridge | ||
| const { getBridge } = await import('../services/omni-bridge.js'); | ||
| if (!options.standalone && getBridge()) { | ||
| console.log('[genie omni] Bridge is already managed by genie serve.'); | ||
| console.log(' Use `genie omni status` to check, or `genie omni start --standalone` to force.'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
This guard cannot see the bridge started by genie serve.
getBridge() is process-local. genie omni start runs in a fresh CLI process, so this check still returns null while another process already owns a live bridge. The command can therefore start a second subscriber, and both bridges will consume omni.message.> traffic.
🤖 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 67 - 75, The current guard in the
omni start action uses the process-local getBridge() which cannot detect a
bridge started by a separate genie serve process; replace or augment that check
with an inter-process existence probe (e.g., try to connect to the bridge's
known admin/health TCP port or unix socket, or check a well-known PID/lock file
that genie serve writes) and only allow starting when that probe fails or when
options.standalone is true; update the omni start handler (the .action block
that calls getBridge()) to call a new helper like isBridgeRunningRemotely() (or
reuse an existing network/IPC connector) and use its result instead of
getBridge() to prevent starting a second bridge.
Comment on lines
+614
to
+618
| // 2.55. Stop Omni bridge (graceful: detach sessions, don't kill them) | ||
| if (handles.omniBridge) { | ||
| handles.omniBridge.stop().catch(() => {}); | ||
| handles.omniBridge = null; | ||
| } |
There was a problem hiding this comment.
Don't fire-and-forget OmniBridge.stop() here.
OmniBridge.stop() now does real cleanup: it drains NATS and closes SDK bridge-session rows. Because the signal handlers exit the process immediately after shutdown(), this promise is very likely abandoned before it finishes, leaving stale active rows behind.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/term-commands/serve.ts` around lines 614 - 618, The code currently
fire-and-forgets OmniBridge.stop() (when handles.omniBridge exists) which causes
its async cleanup to be abandoned; update shutdown() so it awaits
handles.omniBridge.stop() and propagates/reports errors instead of swallowing
them, i.e. call await handles.omniBridge.stop() (or Promise.resolve then await)
and only then set handles.omniBridge = null; also adjust the signal handlers
that call shutdown() to await the returned promise before calling process.exit
to ensure OmniBridge.stop() finishes and its NATS/SDK cleanup completes.
Comment on lines
+885
to
+899
| /** Print Omni bridge status */ | ||
| async function printBridgeStatus(): Promise<void> { | ||
| try { | ||
| const { getBridge } = await import('../services/omni-bridge.js'); | ||
| const bridge = getBridge(); | ||
| if (bridge) { | ||
| const s = await bridge.status(); | ||
| const tag = s.connected ? 'connected' : 'disconnected'; | ||
| console.log(` omni-bridge: ${tag} (${s.activeSessions} sessions, queue: ${s.queueDepth})`); | ||
| } else { | ||
| console.log(' omni-bridge: stopped'); | ||
| } | ||
| } catch { | ||
| console.log(' omni-bridge: unavailable'); | ||
| } |
There was a problem hiding this comment.
serve status cannot see the managed bridge from another process.
getBridge() only reports the singleton in the current process. When users run genie serve status, they're in a new CLI process, so this prints omni-bridge: stopped even if the foreground/daemon serve instance has it running.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/term-commands/serve.ts` around lines 885 - 899, printBridgeStatus
currently calls getBridge() which only returns a singleton in the current CLI
process, so `genie serve status` can't see a bridge running in the
foreground/daemon; change printBridgeStatus to query the running serve process
over its management/status endpoint (or IPC socket) instead of calling
getBridge() locally — attempt an HTTP/IPC request to the serve daemon's status
API and parse the returned {connected, activeSessions, queueDepth} (fall back to
calling getBridge() only if the daemon endpoint is unreachable) so that
bridge.status() values reflect the actual running service rather than the local
singleton.
namastex888
added a commit
that referenced
this pull request
Apr 14, 2026
chore: rolling promotion dev -> main
5 of 6 tasks
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
2 participants
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.
Rolling Promotion PR
Auto-maintained rolling promotion PR from
devtomain.Process:
ready-to-mergeadded when all checks passSummary by CodeRabbit
New Features
Tests
Chores