feat(omni): Model A — persona + session + streaming + ⏳→✅ ack for genie omni serve - #2514
Closed
namastex888 wants to merge 4 commits into
Closed
namastex888 wants to merge 4 commits into
namastex888 wants to merge 4 commits into
Conversation
…odel A) Enhance the `genie omni serve` inbound one-shot into "Model A": - defaultSpawnClaude now runs `claude -p --output-format stream-json --verbose --session-id <uuid> [--append-system-prompt-file <persona>]`, parsing the final reply out of the NDJSON stream (terminal result event → assistant text deltas → raw stdout fallback). Arg construction + parsing are extracted into pure, exported buildClaudeArgs / extractStreamJsonReply. - SpawnClaudeOpts gains optional personaFile + sessionId; the runner resolves personaFile = route.persona ?? <repo>/AGENTS.md and derives a STABLE deterministicSessionId(instance, chat) so a conversation resumes across messages. - Route the inbound WhatsApp stanza id (messageId) through handleMessage → startRoutedRun → runOneShot and set a ⏳→✅/❌ status reaction on it, route-scoped to (route.instance, route.chat). Generalizes the existing approval-scoped emitStatusReaction into a shared emitReaction seam; the route ack records no glyph and skips the reconciliation guard, stays fire-and-forget (drained by whenIdle), and never throws. - Extend the OmniRoute config (schema + runtime type) with optional persona. The approval flow is untouched (same ⏳/✅/❌ behaviour, glyph recording and reconciliation). Adds 12 tests covering argv, stream-json parsing, session-id stability, route-scoped ⏳→✅/❌ acks, the no-messageId no-op, and persona resolution — all with injected spawnClaude + setReaction, zero fork/HTTP.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds optional persona fields to Omni route configuration, introduces resumable Claude session handling with stream-json reply parsing, and ties route reactions to inbound message ids while expanding coverage for the new flow. ChangesPersona and route-scoped ack lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant handleMessage
participant startRoutedRun
participant runOneShot
participant resolvePersonaFile
participant defaultSpawnClaude
participant claudeCLI
handleMessage->>startRoutedRun: run(route, msg.messageId)
startRoutedRun->>runOneShot: runOneShot(messageId)
runOneShot->>resolvePersonaFile: resolve persona path
runOneShot->>defaultSpawnClaude: spawn(message, sessionId, personaFile)
defaultSpawnClaude->>claudeCLI: claude -p --output-format stream-json
claudeCLI-->>defaultSpawnClaude: stdout + exitCode
defaultSpawnClaude-->>runOneShot: extracted reply / error state
runOneShot-->>handleMessage: publish reply and status reactions
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces support for route-scoped personas, stable session threading across messages, and run status reactions (⏳→✅/❌) in the Omni runner. Key changes include deriving deterministic session IDs, parsing Claude's stream-json output, and refactoring reaction emission. Feedback on these changes suggests setting stderr: 'ignore' on the spawned process to prevent potential hangs, improving the stream-json parser to avoid returning raw NDJSON when JSON parsing succeeds but yields no text, and wrapping the setReaction call in a promise chain to safely catch synchronous errors.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+195
to
201
| const proc = Bun.spawn(['claude', ...args], { | ||
| cwd, | ||
| stdout: 'pipe', | ||
| stderr: 'pipe', | ||
| // Bun forwards the AbortSignal: aborting SIGKILLs the child, freeing the route. | ||
| signal, | ||
| }); |
Contributor
There was a problem hiding this comment.
Leaving stderr as 'pipe' without consuming it can cause the child process to hang if the stderr buffer fills up (typically 64KB). Since --verbose is passed to Claude, it is highly likely to produce significant output on stderr. Consider setting stderr: 'ignore' or consuming the stream to prevent potential hangs.
Suggested change
| const proc = Bun.spawn(['claude', ...args], { | |
| cwd, | |
| stdout: 'pipe', | |
| stderr: 'pipe', | |
| // Bun forwards the AbortSignal: aborting SIGKILLs the child, freeing the route. | |
| signal, | |
| }); | |
| const proc = Bun.spawn(['claude', ...args], { | |
| cwd, | |
| stdout: 'pipe', | |
| stderr: 'ignore', | |
| // Bun forwards the AbortSignal: aborting SIGKILLs the child, freeing the route. | |
| signal, | |
| }); |
Comment on lines
+147
to
+170
| export function extractStreamJsonReply(raw: string): string { | ||
| let assistantText = ''; | ||
| for (const line of raw.split('\n')) { | ||
| const trimmed = line.trim(); | ||
| if (!trimmed) continue; | ||
| let evt: unknown; | ||
| try { | ||
| evt = JSON.parse(trimmed); | ||
| } catch { | ||
| continue; // skip a non-JSON line (log noise / partial frame) | ||
| } | ||
| if (!evt || typeof evt !== 'object') continue; | ||
| const o = evt as { type?: string; subtype?: string; result?: unknown; message?: { content?: unknown } }; | ||
| if (o.type === 'result' && o.subtype === 'success' && typeof o.result === 'string' && o.result.length > 0) { | ||
| return o.result; | ||
| } | ||
| if (o.type === 'assistant' && o.message && Array.isArray(o.message.content)) { | ||
| for (const block of o.message.content as Array<{ type?: string; text?: string }>) { | ||
| if (block && block.type === 'text' && typeof block.text === 'string') assistantText += block.text; | ||
| } | ||
| } | ||
| } | ||
| return assistantText || raw; | ||
| } |
Contributor
There was a problem hiding this comment.
If the stream-json output is successfully parsed but contains no assistant text or result, falling back to raw will return the entire raw NDJSON stream to the user, which is a poor user experience. We can track if any valid JSON was parsed and return an empty string instead of the raw NDJSON stream in those cases.
export function extractStreamJsonReply(raw: string): string {
let assistantText = '';
let parsedAnyJson = false;
for (const line of raw.split('\n')) {
const trimmed = line.trim();
if (!trimmed) continue;
let evt: unknown;
try {
evt = JSON.parse(trimmed);
parsedAnyJson = true;
} catch {
continue; // skip a non-JSON line (log noise / partial frame)
}
if (!evt || typeof evt !== 'object') continue;
const o = evt as { type?: string; subtype?: string; result?: unknown; message?: { content?: unknown } };
if (o.type === 'result' && o.subtype === 'success' && typeof o.result === 'string' && o.result.length > 0) {
return o.result;
}
if (o.type === 'assistant' && o.message && Array.isArray(o.message.content)) {
for (const block of o.message.content as Array<{ type?: string; text?: string }>) {
if (block && block.type === 'text' && typeof block.text === 'string') assistantText += block.text;
}
}
}
return assistantText || (parsedAnyJson ? '' : raw);
}
Comment on lines
+674
to
686
| const react = setReaction({ instance, chat, messageId: targetId, emoji }) | ||
| .then((res) => { | ||
| if (res && res.success === false) { | ||
| log(`[omni] status ${emoji} on ${targetId} failed${res.error ? ` (${res.error})` : ''}`); | ||
| return; | ||
| } | ||
| recordStatusGlyph(db, targetId, emoji); // persist only on confirmed success | ||
| if (recordGlyph) recordStatusGlyph(db, targetId, emoji); // persist only on confirmed success | ||
| }) | ||
| .catch((err) => log(`[omni] status ${emoji} on ${targetId} failed: ${errText(err)}`)) | ||
| .finally(() => { | ||
| ackInFlight.delete(targetId); | ||
| if (guard) ackInFlight.delete(targetId); | ||
| inFlightReactions.delete(react); | ||
| }); |
Contributor
There was a problem hiding this comment.
If setReaction throws a synchronous error (e.g., due to configuration or implementation issues), it will bypass the promise .catch block and propagate synchronously, potentially crashing the runner process. Wrapping the call in Promise.resolve().then() ensures any synchronous throws are safely caught and handled by the promise chain.
Suggested change
| const react = setReaction({ instance, chat, messageId: targetId, emoji }) | |
| .then((res) => { | |
| if (res && res.success === false) { | |
| log(`[omni] status ${emoji} on ${targetId} failed${res.error ? ` (${res.error})` : ''}`); | |
| return; | |
| } | |
| recordStatusGlyph(db, targetId, emoji); // persist only on confirmed success | |
| if (recordGlyph) recordStatusGlyph(db, targetId, emoji); // persist only on confirmed success | |
| }) | |
| .catch((err) => log(`[omni] status ${emoji} on ${targetId} failed: ${errText(err)}`)) | |
| .finally(() => { | |
| ackInFlight.delete(targetId); | |
| if (guard) ackInFlight.delete(targetId); | |
| inFlightReactions.delete(react); | |
| }); | |
| const react = Promise.resolve() | |
| .then(() => setReaction({ instance, chat, messageId: targetId, emoji })) | |
| .then((res) => { | |
| if (res && res.success === false) { | |
| log(`[omni] status ${emoji} on ${targetId} failed${res.error ? ` (${res.error})` : ''}`); | |
| return; | |
| } | |
| if (recordGlyph) recordStatusGlyph(db, targetId, emoji); // persist only on confirmed success | |
| }) | |
| .catch((err) => log(`[omni] status ${emoji} on ${targetId} failed: ${errText(err)}`)) | |
| .finally(() => { | |
| if (guard) ackInFlight.delete(targetId); | |
| inFlightReactions.delete(react); | |
| }); |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b12ee0c2b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const content = ok ? truncateReply(result.stdout, maxReplyChars) : errorNotice(`exit code ${result.exitCode}`); | ||
| publish(replySubject, buildRoutedReplyPayload(route.instance, route.chat, content, genId(), now())); | ||
| // ✅ once a genuine reply is published; ❌ when the run exited non-zero. | ||
| emitRouteReaction(route, messageId, ok ? STATUS_APPROVED : STATUS_DENIED); |
There was a problem hiding this comment.
For very fast runs or a slow Omni reaction request, the earlier ⏳ reaction can still be in flight when this ✅/❌ reaction is fired; because route reactions use guard: false and are never awaited or reconciled, the HTTP requests can complete out of order and a late ⏳ can overwrite the terminal status on WhatsApp. The approval path avoids this with ackInFlight/reconciliation, so route acks need similar per-messageId serialization or the terminal reaction should wait for the pending one to settle.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/omni-runner.test.ts`:
- Around line 859-890: The persona fallback tests in omni-runner.test are using
inline try/finally for tmpdir cleanup, but the test suite convention is to
manage fixtures with afterEach. Refactor the two tests around threadingRunner to
create the temporary directory in setup and remove it in a shared afterEach
hook, while keeping the existing mkdtempSync, rmSync, freshDb, and SeenSpawn
assertions intact. This keeps cleanup consistent and avoids leaking temp dirs if
more assertions are added later.
In `@src/lib/omni-runner.ts`:
- Around line 726-730: The resolvePersonaFile helper returns route.persona
without checking whether the file exists, so add the same existsSync validation
used for the AGENTS.md fallback before returning an explicit persona path.
Update resolvePersonaFile in omni-runner so it only returns route.persona when
the path exists, otherwise log the missing persona and fall back to checking
<repo>/AGENTS.md or return undefined if neither exists.
- Around line 195-204: The Claude spawn in omni-runner is leaving proc.stderr as
a pipe without consuming it, which can block the child if stderr fills up.
Update the spawn/response flow in omni-runner’s child-process handling to either
drain proc.stderr concurrently with proc.stdout or change stderr handling to
ignore/inherit, while keeping the existing exitCode and stdout parsing behavior
intact.
- Around line 124-135: The buildClaudeArgs helper in omni-runner is passing
opts.message as the final positional argument without a separator, so prompts
that start with a dash can be misread as claude flags. Update buildClaudeArgs to
insert a standalone "--" before opts.message, keeping the existing option order
and optional personaFile handling intact so the prompt is always treated as an
operand.
- Around line 124-135: The Claude argument builder currently uses a stable
session id for repeated `claude -p` calls, which does not preserve turn-to-turn
state. Update `buildClaudeArgs` and the inbound run path that uses it to use the
supported `--resume` flow instead of `--session-id`, or switch to a
forked-session approach if branching is intended. Keep the existing
`opts.message` and persona-file handling intact, and ensure the session
identifier is applied in the resume-compatible way in the relevant runner logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: aca24ee7-7753-4197-ade6-422d5378294d
📒 Files selected for processing (4)
src/lib/omni-config.tssrc/lib/omni-runner.test.tssrc/lib/omni-runner.tssrc/types/genie-config.ts
Comment on lines
+859
to
+890
| test('falls back to <repo>/AGENTS.md when route.persona is unset', async () => { | ||
| const dir = mkdtempSync(join(tmpdir(), 'genie-omni-persona-')); | ||
| try { | ||
| writeFileSync(join(dir, 'AGENTS.md'), '# persona'); | ||
| const db = freshDb(); | ||
| const seen: SeenSpawn[] = []; | ||
| const runner = threadingRunner(db, seen, [{ instance: INSTANCE, chat: ROUTE_CHAT, repo: dir }]); | ||
|
|
||
| runner.handleMessage(...mappedInboundWithId('m', 'id')); | ||
| await runner.whenIdle(); | ||
|
|
||
| expect(seen[0].personaFile).toBe(join(dir, 'AGENTS.md')); | ||
| } finally { | ||
| rmSync(dir, { recursive: true, force: true }); | ||
| } | ||
| }); | ||
|
|
||
| test('resolves no persona when neither route.persona nor <repo>/AGENTS.md exists', async () => { | ||
| const dir = mkdtempSync(join(tmpdir(), 'genie-omni-nopersona-')); | ||
| try { | ||
| const db = freshDb(); | ||
| const seen: SeenSpawn[] = []; | ||
| const runner = threadingRunner(db, seen, [{ instance: INSTANCE, chat: ROUTE_CHAT, repo: dir }]); | ||
|
|
||
| runner.handleMessage(...mappedInboundWithId('m', 'id')); | ||
| await runner.whenIdle(); | ||
|
|
||
| expect(seen[0].personaFile).toBeUndefined(); | ||
| } finally { | ||
| rmSync(dir, { recursive: true, force: true }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use afterEach for tmpdir cleanup instead of inline try/finally.
Both persona tests build a tmpdir and clean it up with a local try/finally. As per path instructions, **/*.test.{ts,tsx,js,jsx} files should "Use tmpdir with cleanup in afterEach for test fixtures." Moving the mkdtempSync/rmSync pair into beforeEach/afterEach keeps cleanup consistent with the rest of the suite's convention and guards against leaked dirs if additional assertions are added later outside the try block.
♻️ Suggested refactor sketch
- test('falls back to <repo>/AGENTS.md when route.persona is unset', async () => {
- const dir = mkdtempSync(join(tmpdir(), 'genie-omni-persona-'));
- try {
- writeFileSync(join(dir, 'AGENTS.md'), '# persona');
- ...
- } finally {
- rmSync(dir, { recursive: true, force: true });
- }
- });
+ let dir: string;
+ beforeEach(() => {
+ dir = mkdtempSync(join(tmpdir(), 'genie-omni-persona-'));
+ });
+ afterEach(() => {
+ rmSync(dir, { recursive: true, force: true });
+ });
+
+ test('falls back to <repo>/AGENTS.md when route.persona is unset', async () => {
+ writeFileSync(join(dir, 'AGENTS.md'), '# persona');
+ ...
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test('falls back to <repo>/AGENTS.md when route.persona is unset', async () => { | |
| const dir = mkdtempSync(join(tmpdir(), 'genie-omni-persona-')); | |
| try { | |
| writeFileSync(join(dir, 'AGENTS.md'), '# persona'); | |
| const db = freshDb(); | |
| const seen: SeenSpawn[] = []; | |
| const runner = threadingRunner(db, seen, [{ instance: INSTANCE, chat: ROUTE_CHAT, repo: dir }]); | |
| runner.handleMessage(...mappedInboundWithId('m', 'id')); | |
| await runner.whenIdle(); | |
| expect(seen[0].personaFile).toBe(join(dir, 'AGENTS.md')); | |
| } finally { | |
| rmSync(dir, { recursive: true, force: true }); | |
| } | |
| }); | |
| test('resolves no persona when neither route.persona nor <repo>/AGENTS.md exists', async () => { | |
| const dir = mkdtempSync(join(tmpdir(), 'genie-omni-nopersona-')); | |
| try { | |
| const db = freshDb(); | |
| const seen: SeenSpawn[] = []; | |
| const runner = threadingRunner(db, seen, [{ instance: INSTANCE, chat: ROUTE_CHAT, repo: dir }]); | |
| runner.handleMessage(...mappedInboundWithId('m', 'id')); | |
| await runner.whenIdle(); | |
| expect(seen[0].personaFile).toBeUndefined(); | |
| } finally { | |
| rmSync(dir, { recursive: true, force: true }); | |
| } | |
| }); | |
| let dir: string; | |
| beforeEach(() => { | |
| dir = mkdtempSync(join(tmpdir(), 'genie-omni-persona-')); | |
| }); | |
| afterEach(() => { | |
| rmSync(dir, { recursive: true, force: true }); | |
| }); | |
| test('falls back to <repo>/AGENTS.md when route.persona is unset', async () => { | |
| writeFileSync(join(dir, 'AGENTS.md'), '# persona'); | |
| const db = freshDb(); | |
| const seen: SeenSpawn[] = []; | |
| const runner = threadingRunner(db, seen, [{ instance: INSTANCE, chat: ROUTE_CHAT, repo: dir }]); | |
| runner.handleMessage(...mappedInboundWithId('m', 'id')); | |
| await runner.whenIdle(); | |
| expect(seen[0].personaFile).toBe(join(dir, 'AGENTS.md')); | |
| }); | |
| test('resolves no persona when neither route.persona nor <repo>/AGENTS.md exists', async () => { | |
| const db = freshDb(); | |
| const seen: SeenSpawn[] = []; | |
| const runner = threadingRunner(db, seen, [{ instance: INSTANCE, chat: ROUTE_CHAT, repo: dir }]); | |
| runner.handleMessage(...mappedInboundWithId('m', 'id')); | |
| await runner.whenIdle(); | |
| expect(seen[0].personaFile).toBeUndefined(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/omni-runner.test.ts` around lines 859 - 890, The persona fallback
tests in omni-runner.test are using inline try/finally for tmpdir cleanup, but
the test suite convention is to manage fixtures with afterEach. Refactor the two
tests around threadingRunner to create the temporary directory in setup and
remove it in a shared afterEach hook, while keeping the existing mkdtempSync,
rmSync, freshDb, and SeenSpawn assertions intact. This keeps cleanup consistent
and avoids leaking temp dirs if more assertions are added later.
Source: Path instructions
…odel A review)
Address the BLOCKED review of the Model A one-shot:
- CRITICAL — `claude --session-id <id>` CREATES a session and exits 1 "already
in use" on every turn after the first, so multi-turn was broken. Add a
resume-first orchestration (`runClaudeSession`): attempt `--resume <id>` first
(one spawn for turn 2..N and across `omni serve` restarts, since the session
persists on disk) and fall back to `--session-id <id>` only when the session
is missing. Verified LIVE against claude 2.1.201 — the missing-session error is
actually "No conversation found with session ID" (NOT the "not found" the
review assumed), so the detection regex was broadened accordingly. Extracted a
testable `RawClaudeSpawn` seam so the resume/create branching is unit-tested
without a fork; `buildClaudeArgs` gains a `create | resume` mode.
- MEDIUM — `extractStreamJsonReply` ignored `is_error`: an error/empty terminal
result returned the raw NDJSON blob, published as a ✅ reply. It now returns
`{ reply, isError }`; a soft-error (is_error / non-success subtype / empty
result) sets `isError`, and `runOneShot` treats `(exitCode !== 0 || isError)`
as failure → error notice + ❌, never the raw blob. `SpawnClaudeResult` gains
`isError?`.
- LOW — `resolvePersonaFile` now existsSync-checks an explicit `route.persona`
too (a typo'd path would make claude exit 1 every run); a missing path is
logged and dropped.
Also close stdin (`ignore`) in the default fork so claude doesn't wait ~3s for
piped input. `bun run check` green (+9 tests: resume-first create/resume/no-
fallback/already-in-use regression, error+empty result parsing, soft-error ack,
missing-persona drop). Live end-to-end: turn 1 creates ("OK"), turn 2 resumes
and recalls context ("Quokka").
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/omni-runner.ts`:
- Line 264: The NO_SESSION_RE pattern in omni-runner.ts is too broad because the
generic not found branch can incorrectly treat unrelated resume errors as
missing sessions. Update NO_SESSION_RE to keep only the session-specific phrases
in the resume/error handling logic inside omni-runner.ts, and remove the bare
not found match so existing sessions are not retried with --session-id
unnecessarily.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 83581545-0d96-48eb-aa91-4f8f8f897dcd
📒 Files selected for processing (2)
src/lib/omni-runner.test.tssrc/lib/omni-runner.ts
The bare `|not found` alternative matched ANY "not found" stderr (e.g. a "model not found" / MCP "… not found") on a turn whose session actually exists, triggering a spurious `--session-id` create that then errors "already in use" (one wasted spawn + a worse error). Each remaining alternative is session-scoped: `no conversation found` (claude 2.1.201), `no such session` / `session not found` (legacy phrasings). Add a test asserting a generic "… not found" resume failure is NOT treated as a missing session (resume-only, no create fallback).
…names
The route ⏳→✅ ack (and approval acks) POSTed reactions to /api/v2/messages
with {chatId, reaction}, but omni's send endpoint rejects that (400). Omni's
dedicated reaction route is POST /api/v2/messages/send/reaction expecting
{instanceId, to, messageId, emoji}. Corrected the endpoint + field names.
Contributor
Author
|
Superseded: reopened against dev as the base (PRs to main must come from dev). The 4 commits are cherry-picked cleanly onto dev in the new PR. |
This was referenced Jul 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Enhances
genie omni serve(the NATS bridge letting an Omni WhatsApp instance talk to a Claude Code agent) from a bareclaude -p "<msg>"into Model A: persona injection, session continuity, streaming, and a ⏳→✅/❌ reaction ack on the user's message.Changes
src/lib/omni-runner.tsdefaultSpawnClaudenow spawnsclaude -p --output-format stream-json --verbose --session-id <uuid> [--append-system-prompt-file <persona>] <message>and parses the final reply out of the NDJSON stream.buildClaudeArgs({message, sessionId, personaFile?})— the argv contract.extractStreamJsonReply(raw)— reply extraction: prefer the terminal{"type":"result","subtype":"success","result"}event → fall back to concatenatedassistanttext blocks → fall back to raw stdout on non-stream-json output.deterministicSessionId(instance, chat)— stable v5-shaped UUID (sha256 of the pair) so a conversation resumes across messages, with no state to persist.SpawnClaudeOptsgains optionalpersonaFile+sessionId.runOneShotresolvespersonaFile = route.persona ?? <repo>/AGENTS.md (if it exists)and passes the deterministic session id.messageId) is threadedhandleMessage → startRoutedRun → runOneShot, and a route-scoped ⏳→✅/❌ reaction is set on it: ⏳ right before the spawn, ✅ once a genuine reply is published, ❌ on non-zero exit / timeout / crash.emitStatusReactionis generalized into a sharedemitReactionseam; the route ack reuses it but records no approval glyph and skips the reconciliation guard (the ⏳→✅/❌ pair fires exactly once per run). It stays fire-and-forget (drained bywhenIdle) and never throws.Config —
OmniRoutegains optionalpersona(absolute path) in both the Zod schema (src/types/genie-config.ts) and the runtime type (src/lib/omni-config.ts).How the ack is wired (route-scoped + non-blocking)
The ack targets
{instance: route.instance, chat: route.chat, messageId: <inbound stanza id>}— the route's own chat, notconfig.approvalChat. It goes through the same injectablesetReactionseam as the approval flow, is added toinFlightReactionsand drained bywhenIdle, and the run/publish never awaits the HTTP react. A missingmessageIdis a no-op (no reaction at all).Session-id derivation
sha256("omni-session:<instance>:<chat>"), first 128 bits, shaped into a valid v5-style UUID (version nibble5, variant8/9/a/b). Same(instance, chat)⇒ same id ⇒ conversation continuity; deterministic so two hosts serving the same route converge.Constraints honoured
emitStatusReactionkeeps its exact prior semantics (glyph-recorded + guarded) as a thin wrapper over the shared seam.Tests
12 new tests (injected
spawnClaude+setReaction, zero fork/HTTP): argv contract; stream-json parsing (result / assistant-delta / raw fallback); session-id stability + shape; route-scoped ⏳→✅/❌ acks; ❌ on non-zero exit and on timeout; no reaction whenmessageIdabsent; a failed reaction never blocks the publish; persona resolution (explicit / AGENTS.md fallback / none).bun run check(typecheck + biome + knip + tests): green — 647 pass / 1 skip / 0 fail.Summary by CodeRabbit
stream-json/NDJSON, with safe passthrough fallback.