fix(bridge): mission_* tools accept name; resolves #2583 - #3155
Conversation
The bug bash report on #2583 ("Routine creation fails with '5 consecutive code errors'") attributed the failure to the budget cap rework in #2843. A live regression test (added here) reproduces the original prompt verbatim and shows the actual root cause is much smaller: every mission_* handler in the bridge required an `id` UUID, but the LLM and the routine_* alias path naturally pass `name`. Each call collapsed to "invalid mission id: invalid length 0", retried 4–5×, and tripped the orchestrator's `consecutive_action_errors >= max_consecutive_errors` guard. Fix: - New `resolve_mission_id` helper in `src/bridge/effect_adapter.rs` resolves a MissionId from `name` (preferred) → `id` UUID (legacy fallback) → positional arg, looking up names through `MissionManager::list_missions(project_id, user_id)`. UUIDs stay internal. - `mission_get`, `mission_fire`, `mission_pause`, `mission_resume`, `mission_complete`, `mission_update` migrated to use the helper. `mission_update` renames now go through `new_name`; the legacy `name`-as-rename-target behaviour is preserved only when a UUID `id` is also passed. - Schemas in `src/bridge/engine_actions.rs` advertise `name` as primary with `id` documented as a legacy alternative; neither is `required`. Live regression test (`tests/e2e_live_routine.rs`): - Drives the issue's exact prompt through engine v2. - Asserts no "invalid mission id" / "consecutive code errors" surfaces. - Verifies routine_fire delivers a `**[name]**` notification on both the initial fire and a second-turn re-fire. - Recorded trace committed for deterministic replay. Two related observations surfaced by the live test, parked as separate follow-ups (warnings only — not test-failing): - Administrative tools (mission_create / routine_create / mission_fire / routine_fire) do not surface `ApprovalNeeded` gates with `auto_approve_tools=false` even though they are in the `AUTONOMOUS_TOOL_DENYLIST`. - The mission-fire child thread sometimes returns work narration in FINAL() instead of the requested data point — a `build_meta_prompt` guidance issue independent of #2583.
github-actions
Bot
added
size: L
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a name-based resolution system for missions and routines, enabling identification via human-readable names in addition to UUIDs. A centralized resolve_mission_id helper is introduced and integrated into various mission actions, while action definitions are updated to prioritize name as the primary identifier. The PR also adds comprehensive regression tests and a live end-to-end test for routine workflows. Feedback indicates that the mission_update logic fails to preserve legacy behavior for renames when a UUID is provided, and notes a discrepancy between the implementation and documentation regarding the resolution priority of identifiers.
| // Renames use `new_name`. The legacy `name` field is | ||
| // now the lookup key (consumed by `resolve_mission_id`), | ||
| // not the rename target — see the schema in | ||
| // `engine_actions.rs` and the migration note in the | ||
| // helper docs above. | ||
| if let Some(new_name) = params.get("new_name").and_then(|v| v.as_str()) { | ||
| updates.name = Some(new_name.to_string()); | ||
| } |
There was a problem hiding this comment.
The implementation of mission_update does not preserve the legacy behavior where name acts as the rename target when a UUID id is also provided, despite the PR description claiming this preservation. The current code only checks new_name for renames, which breaks backward compatibility for existing callers or recorded traces using the (id=UUID, name=NewName) pattern.
// Renames use new_name. The legacy name field is
// now the lookup key (consumed by resolve_mission_id),
// not the rename target — unless a UUID id is also
// passed, preserving legacy behavior.
if let Some(new_name) = params.get("new_name").and_then(|v| v.as_str()) {
updates.name = Some(new_name.to_string());
} else if let Some(id_val) = params.get("id").and_then(|v| v.as_str()) {
if uuid::Uuid::parse_str(id_val).is_ok() {
if let Some(name) = params.get("name").and_then(|v| v.as_str()) {
updates.name = Some(name.to_string());
}
}
}References
- When intercepting and forwarding a tool call to a backend implementation, ensure all relevant parameters from the original call are passed through, including legacy fields required for backward compatibility.
| /// Resolution order (first match wins): | ||
| /// 1. `params.id` — if present and a valid UUID, use directly. | ||
| /// 2. `params.name` — look up `(project_id, user_id, name)` via | ||
| /// `MissionManager::list_missions`. The same identifier used in | ||
| /// `mission_create` is what we resolve back here. | ||
| /// 3. `params._args[0]` — positional fallback used by some Tier-0 | ||
| /// tool-call shapes; tried as UUID first, then as name. |
There was a problem hiding this comment.
The docstring's resolution order does not match the actual implementation. Specifically, params._args[0] as a valid UUID (checked at line 2200) has higher priority than params.name as a name (checked during the mission list iteration at line 2218), but the docstring lists it as a fallback (#3).
| /// Resolution order (first match wins): | |
| /// 1. `params.id` — if present and a valid UUID, use directly. | |
| /// 2. `params.name` — look up `(project_id, user_id, name)` via | |
| /// `MissionManager::list_missions`. The same identifier used in | |
| /// `mission_create` is what we resolve back here. | |
| /// 3. `params._args[0]` — positional fallback used by some Tier-0 | |
| /// tool-call shapes; tried as UUID first, then as name. | |
| /// Resolution order (first match wins): | |
| /// 1. params.id — if present and a valid UUID, use directly. | |
| /// 2. params._args[0] — if present and a valid UUID, use directly. | |
| /// 3. params.name — look up (project_id, user_id, name) via | |
| /// MissionManager::list_missions. | |
| /// 4. params.id — look up by name (legacy fallback). | |
| /// 5. params._args[0] — look up by name (positional fallback). |
References
- When resolving identifiers from stored tool data or parameters, implement and document a clear fallback chain to check multiple possible field names or positions.
There was a problem hiding this comment.
Fixed in b74c42e. The docstring on resolve_mission_id now matches the new four-step resolution order including the conflict guard. _args[0] no longer short-circuits ahead of explicit name (it's now a name candidate only — see serrrfirat's related comment).
There was a problem hiding this comment.
Pull request overview
This PR fixes routine/mission tool calls in the bridge by allowing mission_* handlers (and their routine_* aliases) to resolve missions by name instead of requiring a UUID id, preventing retry loops that previously tripped the orchestrator’s “consecutive code errors” guard (issue #2583).
Changes:
- Add
resolve_mission_idand migratemission_get/fire/pause/resume/complete/updateto acceptname(preferred) with legacyidfallback. - Update mission tool schemas to advertise
nameas primary and introducenew_namefor renames inmission_update. - Add a new live/replayable end-to-end regression test plus fixture, and extend the test rig with approval helpers.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/bridge/effect_adapter.rs |
Adds resolve_mission_id and switches mission handlers to name-first resolution. |
src/bridge/engine_actions.rs |
Updates mission tool schemas and guidance to prefer name and use new_name for rename. |
tests/support/test_rig.rs |
Adds channel_handle() and send_exec_approval() for background helpers in tests. |
tests/e2e_live_routine.rs |
Adds a live/replayable E2E regression test for #2583. |
tests/fixtures/llm_traces/live/routine_btc_create_test_and_refire.json |
Adds recorded trace fixture for deterministic replay. |
Comments suppressed due to low confidence (1)
src/bridge/effect_adapter.rs:1027
mission_completeresolves an explicit UUID viaresolve_mission_idand then callsMissionManager::complete_mission(id)without any ownership/shared-access check. Sincecomplete_missionitself does not enforce tenant isolation, a caller who knows/guesses another user's mission UUID could complete it. Add an authorization check here (e.g., load the mission and verifymission.is_owned_by(context.user_id)ormission.owner_id().is_shared()before completing), or change the engine API to requireuser_idlikefire_mission/pause_mission/resume_mission/update_missiondo.
"mission_complete" => {
let id =
resolve_mission_id(mgr.as_ref(), context.project_id, &context.user_id, params)
.await;
match id {
Ok(id) => match mgr.complete_mission(id).await {
Ok(()) => Ok(serde_json::json!({"status": "completed"})),
Err(e) => Err(e),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| &["id"], | ||
| &[], | ||
| &[ | ||
| "Identify the mission to update with `name` (preferred) or `id` — exactly one is required.", |
There was a problem hiding this comment.
Fixed in 720a0db. Discovery summary now matches the actual handler tolerance: "Identify the mission with name (preferred) or id (UUID). If both are provided they must identify the same mission, or use the legacy {id, name} rename shape (where name is the new name) — otherwise the resolver errors with 'identify different missions'."
| self.handle.abort(); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Fixed in b74c42e. ApprovalAutoResponder now implements Drop that aborts the handle. shutdown() is kept for explicitness; abort() is idempotent so the double-abort path is safe.
| // We resolved the id via name lookup, so this | ||
| // should be unreachable in practice (the lookup | ||
| // already proved the mission exists). Keep the | ||
| // arm so the match is exhaustive. |
There was a problem hiding this comment.
Fixed in b74c42e. The comment now correctly notes that Ok(None) is reachable when the caller passed an explicit UUID for a deleted/unknown mission — names that didn't match are caught earlier by resolve_mission_id with a more useful error.
|
|
||
| Err(EngineError::Effect { | ||
| reason: format!( | ||
| "mission not found by name: tried {name_candidates:?} for user '{user_id}'. \ |
There was a problem hiding this comment.
Fixed in b74c42e. user_id is no longer included in the mission not found error. Same fix on mission_get's "belongs to another user" arm — the foreign mission's UUID is now scrubbed from the LLM-visible reason and preserved only via tracing::debug!.
| let mut updates = ironclaw_engine::MissionUpdate::default(); | ||
| if let Some(name) = params.get("name").and_then(|v| v.as_str()) { | ||
| updates.name = Some(name.to_string()); | ||
| // Renames use `new_name`. The legacy `name` field is | ||
| // now the lookup key (consumed by `resolve_mission_id`), | ||
| // not the rename target — see the schema in | ||
| // `engine_actions.rs` and the migration note in the | ||
| // helper docs above. | ||
| if let Some(new_name) = params.get("new_name").and_then(|v| v.as_str()) { | ||
| updates.name = Some(new_name.to_string()); | ||
| } |
There was a problem hiding this comment.
Fixed in 0bd586d (initial preservation) + b74c42e (mission_update handler now strips name from the resolver-view params when the legacy shape is detected, so the new conflict guard doesn't reject it). Two regression tests pin it: mission_update_preserves_legacy_id_plus_name_rename and mission_update_renames_via_new_name_and_uses_name_as_lookup.
|
|
||
| // De-duplicate while preserving order. | ||
| let mut seen: std::collections::HashSet<&str> = std::collections::HashSet::new(); | ||
| name_candidates.retain(|s| seen.insert(*s)); |
There was a problem hiding this comment.
Performance regression on hot path (High): every name-keyed mission_* call now loads the user's full mission list. A cron routine firing every 5 min on an account with hundreds of historical missions (including completed/archived) pays an O(N) DB read per fire — the previous UUID-only path was O(1).
Suggest adding MissionManager::find_by_name(project_id, user_id, name) so the bridge does a targeted lookup. Even a small per-(project_id, user_id) name→id cache inside MissionManager (invalidated on create/rename/complete) would pay for itself fast — the same mission name is hit on every fire/get/pause cycle.
There was a problem hiding this comment.
Addressed in b74c42e. Added MissionManager::find_by_name(project_id, user_id, name) -> Result<Option<Mission>, EngineError> typed API; bridge resolver switched off list_missions onto it. Today find_by_name still scans the user's missions in-memory; the doc-comment names the future store-level index drop-in (Store::find_mission_by_name) explicitly. Adding it later is non-breaking — every caller goes through this method now, so the optimization plugs in without churn at the call sites.
| // Renames use `new_name`. The legacy `name` field is | ||
| // now the lookup key (consumed by `resolve_mission_id`), | ||
| // not the rename target — see the schema in | ||
| // `engine_actions.rs` and the migration note in the |
There was a problem hiding this comment.
Foot-gun (High): if a caller passes mission_update({id: <real_uuid_A>, name: "mission-B", new_name: "renamed"}), resolve_mission_id resolves id first (UUID fast-path), so mission A is renamed even though name says "mission-B". The schema text in engine_actions.rs says name is the lookup key — the helper logic contradicts that whenever id is also present.
Defense-in-depth: when both id (UUID) and name are provided and resolve to different missions, error out with "both id and name provided but they identify different missions" instead of silently preferring one. The doc comment on resolve_mission_id should also call out this precedence trap explicitly so the next caller doesn't trip it.
There was a problem hiding this comment.
Fixed in b74c42e. Added the conflict guard you suggested. New regression test mission_resolver_errors_when_id_and_name_disagree pins it. Error reads: "both id and name provided but they identify different missions: id=X vs name=[Y] → Z. Provide only one — or fix the name to match the id." The resolver docstring also explicitly calls out the conflict-guard contract.
| /// cause of the bug bash failure (not the budget-rework hypothesis in | ||
| /// the linked #2843). | ||
| #[tokio::test] | ||
| async fn mission_fire_resolves_by_name_when_id_absent() { |
There was a problem hiding this comment.
Test coverage gap (High): all six mission_* handlers (get, fire, pause, resume, complete, update) now route through resolve_mission_id, but only mission_fire got new by-name regression tests. The most foot-gun-prone handler — mission_update (see related comment on the new_name rename) — has no by-name test at all, and the existing mission_update_string_guardrails_rejected_via_execute_action still passes id directly so doesn't exercise the new path.
Per CLAUDE.md "Test Through the Caller, Not Just the Helper", each migrated handler needs a caller-level test. Suggest three more symmetric to this one: mission_get_resolves_by_name, mission_complete_resolves_by_name, and mission_update_renames_via_new_name_when_name_is_lookup_key (the last one is the regression guard for the id+name foot-gun).
There was a problem hiding this comment.
Fixed in b74c42e. Added by-name caller-level tests for every migrated handler: mission_get_resolves_by_name, mission_complete_resolves_by_name, mission_pause_and_resume_resolve_by_name, plus the conflict-guard regression mission_resolver_errors_when_id_and_name_disagree and the legacy-rename regression mission_update_preserves_legacy_id_plus_name_rename.
| .and_then(|a| a.get(0)) | ||
| .and_then(|v| v.as_str()) | ||
| .filter(|s| !s.is_empty()) | ||
| { |
There was a problem hiding this comment.
Edge case (Medium): if a caller passes {name: "actual-mission", _args: ["<uuid-of-other>"]}, this _args[0] UUID parse short-circuits with return Ok(MissionId(uuid)) — the explicit name field is silently ignored. This contradicts the docstring's stated principle that name is preferred and _args[0] is a "positional fallback used by some Tier-0 tool-call shapes".
Two ways out:
- (a) Preserve preference: move the
_args[0]UUID parse to aftername_candidateslookup falls through, so explicitnamewins. - (b) Treat positional as always-a-name: drop the
_args[0]UUID-parse branch and only push toname_candidates. UUID-shaped strings would still match if any mission happens to be named with a UUID — vanishingly rare and an explicit caller decision.
Either is fine; the current ordering is the worst of both.
There was a problem hiding this comment.
Fixed in b74c42e — went with option (b). _args[0] is now treated as a name candidate only, never honoured as a UUID. UUID-shaped strings would only match if a mission happens to be named with one (vanishingly rare and the explicit caller decision you noted).
| } | ||
| } | ||
|
|
||
| Err(EngineError::Effect { |
There was a problem hiding this comment.
Information disclosure (Medium): echoing user_id into the error reason leaks server-side identity into the LLM context, conversation history, and any tool-error telemetry. The previous error ("mission not found: {id_str}") only echoed user input.
EngineError::Effect::reason flows back to the LLM and is persisted — depending on the channel, user_id is a UUID (low value), but for some flows it's an external identity (chat id, session-derived token). Suggest dropping the for user '{user_id}' segment entirely. The LLM can't act on it anyway (it can't list a different user's missions).
Same goes for the mission {} belongs to another user reason a few lines above (mission_get arm at the new resolve site) — there's no LLM-actionable info in the foreign mission's UUID, and emitting it confirms the existence of a mission the caller has no claim to.
There was a problem hiding this comment.
Fixed in b74c42e. Both error reasons are scrubbed: user_id is gone from the "mission not found" error, and mission_get's "belongs to another user" reason no longer echoes the foreign mission's UUID (preserved only via tracing::debug!).
| /// tool-call shapes; tried as UUID first, then as name. | ||
| /// | ||
| /// A non-UUID `id` value is treated as a name (some callers conflate the | ||
| /// two), which keeps backward compat for `mission_complete`'s historical |
There was a problem hiding this comment.
Architecture (Medium): name-or-id resolution is conceptually a MissionManager concern, not a bridge-adapter concern. Keeping this 60-line policy here means any future caller — CLI mission commands, the WS handler, a WASM channel that wants a mission lookup, integration tests outside the bridge — duplicates the resolution rules.
Suggest moving to a typed API on MissionManager:
enum MissionRef<'a> {
Id(MissionId),
Name(&'a str),
}
impl MissionManager {
pub async fn resolve(&self, project_id: ProjectId, user_id: &str, r: MissionRef<'_>)
-> Result<MissionId, EngineError> { ... }
}The bridge then becomes a one-liner that picks the right variant from params. The two ambiguity rules (id+name simultaneous; _args[0]) collapse into one well-scoped place that has a unit test suite of its own.
There was a problem hiding this comment.
Partially addressed in b74c42e. The typed engine API (MissionManager::find_by_name) is in place — that's the part future callers (CLI, WS, WASM channels, integration tests) will actually need. The 60-line param-parsing in the bridge stayed put because it deals in JSON and Tier-0 calling conventions (_args[0] etc.) that aren't a MissionManager concern. Happy to lift the param-shape policy onto a MissionRef enum + MissionManager::resolve in a follow-up if you'd prefer the typed-API form — should I file that?
| "HIT" | ||
| ], | ||
| [ | ||
| "cf-ray", |
There was a problem hiding this comment.
Trace fixture noise (Medium): the recorded CoinGecko response carries every header from the real Cloudflare/Coingecko response — cf-ray, x-request-id, etag, age, date, cf-cache-status, strict-transport-security, content-security-policy-report-only, etc. They aren't secrets, but they:
- bloat the diff (~941 lines for one e2e test, most of it duplicated headers across three identical price requests),
- pin the fixture to one Cloudflare datacenter (
-FUK) and a 2026-05-01 timestamp, - risk replay surprises if any HTTP cache layer in the stack decides to honor
etag/age.
Suggest a fixture-redaction helper in tests/support/ (e.g. redact_response_headers(allow=["content-type", "content-length"])) applied at fixture-write time. Net result is a smaller, more reproducible fixture and one less drift source for replay-mode tests.
There was a problem hiding this comment.
Deferred — agreed on the diagnosis. The fixture is committed to enable replay-mode CI runs, and a redaction helper that's careful to keep replay deterministic is its own small piece of work. Will file a follow-up issue today if you want; otherwise happy to tackle in a tiny PR right after this one merges.
…alf 1) Issue #3133 reported a Gmail-sending mission firing every 3 minutes with the FINAL() body "Failed to send email. Status: None Error: None / Next focus: Debug Gmail authentication." The new live test under #2583's infrastructure reproduced the actual mechanism: a mission's child thread hits a `tool_activate(gmail)` OAuth gate, the engine emits `ThreadOutcome::GatePaused`, and `process_mission_outcome_and_notify` — which only matched `Completed` / `Failed` / `MaxIterations` — silently swallowed it via a bare `_ => {}` arm. The cron scheduler kept ticking, the user got no auth-tray prompt, and the LLM eventually narrated the two-None pattern when its `http` fallback failed. Engine side (`crates/ironclaw_engine/src/runtime/mission.rs`): - New `MissionGateInfo { gate_name, action_name, parameters, request_id, resume_kind }` struct, exported from the engine crate. - `MissionNotification` gains an `Option<MissionGateInfo> gate` field. - New `ThreadOutcome::GatePaused` arm in `process_mission_outcome_and_notify` that: * Transitions the mission to `MissionStatus::Paused`. The cron tick path filters by `Active` (lines 1058 / 1131 / 1194 / 1781), so pausing is enough to stop the ghost re-firing pattern. * Records `PAUSED: gate '...' on action '...' awaiting <kind>` in `approach_history`. * Emits a `MissionNotification` with `gate: Some(...)` and a user-facing nudge tailored per `ResumeKind` (Authentication / Approval / External). - Regression test `gate_paused_pauses_mission_and_emits_gate_notification` pinning the contract. Bridge side (`src/bridge/router.rs::handle_mission_notification`): - New optional args (`auth_manager`, `tools`, `extension_manager`) plumbed through the mission-notification dispatcher loop. - When `notif.gate` is `Some`, additionally emit a structured `StatusUpdate` per `ResumeKind`: * `Authentication` → `StatusUpdate::AuthRequired { extension_name, instructions, auth_url, setup_url, request_id }`. Extension identity is resolved via the canonical `resolve_extension_for_action` helper (the same one foreground auth gates use), so the gateway UI's auth tray fires on the same path. * `Approval` → `StatusUpdate::ApprovalNeeded { request_id, tool_name, description, parameters, allow_always }`. * `External` → no user-actionable surface; falls through to the text response only. - The friendly text response is still broadcast on `notify_channels` alongside the structured status update, so the user sees both an auth-tray entry and a chat message. This is half 1 of the fix discussed in #3133. Half 2 (auto-resuming a paused mission once auth completes) is a separate follow-up: the mission stays Paused after the user resolves the gate today; they have to explicitly call `mission_resume`. Test infrastructure: - New `tests/e2e_live_mission_gmail.rs` driving the issue's flow with Gmail OAuth seeded from the source DB. Asserts no "Status: None Error: None" pattern and no "consecutive code errors" surface. - Shared mission/routine test helpers extracted to `tests/support/live_mission_helpers.rs` (ApprovalAutoResponder, looks_like_routine_notification, tool_is, wait_for_response_matching) so #2583's routine test and #3133's mission test reuse the same bug-pattern detectors without copy-paste drift.
github-actions
Bot
added
scope: channel/web
|
Half 2 follow-up filed as #3166: 'Mission auto-resume after auth/approval gate resolution'. That issue covers persisting |
Addresses the high/medium-severity findings from the PR #3155 review (serrrfirat, Gemini, Copilot). No behaviour change for the happy path the original PR established (name-keyed mission_* dispatch keeps working); the conflict and information-disclosure paths get safer. Engine (`crates/ironclaw_engine/src/runtime/mission.rs`): - New `MissionManager::find_by_name(project_id, user_id, name)` typed API. Today it's a thin wrapper over `list_missions_with_shared` plus an in-memory filter, but every name-keyed call goes through it so a future store-level index drops in non-breakingly. The doc-comment names the optimisation (Store::find_mission_by_name) and the perf concern from the review explicitly. - Two new unit tests: round-trip via create+find, and the per-user scope (mission for bob is invisible when alice queries). Bridge (`src/bridge/effect_adapter.rs::resolve_mission_id`): - Switched off `list_missions` and onto `mgr.find_by_name`, so the hot path stops doing an O(N) scan over the user's full mission set per fire. - Added the id+name conflict guard. If both are supplied AND the name resolves to a different mission than the id identifies, the resolver now errors loudly: "both id and name provided but they identify different missions: id=X vs name=[Y] → Z. Provide only one — or fix the name to match the id." Previously the UUID would silently win, which is the foot-gun serrrfirat flagged: a typoed `name` would rename the wrong mission. New regression test `mission_resolver_errors_when_id_and_name_disagree` pins it. - Inverted the `_args[0]` precedence: a positional arg that *parses as a UUID* used to short-circuit ahead of `params.name`, which contradicted the docstring's "explicit name preferred". Now `_args[0]` is treated as a name candidate only; positional UUIDs aren't honoured at all (rare in practice — the Tier-0 callers that use `_args[0]` overwhelmingly pass names). - Dropped `user_id` from the "mission not found" error string. The reason field flows to the LLM context and conversation history; echoing the server-side identity served no purpose. Same fix on the `mission_get` "belongs to another user" arm — the foreign mission's UUID is now scrubbed from the LLM-visible reason and preserved only via tracing::debug!. - Updated the stale "unreachable Ok(None)" comment in mission_get per Copilot — the arm IS reachable when the caller passed an explicit UUID for a deleted/unknown mission. Bridge (`mission_update` handler): - The legacy `{id, name=NEWNAME}` rename shape (preserved by the earlier review-fix commit on this branch) was tripping the new conflict guard because the resolver saw both `id` and `name` as identifiers. The handler now strips `name` from the resolver-view params *only* when the legacy shape is detected (id is a valid UUID, `new_name` absent), so the resolver never sees a would-be-rename `name` as a lookup target. Both the legacy and canonical (`new_name`) paths keep their existing tests. By-name regression tests (`src/bridge/effect_adapter.rs`): - Closes the test gap flagged by serrrfirat — `mission_fire` was the only handler with a by-name caller-level test before. Adds: - `mission_get_resolves_by_name` - `mission_complete_resolves_by_name` - `mission_pause_and_resume_resolve_by_name` - `mission_resolver_errors_when_id_and_name_disagree` (the new conflict guard above) - All migrated handlers now have a caller-level test that exercises the name path through `execute_action`. Test infra (`tests/support/live_mission_helpers.rs`): - `ApprovalAutoResponder` now implements `Drop` (per Copilot). A panicking test would otherwise leak the background polling task across test boundaries. `shutdown()` is kept for explicitness; abort() is idempotent.
commented
May 1, 2026
|
Pushed b74c42e addressing the review feedback. Summary by reviewer: serrrfirat:
Gemini:
Copilot:
Deferred:
Test results on b74c42e: 448 bridge tests + 89 engine mission tests, all green. fmt + clippy -D warnings clean. |
left a comment
There was a problem hiding this comment.
Pull request overview
This PR updates the engine-v2 bridge so mission_* (and routine_* aliases) can target missions by name (preferred) with UUID id as a legacy fallback, addressing the retry loop behind issue #2583. It also adds mission gate metadata/notification surfacing and introduces live/replay E2E regression coverage.
Changes:
- Add name→id resolution (
resolve_mission_id) and migratemission_get/fire/pause/resume/complete/updatehandlers to acceptname(with legacyidfallback). - Update mission tool schemas to advertise
nameas primary and introducenew_nameformission_updaterenames. - Add live/replay E2E tests + shared helpers and fixtures; extend mission notifications with gate metadata and bridge-side status emission.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/bridge/effect_adapter.rs |
Adds resolve_mission_id and migrates mission handlers to name-first resolution; adds unit tests around the new behavior. |
src/bridge/engine_actions.rs |
Updates tool schemas/descriptions to prefer name, keeps id as optional legacy, adds new_name for renames. |
src/bridge/router.rs |
Extends mission-notification handling to surface gate state via StatusUpdate::{AuthRequired,ApprovalNeeded}. |
crates/ironclaw_engine/src/runtime/mission.rs |
Adds MissionGateInfo and emits it on ThreadOutcome::GatePaused; pauses missions on gate outcomes. |
crates/ironclaw_engine/src/lib.rs |
Re-exports MissionGateInfo. |
src/channels/web/tests/no_silent_drop.rs |
Adapts test notifications to include the new gate field and updated handler signature. |
tests/support/test_rig.rs |
Adds channel_handle() and a helper to send structured exec approvals. |
tests/support/mod.rs |
Registers shared live-mission helper module behind libsql. |
tests/support/live_mission_helpers.rs |
Adds shared live-test helpers (notification heuristic, approval auto-responder, waiters). |
tests/e2e_live_routine.rs |
Adds live/replay E2E regression test for #2583 prompt flow and notification assertions. |
tests/e2e_live_mission_gmail.rs |
Adds live/replay E2E test scenario around Gmail mission behavior and gate-related regression markers. |
tests/fixtures/llm_traces/live/routine_btc_create_test_and_refire.json |
Adds recorded trace fixture for deterministic replay of the live routine test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if let Some(gate) = ¬if.gate | ||
| && let Some(tools) = tools | ||
| { | ||
| for channel_name in ¬if.notify_channels { | ||
| let metadata = serde_json::json!({"user_id": notif.user_id}); | ||
| let status = match &gate.resume_kind { | ||
| ironclaw_engine::ResumeKind::Authentication { | ||
| credential_name, | ||
| instructions, | ||
| auth_url, | ||
| } => { | ||
| let extension_name = resolve_extension_for_action( | ||
| auth_manager, | ||
| extension_manager, | ||
| tools, | ||
| &gate.action_name, | ||
| &gate.parameters, | ||
| credential_name.as_str(), | ||
| ¬if.user_id, | ||
| ) | ||
| .await; | ||
| StatusUpdate::AuthRequired { | ||
| extension_name, | ||
| instructions: Some(instructions.clone()), | ||
| auth_url: auth_url.clone(), | ||
| setup_url: None, | ||
| request_id: Some(gate.request_id.clone()), | ||
| } | ||
| } | ||
| ironclaw_engine::ResumeKind::Approval { allow_always } => { | ||
| StatusUpdate::ApprovalNeeded { | ||
| request_id: gate.request_id.clone(), | ||
| tool_name: gate.action_name.clone(), | ||
| description: format!( | ||
| "Mission '{}' is waiting for approval to run '{}'.", | ||
| notif.mission_name, gate.action_name | ||
| ), | ||
| parameters: gate.parameters.clone(), | ||
| allow_always: *allow_always, | ||
| } | ||
| } |
There was a problem hiding this comment.
Fixed in 720a0db. MissionGateInfo.request_id renamed to call_id (matches what it actually carries — the engine LLM tool-call id). New MissionGateInfo.gate_request_id: Uuid is freshly generated at gate-emit time, and the bridge now forwards it (not call_id) into StatusUpdate::AuthRequired.request_id / ApprovalNeeded.request_id. The gateway resolve POST (features/chat/mod.rs:183/274/303 + platform/ws.rs:236) now sees a UUID-shaped string and won't 400. Mapping the gateway-issued UUID back to the engine call_id for actual auto-resume is half-2 (#3166).
| /// Engine-side `call_id` used to resume. | ||
| pub request_id: String, |
Addresses Copilot's review on PR #3155 commit b74c42e: > StatusUpdate::ApprovalNeeded.request_id is later parsed as a UUID by > the web approval endpoints / WS handler (they reject non-UUID > request_id). Here the mission gate wiring forwards gate.request_id, > but that value originates from ThreadOutcome::GatePaused.call_id > (LLM tool-call id like call_...), so approvals from mission/routine > notifications will be impossible to resolve via the gateway UI. Confirmed by reading the gateway resolve handler at `src/channels/web/features/chat/mod.rs:183`, `:274`, `:303` and the WS handler at `src/channels/web/platform/ws.rs:236` — all four call `Uuid::parse_str(&req.request_id)` and reject non-UUIDs with 400 Bad Request. Mission auth-tray entries built off the LLM `call_id` would silently 400 every resolve POST, making the tray decorative. Engine (`crates/ironclaw_engine/src/runtime/mission.rs`): - Renamed `MissionGateInfo.request_id` → `MissionGateInfo.call_id` (its actual identity — engine LLM tool-call id) so the field name matches what it carries. - Added `MissionGateInfo.gate_request_id: Uuid` (a fresh UUID generated at gate-emit time). Doc-comment explicitly contrasts the two fields and points at #3166 for the half-2 wiring that maps back from `gate_request_id` to `call_id` for engine resume. - Updated `gate_paused_pauses_mission_and_emits_gate_notification` to assert both fields are populated and that `gate_request_id` is a valid (non-nil) UUID. Bridge (`src/bridge/router.rs::handle_mission_notification`): - AuthRequired and ApprovalNeeded translations now forward `gate.gate_request_id.to_string()` into `StatusUpdate.request_id`, not `gate.call_id`. Inline comment names the gateway/WS handler files that parse it as UUID so the next maintainer doesn't re-introduce the bug. Schema (`src/bridge/engine_actions.rs::mission_update`): - Discovery summary previously said "exactly one is required" for name/id, but the handler tolerates both (with the conflict guard added on b74c42e + the legacy `{id, name}` rename shape). Wording now matches actual behaviour and explicitly calls out the conflict-guard outcome and the legacy rename shape so a tool-using model knows what to expect.
commented
May 1, 2026
|
Pushed 720a0db closing the last review-feedback items. Full status: Just landed (720a0db):
Already landed earlier on this branch:
Deferred (each individually replied on the inline thread):
Test results on 720a0db: 19/19 mission tests in |
Summary
5 consecutive code errors. The actual root cause was much smaller than the linked Cost-based budgeting: replace iteration/time caps with USD budgets cascading user → project → mission → thread #2843 epic suggested: everymission_*handler in the bridge required anidUUID, but the LLM and theroutine_*alias path naturally passname. Each call collapsed toinvalid mission id: invalid length 0, retried 4–5×, and tripped the orchestrator's consecutive-error guard.name(preferred) withidUUID retained as a legacy fallback; UUIDs stay internal.What changed
src/bridge/effect_adapter.rsresolve_mission_idhelper. Handlersmission_get,mission_fire,mission_pause,mission_resume,mission_complete,mission_updatemigrated.mission_updaterenames vianew_name; legacyname-as-rename-target preserved only when a UUIDidis also passed. Four new unit tests.src/bridge/engine_actions.rsnameas primary,idas legacy alternative. Neither field isrequired.mission_updatedocumentsnew_namefor renames.tests/support/test_rig.rschannel_handle()accessor andsend_exec_approvalfor the live test's approval responder.tests/e2e_live_routine.rsinvalid mission idorconsecutive code errors; verifies**[name]**notifications on both fire and refire.tests/fixtures/llm_traces/live/routine_btc_create_test_and_refire.jsonHow the fix was found
Driving the issue's verbatim prompt through engine v2 with a real LLM (the new test) reproduced the failure on first run. The trace showed:
routine_firetranslates tomission_fireviaroutine_to_mission_alias, which passesparams.clone()unchanged. Themission_firehandler then tried to parse the routine name as a UUID and failed. Five errors in a row → orchestrator killed the thread → "5 consecutive code errors" symptom.Verification
cargo test --features libsql --lib bridge::effect_adapter— all 113 effect-adapter tests pass, including 4 new ones for the helper / handler / alias paths.cargo test --features libsql --test e2e_live_routine -- --skip routine_btc— heuristic + structural unit tests pass.IRONCLAW_LIVE_TEST=1 cargo test --features libsql --test e2e_live_routine -- --ignored routine_btc_create_test_and_refire --nocapture— passes in 49s. Tools observed:[\"routine_create(*/5 * * * *)\", \"routine_fire(bitcoin-price-check)\", \"routine_history(bitcoin-price-check)\", \"job_status(...)\"]. Noinvalid mission id. Noconsecutive code errors. Notifications delivered on both the initial fire and the second-turn refire.IRONCLAW_LIVE_TEST) loads the committed fixture and runs deterministically.Out of scope (parked for separate follow-ups)
The live test surfaced two further observations that are independent of this fix and not test-failing:
auto_approve_tools=false,mission_create / routine_create / mission_fire / routine_fire(all classifiedAdministrativepercrates/ironclaw_engine/src/gate/tool_tier.rs) ran without firing a singleStatusUpdate::ApprovalNeeded. Logged as a warning in the test.build_meta_prompt(crates/ironclaw_engine/src/runtime/mission.rs:2132–2139) instructs the child to FINAL() with "1. What you accomplished / 2. Next focus / 3. Goal achieved" — meta-prose. For "fetch X and surface it" routines the user actually wants the data. Logged as a warning in the test.Both warrant their own issues; flagging here so they're not lost.
Test plan