Skip to content

fix(omni-runner): reaction guard, -- terminator, stderr on failure - #2520

Merged
namastex888 merged 1 commit into
devfrom
fix/omni-route-reaction-guard
Jul 5, 2026
Merged

namastex888 merged 1 commit into
devfrom
fix/omni-route-reaction-guard

Conversation

@namastex888

Copy link
Copy Markdown
Contributor

Closes the three omni-serve findings confirmed by the independent review record on #2516 (comment trail there).

Fixes

  1. Reaction/empty-frame guard — a reaction frame in a route-mapped chat used to spawn a spurious claude -p run (prompted with [Reaction: …]), publish a reply to the chat, and flip the ⏳/✅ ack on the reacted-to message. startRoutedRun is now guarded by the existing parseReaction helper plus a blank-body check: such frames are stored to the inbox only. The approval-chat reaction path is untouched — including the previously untested topology where the route chat IS the approval chat (new coexistence test proves 0 spawns + approval still resolves).
  2. -- option terminator — buildClaudeArgs inserts -- before the message positional, so hyphen-leading messages (-v, --help) are prompts, never flags. Verified live against the claude CLI; both create and resume argv paths covered.
  3. stderr surfaced on non-zero exit — the error notice now carries a 500-char tail of stderr (fallback stdout) instead of a bare exit code N; SpawnClaudeResult gains optional stderr through the existing concurrent drain pattern (no new deadlock surface).

Verification

  • bun test src/lib/omni-runner.test.ts: 52 pass / 0 fail (45 pre-existing + 7 new, all revert-sensitive)
  • bun run typecheck + biome: clean
  • Independent review verdict: SHIP — control-flow proof that the guarded call site is the only spawn/publish/ack entry for routed chats; 6/6 adversarial probes (word "Reaction:", multiline, embedded frame, malformed frame, newlines-only, hyphen-only message)
  • Two LOW informational notes (documented, no code change): an unanchored [Reaction: …] frame embedded in a longer legit message is store-only (consistent with the approval path's regex), and a malformed frame omni never emits would still spawn.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Sg8vJv9r2yqmnbtVPqM2vG

Closes the three findings confirmed by the independent reviews on #2516:

- Reaction frames (and blank bodies) in a route-mapped chat no longer
  spawn a claude run, publish a reply, or mutate the reacted-to message's
  status ack — they are stored to the inbox only. The approval-chat
  reaction path is untouched, including when the route chat IS the
  approval chat (new coexistence test).
- buildClaudeArgs inserts '--' before the message positional so a
  hyphen-leading message is always a prompt, never parsed as a flag
  (verified live against the claude CLI).
- Non-zero exits now surface a bounded tail of the child's stderr
  (fallback stdout) in the error notice instead of a bare exit code;
  SpawnClaudeResult carries stderr through the existing drain pattern.

52 tests (45 pre-existing + 7 new), typecheck and biome clean.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Sg8vJv9r2yqmnbtVPqM2vG
@coderabbitai

coderabbitai Bot commented Jul 5, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 2a6e8d61-bd60-4aff-be6d-26c3ed64f036

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/omni-route-reaction-guard

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@namastex888
namastex888 merged commit eee864b into dev Jul 5, 2026
11 checks passed

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request improves error reporting for Claude runner failures by capturing and surfacing the tail of stderr (or falling back to stdout) on non-zero exits. It also adds an option terminator (--) to prevent hyphen-leading prompts from being parsed as flags, and implements a guard to prevent reaction frames or empty messages from triggering runs. The feedback identifies a bug in the tailOf helper function where passing max = 1 causes text.slice(-0) to return the entire string instead of the truncated tail, and provides a code suggestion to fix it.

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 thread src/lib/omni-runner.ts
Comment on lines +624 to +628
function tailOf(text: string, max: number): string {
if (max <= 0) return '';
if (text.length <= max) return text;
return `…${text.slice(-(max - 1))}`;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

When max is 1, max - 1 evaluates to 0. In JavaScript/TypeScript, text.slice(-0) is equivalent to text.slice(0), which returns the entire string instead of an empty string. This causes tailOf to return a string longer than max (e.g., tailOf("abcdef", 1) returns …abcdef instead of …). Using text.slice(text.length - max + 1) avoids negative indices and correctly handles max = 1.

Suggested change
function tailOf(text: string, max: number): string {
if (max <= 0) return '';
if (text.length <= max) return text;
return `…${text.slice(-(max - 1))}`;
}
function tailOf(text: string, max: number): string {
if (max <= 0) return '';
if (text.length <= max) return text;
return '…' + text.slice(text.length - max + 1);
}

@namastex888 namastex888 mentioned this pull request Jul 5, 2026
@automagik-genie
automagik-genie deleted the fix/omni-route-reaction-guard branch September 25, 2026 04:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant