(MOT-3955) fix: make subagent approvals and failures visible#469
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR adds bounded transient recovery and structured failure reporting to the harness, live conversation-scoped approval watching to the console, durable lifecycle notices and trace outcomes, deferred approval-gate hook registration, and updated orchestration documentation. ChangesHarness execution and orchestration
Console approval and lifecycle presentation
Approval-gate hook registration
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: Poem
🚥 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 |
skill-check — worker0 verified, 41 skipped (no docs/).
Four for four. Nicely done. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
harness/src/functions/react.rs (1)
330-340: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFire-rate breaker trips now flood the transcript with durable entries.
Each trip calls
record_reaction_outcomewith a hash-basedentry_idderived from the full event JSON, so distinct triggering events (differentturn_id/timestamp) never dedupe. In the exact runaway/self-triggering scenario this breaker exists to contain, every blocked fire now persists a unique durable "reaction" entry into the session transcript, instead of the previous transienttracing::warn!— turning a safety valve into a storage/UI-spam vector during an incident.Consider either not persisting a durable entry per individual breaker trip (log-only, as before), or using a coarser, window-scoped
entry_id(e.g. keyed bygate_key+ time-bucket) so repeated trips within a window collapse into one durable notice.🤖 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 `@harness/src/functions/react.rs` around lines 330 - 340, Fire-rate breaker trips currently persist a unique transcript entry for every blocked event. In the !deps.react_gate.admit branch of the reaction handler, avoid calling record_reaction_outcome for each trip and retain only the tracing warning, or generate a coarser window-scoped identifier based on gate_key and the fire-rate time bucket so repeated trips deduplicate.
🧹 Nitpick comments (1)
approval-gate/src/configuration.rs (1)
178-203: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd exponential backoff to the retry loop.
The fixed 500ms interval produces ~4
warn-level log lines per second (two frombind_hook, two frombind_filesystem_access_watch_hook) for the entire duration the harness is unavailable. In a slow-start or outage scenario this floods logs and adds unnecessary load on the engine. A capped exponential backoff preserves fast initial detection while throttling noise.♻️ Proposed backoff with 30s cap
pub fn retry_hook_bindings(iii: IIIClient) { tokio::spawn(async move { + let mut interval = HOOK_RETRY_INTERVAL_MS; loop { let pre_trigger_ready = trigger_instance_count(&iii, "harness::hook::pre-trigger") .await .is_some_and(|count| count > 0); if !pre_trigger_ready { bind_hook(&iii); } let post_trigger_ready = trigger_instance_count(&iii, "harness::hook::post-trigger") .await .is_some_and(|count| count > 0); if !post_trigger_ready { bind_filesystem_access_watch_hook(&iii); } if pre_trigger_ready && post_trigger_ready { tracing::info!("approval-gate hook bindings confirmed"); break; } - tokio::time::sleep(Duration::from_millis(HOOK_RETRY_INTERVAL_MS)).await; + tokio::time::sleep(Duration::from_millis(interval)).await; + interval = (interval * 2).min(30_000); } }); }🤖 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 `@approval-gate/src/configuration.rs` around lines 178 - 203, retry_hook_bindings currently retries at a fixed interval, flooding logs while the harness is unavailable. Add capped exponential backoff to the loop in retry_hook_bindings: start with the existing 500ms delay, increase the delay after each unsuccessful iteration, cap it at 30 seconds, and reset or stop as appropriate once both hook bindings are confirmed.
🤖 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 `@approval-gate/src/configuration.rs`:
- Around line 205-216: Update trigger_instance_count to distinguish trigger
probe failures from a legitimate zero or missing count: stop using .ok()? and
the as_u64() fallback, propagate or explicitly report trigger/response-shape
errors, and ensure callers of trigger_instance_count handle the failure rather
than treating it as “not ready” indefinitely.
In `@harness/src/functions/status.rs`:
- Line 80: Compute partial_result_available from the failure/recovery state or a
dedicated persisted partial-result marker rather than record.result.is_some();
update the status construction near finalize_completed handling so normal
successful outputs are reported as non-partial while recovered turns with
preserved partial output remain true.
In `@tech-specs/2026-06-agentic/harness.md`:
- Around line 629-632: Update the harness::react orchestration description to
state that only events with status "completed" and no result_error spawn the
downstream sub-agent; clarify that completed events containing result_error
follow the failure path. Preserve the existing behavior for exhausted, failed,
or cancelled completions and the metadata.continue_on_error override.
---
Outside diff comments:
In `@harness/src/functions/react.rs`:
- Around line 330-340: Fire-rate breaker trips currently persist a unique
transcript entry for every blocked event. In the !deps.react_gate.admit branch
of the reaction handler, avoid calling record_reaction_outcome for each trip and
retain only the tracing warning, or generate a coarser window-scoped identifier
based on gate_key and the fire-rate time bucket so repeated trips deduplicate.
---
Nitpick comments:
In `@approval-gate/src/configuration.rs`:
- Around line 178-203: retry_hook_bindings currently retries at a fixed
interval, flooding logs while the harness is unavailable. Add capped exponential
backoff to the loop in retry_hook_bindings: start with the existing 500ms delay,
increase the delay after each unsuccessful iteration, cap it at 30 seconds, and
reset or stop as appropriate once both hook bindings are confirmed.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0bcf1294-44ff-41c8-b76f-94d5dca3dcc3
📒 Files selected for processing (45)
approval-gate/src/configuration.rsapproval-gate/src/main.rsconsole/web/src/components/chat/ChatView.tsxconsole/web/src/components/function-call/FunctionCallCard.tsxconsole/web/src/components/permissions/PermissionModePicker.tsxconsole/web/src/components/ui/ModeToggle.tsxconsole/web/src/hooks/use-conversations.test.tsconsole/web/src/hooks/use-conversations.tsconsole/web/src/lib/backend/approval-events-live.test.tsconsole/web/src/lib/backend/approval-events-live.tsconsole/web/src/lib/backend/harness-send.test.tsconsole/web/src/lib/backend/harness-send.tsconsole/web/src/lib/backend/real.tsconsole/web/src/lib/backend/translate.test.tsconsole/web/src/lib/backend/translate.tsconsole/web/src/lib/backend/types.tsconsole/web/src/lib/sessions/entry-mapper.test.tsconsole/web/src/lib/sessions/entry-mapper.tsconsole/web/src/pages/TracesV2/lib/traceListItem.test.tsconsole/web/src/pages/TracesV2/lib/traceListItem.tsconsole/web/src/types/iii-agent-event.tsharness/README.mdharness/prompts/cli.txtharness/prompts/default.txtharness/src/config.rsharness/src/deferred.rsharness/src/functions/react.rsharness/src/functions/send.rsharness/src/functions/status.rsharness/src/functions/subscribe.rsharness/src/functions/turn.rsharness/src/ids.rsharness/src/subagent.rsharness/src/turn_loop.rsharness/src/types/turn.rsharness/tests/golden/schemas/harness.status.jsonllm-router/src/chat/chat.rsllm-router/src/chat/telemetry.rsprovider-anthropic/prompts/identity.txtprovider-llamacpp/prompts/identity.txtprovider-openai-codex/prompts/identity.txtprovider-openai/prompts/identity.txtprovider-xai/prompts/identity.txtprovider-zai/prompts/identity.txttech-specs/2026-06-agentic/harness.md
| async fn trigger_instance_count(iii: &IIIClient, trigger_type: &str) -> Option<u64> { | ||
| let response = iii | ||
| .trigger(TriggerRequest { | ||
| function_id: "engine::triggers::info".to_string(), | ||
| payload: json!({ "id": trigger_type }), | ||
| action: None, | ||
| timeout_ms: None, | ||
| }) | ||
| .await | ||
| .ok()?; | ||
| response.get("instance_count").and_then(Value::as_u64) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what timeout_ms: None means in the iii-sdk TriggerRequest
rg -n -C5 'timeout_ms' --type=rust approval-gate/ | head -60
# Check the engine::triggers::info response shape
rg -n -C5 'instance_count' --type=rust harness/ | head -40Repository: iii-hq/workers
Length of output: 4152
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the trigger_instance_count implementation and surrounding retry loop.
rg -n -C12 'trigger_instance_count|instance_count|re-bind|bind|retry' approval-gate/src/configuration.rs
# Find the TriggerRequest type definition and any docs/comments about timeout_ms semantics.
rg -n -C6 'struct TriggerRequest|timeout_ms|TriggerRequest' -g '*.rs' .
# Look for any iii-sdk error handling patterns around trigger() in the repo.
rg -n -C6 '\.trigger\(TriggerRequest' -g '*.rs' approval-gate/ | head -120Repository: iii-hq/workers
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact implementation and its caller loop in approval-gate.
sed -n '180,260p' approval-gate/src/configuration.rs
# Inspect the same helper pattern elsewhere for comparison.
sed -n '1,120p' provider-anthropic/src/state.rs
# Narrowly search for any local docs/comments about TriggerRequest timeout semantics.
rg -n -C3 'no timeout|timeout_ms: None|unresponsive|retry loop|instance_count' approval-gate session-manager storage provider-anthropic lspRepository: iii-hq/workers
Length of output: 15688
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find every reference to the engine::triggers::info probe and any typed response handling.
rg -n -C4 'engine::triggers::info|instance_count|triggers::info' -g '*.rs' .
# Inspect the surrounding helper code in approval-gate for intent/comments.
sed -n '160,220p' approval-gate/src/configuration.rs
# Check if similar best-effort probe helpers swallow errors the same way.
rg -n -C3 '\.ok\(\)\?\;|\.ok\(\)\?' approval-gate provider-anthropic session-manager lsp storageRepository: iii-hq/workers
Length of output: 18226
🌐 Web query:
iii-sdk TriggerRequest timeout_ms None semantics trigger() error handling
💡 Result:
In the iii-sdk, the TriggerRequest object allows you to configure invocation behavior, including a timeout_ms parameter (or timeoutMs in the Node.js/TypeScript SDK) [1][2]. When timeout_ms is set to None (or undefined in TypeScript), the SDK utilizes the default system or engine-defined invocation timeout [2]. Error Handling for trigger(): Failures that occur during a trigger() call—such as timeouts, function execution errors, or connection issues—are handled via the IIIInvocationError class [3][4]. When a trigger() operation fails, the SDK throws an instance of IIIInvocationError [5][3]. Key details regarding IIIInvocationError: - Structure: It includes a code (string), message (string), and optional fields for function_id and stacktrace [3][4]. - Common Error Codes: - TIMEOUT: Indicates a client-side timeout occurred [3][4]. - invocation_stopped: Indicates an engine-side timeout [3][4]. - invocation_failed: Indicates the target handler threw an error [3][4]. - function_not_found or function_not_invokable: Indicates issues locating or invoking the target function [3][4]. - FORBIDDEN: Indicates an RBAC (Role-Based Access Control) denial [3][4]. In practice, because trigger() returns a Promise, you should use standard async/await error handling patterns (try/catch blocks) to manage these failures [6][2][3]. For example: try { const result = await iii.trigger({ function_id: 'my-function', payload: { key: 'value' }, timeoutMs: 5000 // Optional override }); } catch (error) { if (error instanceof IIIInvocationError) { console.error(Error code: ${error.code}, Message: ${error.message}); } }
Citations:
- 1: https://docs.rs/iii-sdk/latest/iii_sdk/protocol/struct.TriggerRequest.html
- 2: https://iii.dev/docs/0-10-0/api-reference/sdk-node
- 3: https://motiadev.mintlify.app/sdk-reference/node-sdk
- 4: https://motiadev.mintlify.app/0-17-0/sdk-reference/node-sdk
- 5: https://iii.dev/docs/sdk-reference/node-sdk
- 6: https://iii.dev/docs/0-10-0/how-to/trigger-actions
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the engine::triggers::info override and any nearby docs/comments
# describing its response shape.
sed -n '520,550p' rbac-proxy/src/engine_overrides.rs
sed -n '1,120p' rbac-proxy/src/engine_overrides.rs
# Look for any tests asserting the response shape beyond instance_count.
rg -n -C3 'triggers_info_recomputes_instance_count_to_accessible|instance_count' rbac-proxy/src/engine_overrides.rsRepository: iii-hq/workers
Length of output: 6817
Avoid collapsing trigger probe failures into None (approval-gate/src/configuration.rs:205-215). timeout_ms: None uses the SDK/engine default timeout, so the call is still bounded. The .ok()? and as_u64() chain still turns transport, dispatch, or response-shape failures into “not ready,” which can leave the retry loop spinning forever without any signal if engine::triggers::info stops returning { "instance_count": <u64> }.
🤖 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 `@approval-gate/src/configuration.rs` around lines 205 - 216, Update
trigger_instance_count to distinguish trigger probe failures from a legitimate
zero or missing count: stop using .ok()? and the as_u64() fallback, propagate or
explicitly report trigger/response-shape errors, and ensure callers of
trigger_instance_count handle the failure rather than treating it as “not ready”
indefinitely.
| max_validation_retries: record.options.max_validation_retries, | ||
| transient_resumes: record.transient_resumes, | ||
| max_transient_resumes: record.options.max_transient_resumes, | ||
| partial_result_available: record.result.is_some(), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not derive partial_result_available from the generic result field.
finalize_completed stores every successful final output in record.result, so this is true for every completed turn—not only turns with preserved partial output. Gate it on the failure/recovery state or persist a dedicated partial-result marker; otherwise clients will mislabel normal results as partial.
🤖 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 `@harness/src/functions/status.rs` at line 80, Compute partial_result_available
from the failure/recovery state or a dedicated persisted partial-result marker
rather than record.result.is_some(); update the status construction near
finalize_completed handling so normal successful outputs are reported as
non-partial while recovered turns with preserved partial output remain true.
| `harness::react` treats these as success-path events by default: only | ||
| `status: "completed"` spawns the downstream sub-agent. Contract exhaustion is a failed turn with | ||
| its best-effort result preserved; failed or cancelled completions are recorded by joins but stop the normal downstream | ||
| spawn. An explicit error handler opts in with `metadata.continue_on_error: true`. Simple |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clarify that completed events with result_error do not continue the success path.
harness/src/functions/react.rs treats a completed event carrying result_error as a failure, so “only status: "completed" spawns” is too broad. Document this as “completed with no result_error” to keep the orchestration contract accurate.
🤖 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 `@tech-specs/2026-06-agentic/harness.md` around lines 629 - 632, Update the
harness::react orchestration description to state that only events with status
"completed" and no result_error spawn the downstream sub-agent; clarify that
completed events containing result_error follow the failure path. Preserve the
existing behavior for exhausted, failed, or cancelled completions and the
metadata.continue_on_error override.
#478) PermissionModePicker passes a `title` tooltip on its approval-mode options since #469, but SelectOption never gained the field, so `tsc -b` fails on main — and with it `pnpm build` and the console worker's build.rs web build on any clean checkout. Add `title?: string` to SelectOption and forward it to the option row as the native tooltip.
…477) The chat's thinking shimmer walked straight from submit to a generic "thinking…"/"dispatching <model>" line with no hint whether the message was accepted, queued, or waiting on the provider. Surface the real phases end to end: - console: translate the harness::send response (accepted/merged/queued) and harness::turn-started — both previously discarded — into a new turn-status stream event that drives the shimmer detail; server status_reason stays highest precedence. - session-manager: session::set-status stores status_reason on working (live phase) as well as error (failure cause) and re-emits status-changed when only the reason changes; no-op now requires status AND reason unchanged. Feature contract, golden schema, internals.md and tech-spec updated. - harness: turn_loop stamps "preparing context" at step start and "waiting for <model>" right before the router::chat RPC. Also restore the missing SelectOption.title field: PermissionModePicker passes title since #469 but the field never landed, so tsc -b (and the console worker build) fails on a clean checkout.
2 participants
Subagents that need approval now clearly pause and show what they are waiting for, even when the request comes from a child session after the parent finishes. Agent pipeline failures are also preserved and explained, so users can understand why work stopped instead of seeing an idle or misleadingly successful session.
Manual approval also remains active when workers connect in a different order. Approval requests will no longer be silently bypassed because the approval service started before the harness.
What changed
Technical details
Validation
Fixes MOT-3955
Summary by CodeRabbit
New Features
Improvements
Documentation