fix(auth-resume): preserve input replay across gate boundaries - #4910
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds ChangesAuth-Resume Replay Metadata Threading
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces capability auth-resume replay functionality to avoid resolving stale or already-consumed input references after a user completes authentication. It adds a replay field to CapabilityAuthResume and PendingAuthResume to capture and persist the original runtime input and resource estimate when an auth gate blocks. Feedback suggests propagating the resume_token from prior_approval to auth_resume if it is missing, ensuring the original invocation identity is preserved.
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.
| match auth_resume.as_mut() { | ||
| Some(resume) => { | ||
| resume.prior_approval.get_or_insert_with(prior_identity); | ||
| resume.replay.get_or_insert_with(prior_replay); | ||
| auth_resume | ||
| } |
There was a problem hiding this comment.
If auth_resume is already Some but its resume_token is None, we should propagate the resume_token from prior_approval to ensure the original invocation identity is preserved. This prevents potential issues where the fingerprinted approval lease fails to match due to a missing or newly minted invocation ID.
match auth_resume.as_mut() {
Some(resume) => {
if resume.resume_token.is_none() {
resume.resume_token = prior_approval.resume_token.clone();
}
resume.prior_approval.get_or_insert_with(prior_identity);
resume.replay.get_or_insert_with(prior_replay);
auth_resume
}There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_agent_loop/src/executor/capabilities.rs (1)
152-160:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConsume the approval resume when auth-resume wins.
After an approval→auth block, the gate checkpoint carries both pending auth and approval resume state. This branch consumes only
pending_auth_resume; a second same-capability_idcall in the batch can then take the leftover approval resume at Lines 157-160, reusing a one-shot approval identity across a distinct invocation. As per coding guidelines, “Fail closed for auth, approvals, trust, filesystem containment, network policy, secret leases, runtime selection, and adapter identity.”Proposed fix
if let Some(auth) = pending_auth_resume .take_if(|auth| auth.capability_id == call.capability_id) { + let _ = pending_approval_resume + .take_if(|resume| resume.capability_id == call.capability_id); return capability_invocation_from_auth_resume_candidate(call, &auth); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_agent_loop/src/executor/capabilities.rs` around lines 152 - 160, The auth-resume branch (lines 152-154) only consumes pending_auth_resume but leaves pending_approval_resume in place, allowing a subsequent call with the same capability_id to reuse the approval identity, which violates the fail-closed security principle for approvals. After the auth-resume is taken in the first branch, also consume the pending_approval_resume by calling take_if or similar to clear it, ensuring the one-shot approval identity cannot be reused across a distinct invocation in the batch.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_agent_loop/src/executor/capabilities.rs`:
- Around line 941-945: The code in the match block where auth_resume is
Some(resume) has a consistency issue: line 943 makes the checkpointed
prior_approval.resume_token authoritative, but lines 944-945 use
get_or_insert_with which preserves any existing prior_approval and replay values
in the resume. This creates a mixed state where the token comes from one
approval while the identity and replay fields may come from a different source.
Either replace the get_or_insert_with calls to directly assign the checkpoint's
prior_approval and replay values to ensure all fields come from the same
authoritative source, or add validation logic to return a contract error if the
existing fields differ from the checkpoint values. Given the requirement to fail
closed for auth and approvals, the safer approach is to make the checkpointed
prior approval the sole source of truth by directly assigning all its
constituent fields.
---
Outside diff comments:
In `@crates/ironclaw_agent_loop/src/executor/capabilities.rs`:
- Around line 152-160: The auth-resume branch (lines 152-154) only consumes
pending_auth_resume but leaves pending_approval_resume in place, allowing a
subsequent call with the same capability_id to reuse the approval identity,
which violates the fail-closed security principle for approvals. After the
auth-resume is taken in the first branch, also consume the
pending_approval_resume by calling take_if or similar to clear it, ensuring the
one-shot approval identity cannot be reused across a distinct invocation in the
batch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c0ed390f-5748-462f-a77d-7fceda427310
📒 Files selected for processing (2)
crates/ironclaw_agent_loop/src/executor/capabilities.rscrates/ironclaw_agent_loop/src/executor/tests.rs
| match auth_resume.as_mut() { | ||
| Some(resume) => { | ||
| resume.resume_token = prior_approval.resume_token.clone(); | ||
| resume.prior_approval.get_or_insert_with(prior_identity); | ||
| resume.replay.get_or_insert_with(prior_replay); |
There was a problem hiding this comment.
Do not checkpoint mixed auth/approval resume identity.
Line 943 makes checkpointed prior_approval.resume_token authoritative, but Lines 944-945 keep any existing auth_resume.prior_approval or auth_resume.replay. If those fields differ, the persisted auth resume has a token from one approval and identity/replay from another. Make the checkpointed prior approval the source of truth here, or return a contract error on mismatch. As per coding guidelines, “Fail closed for auth, approvals, trust, filesystem containment, network policy, secret leases, runtime selection, and adapter identity.”
Proposed fix
Some(resume) => {
resume.resume_token = prior_approval.resume_token.clone();
- resume.prior_approval.get_or_insert_with(prior_identity);
- resume.replay.get_or_insert_with(prior_replay);
+ resume.prior_approval = Some(prior_identity());
+ resume.replay = Some(prior_replay());
auth_resume
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_agent_loop/src/executor/capabilities.rs` around lines 941 -
945, The code in the match block where auth_resume is Some(resume) has a
consistency issue: line 943 makes the checkpointed prior_approval.resume_token
authoritative, but lines 944-945 use get_or_insert_with which preserves any
existing prior_approval and replay values in the resume. This creates a mixed
state where the token comes from one approval while the identity and replay
fields may come from a different source. Either replace the get_or_insert_with
calls to directly assign the checkpoint's prior_approval and replay values to
ensure all fields come from the same authoritative source, or add validation
logic to return a contract error if the existing fields differ from the
checkpoint values. Given the requirement to fail closed for auth and approvals,
the safer approach is to make the checkpointed prior approval the sole source of
truth by directly assigning all its constituent fields.
Source: Coding guidelines
…tures Adopt PR #4910 (preserve input replay across gate boundaries) as the single canonical fix for the auth-gate-resume scope_mismatch, per design decision: - merge origin/main brings in #4910's CapabilityAuthResumeReplay mechanism - (separately) reverted my competing scope-mismatch fix 38aed9a so the two mechanisms don't fight; #4910 is the source of truth - add #4910's new `replay: None` to the rebase_for_run PendingAuthResume test fixture (state.rs) — an --all-features-only constructor site the merge missed - cargo fmt: normalize import-union line wrapping in turn_coordinator_contract.rs Verified --all-features --tests clean: ironclaw_turns / ironclaw_agent_loop / ironclaw_loop_support / ironclaw_hooks / ironclaw_reborn. fmt --check clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
main #4910/Part-1 added the `retryable: Option<bool>` field to TurnLifecycleEvent; the terminal_event test helper in trace_capture.rs predated it. Semantic merge break (textually clean, won't compile under --tests). Set retryable: None to match the other terminal-event test helpers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(runtime-context): key comms preferences by run owner + cover JoinError gaps (#4895)
Addresses post-merge review findings on #4836.
- Bug (Medium): the communication-context provider keyed outbound
delivery preferences by the *actor* instead of the run *owner*. Product
inbound and trusted-trigger runs can carry an explicit thread owner
(subject/creator) distinct from the actor; the stored preference belongs
to the owner. Resolve the caller's user_id via
`scope.explicit_owner_user_id()` with actor fallback — matching
`TurnScope::to_resource_scope` — so shared/channel inbound and trigger
runs render the owner's delivery target, not the actor's. Adds two
regression tests asserting the lookup is keyed by owner vs actor through
a caller-capturing facade.
- Tests (Medium): cover `CommunicationContextFetch::resolve`'s JoinError
branches that were previously unexercised — actorless failure degrades
to `None`, actor-present failure degrades to `Some(Unknown)`.
- Docs (Low): the product-context-factory plan still specified the
rejected 256-byte `RunOriginAdapter` bound; update to the as-built
512-byte cap (mirroring `AdapterKind`) so follow-up work does not
reintroduce the narrowing.
Not addressed (verified out of scope): the `model_safe_label` "injection"
finding is a false positive — label sources are admin/system-set and the
sanitizer strips structural characters (existing hostile-input tests pass);
the shared-route surface assertion finding is already covered by
`shared_user_message_records_channel_surface_type`; the string-based
trigger-helper finding is owned by issue #4851's plan; the
RunOriginAdapter byte-mirror is already mitigated by a named const + docs
+ tests.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix: surface missing-credential auth gate before the approval gate (#4840)
* fix(host_runtime): surface AuthRequired before approval gate on missing credentials (Fix B)
Extracts capability_credential_requirements() as the single source of truth
for credential requirements derived from the capability manifest descriptor.
Both the new credential pre-flight check and the existing dispatch-time
obligation check call this function — no second computation added.
Adds credential_preflight_check() on DefaultHostRuntime and calls it in
invoke_capability() and spawn_capability() BEFORE apply_persistent_approval_policy(),
so AuthRequired is returned without persisting an approval request when a
required credential is absent.
Wires the optional secret store from HostRuntimeServices.build_host_runtime()
into DefaultHostRuntime via with_credential_preflight_store(). The pre-flight
is skipped in minimal/test graphs that don't supply a secret store; the
dispatch-time obligation check remains the enforcement backstop regardless.
Tests (host_runtime_services_contract):
- invoke_capability_missing_credential_returns_auth_before_approval
- invoke_capability_present_credential_proceeds_to_approval
- invoke_capability_no_credential_requirement_proceeds_normally
- credential_requirements_preflight_and_dispatch_agree_on_same_handles
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(host_runtime): correct capability_credential_requirements docstring and strengthen test
The docstring claimed "both the pre-flight check and the dispatch-time
obligation check call this function" — that was false. The obligation
handler in BuiltinObligationHandler derives required handles by iterating
descriptor.runtime_credentials directly; it does not call this function.
Correct the docstring to accurately describe the agreement at source-data
level (both iterate the same required-true entries) and explain why gate-ID
divergence between pre-flight and backstop is moot in practice (pre-flight
fires first when a secret store is wired).
Rename `credential_requirements_preflight_and_dispatch_agree_on_same_handles`
to `credential_requirements_extraction_matches_descriptor_required_credentials`
with a scope note clarifying what the test actually verifies (canonical fn
vs descriptor, not vs obligation handler), add an assertion that
credential_requirements is empty for secret_handle source type, and reference
the caller-level test that covers the gate-ordering guarantee.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(host-runtime): skip credential pre-flight on store errors per review
On a transient SecretStore Err, credential_preflight_check previously
returned AuthRequired (treating backend failure as credential absent),
burning a user auth interaction. Now it returns None on Err so the
dispatch-time obligation check remains the sole enforcement backstop.
Also applied:
- FIX 2: add trust-class-agnostic comment at both pre-flight call sites
- FIX 3: rename field secret_store → credential_preflight_store to match
the with_credential_preflight_store builder
- FIX 4: take registry.snapshot() once in invoke_capability and
spawn_capability and pass it into credential_preflight_check, removing
the redundant internal snapshot
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* test(host-runtime): add credential pre-flight edge-case tests per review
- FIX 5: change section separator to triple-dash style matching
memory_prompt_context.rs
- FIX 6: four new tests:
a. spawn_capability_missing_credential_returns_auth_before_approval —
spawn path mirrors invoke_capability pre-flight behavior
b. invoke_capability_no_credential_requirement_with_wired_store_proceeds_normally —
wired store + zero required credentials hits is_empty() branch, not
no-store early exit
c. invoke_capability_secret_store_error_skips_preflight — erroring
store stub confirms pre-flight skips on Err (FIX 1) and flow
reaches the approval gate
d. credential_requirements_extraction_returns_empty_for_all_optional_credentials —
descriptor with only required=false credentials yields empty vecs
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(host-runtime): address #4840 review — product-auth preflight skip, scope validation, test wiring
- capability_credential_requirements no longer treats ProductAuthAccount injection
slots as presence-checkable secrets, fixing a false-positive AuthRequired preflight
for capabilities with already-connected product-auth accounts.
- Validate context/resource-scope consistency before the credential preflight queries
the secret store, closing a forged-scope presence-probe window (invoke + spawn).
- Wire the preflight store in contract fixtures and seed credentials on the request's
own ResourceScope; add product-auth and scope-validation regression tests.
- silent-ok annotation on the store-error skip; docs name both invoke and spawn;
moved preflight tests to host_runtime_credential_preflight_contract.rs.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(host-runtime): #4840 review round 2 — test fidelity + tighten public surface
- store-error regression now drives the manifest-backed credential backstop via
ApprovalThenGrantAuthorizer (was masked by a test-authorizer-injected obligation).
- add spawn_capability present-credential happy-path test (proceeds to ApprovalRequired).
- make the credential-preflight-store setter test-only/crate-private; tests wire it
through HostRuntimeServices::build_host_runtime.
- stop publishing capability_credential_requirements from the crate root; keep it
crate-private with equivalent coverage.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(host-runtime): exercise the real dispatch-time obligation backstop on store error (#4840)
The store-error regression now grants the required secret so dispatch authorization
passes and the resumed call reaches BuiltinObligationHandler::preflight_secret_injection,
where the AlwaysErrorSecretStore metadata() probe errors and the handler fails closed
(secret_obligation_failed). Previously the grant omitted the secret, so the block came
from grant-matching authorization — not the dispatch-time credential backstop the PR
contract relies on (obligations.rs preflight_secret_injection fails closed on store error).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* refactor(host-runtime): single owner for the secret-presence rule (#4840)
Extract obligations::secret_present as the one definition of 'is this required
secret present in scope', shared by the credential pre-flight (ordering) and the
dispatch-time obligation backstop (enforcement). Removes the duplicated metadata()
presence rule across production.rs and obligations.rs so the two paths cannot drift.
Each caller still owns its store-error policy (pre-flight fails open and skips; the
backstop fails closed). The happy-path double-read remains (documented) — addresses
the split-ownership half of the review; collapsing the read is a separate follow-up.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(host-runtime): make store-error backstop test airtight + cover required product-auth (#4840)
Addresses PR #4840 review round 3:
- The store-error regression now uses a metadata()-call-counting error store and
asserts the resume drove at least one further probe after a counter reset — proving
the resume reaches BuiltinObligationHandler::preflight_secret_injection (authorization
passed) and fails closed there, not at a premature authorization denial. Both paths map
to RuntimeFailureKind::Authorization, so the probe count is the distinguishing signal.
Removes the now-unused AlwaysErrorSecretStore and the stale test doc.
- Corrects the comment wording: the secret-injection obligation comes from grant
evaluation against the manifest, not from ApprovalThenGrantAuthorizer.
- Adds credential_requirements_extraction_excludes_required_product_auth_account unit
test: a required product_auth_account credential is excluded from required_secrets
(no false-positive pre-flight AuthRequired) but still surfaces in credential_requirements.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(host-runtime): preserve secret-store failure cause when failing closed (#4840)
The dispatch-time obligation backstop dropped SecretStoreError with map_err(|_|),
collapsing outages into an opaque secret-obligation failure with no server-side
trail. Bind the error and log it at debug! (SecretStoreError Display carries no raw
secret material) before returning the sanitized secret_obligation_failed(); the
caller still receives the opaque error. Mirror the same cause-logging in the
fail-open pre-flight skip arm.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
* fix(agent-loop): surface resume-origin capability failures instead of dying as scope_mismatch (#4899)
A 2nd+ Slack approval (or auth) resume of a capability could terminally
fail with scope_mismatch ("capability input ref is not scoped to this
loop run"). On a resume dispatch returning a transient Backend error,
handle_capability_error cleared the pending resume slot, then the
RecoveryOutcome::Retry path re-dispatched via
capability_invocation_from_candidate(call, None) — dropping the resume
context. The non-resume path resolved the original-run input_ref against
the resuming run, failed ensure_ref_scoped_to_run, and killed the run as
HostUnavailable.
Intercept Retry for approval-resume AND auth-resume origin failures:
surface the real backend error to the model as a tool result and
continue the loop (so the user can re-approve / re-auth) instead of
re-dispatching. Kills the scope_mismatch (S1) and avoids
double-executing a side effect whose lease is one-shot after a Backend
error (S2).
Adds regression tests for both resume origins.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(webui): keep code block overflow local (#4791)
* fix(webui): keep code block overflow local
* fix(webui): keep code block typography readable after merge
* fix(auth-resume): preserve input replay across gate boundaries (#4910)
* fix auth resume input replay
* fix auth resume approval token carryover
* fix(reborn): normalize bare workspace tool paths (#4846)
* fix(reborn): normalize bare workspace tool paths
* Preserve scoped path URL validation for workspace aliases
* Normalize empty workspace alias segments
---------
Co-authored-by: Robert Yan <mstr.raphael@gmail.com>
* [codex] Represent Slack as a product-adapter extension (#4778)
* feat(reborn): declare Slack channel as extension manifest
* fix(reborn): address extension review feedback
* fix(reborn): address Slack product-adapter extension review feedback
- Reserve "slack" as host-bundled extension id (prevent filesystem shadowing)
- Add builtin_first_party_trust_policy regression test for Slack admin entry
- Activate manifest-backed channel packages from WebUI (suppress only wasm_channel)
- Preserve legacy Slack connect controls for pre-install deployments
- Project only ProductSurfaceKind::ExternalChannel to channel kind
- Parse each manifest once via ExtensionManifestRecord
- Rename product_adapter.host_beta section to stable product_adapter.inbound
- Add caller-level tests: list_extension_registry, extension_info, ChannelsTab render
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* feat(runtime-context): enable connected-channel classification via surface_kinds
#4778 lands the ProductAdapter surface projection, so the lifecycle summary
now carries surface_kinds. Flip CHANNEL_CLASSIFICATION_AVAILABLE to true and
make extension_is_channel_surface a real predicate (ExternalChannel), so
connected channel names render in the model runtime context instead of unknown.
Convert the two stubbed tests to positive cases: empty list -> Known([]),
mixed list -> only active channel-surface extensions reported.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* style(runtime-context): cargo fmt + drop unreachable classification branch
Remove the dead 'if !CHANNEL_CLASSIFICATION_AVAILABLE' arm inside the
Some(Ok(response)) match: lifecycle_fut only issues the ExtensionList call
when classification is enabled, so a present response always means it is on.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(webui-v2): repair Slack extension asset + locale checks after channel refactor
- assets.rs smoke test: assert showLegacySlackConnectActions (the refactor's
built-in Slack status path) instead of the removed slackBuiltinStatus helper
- add extensions.kind.channel to all 10 non-en locales (en gained the key with
the new channel surface kind; locale-parity test requires all locales match)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(webui-v2): cover ExtensionCard channel overflow + localize registry heading
Review findings (PR #4778 review 4493764666):
- F2: add extension-card.test.mjs proving kind=channel/wasm_channel surface
Setup (setup_required/failed) and Reconfigure (active/ready) overflow actions
on the real component; channels-tab.test stubbed ExtensionCard so this was
uncovered.
- F4: render the 'Available channels' registry heading via t(channels.availableChannels)
instead of a hardcoded literal; add the key to all 11 locales. Update the
channels-tab test to locate the registry section by the RegistryCard component
(heading is now an interpolated value, not a template literal); drop the now
unused renderedValueAfter helper.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* refactor(runtime-context): drop permanent classification flag, document surface_kinds cache
Review findings (PR #4778 review 4493764666):
- F5: remove CHANNEL_CLASSIFICATION_AVAILABLE (permanently true after the stub
flip) and run the lifecycle ExtensionList fetch unconditionally when a
lifecycle facade is wired.
- F3: document AvailableExtensionPackage.surface_kinds as an intentional
single-parse cache (re-deriving in summary() would re-run the manifest
projection, undoing the parse-once optimization).
F1 (per-turn ExtensionList cost) accepted as-is: the fetch is spawned off the
critical path under a 500 ms budget with abort-on-drop; caching deferred to a
follow-up.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(webui-v2): suppress Activate for channel kinds during pairing
Review finding (PR #4778 review 4494623523, finding 1): primaryExtensionAction
only suppressed the primary Activate button for legacy wasm_channel, so a
manifest-backed kind=channel Slack card fell through to 'activate' in
pairing_required/pairing states where the dedicated pairing section already
owns the flow. Suppress the primary action for channel-surface kinds in those
states (via isChannelExtensionKind); installed channels still return 'activate'.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(webui-v2): add channels.slack key + regression tests for surface-kind projection
Review findings (PR #4778 review 06:28):
- Add missing channels.slack i18n key to all 11 locales (legacy Slack row
rendered the raw key because the i18n helper returns the key on miss; the
|| "Slack" fallback never fired).
- Add filesystem-path test: a /system manifest with
product_adapter.inbound.surface_kind = external_channel projects to
ExternalChannel surface (previously only the bundled catalog path was covered).
- Add extension_kind regression test: non-channel summaries keep their runtime
wire kind (wasm_tool, mcp_server) while channel surfaces map to "channel".
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(reborn): gate Slack catalog entry behind slack-v2-host-beta; test wasm_channel registry
Review findings (PR #4778 review 07:01):
- Gate the Slack first-party catalog entry, its only-Slack symbols (slack_package,
slack_assets, SLACK_MANIFEST, slack_manifest_digest), the factory trust-policy
Slack AdminEntry, and Slack-asserting tests behind slack-v2-host-beta. Without
the feature the Slack route/runtime/WebUI mounts don't exist, so the catalog
must not advertise an unrunnable Slack extension. Clean clippy + tests in both
feature-on and feature-off configs.
- Add useExtensions hook test proving an uninstalled kind=wasm_channel registry
entry lands in channelRegistry, not toolRegistry (isChannelExtensionKind covers
both channel and wasm_channel).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* refactor(reborn): consolidate Slack trust policy tests (#4778)
---------
Co-authored-by: Henry Park <henrypark133@gmail.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* feat(agent-loop): gated final-answer nudge (reborn empty/canned turn endings) (#4837)
* feat(agent-loop): gated final-answer nudge to avoid empty/canned turn endings
When the reborn loop would otherwise end a turn with no real assistant
answer — an empty/trailed-off reply, the model-call budget exhausted, or
NoProgressDetected (which emits a canned "I stopped repeating the same
step" reply) — issue ONE extra tool-free model call asking the model to
synthesize a closing answer from the work it already did. This is the
reborn equivalent of the legacy loop's on_tool_intent_nudge /
force-text-recovery.
Gated by the (previously unimplemented) SteeringPolicy
`allow_driver_specific_nudges` flag, which defaults to false — so
production behavior is unchanged. Capped at one nudge per run
(`LoopExecutionState.final_answer_nudges_used`) so it can't issue
unbounded extra model calls. Wired into all three exit modes
(assistant_reply empty/trailed, budget IterationLimit, NoProgressDetected);
falls back to existing behavior when disabled, capped, or the model still
declines to answer.
Mechanism note: the tool-free call uses an EMPTY capability_view
(visible_capability_ids: []), not surface_version=None — the reborn model
gateway attaches tools from the capability port regardless of
surface_version, so only an empty view yields a true text-only request.
Evidence (PinchBench, Qwen3.5-122B, claude-haiku judge, controlled A/B on
the same merged tree): nudge OFF 0.682 vs nudge ON 0.768 (+0.086), closing
reborn to v2 parity (0.793). It specifically rescues tasks that do the
work but trip the no-progress detector (stock, events, spreadsheet,
polymarket), turning the canned give-up into a real answer.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* review: route nudge through admission + accounting, drop dead branch, add tests
Addresses review feedback on the final-answer nudge:
- Remove the empty/trailed-reply nudge branch in AssistantReplyStage. As the
reviewer noted, DefaultReplyAdmissionStrategy rejects empty/artifact replies
before they reach AssistantReplyStage, so that branch was dead for the default
family. Empty replies are rejected → the loop continues → NoProgressDetected,
which is where the nudge still fires. assistant_reply.rs is back to main.
- The nudged reply now goes through the SAME admission policy as a normal reply
(ctx.planner.reply_admission().admit_reply) instead of a bare !is_empty()
check — so blank text and provider-transcript artifacts can't be finalized.
- Preserve canonical assistant-reply token accounting: record the nudge turn's
output tokens (provider usage, else the same estimate AssistantReplyStage
uses) into recent_output_token_counts so the diminishing-returns window isn't
fed stale data.
- Add caller-level tests with the gate enabled at the boundaries the nudge
affects: no-progress (synthesizes via one tool-free model call), budget
iteration-limit (completes instead of failing closed), gate-disabled (no model
call, canned fallback), and the one-shot cap (no second call).
All 302 ironclaw_agent_loop lib tests pass.
Note on the remaining structural point (model call still issued from the exit
boundary via the host primitive rather than ModelStage): see PR discussion —
the exit-boundary stages don't own Prompt/Model, so a full "typed stage outcome"
move means restructuring the terminal-exit flow to re-enter the loop for one
tool-free turn. Happy to do that if preferred over this factoring.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* review: address CodeRabbit findings on final-answer nudge
- Move FINAL_ANSWER_NUDGE prompt out of Rust source into
prompts/final_answer_nudge.md, loaded via include_str! (repo prompt-template
invariant).
- Fix stale tool-free comment: clarify that the empty capability view on the
model request (not the surface_version/capability_view None assignments) is
what suppresses provider tools.
- Make MockHost::with_driver_nudges_enabled flip the steering flag in-place so
it composes with other context-level builders regardless of order; drop the
now-unused test_run_context_with_driver_nudges fixture.
- Add legacy-checkpoint decode regression test asserting a payload missing
final_answer_nudges_used decodes to 0 (#[serde(default)] contract).
- Apply rustfmt to the new stage tests.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Pranav Raja <pranav.raja@near.ai>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: serrrfirat <f@nuff.tech>
* Expose outbound delivery targets to Reborn model (#4779)
* feat(reborn): declare Slack channel as extension manifest
* fix(reborn): address extension review feedback
* fix(reborn): address Slack product-adapter extension review feedback
- Reserve "slack" as host-bundled extension id (prevent filesystem shadowing)
- Add builtin_first_party_trust_policy regression test for Slack admin entry
- Activate manifest-backed channel packages from WebUI (suppress only wasm_channel)
- Preserve legacy Slack connect controls for pre-install deployments
- Project only ProductSurfaceKind::ExternalChannel to channel kind
- Parse each manifest once via ExtensionManifestRecord
- Rename product_adapter.host_beta section to stable product_adapter.inbound
- Add caller-level tests: list_extension_registry, extension_info, ChannelsTab render
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* feat(runtime-context): enable connected-channel classification via surface_kinds
#4778 lands the ProductAdapter surface projection, so the lifecycle summary
now carries surface_kinds. Flip CHANNEL_CLASSIFICATION_AVAILABLE to true and
make extension_is_channel_surface a real predicate (ExternalChannel), so
connected channel names render in the model runtime context instead of unknown.
Convert the two stubbed tests to positive cases: empty list -> Known([]),
mixed list -> only active channel-surface extensions reported.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* style(runtime-context): cargo fmt + drop unreachable classification branch
Remove the dead 'if !CHANNEL_CLASSIFICATION_AVAILABLE' arm inside the
Some(Ok(response)) match: lifecycle_fut only issues the ExtensionList call
when classification is enabled, so a present response always means it is on.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(webui-v2): repair Slack extension asset + locale checks after channel refactor
- assets.rs smoke test: assert showLegacySlackConnectActions (the refactor's
built-in Slack status path) instead of the removed slackBuiltinStatus helper
- add extensions.kind.channel to all 10 non-en locales (en gained the key with
the new channel surface kind; locale-parity test requires all locales match)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(webui-v2): cover ExtensionCard channel overflow + localize registry heading
Review findings (PR #4778 review 4493764666):
- F2: add extension-card.test.mjs proving kind=channel/wasm_channel surface
Setup (setup_required/failed) and Reconfigure (active/ready) overflow actions
on the real component; channels-tab.test stubbed ExtensionCard so this was
uncovered.
- F4: render the 'Available channels' registry heading via t(channels.availableChannels)
instead of a hardcoded literal; add the key to all 11 locales. Update the
channels-tab test to locate the registry section by the RegistryCard component
(heading is now an interpolated value, not a template literal); drop the now
unused renderedValueAfter helper.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* refactor(runtime-context): drop permanent classification flag, document surface_kinds cache
Review findings (PR #4778 review 4493764666):
- F5: remove CHANNEL_CLASSIFICATION_AVAILABLE (permanently true after the stub
flip) and run the lifecycle ExtensionList fetch unconditionally when a
lifecycle facade is wired.
- F3: document AvailableExtensionPackage.surface_kinds as an intentional
single-parse cache (re-deriving in summary() would re-run the manifest
projection, undoing the parse-once optimization).
F1 (per-turn ExtensionList cost) accepted as-is: the fetch is spawned off the
critical path under a 500 ms budget with abort-on-drop; caching deferred to a
follow-up.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(webui-v2): suppress Activate for channel kinds during pairing
Review finding (PR #4778 review 4494623523, finding 1): primaryExtensionAction
only suppressed the primary Activate button for legacy wasm_channel, so a
manifest-backed kind=channel Slack card fell through to 'activate' in
pairing_required/pairing states where the dedicated pairing section already
owns the flow. Suppress the primary action for channel-surface kinds in those
states (via isChannelExtensionKind); installed channels still return 'activate'.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(webui-v2): add channels.slack key + regression tests for surface-kind projection
Review findings (PR #4778 review 06:28):
- Add missing channels.slack i18n key to all 11 locales (legacy Slack row
rendered the raw key because the i18n helper returns the key on miss; the
|| "Slack" fallback never fired).
- Add filesystem-path test: a /system manifest with
product_adapter.inbound.surface_kind = external_channel projects to
ExternalChannel surface (previously only the bundled catalog path was covered).
- Add extension_kind regression test: non-channel summaries keep their runtime
wire kind (wasm_tool, mcp_server) while channel surfaces map to "channel".
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(reborn): gate Slack catalog entry behind slack-v2-host-beta; test wasm_channel registry
Review findings (PR #4778 review 07:01):
- Gate the Slack first-party catalog entry, its only-Slack symbols (slack_package,
slack_assets, SLACK_MANIFEST, slack_manifest_digest), the factory trust-policy
Slack AdminEntry, and Slack-asserting tests behind slack-v2-host-beta. Without
the feature the Slack route/runtime/WebUI mounts don't exist, so the catalog
must not advertise an unrunnable Slack extension. Clean clippy + tests in both
feature-on and feature-off configs.
- Add useExtensions hook test proving an uninstalled kind=wasm_channel registry
entry lands in channelRegistry, not toolRegistry (isChannelExtensionKind covers
both channel and wasm_channel).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* refactor(reborn): consolidate Slack trust policy tests (#4778)
* feat(reborn): expose outbound delivery targets to model
---------
Co-authored-by: Henry Park <henrypark133@gmail.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(reborn): unify extension registry flow (#4900)
* fix(reborn): unify extension registry flow
* fix(reborn): address extension registry review comments
* test(reborn): cover merged extension registry flow
* fix(reborn): clean google oauth configure copy
* fix(reborn): align notion oauth setup copy
* fix(reborn): clean token setup copy
* fix(reborn): prefer GitHub extension for repository data (#4894)
* fix(reborn): prefer github extension for repository data
* refactor(reborn): centralize github http routing hint
* fix(reborn): filter PRs from GitHub issue listings (#4888)
* fix(reborn): filter pull requests from github issues
* fix(reborn): preserve github issue pagination
* fix(reborn): use issue-only github search pagination
* fix(reborn): update github wasm trace fixture
* fix: auto-generate BETTER_AUTH_SECRET in bos-dev.sh and track styles.css in git
- bos-dev.sh now generates BETTER_AUTH_SECRET via openssl when .env is created or has an empty value
- Track ui/src/styles.css in git (add ! exception in .gitignore)
- Prevents auth plugin crash and UI build failure on fresh clones
* feat(reborn): polish the Automations panel UI (#4919)
* fix(automations-ui): readable summary cards and NEXT RUN value
Reflow the summary strip to at most three cards per row so the detail
text no longer wraps one word per line, and let StatCard accept a
valueClassName override so the NEXT RUN date renders at a smaller size
instead of truncating to "Jun…". Default StatCard sizing is unchanged.
* fix(automations-ui): surface delivery save errors and gate Slack hint
The delivery-defaults panel swallowed save/clear failures and showed no
feedback; it now renders an inline error from the mutation and flashes the
"Saved" confirmation on Clear as well as Save. The "reply approve <code> in
Slack" footnote is hidden unless an external Slack-style target exists.
* fix(automations-ui): label sub-hourly cron schedules
Minute- and hour-level cadences such as "* * * * *", "*/15 * * * *", and
"0 * * * *" rendered as "Custom schedule" because they have no single clock
time. They now read as "Every minute", "Every 15 minutes", and "Hourly at
:00".
* fix(automations-ui): space the run-row action button icons
The "Open run" and "Logs" buttons in the recent-runs list rendered the
icon flush against the label because the non-primary Button variants don't
add a gap between children. Add the same icon margin the rest of the app
uses for icon+label buttons.
* test(automations): lock the panel UI fixes into the served bundle
Add static-asset assertions driving the composed router so each Automations
panel UX fix — sub-hourly cron labels, summary card reflow + smaller NEXT RUN
value, run-row icon spacing, and delivery save-error/Slack-hint gating — is
guarded against a regression that drops it from the shipped SPA source.
* fix(i18n): add automations.delivery.saveFailed to every locale pack
The new key was added only to en.js, which breaks the i18n consistency test
that requires all locale packs to share the English key set. Add it to the
ten other packs (English placeholder, matching the existing untranslated
automations strings there).
* Explicit gate-open feedback for busy threads (no parking) (#4838)
* feat(product): explicit gate-open feedback for busy threads, no parking
Design decision (supersedes the closed defer-and-drain PR #4812): a message
arriving while another run holds the thread is recorded with the honest
terminal status RejectedBusy and the user gets an explicit notice — gate-aware
("an approval gate is open on this thread — resolve it before continuing,
then resend") when the blocking run is BlockedApproval/BlockedAuth, generic
otherwise. No background resubmission: the user is the retry actor.
- threads: MessageStatus::RejectedBusy + mark_message_rejected_busy (both
backends); DeferredBusy kept as a legacy deserialization label, no longer
written; RejectedBusy -> Submitted allowed so resends work
- product_workflow: ThreadBusy branches mark RejectedBusy; response variant
renamed RejectedBusy with status-derived notice field
- webui: rejected_busy ack renders the notice as a system message in chat;
wire-shape test asserts tag + notice
- slack: copy already honest from #4811; stale #4812 revisit comment removed
- e2e: runtime-level test proves ThreadBusy -> RejectedBusy with notice,
NO resubmission on the blocking run's terminal event, and a fresh submit
succeeds afterward
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(review): make RejectedBusy terminal across replay, ack, compaction, UI
Reviewers found RejectedBusy still inheriting auto-resubmit/defer semantics,
contradicting the no-parking contract. Fixes:
- inbound_turn: from_replay_parts returns a terminal AlreadyRejected handoff
for RejectedBusy (re-rejects, never resubmits); to_ack now emits a settled
ProductInboundAck::RejectedBusy so transport retries get Duplicate instead
of resubmitting. Legacy DeferredBusy rows keep the resubmit path.
- reborn_services: replayed RejectedBusy returns RebornSubmitTurnResponse::
RejectedBusy again (idempotent re-rejection) instead of building a fresh
submission; status-to-notice mapping locked by BlockedApproval/BlockedAuth/
generic tests.
- compaction: RejectedBusy (and frozen legacy DeferredBusy) map to
SkipEphemeral(StableNonModelVisible), not DeferUntilStable, so busy threads
can still compact instead of deferring forever.
- webui useChat: always clear processing on rejected_busy, mark the optimistic
message failed, collision-free system-message id; added hook tests.
- tests: runtime no-resubmission assertion anchored on message identity;
mark_message_rejected_busy negative coverage; webui handler test reuses
StubServices via a queued response.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* refactor(threads): retire mark_message_deferred_busy writer
DeferredBusy is now a read-only legacy label — production writes RejectedBusy.
Remove the live writer from the SessionThreadService trait, both backends, the
Arc forwarder, and all test fakes. Legacy DeferredBusy read/replay coverage is
preserved via a doc-hidden inject_legacy_deferred_busy_for_test back-door on the
in-memory backend (never called from production). The DeferredBusy enum variant
and all read/replay/compaction handling are unchanged.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(review): RejectedBusy follow-ups — Slack hint, honest replay, fail-loud, gating
- slack_delivery: SlackFinalReplyDeliveryObserver now recognizes
ProductInboundAck::RejectedBusy { active_run_id: Some(_) } and posts the
gate-aware busy hint (was DeferredBusy-only, so Slack rejections settled
silently); None active_run_id posts nothing. Tests added.
- reborn_services: RejectedBusy response run metadata (active_run_id, status,
event_cursor) is now Option — fresh ThreadBusy returns Some(real values),
idempotent replay returns None instead of fabricating a fake Running run at
cursor 0. Wire-shape + replay tests updated.
- inbound_turn: RejectedBusy replay fails loud on a malformed stored
turn_run_id instead of silently dropping it.
- compaction: RejectedBusy + frozen legacy DeferredBusy map to
SkipEphemeral(StableNonModelVisible), not DeferUntilStable, so busy threads
compact instead of deferring forever. Integration tests added.
- threads: inject_legacy_deferred_busy_for_test gated behind a test-support
cargo feature (absent from production builds); filesystem contract coverage
for mark_message_rejected_busy (happy + invalid transitions).
- webui useChat: always clear processing on rejected_busy, mark the optimistic
message failed, collision-free system-message id; hook tests added.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* test/docs(review): RejectedBusy coverage, durable timeline rejection, contract docs
- inbound_turn: regression test for fail-loud malformed RejectedBusy turn_run_id
- product_workflow_contract: RejectedBusy(None) settles + transport retry = Duplicate
- webui wire test: assert status + event_cursor present on fresh RejectedBusy path
- thread contracts (both backends): RejectedBusy -> Submitted resend transition
- webui useChat: persisted rejected_busy/deferred_busy rows now render failed on
history reload with durable resend copy (was a normal-looking sent message);
history-messages tests added
- slack_delivery: shared busy-hint path renamed deferred_busy_* -> busy_hint_*
(fns, call sites, logs, docs); DeferredBusy kept only in the legacy arm
- runtime.rs: inline arch justification above the large RejectedBusy e2e test
- docs/reborn/contracts: product-adapters.md + conversation-binding.md document
RejectedBusy as a durable terminal outcome; DeferredBusy marked legacy
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(review): openai-compat build break, Slack duplicate hint, conversations doc
- openai-compat-beta: ProductInboundAck::RejectedBusy added to the four
non-exhaustive match sites (ack_helpers, chat_workflow, responses_workflow x2),
mapped to the same retryable 429 as DeferredBusy — fixes E0004 that broke any
build enabling openai-compat-beta. RejectedBusy->429 tests added.
- slack_delivery: busy-hint run-id extraction now unwraps
Duplicate { prior } recursively, so a transport retry arriving as
Duplicate { prior: RejectedBusy { Some(run) } } still posts the busy-thread
hint when the first was lost; the per-(conversation, run_id) throttle
suppresses genuine repeat posts. Tests added.
- ironclaw_conversations/CLAUDE.md: the idempotency guardrail now distinguishes
transient submit failures (retry, rotate key) from thread-busy admission
(terminal RejectedBusy, no retry-until-submitted, user resends).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(review): RejectedBusy terminal at storage, compaction safety, ledger no-fake-run
- threads: RejectedBusy removed from ensure_user_accepted in both backends —
a stored RejectedBusy row can no longer transition to Submitted; resend is a
fresh message. Prior-pass RejectedBusy->Submitted tests inverted to assert the
transition is now terminal (InvalidMessageTransition). DeferredBusy admission
kept (legacy replay still resubmits).
- compaction: only RejectedBusy (terminal) is SkipEphemeral; DeferredBusy moved
back to DeferUntilStable since legacy rows can still reach Submitted — prevents
a summary silently omitting a message that later becomes model-visible.
- workflow ledger: RejectedBusy { active_run_id: None } maps to
ActionDispatchKind::NoOp instead of minting a fresh TurnRunId; still settles
durably, no fabricated run id.
- inbound_turn test: the misnamed legacy-DeferredBusy test now actually injects a
legacy DeferredBusy row and asserts resubmission, distinct from the RejectedBusy
re-rejection test.
- openai-compat: added cancel-path RejectedBusy->429 handler test.
- slack_delivery: comments corrected to describe the recursive Duplicate{prior}
extraction (no longer claim Duplicate{DeferredBusy} returns None).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(review): non-retryable 429 for terminal RejectedBusy + busy rename + coverage
Address open PR review findings on the busy-thread rejection work:
- openai-compat: split the busy ack arm so terminal RejectedBusy maps to
429 retryable=false (client must issue a new request), while legacy
DeferredBusy keeps retryable=true. Covers chat create, responses create,
and responses cancel paths; regression unit tests on the retryable flag.
- slack_delivery: rename SLACK_DEFERRED_BUSY_* constants to SLACK_BUSY_*
(path now serves RejectedBusy + legacy DeferredBusy); refresh the stale
"silently dropped (pending gate)" comment to cover generic RejectedBusy.
- compaction_task: correct the StableNonModelVisible doc comment — only
terminal RejectedBusy is skipped; legacy DeferredBusy is DeferUntilStable
(it can still transition to Submitted).
- tests: add filesystem legacy DeferredBusy on-disk round-trip coverage and
a reborn_services RejectedBusy mark-failure reconcile-via-replay test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(test): update root busy test to terminal RejectedBusy contract
The root integration test product_workflow_retries_after_filesystem_deferred_busy_release
still asserted the legacy DeferredBusy auto-resubmit contract (busy ->
DeferredBusy -> retry resubmits, submission_count 1->2). The live product
workflow now emits terminal RejectedBusy for busy user messages; the PR
updated crate-level tests but missed this root-level one, failing the
"Reborn root tests" CI job.
Rewrite + rename to product_workflow_rejects_busy_and_does_not_resubmit_on_filesystem_replay,
mirroring crates/ironclaw_product_workflow inbound_turn_contract's
rejected_busy_replay_is_re_rejected_not_resubmitted: first ack is
RejectedBusy (submission_count == 1); a same-event replay settles via the
idempotency ledger and returns Duplicate { prior: RejectedBusy } with no
resubmission (submission_count stays 1).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(compaction): terminal RejectedBusy cut point no longer blocks compaction
Review finding (High): the compaction terminal cut-point validation rejected
every SkipEphemeral disposition with InvalidCutPoint, but RejectedBusy now
classifies as SkipEphemeral(StableNonModelVisible). So a compaction range whose
drop_through_seq landed on a RejectedBusy message hard-failed — contradicting
this PR's goal that terminal RejectedBusy must never block compaction.
Allow a stable-non-model-visible terminal (RejectedBusy) as a legal cut point:
it is excluded from the compacted output like the in-range SkipEphemeral case
and compaction proceeds. Non-User Include and RejectInvalid still error.
Regression test: compaction_port_accepts_terminal_cut_point_that_is_rejected_busy.
Also add legacy_deferred_busy_mark_failure_reconciles_via_replay covering the
reconcile branch's legacy DeferredBusy replay path (RejectedBusy was already
covered).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(compaction): narrow terminal cut-point accept to StableNonModelVisible
Review follow-up: the terminal cut-point arm accepted SkipEphemeral(_) with a
wildcard, which would also admit CapabilityDisplayPreview as a valid terminal.
Only StableNonModelVisible (terminal RejectedBusy) should qualify. Match the
explicit variant so other ephemeral skip reasons fall through to
InvalidCutPoint and fail loud.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(review): reconcile only RejectedBusy as terminal + coverage/naming follow-ups
Address the latest review round (post origin/main merge):
- reborn_services: the mark-failure reconcile path treated legacy DeferredBusy
as a terminal already-settled state. DeferredBusy is non-terminal (a later
replay treats it like Accepted and can resubmit), so claiming terminal over
it violated the no-resubmit guarantee. Drop DeferredBusy from the reconcile
predicate — only RejectedBusy is terminal; a DeferredBusy row now surfaces
the mark failure (503 retryable) instead of a false-terminal RejectedBusy.
Flipped the legacy test to assert the surfaced error.
- compaction: add regression test that a terminal CapabilityDisplayPreview cut
point returns InvalidCutPoint (only StableNonModelVisible is a legal terminal).
- fakes: FakeProductAdapter now records RejectedBusy in accepted_envelopes
(durable like Accepted/DeferredBusy) so fake-based tests don't undercount.
- slack_delivery: arch-exempt comment updated from deferred-busy to busy-thread
/ RejectedBusy terminology.
- reborn_services_contract: retext a busy-submit test that asserts RejectedBusy
but still labeled the path "deferred".
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* docs(test): correct scripted-helper comments — DeferredBusy is non-terminal
Follow-up to the reconcile fix: the DeferredBusyMarkFails scripted helper and
its replay branch still documented the old behavior (DeferredBusy "settles"
reconciliation). reconcile_terminal_duplicate now accepts only RejectedBusy as
terminal, so a DeferredBusy replay surfaces the mark error (Unavailable/503)
instead of a false-terminal RejectedBusy. Comment-only; logic unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* docs(test): align sibling probe-count comments with 3-call reconcile flow
Follow-up: the replay_call_count field doc and rejected_busy_mark_fails() doc
still described the old 2-call probe sequence. Both scripted mark-fail helpers
return None on the first two idempotency probes and Some(..) on the third
(reconcile) probe (count <= 2 guard). Comment-only; logic unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(compaction): reject empty cut-point range instead of summarizing nothing
Review finding (Medium): making a terminal RejectedBusy a legal cut point opened
an edge — a range whose only message is that rejection (or any all-skip-ephemeral
span) produced an empty validated_messages, then still ran inference on an empty
prompt and persisted a meaningless summary artifact.
Guard after the deferred-reason check: if validated_messages is empty, return
InvalidCutPoint before build_input — nothing model-visible to summarize. The
deferred-reason early-return stays first so legitimate deferrals are unaffected.
Regression test: a range whose only message is a terminal RejectedBusy returns
InvalidCutPoint and never calls inference.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test/docs(review): RejectedBusy command ack, ack_helpers, 429 spec, banner
Address review test/doc gaps on the busy-rejection work:
- product_command_workflow_contract: cover the command-dispatch path where
command_service returns RejectedBusy -> UnsupportedActionKind -> terminal
Rejected ack (previously only user-message RejectedBusy was tested).
- ack_helpers: unit-test that internal_refs_from_ack rejects RejectedBusy with
the internal error (no internal refs bound for a terminal busy ack).
- docs/reborn/contracts/openai-compatible-api.md: document the busy 429 split —
terminal RejectedBusy is non-retryable (client must issue a new request),
legacy DeferredBusy stays retryable.
- reborn_services_contract: add the missing opening separator on the Legacy
DeferredBusy test section banner.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(compaction): trim summary span to last visible message at a busy cut point
Review finding (Medium): when the compaction cut point is a terminal RejectedBusy,
the summary span ended at drop_through_seq, covering that non-visible message. The
thread backends' context builder skips any ReplaceRangeWhenSelected summary whose
span covers a non-model-context-visible message (summary_covers_hidden_content), so
the summary was persisted but never applied — a dead artifact.
Trim end_sequence to the last model-visible (Include'd) message's sequence so the
span excludes trailing non-visible terminals; the summary then applies. Folds the
empty-range guard into the same `validated_messages.last()` match (None => empty
range => InvalidCutPoint) — no production unwrap/expect. Regression test asserts a
[visible@1, RejectedBusy@2] range compacted through seq 2 yields a summary spanning
end_sequence=1, not 2.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test: pin command RejectedBusy error to ProductAdapterError::Internal
Review nit: the command-RejectedBusy test used a bare expect_err (any error).
Pin the concrete public variant: ProductAdapterError::Internal — which is what
ProductWorkflowError::UnsupportedActionKind maps to at the adapter boundary. The
kind string ("unsupported action kind: ...") is wrapped in RedactedString and
not exposed via Display, so Internal is the tightest assertable pin from the
public return type.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(threads): summary may span permanently-terminal non-visible messages
Review finding (Medium, egGm): summary_covers_hidden_content blocked a
ReplaceRangeWhenSelected summary whose span covered ANY non-model-context-visible
message. Compaction legitimately spans non-visible rows it skipped from the
summary content (e.g. an interior terminal RejectedBusy, or a capability preview),
so those summaries were silently dropped — the compaction-layer trailing trim
couldn't fix an interior hole.
Block the summary only when the span covers a non-visible message that can still
RESURFACE as model-visible (Draft / Interrupted / Superseded / DeferredBusy).
Permanently-terminal non-visible rows (RejectedBusy, CapabilityDisplayPreview
kind) never resurface, so spanning them is safe — the summary content already
excludes them and they are never shown in context. Identical change in both
in_memory and filesystem backends via a shared can_resurface_as_model_visible
helper; Redacted/Deleted keep blocking. Also corrects the pre-existing
capability-preview span behavior (two tests updated). Regression tests on both
backends: interior RejectedBusy summary is applied; interior Draft is not.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
* fix(reborn): allow read-only GitHub capabilities (#4893)
* fix(reborn): allow read-only github capabilities
* test(reborn): future-proof github approval boundary
* fix(reborn): keep GitHub code search gated
---------
Co-authored-by: Robert Yan <mstr.raphael@gmail.com>
* clean up
* delete
* fix: run scheduled automations and report accurate status (#4920)
* fix(automations-ui): readable summary cards and NEXT RUN value
Reflow the summary strip to at most three cards per row so the detail
text no longer wraps one word per line, and let StatCard accept a
valueClassName override so the NEXT RUN date renders at a smaller size
instead of truncating to "Jun…". Default StatCard sizing is unchanged.
* fix(automations-ui): surface delivery save errors and gate Slack hint
The delivery-defaults panel swallowed save/clear failures and showed no
feedback; it now renders an inline error from the mutation and flashes the
"Saved" confirmation on Clear as well as Save. The "reply approve <code> in
Slack" footnote is hidden unless an external Slack-style target exists.
* fix(automations-ui): label sub-hourly cron schedules
Minute- and hour-level cadences such as "* * * * *", "*/15 * * * *", and
"0 * * * *" rendered as "Custom schedule" because they have no single clock
time. They now read as "Every minute", "Every 15 minutes", and "Hourly at
:00".
* fix(automations-ui): space the run-row action button icons
The "Open run" and "Logs" buttons in the recent-runs list rendered the
icon flush against the label because the non-primary Button variants don't
add a gap between children. Add the same icon margin the rest of the app
uses for icon+label buttons.
* fix(automations-ui): consistent summary counts and next-run
The Running/Failures summary cards counted individual runs while the
matching filter tabs counted automations, so the numbers disagreed; both
now count automations. The soonest "Next run" no longer includes paused
triggers, which keep a stored slot they will never actually fire.
* test(automations): lock the panel UI fixes into the served bundle
Add static-asset assertions driving the composed router so each Automations
panel UX fix — sub-hourly cron labels, summary card reflow + smaller NEXT RUN
value, run-row icon spacing, and delivery save-error/Slack-hint gating — is
guarded against a regression that drops it from the shipped SPA source.
* feat(automations): surface scheduler-off state and run it by default on serve
Scheduled automations never fired because the trigger poller is disabled by
default and nothing told the user. The list response now carries
scheduler_enabled (sourced from runtime readiness) and the panel shows a
"scheduling is turned off" notice when it is false. The local `ironclaw-reborn
serve` surface enables the poller by default; config and env still override it.
* fix(i18n): add automations.delivery.saveFailed to every locale pack
The new key was added only to en.js, which breaks the i18n consistency test
that requires all locale packs to share the English key set. Add it to the
ten other packs (English placeholder, matching the existing untranslated
automations strings there).
* fix(automations-ui): clear stale Saved flash before a new delivery write
The save-error alert is gated on !showSaved, so a "Saved" flash still
showing from a prior success would hide the error of a new failing
save/clear. Reset the flash (and its timer) at the start of every attempt.
* fix(automations): harden next-run filter and lock scheduler_enabled on the wire
Use loose `!= null` in the soonest-next-run filter so a missing
next_run_timestamp can't slip through, and assert scheduler_enabled in the
list-automations handler contract test so a serialization drift of the new
field is caught at the wire, not just in the facade.
* fix(i18n): add automations.schedulerOff keys to every locale pack
The scheduler-off notice keys were added only to en.js, which breaks the
i18n consistency test requiring all locale packs to share the English key
set. Add both keys to the ten other packs (English placeholder).
* i18n(automations): translate schedulerOff strings in all locale packs
The scheduler-off notice was English in every non-English pack, giving
Arabic/German/Spanish/French/Hindi/Japanese/Korean/pt-BR/Ukrainian/zh-CN
users a mixed-language UI. Provide real translations.
* i18n(automations): translate delivery.saveFailed in all locale packs
The save-failed delivery error was English in every non-English pack. Provide
real translations so users don't see mixed-language UI when a save fails.
* Localize automation summary counts
* Remove duplicate automation summary locale keys
---------
Co-authored-by: Robert Yan <mstr.raphael@gmail.com>
* fix(approvals): persist "always allow" across threads — drop thread_id from persistent approval scope (#4825) (#4835)
* make 'always allow' approvals persist (tested on google suite)'
* test(approvals): lock criterion-5 backward-compat for project-scoped policies (#4825)
* reduce slop
* fix(approvals): address approval scope review
* fix(approvals): preserve legacy approval lookup
* fix(approvals): find legacy grants across threads
* fix(approvals): drop legacy approval scope compatibility
* fix(approvals): simplify threadless policy lookup
---------
Co-authored-by: Emil Bogomolov <emil.bogomolov@near.ai>
Co-authored-by: Henry Park <henrypark133@gmail.com>
* [codex] Use WebUI base URL for OAuth callback origins (#4932)
* Fix Railway WebUI OAuth callback origin
* fix(reborn-cli): address OAuth base URL review
* fix(reborn-cli): fail closed on hosted oauth base url
* fix(host-runtime): accept empty body/body_base64 in builtin.http (#4827)
* fix(host-runtime): accept empty body/body_base64 in builtin.http
The HTTP tool's `body()` validator rejected any request that carried
*both* a `body` and a `body_base64` field, even when both were empty
strings. The JSON schema lists both fields, so models routinely emit
`{"body": "", "body_base64": "", ...}` as defaults for a bodyless GET.
That tripped the mutual-exclusion check and failed the call with
`InputEncode` before it ever dispatched — on every attempt — so the
agent could never make a successful request and would retry until its
loop gave up.
Treat a null or empty-string field as absent: only a non-empty value
counts as "set", so the mutual-exclusion check fires only when the
caller genuinely supplies two competing bodies. Behavior for a real
`body`, a real `body_base64`, or a genuine both-non-empty conflict is
unchanged.
Adds unit tests for body() covering empty-both, absent, string body,
base64 body, both-non-empty (rejected), and JSON-object body.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* style: rustfmt the body() unit tests
---------
Co-authored-by: Pranav Raja <pranav.raja@near.ai>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* feat(reborn): observability seams — trajectory observer + LLM provider injection (#4588)
* feat(reborn): expose a trajectory observer hook on RebornRuntimeInput
The reborn runtime is sealed: build_reborn_runtime returns only the final
AssistantReply, and per-step capability (tool) calls + results live in internal
stores. Downstream consumers (benchmark harnesses, UI/debuggers) can't observe
the agent's trajectory.
Add `RebornTrajectoryObserver` (pub trait: on_capability_input(call_id, name,
args) / on_capability_result(call_id, output)) and
`RebornRuntimeInput::with_trajectory_observer`. The local-dev capability IO
(`LocalDevCapabilityIo`) forwards each tool call's name+args (at input staging)
and result (at result write) to the observer when present — reusing the same
data it already records for display previews. No-op when unset; best-effort.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* debug: trace observer hook firing (temporary)
* feat(reborn): trajectory observer — capability_id on result, reliable spine
Provider tool calls are staged by a lower decorator that bypasses the
LocalDevCapabilityIo input path, so on_capability_input does not fire for
them. on_capability_result fires for every completed capability — make it
carry the capability_id so consumers can reconstruct the trajectory (name +
output) from results alone. Input args capture is a follow-up.
* feat(reborn): capture capability input args at the host port chokepoint
Provider tool calls are staged by ProviderToolCallInputResolver, which keeps
args in a private map and bypasses the capability-IO input hook — so inputs
never reached the trajectory observer (only results did). Move the observer
trait down to ironclaw_loop_support (CapabilityTrajectoryObserver, re-exported
from composition as RebornTrajectoryObserver) and hook it in
HostRuntimeLoopCapabilityPort::invoke_capability right after the input
resolves — the one place the model's resolved arguments are visible. Threaded
through HostRuntimeLoopCapabilityPortFactory + the local-dev factory. Result
hook unchanged. Now name + args + output are all captured.
* feat(reborn): host LLM-provider injection seam
ResolvedRebornLlm::with_provider — drive the runtime with a caller-supplied
LlmProvider (e.g. an instrumented wrapper that counts tokens/cost and captures
reasoning) instead of always building one from config; build_llm_gateway honors
the override. The only viable observability path for reborn, whose model calls
run in spawned worker tasks a per-task tracing subscriber can't reach.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* test(reborn): cover trajectory observer + LLM provider override seams
Addresses Firat's two blocking review findings on #4588 (both
missing-integration-test, per AGENTS.md "test through the caller"):
1. Trajectory observer callbacks — drive the real call sites with a
recording CapabilityTrajectoryObserver:
- host port: invoke_capability via HostRuntimeLoopCapabilityPortFactory
::with_trajectory_observer asserts on_capability_input fires with the
resolved capability id + tool-call arguments.
- local-dev IO: register_provider_tool_call_input + write_capability_result
assert on_capability_input and on_capability_result fire and correlate by
input ref.
2. LLM provider override — build_llm_gateway_drives_provider_override_not_config
injects a counting mock via ResolvedRebornLlm::with_provider, points config
at a dead endpoint, and asserts the gateway returns the mock's sentinel
(proving the override is driven, not a config-built chain).
Also fixes pre-existing breakage this surfaced: 5 LocalDevLoopCapabilityPort
Factory test initializers (shell_tests.rs + tests.rs) were missing the
trajectory_observer field added by this PR, so the composition crate's tests
did not compile under --features root-llm-provider.
loop_support: 301 passed; composition (root-llm-provider): 520 passed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(reborn): make trajectory observer input semantics consistent
Addresses Copilot's follow-up findings on the observer seam:
- Drop the `on_capability_input` callback from `LocalDevCapabilityIo::
register_provider_tool_call_input`. It forwarded the raw provider tool
name (`builtin_echo`) as the capability id — conflicting with the
observer contract (resolved dotted `builtin.echo`) and the authoritative
port-level hook — and `ProviderToolCallInputResolver` doesn't delegate
here for provider tool calls, so it never fired in practice anyway.
`HostRuntimeLoopCapabilityPort::invoke_capability` remains the single
source of `on_capability_input` (resolved id); `LocalDevCapabilityIo`
remains the source of `on_capability_result`.
- Clarify the trait doc: `arguments` is the raw model-emitted tool-call
input resolved from the input ref (the callback fires before schema
normalization), which is what the trajectory should record.
- Refocus the local-dev test on `on_capability_result` forwarding +
correlation, and assert input staging does NOT emit `on_capability_input`
from local-dev IO. Port-level input semantics stay covered by the
capability_port.rs test.
loop_support: 301 passed; composition (root-llm-provider): 520 passed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* wire trajectory_observer through RefreshingLocalDevCapabilityPortConfig
Completes the main-merge conflict resolution: local_dev.rs passes
trajectory_observer into the refreshing-port config, so the config struct +
port struct must carry it and build_inner must apply it via
.with_trajectory_observer(). (Missed staging this file in the merge commit.)
* test(reborn): lock down the observability seams against regression
#4588 exposes two seams a downstream harness relies on. Add tests so a
future refactor can't silently break either:
- capability_io_forwards_result_to_trajectory_observer: drives
write_capability_result and asserts on_capability_result fires with the
correct (call_id, capability_id, output) — the result half of the
trajectory observer (tool-call outputs).
- build_llm_gateway_drives_provider_override_not_config: asserts the gateway
drives a provider injected via ResolvedRebornLlm::with_provider (config
points at a dead endpoint), proving the provider-injection seam works —
this is how the bench captures reasoning / tokens / cost / system-prompt /
tool-definitions. (Restores the test dropped during the main merge.)
The input half (on_capability_input) is already covered by
invoke_capability_forwards_resolved_input_to_trajectory_observer in
ironclaw_loop_support. All three pass.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(reborn): drop the false-confidence result-hook test
capability_io_forwards_result_to_trajectory_observer called
write_capability_result directly, so it stayed green even though the
result hook is unreachable end-to-end while capability dispatch fails
(the LocalDevYolo InputEncode regression) — i.e. it did not fail when
the feature it claimed to cover was actually broken.…
…i#4910) * fix auth resume input replay * fix auth resume approval token carryover
Summary
input_ref.Change Type
Linked Issue
Related #4887
Validation
cargo fmt --all -- --checkcargo clippy --all --benches --tests --examples --all-features -- -D warningscargo build:cargo check --workspace --quietcargo test -p ironclaw_agent_loop --quietcargo test -p ironclaw_loop_support --quietcargo test -p ironclaw_turns --quietcargo test -p ironclaw_hooks --quietcargo test -p ironclaw_reborn --test loop_driver_host --quietcargo test -p ironclaw_reborn_composition local_dev --quietcargo test --features integrationif database-backed or integration behavior changedreview-prorpr-shepherd --fixwas run before requesting review: thermo-nuclear code-quality review pass plus Gemini review follow-up addressed.Security Impact
Touches auth/runtime resume handling. This does not weaken auth checks; it preserves the already-validated capability input/estimate for auth resume so the host does not resolve a stale scoped input ref after OAuth. The replay payload is serde-optional for checkpoint compatibility and covered by migration/round-trip tests.
Reborn Trust-Boundary Checklist
CapabilityAuthResumeReplayis host/loop contract data constructed by the runtime conversion path or restored from checkpointed pending auth state.cargo check --workspace --quiet; updatedAuthRequiredconstructors/matches in agent-loop, loop-support, hooks, and tests.serde(default)fields fail closed or have migration tests.PendingAuthResume.replayandCapabilityAuthResume.replayare optional defaults; checkpoint compatibility tests cover absence and round-trip.Transient,Permanent,Misconfigured,PolicyDeniedor equivalent). No error class changes.Database Impact
None. No migrations or persistence backend schema changes. Checkpoint payload shape gains optional serde-default fields only.
Blast Radius
Touches Reborn capability auth-resume flow across
ironclaw_turns,ironclaw_agent_loop,ironclaw_loop_support, and hooks middleware. Regressions would most likely affect approval/auth gated capability replay, especially Gmail/Google workspace tools after OAuth completion.Rollback Plan
Revert this PR. The previous behavior may reintroduce
scope_mismatchon Gmail auth resume when the originalinput_refis stale or consumed.Review Follow-Through
Gemini requested preserving the original approval invocation token when approval flows into auth. The latest commit makes the prior approval token win and extends the caller-level regression test to cover that token carryover.
Review track: C (runtime/auth resume path)