Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 87 additions & 0 deletions .genie/wishes/fix-omni-bridge-hardening/WISH.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
# Wish: Fix Omni Bridge & ClaudeCode Executor Hardening

| Field | Value |
|-------|-------|
| **Status** | ready |
| **Slug** | `fix-omni-bridge-hardening` |
| **Date** | 2026-04-04 |
| **Priority** | P1 |
| **Repo** | `automagik-dev/genie` |
| **Issues** | #1036, #1035 |
| **depends-on** | `fix-sdk-executor-correctness` (P0) |

## Summary

The ClaudeCode Omni executor has chat ID collisions, broken JSON escaping, and weak liveness checks. The OmniBridge silently drops messages when buffer is full, leaks executor processes on shutdown, and allows concurrency bursts during spawn. These are production reliability issues for the Omni→agent pipeline.

## Deliverables

### Group 1: Fix ClaudeCode executor — chat ID collision + JSON escaping + liveness (#1036)

**File:** `src/services/executors/claude-code.ts`

**Bug 1 — Chat ID collision (line 23-25):**
`sanitizeWindowName` strips non-alphanumeric chars and truncates to 40. Different JIDs like `5511999999999@s.whatsapp.net` and `5511888888888@s.whatsapp.net` can collide after stripping.

**Fix:** Use a hash-based approach:
```typescript
function sanitizeWindowName(chatId: string): string {
const hash = createHash('md5').update(chatId).digest('hex').slice(0, 12);
const prefix = chatId.replace(/[^a-zA-Z0-9]/g, '').slice(0, 24);
return `${prefix}-${hash}` || 'chat';
}
```

**Bug 2 — JSON escaping (line 210-214):**
Only `sed 's/"/\\"/g'` is used. Backslashes, tabs, carriage returns produce malformed JSON.

**Fix:** Use `JSON.stringify()` in the Node layer before passing to bash, or use a proper escaping function that handles all JSON-special characters (`\`, `\n`, `\t`, `\r`, control chars).

**Bug 3 — Liveness check:**
`isPaneAlive` at `src/lib/tmux.ts:548-555` only checks `pane_dead` flag, not whether Claude process runs inside.

**Fix:** Add process check: `tmux list-panes -t <pane> -F '#{pane_pid}'` then verify the process tree contains `claude` or the expected process.

**Acceptance:** Different chat IDs always map to different tmux windows. Messages with special characters produce valid JSON. Liveness detects crashed Claude processes in alive panes.

### Group 2: Fix OmniBridge session lifecycle (#1035)

**File:** `src/services/omni-bridge.ts`

**Bug 1 — Silent message drop (line 265-268):**
When `entry.buffer.length >= MAX_BUFFER_PER_CHAT` (50), messages silently dropped.

**Fix:** Log a warning, emit a runtime event, and reply to the sender with an error message (e.g., "Queue full, please retry").

**Bug 2 — Resource leak on stop() (line 158-193):**
`stop()` clears timers and drains NATS but never calls `executor.shutdown()` on live sessions.

**Fix:** Iterate `this.sessions` and call `executor.shutdown()` for each active session before clearing:
```typescript
for (const [key, entry] of this.sessions) {
try { await entry.executor?.shutdown(); } catch (e) { /* log */ }
}
this.sessions.clear();
```

**Bug 3 — Concurrency burst (line 294-295):**
Spawning entries excluded from concurrency count, allowing oversubscription.

**Fix:** Count spawning entries in the active count:
```typescript
const activeCount = Array.from(this.sessions.values()).filter(e => !e.idle).length;
```

**Bug 4 — Buffered messages lost on spawn failure (line 337-340):**
When spawn fails, `this.sessions.delete(key)` removes placeholder including all buffered messages.

**Fix:** Before deleting, attempt to re-queue buffered messages or log them for recovery.

**Acceptance:** No silent message drops — all failures logged and visible. `stop()` cleans up all executor processes. Concurrency limit respected during spawn bursts.

## Validation

```bash
cd /home/genie/workspace/repos/genie
bun run build && bun test --filter "omni|bridge|executor|claude-code"
```
67 changes: 65 additions & 2 deletions src/lib/tmux-alive.test.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,23 @@
import { beforeEach, describe, expect, mock, test } from 'bun:test';
import { afterAll, beforeEach, describe, expect, mock, test } from 'bun:test';

const mockExecuteTmux = mock(async (_cmd: string) => '');
const mockExecSync = mock((_cmd: string, _opts?: object) => '');

mock.module('./tmux-wrapper.js', () => ({
executeTmux: mockExecuteTmux,
genieTmuxPrefix: () => ['-L', 'genie'],
genieTmuxCmd: (sub: string) => `tmux -L genie ${sub}`,
}));

const { isPaneAlive, TmuxUnreachableError } = await import('./tmux.js');
mock.module('node:child_process', () => ({
execSync: mockExecSync,
}));

const { isPaneAlive, isPaneProcessRunning, TmuxUnreachableError } = await import('./tmux.js');

afterAll(() => {
mock.restore();
});

describe('isPaneAlive', () => {
beforeEach(() => {
Expand Down Expand Up @@ -57,3 +66,57 @@ describe('isPaneAlive', () => {
await expect(isPaneAlive('%2')).rejects.toBeInstanceOf(TmuxUnreachableError);
});
});

describe('isPaneProcessRunning', () => {
beforeEach(() => {
mockExecuteTmux.mockReset();
mockExecSync.mockReset();
});

test('returns false for invalid pane ids', async () => {
expect(await isPaneProcessRunning('', 'claude')).toBe(false);
expect(await isPaneProcessRunning('inline', 'claude')).toBe(false);
expect(await isPaneProcessRunning('pane-1', 'claude')).toBe(false);
});

test('returns false when pane pid is empty', async () => {
mockExecuteTmux.mockResolvedValueOnce('');
expect(await isPaneProcessRunning('%2', 'claude')).toBe(false);
});

test('returns false when pane pid is non-numeric', async () => {
mockExecuteTmux.mockResolvedValueOnce('notanumber');
expect(await isPaneProcessRunning('%2', 'claude')).toBe(false);
});

test('returns true when target process found in descendants', async () => {
mockExecuteTmux.mockResolvedValueOnce('12345');
mockExecSync.mockReturnValueOnce('12346 claude --session-id abc\n');
expect(await isPaneProcessRunning('%2', 'claude')).toBe(true);
});

test('returns false when target process not in descendants', async () => {
mockExecuteTmux.mockResolvedValueOnce('12345');
mockExecSync.mockReturnValueOnce('12346 bash\n12347 vim\n');
expect(await isPaneProcessRunning('%2', 'claude')).toBe(false);
});

test('matches process name case-insensitively', async () => {
mockExecuteTmux.mockResolvedValueOnce('12345');
mockExecSync.mockReturnValueOnce('12346 Claude --dangerously-skip-permissions\n');
expect(await isPaneProcessRunning('%2', 'claude')).toBe(true);
});

test('returns false when tmux command fails', async () => {
mockExecuteTmux.mockRejectedValueOnce(new Error("can't find pane %99"));
expect(await isPaneProcessRunning('%99', 'claude')).toBe(false);
});

test('returns false when execSync throws', async () => {
mockExecuteTmux.mockResolvedValueOnce('12345');
mockExecSync.mockImplementationOnce(() => {
throw new Error('command failed');
});
expect(await isPaneProcessRunning('%2', 'claude')).toBe(false);
});
});
Loading
Loading