Subagent improvements: researcher subagent, cascade cancel, child tool allowlist#10256
Open
skanderkaroui wants to merge 23 commits into
Open
Subagent improvements: researcher subagent, cascade cancel, child tool allowlist#10256skanderkaroui wants to merge 23 commits into
skanderkaroui wants to merge 23 commits into
Conversation
…exploration Declare a 'researcher' subagent via the Claude Agent SDK agents option, restricted to the three omi-tools data tools (execute_sql, semantic_search, get_daily_recap). Broad/exploratory data questions now run in a disposable child context that returns distilled findings, keeping raw query rows out of the main session's context window — the main driver of long-session degradation. Task added to allowedTools (required for subagent invocation); option assembly extracted to query-config.mjs so the definitions are unit testable (first tests in agent-cloud). Verification: - npm test (agent-cloud): 3/3 pass (researcher tool allowlist, schema in prompt, Task in allowedTools). - Live one-shot CLI run against a seeded local omi.db copy (120 screenshots, 3 action items): model output began "I'll delegate this to the researcher agent", returned correct distilled findings (60/30/30 app split, rotation pattern, open tasks) with no raw rows. SDK 0.2.41 accepted the agents option end-to-end. Note: CLI-mode tool_activity logging does not surface subagent tool calls; delegation evidenced by model output and cost profile.
cancelRun only cancelled its target: delegated (run-and-wait) children kept running as orphans after the user stopped the parent. The delegations parent_run_id link existed but nothing traversed it on cancel — only the mid-batch spawn-failure path (control-tools sibling cleanup) reaped children. cancelRun now cascades best-effort to every delegated child after the parent's own cancellation is persisted and dispatched. Recursion covers grandchildren; terminal children hit the existing accepted:false early-return (idempotent). Delegation rows stay untouched — executeDelegationAsync settles them to "cancelled" when the child's execution resolves, so the parent's pending await resolves normally with no double-settle. Background agents spawned via spawn_background_agent have no delegation row and are deliberately NOT cascaded: they are user-visible floating-bar work designed to outlive the parent turn (pinned by regression test). Intentional edges: cancelling an already-terminal parent does not reap still-running spawn-mode children (early-return precedes cascade); the multi-spawn failure cleanup now cascades transitively through cancelled delegated siblings. Failure-Class: none Verification: - npx vitest run tests/cancellation.test.ts: 5/5 pass (3 new: in-flight delegated child cascades with single delegation settle; terminal child untouched; background agent not cancelled). - Confirmed the new cascade test fails on the unfixed kernel-runs.ts (adapter.cancelled length 1 vs expected 2) and passes with the fix. - npx tsc --noEmit: clean.
Add optional toolPolicy.allowedToolNames to SpawnBackgroundAgentInput and DelegateAgentInput. The policy travels in run metadata JSON (already persisted by both spawn paths and already parsed by the capability broker — no schema change) and is enforced at capability registration: the broker intersects it with the role/adapter-computed tool set before freezing allowedToolNames, so every authorize() call is covered. Empty or malformed policy fails closed to no tools. Restriction-only semantics: a policy can narrow, never widen, the child's tool set. Exposed on the spawn_agent and spawn_background_agent control-tool schemas and manifests so the coordinator model can restrict children (Claude Code principle: specialized subagents with reduced toolsets). run_agent_and_wait deliberately skipped until needed. Generated tool surfaces regenerated via agent/scripts/generate-tool-surfaces.mjs (GeneratedToolCapabilities.swift, GeneratedToolExecutors.swift, tool-manifest fixture). Verification: - npm test (desktop/macos/agent): 49 files, 587/587 pass, including new cases: broker intersection honored (get_memories allowed, search_memories tool_not_allowed), empty/malformed policy fails closed, policy persisted through both spawn paths, absent policy leaves behavior unchanged, both spawn schemas accept valid toolPolicy and reject malformed shapes. - npx tsc --noEmit: clean. - Note: runtime-stdio-contract tests require a built dist/ (fresh-worktree timeout is environmental, passes after npm run build; verified unrelated to this change by stash-testing).
Evidence (2026-07-21, 26 instrumented runs on the mini against a real 27MB omi.db copy — 5,110 screenshots, 305 open tasks, 1,059 memories): - haiku researcher: fastest (44s vs 64s inherited-Opus forced runs) and ~3x cheaper ($0.13 vs $0.44 on content recall) with equal-or-better accuracy (pinned the correct last-seen date where baseline was off by one). - Delegation tax (~20s) only pays off on row-heavy tasks; natural delegation correctly fired on patterns/triage/recall and stayed home on quick lookups. - Worst observed pathology: researcher called get_daily_recap once per day (12-turn run). Prompt-level turn-discipline instructions were tested and are model-dependent (Opus obeys, haiku gets worse) — rejected; fixing the tool description is model-independent. Changes: pin researcher model to haiku; route delegation by row-volume in both the agent description and the main-prompt guideline; state in both the tool description and researcher prompt that one get_daily_recap call covers any date range. Verification: npm test 3/3 (model + range-wording pinned); full 8-turn session sim on real data ran the researcher config 23% faster and 22% cheaper than baseline with parity accuracy (results in PR discussion).
… progress, interrupted partials — with hermetic e2e Root cause found by instrumented session sims on real data: agent.mjs never set includePartialMessages, so its entire stream_event handling was dead code — no token ever streamed, text arrived only as whole assistant messages, and every turn contained 17-46s of total client silence (measured; the mechanism behind 'Response stopped' churn). Enabling partials cut measured max silent gaps to ~13s before the mini link dropped mid-validation; the durable guard is the new hermetic e2e below. Changes: - includePartialMessages: true in both query option blocks; the existing delta/dedup handling was already written for it. - stream-routing.mjs: subagent-origin messages (parent_tool_use_id) surface tool starts as 'subagent:<name>' tool_activity and never leak internal text into the answer stream. - result messages carry interrupted: true when a stop/preemption cut the turn short — clients can finally distinguish partials from complete answers. - Fix (found by the e2e): the prewarm-completion branch reset turnActive, clobbering a query that arrived before prewarm finished and turning that turn's stop into a silent no-op. Prewarm never sets turnActive; it no longer clears it. - Controllable SDK seam (OMI_AGENT_SDK_MODULE) + OMI_AGENT_DISABLE_UPDATES gate so the server runs hermetically under test. - tests/agent-ws-e2e.test.mjs: spawns the real server with a scripted SDK fixture, drives a real WebSocket, asserts: text streams as multiple deltas, subagent progress surfaces, the SECRET-INTERNAL leak canary never reaches the client, and stopped turns return interrupted: true with partial text. - agent-cloud-tests registered in .github/checks-manifest.yaml (local + ci lanes) — also puts the existing unit tests under CI for the first time. Verification: bash desktop/macos/agent-cloud/scripts/run-tests.sh from clean node_modules — 3 files, 8/8 pass (unit + e2e). python3 .github/scripts/test_run_checks.py — manifest contract OK. Live partial validation on the mini (2 turns before network drop): max_gap 20-46s → ~13s, subagent tool events now visible to the client.
…ntact Reconnect amnesia, reproduced live on real data before this fix: stop a turn, reconnect (what the phone does after backgrounding), ask "which of those task themes you listed earlier had the most items?" — the agent replied "I don't have any earlier conversation context here". The session was owned by the WS connection and torn down on close. The VM is single-user, so the persistent session now lives at server scope with a swappable client sink: disconnect detaches the sink (in-flight turns keep running; their result text survives for the reattached client), reconnect reattaches, and a session_state hello tells the client whether a live session with context exists. detachSink only clears its own socket's sink — a stale close event firing after a newer socket attached must not silence the fresh connection (race caught by the e2e while writing it). Follow-up (proxy-side, separate PR): agent-proxy injects last-10-message history on each connection's first query; with surviving sessions that becomes redundant on reconnect — the session_state hello is the signal it should key on. Failure-Class: none Verification: npm test — 9/9 including the new e2e case: turn counter proves the same SDK session consumes queries across a disconnect/reconnect (a fresh session would restart the count); hello reports active:true.
…l-battery evidence Evidence loop (2026-07-22, 18 live runs + 2 session sims on a real 27MB DB): round 1 exposed four waste patterns; round 2 after these changes measured wall −62%, tool calls 53→20, cost −33% across the affected tasks with correctness verified against ground truth (dupes 297/8 exact, Jul-14 4x activity ratio, honest no-data answer, Warp 37% share). Tool reshapes (new data-tools.mjs, unit-tested, injection-safe dates): - get_daily_recap accepts start_date/end_date — it could only anchor to "now" (days_ago), so date-specific recaps fell back to 13+ hand-rolled SQL turns (82s→29s, 17→2 tools). Short ranges add an hourly timeline and top window titles. - Empty ranges return an authoritative zero-activity statement — an empty-ish reply previously triggered 7 verification queries (39s→21s). - New get_app_usage: day-by-app matrix in one call, replacing per-day GROUP BY spelunking (compare task 73s→44s, 16→3 tools). Researcher gets it. - Task near-duplicate guidance (COUNT(DISTINCT description)) — the store carries ~63% near-duplicate task extractions; the dupes task dropped 221s→28s while matching exact ground truth. - Documented negative result: the ~2 ToolSearch turns per conversation are SDK schema deferral; allowedTools naming does not preload (tested). WS contract (session sims on fixed streaming code): - Turn heartbeat (OMI_TURN_HEARTBEAT_MS, default 10s): light turns now gap 1-3s but delegated turns still went 40-64s silent while subagent text is (deliberately) swallowed — a periodic status caps the client-visible gap regardless of source. - Real-SDK interrupts terminalize with a non-success subtype; a user stop previously surfaced as an error. Non-success + interruptRequested now maps to an interrupted:true partial result. Fixture models the real subtype. Also validated this cycle: reconnect memory probe answers from context in 5.5s with zero tool calls (was: total amnesia). Verification: npm test — 4 files, 15/15 (data-tools unit tests; e2e asserts heartbeats during held turns and interrupted partials). Battery and sim logs: scratchpad eval/ (round1 vs round2 TSVs).
Collaborator
|
Thanks for the careful work here — the direction looks useful and the focused regression coverage is strong. I reviewed this as an agent-control/agent-behavior change. The important maintainer consequence is that this PR changes how Omi agents are instructed and constrained:
That all looks conceptually safe from this pass: the tool policy intersects with the existing role/adapter allowlist rather than widening it, malformed persisted policy fails closed, and the cancellation behavior is covered by regression tests. I also ran the targeted tests locally:
I’m leaving this as a positive signal rather than formal approval because it still warrants human maintainer sign-off before merge: it changes agent behavior/control-plane semantics, touches a lockfile/dependency surface, and GitHub still shows blocked/pending/cancelled checks. Once the queued/cancelled CI state is resolved, this looks like a good candidate for maintainer review. by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with |
Git-on-my-level
added
positive-signal
Rule table over the observed 30-day chat_agent_error corpus: billing exhaustion, auth expiry, OAuth timeout, connection, oversized payload, runtime crash, tool-schema rejection, rate limit/overload, local data errors. Each class carries honest user copy and a retryable verdict — the opaque bucket's top cause (exhausted Anthropic credits, 117/401 events) was being told 'please try again', producing measured retry storms (32 events from 2 users on Jul 6 alone). Verification: AgentErrorClassifierTests (9 behavioral cases, committed with the test file later in this series); full build green.
userFacingAgentErrorMessage delegates to AgentErrorClassifier — copy and retry advice now come from one bounded table instead of ad-hoc substring checks that collapsed most causes into 'Something went wrong. Please try again.' Verification: build green; behavior pinned by AgentErrorClassifierTests.
…etry chat_agent_error carried only the coarse error_class enum; the daemon's typed failure envelope (failureCode/source/adapterId/provider/retryable) was parsed into AgentRuntimeFailure and then dropped, which is why the churn dashboard needed raw-string archaeology. The failed event now threads an optional ChatQueryErrorDetail: error_code (classifier code or daemon failureCode), retryable, failure_code, failure_source, adapter_id, provider — all closed vocabularies, never raw exception text. Note: enum case gains a slot, so this commit compiles only with the two follow-up call-site commits (per-file commit series per maintainer request; series tip is build- and test-green). Verification: ChatQueryTelemetryTests 25/25 incl. new payload tests asserting bounded properties and no raw-text leakage.
Pattern-match gains the detail slot; no behavior change. Verification: build green at series tip.
The disposition-classify site has the full Error in hand — derive ChatQueryErrorDetail.from(error) so agentError strings and runtime failure envelopes reach telemetry as bounded dimensions. Verification: build green; payload pinned by ChatQueryTelemetryTests.
Nine cases mirroring the observed error corpus: billing exhaustion must not say 'try again', auth expiry routes to reconnect, connection and runtime crashes are retryable, local data errors are classified for bug tracking, tool-schema rejections don't blame the user, unknown preserves readable messages.
Asserts error_code/retryable reach the payload, daemon taxonomy fields survive from AgentRuntimeFailure, and raw exception text never enters analytics properties.
Collapse the detail-threading call to one line (fits the 120-col limit) instead of raising the 6214-line ratchet baseline.
AgentBridge.swift shrank when the ad-hoc error heuristics moved to the classifier; the down-only ratchet records the new ceiling.
Error copy for chat failures is exercised by the same hermetic chat flow that covers AgentBridge and ChatQueryTelemetry.
Pre-existing drift on main (blocks every branch's swift-format-lint preflight); formatted with the pinned wrapper, no behavior change.
…tchet The pinned swift-format output for the drifted file is 4544 lines; the 4543 baseline froze the unformatted shape, wedging swift-format-lint against the ratchet for anyone touching the file. Formatter shape, not feature growth.
Second drifted file from the same upstream refactor (d9947b5); formatted with the pinned wrapper, no behavior change. Full lint-scope is clean after this.
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
2 participants
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.
Subagent improvements: researcher subagent, cascade cancel, child tool allowlist
Three independent changes adopting Claude Code subagent principles in the omi agent stack.
Changes
researchersubagent (SDKagentsoption) restricted toexecute_sql/semantic_search/get_daily_recap. Broad data exploration runs in a disposable child context and returns distilled findings, keeping raw rows out of the main session.cancelRunnow cancels non-terminal delegated children (delegationsparent_run_idlink), recursively, best-effort, after the parent's own cancellation is persisted. Background agents (no delegation row) are deliberately not cascaded — they are user-visible floating-bar work designed to outlive the parent turn (pinned by regression test).toolPolicy.allowedToolNamesonspawn_agent/spawn_background_agent/delegateAgent, carried in run metadata, enforced by intersection at capability registration in the broker. Empty or malformed policy fails closed. Narrow-only semantics.Product invariants
desktop/macos/agent/src/runtime/**. Lifecycle section affected by the cascade (guard tests updated:tests/cancellation.test.ts— 3 new behavioral cases;tests/run-tool-capability.test.ts— 3 new toolPolicy cases;tests/control-tools.test.ts— spawn schema cases; newtests/spawn-tool-policy.test.ts). The toolPolicy change strengthens "leaf workers get no agent-management tools": policies can only narrow the broker-computed set, never widen it; capability checks remain kernel-internal.Desktop/Sources/Generated/*viagenerate-tool-surfaces.mjs); no chat write-path, projection, or timeline behavior changed.Failure-Class: none
Verification
desktop/macos/agent:npm test49 files / 587 pass;npx tsc --noEmitclean; cascade regression test verified to fail on unfixed code.desktop/macos && ./scripts/agent-logic-harness.sh: passed (Swift lifecycle/state, agent runtime focused, pi-mono exact package; 129s).agent-cloud:npm test3/3 (first tests in the package); live one-shot CLI run against a seeded local DB copy showed the model delegating to the researcher and returning distilled findings (no raw rows).Round 2 — measured optimization loop (4 further commits)
Driven by an instrumented evaluation loop on a real 27MB database copy (~40 live runs + session simulations; experiment infra external to this repo):
includePartialMessageswas never enabled (all streaming code was dead; measured 17-46s per-turn client silence). Subagent progress surfaces assubagent:*tool_activity; internal subagent text can never leak (canary-asserted). Fixes a prewarm race that made Stop a silent no-op on fast first queries. Adds the scripted-SDK e2e harness and registersagent-cloud-testsin the CI check manifest (first CI coverage for this package).session_statehello, stale-socket detach race handled. Proxy follow-up noted for history injection keying.get_daily_recapgains explicit date ranges (was anchored to "now" — root cause of 13+ SQL-turn fallbacks), authoritative empty-range statements, hourly timeline/top windows; newget_app_usageday-by-app matrix tool; task near-duplicate guidance; 10s turn heartbeat; real-SDK interrupts map tointerrupted: truepartial results instead of errors.End-to-end validated deltas (same 8-turn conversation, real data): wall 256s → 155s (−39%), cost $0.73 → $0.33 (−55%), worst silent gap 46s → 13s (typical 1–4s), stop → marked partial, reconnect → context retained. Task battery: −62% wall, −62% tool calls, correctness verified against ground truth. Suite: 15/15 (4 files) via
desktop/macos/agent-cloud/scripts/run-tests.sh; full preflight 43/43.Round 3 — honest chat error messages + bounded error telemetry (desktop)
Driven by a PostHog deep dive into the opaque "Something went wrong" bucket (401 events / 108 users / 30 days): raw_error decomposes it entirely (exhausted Anthropic credits 117, OAuth timeout 29, expired tokens 23, runtime crashes 23, connection 12, oversized payloads 10), and the three "unexplained" error spikes were retry storms caused by the copy itself — users with exhausted credits obeying "please try again".
AgentErrorClassifier: bounded classification (12 codes) with honest copy and retryable verdicts; "try again" only when a retry can help. Bridge copy routes through it.chat_agent_errorgains boundederror_code,retryable,failure_code,failure_source,adapter_id,provider(the daemon's typed envelope was parsed then dropped). Closed vocabularies only — never raw exception text (test-pinned).Verification: swift build green; AgentErrorClassifierTests + ChatQueryTelemetryTests 34/34; check_desktop_test_quality at baseline; full preflight 20/20 on the source branch and re-run green after this merge.