fix(slack): single-flight gate delivery per run_id (resolution-ack fanout) - #4843
Merged
Merged
Conversation
…er run Each gate-resolution ack (ApprovalResolution(Allow) / AuthResolution(Allowed)) carries the same submitted_run_id as the original user-message ack because it resumes the pre-existing run. should_deliver_after_ack returned true for Allow/Allowed, so every resolution ack spawned a fresh deliver_final_reply loop (L2) with delivered_blocked_marker = None. The original loop (L1) was still alive polling the same run_id; once the resolution made the run terminal, both L1 and L2 called post_slack_message for the final reply. N resolutions produced N+1 concurrent loops and gate N was posted N times. Fix: add active_delivery_run_ids (Mutex<HashSet<TurnRunId>>) on SlackFinalReplyDeliveryObserver. Before calling deliver_final_reply, observe_workflow_ack inserts the run_id and returns early if it was already present. The guard is released after deliver_final_reply exits so a future retry is never permanently blocked. Option (a) over (b): the single-flight guard is robust to future ack variants that target an existing run without requiring changes to should_deliver_after_ack, and makes the invariant visible at the dispatch site rather than as a filter predicate. Regression: gate_prompt_is_posted_exactly_once_when_approval_ack_races_ live_delivery_loop confirms exactly 2 messages (prompt + final reply). Pre-fix count: 3 (prompt + 2 duplicate final replies). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tion fanout The single-flight delivery guard (one live loop per run_id) was added to stop N gate resolutions spawning N+1 loops that each re-post the gate. Two regression gaps remained: - No unit test pinned the TOCTOU ordering — that the guard is checked and the run_id inserted BEFORE the delivery semaphore permit is acquired. `concurrent_ack_for_same_run_id_is_rejected_before_acquiring_permit` drives a blocked first delivery holding the only permit (max_concurrent_deliveries = 1) and asserts a second ack for the same run_id returns promptly via the guard skip instead of blocking on the semaphore. If the guard were checked after permit acquisition the second call would block and the test's timeout would elapse. - The original PR comment named `AuthResolution(Allowed)` as an equal trigger to `ApprovalResolution(Allow)`, but only the approval path was covered end-to-end. `auth_prompt_is_posted_exactly_once_when_auth_resolution_ack_races_live_delivery_loop` drives the full Slack ingress → blocked-auth turn → live delivery loop, injects an `AuthResolution(CallbackCompleted)` ack carrying the original submitted_run_id (the WebUI gate-resolve path, which has no Slack text syntax), and asserts exactly one auth prompt + one final reply — no duplicate post. No production change; both tests pass against the in-place guard fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR closes a TOCTOU window in the Slack delivery observer by enforcing single-flight delivery per run_id using an RAII guard checked before semaphore acquisition. It adds parametrized max_wait to e2e harness and verifies with gate and auth fanout regression tests. ChangesSingle-Flight Delivery Guard & Fanout Regression Tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
github-actions
Bot
added
size: L
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a single-flight guard using a Mutex-protected HashSet and an RAII guard to prevent duplicate delivery loops for the same run ID, resolving a race condition where gate-resolution acks could trigger redundant posts. The feedback suggests simplifying the manual check-and-insert logic on the HashSet to a single, idiomatic insert call to avoid a redundant double-lookup.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+1113
to
+1124
| let already_delivering = { | ||
| let mut guard = self | ||
| .active_delivery_run_ids | ||
| .lock() | ||
| .unwrap_or_else(|e| e.into_inner()); | ||
| if guard.contains(&run_id) { | ||
| true | ||
| } else { | ||
| guard.insert(run_id); | ||
| false | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
The manual check-and-insert pattern on the HashSet can be simplified to a single insert call. HashSet::insert returns true if the element was not present (newly inserted) and false if it was already present. Using this directly is more idiomatic in Rust and avoids a redundant double-lookup (hashing and searching twice) on the set under the mutex lock.
let already_delivering = !self
.active_delivery_run_ids
.lock()
.unwrap_or_else(|e| e.into_inner())
.insert(run_id);![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 235cb71c18
ℹ️ 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".
Comment on lines
+1125
to
+1131
| if already_delivering { | ||
| tracing::debug!( | ||
| target = "ironclaw::reborn::slack_delivery", | ||
| %run_id, | ||
| "skipping redundant delivery loop: a loop is already watching this run" | ||
| ); | ||
| return; |
There was a problem hiding this comment.
When a Slack user replies auth deny ... while the original delivery loop is still active after posting the auth prompt, the workflow accepts an AuthResolution::Denied ack with the same submitted_run_id. This early return treats that ack as redundant and skips deliver_final_reply, so the is_accepted_auth_denial() path that posts SLACK_AUTH_CANCELED_MESSAGE never runs; the original loop only observes the run becoming Cancelled and returns without any notification. Please special-case accepted auth denials before this guard, or allow them through even when the run_id is already active.
Useful? React with 👍 / 👎.
This was referenced Jun 14, 2026
henrypark133
added a commit
that referenced
this pull request
Jun 14, 2026
`slack_approval_then_auth_resume_completes_without_second_approval` was added to this branch but never passed — it fails at the commit that introduced it (9a7fc2b), so it slid in without the composition suite being run. It does not exercise Fix A's lease/identity changes: it fakes auth completion by advancing the recording coordinator directly, so it depends on the Slack delivery loop surviving the approval→auth gate transition with no re-trigger event. That delivery-loop lifecycle is owned by the single-flight delivery work (#4843), not by the capability-host invocation-identity fix. Fix A's real contract is covered by the green ironclaw_capabilities / ironclaw_host_runtime auth-resume contract tests. Remove the test and its now-unused harness scaffolding (the `BlockApprovalThenAuth` TurnMode variant, its match arm, and the `transition_blocked_approval_to_blocked_auth` helper) so #4839 is green. The approval→auth→final-reply delivery scenario is re-homed onto the delivery PR per the tracking issue. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
henrypark133
commented
Jun 14, 2026
henrypark133
left a comment
henrypark133
left a comment
Collaborator
Author
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Prevent duplicate Slack gate/final reply delivery by enforcing one active delivery loop per run_id across resolution acknowledgements.
Stats: 4 findings (from 5 raw, 4 after validation/dedup) across 2 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0
tests
- Low Panic-safe guard release is not covered (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:1110-1111, confidence 75) — anchor: crates/ironclaw_reborn_composition/src/slack_delivery.rs:1110
The new guard is explicitly documented as preventing a permanent delivery block ifdeliver_final_replypanics, but the added tests cover timeout/error returns rather than the panic/unwinding path.
conventions
- Low Auth fanout harness duplicates the existing builder (
crates/ironclaw_reborn_composition/src/slack_serve/e2e_tests.rs:2196-2318, confidence 75) — anchor: .claude/rules/architecture.md:133
The new helper reimplements the same Slack harness composition owned bybuild_harness_with_full_settingsatcrates/ironclaw_reborn_composition/src/slack_serve/e2e_tests.rs:240, adding substantial growth to an already 2,424-line test file. The architecture rule says existing 1,500-3,000 line files should leave shorter unless the feature has nowhere else to live; this can live in the shared builder by returning or optionally exposing the observer instead of duplicating the whole setup.
local-patterns
- Low Comments name non-existent resolution variants (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:144, confidence 75) — anchor: crates/ironclaw_product_adapters/src/inbound.rs:230 and crates/ironclaw_product_adapters/src/inbound.rs:333
The new production comment describesApprovalResolution(Allow)/AuthResolution(Allowed), but the local adapter types useApprovalDecision::{ApproveOnce, AlwaysAllow}andAuthResolutionResult::{CredentialProvided, CallbackCompleted, Denied}. The same inventedAuthResolution(Allowed)name is repeated in the new e2e comments, making grep/navigation for the actual auth-resolution path misleading.
maintainability
- Low Single-flight admission is split from delivery eligibility (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:1112-1139, confidence 75) — anchor: crates/ironclaw_reborn_composition/src/slack_delivery.rs:1112
The new guard decides whether an ack may occupy the per-run single-flight slot by checking onlysubmitted_run_id, thendeliver_final_replylater re-applies the actual delivery predicate withshould_deliver_after_ack. That leaves the admission contract spread across two branches and makes future ack variants easy to route through the guard while still being no-ops inside delivery.
| hint_seen: HintSeenSet, | ||
| /// Single-flight guard: at most one live `deliver_final_reply` loop per run_id. | ||
| /// | ||
| /// A gate-resolution ack (`ApprovalResolution(Allow)` / `AuthResolution(Allowed)`) |
Collaborator
Author
There was a problem hiding this comment.
Low — Comments name non-existent resolution variants.
The new production comment describes ApprovalResolution(Allow) / AuthResolution(Allowed), but the local adapter types use ApprovalDecision::{ApproveOnce, AlwaysAllow} and AuthResolutionResult::{CredentialProvided, CallbackCompleted, Denied}. The same invented AuthResolution(Allowed) name is repeated in the new e2e comments, making grep/navigation for the actual auth-resolution path misleading.
Fix: Use the local type names in comments, e.g. ApprovalResolution with an approving ApprovalDecision and AuthResolutionResult::CallbackCompleted or CredentialProvided, or use plain prose without code-formatted pseudo-variants.
| // the permit and removes the run_id, L2 would wake and pass a now-empty guard | ||
| // set — the exact TOCTOU race this ordering closes. | ||
| // | ||
| // The `RunDeliveryGuard` RAII type ensures the run_id is removed on drop even |
Collaborator
Author
There was a problem hiding this comment.
Low — Panic-safe guard release is not covered.
The new guard is explicitly documented as preventing a permanent delivery block if deliver_final_reply panics, but the added tests cover timeout/error returns rather than the panic/unwinding path.
Fix: Add tests::slack_delivery::guard_is_released_after_delivery_panic_so_subsequent_ack_proceeds covering a deliver_final_reply panic after run_id guard insertion.
| // | ||
| // The `RunDeliveryGuard` RAII type ensures the run_id is removed on drop even | ||
| // if `deliver_final_reply` panics, preventing a permanent delivery block. | ||
| let _delivery_guard = if let Some(run_id) = submitted_run_id(&ack) { |
Collaborator
Author
There was a problem hiding this comment.
Low — Single-flight admission is split from delivery eligibility.
The new guard decides whether an ack may occupy the per-run single-flight slot by checking only submitted_run_id, then deliver_final_reply later re-applies the actual delivery predicate with should_deliver_after_ack. That leaves the admission contract spread across two branches and makes future ack variants easy to route through the guard while still being no-ops inside delivery.
Fix: Compute the eligible delivery run once before taking the guard, then pass that run_id into a delivery helper so the guard, semaphore, and delivery loop share one admission path.
| /// Slack text — it arrives from the WebUI gate-resolve path which calls | ||
| /// `observe_workflow_ack` directly. Exposing the observer lets the test inject | ||
| /// the resolution ack without going through the Slack route. | ||
| async fn build_harness_for_auth_fanout_test( |
Collaborator
Author
There was a problem hiding this comment.
Low — Auth fanout harness duplicates the existing builder.
The new helper reimplements the same Slack harness composition owned by build_harness_with_full_settings at crates/ironclaw_reborn_composition/src/slack_serve/e2e_tests.rs:240, adding substantial growth to an already 2,424-line test file. The architecture rule says existing 1,500-3,000 line files should leave shorter unless the feature has nowhere else to live; this can live in the shared builder by returning or optionally exposing the observer instead of duplicating the whole setup.
Fix: Refactor the shared harness builder to optionally return the observer or expose a small harness parts struct, then delete the duplicate auth-fanout builder.
henrypark133
added a commit
that referenced
this pull request
Jun 14, 2026
… re-approval loop) (#4839) * docs: import approval-invocation-identity fix plan Add Fix A plan covering auth-resume invocation identity preservation so one-shot approvals survive the approval+auth gate sequence. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(agent-loop): extend PendingAuthResume with prior approval identity Add optional `resume_token` and `approval_request_id` fields to `PendingAuthResume` so the auth-gate executor can propagate original invocation identity when the invocation previously passed a one-shot approval. - Gates stage now extracts approval identity from `GateInput.approval_resume` before moving it into `PendingApprovalResume`, then writes both fields into the new `PendingAuthResume` slot so auth re-dispatch carries them. - New compat test `pending_auth_resume_without_resume_token_fields_decodes_to_none` covers forward and backward serde compatibility: round-trip with fields set passes; JSON stripped of the fields decodes to `None` (pre-existing checkpoints decode cleanly). - All existing `PendingAuthResume` constructors updated with `None` defaults. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(turns): add CapabilityAuthResume and CapabilityInvocation.auth_resume field Add CapabilityAuthResume struct carrying the original resume_token and an optional approval_request_id. Add auth_resume: Option<CapabilityAuthResume> to CapabilityInvocation so the host port can distinguish auth-resume re-dispatch (preserving original invocation identity) from a fresh invoke. Backward-compat: serde(default, skip_serializing_if = "Option::is_none") ensures existing in-flight checkpoint payloads decode with auth_resume = None. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(capabilities,host-runtime): add auth-resume capability path ironclaw_capabilities: - Add CapabilityAuthResumeRequest with optional approval_request_id - Add CapabilityHost::auth_resume_json(): validates BlockedAuth status, claims approval lease when approval_request_id is Some, skips lease step when None, then authorizes and dispatches normally ironclaw_host_runtime: - Add RuntimeCapabilityAuthResumeRequest with idempotency_key + approval_request_id - Add HostRuntime::auth_resume_capability() with default fallback to invoke_capability - Override auth_resume_capability in DefaultHostRuntime: evaluates trust, enforces runtime policy, delegates to host.auth_resume_json() Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(loop-support,agent-loop): wire auth-resume dispatch through capability port ironclaw_loop_support / capability_port: - Add CapabilityAuthResume + RuntimeCapabilityAuthResumeRequest to imports - Add dispatch_runtime_capability_auth_resume() routing function - In invoke_capability: when request.auth_resume is Some, restore original invocation_id from resume_token (preserving approval-lease scope), and route through dispatch_runtime_capability_auth_resume rather than the normal invoke path; log at tracing::debug with invocation_id + approval_request_id ironclaw_agent_loop: - Export capability_invocation_from_auth_resume_candidate from executor.rs - In capability_helpers: add capability_invocation_from_auth_resume_candidate() that sets auth_resume from PendingAuthResume fields and approval_resume = None - In capabilities.rs batch builder: check pending_auth_resume first; when the re-dispatched call matches, use auth_resume path so invocation_id is preserved Test fixtures: add auth_resume: None to all CapabilityInvocation struct literals across loop_support, hooks, reborn, and reborn_composition test files; remove spuriously-added auth_resume from CapabilityOutcome::ApprovalRequired and GateInput literals (sed artifact). Adds auth_resume_preserves_invocation_id_and_approval test to ironclaw_reborn/tests/loop_driver_host.rs (red→green). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(tests): update missed CapabilityInvocation initializers and clippy warning Workspace-wide check caught two root-crate test initializers missing the new auth_resume field and one clone-on-Copy in the state round-trip test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(loop-support): give auth-resume dispatches a distinct idempotency key The invocation idempotency key only distinguished approval-resume from first dispatch, so an auth re-dispatch hashed to the same key as the original call and replayed its cached ApprovalRequired outcome instead of dispatching — re-blocking the run on an approval that was already granted. The new port-level lifecycle test (approval gate -> approval resume -> auth gate -> auth resume) caught this and now pins both the distinct-key behavior and the preserved invocation identity reaching the runtime. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(capabilities): harden auth_resume_json validation per review - Add capability_id consistency check in auth_resume_json (host.rs): mismatched run record capability_id returns ResumeContextMismatch, matching the pattern already enforced in resume_json. - Change default auth_resume_capability trait impl (host_runtime) from silent fallback-to-invoke to an explicit Unavailable failure; removes a path where unimplemented runtimes could silently misbehave. - Carry correlation_id through the auth-resume lifecycle: add field to PendingAuthResume and CapabilityAuthResume, thread through capability_invocation_from_auth_resume_candidate, and restore onto InvocationContext in capability_port auth-resume branch. - Remove .unwrap() in capabilities.rs: restructure is_some_and+unwrap to if-let-filter on pending_auth_resume. - Rename complete_run_after_side_effect label "auth_resume_dispatch" to "dispatch" to match sibling paths (host.rs line ~1232). - Extract to_approval_resume() on PendingApprovalResume; replace both manual CapabilityApprovalResume field constructions in capabilities.rs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: cover auth-resume review findings - auth_resume_json_rejects_capability_id_mismatch_against_run_record: starts a run with capability A, attempts auth_resume with capability B; asserts ResumeContextMismatch and zero dispatches. - auth_resume_json_rejects_approval_not_yet_approved: inserts a Pending approval record, calls auth_resume with that approval_request_id; asserts ApprovalNotApproved { status: Pending } and run stays BlockedAuth. - auth_resume_json_returns_store_missing_when_approval_requests_absent: host has run_state but no approval_requests store; calling auth_resume with an approval_request_id asserts ResumeStoreMissing { store: "approval_requests" }. - capability_invocation_from_auth_resume_candidate_with_none_resume_token_sets_auth_resume_none: unit test confirming that a PendingAuthResume with no resume_token produces a CapabilityInvocation with auth_resume None. - Update auth_resume_after_approval_reuses_original_invocation_identity to pass and assert correlation_id is preserved through the auth-resume path. - Fix PendingAuthResume initializers in executor tests to supply the new correlation_id field (None). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(capabilities): cover approval lease survival across real auth bounce Adds `auth_resume_after_real_approval_bounce_reuses_claimed_lease` to capability_host_auth_resume_contract.rs. This test drives the complete real ordering — invoke_json → approve → resume_json (dispatcher returns AuthRequired) → auth_resume_json — and asserts the five post-fix invariants: (a) lease is Claimed (not Revoked) after the resume_json auth bounce (b) auth_resume_json succeeds and dispatches (c) approval request remains Approved (d) lease is Consumed after successful dispatch (e) capability was dispatched with the same invocation_id as the original The test currently FAILS at assertion (a): lease status is Revoked, not Claimed. This proves the bug is real: resume_json unconditionally revokes the claimed lease on a dispatch-error path even when the error is a non-terminal BlockAuth transition (AuthorizationRequiresAuth). Also renames `auth_resume_json_with_approval_request_id_claims_lease_and_dispatches` to `auth_resume_json_with_approval_request_id_claims_active_lease_and_dispatches` to clarify that it tests the clean-ordering shortcut path (Active lease), not the real bounce path covered by the new test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(capabilities): keep claimed approval lease across non-terminal auth bounce When resume_json dispatches and gets AuthorizationRequiresAuth back, the run transitions to BlockedAuth — a non-terminal state. Previously the code unconditionally revoked the approval lease at the dispatch-error site, leaving it Revoked. Subsequent auth_resume_json then called matching_approval_lease (Active-only) and got ApprovalLeaseMissing, breaking the entire approval→auth flow. Part A (resume_json): guard the two post-claim revoke sites so revoke is skipped when the error carries a BlockAuth run-state transition. The lease stays Claimed, keeping it recoverable. Part B (auth_resume_json with-approval path): after failing to find an Active lease via matching_approval_lease, fall back to matching_claimed_approval_lease_for_auth_resume which scans leases_for_scope for a Claimed lease with matching capability and invocation fingerprint. Reuse it directly instead of returning ApprovalLeaseMissing. Consume on success, revoke on terminal failure. Adds matching_claimed_approval_lease_for_auth_resume helper in helpers.rs and is_block_auth_transition predicate in host.rs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(turns): drop forbidden InvocationId identifier from CapabilityAuthResume doc The Reborn architecture gate (reborn_turns_public_surface_uses_turn_ids_not_ runtime_or_process_ids) forbids the runtime/process identifier `InvocationId` in the ironclaw_turns public surface. A doc comment referenced it; reworded to 'invocation identifier' without the forbidden token. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(agent-loop): consume pending_auth_resume on first batch match `pending_auth_resume` was read with `as_ref().filter(…)` (non-consuming), so a batch with two calls to the same capability_id would tag both as auth-resume, reusing one resume_token/invocation_id across distinct calls — a correctness and security bug. Mirror the approval path: use `take_if` so the slot is consumed on the first matching call and subsequent calls in the same batch dispatch normally. Regression test drives CapabilityStage directly with two calls to the same capability_id and asserts only the first carries auth_resume. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(capabilities): consume/revoke auth-resume lease mirroring resume_json `auth_resume_json` claimed or reused a capability lease but never consumed it on success nor revoked it on terminal failure — unlike `resume_json` which does both. Mirror `resume_json`'s lease lifecycle: - Success path: consume the lease via `claimed.grant.id`. - Obligation-error and dispatch-error branches: revoke the lease, but only when the error is NOT a non-terminal BlockAuth transition (guard via `is_block_auth_transition`) so a re-block leaves the lease Claimed for the next auth-resume attempt. - Completion-obligation-error branch: revoke unconditionally (terminal). Regression tests assert: - Lease is Consumed after successful auth-resume dispatch. - Lease is Revoked after a terminal dispatch failure. - Lease stays Claimed after a non-terminal BlockAuth re-block. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(host-runtime): fail BlockedAuth run on auth-resume preflight rejection `auth_resume_capability` returned early on `enforce_runtime_policy` rejection and `evaluate_invocation_trust` failure without transitioning the durable BlockedAuth run record, leaving a stale resumable gate after the caller saw a terminal failure. Mirror `resume_capability`: add `fail_matching_blocked_auth_resume_on_preflight_error` (analogous to the existing `fail_matching_blocked_resume_on_preflight_error`) and call it at both preflight-failure return sites in `auth_resume_capability`. The new helper matches runs in RunStatus::BlockedAuth (not BlockedApproval) and additionally filters on the optional approval_request_id carried by auth-resume state. Regression test drives `auth_resume_capability` against a broken registry (empty → trust eval fails) and asserts the matching BlockedAuth run transitions to Failed while a wrong-scope call leaves the run untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(host-runtime): expect preserved Claimed lease on missing-secret auth bounce The resume-missing-secret contract test pinned the pre-Option-2 behavior (lease Revoked on the auth bounce). Option 2 intentionally preserves the claimed lease across a non-terminal BlockedAuth bounce so the same invocation reuses it on auth-resume without a second approval. Update the assertion to the new contract; the terminal-failure revoke paths remain covered elsewhere. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(agent-loop,turns): model prior-approval identity as one typed value Collapse the two formerly-independent optional fields `approval_request_id: Option<ApprovalRequestId>` and `correlation_id: Option<CorrelationId>` in `PendingAuthResume` and `CapabilityAuthResume` into a single typed struct `AuthResumeApprovalIdentity { approval_request_id, correlation_id }`. The pair is all-or-none: both sub-fields are present only when the invocation previously passed a one-shot approval gate. Modelling them as a single optional struct makes the compile-time invariant explicit and removes the risk of one field being Some while the other is None. `AuthResumeApprovalIdentity` is defined in `ironclaw_turns` (the neutral contract crate) and re-exported from `ironclaw_agent_loop::state` for use in the executor. The forbidden `InvocationId` PascalCase token does not appear anywhere in `ironclaw_turns`. All construction and read sites updated: gates.rs (builds prior_approval from input.approval_resume), capability_helpers.rs (capability_invocation_from_auth_resume_candidate), capability_port.rs (restores invocation id from resume_token, correlation from prior_approval), and test literals. Serde `#[serde(default, skip_serializing_if = ...)]` on the field preserves backward compat with existing checkpoints. Also extract the shared resumed-dispatch helper `dispatch_resumed_capability` in `ironclaw_capabilities::host`. `resume_json` and `auth_resume_json` previously ran identical sequences (authorize → claim/reuse lease → prepare_obligations(Resume) → dispatch_json → complete_dispatch_obligations → consume lease → complete_run_after_side_effect) with copy-pasted error handling. The shared tail is now owned exactly once in the private `dispatch_resumed_capability` method, parameterized by `ResumedDispatchParams`. The original claim-after-authorize order for `resume_json` is preserved via `PendingClaimAfterAuth` so that authorization denial keeps the lease Active (no behavior change). resume_json: 379 → 184 lines; auth_resume_json: 423 → 235 lines; dispatch_resumed_capability helper: 255 lines (new); host.rs total: 2144 → 2055 lines. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: close auth-resume coverage gaps - ironclaw_agent_loop: assert pending_auth_resume cleared after first batch match; assert correlation_id carried through approval→auth bounce; add 3-phase test verifying original correlation_id survives the full approval→auth-block→auth-resume pipeline - ironclaw_host_runtime: add happy-path auth_resume_dispatches_blocked_auth_run (3-phase: invoke→approval gate, approve+resume→BlockedAuth, add credential+auth_resume_capability→Completed) - ironclaw_capabilities: add concurrent_auth_resume_claim_loser_returns_lease_error_without_failing_run (ClaimFailingLeaseStore simulates CAS race loser; asserts Lease error returned and run stays BlockedAuth, not Failed) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(capabilities,agent-loop): replace dual-Option lease fields with 3-state enum; add both-present auth-resume test Fix 1 — replace `ResumedDispatchParams` two mutually-exclusive `Option` fields (`pending_claim`, `claimed_lease`) with a single `ResumedLeaseState` enum that encodes all three valid states at the type level: * `PendingClaim(PendingClaimAfterAuth)` — `resume_json`: claim the Active lease after `authorize_dispatch_with_trust` returns Allow, so a Deny leaves the lease Active. * `AlreadyClaimed(&CapabilityLeaseStore, Box<CapabilityLease>)` — `auth_resume_json` with prior approval: the lease was already transitioned to Claimed in the preamble; reuse without re-claiming. * `NoPriorLease` — `auth_resume_json` with no approval_request_id: no lease step needed. The previous doc comment "exactly one of the two is Some" was incorrect: the no-prior-approval path of `auth_resume_json` set both to None. The enum makes `NoPriorLease` explicit and removes the impossible "both Some" case at the type level. Behavior is unchanged. Fix 2 — add a unit test `capability_invocation_from_auth_resume_candidate_with_both_token_and_prior_approval` in `ironclaw_agent_loop::executor::capability_helpers::tests` covering the case where both `resume_token` and `prior_approval` are set. Asserts the resulting `CapabilityInvocation` carries `auth_resume: Some(...)` with the correct resume token and `prior_approval` identity, and `approval_resume: None`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(authorization,capabilities): close Claimed-lease reuse double-dispatch race with Dispatching state Adds a transient `Dispatching` lease status so exactly one concurrent auth-resume can win the reuse race on an already-Claimed approval lease. - `CapabilityLeaseStatus::Dispatching`: new transient state between `Claimed` and `Consumed` for the reuse-CAS window. - `CapabilityLeaseStore::begin_dispatch_claimed`: atomic CAS Claimed→Dispatching; loser gets `InactiveLease{Dispatching}` and bails like a lost Active `claim()`. Implemented in both InMemory and Filesystem stores. - `CapabilityLeaseStore::abort_dispatch_claimed`: reverts Dispatching→Claimed on non-terminal auth re-bounce so the next auth_resume_json call finds the lease again. No-op if already Claimed. - `auth_resume_json` reuse site: replaces plain read-then-dispatch with `begin_dispatch_claimed`; loser returns Lease error without failing run. - `dispatch_resumed_capability` BlockAuth-skip sites (×2): on `is_block_auth_transition`, call `abort_dispatch_claimed` instead of skipping revoke, so a Dispatching lease reverts to Claimed for the next auth-resume attempt. - `ensure_consumable`: adds `Dispatching` to the OK arm alongside `Active | Claimed`; fingerprint guard allows `Claimed | Dispatching`. - `was_claimed` in both `consume` impls extended to `Claimed | Dispatching`. - `claim_error_may_be_concurrent_resume`: adds `Dispatching` to the match arm so the loser warn path fires correctly. Also adds `concurrent_auth_resume_reuse_loser_does_not_double_dispatch` to the auth-resume contract suite: seeds a Claimed lease, arms a `begin_dispatch_claimed` failure, fires auth_resume_json, asserts exactly one dispatch, loser returns Lease error without run failure, lease ends Consumed. Nits: rewrites stale "pre-fix behavior" comment in `runtime_lifecycle_tests.rs` to state the invariant instead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(slack): remove mis-homed approval-then-auth delivery e2e from Fix A `slack_approval_then_auth_resume_completes_without_second_approval` was added to this branch but never passed — it fails at the commit that introduced it (9a7fc2b), so it slid in without the composition suite being run. It does not exercise Fix A's lease/identity changes: it fakes auth completion by advancing the recording coordinator directly, so it depends on the Slack delivery loop surviving the approval→auth gate transition with no re-trigger event. That delivery-loop lifecycle is owned by the single-flight delivery work (#4843), not by the capability-host invocation-identity fix. Fix A's real contract is covered by the green ironclaw_capabilities / ironclaw_host_runtime auth-resume contract tests. Remove the test and its now-unused harness scaffolding (the `BlockApprovalThenAuth` TurnMode variant, its match arm, and the `transition_blocked_approval_to_blocked_auth` helper) so #4839 is green. The approval→auth→final-reply delivery scenario is re-homed onto the delivery PR per the tracking issue. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(hooks): revert orphaned capability-port test additions `crates/ironclaw_hooks/src/middleware/tests/capability_port.rs` is not wired into any module tree (no `mod`, `include!`, or `path =` reference in the hooks crate). The six `auth_resume: None` field additions added on this branch are dead code that cargo never compiles — a false grep/navigation trail. Restore the file to its main-branch content. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(contracts): document auth-resume path and Dispatching lease status capabilities.md: add Auth-gate resume section covering `CapabilityHost::auth_resume_json` preconditions (BlockedAuth guard, capability-id match, context validate), prior-approval lease handling (Active → Claimed search, begin_dispatch_claimed CAS, abort on non-terminal BlockedAuth re-bounce, consume/revoke on terminal outcome), and the host-runtime integration path through `HostRuntime::auth_resume_capability` / `RuntimeCapabilityAuthResumeRequest`. approvals.md: add `Dispatching` to the `CapabilityLeaseStatus` enum, document `begin_dispatch_claimed` and `abort_dispatch_claimed` store operations, note that `consume`/`revoke` accept both Claimed and Dispatching as source states, and extend the §1 flow diagram with the `auth_resume_json` approval-lease sub-flow. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(host-runtime): drop approval_request_id equality guard in fail_matching_blocked_auth_resume_on_preflight_error The `BlockedAuth` run-state transition explicitly clears `approval_request_id` to `None` on the persisted record. The auth-resume request on the approval-then-auth path still carries `approval_request_id = Some(id)` so it can claim the approval lease. The old three-condition guard compared `record.approval_request_id` (always `None` for BlockedAuth records) against `Some(id)`, which was always inequal, causing the function to return early without transitioning the stale BlockedAuth run to Failed. The result: a terminal failure was returned to the caller while the run stayed BlockedAuth and remained resumable (stuck). Fix: drop the `approval_request_id` equality from the guard. `invocation_id` (the `get` key used two lines above) already uniquely identifies the run, so the approval-id comparison was redundant and actively wrong for the approval-then-auth path. The `status == BlockedAuth` and `capability_id` checks are retained. Because `approval_request_id` is now entirely unused in the function body, the parameter is also removed from the signature and its two call sites. Regression test added: `host_runtime_services_auth_resume_with_approval_id_fails_blocked_auth_run_on_preflight_error` — BlockedAuth record with `approval_request_id = None`, auth-resume request with `approval_request_id = Some(id)`, trust preflight rejection → assert run transitions to Failed (not left as stale BlockedAuth). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(capabilities): add four contract tests flagged by PR review - TEST 1: auth_resume_json_returns_store_missing_when_capability_leases_absent Covers the second ResumeStoreMissing branch (missing capability_leases, not missing approval_requests) — previously exercised only by the approval_requests-absent variant. - TEST 2: auth_resume_json_rejected_prior_approval_fails_blocked_auth_run Exercises the non-Pending (Denied) approval branch: asserts ApprovalNotApproved is returned AND fail_run_if_configured transitions the run to Failed. - TEST 3: filesystem_lease_store_persists_dispatching_begin_abort_and_consume Full begin_dispatch_claimed → abort_dispatch_claimed → begin again → consume lifecycle on FilesystemCapabilityLeaseStore with reload assertions at each step. - TEST 4: concurrent_auth_resume_reuse_loser_does_not_double_dispatch Real winner/loser race on begin_dispatch_claimed using a BarrierLeaseStore that synchronises both callers through leases_for_scope before either calls begin_dispatch_claimed, and a GatingDispatcher that holds the winner in-flight. Asserts loser sees InactiveLease{Dispatching} (exact variant+status), run stays BlockedAuth for the loser, and exactly one dispatch completes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * guard(capability-port): fail-closed on both-resume-modes-set invocation Add an explicit `(Some(_), Some(_))` arm before `(Some, _)` in the `invoke_capability` dispatch match so a `CapabilityInvocation` with both `approval_resume` and `auth_resume` set returns `InvalidInvocation` instead of silently falling through to the approval-resume path. The two resume modes are mutually exclusive by design (the executor clears `approval_resume` on `AuthRequired`), so simultaneous presence indicates a malformed invocation. The guard is purely defensive; it is not producible through the normal executor flow today. Added test `invoke_capability_rejects_both_resume_modes_set` that constructs a dual-resume invocation against a live port and asserts the `InvalidInvocation` error with the mutual-exclusion message. The three valid combinations (approval-only, auth-only, neither) are exercised by the existing test suite which remains fully green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(capabilities): reword review-artifact FIX labels to behavior descriptions Replaces "BUG FIX 1b" / "BEFORE FIX / AFTER FIX" review-bookkeeping labels in the auth-resume contract tests with comments that describe the invariant under test (per PR review nit). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(capabilities): advance fresh Active lease to Dispatching before dispatch in auth_resume_json The FRESH Active-lease branch of `auth_resume_json` called `claim()` (Active→Claimed) but left the lease in `Claimed` for the duration of dispatch. A concurrent `auth_resume_json` that raced past the Active lease check could find the `Claimed` lease in the REUSE branch and call `begin_dispatch_claimed` successfully, advancing it to `Dispatching` and double-firing the invocation. Fix: immediately after `claim()` succeeds in the fresh path, call `begin_dispatch_claimed` to advance `Claimed→Dispatching` — the same in-flight single-winner fence that already protected the REUSE path. The shared tail (`dispatch_resumed_capability`) already accepts both `Claimed` and `Dispatching` via `consume`, so no tail changes were needed. Regression test: `concurrent_auth_resume_fresh_active_lease_loser_does_not_double_dispatch` — uses `GatedLeaseStore` to park the fresh-path caller inside `claim()` while the reuse-path caller wins, then asserts the loser returns `Err(Lease(_))` post-fix instead of dispatching a second time. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(capabilities): check capability existence before acquiring approval lease in auth_resume_json In the pre-fix code, `auth_resume_json` mutated the approval lease (Active→Claimed→Dispatching or Claimed→Dispatching via `begin_dispatch_claimed`) **before** verifying that the capability still exists in the registry. If `get_capability` returned `None`, the `UnknownCapability` early-return fired after the lease was already in `Dispatching`, leaving a one-shot approval lease permanently stranded and unrevokable. Fix: move `self.registry.get_capability(...)` to before the approval-lease acquisition block so an unknown capability returns `UnknownCapability` without touching the lease at all. The `descriptor` binding is hoisted to just above the lease block and threaded into `dispatch_resumed_capability` unchanged. `resume_json` is NOT affected: its `get_capability` check already appears before `matching_approval_lease` (no ordering hole). Regression tests added in `capability_host_auth_resume_contract`: - `auth_resume_json_unknown_capability_does_not_strand_active_approval_lease` (Active lease path — pre-fix: stranded in Dispatching) - `auth_resume_json_unknown_capability_does_not_strand_claimed_approval_lease` (Claimed lease reuse path — pre-fix: stranded in Dispatching) Both tests were RED before the fix (`left: Dispatching`) and GREEN after. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(capabilities): add lease_id to concurrent-resume warn and DRY cleanup state machine CHANGE 1 (JYSbT): add `lease_id = %claimed_lease.grant.id` to the concurrent-auth-resume warn! at the `begin_dispatch_claimed` failure site in `auth_resume_json` — matches the adjacent Active-lease race warn field style so production traces can correlate the two races. CHANGE 2 (JYSbX): extract `cleanup_claimed_lease_after_resume_error` private async helper from `dispatch_resumed_capability`. The obligation- failure arm and dispatch-failure arm repeated the same abort-vs-revoke decision; the only difference was the revoke warn message label ("obligation failure" vs "dispatch failure"), now passed as a `revoke_context: &str` parameter. Behavior byte-for-byte identical. CHANGE 3 (JYSbY): extract `apply_begin_dispatch_claimed_transition` and `apply_abort_dispatch_claimed_transition` as `pub(crate) fn` helpers in `ironclaw_authorization`. Both `InMemoryCapabilityLeaseStore` and `FilesystemCapabilityLeaseStore` now delegate their state-transition rules to these shared functions; each store retains its own persistence / CAS mechanics. Mirrors the `ensure_claimable`/`ensure_consumable` pattern. Behavior identical; all 60 authorization and 106 capabilities tests pass unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(capabilities): add missing coverage for auth_resume_json early-exit branches (JYSbO/JYSbP/JYSbQ) Three new contract tests in capability_host_auth_resume_contract.rs: JYSbO — auth_resume_json_returns_store_missing_when_run_state_absent Builds the host without a run_state store and asserts ResumeStoreMissing { store: "run_state" } before any dispatch. JYSbP — auth_resume_json_unknown_invocation_when_run_record_missing Wires run_state but seeds no record for the invocation; asserts RunState(UnknownInvocation) via the ok_or at host.rs ~798. JYSbQ — three approval-request mismatch branches (action / correlation_id / requested_by) via validate_approval_request_matches_invocation called at host.rs ~894. Each test seeds an Approved approval with exactly one mismatched field, asserts ApprovalRequestMismatch { field }, zero dispatch, and run transitioned to Failed (fail_run_if_configured called on all mismatch paths). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(capabilities): normalize resume mode once before context mutation (JYSbV) Introduce a local `ResolvedResumeMode` enum computed from `(approval_resume, auth_resume)` up front, moving the mutually-exclusive guard (`(Some, Some) => InvalidInvocation`) before any `invocation_context` mutation. Previously the both-set check fired inside the dispatch match after context had already been mutated; now it fires immediately, before touching context. The context mutation (invocation_id restore, correlation restore, validate) and the runtime request build are both driven from the same single `resume_mode` value, eliminating the second tuple match. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
henrypark133
added a commit
that referenced
this pull request
Jun 14, 2026
…4873) * test(slack): re-home approval→auth→final-reply delivery e2e (#4847) Re-adds slack_approval_then_auth_resume_completes_without_second_approval, removed from #4839 (commit b847f51) as born-broken due to a test-harness gap. With #4843's single-flight delivery guard now in main, the production deliver_final_reply loop survives BlockedApproval → BlockedAuth → Completed on one delivery loop. Adds the BlockApprovalThenAuth TurnMode variant and a transition_blocked_approval_to_blocked_auth coordinator helper to stage the two-gate sequence. Test-only; no production changes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(slack): drive approval→auth hop through the approval caller + deterministic completion (#4847) Addresses PR #4873 review: - Route the approval→auth transition through RecordingApprovalInteractionService::resolve (mode-aware on BlockApprovalThenAuth) instead of a coordinator backdoor; post the approve event and assert exactly one approval-service request. list_pending only surfaces the approval gate while the run is BlockedApproval. Removes the transition_blocked_approval_to_blocked_auth backdoor. Satisfies Test-Through-the-Caller. - complete_blocked_run transitions BlockedAuth→Completed in one locked mutation (no observable Running), removing the window where the delivery loop posts the working indicator and flakes the message-count assertion. - Drop the mid-test drain after the approve event: delivery loops are awaited by drain_immediate_ack_tasks, so draining while the run is still BlockedAuth blocked L1 until max_wait, leaving no loop to deliver the final reply. Poll for the async auth prompt instead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
elliotBraem
added a commit
to NEARBuilders/ironclaw
that referenced
this pull request
Jun 15, 2026
* adds nova extension * feat: force-exit on second Ctrl+C in serve command * fix(serve): reliable force-exit on Ctrl+C and shutdown timeout - Use a single tokio::signal::unix::Signal with recv() for both Ctrl+C presses, eliminating the drop/recreate gap where signals could be lost. - Add 15s timeout on runtime.shutdown() as a safety net so the process always exits even if background tasks hang. - Non-Unix platforms fall back to the single Ctrl+C handler. * init * next * wip * wip * wip * script * better contract * ironclaw connected * create thread works * wip * test(slack): re-home approval→auth→final-reply delivery e2e (nearai#4847) (nearai#4873) * test(slack): re-home approval→auth→final-reply delivery e2e (nearai#4847) Re-adds slack_approval_then_auth_resume_completes_without_second_approval, removed from nearai#4839 (commit b847f51) as born-broken due to a test-harness gap. With nearai#4843's single-flight delivery guard now in main, the production deliver_final_reply loop survives BlockedApproval → BlockedAuth → Completed on one delivery loop. Adds the BlockApprovalThenAuth TurnMode variant and a transition_blocked_approval_to_blocked_auth coordinator helper to stage the two-gate sequence. Test-only; no production changes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(slack): drive approval→auth hop through the approval caller + deterministic completion (nearai#4847) Addresses PR nearai#4873 review: - Route the approval→auth transition through RecordingApprovalInteractionService::resolve (mode-aware on BlockApprovalThenAuth) instead of a coordinator backdoor; post the approve event and assert exactly one approval-service request. list_pending only surfaces the approval gate while the run is BlockedApproval. Removes the transition_blocked_approval_to_blocked_auth backdoor. Satisfies Test-Through-the-Caller. - complete_blocked_run transitions BlockedAuth→Completed in one locked mutation (no observable Running), removing the window where the delivery loop posts the working indicator and flakes the message-count assertion. - Drop the mid-test drain after the approve event: delivery loops are awaited by drain_immediate_ack_tasks, so draining while the run is still BlockedAuth blocked L1 until max_wait, leaving no loop to deliver the final reply. Poll for the async auth prompt instead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * wip * feat(runtime-context): surface connected channels, delivery state, and run origin (nearai#4836) * feat(turns): runtime-context communication slice types and rendering Foundation for nearai#4828: TurnRunOrigin enum, run_origin threaded through SubmitTurnRequest/TurnRunState/LoopRunContext, CommunicationRuntimeContext (connected channels, delivery target, origin) rendered in the runtime context section. communication=None renders byte-identical to the nearai#4795 baseline, so existing fingerprints are unaffected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(reborn): wire communication runtime-context slice and run origins Wires nearai#4828 end-to-end: - CommunicationContextProvider trait stamped at loop spawn; composition provider reads outbound preferences (2s timeout, degrades to Unknown, never blocks loop start); delivery_tools_visible derived from the visible capability surface - run_origin set at submit sites (WebUI chat, product inbound with adapter id, conversation inbound deriving trigger vs product from adapter kind) and persisted on TurnRunRecord so it survives restart - DeliveryTargetState::SetUnresolved keeps a stored preference from rendering as "none set" when no target provider registry is wired - extends existing loop_driver_host and default_system_prompt tests Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(reborn): apply code-review fixes to communication context slice - type adapter identity as ProductAdapterId end-to-end; typed is_trusted_trigger() predicate replaces string comparison; replay path carries the real adapter kind instead of an empty string - run_origin becomes a CommunicationContextProvider parameter so the provider returns a fully populated context (no post-mutation) - scheduled-trigger no-delivery-target warning renders unconditionally; only the tool-name sentence is gated on tool visibility - sanitize adapter/display strings at prompt render time - wire connected channels from the lifecycle facade behind an explicit pre-nearai#4778 channel-surface predicate (renders 'none' until the ProductAdapter surface projection lands); 500ms fetch timeout - provider unit tests plus run-origin assertions in existing contract and loop-driver tests Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: ProductContextFactory design spec (nearai#4828 follow-up) Design for ironclaw_product_context: a single ingress resolver that owns turn-origin/surface/owner classification, replacing the scattered run_origin plumbing. Generic ProductTurnContext persisted on the turn; live account state stays in the composition provider. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(reborn): address PR nearai#4836 review — origin integrity, honest channel state, concurrency - ScheduledTrigger origin now requires a Trusted binding policy, not just adapter text; untrusted inbound with adapter_kind "trigger" records ProductInbound (regression test added) - connected-channels renders Unknown (not a false "none") while channel classification is unavailable pre-nearai#4778 - outbound-preferences and lifecycle fetches run concurrently under one 500ms budget instead of sequential 500ms each - surface-state read logs on error instead of silently dropping it - is_trusted_trigger compares against ironclaw_triggers::TRIGGER_TRUSTED_ADAPTER_KIND - channel names sanitized at prompt render; child runs inherit parent origin - CommunicationContextProvider trait documented; mock captures all args - tests: WebUI origin in model request, ordinary inbound origin Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: ProductContextFactory implementation plan (nearai#4828 follow-up) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(turns): generic ProductTurnContext types, replacing TurnRunOrigin Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(product-context): ingress resolver crate (resolve_inbound/resolve_web_ui) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(turns): carry product_context on turns, drop run_origin Replace the deleted `TurnRunOrigin` enum and its `run_origin` field with `product_context: Option<ProductTurnContext>` on all four structs (`SubmitTurnRequest`, `TurnRunState`, `TurnRunRecord`, `LoopRunContext`) and the in-memory run record in `memory.rs`. - Rename `LoopRunContext::with_run_origin` → `with_product_context` - Replace `CommunicationRuntimeContext::run_origin: Option<TurnRunOrigin>` with `product_context: Option<ProductTurnContext>`; update `render_model_content` to match on `TurnOriginKind` instead of enum variants; update `CommunicationContextProvider` trait signature - Swap `pub use crate::TurnRunOrigin` → `pub use crate::ProductTurnContext` in `run_profile/mod.rs` - Rewrite origin serde tests in `agent_loop_host_contract.rs` to cover `ProductTurnContext` round-trips; rename old `run_origin` field tests - Rewrite `filesystem_turn_state_store_persists_run_origin_…` snapshot round-trip test using `ProductTurnContext` - Fix imports in all test files; zero `TurnRunOrigin`/`run_origin` references remain in the crate Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor: resolve product_context at the four ingress submit sites Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(reborn): migrate test mocks/fixtures to product_context Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore: migrate remaining loop_support/host_runtime test literals to product_context Completes the run_origin → product_context field rename across the remaining test crates; fmt + clippy --all --all-features clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(product-context): address PR review — skip discarded fetch, thread surface_type, validate-on-deserialize, tests, docs Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(product-context): seal ProductTurnContext construction + collapse resolver inputs - ProductTurnContext is #[non_exhaustive] with a new() constructor, so external crates can no longer mint a ScheduledTrigger origin via struct literal; the resolver is the single intended mint point (turn submission stays a trusted boundary — honest framing, not a hard seal) - resolver takes one InboundClassification {TrustedTrigger,TrustedOther,Untrusted} instead of a separable (TrustLevel, is_trigger_adapter) pair, so a mismatched pair is unrepresentable; TrustLevel removed - call sites collapse their policy+trigger signal into the single classification Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(runtime-context): render run origin from LoopRuntimeContext, not the communication provider Moves the persisted ProductTurnContext onto LoopRuntimeContext (sibling of the live communication state) and drops it from CommunicationRuntimeContext and the CommunicationContextProvider signature. Origin/surface/owner now render directly from the run context — independent of whether a communication provider exists — and provider fakes no longer carry state they don't own. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(runtime-context): degrade model-unsafe labels instead of failing the prompt External labels (channel name, delivery target display/channel, adapter) are now rendered via model_safe_label: sanitized, then validated against the same model-safe-text policy the prompt bundle enforces. A legitimate label that would still trip that policy (e.g. a channel named #secret-alerts / "authorization") degrades to a placeholder so it can never fail prompt construction — the slice degrades, not the whole bundle. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(product-context): address 2nd-round PR review — docs, channel-list cap, surface tests, trigger-predicate layering Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(runtime-context): clarify scheduled-trigger warning requires known delivery state The no-delivery warning renders only when delivery state is known NoneSet (which requires the communication slice). When communication is absent the delivery state is unknown, so no warning is emitted — a target may exist and claiming otherwise would be wrong. Production triggered runs carry the slice. * fix(context-slice): address PR review — dedup, surface reuse, tests, docs - loop_driver_host: compute delivery_tools_visible from the captured visible_capabilities() surface instead of a second surface_state.current() scan; drop the redundant warn! degradation arm (JYDXp) - runtime_context: extract render_origin_line() helper; both render branches share one origin-line path, warning logic stays comm-gated (JYDXv) - inbound: add trusted_non_trigger_adapter_records_inbound_origin caller test for the TrustedOther -> Inbound branch (JYDXq) - origin: add deserialize_rejects_overlong_run_origin_adapter serde boundary test; point doc comment at resolve_inbound/resolve_web_ui as the mint points, new() as the low-level constructor (JYDXr, JYDXt) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * perf(context-slice): overlap advisory communication fetch with loop start The communication slice is advisory and must never block loop start, but the provider was awaited inline before prompt construction — adding up to the provider's 500ms timeout budget serially to every run (JYDXo). Make overlap a property of the provider contract instead of an ad-hoc spawn: - CommunicationContextProvider::communication_context (async, takes delivery_tools_visible) -> begin_communication_context (sync, returns a CommunicationContextFetch handle). delivery_tools_visible is surface- derived, not a fetch input, so it leaves the fetch signature entirely. - New CommunicationContextFetch: the provider drives the backend lookups concurrently (the production impl spawns); the caller joins later via resolve(delivery_tools_visible), which stamps the surface-derived flag onto the resolved context. - loop_driver_host starts the fetch right after run_context is bound, so its latency + timeout budget overlaps gate/dispatcher construction and capability-surface computation. In the common case the fetch is already resolved by prompt-build, adding ~0ms to the critical path; worst case is bounded by the same 500ms budget, now spent in parallel. No coverage loss vs the fail-fast alternative and no stale-cache risk. Provider unit tests updated to begin(...).resolve(flag); the host-level "capability in surface -> flag true" test now asserts the rendered tool-hint warning (end-to-end through resolve) instead of inspecting a recorded provider argument that no longer exists. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * obs(context-slice): debug! when trigger run renders with no comm slice The no-delivery safety warning only fires in the communication-present render branch. Production triggered runs are expected to always carry a communication slice, so a ScheduledTrigger reaching the origin-only branch means the warning is silently skipped — an invariant breach. Emit a debug! there so the breach is observable without changing rendered output (render stays correct: asserting "won't be delivered" without known delivery state would be wrong). Enforcing the invariant on the trigger composition path is tracked as a follow-up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(context-slice): address PR nearai#4836 review — WebUI owner, observability, tests - runtime: turn_scope_for now carries the runtime's explicit owner via new_with_owner(Some(actor_user_id)); WebUI chat runs persist TurnOwner::Personal{user} instead of SharedAgent. Document send_user_message as a WebUI-only origin path (resolve_web_ui); non-WebUI ingress must resolve its own origin. New regression send_user_message_persists_personal_owner_for_webui. - communication_context: match JoinError explicitly (debug! + // silent-ok:) instead of .ok().flatten(); emit debug! on the timeout arm and both Err degradation paths before collapsing to Unknown; keep the plain (skipped) lifecycle None arm silent. - product_workflow: add shared_user_message_records_channel_surface_type asserting a BotMention shared route persists surface_type = Channel. - turns: add submit_child_run_inherits_parent_product_context asserting a child run carries the parent's product_context. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(context-slice): address PR nearai#4836 review — doc/comment hygiene - plan: remove developer-local absolute worktree paths (doc-hygiene rule); use repo-relative working-dir note and run command. - design spec: update the resolver API from the removed `TrustLevel + is_trigger_adapter` pair to `InboundClassification` ({TrustedTrigger, TrustedOther, Untrusted}) and the current `resolve_inbound(classification, adapter, surface_type, owner)` signature. - turns: rename stale `WebUiChat` references in a runtime-context test to `WebUi` to match the renamed origin variant. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(context-slice): seal trigger origin via typed evidence; address PR nearai#4836 review Folds the nearai#4851 trust-seal follow-up into this PR. Trust seal (Option 7) — origin classification no longer re-derives trigger-ness from the adapter_kind string: - conversations: carry a typed `TrustedInboundKind::{Trigger,Other}` on `TrustedInboundTurnRequest`; the trusted-trigger submit seam sets `Trigger`. `handle_inbound_turn_inner` classifies from the typed kind on `BindingResolutionPolicy::Trusted`, not `is_trusted_trigger_adapter_kind(adapter_kind)`. A `ScheduledTrigger` origin is now a structural consequence of entering through the trusted-trigger seam (.claude/rules/types.md). Advisory comm slice: - communication_context: an actor-present JoinError now degrades to `Some(Unknown)` (not `None`, which is the no-actor sentinel) so the degrade-to-unknown contract holds and delivery_tools_visible is still stamped; regression test added. Tests: - conversations: InvalidRunOriginAdapter classification (→ SubmitRejected) and submit-key non-rotation regressions. Docs: - origin.rs / product_context AGENTS.md: correct the over-claimed "only place that can mint ScheduledTrigger" — `ProductTurnContext::new` is a low-level constructor, not a hard cross-crate seal (Rust has no friend-crate visibility); the enforced boundary is the typed trusted-trigger ingress seam. - plan + design spec: reconcile to the implemented `InboundClassification` 4-arg resolver and the typed trigger-evidence seal. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(context-slice): address PR nearai#4836 review round — newtype shape, abort-on-drop, delivery tools - RunOriginAdapter: adopt the canonical newtype shape (Hash, shared validate(), into_inner, AsRef<str>, Display, From<_> for String) per types.md; align its byte bound to AdapterKind's 512 (via MAX_RUN_ORIGIN_ADAPTER_BYTES) so a valid long adapter kind is no longer narrowed/rejected before submit. Tests at the 512 boundary + a caller-level long-adapter-kind acceptance test. - CommunicationContextFetch: own an abort-on-drop JoinHandle instead of a boxed future. Dropping an unresolved fetch (e.g. early host-construction failure) now aborts the spawned backend lookup instead of detaching it; the type can only be built from a spawned handle, enforcing the concurrency contract. The actor-present JoinError → Some(Unknown) degrade moves into resolve(). Added a drop-before-resolve abort regression test. - loop_driver_host: delivery_tools_visible requires BOTH outbound delivery capabilities (list + set) before rendering guidance that names both tools, so a setter-only profile no longer prompts the model to call an unavailable lister. Caller-level test for the setter-only suppression. - triggers: direct test for is_trusted_trigger_adapter_kind. - composition: rename OUTBOUND_PREFERENCES_TIMEOUT → COMMUNICATION_CONTEXT_FETCH_TIMEOUT (now governs the whole communication fetch, not just delivery prefs). - product_context AGENTS.md: stop pointing at a nonexistent crate-local CLAUDE.md; point at root guardrails + .claude/rules. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(context-slice): address CodeRabbit round on 2011dd9 - runtime_context: `CommunicationContextFetch::resolve` awaits the handle via `as_mut()` instead of moving it out, so a `resolve` future dropped mid-await still lets `Drop` abort the task (the `take()` version detached on cancel). - communication_context: rewrite the abort-on-drop regression to use a drop guard whose `Drop` fires only when the task future is dropped (abort), so the test actually fails if abort-on-drop regresses (prior version's "completed" flag stayed false whether aborted or merely parked — false positive). - triggers: move `mod tests` to the bottom of trusted_submit.rs per repo layout. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(context-slice): address review round on 6374bf3 - turns/status: RunOriginAdapter error text said "1..=256 bytes" but the bound is now 512; corrected to 512 (with a comment to keep it in sync with MAX_RUN_ORIGIN_ADAPTER_BYTES). - composition: add send_user_message_renders_webui_origin_in_model_request, asserting the runtime send path renders "Run origin: WebUI chat; replies render in this chat." into the model request (not just persisted owner). - .gitignore: drop the unrelated .codegraph/ entry added on this branch (tooling artifact, out of scope for this feature). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(context-slice): untrack accidentally-committed .codegraph/ state `.codegraph/daemon.pid` and `.codegraph/.gitignore` (machine-local CodeGraph daemon state) were swept into 2dfb5b9 by `git add -A` after the root `.gitignore` `.codegraph/` entry was removed. Restore the gitignore entry and untrack the directory — daemon PID/socket state must not be committed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * most working * working * feat(reborn): attachment web UX on the WebChat v2 SPA (nearai#4644) (nearai#4738) * feat(reborn): attachment web UX on the WebChat v2 SPA (nearai#4644) Wires the upload UX into the Reborn WebChat v2 SPA. The backend stack (registry, contract, ingress upload, MountView landing, extraction, context folding) already lands attachments and the timeline already returns their refs; the gaps were the frontend send/staging/render path and exposing the registry's accept= list. Backend: - product_workflow: WebUiAttachmentCapabilities + webui_attachment_capabilities() (registry accept tokens + the 10-file / 5 MiB / 10 MiB budgets decode_attachments enforces); re-exported. - webui_v2: GET /session returns an `attachments` block from that helper, so the SPA picker derives accept= from the server. No static frontend list to drift (kills the four-list class by construction). Frontend (ironclaw_webui_v2_static): - lib/attachments.js: stageFiles (file -> base64 + preview, validated against the server contract), formatBytes, isAcceptedFile, wire/render mappers. - hooks/useAttachmentConfig.js: reads session.attachments via the cached query. - chat-input.js: dead attach button -> real "+" picker with paste/drop staging, removable preview chips, capability-driven validation. - useChat.js: drop the unsupported-payload throw; staged files flow through as WebUiInboundAttachment and onto the optimistic bubble. - api.js: sendMessage carries attachments. - history-messages.js: project record.attachments into render cards so they survive refresh / thread switch (nearai#3272). - message-bubble.js: image thumbnails for cards with a preview. - en.js: real validation strings. Tests: - webui_v2 handler contract: /session advertises the registry accept + budgets. - product_workflow: get_timeline returns attachment refs on the user message (refresh-survival at the backend tier). - JS unit: stageFiles/limits/mappers, timeline->card projection; the two old useChat "reject" tests rewritten into acceptance tests. - e2e (test_reborn_gateway_smoke): v2-native land+persist+timeline, extracted-text-reaches-model (marker + canned mock reply), oversize reject, and a browser upload/refresh test (skip-guarded on webui-v2-beta, per the existing test_v2_* convention). - build.rs: exclude *.test.mjs (not just *.test.js) from the embedded bundle. Deferred (per nearai#4677): image pixels to the vision model and audio transcription; no attachment-bytes endpoint so post-refresh images render as cards; attachment-only sends (v2 requires non-empty content). * fix(webui-v2): handle */* accept token and skip-don't-abort on total-budget Addresses gemini review on the attachment staging helpers: - isAcceptedFile now treats the universal */* (and *) accept tokens as accept-anything, matching standard HTML file-input accept semantics. - stageFiles skips a file that would exceed maxTotalBytes (continue) instead of break, so a later smaller file can still fit the remaining budget; the over-budget notice is de-duplicated. - Two new unit tests lock both behaviours. * fix(reborn): land attachments through a read-write workspace mount The WebUI attachment lander wrote through rt.workspace_filesystem, which is intentionally read-only (it backs setup-marker reads — see local_dev_setup_marker_workspace_filesystem_is_read_only). Every upload therefore failed closed with PermissionDenied, surfaced to the browser as a bare 500 'Internal' with no detail because the lander's map_err discarded the underlying error. - webui_workspace_filesystem() now builds a read-write ScopedFilesystem over the same root (extension_filesystem) using the read-write workspace_mounts the agent's file_read/file_write tools resolve through, so a landed attachment is addressable at its recorded storage_key. - The lander now logs the underlying AttachmentLandingError (warn) before mapping to the sanitized 500, so a failure is diagnosable in the CLI. * observability: never let a sanitized 5xx leave the gateway un-logged Three layers so an internal error like the read-only attachment mount can't recur as a bare 'Internal' 500 with nothing in the CLI: 1. Boundary net: WebUiV2HttpError::into_response_parts now logs every server error (5xx) with code/kind/status/retryable. Surfacing internal errors is now a property of the single HTTP egress point, not of each call site remembering to log before it maps. 2. Carry the cause: new RebornServicesError::internal_from(source) logs the underlying backend error and returns the sanitized 500 — the cause is logged (never serialized to the client). The attachment lander uses it instead of map_err(|_| …Internal…) + a manual warn. 3. Guard the regression: .claude/rules/error-handling.md now flags map_err(|_| …) (a closure discarding the error binding) as a silent-failure anti-pattern alongside unwrap_or_default()/.ok()?, requiring the cause be carried/logged or annotated // silent-ok. Verified: ironclaw_webui_v2 handler contract 51/51, lander read-only test still maps to Internal, all three crates compile warning-free. * address CodeRabbit review on the attachment web UX - chat-input.js: serialize addFiles() staging through a queue + attachmentsRef so overlapping async stageFiles calls validate against the latest set (no over-admitting past the per-message budget). - useAttachmentConfig.js: filter non-string accept tokens so isAcceptedFile's token.trim() can't throw and break the picker. - reborn_services_contract.rs: assert the timeline ref's storage_key is non-empty (not just Some). - test_reborn_gateway_smoke.py: drop the unconditional skip on the attachment card e2e; gate on the _require_v2 runtime probe like its siblings; use the chat-composer testid. - webui_v2 handlers contract: registry accept tokens are explicit extensions (.png/.pdf/...), not image/* wildcards — assert an image extension. * fix(webui-v2): restore .test.mjs assets, complete attachment i18n, review fixes CI (Test ironclaw_webui_v2_static was the fail-fast root): - build.rs no longer excludes `*.test.mjs` from the embedded asset table. main reuses the asset table as a fixture for the `assets.rs` caller-level JS regression tests (asserting `.test.mjs` content via `asset_text`); this branch had unilaterally excluded `.test.mjs`, breaking those tests on rebase. - Complete the attachment i18n: the obsolete `chat.attachmentsUnsupported` ("not supported yet") key is replaced by the 8 granular `chat.attach*` keys in en, but the 10 other locales still had the stale key and lacked the new ones (locale key-set parity test failed). Translate all 8 (+ a new `chat.attachmentStagingFailed`) into ar/de/es/fr/hi/ja/ko/pt-BR/uk/zh-CN. Review: - chat-input addFiles: skip staging when the composer is disabled, and `.catch` the staging chain so an unexpected failure can't permanently reject the shared queue and drop every later add (surfaces a generic error). - e2e: target `input[type=file][multiple]` (the composer picker) so the test can't attach to the ambiguous Settings file input. - error-handling rule: `map_err(|_| …)` is not silent-ok-exemptible — a comment doesn't make the dropped cause reappear; carry or log it instead. * fix(attachments): advertise exact MIME types in the picker accept set The file picker's `accept` attribute was built from extension-only registry tokens (`.png,.pdf,…`). On macOS that makes Chromium hand NSOpenPanel an extension-based `allowedFileTypes` filter that renders folders non-navigable — double-clicking a folder dismisses the picker instead of opening it. Emit the exact MIME type alongside each extension (`image/png,.png,…`); MIME types map to UTIs that keep directory navigation working. Exact, never `image/*` wildcards, so the advertised set still equals the supported set; the JS validator already matches exact-MIME tokens. Registry + webui_v2 session tests updated. * fix(webui-v2): keep staged attachments across composer navigation The composer persisted the text draft across navigation (draft-store) but reset `attachments` to `[]` on every mount, so attaching a file on the new-chat screen, leaving, and returning silently dropped the file while the text came back. Give attachments a parallel per-key draft store. It is in-memory (not localStorage) because the staged files carry base64 bytes that would blow the ~5MB quota — so they survive SPA navigation but not a full page reload, which is the right trade-off for unsent files. The composer initializes `attachments` from the store, persists on change, re-reads on a conversation switch (with a prev-key guard so one conversation's files can't leak into another), and clears on a successful send. Sign-out drops the in-memory store too. Regression: assets.rs locks the draft-store exports and the chat-input wiring. * style: rustfmt the accept_tokens wildcard assertion * fix(webui-v2): stop serving *.test.mjs; address composer review nits - build.rs again excludes `*.test.mjs` from the embedded/served asset table, so Node unit tests are not shipped to /v2 clients. The 3 `assets.rs` regression tests that assert `.test.mjs` content now read it from disk (`source_text`), not the table — keeping the coverage without the exposure. - WebUiAttachmentCapabilities.accept doc no longer shows `image/*` wildcards; the registry advertises exact MIME types + extensions. - chat-input: don't show the drop overlay while the composer is disabled (onDragOver guards on `disabled`); keep `attachmentsRef` in lockstep on the remove and post-send paths so a same-tick add validates the current set. * fix(webui-v2): guard readAsDataUrl against non-string FileReader result A FileReader that resolves with a non-string result (null/ArrayBuffer) would crash splitDataUrl's .indexOf and break attachment staging. Guard the contract in readAsDataUrl so a non-string result rejects and is surfaced as chat.attachmentReadFailed instead. Regression test added in attachments.test.mjs (node --test); the hook only detects Rust tests. [skip-regression-check] * working * handlers * improve * rename * improvements to chat * improve stream * better conversation api --------- Co-authored-by: Henry Park <henrypark133@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Illia Polosukhin <ilblackdragon@gmail.com>
theredspoon
pushed a commit
to theredspoon/ironclaw
that referenced
this pull request
Jun 21, 2026
…nout) (nearai#4843) * fix(slack): post each gate prompt once by single-flighting delivery per run Each gate-resolution ack (ApprovalResolution(Allow) / AuthResolution(Allowed)) carries the same submitted_run_id as the original user-message ack because it resumes the pre-existing run. should_deliver_after_ack returned true for Allow/Allowed, so every resolution ack spawned a fresh deliver_final_reply loop (L2) with delivered_blocked_marker = None. The original loop (L1) was still alive polling the same run_id; once the resolution made the run terminal, both L1 and L2 called post_slack_message for the final reply. N resolutions produced N+1 concurrent loops and gate N was posted N times. Fix: add active_delivery_run_ids (Mutex<HashSet<TurnRunId>>) on SlackFinalReplyDeliveryObserver. Before calling deliver_final_reply, observe_workflow_ack inserts the run_id and returns early if it was already present. The guard is released after deliver_final_reply exits so a future retry is never permanently blocked. Option (a) over (b): the single-flight guard is robust to future ack variants that target an existing run without requiring changes to should_deliver_after_ack, and makes the invariant visible at the dispatch site rather than as a filter predicate. Regression: gate_prompt_is_posted_exactly_once_when_approval_ack_races_ live_delivery_loop confirms exactly 2 messages (prompt + final reply). Pre-fix count: 3 (prompt + 2 duplicate final replies). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(slack): lock single-flight guard ordering against the AuthResolution fanout The single-flight delivery guard (one live loop per run_id) was added to stop N gate resolutions spawning N+1 loops that each re-post the gate. Two regression gaps remained: - No unit test pinned the TOCTOU ordering — that the guard is checked and the run_id inserted BEFORE the delivery semaphore permit is acquired. `concurrent_ack_for_same_run_id_is_rejected_before_acquiring_permit` drives a blocked first delivery holding the only permit (max_concurrent_deliveries = 1) and asserts a second ack for the same run_id returns promptly via the guard skip instead of blocking on the semaphore. If the guard were checked after permit acquisition the second call would block and the test's timeout would elapse. - The original PR comment named `AuthResolution(Allowed)` as an equal trigger to `ApprovalResolution(Allow)`, but only the approval path was covered end-to-end. `auth_prompt_is_posted_exactly_once_when_auth_resolution_ack_races_live_delivery_loop` drives the full Slack ingress → blocked-auth turn → live delivery loop, injects an `AuthResolution(CallbackCompleted)` ack carrying the original submitted_run_id (the WebUI gate-resolve path, which has no Slack text syntax), and asserts exactly one auth prompt + one final reply — no duplicate post. No production change; both tests pass against the in-place guard fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
theredspoon
pushed a commit
to theredspoon/ironclaw
that referenced
this pull request
Jun 21, 2026
… re-approval loop) (nearai#4839) * docs: import approval-invocation-identity fix plan Add Fix A plan covering auth-resume invocation identity preservation so one-shot approvals survive the approval+auth gate sequence. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(agent-loop): extend PendingAuthResume with prior approval identity Add optional `resume_token` and `approval_request_id` fields to `PendingAuthResume` so the auth-gate executor can propagate original invocation identity when the invocation previously passed a one-shot approval. - Gates stage now extracts approval identity from `GateInput.approval_resume` before moving it into `PendingApprovalResume`, then writes both fields into the new `PendingAuthResume` slot so auth re-dispatch carries them. - New compat test `pending_auth_resume_without_resume_token_fields_decodes_to_none` covers forward and backward serde compatibility: round-trip with fields set passes; JSON stripped of the fields decodes to `None` (pre-existing checkpoints decode cleanly). - All existing `PendingAuthResume` constructors updated with `None` defaults. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(turns): add CapabilityAuthResume and CapabilityInvocation.auth_resume field Add CapabilityAuthResume struct carrying the original resume_token and an optional approval_request_id. Add auth_resume: Option<CapabilityAuthResume> to CapabilityInvocation so the host port can distinguish auth-resume re-dispatch (preserving original invocation identity) from a fresh invoke. Backward-compat: serde(default, skip_serializing_if = "Option::is_none") ensures existing in-flight checkpoint payloads decode with auth_resume = None. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(capabilities,host-runtime): add auth-resume capability path ironclaw_capabilities: - Add CapabilityAuthResumeRequest with optional approval_request_id - Add CapabilityHost::auth_resume_json(): validates BlockedAuth status, claims approval lease when approval_request_id is Some, skips lease step when None, then authorizes and dispatches normally ironclaw_host_runtime: - Add RuntimeCapabilityAuthResumeRequest with idempotency_key + approval_request_id - Add HostRuntime::auth_resume_capability() with default fallback to invoke_capability - Override auth_resume_capability in DefaultHostRuntime: evaluates trust, enforces runtime policy, delegates to host.auth_resume_json() Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(loop-support,agent-loop): wire auth-resume dispatch through capability port ironclaw_loop_support / capability_port: - Add CapabilityAuthResume + RuntimeCapabilityAuthResumeRequest to imports - Add dispatch_runtime_capability_auth_resume() routing function - In invoke_capability: when request.auth_resume is Some, restore original invocation_id from resume_token (preserving approval-lease scope), and route through dispatch_runtime_capability_auth_resume rather than the normal invoke path; log at tracing::debug with invocation_id + approval_request_id ironclaw_agent_loop: - Export capability_invocation_from_auth_resume_candidate from executor.rs - In capability_helpers: add capability_invocation_from_auth_resume_candidate() that sets auth_resume from PendingAuthResume fields and approval_resume = None - In capabilities.rs batch builder: check pending_auth_resume first; when the re-dispatched call matches, use auth_resume path so invocation_id is preserved Test fixtures: add auth_resume: None to all CapabilityInvocation struct literals across loop_support, hooks, reborn, and reborn_composition test files; remove spuriously-added auth_resume from CapabilityOutcome::ApprovalRequired and GateInput literals (sed artifact). Adds auth_resume_preserves_invocation_id_and_approval test to ironclaw_reborn/tests/loop_driver_host.rs (red→green). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(tests): update missed CapabilityInvocation initializers and clippy warning Workspace-wide check caught two root-crate test initializers missing the new auth_resume field and one clone-on-Copy in the state round-trip test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(loop-support): give auth-resume dispatches a distinct idempotency key The invocation idempotency key only distinguished approval-resume from first dispatch, so an auth re-dispatch hashed to the same key as the original call and replayed its cached ApprovalRequired outcome instead of dispatching — re-blocking the run on an approval that was already granted. The new port-level lifecycle test (approval gate -> approval resume -> auth gate -> auth resume) caught this and now pins both the distinct-key behavior and the preserved invocation identity reaching the runtime. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(capabilities): harden auth_resume_json validation per review - Add capability_id consistency check in auth_resume_json (host.rs): mismatched run record capability_id returns ResumeContextMismatch, matching the pattern already enforced in resume_json. - Change default auth_resume_capability trait impl (host_runtime) from silent fallback-to-invoke to an explicit Unavailable failure; removes a path where unimplemented runtimes could silently misbehave. - Carry correlation_id through the auth-resume lifecycle: add field to PendingAuthResume and CapabilityAuthResume, thread through capability_invocation_from_auth_resume_candidate, and restore onto InvocationContext in capability_port auth-resume branch. - Remove .unwrap() in capabilities.rs: restructure is_some_and+unwrap to if-let-filter on pending_auth_resume. - Rename complete_run_after_side_effect label "auth_resume_dispatch" to "dispatch" to match sibling paths (host.rs line ~1232). - Extract to_approval_resume() on PendingApprovalResume; replace both manual CapabilityApprovalResume field constructions in capabilities.rs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: cover auth-resume review findings - auth_resume_json_rejects_capability_id_mismatch_against_run_record: starts a run with capability A, attempts auth_resume with capability B; asserts ResumeContextMismatch and zero dispatches. - auth_resume_json_rejects_approval_not_yet_approved: inserts a Pending approval record, calls auth_resume with that approval_request_id; asserts ApprovalNotApproved { status: Pending } and run stays BlockedAuth. - auth_resume_json_returns_store_missing_when_approval_requests_absent: host has run_state but no approval_requests store; calling auth_resume with an approval_request_id asserts ResumeStoreMissing { store: "approval_requests" }. - capability_invocation_from_auth_resume_candidate_with_none_resume_token_sets_auth_resume_none: unit test confirming that a PendingAuthResume with no resume_token produces a CapabilityInvocation with auth_resume None. - Update auth_resume_after_approval_reuses_original_invocation_identity to pass and assert correlation_id is preserved through the auth-resume path. - Fix PendingAuthResume initializers in executor tests to supply the new correlation_id field (None). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(capabilities): cover approval lease survival across real auth bounce Adds `auth_resume_after_real_approval_bounce_reuses_claimed_lease` to capability_host_auth_resume_contract.rs. This test drives the complete real ordering — invoke_json → approve → resume_json (dispatcher returns AuthRequired) → auth_resume_json — and asserts the five post-fix invariants: (a) lease is Claimed (not Revoked) after the resume_json auth bounce (b) auth_resume_json succeeds and dispatches (c) approval request remains Approved (d) lease is Consumed after successful dispatch (e) capability was dispatched with the same invocation_id as the original The test currently FAILS at assertion (a): lease status is Revoked, not Claimed. This proves the bug is real: resume_json unconditionally revokes the claimed lease on a dispatch-error path even when the error is a non-terminal BlockAuth transition (AuthorizationRequiresAuth). Also renames `auth_resume_json_with_approval_request_id_claims_lease_and_dispatches` to `auth_resume_json_with_approval_request_id_claims_active_lease_and_dispatches` to clarify that it tests the clean-ordering shortcut path (Active lease), not the real bounce path covered by the new test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(capabilities): keep claimed approval lease across non-terminal auth bounce When resume_json dispatches and gets AuthorizationRequiresAuth back, the run transitions to BlockedAuth — a non-terminal state. Previously the code unconditionally revoked the approval lease at the dispatch-error site, leaving it Revoked. Subsequent auth_resume_json then called matching_approval_lease (Active-only) and got ApprovalLeaseMissing, breaking the entire approval→auth flow. Part A (resume_json): guard the two post-claim revoke sites so revoke is skipped when the error carries a BlockAuth run-state transition. The lease stays Claimed, keeping it recoverable. Part B (auth_resume_json with-approval path): after failing to find an Active lease via matching_approval_lease, fall back to matching_claimed_approval_lease_for_auth_resume which scans leases_for_scope for a Claimed lease with matching capability and invocation fingerprint. Reuse it directly instead of returning ApprovalLeaseMissing. Consume on success, revoke on terminal failure. Adds matching_claimed_approval_lease_for_auth_resume helper in helpers.rs and is_block_auth_transition predicate in host.rs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(turns): drop forbidden InvocationId identifier from CapabilityAuthResume doc The Reborn architecture gate (reborn_turns_public_surface_uses_turn_ids_not_ runtime_or_process_ids) forbids the runtime/process identifier `InvocationId` in the ironclaw_turns public surface. A doc comment referenced it; reworded to 'invocation identifier' without the forbidden token. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(agent-loop): consume pending_auth_resume on first batch match `pending_auth_resume` was read with `as_ref().filter(…)` (non-consuming), so a batch with two calls to the same capability_id would tag both as auth-resume, reusing one resume_token/invocation_id across distinct calls — a correctness and security bug. Mirror the approval path: use `take_if` so the slot is consumed on the first matching call and subsequent calls in the same batch dispatch normally. Regression test drives CapabilityStage directly with two calls to the same capability_id and asserts only the first carries auth_resume. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(capabilities): consume/revoke auth-resume lease mirroring resume_json `auth_resume_json` claimed or reused a capability lease but never consumed it on success nor revoked it on terminal failure — unlike `resume_json` which does both. Mirror `resume_json`'s lease lifecycle: - Success path: consume the lease via `claimed.grant.id`. - Obligation-error and dispatch-error branches: revoke the lease, but only when the error is NOT a non-terminal BlockAuth transition (guard via `is_block_auth_transition`) so a re-block leaves the lease Claimed for the next auth-resume attempt. - Completion-obligation-error branch: revoke unconditionally (terminal). Regression tests assert: - Lease is Consumed after successful auth-resume dispatch. - Lease is Revoked after a terminal dispatch failure. - Lease stays Claimed after a non-terminal BlockAuth re-block. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(host-runtime): fail BlockedAuth run on auth-resume preflight rejection `auth_resume_capability` returned early on `enforce_runtime_policy` rejection and `evaluate_invocation_trust` failure without transitioning the durable BlockedAuth run record, leaving a stale resumable gate after the caller saw a terminal failure. Mirror `resume_capability`: add `fail_matching_blocked_auth_resume_on_preflight_error` (analogous to the existing `fail_matching_blocked_resume_on_preflight_error`) and call it at both preflight-failure return sites in `auth_resume_capability`. The new helper matches runs in RunStatus::BlockedAuth (not BlockedApproval) and additionally filters on the optional approval_request_id carried by auth-resume state. Regression test drives `auth_resume_capability` against a broken registry (empty → trust eval fails) and asserts the matching BlockedAuth run transitions to Failed while a wrong-scope call leaves the run untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(host-runtime): expect preserved Claimed lease on missing-secret auth bounce The resume-missing-secret contract test pinned the pre-Option-2 behavior (lease Revoked on the auth bounce). Option 2 intentionally preserves the claimed lease across a non-terminal BlockedAuth bounce so the same invocation reuses it on auth-resume without a second approval. Update the assertion to the new contract; the terminal-failure revoke paths remain covered elsewhere. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(agent-loop,turns): model prior-approval identity as one typed value Collapse the two formerly-independent optional fields `approval_request_id: Option<ApprovalRequestId>` and `correlation_id: Option<CorrelationId>` in `PendingAuthResume` and `CapabilityAuthResume` into a single typed struct `AuthResumeApprovalIdentity { approval_request_id, correlation_id }`. The pair is all-or-none: both sub-fields are present only when the invocation previously passed a one-shot approval gate. Modelling them as a single optional struct makes the compile-time invariant explicit and removes the risk of one field being Some while the other is None. `AuthResumeApprovalIdentity` is defined in `ironclaw_turns` (the neutral contract crate) and re-exported from `ironclaw_agent_loop::state` for use in the executor. The forbidden `InvocationId` PascalCase token does not appear anywhere in `ironclaw_turns`. All construction and read sites updated: gates.rs (builds prior_approval from input.approval_resume), capability_helpers.rs (capability_invocation_from_auth_resume_candidate), capability_port.rs (restores invocation id from resume_token, correlation from prior_approval), and test literals. Serde `#[serde(default, skip_serializing_if = ...)]` on the field preserves backward compat with existing checkpoints. Also extract the shared resumed-dispatch helper `dispatch_resumed_capability` in `ironclaw_capabilities::host`. `resume_json` and `auth_resume_json` previously ran identical sequences (authorize → claim/reuse lease → prepare_obligations(Resume) → dispatch_json → complete_dispatch_obligations → consume lease → complete_run_after_side_effect) with copy-pasted error handling. The shared tail is now owned exactly once in the private `dispatch_resumed_capability` method, parameterized by `ResumedDispatchParams`. The original claim-after-authorize order for `resume_json` is preserved via `PendingClaimAfterAuth` so that authorization denial keeps the lease Active (no behavior change). resume_json: 379 → 184 lines; auth_resume_json: 423 → 235 lines; dispatch_resumed_capability helper: 255 lines (new); host.rs total: 2144 → 2055 lines. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: close auth-resume coverage gaps - ironclaw_agent_loop: assert pending_auth_resume cleared after first batch match; assert correlation_id carried through approval→auth bounce; add 3-phase test verifying original correlation_id survives the full approval→auth-block→auth-resume pipeline - ironclaw_host_runtime: add happy-path auth_resume_dispatches_blocked_auth_run (3-phase: invoke→approval gate, approve+resume→BlockedAuth, add credential+auth_resume_capability→Completed) - ironclaw_capabilities: add concurrent_auth_resume_claim_loser_returns_lease_error_without_failing_run (ClaimFailingLeaseStore simulates CAS race loser; asserts Lease error returned and run stays BlockedAuth, not Failed) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(capabilities,agent-loop): replace dual-Option lease fields with 3-state enum; add both-present auth-resume test Fix 1 — replace `ResumedDispatchParams` two mutually-exclusive `Option` fields (`pending_claim`, `claimed_lease`) with a single `ResumedLeaseState` enum that encodes all three valid states at the type level: * `PendingClaim(PendingClaimAfterAuth)` — `resume_json`: claim the Active lease after `authorize_dispatch_with_trust` returns Allow, so a Deny leaves the lease Active. * `AlreadyClaimed(&CapabilityLeaseStore, Box<CapabilityLease>)` — `auth_resume_json` with prior approval: the lease was already transitioned to Claimed in the preamble; reuse without re-claiming. * `NoPriorLease` — `auth_resume_json` with no approval_request_id: no lease step needed. The previous doc comment "exactly one of the two is Some" was incorrect: the no-prior-approval path of `auth_resume_json` set both to None. The enum makes `NoPriorLease` explicit and removes the impossible "both Some" case at the type level. Behavior is unchanged. Fix 2 — add a unit test `capability_invocation_from_auth_resume_candidate_with_both_token_and_prior_approval` in `ironclaw_agent_loop::executor::capability_helpers::tests` covering the case where both `resume_token` and `prior_approval` are set. Asserts the resulting `CapabilityInvocation` carries `auth_resume: Some(...)` with the correct resume token and `prior_approval` identity, and `approval_resume: None`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(authorization,capabilities): close Claimed-lease reuse double-dispatch race with Dispatching state Adds a transient `Dispatching` lease status so exactly one concurrent auth-resume can win the reuse race on an already-Claimed approval lease. - `CapabilityLeaseStatus::Dispatching`: new transient state between `Claimed` and `Consumed` for the reuse-CAS window. - `CapabilityLeaseStore::begin_dispatch_claimed`: atomic CAS Claimed→Dispatching; loser gets `InactiveLease{Dispatching}` and bails like a lost Active `claim()`. Implemented in both InMemory and Filesystem stores. - `CapabilityLeaseStore::abort_dispatch_claimed`: reverts Dispatching→Claimed on non-terminal auth re-bounce so the next auth_resume_json call finds the lease again. No-op if already Claimed. - `auth_resume_json` reuse site: replaces plain read-then-dispatch with `begin_dispatch_claimed`; loser returns Lease error without failing run. - `dispatch_resumed_capability` BlockAuth-skip sites (×2): on `is_block_auth_transition`, call `abort_dispatch_claimed` instead of skipping revoke, so a Dispatching lease reverts to Claimed for the next auth-resume attempt. - `ensure_consumable`: adds `Dispatching` to the OK arm alongside `Active | Claimed`; fingerprint guard allows `Claimed | Dispatching`. - `was_claimed` in both `consume` impls extended to `Claimed | Dispatching`. - `claim_error_may_be_concurrent_resume`: adds `Dispatching` to the match arm so the loser warn path fires correctly. Also adds `concurrent_auth_resume_reuse_loser_does_not_double_dispatch` to the auth-resume contract suite: seeds a Claimed lease, arms a `begin_dispatch_claimed` failure, fires auth_resume_json, asserts exactly one dispatch, loser returns Lease error without run failure, lease ends Consumed. Nits: rewrites stale "pre-fix behavior" comment in `runtime_lifecycle_tests.rs` to state the invariant instead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(slack): remove mis-homed approval-then-auth delivery e2e from Fix A `slack_approval_then_auth_resume_completes_without_second_approval` was added to this branch but never passed — it fails at the commit that introduced it (9a7fc2b), so it slid in without the composition suite being run. It does not exercise Fix A's lease/identity changes: it fakes auth completion by advancing the recording coordinator directly, so it depends on the Slack delivery loop surviving the approval→auth gate transition with no re-trigger event. That delivery-loop lifecycle is owned by the single-flight delivery work (nearai#4843), not by the capability-host invocation-identity fix. Fix A's real contract is covered by the green ironclaw_capabilities / ironclaw_host_runtime auth-resume contract tests. Remove the test and its now-unused harness scaffolding (the `BlockApprovalThenAuth` TurnMode variant, its match arm, and the `transition_blocked_approval_to_blocked_auth` helper) so nearai#4839 is green. The approval→auth→final-reply delivery scenario is re-homed onto the delivery PR per the tracking issue. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(hooks): revert orphaned capability-port test additions `crates/ironclaw_hooks/src/middleware/tests/capability_port.rs` is not wired into any module tree (no `mod`, `include!`, or `path =` reference in the hooks crate). The six `auth_resume: None` field additions added on this branch are dead code that cargo never compiles — a false grep/navigation trail. Restore the file to its main-branch content. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(contracts): document auth-resume path and Dispatching lease status capabilities.md: add Auth-gate resume section covering `CapabilityHost::auth_resume_json` preconditions (BlockedAuth guard, capability-id match, context validate), prior-approval lease handling (Active → Claimed search, begin_dispatch_claimed CAS, abort on non-terminal BlockedAuth re-bounce, consume/revoke on terminal outcome), and the host-runtime integration path through `HostRuntime::auth_resume_capability` / `RuntimeCapabilityAuthResumeRequest`. approvals.md: add `Dispatching` to the `CapabilityLeaseStatus` enum, document `begin_dispatch_claimed` and `abort_dispatch_claimed` store operations, note that `consume`/`revoke` accept both Claimed and Dispatching as source states, and extend the §1 flow diagram with the `auth_resume_json` approval-lease sub-flow. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(host-runtime): drop approval_request_id equality guard in fail_matching_blocked_auth_resume_on_preflight_error The `BlockedAuth` run-state transition explicitly clears `approval_request_id` to `None` on the persisted record. The auth-resume request on the approval-then-auth path still carries `approval_request_id = Some(id)` so it can claim the approval lease. The old three-condition guard compared `record.approval_request_id` (always `None` for BlockedAuth records) against `Some(id)`, which was always inequal, causing the function to return early without transitioning the stale BlockedAuth run to Failed. The result: a terminal failure was returned to the caller while the run stayed BlockedAuth and remained resumable (stuck). Fix: drop the `approval_request_id` equality from the guard. `invocation_id` (the `get` key used two lines above) already uniquely identifies the run, so the approval-id comparison was redundant and actively wrong for the approval-then-auth path. The `status == BlockedAuth` and `capability_id` checks are retained. Because `approval_request_id` is now entirely unused in the function body, the parameter is also removed from the signature and its two call sites. Regression test added: `host_runtime_services_auth_resume_with_approval_id_fails_blocked_auth_run_on_preflight_error` — BlockedAuth record with `approval_request_id = None`, auth-resume request with `approval_request_id = Some(id)`, trust preflight rejection → assert run transitions to Failed (not left as stale BlockedAuth). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(capabilities): add four contract tests flagged by PR review - TEST 1: auth_resume_json_returns_store_missing_when_capability_leases_absent Covers the second ResumeStoreMissing branch (missing capability_leases, not missing approval_requests) — previously exercised only by the approval_requests-absent variant. - TEST 2: auth_resume_json_rejected_prior_approval_fails_blocked_auth_run Exercises the non-Pending (Denied) approval branch: asserts ApprovalNotApproved is returned AND fail_run_if_configured transitions the run to Failed. - TEST 3: filesystem_lease_store_persists_dispatching_begin_abort_and_consume Full begin_dispatch_claimed → abort_dispatch_claimed → begin again → consume lifecycle on FilesystemCapabilityLeaseStore with reload assertions at each step. - TEST 4: concurrent_auth_resume_reuse_loser_does_not_double_dispatch Real winner/loser race on begin_dispatch_claimed using a BarrierLeaseStore that synchronises both callers through leases_for_scope before either calls begin_dispatch_claimed, and a GatingDispatcher that holds the winner in-flight. Asserts loser sees InactiveLease{Dispatching} (exact variant+status), run stays BlockedAuth for the loser, and exactly one dispatch completes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * guard(capability-port): fail-closed on both-resume-modes-set invocation Add an explicit `(Some(_), Some(_))` arm before `(Some, _)` in the `invoke_capability` dispatch match so a `CapabilityInvocation` with both `approval_resume` and `auth_resume` set returns `InvalidInvocation` instead of silently falling through to the approval-resume path. The two resume modes are mutually exclusive by design (the executor clears `approval_resume` on `AuthRequired`), so simultaneous presence indicates a malformed invocation. The guard is purely defensive; it is not producible through the normal executor flow today. Added test `invoke_capability_rejects_both_resume_modes_set` that constructs a dual-resume invocation against a live port and asserts the `InvalidInvocation` error with the mutual-exclusion message. The three valid combinations (approval-only, auth-only, neither) are exercised by the existing test suite which remains fully green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(capabilities): reword review-artifact FIX labels to behavior descriptions Replaces "BUG FIX 1b" / "BEFORE FIX / AFTER FIX" review-bookkeeping labels in the auth-resume contract tests with comments that describe the invariant under test (per PR review nit). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(capabilities): advance fresh Active lease to Dispatching before dispatch in auth_resume_json The FRESH Active-lease branch of `auth_resume_json` called `claim()` (Active→Claimed) but left the lease in `Claimed` for the duration of dispatch. A concurrent `auth_resume_json` that raced past the Active lease check could find the `Claimed` lease in the REUSE branch and call `begin_dispatch_claimed` successfully, advancing it to `Dispatching` and double-firing the invocation. Fix: immediately after `claim()` succeeds in the fresh path, call `begin_dispatch_claimed` to advance `Claimed→Dispatching` — the same in-flight single-winner fence that already protected the REUSE path. The shared tail (`dispatch_resumed_capability`) already accepts both `Claimed` and `Dispatching` via `consume`, so no tail changes were needed. Regression test: `concurrent_auth_resume_fresh_active_lease_loser_does_not_double_dispatch` — uses `GatedLeaseStore` to park the fresh-path caller inside `claim()` while the reuse-path caller wins, then asserts the loser returns `Err(Lease(_))` post-fix instead of dispatching a second time. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(capabilities): check capability existence before acquiring approval lease in auth_resume_json In the pre-fix code, `auth_resume_json` mutated the approval lease (Active→Claimed→Dispatching or Claimed→Dispatching via `begin_dispatch_claimed`) **before** verifying that the capability still exists in the registry. If `get_capability` returned `None`, the `UnknownCapability` early-return fired after the lease was already in `Dispatching`, leaving a one-shot approval lease permanently stranded and unrevokable. Fix: move `self.registry.get_capability(...)` to before the approval-lease acquisition block so an unknown capability returns `UnknownCapability` without touching the lease at all. The `descriptor` binding is hoisted to just above the lease block and threaded into `dispatch_resumed_capability` unchanged. `resume_json` is NOT affected: its `get_capability` check already appears before `matching_approval_lease` (no ordering hole). Regression tests added in `capability_host_auth_resume_contract`: - `auth_resume_json_unknown_capability_does_not_strand_active_approval_lease` (Active lease path — pre-fix: stranded in Dispatching) - `auth_resume_json_unknown_capability_does_not_strand_claimed_approval_lease` (Claimed lease reuse path — pre-fix: stranded in Dispatching) Both tests were RED before the fix (`left: Dispatching`) and GREEN after. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(capabilities): add lease_id to concurrent-resume warn and DRY cleanup state machine CHANGE 1 (JYSbT): add `lease_id = %claimed_lease.grant.id` to the concurrent-auth-resume warn! at the `begin_dispatch_claimed` failure site in `auth_resume_json` — matches the adjacent Active-lease race warn field style so production traces can correlate the two races. CHANGE 2 (JYSbX): extract `cleanup_claimed_lease_after_resume_error` private async helper from `dispatch_resumed_capability`. The obligation- failure arm and dispatch-failure arm repeated the same abort-vs-revoke decision; the only difference was the revoke warn message label ("obligation failure" vs "dispatch failure"), now passed as a `revoke_context: &str` parameter. Behavior byte-for-byte identical. CHANGE 3 (JYSbY): extract `apply_begin_dispatch_claimed_transition` and `apply_abort_dispatch_claimed_transition` as `pub(crate) fn` helpers in `ironclaw_authorization`. Both `InMemoryCapabilityLeaseStore` and `FilesystemCapabilityLeaseStore` now delegate their state-transition rules to these shared functions; each store retains its own persistence / CAS mechanics. Mirrors the `ensure_claimable`/`ensure_consumable` pattern. Behavior identical; all 60 authorization and 106 capabilities tests pass unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(capabilities): add missing coverage for auth_resume_json early-exit branches (JYSbO/JYSbP/JYSbQ) Three new contract tests in capability_host_auth_resume_contract.rs: JYSbO — auth_resume_json_returns_store_missing_when_run_state_absent Builds the host without a run_state store and asserts ResumeStoreMissing { store: "run_state" } before any dispatch. JYSbP — auth_resume_json_unknown_invocation_when_run_record_missing Wires run_state but seeds no record for the invocation; asserts RunState(UnknownInvocation) via the ok_or at host.rs ~798. JYSbQ — three approval-request mismatch branches (action / correlation_id / requested_by) via validate_approval_request_matches_invocation called at host.rs ~894. Each test seeds an Approved approval with exactly one mismatched field, asserts ApprovalRequestMismatch { field }, zero dispatch, and run transitioned to Failed (fail_run_if_configured called on all mismatch paths). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(capabilities): normalize resume mode once before context mutation (JYSbV) Introduce a local `ResolvedResumeMode` enum computed from `(approval_resume, auth_resume)` up front, moving the mutually-exclusive guard (`(Some, Some) => InvalidInvocation`) before any `invocation_context` mutation. Previously the both-set check fired inside the dispatch match after context had already been mutated; now it fires immediately, before touching context. The context mutation (invocation_id restore, correlation restore, validate) and the runtime request build are both driven from the same single `resume_mode` value, eliminating the second tuple match. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
theredspoon
pushed a commit
to theredspoon/ironclaw
that referenced
this pull request
Jun 21, 2026
) (nearai#4873) * test(slack): re-home approval→auth→final-reply delivery e2e (nearai#4847) Re-adds slack_approval_then_auth_resume_completes_without_second_approval, removed from nearai#4839 (commit b847f51) as born-broken due to a test-harness gap. With nearai#4843's single-flight delivery guard now in main, the production deliver_final_reply loop survives BlockedApproval → BlockedAuth → Completed on one delivery loop. Adds the BlockApprovalThenAuth TurnMode variant and a transition_blocked_approval_to_blocked_auth coordinator helper to stage the two-gate sequence. Test-only; no production changes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(slack): drive approval→auth hop through the approval caller + deterministic completion (nearai#4847) Addresses PR nearai#4873 review: - Route the approval→auth transition through RecordingApprovalInteractionService::resolve (mode-aware on BlockApprovalThenAuth) instead of a coordinator backdoor; post the approve event and assert exactly one approval-service request. list_pending only surfaces the approval gate while the run is BlockedApproval. Removes the transition_blocked_approval_to_blocked_auth backdoor. Satisfies Test-Through-the-Caller. - complete_blocked_run transitions BlockedAuth→Completed in one locked mutation (no observable Running), removing the window where the delivery loop posts the working indicator and flakes the message-count assertion. - Drop the mid-test drain after the approve event: delivery loops are awaited by drain_immediate_ack_tasks, so draining while the run is still BlockedAuth blocked L1 until max_wait, leaving no loop to deliver the final reply. Poll for the async auth prompt instead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
17 of 22 tasks
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
1 participant
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.
Problem
Bug 3 of the Slack re-approval-loop triage. A gate-resolution ack
(
ApprovalResolution(Allow)/AuthResolution(Allowed)) carries thesame
submitted_run_idas the original user-message ack because itresumes the pre-existing run. The original delivery loop (L1) is still
alive and will observe the unblock on its next poll and post the next
gate / final reply exactly once.
Before this fix, each resolution ack spawned its own delivery loop:
N resolutions ⇒ N+1 concurrent loops ⇒ gate N posted N times. That is
the user-visible "spanning approval loop" — the same gate prompt
re-posted on every resolution.
Fix
SlackFinalReplyDeliveryObserverkeeps a single-flight set(
active_delivery_run_ids): at most one live delivery loop per run_id.A second ack for an in-flight run_id is rejected before doing any work.
Two ordering/safety properties matter and are now locked by tests:
inserted before acquiring the delivery semaphore permit. If it were
checked after, a second ack could block on the permit while L1 holds
it; when L1 releases the permit and clears the run_id, the second loop
would wake against a now-empty guard set — the exact race this
ordering closes.
RunDeliveryGuardremoves the run_id onDrop(poison-tolerant), so a delivery error or panic never leaves the
run_id stuck and blocking all future delivery for that run.
Tests
concurrent_ack_for_same_run_id_is_rejected_before_acquiring_permit(unit) — blocked first delivery holds the only permit
(
max_concurrent_deliveries = 1); a second ack for the same run_idmust return promptly via the guard skip, not block on the semaphore.
auth_prompt_is_posted_exactly_once_when_auth_resolution_ack_races_live_delivery_loop(e2e) — full Slack ingress → blocked-auth turn → live loop, then an
injected
AuthResolution(CallbackCompleted)ack on the originalrun_id (the WebUI gate-resolve path). Asserts exactly one auth prompt
ApprovalResolutionvariant was covered end-to-end.
guard_is_released_after_delivery_error_so_subsequent_ack_proceeds(e2e) — delivery timeout releases the guard so a later ack proceeds.
cargo test -p ironclaw_reborn_composition --features slack-v2-host-beta,test-support(65 relevant tests) and
cargo clippyclean.🤖 Generated with Claude Code
Summary by CodeRabbit