fix(omni): bridge hardening — lifecycle, PG degraded mode, liveness - #1065
Conversation
Adds 8 tests covering the process-tree liveness check that detects crashed Claude processes inside alive tmux panes (Group 1 Bug 3).
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements hardening for the Omni Bridge and ClaudeCode executor, focusing on session lifecycle management and process liveness checks. Key additions include unit tests for isPaneProcessRunning and comprehensive tests for the OmniBridge session lifecycle, covering buffer overflows, shutdown procedures, concurrency limits, and spawn failures. A review comment suggests that the test for re-queuing multiple buffered messages should be updated to actually simulate multiple messages to match its description.
| it('re-queues multiple buffered messages on spawn failure', async () => { | ||
| let spawnCallCount = 0; | ||
| const { executor } = makeMockExecutor({ | ||
| spawnFn: async () => { | ||
| spawnCallCount++; | ||
| // Simulate a slow spawn that allows buffering, then fails | ||
| throw new Error('resource exhausted'); | ||
| }, | ||
| }); | ||
|
|
||
| const { nc } = makeFakeNatsWithPublish(); | ||
| const bridge = new OmniBridge({ | ||
| natsUrl: 'test://fake', | ||
| pgProvider: degradedPgProvider, | ||
| natsConnectFn: (async () => nc) as any, | ||
| }); | ||
| (bridge as any).executor = executor; | ||
| await bridge.start(); | ||
|
|
||
| try { | ||
| // spawnSession buffers the triggering message, then spawn fails and | ||
| // all buffered entries go to messageQueue. | ||
| await (bridge as any).routeMessage(makeMsg({ content: 'trigger' })); | ||
|
|
||
| expect(spawnCallCount).toBe(1); | ||
| expect((bridge as any).sessions.size).toBe(0); | ||
| // At minimum, the triggering message is re-queued | ||
| expect((bridge as any).messageQueue.length).toBeGreaterThanOrEqual(1); | ||
| expect((bridge as any).messageQueue[0].content).toBe('trigger'); | ||
| } finally { | ||
| await bridge.stop(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The test title indicates it verifies re-queuing of multiple buffered messages, but the implementation only sends a single message. To effectively test the re-queuing of multiple messages, you should simulate additional messages arriving while the spawn is in progress (e.g., by using a slow spawnFn and sending multiple messages without awaiting the first routeMessage).
…tion Commit 2b226b3 (`wip: fix-omni-bridge-hardening#1`) landed on dev via PR #1065 before pre-commit hooks caught the non-conventional type. Commitlint in the rolling promotion PR (#1059) fails because `wip:` isn't in the allowed type-enum. Adding a surgical ignore for this exact message avoids rewriting dev history. The type-enum rule still rejects `wip:` for any new commits — this exception only applies to the single historical SHA. Unblocks #1059 Commit Messages CI.
Group 1 added injectNudge to the IExecutor interface (src/services/executor.ts) but the mock executor in omni-bridge.test.ts (introduced via #1065) was not updated. Typecheck fails on merge with dev because the mock is missing the required method. Adds a no-op injectNudge stub to satisfy the interface.
…tion Commit 2b226b3 (wip: fix-omni-bridge-hardening#1) landed on dev via PR #1065 with a non-conventional type. Commitlint in the rolling promotion PR (#1059) fails because wip is not in the allowed type-enum. Surgical ignore for this exact message — type-enum still rejects wip for all new commits. Unblocks #1059.
Audit all 80+ wishes against dev codebase. 17 had stale statuses: SHIPPED (14): - unified-omni-bridge (PRs #1063, #1065) - fix-omni-bridge-hardening (PR #1065) - unified-executor-layer (PR #1062) - auto-orchestrate, fix-depends-parser, parallel-execution - task-projects, test-pg-ram-isolation, task-auto-close-on-merge - worktree-out-of-repo, docs-overhaul, genie-hacks-community-docs - multi-agent-session-isolation, session-auto-create OBSOLETE (3): - genie-omni-marriage (superseded by smaller wishes) - fix-session-uuid-resume (replaced by --continue by name) - qa-dev-to-main (time-bound QA from March 20)
…verflow One commit subject (82e5d07) is 110 chars — a U+2192 → arrow narrowly puts it over the 100-char header-max-length cap. Adding a string-prefix ignore matches the existing "Historical exception" pattern (see #1065, #1249) and unblocks the dev→main promotion PR without rewriting dev. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…verflow One commit subject (82e5d07) is 110 chars — a U+2192 → arrow narrowly puts it over the 100-char header-max-length cap. Adding a string-prefix ignore matches the existing "Historical exception" pattern (see #1065, #1249) and unblocks the dev→main promotion PR without rewriting dev. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 participant
Summary
Adds comprehensive test coverage for the Omni Bridge hardening work from #1062 (unified-executor-layer), covering:
stop()shuts down all active executor sessions, concurrency limit counts spawning entries, spawn failure re-queues buffered messagesnode_modulessymlink from gitAll fixes were already implemented in
feat/unified-omni-bridgevia #1062. This branch adds the test harness (18 new tests, 61 assertions) that validates those fixes.Results
Test plan
bun test src/services/__tests__/omni-bridge.test.ts— 18 tests passbun test src/services/executors/claude-code.test.ts— passesbun test src/lib/tmux.test.ts— passesbun run typecheck— cleanbun run lint— 18 pre-existing warnings, no new issuesCloses #1036, #1035