diff --git a/.genie/wishes/genie-omni-marriage/WISH.md b/.genie/wishes/genie-omni-marriage/WISH.md index 4b1b5b3eb..e04a41509 100644 --- a/.genie/wishes/genie-omni-marriage/WISH.md +++ b/.genie/wishes/genie-omni-marriage/WISH.md @@ -297,6 +297,10 @@ pm2 start ecosystem.config.cjs && pm2 ls | grep genie --- +## Reconciliation Notes + +> **Omni-bridge is a message source, not a state owner.** The omni-bridge relay delivers inbound messages and publishes replies via NATS, but session lifecycle (spawn, resume, freeze, registry) is owned entirely by genie's executor layer. PR #1042 attempted to add a PG-backed session registry inside the bridge — this created a third source of truth alongside the executor table and the worker registry. The unified-executor-layer branch resolves this by keeping session persistence in the executors table and treating the bridge as a stateless message transport. Bridge restarts no longer require session recovery because the bridge holds no session state. + ## Files to Create/Modify ``` diff --git a/.genie/wishes/unified-executor-layer/AUDIT.md b/.genie/wishes/unified-executor-layer/AUDIT.md new file mode 100644 index 000000000..1d1b43699 --- /dev/null +++ b/.genie/wishes/unified-executor-layer/AUDIT.md @@ -0,0 +1,322 @@ +# Group 1 Audit — World B Dependency Map + +**Wish:** `unified-executor-layer` +**Group:** 1 (trace) +**Date:** 2026-04-04 +**Scope:** Map every consumer of World B so Groups 4–11 know what to touch. + +> **World B** = `src/services/executor.ts` (parallel `IExecutor`/`OmniSession`/`OmniMessage` types), `src/services/executors/claude-code.ts`, `src/services/executors/claude-sdk.ts`, `src/services/omni-bridge.ts` (in-memory session Map). + +--- + +## 1. Import Map (per importee) + +### 1.1 `src/services/executor.ts` + +Exports: `IExecutor`, `OmniSession`, `OmniMessage` (types only). + +| Importer | Line | What it imports | +|----------|------|-----------------| +| `src/services/omni-bridge.ts` | 14 | `IExecutor`, `OmniMessage`, `OmniSession` (type-only) | +| `src/services/executors/claude-code.ts` | 16 | `IExecutor`, `OmniMessage`, `OmniSession` (type-only) | +| `src/services/executors/claude-sdk.ts` | 5 | `IExecutor`, `OmniMessage`, `OmniSession` (type-only) | + +**Observation:** 3 importers, all inside World B itself. No file outside `src/services/` imports `executor.ts`. Once the three files above migrate to World A's `Executor` type (`src/lib/executor-types.ts`), `executor.ts` has zero consumers and can be deleted outright. + +--- + +### 1.2 `src/services/executors/claude-code.ts` + +Exports: `ClaudeCodeOmniExecutor` (class), `sanitizeWindowName` (pure helper). + +| Importer | Line | What it imports | +|----------|------|-----------------| +| `src/services/omni-bridge.ts` | 15 | `ClaudeCodeOmniExecutor` (constructed on line 106) | +| `src/services/executors/claude-code.test.ts` | 2 | `sanitizeWindowName` only (pure utility test) | + +**Observation:** Only two importers. The test file `claude-code.test.ts` imports a pure utility (`sanitizeWindowName`) — no coupling to `OmniSession` shape. It survives any refactor unless `sanitizeWindowName` is moved/deleted. The class itself is only instantiated by `omni-bridge.ts`. + +--- + +### 1.3 `src/services/executors/claude-sdk.ts` + +Exports: `ClaudeSdkOmniExecutor` (class). + +| Importer | Line | What it imports | +|----------|------|-----------------| +| `src/services/omni-bridge.ts` | 16 | `ClaudeSdkOmniExecutor` (constructed on line 104, type-checked on line 132) | +| `src/services/executors/__tests__/claude-sdk.test.ts` | 35 | `ClaudeSdkOmniExecutor` (dynamic `await import`) | + +**Observation:** Two importers. The test file DOES assert on `OmniSession` shape (see §2). + +--- + +### 1.4 `src/services/omni-bridge.ts` + +Exports: `OmniBridge` (class), `getBridge` (singleton accessor), `BridgeStatus` (type). + +| Importer | Line(s) | What it imports | +|----------|---------|-----------------| +| `src/term-commands/omni.ts` | 21, 51, 66 | `OmniBridge` (construct in `start`), `getBridge` (in `stop`/`status`) — all via dynamic `await import` | + +**Observation:** Exactly one consumer outside World B: `src/term-commands/omni.ts`. No direct imports from the TUI, from `genie.ts`, from `agents.ts`, or from any other term-command. Confirmed by grepping for `OmniBridge|getBridge|BridgeStatus|BridgeConfig` across the whole codebase — only `term-commands/omni.ts` and `services/omni-bridge.ts` match. + +--- + +## 2. Tests Asserting on `OmniSession` Fields + +The only test that asserts on World B's tmux-shaped `OmniSession` fields (`tmuxSession`, `tmuxWindow`, `paneId`) is: + +**`src/services/executors/__tests__/claude-sdk.test.ts`** + +| Line | Assertion | Context | +|------|-----------|---------| +| 51 | `expect(session.paneId).toBe('sdk-chat-123')` | `spawn()` returns SDK-specific pane id | +| 52 | `expect(session.tmuxSession).toBe('')` | SDK stubs the field to empty | +| 53 | `expect(session.tmuxWindow).toBe('')` | SDK stubs the field to empty | +| 84–86 | `tmuxSession: ''`, `tmuxWindow: ''`, `paneId: ''` | `isAlive()` fake-session literal (unknown-session test) | +| 107–109 | `tmuxSession: ''`, `tmuxWindow: ''`, `paneId: ''` | `shutdown()` idempotency fake-session literal | +| 172–174 | `tmuxSession: ''`, `tmuxWindow: ''`, `paneId: ''` | `deliver()` fake-session literal (error path) | + +**All six sites** are there only because `OmniSession` is hardcoded with tmux-only fields. Once Group 11 replaces `OmniSession` with World A's `Executor` (which has `tmuxSession/tmuxWindow/tmuxWindowId` as `string | null`), these assertions should be rewritten to either (a) pass `null` for tmux fields, or (b) drop the tmux assertions entirely and assert on `transport='api'` and metadata instead. The fake-session literals in lines 84–86 / 107–109 / 172–174 can also be replaced by `Executor`-shaped fixtures. + +**Unrelated `paneId`/`tmuxSession` assertions in the codebase** (grepped but confirmed to be World A or tmux-native): `src/lib/executor-registry.test.ts`, `src/lib/target-resolver.test.ts`, `src/lib/protocol-router.test.ts`, `src/lib/event-listener.test.ts`, `src/lib/scheduler-daemon.test.ts`, `src/__tests__/resume.test.ts`, `src/__tests__/events.test.ts`, `src/tui/session-tree.test.ts`, `src/lib/claude-logs.test.ts`, `src/lib/unified-log.test.ts`, `src/term-commands/approve.test.ts`, `src/lib/__tests__/zombie-spawns.test.ts`, `src/lib/__tests__/edge-cases-stability.test.ts`. **None of these import World B** — they are all World A / tmux-resolver tests and are out of scope for Group 11. + +--- + +## 3. In-Memory Session Map Call Sites + +**Declaration:** `src/services/omni-bridge.ts:152` — `private sessions = new Map();` +**Type:** `SessionEntry` — defined at `src/services/omni-bridge.ts:58`. + +> **Note:** Line numbers re-verified against the current 644-line file after Group 3's PG scaffolding landed. The Map's structure and count of call sites (14) are unchanged — only the line numbers shifted. + +All read/write sites inside `omni-bridge.ts`: + +| Line | Operation | Method | Purpose | +|------|-----------|--------|---------| +| 152 | `new Map()` | (field decl) | Declare the Map | +| 260 | `for (const [key, entry] of this.sessions)` | `stop()` | Iterate all sessions to shut them down | +| 270 | `this.sessions.clear()` | `stop()` | Empty the Map after shutdown | +| 305 | `this.sessions.size` | `status()` | Report `activeSessions` count | +| 309 | `Array.from(this.sessions.entries()).map(...)` | `status()` | Build `BridgeStatus.sessions` payload | +| 435 | `this.sessions.get(key)` | `routeMessage()` | Look up existing session for a `{agent}:{chatId}` key | +| 475 | `this.sessions.size` | `spawnSession()` | Enforce `maxConcurrent` limit | +| 492 | `this.sessions.set(key, placeholder)` | `spawnSession()` | Register the "spawning" placeholder before the executor spawns | +| 528 | `this.sessions.delete(key)` | `spawnSession()` (catch) | Clean up placeholder if spawn throws | +| 536 | `this.sessions.get(key)` | `resetIdleTimer()` | Look up the entry whose idle timer to refresh | +| 560 | `for (const [key, entry] of this.sessions)` | `checkIdleSessions()` | Iterate every 30s to kill idle/dead sessions | +| 589 | `this.sessions.get(key)` | `removeSession()` | Fetch entry to clear its idle timer | +| 591 | `this.sessions.delete(key)` | `removeSession()` | Evict the entry | +| 599 | `this.sessions.size` | `drainQueue()` | Check capacity before draining queued messages | + +**Count:** 14 references. **Group 6** (`Refactor omni-bridge.ts`) must replace every one of these with a query against World A's `executor-registry` (filtered by `metadata->>'source'='omni'`). Notes for Group 6: + +- **Spawning placeholder (lines 317–324, 337–338, 360):** World A has no in-memory "spawning" state. Either add a `state='spawning'` row immediately and update to `'running'` on success, or keep a small per-bridge `Map` limited to in-flight spawn/buffer concerns (NOT long-lived session truth). The buffering machinery is a bridge concern and can stay local; the session identity must live in PG. +- **Idle timer (lines 368, 373, 392, 421):** Per-session `setTimeout` handles are ephemeral and belong in a local `Map`. They should NOT be persisted — but they should be keyed by `executor.id` from World A rather than the synthetic `${agent}:${chatId}` string. +- **Concurrency count (307, 431):** Compute by counting un-ended `executors` rows where `metadata->>'source'='omni'` — OR cache the count locally and recompute on each spawn/shutdown. The wish leaves this detail to Group 6. +- **`status()` payload (215):** Must read from `executor-registry` via metadata filter. `BridgeStatus.sessions` schema may change (Group 11 confirms). +- **`SessionEntry` interface (line 39):** Goes away; replace any remaining local state with a minimal `{ buffer: OmniMessage[]; idleTimer: Timer | null; spawning: boolean }` keyed by executor id. + +--- + +## 4. CLI Commands Importing World B + +| Command file | Imports from World B | Purpose | +|--------------|----------------------|---------| +| `src/term-commands/omni.ts` | `OmniBridge`, `getBridge` (dynamic imports of `../services/omni-bridge.js`) | The only CLI surface that binds to the bridge | + +**No other CLI files import World B.** Verified by grepping every file under `src/term-commands/` for `omni-bridge|services/executor|ClaudeCodeOmniExecutor|ClaudeSdkOmniExecutor|IExecutor|OmniSession`. Zero matches. + +> Note: `src/term-commands/agents.ts:936` imports `ClaudeSdkProvider` from `src/lib/providers/claude-sdk.js` — this is the **provider** in World A's `lib/providers/` tree, NOT the World B **executor** in `services/executors/`. Out of scope. + +--- + +## 5. Surprise Dependencies + +**None found.** Group 1 swept the entire codebase for anything unexpected: + +- **TUI (`src/tui/**`):** No imports of `omni-bridge`, `executor.ts`, `claude-code.ts`, `claude-sdk.ts`, `OmniSession`, `IExecutor`, or `OmniBridge`. The TUI is entirely World A / tmux-resolver based. +- **Hooks (`src/hooks/**`):** No references. +- **`src/genie.ts` entry point:** No references. +- **Other `src/services/*`:** `omni-reply.ts` stands alone as a standalone CLI JSON publisher (`import.meta.url` main-check). It is referenced ONLY by a bash heredoc inside `claude-code.ts:248` (runtime string path, not a TS import). When `claude-code.ts` is removed/refactored, the heredoc goes too. `omni-reply.ts` has its own life as a CLI shim and is out of Group 1's scope (covered by the bridge-process footnote in the wish). +- **`knip.json:23`** lists `src/services/omni-bridge.ts` as a knip entry point (so its exports don't flag as unused). When Group 11 refactors/renames the bridge, this line must stay in sync. +- **Docs mentions:** + - `docs/ARCHITECTURE.md:55` and `:259` mention `src/services/omni-bridge.ts` + - `docs/CLI-REFERENCE.md:22` links to `#omni-bridge` + - `docs/sdk-executor-guide.md:564` mentions the `src/services/executors/` path + These are documentation-only and should be updated in Wave 4 cleanup (not a compile-time dependency). +- **`src/services/executors/claude-code.test.ts`** imports the pure helper `sanitizeWindowName` only. It does **not** reference `IExecutor` or `OmniSession`. If `sanitizeWindowName` is retained when `claude-code.ts` is refactored/deleted, this test remains valid; otherwise it should be deleted along with the function. + +**Zero surprises.** World B is fully contained: the only external seam is `term-commands/omni.ts`. + +--- + +## 5a. Does `ClaudeCodeOmniExecutor` (tmux) Already Register in World A's `executor-registry`? + +**Answer: NO.** The tmux executor in World B (`src/services/executors/claude-code.ts`) does **not** call `createAndLinkExecutor`, `updateExecutorState`, or `terminateExecutor` anywhere. Evidence: + +``` +$ grep -n "createAndLinkExecutor\|updateExecutorState\|terminateExecutor\|executor-registry" src/services/executors/claude-code.ts +(zero matches) +``` + +What it *does* use from World A's stack: +- `ensureTeamWindow`, `executeTmux`, `isPaneAlive`, `isPaneProcessRunning`, `killWindow` from `src/lib/tmux.js` (line 15) — these are **tmux helpers**, shared with the normal spawn path, but they don't touch the `executors` PG table. +- `directory.resolve` from `src/lib/agent-directory.js` (line 13). +- `shellQuote` from `src/lib/team-lead-command.js` (line 14). + +All of these are utility modules. None of them persist to `executors`, `sessions`, `session_content`, or `audit_events`. + +**Contrast — the normal `genie spawn` tmux path** (`src/term-commands/agents.ts:832–851`, helper `createTmuxExecutor`) DOES register via `createAndLinkExecutor(agentIdentityId, provider, 'tmux', { tmuxSession, tmuxPaneId, tmuxWindow, tmuxWindowId, claudeSessionId, state: 'spawning', repoPath, paneColor })`. That path is invoked from `agents.ts:1022, 1054, 1785`. + +**Implication for Group 11:** +- When a WhatsApp message arrives and `GENIE_EXECUTOR=tmux`, the bridge spawns a tmux window **invisible** to `genie ls`, `genie sessions`, and `genie events timeline`. No row is created in the `executors` table; no `audit_events` are emitted. +- This means Group 4's scope is **understated** in the wish: the wish says the tmux executor is "already partially integrated" and "Group 4 only verifies it, doesn't refactor it" — but based on this audit, the tmux executor needs the **same** `createAndLinkExecutor` / `updateExecutorState` / `terminateExecutor` wiring that Group 4 plans to add to the SDK executor. Either: + - **(Option A)** Group 4 adds executor-registry calls to **both** `claude-sdk.ts` and `claude-code.ts`, OR + - **(Option B)** `ClaudeCodeOmniExecutor` is refactored to delegate tmux spawning to `term-commands/agents.ts`'s spawn helpers (reuse existing `createTmuxExecutor`), OR + - **(Option C)** `ClaudeCodeOmniExecutor` is deleted entirely and the bridge simply shells out to `genie spawn --role ` when `GENIE_EXECUTOR=tmux` (heaviest refactor but cleanest — tmux already has full registry integration via the main spawn path). +- Group 11's delete-or-keep decision for `claude-code.ts` hinges on this. If Option C is taken, **both `claude-code.ts` and `claude-code.test.ts` can be deleted** (the `sanitizeWindowName` helper would move to `lib/tmux.ts` or be inlined into whatever bridge code produces window names). + +**Recommendation for the orchestrator:** Revisit Group 4's scope to cover the tmux executor explicitly. As written, the wish would leave `GENIE_EXECUTOR=tmux` runs invisible to World A — a latent regression that breaks success criterion #1 ("`genie ls` shows SDK-backed omni sessions alongside tmux-backed genie sessions") for the tmux code path specifically. + +--- + +## 5b. Wave 1 Verification — Groups 2 & 3 Status + +The team-lead asked Group 1 to verify two hypotheses about Wave 1 scope. Both are answered here with evidence. + +### 5b.1 Group 2 — NATS reply path in `claude-sdk.ts` — **NO-OP CONFIRMED** + +The hypothesis "claude-sdk.ts already has `natsPublish` wired; Group 2 is a no-op" is **correct**. Evidence: + +``` +$ grep -n 'execFile\|child_process\|execFileAsync' src/services/executors/claude-sdk.ts +(zero matches) + +$ grep -n 'natsPublish\|setNatsPublish' src/services/executors/claude-sdk.ts +73: private natsPublish: ((topic: string, payload: string) => void) | null = null; +86: setNatsPublish(fn: (topic: string, payload: string) => void): void { +87: this.natsPublish = fn; +190: if (replyText && this.natsPublish) { +199: this.natsPublish(topic, payload); +``` + +- Line 73: private field declared, typed `((topic, payload) => void) | null` +- Lines 86–88: `setNatsPublish(fn)` setter method present +- Line 190: reply path gates on `replyText && this.natsPublish` +- Line 199: in-process `this.natsPublish(topic, payload)` call (microsecond, not fork) +- Line 191: topic built as `omni.reply.${message.instanceId}.${message.chatId}` (matches wish spec) + +And the bridge wiring is already in place — `src/services/omni-bridge.ts:219–225`: +``` +if (this.executor instanceof ClaudeSdkOmniExecutor) { + const nc = this.nc; + const sc = this.sc; + this.executor.setNatsPublish((topic, payload) => { + nc.publish(topic, sc.encode(payload)); + }); +} +``` + +**Group 2 scope reshapes from "restore NATS reply path" to "verify nothing regressed" — no code change needed.** The test file `claude-sdk.test.ts` already asserts on `natsPublish` (lines 120–143 in the test file) rather than `execFile`. PR #1042's `execFile` regression never landed on this branch. + +### 5b.2 Group 3 — PG optional / degraded mode in `omni-bridge.ts` — **SCAFFOLDING ALREADY LANDED** + +The hypothesis "omni-bridge has zero PG refs" is **outdated**. Group 3's scaffolding is already present in the current `omni-bridge.ts` (644 lines). Evidence: + +| Line(s) | Symbol | Purpose | +|---------|--------|---------| +| 14 | `import type { Sql } from '../lib/db.js'` | PG client type | +| 28–29 | `PG_STARTUP_PROBE_TIMEOUT_MS = 5_000`, `PG_RUNTIME_QUERY_TIMEOUT_MS = 2_000` | Matches wish's "5s startup / 2s read" budgets | +| 36 | `export type PgProvider = () => Promise` | DI factory type | +| 42–45 | `SafePgCallContext { executorId?, chatId? }` | Log context shape | +| 53 | `config.pgProvider?: PgProvider` | DI hook in `BridgeConfig` | +| 70 | `BridgeStatus.pgAvailable: boolean` | Exposed on status payload | +| 106–121 | `function withTimeout(p, ms, label)` | Timeout helper with `timer.unref()` | +| 131–142 | `function isPgConnectionError(err)` | Connection-error classifier (ECONNREFUSED, ECONNRESET, ETIMEDOUT, connection terminated, etc.) | +| 158, 160, 161 | `private sql: Sql \| null`, `private pgAvailable = false`, `private readonly pgProvider: PgProvider` | Instance state | +| 177–182 | Default `pgProvider` = `async () => (await getConnection()) as Sql` | Lazy-imports `lib/db.js` | +| 216 | `await this.probePg()` | Called from `start()` after NATS connect, **never throws** | +| 286–289 | PG state reset in `stop()` | Clears `sql` and `pgAvailable` | +| 304 | `pgAvailable: this.pgAvailable` in `status()` | Exposed on `BridgeStatus` | +| 332–353 | `private async probePg()` | Graceful startup probe with 5s timeout, `SELECT 1` check, degraded-mode warning log | +| 370–394 | `private async safePgCall(op, fn, fallback, ctx?)` | **The only way** downstream groups are supposed to touch PG. Fast-paths to fallback if `pgAvailable` is false, single attempt with 2s timeout, logs at warn, flips `pgAvailable=false` on connection-level errors | + +**What Group 3 still has to do:** Verify the existing scaffolding matches the wish's PG Error Handling Strategy table (§3 of the wish) and confirm the unit tests exist. Based on this audit, the scaffolding matches the spec exactly: + +- ✅ Startup connect: fail-fast with graceful degradation (line 347–352) +- ✅ Runtime write: single attempt, log error, continue (line 370–394) +- ✅ Runtime read: 2s query timeout, return fallback on failure (line 381) +- ✅ Connection loss mid-run: flip `pgAvailable=false`, log "switching to degraded mode" (lines 387–391) +- ⚠️ Migration missing / schema mismatch: **not explicitly handled** — the current `probePg` treats all startup failures the same (degraded mode). The wish says schema mismatches should be fail-fast with a clear message, but the scaffolding silently degrades. **This is a gap Group 3 should close** — or the wish should be updated to accept "always degrade on startup failure" as the final behavior. +- ✅ Never drops a user reply due to PG failure (safePgCall returns fallback, doesn't throw) + +**What is NOT yet present:** +1. **No downstream call site actually uses `safePgCall` yet.** The helper exists and is private; no method in `omni-bridge.ts` currently wraps a PG query through it. Groups 4, 5, 6, and 7 will wire their PG writes/reads through `this.safePgCall(...)` as they add functionality. (Because `safePgCall` is `private`, downstream groups will need to either call methods on `OmniBridge` that internally wrap `safePgCall`, or Group 3 will need to expose a narrower public API. This is a minor surface-area decision for Group 6's refactor.) +2. **No unit tests exist yet** for `probePg()` or `safePgCall()` (checked `src/services/__tests__/` — directory does not exist). The wish's Group 3 deliverable #5/#6 require: + - Test: start a bridge with broken PG → assert `start()` succeeds + `status().pgAvailable === false` + - Test: inject mid-run PG error → assert `safePgCall` returns fallback + `pgAvailable` flips + delivery loop continues + + These tests are **the remaining work for Group 3**. The helper code is done; the coverage is missing. + +**Correction for the task list:** Task #5's description ("omni-bridge has zero PG refs") is **incorrect** as of the current worktree state. The bridge is already PG-aware as of the scaffolding commit. Group 3's remaining scope is: (a) add the two unit tests from wish deliverables §5/§6, (b) decide on the migration-mismatch behavior gap, (c) consider whether `safePgCall` should become public (or exposed via a typed method) so Groups 4–7 can consume it. The task description should be updated to "Finish Group 3 — add degraded-mode unit tests; resolve migration-mismatch gap" rather than "VERIFY omni-bridge has zero PG refs". + +### 5b.3 Summary of Wave 1 Status + +| Group | Hypothesis | Verdict | Remaining work | +|-------|-----------|---------|----------------| +| 2 (NATS reply) | No-op (already wired) | ✅ CONFIRMED | Nothing. Close the group. | +| 3 (PG optional) | Zero PG refs | ❌ OUTDATED. Scaffolding already landed. | (a) Add the two degraded-mode unit tests. (b) Handle migration-mismatch gap. (c) Decide public API shape for `safePgCall` so Groups 4–7 can consume it. | + +Wave 1's original parallelization assumption no longer holds for Group 3 — the helper is present, but the tests and DI surface are not. The team-lead should adjust Group 3's scope before dispatching Wave 2, because Group 4 (SDK executor registers in World A) needs to call PG writes through `safePgCall`, which is currently `private`. + +--- + +## 6. Proposed Deletion / Refactor Order (for Group 11) + +Work bottom-up in the dependency graph so TypeScript compilation stays green at every step: + +``` +term-commands/omni.ts + │ + ▼ +services/omni-bridge.ts + │ + ├──▶ services/executors/claude-code.ts + │ │ + │ └──▶ services/executor.ts (types) + │ + └──▶ services/executors/claude-sdk.ts + │ + └──▶ services/executor.ts (types) +``` + +**Order within Group 11 (assumes Groups 4–10 already landed):** + +1. **Rewrite `src/services/executors/__tests__/claude-sdk.test.ts`** — replace `OmniSession` fake-session literals with `Executor`-shaped fixtures (or drop the tmux-field assertions entirely, since `transport='api'` and metadata are the new truth). This unblocks every downstream edit without a failing test run. +2. **Refactor `src/services/executors/claude-sdk.ts`** — replace `import type { IExecutor, OmniMessage, OmniSession } from '../executor.js'` with `import type { Executor } from '../../lib/executor-types.js'`. Drop the `tmuxSession: ''` / `tmuxWindow: ''` / synthetic `paneId` stubs; return an `Executor` row created via `createAndLinkExecutor` (this work is actually Group 4 — Group 11 just finalizes it by deleting any leftover `OmniSession` residue). +3. **Refactor or delete `src/services/executors/claude-code.ts`** — same type swap. Because the tmux backend is only used when `GENIE_EXECUTOR=tmux` and the wish's Group 4 comment says "it's already partially integrated; Group 4 only verifies it", the existing file can likely be refactored in place (not deleted). Its `sanitizeWindowName` export must be preserved for the unit test. +4. **Refactor `src/services/omni-bridge.ts`** — remove the `import type { IExecutor, OmniMessage, OmniSession } from './executor.js'` line and the private session Map. Replace with executor-registry queries + a minimal local `Map` for spawn/idle concerns only. Keep the singleton `getBridge()` accessor — `term-commands/omni.ts` still depends on it. +5. **Delete `src/services/executor.ts`** once nothing imports it. Verify with `grep -r "from.*services/executor\\.js" src/`. The wish permits an optional ≤20-line adapter re-export if any edge caller remains — based on this audit, nothing remains, so **full delete** is on the table. +6. **Update `knip.json:23`** only if `omni-bridge.ts` is renamed. If the path stays the same, no change. +7. **Touch docs (`ARCHITECTURE.md`, `CLI-REFERENCE.md`, `sdk-executor-guide.md`)** — Wave 4 cleanup, not blocking. + +**Safe-to-delete-first files (if the wish decides to nuke rather than refactor):** None. `src/services/executor.ts` is the only file that could conceivably be deleted without touching others — but doing so first would break the remaining World B files that still `import type` from it. The correct order is: **consumer edits first, producer delete last.** + +--- + +## 7. Summary + +| Metric | Count | +|--------|-------| +| World B files | 4 | +| Unique external importers of World B | 1 (`src/term-commands/omni.ts`) | +| Internal cross-imports within World B | 3 (omni-bridge → claude-code, omni-bridge → claude-sdk, each executor → executor.ts types) | +| In-memory `sessions` Map call sites | 14 (all in `omni-bridge.ts`) | +| Tests asserting on `OmniSession` tmux fields | 1 file, 6 assertion sites (all in `claude-sdk.test.ts`) | +| Surprise dependencies | 0 | +| Files that can be deleted outright after Group 11 | 1 (`src/services/executor.ts`) | +| CLI files referencing World B | 1 (`src/term-commands/omni.ts`) | + +**Bottom line:** World B is impressively self-contained. The merge into World A only touches the files Group 1 has listed here. Groups 4, 5, 6, and 11 now have a precise map of every location they need to edit. diff --git a/.genie/wishes/unified-executor-layer/WISH.md b/.genie/wishes/unified-executor-layer/WISH.md new file mode 100644 index 000000000..db5515bc7 --- /dev/null +++ b/.genie/wishes/unified-executor-layer/WISH.md @@ -0,0 +1,782 @@ +# Wish: Unified Executor Layer + +| Field | Value | +|-------|-------| +| **Status** | DRAFT | +| **Slug** | `unified-executor-layer` | +| **Date** | 2026-04-04 | +| **Supersedes** | `automagik-dev/genie#1042` (close, do not merge) | +| **Reconciles** | `genie-omni-marriage` (adds explicit bridge-process footnote) | +| **Repo** | `automagik-dev/genie` | + +## Summary + +Genie has two parallel session/executor subsystems. "World A" is the mature registry (`agents`, `executors`, `sessions`, `session_content`, `audit_events`, `executor-registry.ts`, `session-capture.ts`, `genie sessions` CLI, PG LISTEN/NOTIFY). "World B" is the omni bridge (`services/executor.ts`, `services/omni-bridge.ts`, `services/executors/claude-sdk.ts`) — it never touches World A. This wish **merges World B into World A**: the omni bridge becomes a thin NATS subscriber that calls `createAndLinkExecutor`, the SDK executor registers in the `executors` table with transport `'api'`, session content is emitted inline by the SDK executor (since it has no JSONL filewatch source), and the backend becomes a runtime flip switch via `GENIE_EXECUTOR=tmux|sdk`. Result: one CLI surface (`genie ls`, `genie sessions`, `genie kill`, `genie events timeline`), one observability pipeline (audit_events), one transparent USB between Claude Code and Claude Agent SDK. + +PR #1042 attempted a similar goal but added a third registry (`omni_sessions` table) inside World B. This wish closes #1042 and extracts only its legitimately salvageable ideas (lazy resume via stored `claude_session_id`) into the right files. + +## Scope + +### IN + +1. **Merge World B into World A's executor registry** + - `services/executors/claude-sdk.ts` calls `lib/executor-registry.createAndLinkExecutor()` on spawn with `transport='api'` + - State transitions (`spawning → running → idle → working → …`) go through `updateExecutorState()` + - `shutdown()` calls `terminateExecutor()` and updates agent's `current_executor_id` + - Metadata JSONB carries `{ source: 'omni', chat_id, instance_id }` +2. **Emit session content from SDK executor inline** + - As Claude SDK streams response messages, write `session_content` rows (turn index, role, content, tool_name, timestamp) + - Write `audit_events` for `spawn`, `deliver_start`, `deliver_end`, `tool_use`, `shutdown` so OTel pipeline sees the session + - Mirror what `session-capture.ts` produces for tmux sessions — same shape, same tables, same queries +3. **Refactor `services/omni-bridge.ts`** + - Remove in-memory session Map; look up sessions via `executor-registry` queries filtered by metadata `source='omni'` + - On NATS inbound message: find-or-create executor via `createAndLinkExecutor`, then call `executor.deliver()` + - Idle timeout and max concurrency logic stays in the bridge (it's a dispatcher concern, not an executor concern) +4. **Delete or demote `services/executor.ts`** + - If it's fully subsumed by World A's interface, delete the file + - Otherwise reduce to a ≤20-line adapter that re-exports World A types + - `OmniSession` interface (with hardcoded `tmuxSession`, `tmuxPaneId`, `paneId` fields) is replaced by World A's `Executor` type +5. **Restore NATS reply path** + - Revert PR #1042's `execFile('omni', 'send', …)` change + - `ClaudeSdkOmniExecutor` publishes reply via `nc.publish('omni.reply..', …)` as before + - `setNatsPublish(fn)` hook is restored on the executor +6. **PG optional, degraded mode** + - Bridge start does not `process.exit(1)` if PG is unavailable + - Falls back to in-memory executor tracking (no session recovery across restarts) + - Logs `[omni-bridge] PG unavailable — session recovery disabled` on startup + - Executor registry calls are wrapped in PG-available guard; no-ops in degraded mode +7. **Lazy resume via executor lookup** (PR #1042's good idea, correct placement) + - On spawn, before creating a new executor, query `executors` table for `WHERE metadata->>'source'='omni' AND metadata->>'chat_id'= AND agent_id= AND ended_at IS NULL` ordered by `started_at DESC LIMIT 1` + - If found, reuse its `claude_session_id` for SDK resume; update existing executor row rather than creating new + - If not found OR Claude backend rejects resume, create fresh executor and log the fallback to audit_events +8. **Flip switch: `GENIE_EXECUTOR=tmux|sdk`** + - Env var controls which executor the bridge uses + - `genie omni start --executor sdk|tmux` CLI flag override + - `genie config set executor ` for persistent config + - Optional: extend to `genie spawn --executor sdk` for human/team spawns (stretch — may defer) +9. **Collapse `genie omni` CLI** + - Final surface: `genie omni start`, `genie omni stop`, `genie omni status` + - `start` accepts `--executor sdk|tmux` and passes through + - `status` shows: bridge process state, NATS connection, active executors (via World A query filtered by metadata `source='omni'`), idle timer, queue depth + - Delete any `genie omni sessions/logs/config/kill/reset` subcommands if #1042 landed +10. **`source` filter in existing CLIs** + - `genie ls --source omni` and `genie sessions list --source omni` filter by `executors.metadata->>'source'` + - No new commands, just a filter flag on existing ones +11. **Close PR #1042** + - Post a closing comment linking this wish + - Salvage the lazy-resume concept (reimplemented in Group 7 against executors table, not a new `omni_sessions` table) + - Do not merge #1042 +12. **Reconciliation note on `genie-omni-marriage`** + - Update `.genie/wishes/genie-omni-marriage/WISH.md` with a short footnote: "The omni-bridge NATS subscriber process exists as of [date]. It is a single optional PM2 service, not a genie daemon. Genie-as-CLI boundary preserved — the bridge is a message source, not a state owner. Sessions live in genie's existing `executors` table." + +### OUT + +- Renaming `genie omni` to `genie bridge` (user decision: keep `genie omni`, only 3 commands) +- New `omni_sessions` PG table (PR #1042's design — rejected; use existing `executors` table) +- Multi-bridge HA, load balancing, failover (separate wish) +- New message sources beyond terminal/team/omni (no cron/scheduler integration here) +- OTel exporter changes, new metrics dashboards (use existing `audit_events`/`session-capture` pipeline) +- Changes to the Omni server or its API +- Full migration of `sessions.agent_id` → `sessions.executor_id` (deferred per migration 012 comment) +- Worktree isolation for omni-sourced sessions (already shipped in `omni-session-isolation`) +- Deleting the `omni_sessions` table (only relevant if #1042 lands before this wish; handled as Group 0 cleanup if needed) +- `genie send` / `genie kill` / `genie logs` CLI changes — they already work for World A and will cover omni sessions automatically once those sessions register in `executors` table +- Rewriting the tmux executor (it's already partially integrated; Group 4 only verifies it, doesn't refactor it) +- SDK executor support for tools beyond what the existing SDK provider already exposes + +## Decisions + +| Decision | Rationale | +|----------|-----------| +| Merge World B into World A, not the other way around | World A is mature (8+ migrations, full state machine, capture pipeline, CLI, observability). World B is a thin 400-line duplicate. Merge direction is obvious. | +| SDK executor uses `transport='api'` | The enum already supports `'tmux' \| 'api' \| 'process'`. Zero migration cost. | +| Source carried via `metadata` JSONB, not new column | Additive, no migration. Filterable with `metadata->>'source'='omni'` JSONB operators. Indexed later if hot. | +| SDK executor emits `session_content` inline, not via filewatch | No JSONL files exist for in-process SDK. Replicate capture inline (~200-300 LOC: helper module + deliver() integration + turn indexing + tool event mapping + audit hooks + tests). This is the unavoidable new code. | +| Reply path stays NATS publish | PR #1042's `execFile('omni', …)` is a ~230ms per-reply fork. In-process `nc.publish` is microseconds. No contest. | +| PG optional with degraded mode | Hard-gating PG kills local dev onboarding. Degraded mode (no recovery across restarts) is acceptable for dev. | +| Lazy resume via `executors` table query, not new table | PR #1042 created `omni_sessions` unnecessarily. The `executors.claude_session_id` column already exists on the right table. | +| `GENIE_EXECUTOR` env var as flip switch | Single point of control. Human can override per-invocation with `--executor` flag. Persistable via `genie config set`. | +| Keep `genie omni` namespace | User explicit decision. Only 3 commands (start/stop/status) — all about the bridge **process**, not about sessions. No naming collision because the bridge is a long-lived service. | +| Close PR #1042 without merging | ~75% of its changes are wrong direction (execFile reply, PG mandatory, new CLI commands, new table). Salvaging in-place costs more than rebuilding on World A. | +| Do not rename `OmniSession` to `Executor` in tests | Tests should be rewritten to use World A types directly after refactor, not bridge-and-adapt. Cleaner diff. | + +## Success Criteria + +- [ ] `genie ls` shows SDK-backed omni sessions alongside tmux-backed genie sessions (confirmed by spawning via both paths and listing) +- [ ] `genie sessions list --source omni` returns only omni-sourced executors, matching what's in `executors` table +- [ ] `genie events timeline ` returns audit events for an SDK-backed omni session (spawn, deliver_start, deliver_end, shutdown) +- [ ] `genie kill ` terminates an SDK-backed omni session and the bridge stops delivering to it +- [ ] `genie sessions replay ` shows turn-by-turn content for an SDK-backed omni session +- [ ] Restarting the omni bridge recovers the `claude_session_id` from the `executors` table for in-flight chats (within Claude backend TTL); logs a fallback audit event when TTL is exceeded +- [ ] `GENIE_EXECUTOR=sdk` makes `genie omni start` run the SDK executor; `GENIE_EXECUTOR=tmux` runs the tmux executor; no code changes needed to switch +- [ ] `genie omni start --executor sdk` overrides the env var for that invocation +- [ ] Reply path uses `nc.publish('omni.reply..', …)` — no subprocess forks per reply (verified by tracing `execFile` calls in runtime) +- [ ] Bridge starts successfully when PG is offline, logs a warning, runs in degraded mode (no recovery) +- [ ] `services/executor.ts` is deleted OR reduced to ≤20 lines re-exporting from `lib/executor-types.ts` +- [ ] `services/omni-bridge.ts` no longer maintains an in-memory session Map; all session queries go through `executor-registry` +- [ ] `genie omni` CLI has exactly 3 subcommands: `start`, `stop`, `status` (plus `start`'s `--executor` flag) +- [ ] PR #1042 is closed (not merged) with a comment linking this wish +- [ ] `genie-omni-marriage` wish has a reconciliation footnote +- [ ] Matrix test: all 6 combinations of (tmux \| sdk) × (human \| team \| omni) spawn, appear in `genie ls`, emit audit events, and can be killed via `genie kill` +- [ ] `bun run check` passes (typecheck + lint + all tests, including the bun `mock.module` leak fix) +- [ ] Zero regressions in existing `genie sessions list/replay/search` behavior + +## Execution Strategy + +### Wave 1 (parallel — foundations, no dependencies) + +| Group | Agent | Description | +|-------|-------|-------------| +| 1 | trace | Audit all callers/consumers of `services/executor.ts`, `services/omni-bridge.ts`'s session Map, and `services/executors/*.ts`. Produce a deletion/refactor map. | +| 2 | engineer | Restore NATS reply path in `services/executors/claude-sdk.ts` (revert #1042's `execFile` change; restore `setNatsPublish` hook) | +| 3 | engineer | PG-optional bridge startup — replace `process.exit(1)` in `omni-bridge.ts` with warning + degraded mode guard | + +### Wave 2 (after Wave 1 — core refactor) + +Order within Wave 2 matters: **4 → 5 → 7 → 6**. Group 5 defines the shared audit-event enum and `safePgCall`. Group 7 adds the metadata index and `findLatestByMetadata`. Group 6 then consumes both. + +| Group | Agent | Description | +|-------|-------|-------------| +| 4 | engineer | Register SDK executor in World A: call `createAndLinkExecutor(agentId, 'claude', 'api', {claudeSessionId, metadata: {source: 'omni', chat_id, instance_id}})` on spawn; `updateExecutorState` on transitions; `terminateExecutor` on shutdown | +| 5 | engineer | Inline `session_content` emission + shared `src/lib/audit-events.ts` enum. As Claude SDK streams, write rows to `sessions` + `session_content` + `audit_events` tables. Match shape produced by `session-capture.ts`. | +| 7 | engineer | Migration 027 (JSONB metadata index) + `findLatestByMetadata` helper + lazy resume: on spawn, query latest un-ended executor for (agent_id, source='omni', chat_id); reuse `claude_session_id` if found; handle SDK resume rejection with fallback + audit log | +| 6 | engineer | Refactor `omni-bridge.ts` to delegate find-or-create to the executor (which uses `findLatestByMetadata` from Group 7). Delete in-memory session Map. No query duplication. | + +### Wave 3 (after Wave 2 — CLI + flip switch) + +| Group | Agent | Description | +|-------|-------|-------------| +| 8 | engineer | `GENIE_EXECUTOR` flip switch: env var parsing, `genie config set executor `, `genie omni start --executor ` override, bridge reads resolved value at startup | +| 9 | engineer | Collapse `src/term-commands/omni.ts` to exactly `start` / `stop` / `status`. If PR #1042 landed before this wish, delete its added subcommands. `status` reads executors via World A query. | +| 10 | engineer | Add `--source ` filter to `genie ls` and `genie sessions list` via `metadata->>'source'` JSONB filter | + +### Wave 4 (after Wave 3 — cleanup + validation) + +| Group | Agent | Description | +|-------|-------|-------------| +| 11 | engineer | Delete `services/executor.ts` if fully subsumed (or reduce to ≤20-line adapter). Delete `OmniSession` references; replace with World A `Executor` type. | +| 12 | engineer | Close PR #1042 with closing comment linking this wish. Append reconciliation footnote to `genie-omni-marriage/WISH.md`. | +| qa | qa | Run the 6-combination matrix test: (tmux \| sdk) × (human \| team \| omni). Verify `genie ls`, `genie kill`, audit events for all six. | +| review | reviewer | Review all changes against success criteria | + +## Execution Groups + +### Group 1: Audit callers + +**Goal:** Produce a complete map of what depends on World B so later groups know what to touch. + +**Deliverables:** +1. List every import of `services/executor.ts`, `services/executors/claude-code.ts`, `services/executors/claude-sdk.ts`, `services/omni-bridge.ts` +2. Identify tests that assert on `OmniSession` fields (`tmuxSession`, `tmuxWindow`, `paneId`) — these need rewrites +3. List all call sites of the in-memory session Map in `omni-bridge.ts` +4. Identify any CLI commands that import from World B directly (beyond `term-commands/omni.ts`) +5. Output a markdown report at `.genie/wishes/unified-executor-layer/AUDIT.md` + +**Acceptance Criteria:** +- [ ] Report lists every file that imports World B modules +- [ ] Report identifies tests with tmux-field assertions +- [ ] Report flags any surprise dependencies (e.g., TUI modules, other services) +- [ ] Report proposes a deletion order (which files can go first without breaking builds) + +**Validation:** +```bash +# Report exists and is non-empty +test -s .genie/wishes/unified-executor-layer/AUDIT.md +``` + +**depends-on:** none + +--- + +### Group 2: Restore NATS reply path + +**Goal:** Reply goes back via `nc.publish`, not subprocess fork. + +**Deliverables:** +1. In `src/services/executors/claude-sdk.ts`: remove `sendViaOmniCli` / `execFileAsync` / `node:child_process` imports +2. Restore `private natsPublish: ((topic: string, payload: string) => void) \| null = null;` +3. Restore `setNatsPublish(fn)` method +4. Reply path uses `this.natsPublish(topic, payload)` with topic `omni.reply.${message.instanceId}.${message.chatId}` +5. In `src/services/omni-bridge.ts`: restore the `setNatsPublish` wiring inside `start()` when executor is `ClaudeSdkOmniExecutor` +6. Update `claude-sdk.test.ts` to assert `natsPublish` was called with correct topic/payload (revert the `execFile` assertion) + +**Acceptance Criteria:** +- [ ] No `execFile`/`execFileAsync`/`node:child_process` import in `claude-sdk.ts` +- [ ] `setNatsPublish` hook exists and is called by bridge +- [ ] Tests assert NATS publish, not execFile +- [ ] Tracing the code path from `deliver()` shows a single in-process publish call, not a subprocess spawn + +**Validation:** +```bash +cd genie && bun run typecheck && bun run lint && bun test src/services/executors/__tests__/claude-sdk.test.ts +# Grep must find zero matches +! grep -q "execFile\|child_process" src/services/executors/claude-sdk.ts +``` + +**depends-on:** none + +--- + +### Group 3: PG optional, degraded mode + +**Goal:** Bridge starts without PG. No `process.exit(1)`. Clear error handling strategy for PG failures during runtime. + +**PG Error Handling Strategy (explicit):** + +| Failure Type | Strategy | Rationale | +|--------------|----------|-----------| +| **Startup connect** (initial `SELECT 1`) | Fail-fast with graceful degradation: log warning, set `pgAvailable=false`, continue in degraded mode | Never block bridge startup; a dev without PG must still be able to run omni | +| **Runtime write** (executor state update, audit event insert, session_content insert) | Try once, log error with context, continue; do NOT retry inline, do NOT crash the delivery loop | A single failed write should never drop a user-visible message. Dropped observability is recoverable; dropped replies are not. | +| **Runtime read** (lazy resume lookup) | Try once with 2s query timeout; on failure or timeout, fall back to "no prior session" path and log; do NOT block message delivery | Slow PG must not slow human replies | +| **Connection loss mid-run** | On any write error classified as connection-level (ECONNREFUSED, connection terminated), set `pgAvailable=false` and log `[omni-bridge] PG connection lost — switching to degraded mode`; do not try to reconnect (operator restarts bridge) | Simple and predictable. Reconnect logic belongs in a later wish. | +| **Migration missing / schema mismatch** | Fail-fast on startup with clear error message pointing at migration command | Silent data corruption is worse than noisy failure | + +All strategies: log at `warn` level with `executor_id` (when known), `chat_id`, and the operation name. Never log PG error stack traces at `info`. + +**Deliverables:** +1. In `src/services/omni-bridge.ts` `start()`: + - Remove the `process.exit(1)` on failed `SELECT 1` + - Wrap the check in a boolean `this.pgAvailable` + - Log `[omni-bridge] PG unavailable — session recovery disabled` when false +2. Expose `pgAvailable` on `BridgeStatus` interface +3. All downstream executor calls that write to PG check `this.pgAvailable` and no-op (with a trace log) when false +4. Wrap each PG call site in a helper `safePgCall(op: string, fn: () => Promise, fallback: T): Promise` that implements the runtime write/read strategy above (single attempt, log on failure, return fallback, flip `pgAvailable=false` on connection-level errors) +5. Unit test: start a bridge with a broken PG connection string; assert `start()` succeeds and `status().connected === true` for NATS but `status().pgAvailable === false` +6. Unit test: inject a mid-run PG error; assert `safePgCall` returns fallback and `pgAvailable` flips to `false`; assert delivery loop continues + +**Acceptance Criteria:** +- [ ] Bridge starts with PG offline, no exit +- [ ] `BridgeStatus` exposes `pgAvailable` boolean +- [ ] Runtime PG errors never drop a user reply +- [ ] `safePgCall` helper is the only way downstream code touches PG +- [ ] Tests cover: degraded startup, mid-run connection loss, slow query fallback + +**Validation:** +```bash +bun test src/services/__tests__/omni-bridge.test.ts +``` + +**depends-on:** none + +--- + +### Group 4: SDK executor registers in World A + +**Goal:** SDK-backed omni sessions appear in the `executors` table with `transport='api'`. + +**Deliverables:** +1. In `src/services/executors/claude-sdk.ts`: + - Import `createAndLinkExecutor`, `updateExecutorState`, `terminateExecutor`, `getExecutor` from `lib/executor-registry.js` (real API — verified against `origin/dev:src/lib/executor-registry.ts` lines 84, 137, 151, 119) + - Import `findOrCreateAgent` (or equivalent) from `lib/agent-registry.js` + - On `spawn(agentName, chatId, env)`: + - Call `agent-registry.findOrCreateAgent(agentName, …)` to get `agentId` + - Call `createAndLinkExecutor(agentId, 'claude', 'api', { claudeSessionId: undefined, metadata: { source: 'omni', chat_id: chatId, instance_id: env.OMNI_INSTANCE_ID } })` + - Store the returned `Executor` in place of the ad-hoc in-memory state (or alongside it during transition) + - On state transitions (before/after query): call `updateExecutorState(executorId, 'running' \| 'working' \| 'idle')` + - On `shutdown`: call `terminateExecutor(executorId)` (which sets `ended_at` and state `'terminated'`) and clear agent's `current_executor_id` link + - Guard all calls with `this.pgAvailable` from Group 3 — no-op in degraded mode +2. Remove the `SdkSessionState` Map approach where possible (keep AbortController tracking but source executor data from PG) +3. `OmniSession` fields that don't apply (`tmuxSession`, `tmuxWindow`, `paneId`) are stubbed with empty strings for now — full removal in Group 11 +4. Update `claude-sdk.test.ts` to mock `executor-registry` calls and assert they happen + +**Acceptance Criteria:** +- [ ] `spawn()` creates a row in `executors` with `transport='api'` and metadata `{source, chat_id, instance_id}` +- [ ] State transitions are reflected in `executors.state` +- [ ] `shutdown()` sets `ended_at` and state `'terminated'` +- [ ] Tests mock the registry and assert the right calls +- [ ] Code runs in degraded mode (no PG) without errors + +**Validation:** +```bash +bun run typecheck && bun test src/services/executors/__tests__/claude-sdk.test.ts +# Manual: start bridge, send one WhatsApp message, query `SELECT * FROM executors WHERE transport='api'` +``` + +**depends-on:** Group 3 + +--- + +### Group 5: Inline session content capture for SDK + +**Goal:** SDK-backed sessions produce the same `session_content` + `audit_events` rows that `session-capture.ts` produces for tmux sessions. + +**Shared Audit Event Type Enum (used by Groups 5 and 7):** + +Defined in `src/lib/audit-events.ts` as a single source of truth. Groups 5 and 7 MUST use these exact strings — no ad-hoc variants. + +```typescript +export type AuditEventType = + // Lifecycle (Group 5) + | 'executor.spawn' + | 'executor.shutdown' + | 'executor.state_transition' + // Delivery (Group 5) + | 'deliver.start' + | 'deliver.end' + | 'deliver.error' + | 'deliver.tool_use' + // Resume (Group 7) + | 'session.resumed' + | 'session.resume_rejected' + | 'session.created_fresh'; +``` + +Naming convention: `.` dotted. All new audit events for this wish live in this enum; no string literals elsewhere. Existing audit event strings in the codebase stay as-is unless a group explicitly touches them. + +**Deliverables:** +1. New `src/lib/audit-events.ts` with the `AuditEventType` union above and a `recordAuditEvent(type: AuditEventType, attrs: Record)` helper that wraps the existing audit_events insert with `safePgCall` (from Group 3) +2. Helper module `src/services/executors/sdk-session-capture.ts`: + - `startSession(executorId, claudeSessionId, agentId, team, role, wishSlug?)` → creates a row in `sessions` table + - `recordTurn(sessionId, turnIndex, role, content, toolName?, timestamp)` → writes `session_content` + - `recordToolEvent(sessionId, turnIndex, toolName, inputRaw, outputRaw, timestamp)` → writes to tool events table (reuse whatever `session-capture.ts` writes to) + - `endSession(sessionId, status)` → updates `sessions.ended_at`, `sessions.status` + - All writes go through `safePgCall` +3. In `claude-sdk.ts` `deliver()`: + - Call `startSession` on first message for the executor (or during spawn if the Claude session is pre-created) + - As the SDK query streams, iterate messages and call `recordTurn` / `recordToolEvent` per turn + - Call `recordAuditEvent('executor.spawn' | 'deliver.start' | 'deliver.end' | 'deliver.tool_use' | 'executor.shutdown', attrs)` with consistent attribute keys: `{ executor_id, agent_id, chat_id, instance_id, session_id }` + - Update `sessions.total_turns` and `sessions.last_ingested_offset` (use turn count as pseudo-offset since there's no JSONL file) +4. All writes guarded by `pgAvailable` via `safePgCall` +5. Test: deliver two messages to a fake SDK session, assert `session_content` has 2 rows and `audit_events` has `deliver.start`/`deliver.end` pairs with expected attrs + +**Acceptance Criteria:** +- [ ] `sessions` row exists for every SDK-backed omni session +- [ ] `session_content` rows mirror the shape produced by tmux filewatch (same columns, same roles) +- [ ] `audit_events` rows exist for spawn/deliver/shutdown +- [ ] `genie sessions replay ` works against an SDK session end-to-end +- [ ] No writes happen in degraded mode + +**Validation:** +```bash +bun test src/services/executors/__tests__/sdk-session-capture.test.ts +# Manual end-to-end: +GENIE_EXECUTOR=sdk genie omni start & +# send a test WhatsApp message +genie sessions replay # should show the content +``` + +**depends-on:** Group 4 + +--- + +### Group 6: Refactor omni-bridge to use World A + +**Goal:** `omni-bridge.ts` has no in-memory session Map. All session state comes from PG. + +**Deliverables:** +1. Delete the `sessions: Map` field in `OmniBridge` +2. On inbound NATS message: + - Parse `agent`, `chatId`, `instanceId` from the message + - Resolve `agentId` via `agent-registry.findOrCreateAgent(agent)` + - Delegate lookup to the executor: call `executor.spawn(agentName, chatId, env)` which internally uses `executor-registry.findLatestByMetadata({ agentId, source: 'omni', chatId })` (the same helper added in Group 7) to find-or-create + - The bridge does NOT duplicate the query logic — it delegates to the executor, which owns the find-or-create contract. This keeps one query path shared between spawn-on-new-message and restart-resume, so Group 7's index covers both. + - Call `executor.deliver()` with the message +3. Idle timeout logic: + - Keep an in-memory Map of `executorId → idleTimer` (small, timer-only, not session state) + - On timer fire: call `executor.shutdown()` which updates PG state +4. Queue depth and max concurrency logic stays but queries active executor count from PG (cached with short TTL) +5. Replace `BridgeStatus.sessions` with a PG query at status time +6. Degraded mode: if PG unavailable, fall back to in-memory Map (current behavior) with a clear warning + +**Acceptance Criteria:** +- [ ] No `Map` for session state in `omni-bridge.ts` +- [ ] NATS inbound handler does find-or-create via PG +- [ ] `genie omni status` shows correct active count, matching what `SELECT count(*) FROM executors WHERE ended_at IS NULL AND metadata->>'source'='omni'` returns +- [ ] Degraded mode works — bridge processes messages without PG +- [ ] Existing idle timeout and concurrency limit tests pass + +**Validation:** +```bash +bun test src/services/__tests__/omni-bridge.test.ts +``` + +**depends-on:** Group 4, Group 5, Group 7 (find-or-create helper + metadata index live in Group 7; Group 6 consumes them) + +--- + +### Group 7: Lazy resume via executors table + metadata index + +**Goal:** Bridge restart recovers in-flight Claude sessions by looking up `executors.claude_session_id`. Lookup stays fast even as executor count grows. + +**Deliverables:** +1. New migration `src/db/migrations/027_executors_omni_metadata_index.sql` — indexes the JSONB metadata fields that Group 6's find-or-create and Group 7's lazy resume both query on every inbound message: + ```sql + CREATE INDEX IF NOT EXISTS executors_omni_lookup + ON executors ( + agent_id, + (metadata->>'source'), + (metadata->>'chat_id') + ) + WHERE ended_at IS NULL; + + -- Optional covering index if resume path becomes hot: + -- CREATE INDEX IF NOT EXISTS executors_omni_resume + -- ON executors (agent_id, (metadata->>'source'), (metadata->>'chat_id'), started_at DESC) + -- INCLUDE (claude_session_id, state) + -- WHERE ended_at IS NULL; + ``` +2. Add `executor-registry.findLatestByMetadata(filter: { agentId: string; source: string; chatId: string }): Promise` — queries ordered by `started_at DESC LIMIT 1` with `ended_at IS NULL`. Signature is explicit (not a generic `Record`) so the index above is always used. +3. In `claude-sdk.ts` `spawn()`: + - Before calling `createAndLinkExecutor`, call `findLatestByMetadata({ agentId, source: 'omni', chatId })` + - If found AND `claudeSessionId` is set, **reuse** this executor (do not create new). Update `last_activity_at` via `updateExecutorState(executor.id, 'running')`. Write `recordAuditEvent('session.resumed', …)`. + - If found but `claudeSessionId` is null, still reuse — the Claude session will be set when first query returns + - If not found, create new executor and write `recordAuditEvent('session.created_fresh', …)` +4. In `deliver()`: + - Pass `state.claudeSessionId` to `runQuery` as the `resume` parameter + - After query returns, if SDK returned a different session ID (Claude rejected resume), update `executors.claude_session_id` via registry and write `recordAuditEvent('session.resume_rejected', { old_session_id, new_session_id, executor_id })` +5. All new audit events use the shared enum from Group 5's `src/lib/audit-events.ts` — no string literals. + +**Acceptance Criteria:** +- [ ] Migration 027 applies cleanly; `EXPLAIN` on the find-latest-by-metadata query shows index use +- [ ] Restarting the bridge mid-conversation and sending another WhatsApp message resumes the same Claude session (within TTL) +- [ ] When Claude rejects resume (TTL expired), a fresh session is created and `session.resume_rejected` audit event is written +- [ ] `genie events timeline ` shows `session.resumed` / `session.resume_rejected` / `session.created_fresh` events clearly +- [ ] Test: mock Claude SDK to alternate resume success/fail and verify both paths + +**Validation:** +```bash +bun run migrate:test # ensure migration 027 applies +bun test src/services/executors/__tests__/claude-sdk-resume.test.ts +# Verify index is used: +psql -c "EXPLAIN SELECT * FROM executors WHERE agent_id='' AND metadata->>'source'='omni' AND metadata->>'chat_id'='' AND ended_at IS NULL ORDER BY started_at DESC LIMIT 1" | grep "executors_omni_lookup" +``` + +**depends-on:** Group 4, Group 5 (for the shared audit event enum) + +--- + +### Group 8: `GENIE_EXECUTOR` flip switch + +**Goal:** One env var picks the executor. + +**Deliverables:** +1. `src/lib/executor-config.ts`: `resolveExecutorType(override?: string): 'tmux' \| 'sdk'` — reads override > env `GENIE_EXECUTOR` > persisted config > default `'tmux'` +2. `omni-bridge.ts` constructor calls `resolveExecutorType(config.executorType)` — no change to existing flag, just use this helper +3. `genie config set executor ` subcommand — persists to genie's config file (wherever existing `genie config` writes) +4. `genie omni start --executor ` flag — passes to `resolveExecutorType` as override +5. `genie omni status` shows resolved executor type +6. Documentation in `.genie/wishes/unified-executor-layer/FLIP-SWITCH.md` explaining precedence + +**Acceptance Criteria:** +- [ ] `GENIE_EXECUTOR=sdk genie omni start` runs SDK executor +- [ ] `genie omni start --executor tmux` overrides env var +- [ ] `genie config get executor` returns persisted value +- [ ] Precedence order is tested + +**Validation:** +```bash +bun test src/lib/__tests__/executor-config.test.ts +``` + +**depends-on:** Group 6 + +--- + +### Group 9: Collapse `genie omni` CLI + +**Goal:** `genie omni` has exactly 3 subcommands. + +**Deliverables:** +1. `src/term-commands/omni.ts` contains only `start`, `stop`, `status` +2. `start` accepts `--executor ` (Group 8) +3. `status` reads active executors from PG (if available) + bridge process state +4. Delete any `sessions`/`logs`/`config`/`kill`/`reset` subcommands if present from PR #1042 +5. File size should drop back to ~100-150 lines (from 300+ after #1042) + +**Acceptance Criteria:** +- [ ] `genie omni --help` lists exactly: `start`, `stop`, `status` +- [ ] Removed commands are gone from the file +- [ ] `status` query shape matches World A (via `executor-registry`) + +**Validation:** +```bash +genie omni --help | grep -E "^\s+(start|stop|status|sessions|logs|config)" | sort +# Expected: only start/stop/status +``` + +**depends-on:** Group 6, Group 8 + +--- + +### Group 10: `--source` filter on existing CLIs + +**Goal:** `genie ls --source omni` and `genie sessions list --source omni` work. + +**Deliverables:** +1. Add `--source ` option to `genie ls` (`term-commands/agent/list.ts` or wherever the ls handler lives) +2. Add `--source ` option to `genie sessions list` (`term-commands/sessions.ts`) +3. Both filter `executors` rows via `WHERE metadata->>'source' = ` +4. When no source is specified, return all (current behavior) +5. Add `--source` to `--help` output +6. Test: spawn executors with different metadata sources; verify filter returns correct subset + +**Acceptance Criteria:** +- [ ] `genie ls --source omni` returns only omni-sourced executors +- [ ] `genie sessions list --source omni` returns only omni sessions +- [ ] No source filter preserves existing output +- [ ] Tests cover the filter path + +**Validation:** +```bash +bun test src/term-commands/__tests__/sessions.test.ts +``` + +**depends-on:** Group 4 + +--- + +### Group 11: Delete or demote `services/executor.ts` + +**Goal:** World B's parallel interface is gone. + +**Deliverables:** +1. Audit (from Group 1) tells us what still imports `services/executor.ts` +2. Option A: if nothing imports it, delete the file +3. Option B: if small surface still needed, reduce to ≤20 lines re-exporting World A types (`Executor`, `ExecutorState`, etc.) +4. Update `services/executors/claude-code.ts` and `claude-sdk.ts` to import from `lib/executor-types.js`, not `../executor.js` +5. Remove `OmniSession` interface; replace with `Executor` from World A (may require renaming variables in `omni-bridge.ts`) +6. `OmniMessage` can stay — it's a NATS payload shape, not a session shape + +**Acceptance Criteria:** +- [ ] `services/executor.ts` is either deleted or ≤20 lines +- [ ] No references to `OmniSession` in `src/` +- [ ] `bun run check` passes +- [ ] `bun run knip` shows no new unused exports + +**Validation:** +```bash +bun run check +! grep -r "OmniSession" src/ --include="*.ts" | grep -v ".test.ts" +``` + +**depends-on:** Group 6, Group 9 + +--- + +### Group 12: Close PR #1042 + reconciliation + +**Goal:** Cleanup. PR #1042 is closed with context. `genie-omni-marriage` wish reflects reality. + +**Deliverables:** +1. Post a closing comment on `automagik-dev/genie#1042` with a summary of why and a link to this wish: + > This PR tackled a real problem (session persistence across bridge restarts) but added a third session registry (`omni_sessions` table) alongside two existing ones in genie core. We've opted to unify the layers instead — see wish `unified-executor-layer`. The lazy-resume idea is preserved, reimplemented against the existing `executors` table with `transport='api'`. Reply path stays NATS publish (the `execFile('omni', …)` change would have introduced a ~230ms per-reply fork). Thanks for the push on making this visible — closing in favor of the unified path. +2. `gh pr close 1042 --comment "..."` +3. Append reconciliation note to `.genie/wishes/genie-omni-marriage/WISH.md`: + > **2026-04-04 footnote:** The `omni-bridge` NATS subscriber process exists as a single optional PM2 service. It is a message source, not a state owner — all session state lives in genie's existing `executors`/`sessions` tables. The "Genie = CLI, never a server" boundary is preserved: the bridge is a CLI subcommand (`genie omni start`) that happens to be long-lived, not a genie daemon with its own state store. +4. If any docs under `docs/` reference the removed CLI commands, update them + +**Acceptance Criteria:** +- [ ] PR #1042 is closed (state = CLOSED, not MERGED) +- [ ] Closing comment is posted with wish link +- [ ] `genie-omni-marriage` wish has the footnote +- [ ] Docs are updated if needed + +**Validation:** +```bash +gh pr view 1042 --repo automagik-dev/genie --json state | jq -r '.state' +# Expected: "CLOSED" +``` + +**depends-on:** Group 11 + +--- + +### QA Group: 6-combination matrix test + +**Goal:** Prove the unified layer works for every (executor × source) combination. + +**Deliverables:** +1. Test plan at `.genie/wishes/unified-executor-layer/QA-MATRIX.md` +2. Execute all 6: + - tmux × human (`genie spawn engineer` in terminal) + - tmux × team (`genie team create foo --repo ... --wish ...`) + - tmux × omni (WhatsApp → `GENIE_EXECUTOR=tmux genie omni start`) + - sdk × human (`GENIE_EXECUTOR=sdk genie spawn engineer` — if supported; else skip with note) + - sdk × team (if supported; else skip) + - sdk × omni (WhatsApp → `GENIE_EXECUTOR=sdk genie omni start`) +3. For each: verify `genie ls` shows it, `genie sessions list` shows it, `genie events timeline ` returns audit events, `genie kill ` terminates cleanly + +**Acceptance Criteria:** +- [ ] All supported combinations verified +- [ ] Unsupported combinations explicitly skipped with reasoning +- [ ] Evidence captured in QA-MATRIX.md + +**Validation:** +```bash +cat .genie/wishes/unified-executor-layer/QA-MATRIX.md +# Should contain PASS/SKIP per row +``` + +**depends-on:** Group 11 + +--- + +## Dependencies + +``` +Wave 1 (parallel foundations) +Group 1 (audit) ──┐ +Group 2 (nats reply) ─┤ +Group 3 (pg optional) ┤ + │ +Wave 2 (core refactor, strict order inside) + └──→ Group 4 (register SDK in World A) + │ + ├──→ Group 5 (inline capture + audit enum) + │ │ + │ └──→ Group 7 (metadata index + lazy resume) + │ │ + │ └──→ Group 6 (refactor bridge, delegates to 7) + │ + └──────────────────────────→ Group 10 (--source filter) + │ +Wave 3 (CLI + flip switch) │ + Group 8 (GENIE_EXECUTOR) ←── Group 6 │ + Group 9 (collapse omni CLI) ←── Group 6 + Group 8 │ + │ +Wave 4 (cleanup + validation) │ + Group 11 (delete World B) ←── Group 6 + Group 9 │ + Group 12 (close PR #1042 + reconcile) ←── Group 11 │ + QA matrix ←── Group 11 │ + Review ←── all │ +``` + +--- + +## QA Criteria + +_What must be verified on dev after merge. QA agent tests each criterion._ + +### Functional +- [ ] `genie omni start` (with default or `GENIE_EXECUTOR=tmux`) works as before +- [ ] `GENIE_EXECUTOR=sdk genie omni start` starts the SDK-backed bridge +- [ ] A WhatsApp message to an SDK-backed agent gets a reply +- [ ] A WhatsApp message to a tmux-backed agent gets a reply +- [ ] Bridge restart mid-conversation: next WhatsApp message resumes the same Claude context (when TTL permits) +- [ ] `genie ls` shows all sessions regardless of source +- [ ] `genie ls --source omni` filters correctly +- [ ] `genie sessions replay ` shows content for omni sessions (both tmux and sdk backed) +- [ ] `genie events timeline ` shows full audit trail +- [ ] `genie kill ` terminates the session and bridge stops delivering to it + +### Integration +- [ ] Bridge process is managed by PM2 (unchanged) +- [ ] PG-optional: bridge starts and processes messages with PG offline +- [ ] `omni.reply..` NATS messages are published by the bridge (no subprocess forks) +- [ ] `session-capture.ts` filewatch still works for tmux sessions (unchanged) +- [ ] SDK sessions produce `session_content` rows via the inline capture path +- [ ] `audit_events` contains spawn/deliver/shutdown rows for both tmux and SDK sessions + +### Regression +- [ ] `genie spawn`, `genie team create`, `genie send` work as before +- [ ] `genie sessions list/replay/search` existing behavior unchanged +- [ ] `genie events` existing output shape unchanged +- [ ] TUI (if it reads sessions) unchanged or updated to handle the new source filter +- [ ] `bun run check` passes +- [ ] Existing tests all pass, including across-file test runs (not just individually — the bun `mock.module` leak from #1042 must be fixed or avoided) + +--- + +## Assumptions / Risks + +| Risk | Severity | Mitigation | +|------|----------|------------| +| World A `executors` schema doesn't perfectly fit SDK metadata needs | Medium | Use `metadata` JSONB for anything not in the column set. No migration needed. | +| `session-capture.ts` shape is tightly coupled to JSONL structure | Medium | Group 5 produces a minimal subset of that shape that downstream queries need. Verify `genie sessions replay` works against SDK-produced rows before merging Group 5. | +| Claude SDK resume TTL is shorter than operators expect | Low | Document the TTL in FLIP-SWITCH.md. Audit events make fallback visible. Not a correctness issue, just an expectations issue. | +| PR #1042 lands before this wish does | Medium | Group 9 explicitly handles the "if landed" case by deleting the added CLI commands. Group 11 handles the `omni_sessions` table if it was created (add a `DROP TABLE IF EXISTS` migration in a Group 11 sub-step). | +| `executor-registry` queries against `metadata->>'source'` / `metadata->>'chat_id'` hit on every inbound message | Resolved in wish | Group 7 ships migration 027 (`executors_omni_lookup` partial index on `(agent_id, metadata->>'source', metadata->>'chat_id') WHERE ended_at IS NULL`). Validation runs `EXPLAIN` to confirm index use. | +| Refactoring `omni-bridge.ts` introduces subtle concurrency bugs (queue depth, idle timer) | High | Wave 2 + Wave 3 keep existing tests passing. Group 6 explicitly preserves idle timer Map structure. QA matrix test exercises concurrency scenarios. | +| Tmux executor in World B may already be partially in World A | Low | Group 1 audit resolves this. If tmux is already registered via another code path, Group 4 touches only SDK and Group 11 has less to delete. | +| `genie config set executor` may require new config file plumbing | Low | If persistent config doesn't support it yet, just honor env var + flag; skip `genie config set` (acceptance criterion downgraded). | +| Deleting `services/executor.ts` breaks imports we missed | Medium | Group 1 audit catches this. Group 11 runs `bun run check` before merging. | + +--- + +## Post-Audit Decisions (2026-04-04) + +After Group 1's audit (`AUDIT.md`) surfaced findings that invalidated parts of the original wave plan, the team lead made three binding decisions with the user. These **override** any conflicting text in the Group sections above. + +### Decision 1 — Tmux executor scope: Option A (symmetric wire-up) + +**Finding (AUDIT §5a):** `ClaudeCodeOmniExecutor` has **zero** `createAndLinkExecutor`/`updateExecutorState`/`terminateExecutor` calls. The wish's claim that tmux is "already partially integrated" is wrong. Both World B executors are equally disconnected from World A. + +**Decision:** Group 4's scope expands to wire **both** executors symmetrically. Each executor keeps its own spawn/state/shutdown calls — no delegation, no shelling out to `genie spawn`. + +**Scope change for Group 4:** +- SDK path: `createAndLinkExecutor(agentId, 'claude', 'api', { claudeSessionId, metadata: { source: 'omni', chat_id, instance_id } })` +- Tmux path: `createAndLinkExecutor(agentId, 'claude', 'tmux', { tmuxSession, tmuxPaneId, tmuxWindow, tmuxWindowId, metadata: { source: 'omni', chat_id, instance_id } })` +- Both paths: `updateExecutorState` on transitions, `terminateExecutor` on shutdown +- All PG writes wrapped in `bridge.safePgCall(...)` (see Decision 2) + +**Scope change for Group 11:** `claude-code.ts` is **refactored**, not deleted. Its `sanitizeWindowName` export and `claude-code.test.ts` stay. + +**Rationale:** Each executor self-contained, no cross-file coupling, no new plumbing for reply-channel attachment. Accepts ~2x registry-call duplication as the tradeoff. + +### Decision 2 — safePgCall surface: public method on OmniBridge + +**Finding (AUDIT §5b.2):** `safePgCall` is `private` on `OmniBridge`. Groups 4/5/6/7 cannot import it. The helper is the **only** way downstream code is supposed to touch PG. + +**Decision:** Flip `private async safePgCall(...)` to `public async safePgCall(...)`. Groups 4/5/6/7 hold an `OmniBridge` reference and call `bridge.safePgCall('op-name', () => sql`...`, fallback, { executorId, chatId })`. + +**Scope change for Group 3 finish work:** Add the visibility flip as part of closing Group 3's remaining items. + +**Scope change for Groups 4/5/6/7:** Each group receives a reference to the `OmniBridge` instance (or its `safePgCall` bound method) rather than importing a module function. + +**Tradeoff accepted:** Executor code couples to `OmniBridge` class. Unit tests for Groups 4/5/6/7 need a bridge fixture or a stubbed `safePgCall` function parameter. + +### Decision 3 — Close both Group 3 gaps before Wave 2 + +**Finding (AUDIT §5b.2):** Group 3's scaffolding (`9382f299`) is landed but (a) migration-mismatch errors silently degrade instead of fail-fast, and (b) the slow-query 2s-timeout fallback test is missing. + +**Decision:** Close both gaps before Wave 2 dispatches. The wish's PG Error Handling Strategy table stands as-written — schema mismatch should fail-fast. + +**New scope for Group 3 finish:** +1. In `probePg()`: classify startup errors. If `isPgConnectionError(err)` → degrade (current behavior). Otherwise (schema mismatch, permission denied, etc.) → throw with a clear message pointing at the migration command. Add a unit test for the fail-fast path. +2. Add unit test: inject a slow PG read (delayed promise, >2s) through `safePgCall`; assert returned fallback, assert `pgAvailable` stays truthy (slow is not a connection error), assert delivery loop continues. +3. Make `safePgCall` public (Decision 2). +4. Validation: `bun test src/services/__tests__/omni-bridge.test.ts` — all existing tests continue to pass, two new tests added. + +**Depends-on:** nothing new. Runs in parallel with Group 1's completed audit review. + +**Blocks Wave 2:** yes. Group 4 needs `safePgCall` to be public before it can start. + +--- + +## Review Results + +_Populated by `/review` after execution completes._ + +--- + +## Files to Create/Modify + +``` +# In automagik-dev/genie (target repo) +src/lib/audit-events.ts (new — Group 5, shared enum) +src/lib/executor-config.ts (new — Group 8) +src/lib/executor-registry.ts (modify — add findLatestByMetadata, Group 7) +src/db/migrations/027_executors_omni_metadata_index.sql (new — Group 7, JSONB index) +src/services/executors/claude-sdk.ts (modify — Groups 2, 4, 5, 7) +src/services/executors/sdk-session-capture.ts (new — Group 5) +src/services/executors/__tests__/claude-sdk.test.ts (modify — Groups 2, 4) +src/services/executors/__tests__/claude-sdk-resume.test.ts (new — Group 7) +src/services/executors/__tests__/sdk-session-capture.test.ts (new — Group 5) +src/services/executor.ts (delete or demote — Group 11) +src/services/omni-bridge.ts (modify — Groups 3, 6) +src/services/__tests__/omni-bridge.test.ts (modify — Groups 3, 6) +src/term-commands/omni.ts (rewrite — Group 9) +src/term-commands/agent/list.ts (modify — Group 10) +src/term-commands/sessions.ts (modify — Group 10) +docs/sdk-executor-guide.md (update — Group 12) +.genie/wishes/genie-omni-marriage/WISH.md (append footnote — Group 12) + +# In this workspace (planning artifacts) +.genie/wishes/unified-executor-layer/WISH.md (this file) +.genie/wishes/unified-executor-layer/AUDIT.md (Group 1 output) +.genie/wishes/unified-executor-layer/FLIP-SWITCH.md (Group 8 docs) +.genie/wishes/unified-executor-layer/QA-MATRIX.md (QA Group output) + +# GitHub actions +Close PR automagik-dev/genie#1042 with closing comment (Group 12) +``` diff --git a/.gitignore b/.gitignore index 727e7858f..161ed75de 100644 --- a/.gitignore +++ b/.gitignore @@ -58,7 +58,6 @@ yarn-error.log* .genie/agents.json .genie/teams/ .genie/brainstorms/ -.genie/wishes/ # Team runtime state .genie/mailbox/ diff --git a/knip.json b/knip.json index 589d3cd7e..8b17a1d6b 100644 --- a/knip.json +++ b/knip.json @@ -5,6 +5,7 @@ "src/tui/render.tsx", "src/tui/tmux.ts", "src/lib/executor-types.ts", + "src/lib/safe-pg-call.ts", "src/lib/executor-registry.ts", "src/lib/assignment-registry.ts", "src/lib/providers/*.ts", @@ -20,14 +21,15 @@ "src/term-commands/exec/index.ts", "src/term-commands/brief.ts", "src/services/omni-reply.ts", - "src/services/omni-bridge.ts" + "src/services/omni-bridge.ts", + "src/lib/audit-events.ts", + "src/lib/executor-config.ts" ], "project": ["src/**/*.ts", "src/**/*.tsx"], - "ignoreBinaries": ["tmux", "which"], + "ignoreBinaries": ["which"], "ignoreExportsUsedInFile": false, "ignoreDependencies": [ "@automagik/genie-brain", - "esbuild", "@tauri-apps/cli", "@tauri-apps/api", "react-dom", diff --git a/src/db/migrations/026_executors_omni_metadata_index.sql b/src/db/migrations/026_executors_omni_metadata_index.sql new file mode 100644 index 000000000..6a4af1c7c --- /dev/null +++ b/src/db/migrations/026_executors_omni_metadata_index.sql @@ -0,0 +1,11 @@ +-- Partial index for omni-sourced executor lookups. +-- Covers: findLatestByMetadata in executor-registry (lazy resume), +-- bridge's find-or-create on inbound NATS message, +-- genie ls --source omni / genie sessions list --source omni. +CREATE INDEX IF NOT EXISTS executors_omni_lookup + ON executors ( + agent_id, + (metadata->>'source'), + (metadata->>'chat_id') + ) + WHERE ended_at IS NULL; diff --git a/src/genie.ts b/src/genie.ts index 8e192e68d..23becbebc 100644 --- a/src/genie.ts +++ b/src/genie.ts @@ -487,7 +487,8 @@ program .command('ls') .description('List registered agents with runtime status') .option('--json', 'Output as JSON') - .action(async (options: { json?: boolean }) => { + .option('--source ', 'Filter by executor metadata source (e.g. omni)') + .action(async (options: { json?: boolean; source?: string }) => { try { await handleLsCommand(options); } catch (error) { diff --git a/src/lib/__tests__/executor-config.test.ts b/src/lib/__tests__/executor-config.test.ts new file mode 100644 index 000000000..82cc4064b --- /dev/null +++ b/src/lib/__tests__/executor-config.test.ts @@ -0,0 +1,67 @@ +import { afterEach, beforeEach, describe, expect, test } from 'bun:test'; +import { resolveExecutorType } from '../executor-config.js'; + +describe('resolveExecutorType', () => { + let originalEnv: string | undefined; + + beforeEach(() => { + originalEnv = process.env.GENIE_EXECUTOR; + process.env.GENIE_EXECUTOR = undefined as unknown as string; + }); + + afterEach(() => { + if (originalEnv !== undefined) { + process.env.GENIE_EXECUTOR = originalEnv; + } else { + process.env.GENIE_EXECUTOR = undefined as unknown as string; + } + }); + + test('defaults to tmux when nothing is set', () => { + expect(resolveExecutorType()).toBe('tmux'); + }); + + test('override argument wins over everything', () => { + process.env.GENIE_EXECUTOR = 'tmux'; + expect(resolveExecutorType('sdk')).toBe('sdk'); + }); + + test('env var is used when no override', () => { + process.env.GENIE_EXECUTOR = 'sdk'; + expect(resolveExecutorType()).toBe('sdk'); + }); + + test('env var tmux is respected', () => { + process.env.GENIE_EXECUTOR = 'tmux'; + expect(resolveExecutorType()).toBe('tmux'); + }); + + test('override beats env var', () => { + process.env.GENIE_EXECUTOR = 'sdk'; + expect(resolveExecutorType('tmux')).toBe('tmux'); + }); + + test('invalid override falls through to env', () => { + process.env.GENIE_EXECUTOR = 'sdk'; + expect(resolveExecutorType('bogus')).toBe('sdk'); + }); + + test('invalid override and no env falls back to tmux', () => { + expect(resolveExecutorType('invalid')).toBe('tmux'); + }); + + test('invalid env falls back to tmux', () => { + process.env.GENIE_EXECUTOR = 'docker'; + expect(resolveExecutorType()).toBe('tmux'); + }); + + test('empty string override falls through', () => { + process.env.GENIE_EXECUTOR = 'sdk'; + expect(resolveExecutorType('')).toBe('sdk'); + }); + + test('undefined override falls through', () => { + process.env.GENIE_EXECUTOR = 'sdk'; + expect(resolveExecutorType(undefined)).toBe('sdk'); + }); +}); diff --git a/src/lib/audit-events.ts b/src/lib/audit-events.ts new file mode 100644 index 000000000..a286fb1b6 --- /dev/null +++ b/src/lib/audit-events.ts @@ -0,0 +1,64 @@ +/** + * Shared audit event types and recording helper. + * + * Used by the SDK executor (Group 5) and resume logic (Group 7). + * Writes to the `audit_events` table via safePgCall so degraded-mode + * (no PG) silently skips — no throw, no data loss risk. + */ + +import type { SafePgCallFn } from './safe-pg-call.js'; + +// ============================================================================ +// Audit event type union +// ============================================================================ + +export type AuditEventType = + // Lifecycle + | 'executor.spawn' + | 'executor.shutdown' + | 'executor.state_transition' + // Delivery + | 'deliver.start' + | 'deliver.end' + | 'deliver.error' + | 'deliver.tool_use' + // Resume (Group 7) + | 'session.resumed' + | 'session.resume_rejected' + | 'session.created_fresh'; + +// ============================================================================ +// Record helper +// ============================================================================ + +/** + * Write an audit event row via safePgCall. + * + * Maps to the real audit_events schema: + * entity_type, entity_id, event_type, actor, details, created_at + * + * `entityType` defaults to 'executor' when not provided in attrs. + * `entityId` defaults to attrs.executor_id. + * `actor` defaults to attrs.agent_id. + */ +export async function recordAuditEvent( + safePgCall: SafePgCallFn, + type: AuditEventType, + attrs: Record, +): Promise { + const entityType = (attrs.entity_type as string) ?? 'executor'; + const entityId = (attrs.entity_id as string) ?? (attrs.executor_id as string) ?? ''; + const actor = (attrs.actor as string) ?? (attrs.agent_id as string) ?? null; + + // Strip meta keys from details — they're top-level columns already. + const { entity_type: _et, entity_id: _eid, actor: _a, ...details } = attrs; + + await safePgCall( + `audit:${type}`, + (sql) => + sql`INSERT INTO audit_events (entity_type, entity_id, event_type, actor, details) + VALUES (${entityType}, ${entityId}, ${type}, ${actor}, ${JSON.stringify(details)})`, + undefined, + { executorId: entityId, chatId: (attrs.chat_id as string) ?? '' }, + ); +} diff --git a/src/lib/executor-config.ts b/src/lib/executor-config.ts new file mode 100644 index 000000000..ed1be0dc2 --- /dev/null +++ b/src/lib/executor-config.ts @@ -0,0 +1,40 @@ +/** + * Executor type resolver. + * + * Resolution order: + * 1. Explicit override argument (CLI --executor flag) + * 2. GENIE_EXECUTOR env var + * 3. Persisted config (~/.genie/config.json → omni.executor) + * 4. Default: 'tmux' + */ + +import { loadGenieConfigSync } from './genie-config.js'; + +export type ExecutorType = 'tmux' | 'sdk'; + +const VALID: ReadonlySet = new Set(['tmux', 'sdk']); + +function isValid(value: unknown): value is ExecutorType { + return typeof value === 'string' && VALID.has(value); +} + +export function resolveExecutorType(override?: string): ExecutorType { + // 1. Explicit override + if (isValid(override)) return override; + + // 2. Env var + const env = process.env.GENIE_EXECUTOR; + if (isValid(env)) return env; + + // 3. Persisted config + try { + const cfg = loadGenieConfigSync(); + const persisted = cfg.omni?.executor; + if (isValid(persisted)) return persisted; + } catch { + // Config unreadable — fall through + } + + // 4. Default + return 'tmux'; +} diff --git a/src/lib/executor-registry.test.ts b/src/lib/executor-registry.test.ts index 03fa065f4..24e736101 100644 --- a/src/lib/executor-registry.test.ts +++ b/src/lib/executor-registry.test.ts @@ -236,6 +236,41 @@ describe.skipIf(!DB_AVAILABLE)('executor-registry', () => { expect(list[0].id).toBe(e2.id); // Newest first expect(list[1].id).toBe(e1.id); }); + + test('filters by metadata source', async () => { + const a1 = await seedAgent('omni-agent', 'team1'); + const a2 = await seedAgent('cli-agent', 'team1'); + await createExecutor(a1, 'claude', 'api', { metadata: { source: 'omni', chat_id: 'c1' } }); + await createExecutor(a2, 'claude', 'tmux'); + await createExecutor(a2, 'claude', 'tmux', { metadata: { source: 'cli' } }); + + const omniOnly = await listExecutors(undefined, 'omni'); + expect(omniOnly.length).toBe(1); + expect(omniOnly[0].agentId).toBe(a1); + expect(omniOnly[0].metadata).toEqual({ source: 'omni', chat_id: 'c1' }); + + const cliOnly = await listExecutors(undefined, 'cli'); + expect(cliOnly.length).toBe(1); + expect(cliOnly[0].agentId).toBe(a2); + + // No source returns all + const all = await listExecutors(); + expect(all.length).toBe(3); + }); + + test('filters by both agent ID and source', async () => { + const a1 = await seedAgent('multi-agent', 'team1'); + await createExecutor(a1, 'claude', 'api', { metadata: { source: 'omni', chat_id: 'c1' } }); + await createExecutor(a1, 'claude', 'tmux'); + + const filtered = await listExecutors(a1, 'omni'); + expect(filtered.length).toBe(1); + expect(filtered[0].metadata).toEqual({ source: 'omni', chat_id: 'c1' }); + + // Agent filter alone returns both + const allForAgent = await listExecutors(a1); + expect(allForAgent.length).toBe(2); + }); }); // ========================================================================== diff --git a/src/lib/executor-registry.ts b/src/lib/executor-registry.ts index b46ed35ce..77d5e08ef 100644 --- a/src/lib/executor-registry.ts +++ b/src/lib/executor-registry.ts @@ -180,12 +180,16 @@ export async function terminateActiveExecutor(agentId: string): Promise { await sql`UPDATE agents SET current_executor_id = NULL WHERE id = ${agentId} AND current_executor_id = ${executorId}`; } -/** List executors, optionally filtered by agent ID. */ -export async function listExecutors(agentId?: string): Promise { +/** List executors, optionally filtered by agent ID and/or metadata source. */ +export async function listExecutors(agentId?: string, source?: string): Promise { const sql = await getConnection(); - const rows = agentId - ? await sql`SELECT * FROM executors WHERE agent_id = ${agentId} ORDER BY started_at DESC` - : await sql`SELECT * FROM executors ORDER BY started_at DESC`; + const rows = await sql` + SELECT * FROM executors + WHERE true + ${agentId ? sql`AND agent_id = ${agentId}` : sql``} + ${source ? sql`AND metadata->>'source' = ${source}` : sql``} + ORDER BY started_at DESC + `; return rows.map(rowToExecutor); } @@ -205,3 +209,39 @@ export async function findExecutorBySession(claudeSessionId: string): Promise 0 ? rowToExecutor(rows[0]) : null; } + +/** + * Find the latest live executor matching omni metadata. + * Used for lazy resume: on bridge restart, look up an existing executor + * for this agent + chat combination so we can reuse its Claude session. + * Uses the `executors_omni_lookup` partial index (migration 026). + */ +export async function findLatestByMetadata(filter: { + agentId: string; + source: string; + chatId: string; +}): Promise { + const sql = await getConnection(); + const rows = await sql` + SELECT * FROM executors + WHERE agent_id = ${filter.agentId} + AND metadata->>'source' = ${filter.source} + AND metadata->>'chat_id' = ${filter.chatId} + AND ended_at IS NULL + ORDER BY started_at DESC + LIMIT 1 + `; + return rows.length > 0 ? rowToExecutor(rows[0]) : null; +} + +/** Relink an existing executor to an agent (set current_executor_id FK). */ +export async function relinkExecutorToAgent(executorId: string, agentId: string): Promise { + const sql = await getConnection(); + await sql`UPDATE agents SET current_executor_id = ${executorId} WHERE id = ${agentId}`; +} + +/** Update the Claude session ID on an executor row. */ +export async function updateClaudeSessionId(executorId: string, sessionId: string): Promise { + const sql = await getConnection(); + await sql`UPDATE executors SET claude_session_id = ${sessionId} WHERE id = ${executorId}`; +} diff --git a/src/lib/safe-pg-call.ts b/src/lib/safe-pg-call.ts new file mode 100644 index 000000000..f87d3679a --- /dev/null +++ b/src/lib/safe-pg-call.ts @@ -0,0 +1,23 @@ +/** + * SafePgCallFn — Structural type for the bridge's PG-guarded call pattern. + * + * Extracted from services/executor.ts so that lib/ modules (audit-events, + * sdk-session-capture) can reference it without a cross-layer import. + */ + +import type { Sql } from './db.js'; + +/** + * Bound `safePgCall` injected by the bridge into each executor after construction. + * + * Mirror of `OmniBridge#safePgCall` — declared here as a structural type so + * executors can call it without importing `OmniBridge` (avoids a circular + * dependency between the bridge and its own executors). Tests pass a plain + * function; production wires `bridge.safePgCall.bind(bridge)`. + */ +export type SafePgCallFn = ( + op: string, + fn: (sql: Sql) => Promise, + fallback: T, + ctx?: { executorId?: string; chatId?: string }, +) => Promise; diff --git a/src/services/__tests__/omni-bridge.test.ts b/src/services/__tests__/omni-bridge.test.ts new file mode 100644 index 000000000..b3a5b50ae --- /dev/null +++ b/src/services/__tests__/omni-bridge.test.ts @@ -0,0 +1,356 @@ +/** + * Omni Bridge — PG degraded mode tests (Group 3). + * + * Covers every row of the wish's PG Error Handling Strategy table: + * - Startup connection failure → degrade gracefully, pgAvailable=false + * - Startup schema mismatch → fail-fast with actionable error + * - Mid-run connection loss → safePgCall returns fallback + flips pgAvailable=false + * - Runtime non-connection err → safePgCall returns fallback, pgAvailable stays true + * - Slow query > 2s timeout → safePgCall returns fallback, pgAvailable stays true + * - Happy path → safePgCall forwards the fn result + * + * All tests inject fake NATS + PG so the suite stays hermetic — no real + * nats-server or postgres required (bun:test preload already boots a test + * pgserve, but this file deliberately avoids it to exercise the error paths). + */ + +import { describe, expect, it } from 'bun:test'; +import type { NatsConnection, Subscription } from 'nats'; + +import { OmniBridge } from '../omni-bridge.js'; + +// ---------------------------------------------------------------------------- +// Fakes +// ---------------------------------------------------------------------------- + +/** Build a minimal NatsConnection stub — no real socket. */ +function makeFakeNats(): NatsConnection { + const fakeSub: Partial & AsyncIterable = { + unsubscribe: () => { + /* no-op */ + }, + // Empty async iterator — `for await ... of` exits immediately, + // so processSubscription() returns without blocking. + [Symbol.asyncIterator]: async function* () { + // yields nothing + }, + }; + + const fake: Partial = { + info: undefined, + closed: async () => undefined, + close: async () => undefined, + drain: async () => undefined, + publish: () => { + /* no-op */ + }, + subscribe: () => fakeSub as Subscription, + }; + + return fake as NatsConnection; +} + +/** Make a minimal postgres.js tagged-template client that returns a stock row. */ +function makeFakeSql(result: unknown = [{ one: 1 }]): any { + // postgres.js's Sql type is a tagged-template function. A plain function + // with the same call signature is assignment-compatible via `any`. + return (_strings: TemplateStringsArray, ..._values: unknown[]) => Promise.resolve(result); +} + +/** Inject fakes into the bridge constructor. */ +function makeBridge(overrides: { pgProvider: () => Promise; natsConnectFn?: any }) { + return new OmniBridge({ + natsUrl: 'test://fake-nats', + pgProvider: overrides.pgProvider, + natsConnectFn: overrides.natsConnectFn ?? ((async () => makeFakeNats()) as any), + }); +} + +// ---------------------------------------------------------------------------- +// Tests +// ---------------------------------------------------------------------------- + +describe('OmniBridge — PG degraded mode', () => { + it('Test A: start() succeeds and status().pgAvailable=false when PG provider throws', async () => { + const bridge = makeBridge({ + pgProvider: async () => { + // Simulates a broken PG connection string — provider cannot build a client. + const err = new Error('connect ECONNREFUSED 127.0.0.1:1'); + (err as any).code = 'ECONNREFUSED'; + throw err; + }, + }); + + await bridge.start(); + try { + const s = await bridge.status(); + expect(s.connected).toBe(true); // NATS connected via fake + expect(s.pgAvailable).toBe(false); // PG degraded + expect(s.natsUrl).toBe('test://fake-nats'); + } finally { + await bridge.stop(); + } + }); + + it('Test A (variant): degrades when SELECT 1 probe itself throws a connection error', async () => { + const bridge = makeBridge({ + pgProvider: async () => { + // Provider returns a client, but SELECT 1 fails with a connection-level error. + return ((_s: TemplateStringsArray, ..._v: unknown[]) => + Promise.reject(new Error('connection terminated unexpectedly'))) as any; + }, + }); + + await bridge.start(); + try { + expect((await bridge.status()).pgAvailable).toBe(false); + expect((await bridge.status()).connected).toBe(true); + } finally { + await bridge.stop(); + } + }); + + it('Test A (fail-fast): throws when probePg hits a schema mismatch (non-connection error)', async () => { + const bridge = makeBridge({ + pgProvider: async () => { + // Provider returns a client, but SELECT 1 fails with a schema-level error. + // This is the "migration missing / schema mismatch" row from the wish's + // PG Error Handling Strategy table — must fail-fast, not degrade. + return ((_s: TemplateStringsArray, ..._v: unknown[]) => + Promise.reject(new Error('relation "sessions" does not exist'))) as any; + }, + }); + + // start() should propagate the error with a migration hint. + let caught: unknown; + try { + await bridge.start(); + } catch (err) { + caught = err; + } + + expect(caught).toBeInstanceOf(Error); + const msg = (caught as Error).message; + expect(msg).toContain('PG schema mismatch'); + expect(msg).toContain('relation "sessions" does not exist'); + expect(msg.toLowerCase()).toContain('migrate'); + }); + + it('Test A (fail-fast variant): throws when the provider itself throws a non-connection error', async () => { + const bridge = makeBridge({ + pgProvider: async () => { + const err = new Error('permission denied for table executors'); + (err as any).code = '42501'; // postgres.js error code, not a network code + throw err; + }, + }); + + let caught: unknown; + try { + await bridge.start(); + } catch (err) { + caught = err; + } + + expect(caught).toBeInstanceOf(Error); + expect((caught as Error).message).toContain('PG schema mismatch'); + expect((caught as Error).message).toContain('permission denied'); + }); + + it('Test B: safePgCall returns fallback and flips pgAvailable on mid-run connection loss', async () => { + const bridge = makeBridge({ pgProvider: async () => makeFakeSql() }); + await bridge.start(); + + try { + // Startup probe succeeded — we begin in the healthy state. + expect((await bridge.status()).pgAvailable).toBe(true); + + // Simulate a mid-run connection loss. safePgCall must: + // (a) return the fallback value + // (b) flip pgAvailable to false + // (c) NOT throw (delivery loop must stay alive) + const fallback = { recovered: false, id: null as string | null }; + const result = await bridge.safePgCall( + 'executor_state_update', + async () => { + throw new Error('connection terminated unexpectedly'); + }, + fallback, + { executorId: 'exec-abc', chatId: 'chat-xyz' }, + ); + + expect(result).toBe(fallback); + expect((await bridge.status()).pgAvailable).toBe(false); + + // Delivery loop continuity proxy: further safePgCall invocations are + // fast-pathed to fallback without invoking fn. + let secondInvoked = false; + const second = await bridge.safePgCall( + 'audit_event_insert', + async () => { + secondInvoked = true; + return 'should-not-be-returned'; + }, + 'FALLBACK_2' as const, + ); + expect(second).toBe('FALLBACK_2'); + expect(secondInvoked).toBe(false); + } finally { + await bridge.stop(); + } + }); + + it('Test B (variant): non-connection PG errors keep pgAvailable=true', async () => { + const bridge = makeBridge({ pgProvider: async () => makeFakeSql() }); + await bridge.start(); + + try { + expect((await bridge.status()).pgAvailable).toBe(true); + + // A SQL-level error (e.g., constraint violation) must NOT degrade the bridge. + const result = await bridge.safePgCall( + 'session_content_insert', + async () => { + throw new Error('duplicate key value violates unique constraint'); + }, + null, + ); + expect(result).toBeNull(); + // Still healthy — only connection-level errors flip the flag. + expect((await bridge.status()).pgAvailable).toBe(true); + } finally { + await bridge.stop(); + } + }); + + it('safePgCall returns fn result when PG is healthy', async () => { + const bridge = makeBridge({ pgProvider: async () => makeFakeSql() }); + await bridge.start(); + + try { + const result = await bridge.safePgCall('ping', async () => ({ value: 42 }), { value: -1 }); + expect(result).toEqual({ value: 42 }); + expect((await bridge.status()).pgAvailable).toBe(true); + } finally { + await bridge.stop(); + } + }); + + it('safePgCall short-circuits to fallback when pgAvailable=false from startup', async () => { + const bridge = makeBridge({ + pgProvider: async () => { + throw new Error('ECONNREFUSED'); + }, + }); + await bridge.start(); + + try { + expect((await bridge.status()).pgAvailable).toBe(false); + + let invoked = false; + const result = await bridge.safePgCall( + 'lazy_resume_lookup', + async () => { + invoked = true; + return 'never'; + }, + 'degraded', + ); + expect(result).toBe('degraded'); + expect(invoked).toBe(false); + } finally { + await bridge.stop(); + } + }); + + it('slow-query: safePgCall returns fallback when fn exceeds PG_RUNTIME_QUERY_TIMEOUT_MS, pgAvailable stays true', async () => { + // The wish mandates a 2s read budget. Inject a fn that delays beyond that. + // A slow query is NOT a connection-level error, so pgAvailable must stay + // truthy — the next call gets a fresh attempt. + const bridge = makeBridge({ pgProvider: async () => makeFakeSql() }); + await bridge.start(); + + try { + expect((await bridge.status()).pgAvailable).toBe(true); + + const started = Date.now(); + const result = await bridge.safePgCall( + 'lazy_resume_lookup', + () => + new Promise((resolve) => { + // 2500ms > 2000ms runtime budget → withTimeout rejects first + const t = setTimeout(() => resolve('too-late'), 2500); + t.unref?.(); + }), + 'fallback-on-timeout', + { chatId: 'chat-slow' }, + ); + const elapsed = Date.now() - started; + + expect(result).toBe('fallback-on-timeout'); + // Should resolve close to the 2s budget, not wait the full 2.5s. + expect(elapsed).toBeGreaterThanOrEqual(1900); + expect(elapsed).toBeLessThan(2400); + // Critical: timeout != connection loss. Next call should still try fn. + expect((await bridge.status()).pgAvailable).toBe(true); + + // Proves the flag really held: a fast follow-up call succeeds. + const follow = await bridge.safePgCall('ping', async () => 'ok', 'fallback'); + expect(follow).toBe('ok'); + } finally { + await bridge.stop(); + } + }); + + it('status() queries PG for active executor count when pgAvailable=true', async () => { + // Build a fake SQL that returns 3 active omni executors when status() queries. + const fakeRows = [{ id: 'exec-aaa' }, { id: 'exec-bbb' }, { id: 'exec-ccc' }]; + const fakeSql = (strings: TemplateStringsArray, ..._values: unknown[]) => { + const query = strings.join(''); + // The status() query selects from executors with source='omni' and ended_at IS NULL. + if (query.includes('executors') && query.includes('source')) { + return Promise.resolve(fakeRows); + } + // Default: SELECT 1 probe + return Promise.resolve([{ one: 1 }]); + }; + + const bridge = makeBridge({ pgProvider: async () => fakeSql as any }); + await bridge.start(); + + try { + expect((await bridge.status()).pgAvailable).toBe(true); + + const s = await bridge.status(); + // activeSessions should come from PG (3), not the local Map (0). + expect(s.activeSessions).toBe(3); + expect(s.executorIds).toEqual(['exec-aaa', 'exec-bbb', 'exec-ccc']); + // Local sessions Map is empty — no actual spawns happened. + expect(s.sessions).toHaveLength(0); + } finally { + await bridge.stop(); + } + }); + + it('status() falls back to local Map size when pgAvailable=false', async () => { + const bridge = makeBridge({ + pgProvider: async () => { + const err = new Error('connect ECONNREFUSED 127.0.0.1:5432'); + (err as any).code = 'ECONNREFUSED'; + throw err; + }, + }); + await bridge.start(); + + try { + expect((await bridge.status()).pgAvailable).toBe(false); + + const s = await bridge.status(); + // No PG → falls back to local Map size (0, since no sessions spawned). + expect(s.activeSessions).toBe(0); + expect(s.executorIds).toEqual([]); + } finally { + await bridge.stop(); + } + }); +}); diff --git a/src/services/executor.ts b/src/services/executor.ts index db62b71e5..9c4e4a5db 100644 --- a/src/services/executor.ts +++ b/src/services/executor.ts @@ -1,37 +1,24 @@ /** - * IExecutor — Executor interface for the Omni bridge. + * World B executor interface — TRANSITIONAL, targeted for removal. * - * Simpler than the internal ExecutorProvider: this interface is what - * the NATS bridge uses to spawn/manage agent sessions per chat. - * Claude Code tmux is the first implementation. Claude SDK (1 process, - * N chats) is the scaling path. - * - * The executor is stateless — Genie (omni-bridge) manages session - * lifecycle, idle timeouts, and concurrency limits. The executor - * just runs agents. + * TODO(unified-executor-layer): Replace OmniSession with World A's Executor + * type from lib/executor-types.ts. This requires the bridge's SessionEntry to + * store an Executor row instead of OmniSession, and spawn() to return Executor. + * Once done, IExecutor collapses into ExecutorProvider and this file is deleted. */ -// ============================================================================ -// Types -// ============================================================================ +// SafePgCallFn relocated to lib/safe-pg-call.ts — re-export for back-compat. +export type { SafePgCallFn } from '../lib/safe-pg-call.js'; -/** Opaque session handle returned by spawn(). */ +/** Opaque session handle returned by spawn(). TODO: replace with Executor from lib/executor-types. */ export interface OmniSession { - /** Unique session key (typically `${agentName}:${chatId}`). */ id: string; - /** Agent name (from genie directory). */ agentName: string; - /** Chat ID from Omni (WhatsApp thread). */ chatId: string; - /** Tmux session name hosting this window. */ tmuxSession: string; - /** Tmux window name. */ tmuxWindow: string; - /** Tmux pane ID (e.g., "%5"). */ paneId: string; - /** Timestamp of session creation. */ createdAt: number; - /** Timestamp of last message delivered. */ lastActivityAt: number; } @@ -39,36 +26,17 @@ export interface OmniSession { export interface OmniMessage { content: string; sender: string; - /** Omni instance ID. */ instanceId: string; - /** Chat/thread ID. */ chatId: string; - /** Agent role to route to. */ agent: string; - /** ISO timestamp. */ timestamp?: string; } -// ============================================================================ -// Interface -// ============================================================================ - -/** - * IExecutor — pluggable executor backend for the Omni bridge. - * - * Each executor type handles reply routing its own way but always - * publishes to `omni.reply.{instance}.{chat_id}`. - */ +/** Pluggable executor backend for the Omni bridge. TODO: merge into ExecutorProvider. */ export interface IExecutor { - /** Spawn an agent session for a chat. */ spawn(agentName: string, chatId: string, env: Record): Promise; - - /** Deliver a message to an already-running session. */ deliver(session: OmniSession, message: OmniMessage): Promise; - - /** Shut down a session (kill tmux window). */ shutdown(session: OmniSession): Promise; - - /** Check if a session is still alive. */ isAlive(session: OmniSession): Promise; + setSafePgCall(fn: import('../lib/safe-pg-call.js').SafePgCallFn): void; } diff --git a/src/services/executors/__tests__/claude-sdk-resume.test.ts b/src/services/executors/__tests__/claude-sdk-resume.test.ts new file mode 100644 index 000000000..48b8b2eb9 --- /dev/null +++ b/src/services/executors/__tests__/claude-sdk-resume.test.ts @@ -0,0 +1,474 @@ +/** + * Tests for Group 7 — lazy resume via executors table + metadata index. + * + * Covers: + * - spawn() reuses an existing executor when findLatestByMetadata returns one + * - spawn() creates a fresh executor when no match found + * - deliver() detects resume rejection (SDK returns different session ID) + * - deliver() persists session ID to registry on first query + * - Audit events: session.resumed, session.created_fresh, session.resume_rejected + */ + +import { afterAll, beforeEach, describe, expect, it, mock } from 'bun:test'; + +// Clean up process-global mock.module registrations when this file finishes. +// Without this, mocked modules leak into later test files (bun mock.module +// is process-global and persists across file boundaries). +afterAll(() => { + mock.restore(); +}); + +// ============================================================================ +// Mocks — must be registered before any import of the production module +// ============================================================================ + +// Mock agent directory +mock.module('../../../lib/agent-directory.js', () => ({ + resolve: mock(async (name: string) => ({ + entry: { + name, + dir: '/tmp/test', + promptMode: 'system' as const, + model: 'sonnet', + registeredAt: new Date().toISOString(), + permissions: { preset: 'full' }, + }, + builtin: false, + })), + loadIdentity: mock(() => null), +})); + +// Mock SDK query — default yields a result with session_id +const queryMock = mock(() => { + const gen = (async function* () { + yield { type: 'assistant', message: { content: [{ type: 'text', text: 'reply' }] } }; + yield { type: 'result', subtype: 'success', session_id: 'sdk-session-aaa' }; + })(); + return Object.assign(gen, { + interrupt: mock(), + setPermissionMode: mock(), + setModel: mock(), + return: mock(async () => ({ value: undefined, done: true })), + throw: mock(async () => ({ value: undefined, done: true })), + }); +}); +mock.module('@anthropic-ai/claude-agent-sdk', () => ({ + query: queryMock, +})); + +// Mock audit-events — capture calls for assertion. +// NOTE: audit-events is intentionally NOT mocked via mock.module here. +// mock.module leaks across files and breaks sdk-session-capture.test.ts. +// Instead, audit events are tracked via happySafePgCall's call log — +// recordAuditEvent calls safePgCall('audit:', ...) so we capture +// the event type from the op string prefix. + +// Mock sdk-session-capture (Group 5) +// NOTE: sdk-session-capture is intentionally NOT mocked here. +// The real module is used — its PG writes go through safePgCall which is +// wired to happySafePgCall (a no-op that invokes fn with a fake sql). +// Previously this was mocked via mock.module, but that leaked across files +// and broke sdk-session-capture.test.ts (bun mock.module is process-global). + +// Mock agent-registry +const findOrCreateAgentMock = mock(async () => ({ + id: 'agent-id-fixture', + startedAt: new Date().toISOString(), + currentExecutorId: null, +})); +mock.module('../../../lib/agent-registry.js', () => ({ + findOrCreateAgent: findOrCreateAgentMock, +})); + +// Mock executor-registry — Group 7 functions. +// Use `as any` return types so mockImplementation can return richer objects. +const findLatestByMetadataMock = mock((_filter: any): Promise => Promise.resolve(null)); +const relinkExecutorToAgentMock = mock((..._args: any[]): Promise => Promise.resolve()); +const updateClaudeSessionIdMock = mock((..._args: any[]): Promise => Promise.resolve()); +const createAndLinkExecutorMock = mock(async () => ({ + id: 'fresh-executor-id', + agentId: 'agent-id-fixture', + provider: 'claude', + transport: 'api', + state: 'spawning', + metadata: {}, + claudeSessionId: null, +})); +const updateExecutorStateMock = mock(async () => undefined); +const terminateExecutorMock = mock(async () => undefined); +mock.module('../../../lib/executor-registry.js', () => ({ + findLatestByMetadata: findLatestByMetadataMock, + relinkExecutorToAgent: relinkExecutorToAgentMock, + updateClaudeSessionId: updateClaudeSessionIdMock, + createAndLinkExecutor: createAndLinkExecutorMock, + updateExecutorState: updateExecutorStateMock, + terminateExecutor: terminateExecutorMock, +})); + +// ============================================================================ +// Import production module (after all mocks) +// ============================================================================ + +const { ClaudeSdkOmniExecutor } = await import('../claude-sdk.js'); + +// ============================================================================ +// Helpers +// ============================================================================ + +/** Log of safePgCall operations — tracks audit events without mock.module. */ +interface PgCallEntry { + op: string; + sqlValues: unknown[]; +} +const safePgCallLog: PgCallEntry[] = []; + +/** Fake bridge.safePgCall that invokes fn with a tracking sql template. */ +const happySafePgCall = async ( + op: string, + fn: (sql: any) => Promise, + _fallback: T, + _ctx?: { executorId?: string; chatId?: string }, +): Promise => { + const entry: PgCallEntry = { op, sqlValues: [] }; + safePgCallLog.push(entry); + const trackingSql = (_strings: TemplateStringsArray, ...values: unknown[]) => { + entry.sqlValues = values; + return [{ id: values[0] ?? 'fake-id' }]; + }; + return fn(trackingSql); +}; + +/** Build a minimal OmniMessage. */ +function mkMsg(chatId: string, instanceId = 'inst-1') { + return { + content: 'Hello', + sender: 'user', + instanceId, + chatId, + agent: 'test-agent', + }; +} + +/** Extract audit event types from safePgCall log (op = 'audit:'). */ +function auditEventTypes(): string[] { + return safePgCallLog.filter((e) => e.op.startsWith('audit:')).map((e) => e.op.slice('audit:'.length)); +} + +// ============================================================================ +// Tests +// ============================================================================ + +describe('ClaudeSdkOmniExecutor — lazy resume (Group 7)', () => { + let executor: InstanceType; + + beforeEach(() => { + executor = new ClaudeSdkOmniExecutor(); + executor.setSafePgCall(happySafePgCall); + executor.setNatsPublish(mock()); + + // Reset all mocks + findOrCreateAgentMock.mockClear(); + findLatestByMetadataMock.mockClear(); + relinkExecutorToAgentMock.mockClear(); + updateClaudeSessionIdMock.mockClear(); + createAndLinkExecutorMock.mockClear(); + updateExecutorStateMock.mockClear(); + terminateExecutorMock.mockClear(); + safePgCallLog.length = 0; + + // Default: no existing executor (fresh creation path) + findLatestByMetadataMock.mockImplementation(async () => null); + }); + + // ========================================================================== + // spawn() — fresh creation + // ========================================================================== + + describe('spawn — no existing executor', () => { + it('creates a fresh executor and writes session.created_fresh audit event', async () => { + await executor.spawn('test-agent', 'chat-fresh', { OMNI_INSTANCE_ID: 'inst-1' }); + + // findLatestByMetadata was called to look for an existing executor + expect(findLatestByMetadataMock).toHaveBeenCalledTimes(1); + const filter = findLatestByMetadataMock.mock.calls[0][0]; + expect(filter).toMatchObject({ + agentId: 'agent-id-fixture', + source: 'omni', + chatId: 'chat-fresh', + }); + + // No existing executor found → createAndLinkExecutor was called + expect(createAndLinkExecutorMock).toHaveBeenCalledTimes(1); + expect(relinkExecutorToAgentMock).not.toHaveBeenCalled(); + + // session.created_fresh audit event was written + expect(auditEventTypes()).toContain('session.created_fresh'); + }); + }); + + // ========================================================================== + // spawn() — resume existing executor + // ========================================================================== + + describe('spawn — existing executor with claudeSessionId', () => { + const existingExecutor = { + id: 'existing-exec-id', + agentId: 'agent-id-fixture', + provider: 'claude' as const, + transport: 'api' as const, + pid: null, + tmuxSession: null, + tmuxPaneId: null, + tmuxWindow: null, + tmuxWindowId: null, + claudeSessionId: 'resume-session-xyz', + state: 'idle' as const, + metadata: { source: 'omni', chat_id: 'chat-resume' }, + worktree: null, + repoPath: null, + paneColor: null, + startedAt: new Date().toISOString(), + endedAt: null, + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + }; + + beforeEach(() => { + findLatestByMetadataMock.mockImplementation(async () => existingExecutor); + }); + + it('reuses existing executor and does NOT create a new one', async () => { + await executor.spawn('test-agent', 'chat-resume', { OMNI_INSTANCE_ID: 'inst-1' }); + + // Should NOT create a new executor + expect(createAndLinkExecutorMock).not.toHaveBeenCalled(); + + // Should relink existing executor to agent + expect(relinkExecutorToAgentMock).toHaveBeenCalledTimes(1); + const [execId, agentId] = relinkExecutorToAgentMock.mock.calls[0]; + expect(execId).toBe('existing-exec-id'); + expect(agentId).toBe('agent-id-fixture'); + }); + + it('writes session.resumed audit event', async () => { + await executor.spawn('test-agent', 'chat-resume', { OMNI_INSTANCE_ID: 'inst-1' }); + expect(auditEventTypes()).toContain('session.resumed'); + expect(auditEventTypes()).not.toContain('session.created_fresh'); + }); + + it('passes claudeSessionId as resume parameter on first delivery', async () => { + const session = await executor.spawn('test-agent', 'chat-resume', { OMNI_INSTANCE_ID: 'inst-1' }); + + await executor.deliver(session, mkMsg('chat-resume')); + await executor.waitForDeliveries(session.id); + + // The SDK query mock was called — the resume parameter is passed + // through extraOptions inside runQuery. We verify the query ran. + expect(queryMock).toHaveBeenCalled(); + }); + }); + + // ========================================================================== + // spawn() — existing executor WITHOUT claudeSessionId + // ========================================================================== + + describe('spawn — existing executor without claudeSessionId', () => { + beforeEach(() => { + findLatestByMetadataMock.mockImplementation(async () => ({ + id: 'existing-no-session', + agentId: 'agent-id-fixture', + provider: 'claude', + transport: 'api', + pid: null, + tmuxSession: null, + tmuxPaneId: null, + tmuxWindow: null, + tmuxWindowId: null, + claudeSessionId: null, + state: 'idle', + metadata: { source: 'omni', chat_id: 'chat-no-session' }, + worktree: null, + repoPath: null, + paneColor: null, + startedAt: new Date().toISOString(), + endedAt: null, + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + })); + }); + + it('still reuses executor — session ID will be set on first query', async () => { + await executor.spawn('test-agent', 'chat-no-session', { OMNI_INSTANCE_ID: 'inst-1' }); + + expect(createAndLinkExecutorMock).not.toHaveBeenCalled(); + expect(relinkExecutorToAgentMock).toHaveBeenCalledTimes(1); + expect(auditEventTypes()).toContain('session.resumed'); + }); + + it('persists session ID to registry after first query returns one', async () => { + const session = await executor.spawn('test-agent', 'chat-no-session', { OMNI_INSTANCE_ID: 'inst-1' }); + updateClaudeSessionIdMock.mockClear(); + + await executor.deliver(session, mkMsg('chat-no-session')); + await executor.waitForDeliveries(session.id); + + // The query returned session_id 'sdk-session-aaa' (from the mock). + // Since the executor had no claudeSessionId, this is a new session ID + // → updateClaudeSessionId should have been called. + expect(updateClaudeSessionIdMock).toHaveBeenCalledTimes(1); + const [execId, sessId] = updateClaudeSessionIdMock.mock.calls[0]; + expect(execId).toBe('existing-no-session'); + expect(sessId).toBe('sdk-session-aaa'); + }); + }); + + // ========================================================================== + // deliver() — resume rejection + // ========================================================================== + + describe('deliver — resume rejected by SDK', () => { + beforeEach(() => { + findLatestByMetadataMock.mockImplementation(async () => ({ + id: 'exec-with-old-session', + agentId: 'agent-id-fixture', + provider: 'claude', + transport: 'api', + pid: null, + tmuxSession: null, + tmuxPaneId: null, + tmuxWindow: null, + tmuxWindowId: null, + claudeSessionId: 'old-session-id', + state: 'idle', + metadata: { source: 'omni', chat_id: 'chat-reject' }, + worktree: null, + repoPath: null, + paneColor: null, + startedAt: new Date().toISOString(), + endedAt: null, + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + })); + + // SDK returns a DIFFERENT session ID (resume was rejected) + queryMock.mockImplementation(() => { + const gen = (async function* () { + yield { type: 'assistant', message: { content: [{ type: 'text', text: 'fresh reply' }] } }; + yield { type: 'result', subtype: 'success', session_id: 'new-session-after-reject' }; + })(); + return Object.assign(gen, { + interrupt: mock(), + setPermissionMode: mock(), + setModel: mock(), + return: mock(async () => ({ value: undefined, done: true })), + throw: mock(async () => ({ value: undefined, done: true })), + }); + }); + }); + + it('writes session.resume_rejected audit event', async () => { + const session = await executor.spawn('test-agent', 'chat-reject', { OMNI_INSTANCE_ID: 'inst-1' }); + safePgCallLog.length = 0; + + await executor.deliver(session, mkMsg('chat-reject')); + await executor.waitForDeliveries(session.id); + + expect(auditEventTypes()).toContain('session.resume_rejected'); + }); + + it('includes old and new session IDs in the rejection audit event', async () => { + const session = await executor.spawn('test-agent', 'chat-reject', { OMNI_INSTANCE_ID: 'inst-1' }); + safePgCallLog.length = 0; + + await executor.deliver(session, mkMsg('chat-reject')); + await executor.waitForDeliveries(session.id); + + const rejectionEntry = safePgCallLog.find((e) => e.op === 'audit:session.resume_rejected'); + expect(rejectionEntry).toBeDefined(); + // recordAuditEvent serializes attrs to JSON in the SQL values + const detailsJson = rejectionEntry!.sqlValues.find((v) => typeof v === 'string' && v.includes('old_session_id')); + expect(detailsJson).toBeDefined(); + const details = JSON.parse(detailsJson as string); + expect(details.old_session_id).toBe('old-session-id'); + expect(details.new_session_id).toBe('new-session-after-reject'); + }); + + it('updates claude_session_id in registry with the new session ID', async () => { + const session = await executor.spawn('test-agent', 'chat-reject', { OMNI_INSTANCE_ID: 'inst-1' }); + updateClaudeSessionIdMock.mockClear(); + + await executor.deliver(session, mkMsg('chat-reject')); + await executor.waitForDeliveries(session.id); + + expect(updateClaudeSessionIdMock).toHaveBeenCalledTimes(1); + const [execId, sessId] = updateClaudeSessionIdMock.mock.calls[0]; + expect(execId).toBe('exec-with-old-session'); + expect(sessId).toBe('new-session-after-reject'); + }); + }); + + // ========================================================================== + // deliver() — successful resume (same session ID returned) + // ========================================================================== + + describe('deliver — resume accepted by SDK', () => { + beforeEach(() => { + findLatestByMetadataMock.mockImplementation(async () => ({ + id: 'exec-resume-ok', + agentId: 'agent-id-fixture', + provider: 'claude', + transport: 'api', + pid: null, + tmuxSession: null, + tmuxPaneId: null, + tmuxWindow: null, + tmuxWindowId: null, + claudeSessionId: 'sdk-session-aaa', + state: 'idle', + metadata: { source: 'omni', chat_id: 'chat-ok' }, + worktree: null, + repoPath: null, + paneColor: null, + startedAt: new Date().toISOString(), + endedAt: null, + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + })); + + // SDK returns the SAME session ID (resume accepted) + queryMock.mockImplementation(() => { + const gen = (async function* () { + yield { type: 'assistant', message: { content: [{ type: 'text', text: 'resumed reply' }] } }; + yield { type: 'result', subtype: 'success', session_id: 'sdk-session-aaa' }; + })(); + return Object.assign(gen, { + interrupt: mock(), + setPermissionMode: mock(), + setModel: mock(), + return: mock(async () => ({ value: undefined, done: true })), + throw: mock(async () => ({ value: undefined, done: true })), + }); + }); + }); + + it('does NOT write session.resume_rejected when session IDs match', async () => { + const session = await executor.spawn('test-agent', 'chat-ok', { OMNI_INSTANCE_ID: 'inst-1' }); + safePgCallLog.length = 0; + + await executor.deliver(session, mkMsg('chat-ok')); + await executor.waitForDeliveries(session.id); + + expect(auditEventTypes()).not.toContain('session.resume_rejected'); + }); + + it('does NOT update claude_session_id when session IDs match', async () => { + const session = await executor.spawn('test-agent', 'chat-ok', { OMNI_INSTANCE_ID: 'inst-1' }); + updateClaudeSessionIdMock.mockClear(); + + await executor.deliver(session, mkMsg('chat-ok')); + await executor.waitForDeliveries(session.id); + + expect(updateClaudeSessionIdMock).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/src/services/executors/__tests__/claude-sdk.test.ts b/src/services/executors/__tests__/claude-sdk.test.ts index c6e47d56e..875218a08 100644 --- a/src/services/executors/__tests__/claude-sdk.test.ts +++ b/src/services/executors/__tests__/claude-sdk.test.ts @@ -1,4 +1,11 @@ -import { beforeEach, describe, expect, it, mock } from 'bun:test'; +import { afterAll, afterEach, beforeEach, describe, expect, it, mock } from 'bun:test'; + +// Clean up process-global mock.module registrations when this file finishes. +// Without this, mocked modules leak into later test files (bun mock.module +// is process-global and persists across file boundaries). +afterAll(() => { + mock.restore(); +}); // Mock agent directory mock.module('../../../lib/agent-directory.js', () => ({ @@ -32,6 +39,54 @@ mock.module('@anthropic-ai/claude-agent-sdk', () => ({ }), })); +// Mock audit-events (Group 5 + 7 — writes audit via safePgCall) +mock.module('../../../lib/audit-events.js', () => ({ + recordAuditEvent: mock(async () => {}), +})); + +// Mock sdk-session-capture (Group 5 — session content capture) +mock.module('../sdk-session-capture.js', () => ({ + startSession: mock(async () => null), + recordTurn: mock(async () => {}), + updateTurnCount: mock(async () => {}), + endSession: mock(async () => {}), +})); + +// Mock agent-registry and executor-registry so spawn()/shutdown() never touch real PG. +// The tests override these with fresh mocks per-test for call-count assertions. +const findOrCreateAgentMock = mock(async (_name: string, _team: string, _role?: string) => ({ + id: 'agent-id-fixture', + startedAt: new Date().toISOString(), + currentExecutorId: null, +})); +mock.module('../../../lib/agent-registry.js', () => ({ + findOrCreateAgent: findOrCreateAgentMock, +})); + +const createAndLinkExecutorMock = mock( + async (_agentId: string, _provider: string, _transport: string, _opts?: any) => ({ + id: 'executor-id-fixture', + agentId: 'agent-id-fixture', + provider: 'claude', + transport: 'api', + state: 'spawning', + metadata: {}, + }), +); +const updateExecutorStateMock = mock(async (_id: string, _state: string) => undefined); +const terminateExecutorMock = mock(async (_id: string) => undefined); +const findLatestByMetadataMock = mock(async (_filter: any) => null); +const relinkExecutorToAgentMock = mock(async () => undefined); +const updateClaudeSessionIdMock = mock(async () => undefined); +mock.module('../../../lib/executor-registry.js', () => ({ + createAndLinkExecutor: createAndLinkExecutorMock, + updateExecutorState: updateExecutorStateMock, + terminateExecutor: terminateExecutorMock, + findLatestByMetadata: findLatestByMetadataMock, + relinkExecutorToAgent: relinkExecutorToAgentMock, + updateClaudeSessionId: updateClaudeSessionIdMock, +})); + const { ClaudeSdkOmniExecutor } = await import('../claude-sdk.js'); const directory = await import('../../../lib/agent-directory.js'); @@ -220,6 +275,23 @@ describe('ClaudeSdkOmniExecutor', () => { }); describe('concurrent delivery', () => { + afterEach(async () => { + // Restore the default SDK query mock so later tests don't hang on unresolved barriers. + const sdk = await import('@anthropic-ai/claude-agent-sdk'); + (sdk.query as ReturnType).mockImplementation(() => { + const gen = (async function* () { + yield { type: 'assistant', message: { content: [{ type: 'text', text: 'response text' }] } }; + })(); + return Object.assign(gen, { + interrupt: mock(), + setPermissionMode: mock(), + setModel: mock(), + return: mock(async () => ({ value: undefined, done: true })), + throw: mock(async () => ({ value: undefined, done: true })), + }); + }); + }); + it('3 concurrent sessions process independently without blocking each other', async () => { // Track the order of query starts and completions per session const events: string[] = []; @@ -315,4 +387,168 @@ describe('ClaudeSdkOmniExecutor', () => { expect(await executor.isAlive(s2)).toBe(true); }); }); + + // ========================================================================== + // World A registry wiring (Group 4 — Decision 1) + // ========================================================================== + + describe('World A registry integration', () => { + /** Fake sql tagged-template that returns an empty array (no rows). */ + const mockSql = ((_strings: TemplateStringsArray, ..._values: any[]) => Promise.resolve([])) as any; + + /** Fake bridge.safePgCall that invokes fn directly — happy path. */ + const happySafePgCall = async ( + _op: string, + fn: (_sql: any) => Promise, + _fallback: T, + _ctx?: { executorId?: string; chatId?: string }, + ): Promise => fn(mockSql); + + /** Fake bridge.safePgCall that never invokes fn — simulates pgAvailable=false. */ + const degradedSafePgCall = async ( + _op: string, + _fn: (_sql: any) => Promise, + fallback: T, + _ctx?: { executorId?: string; chatId?: string }, + ): Promise => fallback; + + beforeEach(() => { + findOrCreateAgentMock.mockClear(); + createAndLinkExecutorMock.mockClear(); + updateExecutorStateMock.mockClear(); + terminateExecutorMock.mockClear(); + }); + + it('spawn(): calls findOrCreateAgent and createAndLinkExecutor with transport="api" and omni metadata', async () => { + executor.setSafePgCall(happySafePgCall); + const session = await executor.spawn('test-agent', 'chat-123', { OMNI_INSTANCE_ID: 'inst-abc' }); + + expect(findOrCreateAgentMock).toHaveBeenCalledWith('test-agent', 'omni', 'omni'); + expect(createAndLinkExecutorMock).toHaveBeenCalledTimes(1); + + const [agentId, provider, transport, opts] = createAndLinkExecutorMock.mock.calls[0]; + expect(agentId).toBe('agent-id-fixture'); + expect(provider).toBe('claude'); + expect(transport).toBe('api'); + expect(opts).toMatchObject({ + claudeSessionId: undefined, + metadata: { source: 'omni', chat_id: 'chat-123', instance_id: 'inst-abc' }, + }); + + // Session still built and returned as before. + expect(session.id).toBe('test-agent:chat-123'); + }); + + it('spawn(): transitions state spawning → running after executor is linked', async () => { + executor.setSafePgCall(happySafePgCall); + await executor.spawn('test-agent', 'chat-1', { OMNI_INSTANCE_ID: 'inst-1' }); + + // updateExecutorState should have been called at least once with "running". + const runningCalls = updateExecutorStateMock.mock.calls.filter(([, state]) => state === 'running'); + expect(runningCalls.length).toBeGreaterThanOrEqual(1); + expect(runningCalls[0][0]).toBe('executor-id-fixture'); + }); + + it('deliver(): transitions state working → idle around the query', async () => { + executor.setSafePgCall(happySafePgCall); + executor.setNatsPublish(mock()); + + const session = await executor.spawn('test-agent', 'chat-deliver', { OMNI_INSTANCE_ID: 'inst-1' }); + updateExecutorStateMock.mockClear(); // reset so we only see the deliver transitions + + await executor.deliver(session, { + content: 'hi', + sender: 'user', + instanceId: 'inst-1', + chatId: 'chat-deliver', + agent: 'test-agent', + }); + await executor.waitForDeliveries(session.id); + + const states = updateExecutorStateMock.mock.calls.map(([, state]) => state); + expect(states).toContain('working'); + expect(states).toContain('idle'); + // Order matters: working must come before idle. + expect(states.indexOf('working')).toBeLessThan(states.indexOf('idle')); + }); + + it('shutdown(): calls terminateExecutor with the linked executor ID', async () => { + executor.setSafePgCall(happySafePgCall); + const session = await executor.spawn('test-agent', 'chat-shutdown', { OMNI_INSTANCE_ID: 'inst-1' }); + + await executor.shutdown(session); + + expect(terminateExecutorMock).toHaveBeenCalledTimes(1); + expect(terminateExecutorMock.mock.calls[0][0]).toBe('executor-id-fixture'); + }); + + it('safePgCall is the mechanism: mocked safePgCall sees all registry op names', async () => { + const safePgCallSpy = mock( + async (_op: string, fn: (_sql: any) => Promise, _fallback: T): Promise => fn(mockSql), + ); + executor.setSafePgCall(safePgCallSpy as never); + executor.setNatsPublish(mock()); + + const session = await executor.spawn('spy-agent', 'chat-spy', { OMNI_INSTANCE_ID: 'inst-1' }); + await executor.deliver(session, { + content: 'hi', + sender: 'u', + instanceId: 'inst-1', + chatId: 'chat-spy', + agent: 'spy-agent', + }); + await executor.waitForDeliveries(session.id); + await executor.shutdown(session); + + const ops = safePgCallSpy.mock.calls.map((args) => args[0]); + expect(ops).toContain('sdk-find-or-create-agent'); + expect(ops).toContain('sdk-create-executor'); + expect(ops).toContain('sdk-update-executor-state'); + expect(ops).toContain('sdk-terminate-executor'); + }); + + it('degraded mode: safePgCall returning fallback immediately keeps spawn/deliver/shutdown working', async () => { + executor.setSafePgCall(degradedSafePgCall); + executor.setNatsPublish(mock()); + + // No PG work should happen because fn is never invoked. + const session = await executor.spawn('test-agent', 'chat-degraded', { OMNI_INSTANCE_ID: 'inst-1' }); + expect(findOrCreateAgentMock).not.toHaveBeenCalled(); + expect(createAndLinkExecutorMock).not.toHaveBeenCalled(); + expect(updateExecutorStateMock).not.toHaveBeenCalled(); + + await executor.deliver(session, { + content: 'hi', + sender: 'user', + instanceId: 'inst-1', + chatId: 'chat-degraded', + agent: 'test-agent', + }); + await executor.waitForDeliveries(session.id); + + await executor.shutdown(session); + expect(terminateExecutorMock).not.toHaveBeenCalled(); + + // Session lifecycle still worked end-to-end. + expect(await executor.isAlive(session)).toBe(false); + }); + + it('no bridge attached: spawn/deliver/shutdown work without any registry calls', async () => { + // No setSafePgCall call — executor.safePgCall stays null. + executor.setNatsPublish(mock()); + const session = await executor.spawn('test-agent', 'chat-solo', { OMNI_INSTANCE_ID: 'inst-1' }); + await executor.deliver(session, { + content: 'hi', + sender: 'user', + instanceId: 'inst-1', + chatId: 'chat-solo', + agent: 'test-agent', + }); + await executor.waitForDeliveries(session.id); + await executor.shutdown(session); + + expect(findOrCreateAgentMock).not.toHaveBeenCalled(); + expect(createAndLinkExecutorMock).not.toHaveBeenCalled(); + }); + }); }); diff --git a/src/services/executors/__tests__/sdk-session-capture.test.ts b/src/services/executors/__tests__/sdk-session-capture.test.ts new file mode 100644 index 000000000..88e1ac402 --- /dev/null +++ b/src/services/executors/__tests__/sdk-session-capture.test.ts @@ -0,0 +1,313 @@ +import { beforeEach, describe, expect, it } from 'bun:test'; +import { recordAuditEvent } from '../../../lib/audit-events.js'; +import type { SafePgCallFn } from '../../executor.js'; +import { endSession, recordTurn, startSession, updateTurnCount } from '../sdk-session-capture.js'; + +// ============================================================================ +// Mock safePgCall that captures SQL template calls +// ============================================================================ + +interface CapturedCall { + op: string; + /** The raw SQL strings array from the tagged template. */ + sqlStrings: string[]; + /** The interpolated values from the tagged template. */ + sqlValues: unknown[]; + ctx?: { executorId?: string; chatId?: string }; +} + +/** + * Build a mock safePgCall that records every call and executes the fn with a + * fake sql tagged-template function. Returns the captured calls array. + */ +function buildMockSafePgCall(): { safePgCall: SafePgCallFn; calls: CapturedCall[] } { + const calls: CapturedCall[] = []; + + const safePgCall: SafePgCallFn = async ( + op: string, + fn: (sql: any) => Promise, + _fallback: T, + ctx?: { executorId?: string; chatId?: string }, + ): Promise => { + // Fake sql tagged-template function — captures the template + values. + const fakeSql = (strings: TemplateStringsArray, ...values: unknown[]) => { + calls.push({ + op, + sqlStrings: [...strings], + sqlValues: values, + ctx, + }); + // Return a result shaped like a postgres.js INSERT...RETURNING or UPDATE. + // startSession expects a truthy result to extract the session ID. + return [{ id: values[0] }]; + }; + return fn(fakeSql); + }; + + return { safePgCall, calls }; +} + +/** + * Build a degraded safePgCall that always returns fallback (PG unavailable). + */ +function buildDegradedSafePgCall(): { safePgCall: SafePgCallFn; calls: CapturedCall[] } { + const calls: CapturedCall[] = []; + const safePgCall: SafePgCallFn = async ( + op: string, + _fn: (sql: any) => Promise, + fallback: T, + ctx?: { executorId?: string; chatId?: string }, + ): Promise => { + calls.push({ op, sqlStrings: [], sqlValues: [], ctx }); + return fallback; + }; + return { safePgCall, calls }; +} + +// ============================================================================ +// Tests +// ============================================================================ + +describe('sdk-session-capture', () => { + let safePgCall: SafePgCallFn; + let calls: CapturedCall[]; + + beforeEach(() => { + ({ safePgCall, calls } = buildMockSafePgCall()); + }); + + // -------------------------------------------------------------------------- + // startSession + // -------------------------------------------------------------------------- + + describe('startSession', () => { + it('creates a sessions row with correct columns', async () => { + const sessionId = await startSession( + safePgCall, + 'exec-1', + 'claude-sess-abc', + 'agent-1', + 'team-a', + 'engineer', + 'wish-1', + ); + + expect(sessionId).toBe('claude-sess-abc'); + expect(calls).toHaveLength(1); + expect(calls[0].op).toBe('sdk-session-start'); + + // The SQL should contain INSERT INTO sessions + const sql = calls[0].sqlStrings.join('?'); + expect(sql).toContain('INSERT INTO sessions'); + expect(sql).toContain('session'); + + // Values should include sessionId, agentId, executorId, team, role, wishSlug + expect(calls[0].sqlValues).toContain('claude-sess-abc'); + expect(calls[0].sqlValues).toContain('agent-1'); + expect(calls[0].sqlValues).toContain('exec-1'); + expect(calls[0].sqlValues).toContain('team-a'); + expect(calls[0].sqlValues).toContain('engineer'); + expect(calls[0].sqlValues).toContain('wish-1'); + }); + + it('generates synthetic session ID when claudeSessionId is undefined', async () => { + const sessionId = await startSession(safePgCall, 'exec-2', undefined, 'agent-2'); + + expect(sessionId).toMatch(/^sdk-exec-2-\d+$/); + expect(calls[0].sqlValues[0]).toBe(sessionId); + }); + + it('returns null when safePgCall is in degraded mode', async () => { + const { safePgCall: degraded } = buildDegradedSafePgCall(); + const sessionId = await startSession(degraded, 'exec-3', 'sess-x', 'agent-3'); + expect(sessionId).toBeNull(); + }); + }); + + // -------------------------------------------------------------------------- + // recordTurn + // -------------------------------------------------------------------------- + + describe('recordTurn', () => { + it('inserts a session_content row with correct shape', async () => { + await recordTurn(safePgCall, 'sess-1', 0, 'assistant', 'Hello world', undefined, '2026-01-01T00:00:00Z'); + + expect(calls).toHaveLength(1); + expect(calls[0].op).toBe('sdk-session-turn'); + + const sql = calls[0].sqlStrings.join('?'); + expect(sql).toContain('INSERT INTO session_content'); + + expect(calls[0].sqlValues).toContain('sess-1'); + expect(calls[0].sqlValues).toContain(0); + expect(calls[0].sqlValues).toContain('assistant'); + expect(calls[0].sqlValues).toContain('Hello world'); + expect(calls[0].sqlValues).toContain('2026-01-01T00:00:00Z'); + }); + + it('writes tool_name when provided', async () => { + await recordTurn(safePgCall, 'sess-1', 1, 'tool_input', '{"cmd":"ls"}', 'Bash'); + + expect(calls[0].sqlValues).toContain('Bash'); + }); + + it('writes null tool_name when omitted', async () => { + await recordTurn(safePgCall, 'sess-1', 2, 'user', 'hello'); + + expect(calls[0].sqlValues).toContain(null); + }); + + it('mirrors session_content columns: session_id, turn_index, role, content, tool_name, timestamp', async () => { + await recordTurn(safePgCall, 'sess-x', 5, 'tool_output', 'file list', 'Read', '2026-06-01T12:00:00Z'); + + const vals = calls[0].sqlValues; + // Positional match to INSERT column order + expect(vals[0]).toBe('sess-x'); // session_id + expect(vals[1]).toBe(5); // turn_index + expect(vals[2]).toBe('tool_output'); // role + expect(vals[3]).toBe('file list'); // content + expect(vals[4]).toBe('Read'); // tool_name + expect(vals[5]).toBe('2026-06-01T12:00:00Z'); // timestamp + }); + }); + + // -------------------------------------------------------------------------- + // updateTurnCount + // -------------------------------------------------------------------------- + + describe('updateTurnCount', () => { + it('updates sessions.total_turns', async () => { + await updateTurnCount(safePgCall, 'sess-1', 7); + + expect(calls).toHaveLength(1); + expect(calls[0].op).toBe('sdk-session-turn-count'); + + const sql = calls[0].sqlStrings.join('?'); + expect(sql).toContain('UPDATE sessions'); + expect(sql).toContain('total_turns'); + expect(calls[0].sqlValues).toContain(7); + expect(calls[0].sqlValues).toContain('sess-1'); + }); + }); + + // -------------------------------------------------------------------------- + // endSession + // -------------------------------------------------------------------------- + + describe('endSession', () => { + it('updates sessions.ended_at and status', async () => { + await endSession(safePgCall, 'sess-1', 'completed'); + + expect(calls).toHaveLength(1); + expect(calls[0].op).toBe('sdk-session-end'); + + const sql = calls[0].sqlStrings.join('?'); + expect(sql).toContain('UPDATE sessions'); + expect(sql).toContain('ended_at'); + expect(sql).toContain('status'); + expect(calls[0].sqlValues).toContain('completed'); + expect(calls[0].sqlValues).toContain('sess-1'); + }); + + it('accepts crashed status', async () => { + await endSession(safePgCall, 'sess-2', 'crashed'); + expect(calls[0].sqlValues).toContain('crashed'); + }); + + it('defaults to completed when status omitted', async () => { + await endSession(safePgCall, 'sess-3'); + expect(calls[0].sqlValues).toContain('completed'); + }); + }); + + // -------------------------------------------------------------------------- + // recordAuditEvent + // -------------------------------------------------------------------------- + + describe('recordAuditEvent', () => { + it('writes to audit_events with correct schema columns', async () => { + await recordAuditEvent(safePgCall, 'deliver.start', { + executor_id: 'exec-1', + agent_id: 'agent-1', + chat_id: 'chat-1', + instance_id: 'inst-1', + }); + + expect(calls).toHaveLength(1); + expect(calls[0].op).toBe('audit:deliver.start'); + + const sql = calls[0].sqlStrings.join('?'); + expect(sql).toContain('INSERT INTO audit_events'); + expect(sql).toContain('entity_type'); + expect(sql).toContain('entity_id'); + expect(sql).toContain('event_type'); + expect(sql).toContain('actor'); + expect(sql).toContain('details'); + + // entity_type defaults to 'executor' + expect(calls[0].sqlValues).toContain('executor'); + // entity_id comes from executor_id + expect(calls[0].sqlValues).toContain('exec-1'); + // event_type is the AuditEventType + expect(calls[0].sqlValues).toContain('deliver.start'); + // actor comes from agent_id + expect(calls[0].sqlValues).toContain('agent-1'); + }); + + it('writes deliver.end with turn_count in details', async () => { + await recordAuditEvent(safePgCall, 'deliver.end', { + executor_id: 'exec-1', + agent_id: 'agent-1', + chat_id: 'chat-1', + instance_id: 'inst-1', + turn_count: 4, + }); + + expect(calls[0].op).toBe('audit:deliver.end'); + + // details JSONB should include turn_count + const detailsJson = calls[0].sqlValues.find((v) => typeof v === 'string' && v.includes('turn_count')); + expect(detailsJson).toBeDefined(); + const details = JSON.parse(detailsJson as string); + expect(details.turn_count).toBe(4); + expect(details.chat_id).toBe('chat-1'); + }); + + it('no-ops when safePgCall is in degraded mode', async () => { + const { safePgCall: degraded, calls: degradedCalls } = buildDegradedSafePgCall(); + await recordAuditEvent(degraded, 'deliver.start', { executor_id: 'x' }); + + // safePgCall was called (it returns fallback), but fn was never invoked + expect(degradedCalls).toHaveLength(1); + expect(degradedCalls[0].sqlStrings).toHaveLength(0); // fn was not called + }); + }); + + // -------------------------------------------------------------------------- + // Full lifecycle integration + // -------------------------------------------------------------------------- + + describe('full lifecycle', () => { + it('start → recordTurn (user) → recordTurn (assistant) → updateTurnCount → end', async () => { + const sessionId = await startSession(safePgCall, 'exec-lifecycle', 'claude-lc', 'agent-lc'); + expect(sessionId).toBe('claude-lc'); + + await recordTurn(safePgCall, sessionId!, 0, 'user', 'What is 2+2?'); + await recordTurn(safePgCall, sessionId!, 1, 'assistant', '4'); + await updateTurnCount(safePgCall, sessionId!, 2); + await endSession(safePgCall, sessionId!); + + // 5 total calls: startSession + 2 recordTurn + updateTurnCount + endSession + expect(calls).toHaveLength(5); + + const ops = calls.map((c) => c.op); + expect(ops).toEqual([ + 'sdk-session-start', + 'sdk-session-turn', + 'sdk-session-turn', + 'sdk-session-turn-count', + 'sdk-session-end', + ]); + }); + }); +}); diff --git a/src/services/executors/claude-code.ts b/src/services/executors/claude-code.ts index 8f6cd6a99..d3dbbca1f 100644 --- a/src/services/executors/claude-code.ts +++ b/src/services/executors/claude-code.ts @@ -11,9 +11,24 @@ import { chmodSync, existsSync, mkdirSync, writeFileSync } from 'node:fs'; import { homedir } from 'node:os'; import { join } from 'node:path'; import * as directory from '../../lib/agent-directory.js'; +import * as agents from '../../lib/agent-registry.js'; +import * as registry from '../../lib/executor-registry.js'; import { shellQuote } from '../../lib/team-lead-command.js'; import { ensureTeamWindow, executeTmux, isPaneAlive, isPaneProcessRunning, killWindow } from '../../lib/tmux.js'; -import type { IExecutor, OmniMessage, OmniSession } from '../executor.js'; +import type { IExecutor, OmniMessage, OmniSession, SafePgCallFn } from '../executor.js'; + +/** + * Per-session state tracked locally by the tmux executor. Mirrors the SDK + * executor's `SdkSessionState` for the World A registry integration. + */ +interface TmuxSessionState { + /** + * World A executor row ID. Set after successful `createAndLinkExecutor`. + * Null when PG was unavailable (degraded mode); downstream state updates + * short-circuit via `bridge.safePgCall`. + */ + executorId: string | null; +} // ============================================================================ // Constants @@ -36,12 +51,35 @@ function getReplyScriptPath(): string { // ============================================================================ export class ClaudeCodeOmniExecutor implements IExecutor { + /** Per-chat local state keyed by `${agentName}:${chatId}` — mirrors SDK executor. */ + private sessions = new Map(); + /** + * Bridge-provided `safePgCall`. Null until the bridge wires it via + * `setSafePgCall()`. When null (no bridge attached, e.g. standalone tests), + * registry calls are skipped entirely — the executor falls through to the + * pre-World-A behavior. + */ + private safePgCall: SafePgCallFn | null = null; + + /** + * Inject the bridge's `safePgCall` helper so World A registry writes are + * guarded by the same pgAvailable / connection-loss logic as the rest of + * the bridge. Mirrors the SDK executor's `setSafePgCall`. + */ + setSafePgCall(fn: SafePgCallFn): void { + this.safePgCall = fn; + } + /** * Spawn a Claude Code process in a tmux window for a specific chat. * * - Resolves agent from genie directory * - Creates tmux window in agent's session * - Starts Claude Code with env vars for NATS reply routing + * - Registers the agent + executor in World A via `findOrCreateAgent` + + * `createAndLinkExecutor` with `transport='tmux'` and omni metadata. + * All PG writes go through `bridge.safePgCall` — degraded mode keeps + * `executorId=null` and the session still works without persistence. */ async spawn(agentName: string, chatId: string, env: Record): Promise { // Resolve agent from directory @@ -95,9 +133,27 @@ export class ClaudeCodeOmniExecutor implements IExecutor { await executeTmux(`send-keys -t '${paneId}' ${shellQuote(cmd)} Enter`); } + // World A registration (Group 4 — Decision 1, tmux path). + // Symmetric with the SDK executor: both transports go through the same + // findOrCreateAgent + createAndLinkExecutor pipeline, only `transport` + // and the tmux-specific fields differ. + const sessionKey = `${agentName}:${chatId}`; + const executorId = await this.registerInWorldA( + agentName, + chatId, + env.OMNI_INSTANCE_ID ?? '', + tmuxSession, + windowName, + paneId, + ); + this.sessions.set(sessionKey, { executorId }); + if (executorId) { + await this.updateState(executorId, 'running', chatId); + } + const now = Date.now(); return { - id: `${agentName}:${chatId}`, + id: sessionKey, agentName, chatId, tmuxSession, @@ -108,13 +164,71 @@ export class ClaudeCodeOmniExecutor implements IExecutor { }; } + /** + * Register agent + tmux executor in World A (Decision 1 from WISH Post-Audit). + * Returns the created executor ID on success, or null when PG is unavailable. + * All writes are wrapped in `bridge.safePgCall` so degraded mode is silent. + */ + private async registerInWorldA( + agentName: string, + chatId: string, + instanceId: string, + tmuxSession: string, + tmuxWindow: string, + tmuxPaneId: string, + ): Promise { + if (!this.safePgCall) return null; + + const agent = await this.safePgCall( + 'tmux-find-or-create-agent', + () => agents.findOrCreateAgent(agentName, 'omni', 'omni'), + null, + { chatId }, + ); + if (!agent) return null; + + const executor = await this.safePgCall( + 'tmux-create-executor', + () => + registry.createAndLinkExecutor(agent.id, 'claude', 'tmux', { + tmuxSession, + tmuxWindow, + tmuxPaneId, + tmuxWindowId: null, // tmux window IDs aren't surfaced by `ensureTeamWindow` — fill in a follow-up. + metadata: { source: 'omni', chat_id: chatId, instance_id: instanceId }, + }), + null, + { chatId }, + ); + return executor?.id ?? null; + } + + /** Update executor state through safePgCall. No-op when PG is degraded. */ + private async updateState(executorId: string, state: 'running' | 'working' | 'idle', chatId: string): Promise { + if (!this.safePgCall) return; + await this.safePgCall( + 'tmux-update-executor-state', + () => registry.updateExecutorState(executorId, state), + undefined, + { executorId, chatId }, + ); + } + /** * Deliver a message to a running Claude Code session via native team inbox. * * Writes to ~/.claude/teams//inboxes/.json so Claude Code - * picks it up natively. + * picks it up natively. Also records a `working → idle` transition in the + * World A registry (wrapped in `safePgCall`, no-op in degraded mode). */ async deliver(session: OmniSession, message: OmniMessage): Promise { + const state = this.sessions.get(session.id); + + // World A state transition: the inbox write itself is effectively instantaneous, + // but the transitions exist so observability consumers can tell whether a given + // tmux executor is currently processing a delivery or waiting. + if (state?.executorId) await this.updateState(state.executorId, 'working', session.chatId); + const inboxDir = join( process.env.CLAUDE_CONFIG_DIR ?? join(homedir(), '.claude'), 'teams', @@ -145,13 +259,29 @@ export class ClaudeCodeOmniExecutor implements IExecutor { writeFileSync(inboxFile, JSON.stringify(messages, null, 2)); session.lastActivityAt = Date.now(); + + if (state?.executorId) await this.updateState(state.executorId, 'idle', session.chatId); } /** - * Shut down a session by killing its tmux window. + * Shut down a session by killing its tmux window and terminating the + * World A executor row (if one was created). */ async shutdown(session: OmniSession): Promise { - await killWindow(session.tmuxSession, session.tmuxWindow); + const state = this.sessions.get(session.id); + try { + await killWindow(session.tmuxSession, session.tmuxWindow); + } finally { + if (state?.executorId && this.safePgCall) { + await this.safePgCall( + 'tmux-terminate-executor', + () => registry.terminateExecutor(state.executorId as string), + undefined, + { executorId: state.executorId, chatId: session.chatId }, + ); + } + this.sessions.delete(session.id); + } } /** diff --git a/src/services/executors/claude-sdk.ts b/src/services/executors/claude-sdk.ts index 54fb7c806..4f7677cb5 100644 --- a/src/services/executors/claude-sdk.ts +++ b/src/services/executors/claude-sdk.ts @@ -1,8 +1,12 @@ import type { Query } from '@anthropic-ai/claude-agent-sdk'; import * as directory from '../../lib/agent-directory.js'; +import * as agents from '../../lib/agent-registry.js'; +import { recordAuditEvent } from '../../lib/audit-events.js'; +import * as registry from '../../lib/executor-registry.js'; import { resolvePermissionConfig } from '../../lib/providers/claude-sdk-permissions.js'; import { ClaudeSdkProvider } from '../../lib/providers/claude-sdk.js'; -import type { IExecutor, OmniMessage, OmniSession } from '../executor.js'; +import type { IExecutor, OmniMessage, OmniSession, SafePgCallFn } from '../executor.js'; +import { endSession, recordTurn, startSession, updateTurnCount } from './sdk-session-capture.js'; // ============================================================================ // Types @@ -14,6 +18,16 @@ interface SdkSessionState { provider: ClaudeSdkProvider; /** Claude session ID for resume (set after first query completes). */ claudeSessionId?: string; + /** + * World A executor row ID. Set after successful `createAndLinkExecutor` in + * spawn(). Null when PG was unavailable (degraded mode) — downstream state + * updates short-circuit via `bridge.safePgCall` in that case. + */ + executorId: string | null; + /** PG sessions row ID for session_content capture (Group 5). Null in degraded mode. */ + dbSessionId: string | null; + /** Running turn counter for session_content rows. */ + turnIndex: number; } // ============================================================================ @@ -71,6 +85,13 @@ async function collectQueryResult(queryMessages: Query): Promise { export class ClaudeSdkOmniExecutor implements IExecutor { private sessions = new Map(); private natsPublish: ((topic: string, payload: string) => void) | null = null; + /** + * Bridge-provided `safePgCall`. Null until the bridge wires it via + * `setSafePgCall()`. When null (no bridge attached, e.g. standalone tests), + * registry calls are skipped entirely — the executor falls through to the + * pre-World-A behavior. + */ + private safePgCall: SafePgCallFn | null = null; /** * Per-session delivery queues. Each session chains its deliveries so messages @@ -87,13 +108,25 @@ export class ClaudeSdkOmniExecutor implements IExecutor { this.natsPublish = fn; } + /** + * Inject the bridge's `safePgCall` helper so World A registry writes are + * guarded by the same pgAvailable / connection-loss logic as the rest of + * the bridge. Mirrors {@link setNatsPublish}. + */ + setSafePgCall(fn: SafePgCallFn): void { + this.safePgCall = fn; + } + /** * Spawn an SDK-backed agent session for a chat. * - * Resolves the agent from the genie directory, creates a ClaudeSdkProvider - * instance, and stores an AbortController for shutdown. + * - Resolves the agent from the genie directory. + * - Registers the agent identity in World A via `findOrCreateAgent`. + * - Creates an `executors` row with `transport='api'` and omni metadata. + * - All PG writes go through `bridge.safePgCall` — degraded mode keeps + * `executorId=null` and the session still works without persistence. */ - async spawn(agentName: string, chatId: string, _env: Record): Promise { + async spawn(agentName: string, chatId: string, env: Record): Promise { const resolved = await directory.resolve(agentName); if (!resolved) { throw new Error(`Agent "${agentName}" not found in genie directory`); @@ -101,14 +134,30 @@ export class ClaudeSdkOmniExecutor implements IExecutor { const provider = new ClaudeSdkProvider(); const abortController = new AbortController(); - const sessionId = `${agentName}:${chatId}`; + + // World A registration (Group 4 + Group 7 lazy resume). + // Returns null when PG is in degraded mode; downstream state transitions + // short-circuit via safePgCall's pgAvailable fast-path. + // When an existing executor is found (bridge restart), returns its + // claudeSessionId so the next query can resume the Claude session. + const registration = await this.registerInWorldA(agentName, chatId, env.OMNI_INSTANCE_ID ?? ''); + this.sessions.set(sessionId, { abortController, running: true, provider, + executorId: registration?.executorId ?? null, + claudeSessionId: registration?.claudeSessionId, + dbSessionId: null, + turnIndex: 0, }); + // Transition spawning → running once the session is in the local map. + if (registration?.executorId) { + await this.updateState(registration.executorId, 'running', chatId); + } + const now = Date.now(); return { id: sessionId, @@ -122,6 +171,90 @@ export class ClaudeSdkOmniExecutor implements IExecutor { }; } + /** + * Register agent + executor in World A (Decision 1 from WISH Post-Audit). + * Returns the executor ID + optional Claude session ID on success, or null + * when PG is unavailable. All writes go through `bridge.safePgCall`. + * + * Group 7 — Lazy resume: before creating a fresh executor, look for an + * existing live executor for this agent + chat. If found, reuse it and + * recover the Claude session ID so the next query can resume. + */ + private async registerInWorldA( + agentName: string, + chatId: string, + instanceId: string, + ): Promise<{ executorId: string; claudeSessionId?: string } | null> { + if (!this.safePgCall) return null; + + const agent = await this.safePgCall( + 'sdk-find-or-create-agent', + () => agents.findOrCreateAgent(agentName, 'omni', 'omni'), + null, + { chatId }, + ); + if (!agent) return null; + + // Lazy resume: look for an existing live executor for this chat. + const existing = await this.safePgCall( + 'sdk-find-existing-executor', + () => registry.findLatestByMetadata({ agentId: agent.id, source: 'omni', chatId }), + null, + { chatId }, + ); + + if (existing) { + // Reuse existing executor — relink to agent and recover session. + await this.safePgCall( + 'sdk-relink-executor', + () => registry.relinkExecutorToAgent(existing.id, agent.id), + undefined, + { executorId: existing.id, chatId }, + ); + await recordAuditEvent(this.safePgCall, 'session.resumed', { + executor_id: existing.id, + agent_id: agentName, + chat_id: chatId, + claude_session_id: existing.claudeSessionId, + }); + return { + executorId: existing.id, + claudeSessionId: existing.claudeSessionId ?? undefined, + }; + } + + // Fresh: create new executor and link to agent. + const executor = await this.safePgCall( + 'sdk-create-executor', + () => + registry.createAndLinkExecutor(agent.id, 'claude', 'api', { + claudeSessionId: undefined, + metadata: { source: 'omni', chat_id: chatId, instance_id: instanceId }, + }), + null, + { chatId }, + ); + if (executor) { + await recordAuditEvent(this.safePgCall, 'session.created_fresh', { + executor_id: executor.id, + agent_id: agentName, + chat_id: chatId, + }); + } + return executor ? { executorId: executor.id } : null; + } + + /** Update executor state through safePgCall. No-op when PG is degraded. */ + private async updateState(executorId: string, state: 'running' | 'working' | 'idle', chatId: string): Promise { + if (!this.safePgCall) return; + await this.safePgCall( + 'sdk-update-executor-state', + () => registry.updateExecutorState(executorId, state), + undefined, + { executorId, chatId }, + ); + } + /** * Deliver a message by enqueuing an async SDK query. * @@ -160,6 +293,19 @@ export class ClaudeSdkOmniExecutor implements IExecutor { const permissionConfig = resolvePermissionConfig(entry.permissions); const systemPrompt = await loadSystemPrompt(entry); + // State: working (query in flight) — before the blocking operation starts. + if (state.executorId) await this.updateState(state.executorId, 'working', session.chatId); + + // Audit: deliver.start + if (this.safePgCall) { + await recordAuditEvent(this.safePgCall, 'deliver.start', { + executor_id: state.executorId ?? session.id, + agent_id: session.agentName, + chat_id: message.chatId, + instance_id: message.instanceId, + }); + } + // Resume existing session or start fresh const extraOptions: Record = { abortController: state.abortController }; if (state.claudeSessionId) { @@ -182,11 +328,18 @@ export class ClaudeSdkOmniExecutor implements IExecutor { entry.sdk, ); + // Record user turn (lazily creates PG session row on first delivery). + await this.captureUserTurn(state, session.agentName, session.id, message.content); + const result = await collectQueryResult(queryMessages); if (result.sessionId) { - state.claudeSessionId = result.sessionId; + await this.reconcileSessionId(state, session, result.sessionId); } const replyText = result.text; + + // Record assistant turn and update turn count. + await this.captureAssistantTurn(state, session.id, result.sessionId, session.agentName, replyText); + if (replyText && this.natsPublish) { const topic = `omni.reply.${message.instanceId}.${message.chatId}`; const payload = JSON.stringify({ @@ -200,6 +353,91 @@ export class ClaudeSdkOmniExecutor implements IExecutor { } session.lastActivityAt = Date.now(); + + // Audit: deliver.end + if (this.safePgCall) { + await recordAuditEvent(this.safePgCall, 'deliver.end', { + executor_id: state.executorId ?? session.id, + agent_id: session.agentName, + chat_id: message.chatId, + instance_id: message.instanceId, + turn_count: state.turnIndex, + }); + } + + // State: idle (query finished, waiting for next message). + if (state.executorId) await this.updateState(state.executorId, 'idle', session.chatId); + } + + /** + * Group 7 — Reconcile the Claude session ID after a query completes. + * Detects resume rejection (SDK returned a different session), persists + * new/changed session IDs to the registry, and updates local state. + */ + private async reconcileSessionId( + state: SdkSessionState, + session: OmniSession, + returnedSessionId: string, + ): Promise { + const isResumeRejected = state.claudeSessionId && returnedSessionId !== state.claudeSessionId; + if (isResumeRejected && this.safePgCall) { + await recordAuditEvent(this.safePgCall, 'session.resume_rejected', { + executor_id: state.executorId ?? session.id, + agent_id: session.agentName, + chat_id: session.chatId, + old_session_id: state.claudeSessionId, + new_session_id: returnedSessionId, + }); + } + + // Persist session ID to registry if it changed (first query or resume rejection). + const execId = state.executorId; + if (execId && this.safePgCall && returnedSessionId !== state.claudeSessionId) { + await this.safePgCall( + 'sdk-update-claude-session', + () => registry.updateClaudeSessionId(execId, returnedSessionId), + undefined, + { executorId: execId, chatId: session.chatId }, + ); + } + + state.claudeSessionId = returnedSessionId; + } + + /** Lazily create PG session row and record the user turn. */ + private async captureUserTurn( + state: SdkSessionState, + agentName: string, + _sessionKey: string, + content: string, + ): Promise { + if (!this.safePgCall) return; + if (!state.dbSessionId && state.executorId) { + state.dbSessionId = await startSession(this.safePgCall, state.executorId, state.claudeSessionId, agentName); + } + if (state.dbSessionId) { + await recordTurn(this.safePgCall, state.dbSessionId, state.turnIndex++, 'user', content); + } + } + + /** Record the assistant turn, re-key session ID if needed, bump turn count. */ + private async captureAssistantTurn( + state: SdkSessionState, + sessionKey: string, + claudeSessionId: string | undefined, + agentName: string, + replyText: string, + ): Promise { + if (!this.safePgCall) return; + // Re-key the PG session row with the real Claude session ID once available. + if (claudeSessionId && state.dbSessionId?.startsWith('sdk-')) { + const newId = await startSession(this.safePgCall, state.executorId ?? sessionKey, claudeSessionId, agentName); + if (newId) state.dbSessionId = newId; + } + if (state.dbSessionId && replyText) { + await recordTurn(this.safePgCall, state.dbSessionId, state.turnIndex++, 'assistant', replyText); + await updateTurnCount(this.safePgCall, state.dbSessionId, state.turnIndex); + } } /** @@ -215,16 +453,33 @@ export class ClaudeSdkOmniExecutor implements IExecutor { } /** - * Shut down a session by aborting any active query. + * Shut down a session by aborting any active query and terminating its + * World A executor row (if one was created). */ async shutdown(session: OmniSession): Promise { const state = this.sessions.get(session.id); - if (state) { - state.abortController.abort(); - state.running = false; - this.sessions.delete(session.id); - this.deliveryQueues.delete(session.id); + if (!state) return; + + state.abortController.abort(); + state.running = false; + + // End the PG session row (Group 5) before terminating the executor. + if (state.dbSessionId && this.safePgCall) { + await endSession(this.safePgCall, state.dbSessionId, 'completed'); + } + + // Terminate World A executor row — safePgCall short-circuits in degraded mode. + if (state.executorId && this.safePgCall) { + await this.safePgCall( + 'sdk-terminate-executor', + () => registry.terminateExecutor(state.executorId as string), + undefined, + { executorId: state.executorId, chatId: session.chatId }, + ); } + + this.sessions.delete(session.id); + this.deliveryQueues.delete(session.id); } /** diff --git a/src/services/executors/sdk-session-capture.ts b/src/services/executors/sdk-session-capture.ts new file mode 100644 index 000000000..d1c9cc887 --- /dev/null +++ b/src/services/executors/sdk-session-capture.ts @@ -0,0 +1,116 @@ +/** + * SDK Session Capture — inline session + content recording for API-backed executors. + * + * Mirrors the rows that session-capture.ts (filewatch) produces for tmux sessions, + * but writes them directly during the SDK query lifecycle instead of parsing JSONL. + * + * All writes go through SafePgCallFn so degraded mode (no PG) silently skips. + * + * Table targets: + * sessions — one row per SDK session (Group 5, startSession / endSession) + * session_content — one row per turn (recordTurn) + */ + +import type { SafePgCallFn } from '../../lib/safe-pg-call.js'; + +// ============================================================================ +// startSession — create a sessions row +// ============================================================================ + +/** + * Create a sessions row for an SDK-backed executor session. + * Returns the session ID on success, null when PG is degraded. + * + * The session ID is the executor's `claudeSessionId` when available, + * otherwise a synthetic `sdk--` key. + */ +export async function startSession( + safePgCall: SafePgCallFn, + executorId: string, + claudeSessionId: string | undefined, + agentId: string | null, + team?: string, + role?: string, + wishSlug?: string, +): Promise { + const sessionId = claudeSessionId ?? `sdk-${executorId}-${Date.now()}`; + + const created = await safePgCall( + 'sdk-session-start', + (sql) => + sql`INSERT INTO sessions (id, agent_id, executor_id, team, role, wish_slug, status, jsonl_path, project_path) + VALUES (${sessionId}, ${agentId}, ${executorId}, ${team ?? null}, ${role ?? null}, ${wishSlug ?? null}, 'active', '', '') + ON CONFLICT (id) DO NOTHING + RETURNING id`, + null, + { executorId, chatId: '' }, + ); + + // created is null when safePgCall fell back (degraded), or the array from the INSERT. + if (!created) return null; + return sessionId; +} + +// ============================================================================ +// recordTurn — write a session_content row +// ============================================================================ + +/** + * Record a single turn in session_content. + * Mirrors the shape that session-capture.ts writes for tmux JSONL content: + * session_id, turn_index, role, content, tool_name, timestamp + */ +export async function recordTurn( + safePgCall: SafePgCallFn, + sessionId: string, + turnIndex: number, + role: 'assistant' | 'tool_input' | 'tool_output' | 'user', + content: string, + toolName?: string, + timestamp?: string, +): Promise { + const ts = timestamp ?? new Date().toISOString(); + + await safePgCall( + 'sdk-session-turn', + (sql) => + sql`INSERT INTO session_content (session_id, turn_index, role, content, tool_name, timestamp) + VALUES (${sessionId}, ${turnIndex}, ${role}, ${content}, ${toolName ?? null}, ${ts}) + ON CONFLICT (session_id, turn_index) DO NOTHING`, + undefined, + { executorId: '', chatId: '' }, + ); +} + +// ============================================================================ +// updateTurnCount — bump sessions.total_turns +// ============================================================================ + +export async function updateTurnCount(safePgCall: SafePgCallFn, sessionId: string, totalTurns: number): Promise { + await safePgCall( + 'sdk-session-turn-count', + (sql) => sql`UPDATE sessions SET total_turns = ${totalTurns}, updated_at = now() WHERE id = ${sessionId}`, + undefined, + { executorId: '', chatId: '' }, + ); +} + +// ============================================================================ +// endSession — mark session as ended +// ============================================================================ + +/** + * Mark a session as ended by setting ended_at and status. + */ +export async function endSession( + safePgCall: SafePgCallFn, + sessionId: string, + status: 'completed' | 'crashed' | 'orphaned' = 'completed', +): Promise { + await safePgCall( + 'sdk-session-end', + (sql) => sql`UPDATE sessions SET ended_at = now(), status = ${status}, updated_at = now() WHERE id = ${sessionId}`, + undefined, + { executorId: '', chatId: '' }, + ); +} diff --git a/src/services/omni-bridge.ts b/src/services/omni-bridge.ts index 10e28737f..68373222b 100644 --- a/src/services/omni-bridge.ts +++ b/src/services/omni-bridge.ts @@ -11,6 +11,8 @@ */ import { type NatsConnection, StringCodec, type Subscription, connect } from 'nats'; +import type { Sql } from '../lib/db.js'; +import { resolveExecutorType } from '../lib/executor-config.js'; import type { IExecutor, OmniMessage, OmniSession } from './executor.js'; import { ClaudeCodeOmniExecutor } from './executors/claude-code.js'; import { ClaudeSdkOmniExecutor } from './executors/claude-sdk.js'; @@ -24,16 +26,34 @@ const DEFAULT_IDLE_TIMEOUT_MS = 15 * 60 * 1000; // 15 minutes const DEFAULT_MAX_CONCURRENT = 20; const MAX_BUFFER_PER_CHAT = 50; const IDLE_CHECK_INTERVAL_MS = 30_000; // Check idle sessions every 30s +const PG_STARTUP_PROBE_TIMEOUT_MS = 5_000; +const PG_RUNTIME_QUERY_TIMEOUT_MS = 2_000; // ============================================================================ // Types // ============================================================================ +/** Factory that returns a ready-to-use postgres.js tagged-template client. */ +export type PgProvider = () => Promise; + +/** Minimal shape for NATS connect — lets tests inject a fake NATS without touching the network. */ +export type NatsConnectFn = typeof connect; + +/** Optional context attached to safePgCall log lines. */ +export interface SafePgCallContext { + executorId?: string; + chatId?: string; +} + interface BridgeConfig { natsUrl?: string; idleTimeoutMs?: number; maxConcurrent?: number; executorType?: 'tmux' | 'sdk'; + /** Test/DI hook: override the PG provider. Defaults to `getConnection()` from lib/db.js. */ + pgProvider?: PgProvider; + /** Test/DI hook: override the NATS connect function. Defaults to the real `connect` from the nats package. */ + natsConnectFn?: NatsConnectFn; } interface SessionEntry { @@ -47,10 +67,16 @@ interface SessionEntry { export interface BridgeStatus { connected: boolean; natsUrl: string; + /** True when the startup PG probe succeeded and runtime writes are allowed. */ + pgAvailable: boolean; activeSessions: number; maxConcurrent: number; idleTimeoutMs: number; queueDepth: number; + /** Which executor backend is in use: tmux or sdk. */ + executorType: 'tmux' | 'sdk'; + /** Executor IDs from PG (omni source, not ended). Empty in degraded mode. */ + executorIds: string[]; sessions: Array<{ id: string; agentName: string; @@ -73,6 +99,53 @@ export function getBridge(): OmniBridge | null { return bridgeInstance; } +// ============================================================================ +// PG helpers (Group 3 — scaffolding for degraded-mode PG access) +// ============================================================================ + +/** + * Race a promise against a timeout. The timeout timer is `unref`'d so it never + * holds the event loop open on its own. Used by the bridge's PG probe and + * safePgCall helper to honor the wish's 2s read / 5s startup budgets. + */ +function withTimeout(p: Promise, ms: number, label: string): Promise { + return new Promise((resolve, reject) => { + const timer = setTimeout(() => reject(new Error(`${label} timed out after ${ms}ms`)), ms); + timer.unref?.(); + p.then( + (v) => { + clearTimeout(timer); + resolve(v); + }, + (err) => { + clearTimeout(timer); + reject(err); + }, + ); + }); +} + +/** + * Classify a thrown value as a PG connection-level error. Used by safePgCall + * to decide whether to flip `pgAvailable=false` after a failure. + * + * We match both by postgres.js error codes (.code) and by common message + * fragments, because postgres.js surfaces some errors as generic Errors with + * no code (e.g., "connection terminated unexpectedly"). + */ +function isPgConnectionError(err: unknown): boolean { + if (!err || typeof err !== 'object') return false; + const e = err as { code?: string; message?: string }; + const code = e.code ?? ''; + if (['ECONNREFUSED', 'ECONNRESET', 'ETIMEDOUT', 'ENOTFOUND', 'EPIPE', 'EHOSTUNREACH'].includes(code)) { + return true; + } + const msg = e.message ?? String(err); + return /ECONNREFUSED|ECONNRESET|ETIMEDOUT|ENOTFOUND|EPIPE|connection terminated|connection closed|server closed the connection|the database system is shutting down/i.test( + msg, + ); +} + // ============================================================================ // Bridge // ============================================================================ @@ -81,14 +154,37 @@ export class OmniBridge { private nc: NatsConnection | null = null; private sub: Subscription | null = null; private executor: IExecutor; + + /** + * Process-local runtime handles — NOT the source of truth for session identity. + * + * This Map holds per-process runtime handles (executor instances, idle timers, + * message buffers). Session identity and state live in PG via executor-registry. + * This Map exists because: + * - executor instances are in-memory objects that can't be persisted + * - idle timers (setTimeout handles) are per-process + * - message buffers during spawn are ephemeral + * + * The Map does NOT define which sessions exist — PG does. The status() method + * queries PG for active session count when available, falling back to this + * Map's size only in degraded (no-PG) mode. + */ private sessions = new Map(); private messageQueue: OmniMessage[] = []; private idleCheckTimer: ReturnType | null = null; private sc = StringCodec(); + /** Postgres client, set after a successful startup probe. Null in degraded mode. */ + private sql: Sql | null = null; + /** Flipped by `probePg()` at startup; flipped to false on any connection-level runtime error. */ + private pgAvailable = false; + private readonly pgProvider: PgProvider; + private readonly natsConnectFn: NatsConnectFn; + readonly natsUrl: string; readonly idleTimeoutMs: number; readonly maxConcurrent: number; + readonly executorType: 'tmux' | 'sdk'; constructor(config: BridgeConfig = {}) { this.natsUrl = config.natsUrl ?? process.env.GENIE_NATS_URL ?? DEFAULT_NATS_URL; @@ -99,8 +195,16 @@ export class OmniBridge { config.maxConcurrent ?? (process.env.GENIE_MAX_CONCURRENT ? Number(process.env.GENIE_MAX_CONCURRENT) : DEFAULT_MAX_CONCURRENT); - const executorType = config.executorType ?? (process.env.GENIE_EXECUTOR_TYPE as 'tmux' | 'sdk') ?? 'tmux'; - if (executorType === 'sdk') { + this.pgProvider = + config.pgProvider ?? + (async () => { + const { getConnection } = await import('../lib/db.js'); + return (await getConnection()) as Sql; + }); + this.natsConnectFn = config.natsConnectFn ?? connect; + + this.executorType = resolveExecutorType(config.executorType); + if (this.executorType === 'sdk') { this.executor = new ClaudeSdkOmniExecutor(); } else { this.executor = new ClaudeCodeOmniExecutor(); @@ -118,7 +222,7 @@ export class OmniBridge { console.log(`[omni-bridge] Connecting to NATS at ${this.natsUrl}...`); - this.nc = await connect({ + this.nc = await this.natsConnectFn({ servers: this.natsUrl, name: 'genie-omni-bridge', reconnect: true, @@ -128,6 +232,17 @@ export class OmniBridge { console.log('[omni-bridge] Connected to NATS'); + // PG probe: graceful degradation on failure — never block startup. + // Group 3 scaffolding; Group 4 consumers (SDK + tmux executors) receive a + // bound `safePgCall` reference below. + await this.probePg(); + + // Inject the bridge's safePgCall into the executor so its World A registry + // writes are guarded by the same pgAvailable / connection-loss logic as + // the rest of the bridge. Both executors expose `setSafePgCall` (Group 4, + // Decision 2 in WISH Post-Audit). + this.executor.setSafePgCall(this.safePgCall.bind(this)); + // Wire NATS publish into SDK executor for reply routing if (this.executor instanceof ClaudeSdkOmniExecutor) { const nc = this.nc; @@ -195,6 +310,12 @@ export class OmniBridge { // Connection may already be closed } this.nc = null; + + // Reset PG state — the shared `getConnection()` singleton (lib/db.js) owns + // the actual client lifecycle, so we only clear our local references. + this.sql = null; + this.pgAvailable = false; + bridgeInstance = null; console.log('[omni-bridge] Stopped'); @@ -202,16 +323,44 @@ export class OmniBridge { /** * Get current bridge status for `genie omni status`. + * + * When PG is available, queries the executors table for the authoritative + * active session count and executor IDs. Falls back to the local sessions + * Map size in degraded (no-PG) mode. */ - status(): BridgeStatus { + async status(): Promise { const now = Date.now(); + + // PG-backed active session count + executor IDs when available. + let activeFromPg: number | null = null; + let executorIds: string[] = []; + + if (this.pgAvailable && this.sql) { + const rows = await this.safePgCall( + 'status_active_count', + async (sql) => + sql<{ id: string }[]>` + SELECT id FROM executors + WHERE ended_at IS NULL AND metadata->>'source' = 'omni' + `, + null, + ); + if (rows) { + activeFromPg = rows.length; + executorIds = rows.map((r) => r.id); + } + } + return { connected: this.nc !== null, natsUrl: this.natsUrl, - activeSessions: this.sessions.size, + pgAvailable: this.pgAvailable, + activeSessions: activeFromPg ?? this.sessions.size, maxConcurrent: this.maxConcurrent, idleTimeoutMs: this.idleTimeoutMs, queueDepth: this.messageQueue.length, + executorType: this.executorType, + executorIds, sessions: Array.from(this.sessions.entries()).map(([key, entry]) => ({ id: key, agentName: entry.session.agentName, @@ -225,6 +374,92 @@ export class OmniBridge { }; } + // ========================================================================== + // PG lifecycle — Group 3 scaffolding + // ========================================================================== + + /** + * Probe PG at startup. Classifies failures into two buckets: + * + * - **Connection-level** (ECONNREFUSED, ETIMEDOUT, connection terminated, …): + * degrade gracefully — set `pgAvailable=false`, log warn, let the bridge + * keep running. Developers without PG must still be able to run omni. + * - **Anything else** (schema mismatch, missing relation, permission denied, + * migration not yet applied, …): fail-fast by rethrowing a clear error + * that tells the operator which migration command to run. Silent data + * corruption is worse than noisy startup failure — this matches the + * wish's PG Error Handling Strategy table. + * + * On success: caches the client and sets `pgAvailable=true`. + */ + private async probePg(): Promise { + try { + const sql = await withTimeout(this.pgProvider(), PG_STARTUP_PROBE_TIMEOUT_MS, 'PG provider startup'); + await withTimeout(Promise.resolve(sql`SELECT 1`), PG_STARTUP_PROBE_TIMEOUT_MS, 'PG SELECT 1 probe'); + this.sql = sql; + this.pgAvailable = true; + console.log('[omni-bridge] PG reachable — session recovery enabled'); + } catch (err) { + this.sql = null; + this.pgAvailable = false; + const msg = err instanceof Error ? err.message : String(err); + + if (isPgConnectionError(err)) { + // Expected degraded-mode path: PG is simply unreachable. + console.warn(`[omni-bridge] PG unavailable — session recovery disabled (${msg})`); + return; + } + + // Non-connection failure at startup = schema mismatch, missing migration, + // permission denied, or similar. Fail-fast with an actionable message. + const hint = 'Run `bun run migrate` (or the equivalent migration command) and retry.'; + throw new Error(`[omni-bridge] PG schema mismatch or setup error: ${msg}. ${hint}`); + } + } + + /** + * Single entry point for every runtime PG call made by the bridge and its + * downstream executors. Guarantees the delivery loop never crashes on a + * transient PG fault. + * + * Semantics (matches the wish's PG Error Handling Strategy table): + * - If `pgAvailable` is already false, returns `fallback` without calling `fn`. + * - Otherwise calls `fn` once (no retry). Applies a 2s timeout for reads. + * - On error: logs at warn with `op`, `executor_id`, and `chat_id` context. + * - On connection-level errors (ECONNREFUSED, connection terminated, etc.), + * flips `pgAvailable` to false so later calls fast-path to fallback. + * - Always returns `fallback` on error — the caller never sees the throw. + * + * Downstream groups (4, 5, 6, 7) wire their PG writes through this helper. + * Public by design (Decision 2 in WISH Post-Audit Decisions) — executors + * hold an `OmniBridge` reference and call `bridge.safePgCall(...)` directly. + */ + public async safePgCall( + op: string, + fn: (sql: Sql) => Promise, + fallback: T, + ctx?: SafePgCallContext, + ): Promise { + if (!this.pgAvailable || !this.sql) { + return fallback; + } + const sql = this.sql; + try { + return await withTimeout(fn(sql), PG_RUNTIME_QUERY_TIMEOUT_MS, `safePgCall(${op})`); + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + const execPart = ctx?.executorId ? ` executor_id=${ctx.executorId}` : ''; + const chatPart = ctx?.chatId ? ` chat_id=${ctx.chatId}` : ''; + console.warn(`[omni-bridge] safePgCall(${op}) failed${execPart}${chatPart}: ${msg}`); + if (isPgConnectionError(err)) { + this.pgAvailable = false; + this.sql = null; + console.warn('[omni-bridge] PG connection lost — switching to degraded mode'); + } + return fallback; + } + } + // ========================================================================== // Internal // ========================================================================== diff --git a/src/term-commands/agent/list.ts b/src/term-commands/agent/list.ts index d50a54fdd..7ec53eb01 100644 --- a/src/term-commands/agent/list.ts +++ b/src/term-commands/agent/list.ts @@ -12,7 +12,8 @@ export function registerAgentList(parent: Command): void { .alias('ls') .description('List registered agents with runtime status') .option('--json', 'Output as JSON') - .action(async (options: { json?: boolean }) => { + .option('--source ', 'Filter by executor metadata source (e.g. omni)') + .action(async (options: { json?: boolean; source?: string }) => { try { await handleLsCommand(options); } catch (error) { diff --git a/src/term-commands/agents.ts b/src/term-commands/agents.ts index 76e26793f..513092536 100644 --- a/src/term-commands/agents.ts +++ b/src/term-commands/agents.ts @@ -1915,13 +1915,24 @@ async function buildWorkerStatusMap(workers: registry.Agent[]): Promise> { + const executorRegistry = await import('../lib/executor-registry.js'); + const agentRegistry = await import('../lib/agent-registry.js'); + const executors = await executorRegistry.listExecutors(undefined, source); + const agentIds = new Set(executors.map((e) => e.agentId)); + const agents = await agentRegistry.listAgents({}); + return new Set(agents.filter((a) => agentIds.has(a.id)).map((a) => a.customName ?? a.role ?? a.id)); +} + /** * genie ls — Smart view of registered agents with runtime status. */ -export async function handleLsCommand(options: { json?: boolean }): Promise { +export async function handleLsCommand(options: { json?: boolean; source?: string }): Promise { const dirEntries = await directory.ls(); const workers = await registry.list(); const statusMap = await buildWorkerStatusMap(workers); + const sourceAgentNames = options.source ? await resolveAgentNamesBySource(options.source) : undefined; type LsEntry = { name: string; @@ -1933,7 +1944,7 @@ export async function handleLsCommand(options: { json?: boolean }): Promise sourceAgentNames.has(e.name)); + } + if (options.json) { console.log(JSON.stringify(entries, null, 2)); return; diff --git a/src/term-commands/exec/index.ts b/src/term-commands/exec/index.ts index e47c33792..db37c5142 100644 --- a/src/term-commands/exec/index.ts +++ b/src/term-commands/exec/index.ts @@ -49,7 +49,12 @@ function printExecutorDetail(executor: any): void { console.log(''); } -async function listExecutors(options: { agent?: string; state?: string; json?: boolean }): Promise { +async function listExecutors(options: { + agent?: string; + state?: string; + source?: string; + json?: boolean; +}): Promise { const executorRegistry = await import('../../lib/executor-registry.js'); const agentRegistry = await import('../../lib/agent-registry.js'); @@ -66,7 +71,7 @@ async function listExecutors(options: { agent?: string; state?: string; json?: b } } - let executors = await executorRegistry.listExecutors(agentId); + let executors = await executorRegistry.listExecutors(agentId, options.source); if (options.state) { executors = executors.filter((e) => e.state === options.state); } @@ -94,8 +99,9 @@ export function registerExecCommands(program: Command): void { .description('List all executors') .option('--agent ', 'Filter by agent name/ID') .option('--state ', 'Filter by state (running, idle, terminated, etc.)') + .option('--source ', 'Filter by metadata source (e.g. omni)') .option('--json', 'Output as JSON') - .action(async (options: { agent?: string; state?: string; json?: boolean }) => { + .action(async (options: { agent?: string; state?: string; source?: string; json?: boolean }) => { try { await listExecutors(options); } catch (error) { diff --git a/src/term-commands/omni.ts b/src/term-commands/omni.ts index df6e37758..8e6940a2f 100644 --- a/src/term-commands/omni.ts +++ b/src/term-commands/omni.ts @@ -6,6 +6,40 @@ */ import type { Command } from 'commander'; +import type { BridgeStatus } from '../services/omni-bridge.js'; + +function printStatus(s: BridgeStatus): void { + const pgTag = s.pgAvailable ? '✓ connected' : '✗ degraded'; + const connTag = s.connected ? '✓ connected' : '✗ disconnected'; + const activeSource = s.pgAvailable ? ' (PG-backed)' : ' (in-memory)'; + + console.log('\nOmni Bridge Status'); + console.log('─'.repeat(50)); + console.log(` Bridge: ${connTag}`); + console.log(` NATS URL: ${s.natsUrl}`); + console.log(` PG: ${pgTag}`); + console.log(` Executor type: ${s.executorType}`); + console.log(` Active: ${s.activeSessions} / ${s.maxConcurrent}${activeSource}`); + console.log(` Queue depth: ${s.queueDepth}`); + console.log(` Idle timeout: ${Math.round(s.idleTimeoutMs / 1000)}s`); + + if (s.executorIds.length > 0) { + console.log('\n Executors (PG):'); + for (const id of s.executorIds) { + console.log(` ${id}`); + } + } + + if (s.sessions.length > 0) { + console.log('\n Sessions:'); + for (const sess of s.sessions) { + const idleSec = Math.round(sess.idleMs / 1000); + const status = sess.spawning ? 'spawning' : `idle ${idleSec}s`; + console.log(` ${sess.agentName}:${sess.chatId} — pane=${sess.paneId} (${status})`); + } + } + console.log(''); +} export function registerOmniCommands(program: Command): void { const omni = program.command('omni').description('Manage the Omni ↔ Genie NATS bridge'); @@ -16,7 +50,7 @@ export function registerOmniCommands(program: Command): void { .option('--nats-url ', 'NATS server URL', process.env.GENIE_NATS_URL ?? 'localhost:4222') .option('--max-concurrent ', 'Max concurrent agent sessions', process.env.GENIE_MAX_CONCURRENT ?? '20') .option('--idle-timeout ', 'Idle timeout in ms', process.env.GENIE_IDLE_TIMEOUT_MS ?? '900000') - .option('--executor ', 'Executor type: tmux (default) or sdk', process.env.GENIE_EXECUTOR_TYPE ?? 'tmux') + .option('--executor ', 'Executor type: tmux (default) or sdk') .action(async (options) => { const { OmniBridge } = await import('../services/omni-bridge.js'); @@ -24,7 +58,7 @@ export function registerOmniCommands(program: Command): void { natsUrl: options.natsUrl, maxConcurrent: Number(options.maxConcurrent), idleTimeoutMs: Number(options.idleTimeout), - executorType: options.executor as 'tmux' | 'sdk', + executorType: options.executor, }); await bridge.start(); @@ -71,29 +105,13 @@ export function registerOmniCommands(program: Command): void { return; } - const s = bridge.status(); + const s = await bridge.status(); if (options.json) { console.log(JSON.stringify(s, null, 2)); return; } - console.log('\nOmni Bridge Status'); - console.log('─'.repeat(50)); - console.log(` Connected: ${s.connected ? '✓ yes' : '✗ no'}`); - console.log(` NATS URL: ${s.natsUrl}`); - console.log(` Active: ${s.activeSessions} / ${s.maxConcurrent}`); - console.log(` Queue depth: ${s.queueDepth}`); - console.log(` Idle timeout: ${Math.round(s.idleTimeoutMs / 1000)}s`); - - if (s.sessions.length > 0) { - console.log('\n Sessions:'); - for (const sess of s.sessions) { - const idleSec = Math.round(sess.idleMs / 1000); - const status = sess.spawning ? 'spawning' : `idle ${idleSec}s`; - console.log(` ${sess.agentName}:${sess.chatId} — pane=${sess.paneId} (${status})`); - } - } - console.log(''); + printStatus(s); }); } diff --git a/src/term-commands/sessions.ts b/src/term-commands/sessions.ts index c81ea9781..a2c9d418e 100644 --- a/src/term-commands/sessions.ts +++ b/src/term-commands/sessions.ts @@ -50,6 +50,7 @@ interface ListOptions { active?: boolean; orphaned?: boolean; agent?: string; + source?: string; limit?: string; json?: boolean; } @@ -71,6 +72,9 @@ async function sessionsListCommand(options: ListOptions): Promise { const sql = await getConnection(); const limit = Number(options.limit) || 50; + // Build optional source filter fragment + const sourceFilter = options.source ? sql`AND e.metadata->>'source' = ${options.source}` : sql``; + let rows: SessionRow[]; if (options.active) { rows = await sql` @@ -78,7 +82,7 @@ async function sessionsListCommand(options: ListOptions): Promise { FROM sessions s LEFT JOIN executors e ON s.executor_id = e.id LEFT JOIN agents a ON e.agent_id = a.id - WHERE s.status = 'active' + WHERE s.status = 'active' ${sourceFilter} ORDER BY s.started_at DESC LIMIT ${limit}`; } else if (options.orphaned) { rows = await sql` @@ -86,7 +90,7 @@ async function sessionsListCommand(options: ListOptions): Promise { FROM sessions s LEFT JOIN executors e ON s.executor_id = e.id LEFT JOIN agents a ON e.agent_id = a.id - WHERE s.status = 'orphaned' + WHERE s.status = 'orphaned' ${sourceFilter} ORDER BY s.started_at DESC LIMIT ${limit}`; } else if (options.agent) { // Search by agent name/id via executor join, fall back to legacy agent_id @@ -95,10 +99,18 @@ async function sessionsListCommand(options: ListOptions): Promise { FROM sessions s LEFT JOIN executors e ON s.executor_id = e.id LEFT JOIN agents a ON e.agent_id = a.id - WHERE a.custom_name = ${options.agent} + WHERE (a.custom_name = ${options.agent} OR a.role = ${options.agent} OR s.agent_id = ${options.agent} - OR s.agent_id LIKE ${`%${options.agent}%`} + OR s.agent_id LIKE ${`%${options.agent}%`}) ${sourceFilter} + ORDER BY s.started_at DESC LIMIT ${limit}`; + } else if (options.source) { + rows = await sql` + SELECT s.*, a.custom_name as agent_name + FROM sessions s + LEFT JOIN executors e ON s.executor_id = e.id + LEFT JOIN agents a ON e.agent_id = a.id + WHERE e.metadata->>'source' = ${options.source} ORDER BY s.started_at DESC LIMIT ${limit}`; } else { rows = await sql` @@ -292,6 +304,7 @@ export function registerSessionsCommands(program: Command): void { .option('--active', 'Show only active sessions') .option('--orphaned', 'Show only orphaned sessions') .option('--agent ', 'Filter by agent') + .option('--source ', 'Filter by executor metadata source (e.g. omni)') .option('--limit ', 'Max number of sessions to return (default: 50)') .option('--json', 'Output as JSON') .action(async (options: ListOptions) => { diff --git a/src/types/genie-config.ts b/src/types/genie-config.ts index 069afdc06..861adfa9e 100644 --- a/src/types/genie-config.ts +++ b/src/types/genie-config.ts @@ -70,6 +70,8 @@ const OmniConfigSchema = z.object({ apiUrl: z.string(), apiKey: z.string().optional(), defaultInstanceId: z.string().optional(), + /** Executor type for the omni bridge: 'tmux' (default) or 'sdk'. */ + executor: z.enum(['tmux', 'sdk']).optional(), }); // Council preset configuration