chore: rolling promotion dev -> main - #1087
Conversation
|
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 (5)
📝 WalkthroughWalkthroughBumps genie package/plugin versions to 4.260407.2; adds a SendMessage PreToolUse hook in the Claude executor to publish Omni replies and deny local handling; expands OmniBridge to handle Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant NATS as NATS (omni.session.reset.>)
participant Bridge as OmniBridge
participant Map as SessionMap
participant Spawn as SpawningPlaceholder
participant Exec as Executor
participant Queue as MessageQueue
NATS->>Bridge: publish reset subject (omni.session.reset.{inst}.{chat})
Bridge->>Map: findSessionKey(instance, chat) (match spawning placeholders or live key)
alt placeholder exists & spawning
Bridge->>Spawn: mark cancelled = true, clear buffered messages, close turn tracker
Bridge->>Map: remove session entry
Bridge->>Queue: drain queued messages (free slot -> spawn new)
Note right of Spawn: when spawn resolves
Spawn->>Exec: created executor detected as cancelled -> shutdown executor
else live session exists
Bridge->>Exec: close turn tracker, executor.shutdown(...)
Bridge->>Map: remove session entry
Bridge->>Queue: drain queued messages
else no session found
Bridge-->>NATS: no-op (no shutdown)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Code Review
This pull request updates the version of the genie plugin and its associated packages from 4.260406.4 to 4.260406.5 across multiple configuration files, including marketplace.json and package.json. I have no feedback to provide as the changes are consistent version increments.
…ly path
Agents spawned via the omni bridge SDK executor (eugenia-seller, etc.)
call `SendMessage(recipient: "omni", message: "...")` to send replies,
mirroring the tmux mode contract. In SDK mode the call was a no-op:
the `done` MCP tool was the only NATS publish path, and the agent's
SendMessage calls fell through with no interception.
Fix: add a PreToolUse hook in `_processDelivery()` that fires only when
OMNI_INSTANCE is set in the executor env. The hook intercepts
`SendMessage` to recipient "omni", side-effect publishes the body to
`omni.reply.{instance}.{chatId}` (mirrors `handleDoneTool`'s text
action), and returns deny + reason "Message delivered to user via omni
bridge." The deny reason becomes the tool result the agent reads, so
the agent treats it as a successful send.
Defensive on field shape: accepts both `recipient`/`to` and
`message`/`content` (matches identity-inject's pattern).
Also updates `turn-based-prompt.ts` to teach SendMessage as the
canonical reply verb — the old prompt referenced `omni say` CLI verbs
that don't exist in SDK mode (no shell, in-process query).
Tests: 8 new cases covering publish path, alternate field shapes,
non-omni passthrough, non-SendMessage passthrough, bridge-unavailable
deny, OMNI_INSTANCE-absent passthrough, and wiring proof in both
directions. Full suite: 2251 pass / 0 fail.
Closes #1088
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ssions Closes #1089 Adds the missing NATS subscription so the bridge actually reacts to session-reset events published by Omni (e.g. user sends 🗑️). Without this wiring, reset events fell on the floor and stale sessions kept serving turns until idle timeout. - subscribe('omni.session.reset.>') in start() (recursive `>` because WhatsApp chat ids contain dots) - processSessionResetEvents() parses subject `omni.session.reset.{instance}.{chat}`, tolerates malformed JSON payloads, and routes to handleSessionReset - handleSessionReset() mirrors handleTurnTimeout: clears idle timer, closes turn, calls executor.shutdown, removes from sessions Map. Cold-chat resets are a logged no-op for traceability. - 7 new tests covering hot/cold reset, idle timer cleanup, dotted WhatsApp chat ids, malformed JSON tolerance, malformed subject rejection, and subscription wiring proof. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ClaudeSdkProvider test registered its own mock.module for @anthropic-ai/claude-agent-sdk, racing with _sdk-mocks.ts. Bun's mock.module is process-global (first registration wins), so in CI (alphabetical file ordering) the provider test's mock won and the executor tests' queryMock was never called — 9 flaky failures. Fix: provider test imports queryMock from _sdk-mocks.ts instead of registering a duplicate mock.module. Single source of truth for the SDK mock across all 3 test files.
Address Codex P2 on #1091. Previously, if the model emitted SendMessage(recipient: 'omni') with the wrong field name (e.g. `text` instead of `message`/`content`) or an empty/whitespace string, the hook still published `content: ""` to the omni reply path AND returned a deny reason that read like success. The agent believed delivery succeeded and would not retry, leaving the user with a blank reply or no reply at all. The hook now rejects empty/whitespace-only bodies with an explicit error reason that prompts the model to retry with a real payload, BEFORE the NATS publish. - Two new tests cover the missing-field and whitespace-only cases. - All 34 claude-sdk tests pass; full gate green (2253/2253). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Address review feedback on #1092: - Codex P1: handleSessionReset misclassified spawning sessions as cold chats because findSessionKey only matched entries with entry.session?.chatId. A reset arriving in the narrow window between placeholder insertion and spawn completion was ignored, so the session finished spawning and kept serving turns — defeating the reset semantics. - Gemini medium: handleSessionReset duplicated the timer-clear and map-delete that removeSession() already encapsulates, and never called drainQueue() so queued messages couldn't take the freed concurrency slot. Changes: - SessionEntry gains an optional `cancelled` flag. - findSessionKey falls back to the map-key suffix (`:${chatId}`) when the entry is in the spawning state and entry.session is null. - handleSessionReset now has two branches: * spawning entries: mark cancelled, clear buffer, close turn, removeSession(), drainQueue(). The in-flight executor.spawn cannot be aborted, but the placeholder is removed immediately so subsequent messages spawn a fresh session. * live entries: close turn, executor.shutdown, removeSession(), drainQueue(). - spawnSession checks placeholder.cancelled after executor.spawn resolves and tears down the freshly-created session if a reset hit mid-spawn (no deliver, no idle timer). - Two new tests: 1. Cancel-mid-spawn: spawn promise is held open, reset fires while placeholder is in spawning state, then spawn is released — asserts shutdown is called and the buffered message is NOT delivered. 2. Drain-after-reset: maxConcurrent=1, queued message waits, reset frees the slot, drainQueue spawns the queued chat. All 27 omni-bridge tests pass. Full gate green (2252/2252). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…omni-routing fix(omni): intercept SendMessage(to: omni) in SDK executor → NATS reply path
…et-subscription fix(omni-bridge): subscribe to omni.session.reset.> and evict live sessions
GitHub API unavailable (no gh CLI, no token, no MCP tools). README not updated per fallback policy. state.json and runs.jsonl updated. https://claude.ai/code/session_01JFHjVBeMjrGvRzEzvXJqgo
feat: cascading model resolution with workspace defaults
|
@codex review this pr |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.genie/agents/metrics-updater/runs.jsonl:
- Line 22: The state-load step for LAST_METRICS is silently discarding Python
stderr via `2>/dev/null || true`, which hides parse/load failures and prevents
fallback; update the state-load invocation in run-metrics.sh (the command that
sets LAST_METRICS) to remove the blind redirection and instead capture the
Python command's stderr and exit status, log a clear error message to stderr or
the script logger including the stderr output and the failing state file name,
and on non-zero exit set a fallback flag or ensure LAST_METRICS is left
unset/empty but record `fallback=true` so cached metrics are used; locate the
block around the LAST_METRICS assignment and replace the `2>/dev/null || true`
pattern with explicit error handling that logs the Python error and handles the
exit code.
In `@src/services/__tests__/omni-bridge.test.ts`:
- Around line 899-927: The test uses a fixed sleep to wait for the placeholder
session to be installed which is timing-sensitive; change the test to use a
deterministic barrier by having the provided spawnFn signal when it has reached
the blocked state (e.g., resolve a "spawnStarted" promise inside spawnFn) and
have the test await that promise before calling bridge.reset, then release the
original spawnGate via releaseSpawn to continue; update the concurrency
assertions to use Promise.allSettled() for any parallel operations (references:
spawnFn, spawnGate/releaseSpawn, routeMessage on bridge, and the OmniBridge
sessions map) so the test no longer relies on setTimeout timing.
In `@src/services/executors/__tests__/claude-sdk.test.ts`:
- Around line 694-719: The test hides the real failure because
waitForDeliveries() only waits the caught queue promise and swallows
_processDelivery() rejections; update the test to reset shared mocks
(resetAllMocks or clearMocks) at the start of this describe and replace the
broad waitForDeliveries() usage with an explicit delivery-specific signal: after
calling executor.deliver(...) await a promise that resolves when that delivery's
processing completes (for example by awaiting a delivery-specific event/Promise
returned by executor.deliver or by spying on executor._processDelivery and
awaiting its resolved/rejected call), then assert queryMock was called; ensure
you reference executor.spawn, executor.deliver, executor._processDelivery and
queryMock when locating the code to change.
In `@src/services/executors/claude-sdk.ts`:
- Around line 208-212: The executor currently returns an empty object when
OMNI_INSTANCE is set but OMNI_CHAT is missing (instanceId/chatId variables),
which silently allows fallback; change this to fail closed by returning the same
explicit deny used in the natsPublish branch (mirror its behavior) so the caller
receives an explicit deny response instead of {}; update the logic in the Claude
executor (where instanceId, chatId, agent are read) to check if instanceId is
present and chatId is missing and then return the explicit deny object used
elsewhere in this module (use the same shape/value as the natsPublish-deny) to
prevent silent fallback to SendMessage.
In `@src/services/omni-bridge.ts`:
- Around line 667-675: The session entry remains routable while awaiting
executor.shutdown, allowing routeMessage to re-resolve it; before awaiting
shutdown in the block handling reset (around the console.log for
sessionKey/actionTag and this.executor.shutdown(entry.session)), mark the
session as non-routable (e.g., set a flag like entry.routable = false or remove
the entry from this.sessions) immediately after
this.turnTracker.close(sessionKey, 'reset'), then await
this.executor.shutdown(entry.session); after shutdown complete call
this.removeSession(sessionKey) and await this.drainQueue(); ensure you update
any callers that check routability (routeMessage) to respect the chosen flag.
- Around line 659-668: The code emits a close with action 'reset' but the
TurnTracker/closedAction type currently permits only
'message'|'react'|'skip'|'timeout', so update the type definitions and
signatures to include 'reset' to keep typing exhaustive and metrics accurate:
add 'reset' to the closedAction union (and any alias types/interfaces) used by
TurnTracker.close and related files (e.g., the TurnTracker class/method
signature and the closedAction type in omni-turn.ts), remove any casts that
force the string, and ensure all handlers/metrics switch statements are updated
to handle the new 'reset' case consistently where TurnTracker.close(sessionKey,
'reset') is used (including the emit in omni-bridge.ts).
- Around line 642-667: Replace the new console.log calls with the module's
logger (use this.logger.debug or appropriate logger method) so core source
doesn't use console.log; e.g., in the reset-path around sessionKey,
entry.spawning and the subsequent branches (the log lines for "Session reset for
cold chat ... — no-op", "Session reset for spawning ... marking cancelled", and
"Session reset for ... evicting") should call this.logger.debug(`[omni-bridge]
...`) (or this.logger.info if you prefer a higher level) instead of console.log,
preserving the same message and actionTag interpolation; ensure the class has a
logger property or use the existing logger instance used elsewhere and remove
the console.log imports/usages afterwards.
🪄 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: 430910b0-df01-49cb-be02-d9205afe2504
📒 Files selected for processing (11)
.claude-plugin/marketplace.json.genie/agents/metrics-updater/runs.jsonl.genie/agents/metrics-updater/state.jsonpackage.jsonplugins/genie/.claude-plugin/plugin.jsonplugins/genie/package.jsonsrc/services/__tests__/omni-bridge.test.tssrc/services/executors/__tests__/claude-sdk.test.tssrc/services/executors/claude-sdk.tssrc/services/executors/turn-based-prompt.tssrc/services/omni-bridge.ts
| {"timestamp":"2026-04-04T00:00:00.000Z","duration_ms":30000,"api_calls":0,"tools_generated":0,"errors":["gh CLI not available","GitHub MCP tools not present in session"],"status":"success","fallback":true,"metrics":{"releases_24h":2,"merged_prs_7d":35,"avg_merge_time_h":0.3,"ship_rate_pct":85}} | ||
| {"timestamp":"2026-04-05T12:24:47Z","duration_ms":32000,"api_calls":0,"tools_generated":0,"errors":["gh CLI not available","GitHub MCP tools not present in session"],"status":"success","fallback":true,"metrics":{"releases_24h":2,"merged_prs_7d":35,"avg_merge_time_h":0.3,"ship_rate_pct":85}} | ||
| {"timestamp":"2026-04-06T12:29:47.000Z","duration_ms":12500,"api_calls":0,"tools_generated":1,"errors":["gh_cli_unavailable: fallback to last_metrics from state.json"],"metrics":{"releases_24h":2,"merged_prs_7d":35,"avg_merge_time_h":0.3,"ship_rate_pct":85}} | ||
| {"timestamp":"2026-04-07T12:04:30Z","duration_ms":27000,"api_calls":0,"tools_generated":0,"errors":["gh CLI not available","no GitHub token or MCP GitHub tools present in session"],"status":"failed","fallback":false,"metrics":null} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Latest persisted state =="
cat .genie/agents/metrics-updater/state.json
echo
echo "== Last 3 run records =="
tail -n 3 .genie/agents/metrics-updater/runs.jsonl
echo
echo "== Verify fallback gate and final logging logic =="
fd run-metrics.sh .genie | while read -r f; do
echo "--- $f"
rg -n -C3 'LAST_METRICS|FETCH_OK|fallback|RUN_STATUS|RUNS_FILE' "$f"
doneRepository: automagik-dev/genie
Length of output: 4877
🏁 Script executed:
#!/bin/bash
# Check if the Python state load in run-metrics.sh has error handling
# and verify the exact logic around lines 79-88
sed -n '75,90p' .genie/agents/metrics-updater/tools/run-metrics.sh
echo "---"
echo "Looking for error handling in Python state load:"
sed -n '78,88p' .genie/agents/metrics-updater/tools/run-metrics.sh | cat -nRepository: automagik-dev/genie
Length of output: 774
State file loading silently fails, disabling fallback when fetch errors occur.
The 2>/dev/null || true on line 86 of .genie/agents/metrics-updater/tools/run-metrics.sh suppresses all stderr from the Python state load, causing LAST_METRICS to remain empty if the state file is corrupted, unparseable, or missing. When line 22 ran and the fetch failed, this silent error prevented fallback from activating—even though cached metrics likely existed. The run was correctly recorded as fallback:false (since no metrics were available), but the root cause (state load failure) is invisible.
Replace 2>/dev/null || true with explicit error handling that logs state load failures so they can be diagnosed and recovered.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.genie/agents/metrics-updater/runs.jsonl at line 22, The state-load step for
LAST_METRICS is silently discarding Python stderr via `2>/dev/null || true`,
which hides parse/load failures and prevents fallback; update the state-load
invocation in run-metrics.sh (the command that sets LAST_METRICS) to remove the
blind redirection and instead capture the Python command's stderr and exit
status, log a clear error message to stderr or the script logger including the
stderr output and the failing state file name, and on non-zero exit set a
fallback flag or ensure LAST_METRICS is left unset/empty but record
`fallback=true` so cached metrics are used; locate the block around the
LAST_METRICS assignment and replace the `2>/dev/null || true` pattern with
explicit error handling that logs the Python error and handles the exit code.
| it('cancels a spawning session on reset and tears down the freshly-spawned executor', async () => { | ||
| // Hold the spawn promise open so we can fire reset mid-spawn. | ||
| let releaseSpawn!: (s: OmniSession) => void; | ||
| const spawnGate = new Promise<OmniSession>((resolve) => { | ||
| releaseSpawn = resolve; | ||
| }); | ||
|
|
||
| const { executor, calls, makeSession } = makeMockExecutor({ | ||
| spawnFn: async (agentName, chatId) => { | ||
| // Block until the test releases the spawn. | ||
| const session = await spawnGate; | ||
| return session ?? makeSession(agentName, chatId); | ||
| }, | ||
| }); | ||
| const bridge = new OmniBridge({ | ||
| natsUrl: 'test://fake', | ||
| pgProvider: degradedPgProvider, | ||
| natsConnectFn: (async () => makeFakeNats()) as any, | ||
| }); | ||
| (bridge as any).executor = executor; | ||
| await bridge.start(); | ||
|
|
||
| try { | ||
| // Kick off the spawn — routeMessage will block on spawnGate. | ||
| const routePromise = (bridge as any).routeMessage(makeMsg({ content: 'first' })); | ||
|
|
||
| // Yield so spawnSession installs the placeholder before we reset. | ||
| await new Promise((r) => setTimeout(r, 5)); | ||
| expect((bridge as any).sessions.has('test-agent:chat-1')).toBe(true); |
There was a problem hiding this comment.
Replace the fixed sleep with a deterministic barrier.
Line 926 makes this reset-during-spawn test timing-sensitive. On a busy runner the placeholder may not exist yet, and on a fast runner the sleep is just dead time. Signal spawnFn when the spawn has reached the blocked state and await that promise instead.
Suggested fix
// Hold the spawn promise open so we can fire reset mid-spawn.
let releaseSpawn!: (s: OmniSession) => void;
+ let markSpawnStarted!: () => void;
const spawnGate = new Promise<OmniSession>((resolve) => {
releaseSpawn = resolve;
});
+ const spawnStarted = new Promise<void>((resolve) => {
+ markSpawnStarted = resolve;
+ });
const { executor, calls, makeSession } = makeMockExecutor({
spawnFn: async (agentName, chatId) => {
+ markSpawnStarted();
// Block until the test releases the spawn.
const session = await spawnGate;
return session ?? makeSession(agentName, chatId);
},
});
@@
- // Yield so spawnSession installs the placeholder before we reset.
- await new Promise((r) => setTimeout(r, 5));
+ // Wait until the spawn path is definitely blocked.
+ await spawnStarted;
expect((bridge as any).sessions.has('test-agent:chat-1')).toBe(true);
expect((bridge as any).sessions.get('test-agent:chat-1').spawning).toBe(true);As per coding guidelines, **/*.test.ts: Use Promise.allSettled() pattern for concurrency tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/__tests__/omni-bridge.test.ts` around lines 899 - 927, The test
uses a fixed sleep to wait for the placeholder session to be installed which is
timing-sensitive; change the test to use a deterministic barrier by having the
provided spawnFn signal when it has reached the blocked state (e.g., resolve a
"spawnStarted" promise inside spawnFn) and have the test await that promise
before calling bridge.reset, then release the original spawnGate via
releaseSpawn to continue; update the concurrency assertions to use
Promise.allSettled() for any parallel operations (references: spawnFn,
spawnGate/releaseSpawn, routeMessage on bridge, and the OmniBridge sessions map)
so the test no longer relies on setTimeout timing.
| it('wires the SendMessage hook into runQuery options when OMNI_INSTANCE is set', async () => { | ||
| const session = await executor.spawn('test-agent', 'chat-wire', { | ||
| OMNI_INSTANCE: 'inst-wire', | ||
| OMNI_CHAT: 'chat-wire', | ||
| OMNI_AGENT: 'wirebot', | ||
| }); | ||
|
|
||
| await executor.deliver(session, { | ||
| content: 'Hi', | ||
| sender: 'Alice', | ||
| instanceId: 'inst-wire', | ||
| chatId: 'chat-wire', | ||
| agent: 'test-agent', | ||
| }); | ||
| await executor.waitForDeliveries(session.id); | ||
|
|
||
| expect(queryMock).toHaveBeenCalled(); | ||
| const callArgs = ( | ||
| queryMock.mock.calls.at(-1) as unknown as [{ options?: { hooks?: Record<string, unknown[]> } }] | ||
| )[0]; | ||
| const preToolUseHooks = callArgs.options?.hooks?.PreToolUse; | ||
| expect(Array.isArray(preToolUseHooks)).toBe(true); | ||
| // permission gate matcher (*) + SendMessage matcher | ||
| const matchers = (preToolUseHooks as Array<{ matcher?: string }>).map((h) => h.matcher); | ||
| expect(matchers).toContain('SendMessage'); | ||
| }); |
There was a problem hiding this comment.
This regression test is already red and it hides the real failure.
The quality gate is failing at Line 710 with 0 queryMock calls. Because waitForDeliveries() only waits on the caught queue promise, any _processDelivery() rejection is swallowed and this test degrades into a misleading "not called" assertion. Reset the shared mocks in this describe and assert through a delivery-specific signal so the underlying setup error surfaces.
🧰 Tools
🪛 GitHub Check: Quality Gate (typecheck + lint + test)
[failure] 710-710: error: expect(received).toHaveBeenCalled()
Expected number of calls: >= 1
Received number of calls: 0
at <anonymous> (/home/runner/_work/genie/genie/src/services/executors/__tests__/claude-sdk.test.ts:710:25)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/executors/__tests__/claude-sdk.test.ts` around lines 694 - 719,
The test hides the real failure because waitForDeliveries() only waits the
caught queue promise and swallows _processDelivery() rejections; update the test
to reset shared mocks (resetAllMocks or clearMocks) at the start of this
describe and replace the broad waitForDeliveries() usage with an explicit
delivery-specific signal: after calling executor.deliver(...) await a promise
that resolves when that delivery's processing completes (for example by awaiting
a delivery-specific event/Promise returned by executor.deliver or by spying on
executor._processDelivery and awaiting its resolved/rejected call), then assert
queryMock was called; ensure you reference executor.spawn, executor.deliver,
executor._processDelivery and queryMock when locating the code to change.
| const instanceId = env.OMNI_INSTANCE ?? ''; | ||
| const chatId = env.OMNI_CHAT ?? ''; | ||
| const agent = env.OMNI_AGENT ?? ''; | ||
|
|
||
| if (!instanceId || !chatId) return {}; |
There was a problem hiding this comment.
Return an explicit deny when bridge context is incomplete.
Once OMNI_INSTANCE is set, this executor is already in turn-based/Omni mode. Returning {} here when OMNI_CHAT is empty silently falls back to native SendMessage, so the model believes it replied even though nothing can be published to omni.reply.*. Mirror the !natsPublish branch and fail closed instead.
Suggested fix
- if (!instanceId || !chatId) return {};
+ if (!instanceId || !chatId) {
+ return {
+ hookSpecificOutput: {
+ hookEventName: 'PreToolUse',
+ permissionDecision: 'deny',
+ permissionDecisionReason: 'Omni bridge unavailable — missing OMNI_INSTANCE/OMNI_CHAT context.',
+ },
+ };
+ }📝 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.
| const instanceId = env.OMNI_INSTANCE ?? ''; | |
| const chatId = env.OMNI_CHAT ?? ''; | |
| const agent = env.OMNI_AGENT ?? ''; | |
| if (!instanceId || !chatId) return {}; | |
| const instanceId = env.OMNI_INSTANCE ?? ''; | |
| const chatId = env.OMNI_CHAT ?? ''; | |
| const agent = env.OMNI_AGENT ?? ''; | |
| if (!instanceId || !chatId) { | |
| return { | |
| hookSpecificOutput: { | |
| hookEventName: 'PreToolUse', | |
| permissionDecision: 'deny', | |
| permissionDecisionReason: 'Omni bridge unavailable — missing OMNI_INSTANCE/OMNI_CHAT context.', | |
| }, | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/executors/claude-sdk.ts` around lines 208 - 212, The executor
currently returns an empty object when OMNI_INSTANCE is set but OMNI_CHAT is
missing (instanceId/chatId variables), which silently allows fallback; change
this to fail closed by returning the same explicit deny used in the natsPublish
branch (mirror its behavior) so the caller receives an explicit deny response
instead of {}; update the logic in the Claude executor (where instanceId,
chatId, agent are read) to check if instanceId is present and chatId is missing
and then return the explicit deny object used elsewhere in this module (use the
same shape/value as the natsPublish-deny) to prevent silent fallback to
SendMessage.
| console.log(`[omni-bridge] Session reset for cold chat ${instanceId}/${chatId} — no-op`); | ||
| return; | ||
| } | ||
|
|
||
| const entry = this.sessions.get(sessionKey); | ||
| if (!entry) return; | ||
|
|
||
| const actionTag = action ? ` (action=${action})` : ''; | ||
|
|
||
| // Spawning sessions: we cannot interrupt the in-flight executor.spawn call, | ||
| // but we can flag the entry so spawnSession tears down the freshly-created | ||
| // session as soon as the await resolves. The placeholder is removed from | ||
| // the map immediately so subsequent messages spawn a fresh session. | ||
| if (entry.spawning) { | ||
| console.log(`[omni-bridge] Session reset for spawning ${sessionKey}${actionTag}, marking cancelled`); | ||
| entry.cancelled = true; | ||
| entry.buffer = []; // Drop buffered messages — user explicitly reset. | ||
| this.turnTracker.close(sessionKey, 'reset'); | ||
| this.removeSession(sessionKey); | ||
| await this.drainQueue(); | ||
| return; | ||
| } | ||
|
|
||
| if (!entry.session) return; | ||
|
|
||
| console.log(`[omni-bridge] Session reset for ${sessionKey}${actionTag}, evicting`); |
There was a problem hiding this comment.
Don't add new console.log calls in src/.
These reset-path logs introduce fresh console.log usage in core source. Route them through the existing logger path or a permitted level instead.
As per coding guidelines, src/**/*.ts: No console.log in source code (enforced by biome, relaxed in tests).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/omni-bridge.ts` around lines 642 - 667, Replace the new
console.log calls with the module's logger (use this.logger.debug or appropriate
logger method) so core source doesn't use console.log; e.g., in the reset-path
around sessionKey, entry.spawning and the subsequent branches (the log lines for
"Session reset for cold chat ... — no-op", "Session reset for spawning ...
marking cancelled", and "Session reset for ... evicting") should call
this.logger.debug(`[omni-bridge] ...`) (or this.logger.info if you prefer a
higher level) instead of console.log, preserving the same message and actionTag
interpolation; ensure the class has a logger property or use the existing logger
instance used elsewhere and remove the console.log imports/usages afterwards.
| this.turnTracker.close(sessionKey, 'reset'); | ||
| this.removeSession(sessionKey); | ||
| await this.drainQueue(); | ||
| return; | ||
| } | ||
|
|
||
| if (!entry.session) return; | ||
|
|
||
| console.log(`[omni-bridge] Session reset for ${sessionKey}${actionTag}, evicting`); | ||
| this.turnTracker.close(sessionKey, 'reset'); |
There was a problem hiding this comment.
reset is outside the current turn-close contract.
TurnTracker.close() currently stores closedAction as 'message' | 'react' | 'skip' | 'timeout'. Emitting 'reset' here only works because src/services/omni-turn.ts casts the string, so downstream exhaustive handling and metrics still don't know reset closes exist. Extend that union before writing this value.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/omni-bridge.ts` around lines 659 - 668, The code emits a close
with action 'reset' but the TurnTracker/closedAction type currently permits only
'message'|'react'|'skip'|'timeout', so update the type definitions and
signatures to include 'reset' to keep typing exhaustive and metrics accurate:
add 'reset' to the closedAction union (and any alias types/interfaces) used by
TurnTracker.close and related files (e.g., the TurnTracker class/method
signature and the closedAction type in omni-turn.ts), remove any casts that
force the string, and ensure all handlers/metrics switch statements are updated
to handle the new 'reset' case consistently where TurnTracker.close(sessionKey,
'reset') is used (including the emit in omni-bridge.ts).
| console.log(`[omni-bridge] Session reset for ${sessionKey}${actionTag}, evicting`); | ||
| this.turnTracker.close(sessionKey, 'reset'); | ||
| try { | ||
| await this.executor.shutdown(entry.session); | ||
| } catch (err) { | ||
| console.warn(`[omni-bridge] Error shutting down reset session ${sessionKey}:`, err); | ||
| } | ||
| this.removeSession(sessionKey); | ||
| await this.drainQueue(); |
There was a problem hiding this comment.
Make the session non-routable before awaiting shutdown.
The first await in this branch is executor.shutdown(entry.session), but the entry stays in this.sessions until that completes. If another omni.message.* arrives for the same chat in that window, routeMessage() can still resolve this entry and deliver into the session the user just reset. Remove or mark it non-routable before the await.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/omni-bridge.ts` around lines 667 - 675, The session entry
remains routable while awaiting executor.shutdown, allowing routeMessage to
re-resolve it; before awaiting shutdown in the block handling reset (around the
console.log for sessionKey/actionTag and this.executor.shutdown(entry.session)),
mark the session as non-routable (e.g., set a flag like entry.routable = false
or remove the entry from this.sessions) immediately after
this.turnTracker.close(sessionKey, 'reset'), then await
this.executor.shutdown(entry.session); after shutdown complete call
this.removeSession(sessionKey) and await this.drainQueue(); ensure you update
any callers that check routability (routeMessage) to respect the chosen flag.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ae5f390e3
ℹ️ 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".
| if (placeholder.cancelled) { | ||
| console.log(`[omni-bridge] Spawn for ${key} completed but was cancelled by reset, shutting down`); | ||
| try { | ||
| await this.executor.shutdown(session); | ||
| } catch (err) { |
There was a problem hiding this comment.
Guard cancelled-spawn cleanup against newer same-key sessions
When a reset arrives during spawnSession, the placeholder is removed immediately, so a new spawn for the same {agent}:{chat} key can start before the original executor.spawn resolves. In that case, this cancelled-path executor.shutdown(session) can target the newer live session because executors key state by session.id (same {agent}:{chat}), which can terminate the replacement session and drop subsequent messages. Add a generation/token check (or verify the current map entry is still this placeholder) before shutting down.
Useful? React with 👍 / 👎.
Rolling Promotion PR
Auto-maintained rolling promotion PR from
devtomain.Process:
ready-to-mergeadded when all checks passSummary by CodeRabbit
Chores
New Features
Improvements
Tests