feat(runtime-context): surface connected channels, delivery state, and run origin - #4836
Merged
Merged
Conversation
Foundation for #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 #4795 baseline, so existing fingerprints are unaffected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Wires #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>
- 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-#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>
github-actions
Bot
added
size: XL
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughIntroduces a new ChangesProduct context and communication context pipeline
Sequence Diagram(s)sequenceDiagram
participant Ingress as Inbound / WebUI Ingress
participant Resolver as ironclaw_product_context
participant Submit as SubmitTurnRequest
participant Store as TurnStateStore
participant Host as RebornLoopDriverHostFactory
participant Provider as RuntimeCommunicationContextProvider
participant Prompt as LoopRuntimeContext.render_model_content
Ingress->>Resolver: resolve_inbound(classification, adapter, surface, owner)
Resolver-->>Submit: ProductTurnContext { origin, surface_type, adapter, owner }
Submit->>Store: submit_turn → TurnRunState.product_context
Store-->>Host: ClaimedTurnRun.state.product_context
Host->>Provider: begin_communication_context(scope, actor)
Provider-->>Host: CommunicationContextFetch (async, 500ms budget)
Host->>Host: visible_capabilities → delivery_tools_visible
Host->>Provider: fetch.resolve(delivery_tools_visible)
Provider-->>Host: CommunicationRuntimeContext
Host->>Prompt: LoopRuntimeContext { communication, product_context }
Prompt-->>Prompt: render channels + delivery target + Run origin line
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Poem
|
![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 turn run origin tracking and a model-visible communication runtime context. It defines the TurnRunOrigin enum and propagates it through turn submission, run state, and loop execution. Additionally, it implements a CommunicationContextProvider to fetch and render connected channels, outbound delivery targets, and run origins into the model's system prompt. The review feedback suggests that child runs should inherit their parent's run_origin rather than defaulting to None to maintain consistent context across the execution tree.
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.
![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: 85633b82a6
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/ironclaw_reborn_composition/src/runtime.rs (2)
1299-1313: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon't classify the runtime task API as
WebUiChat.
send_user_message*is this file's CLI/task-level ingress, but this stamps every submission asWebUiChat. That feeds the new runtime-context slice the wrong origin for CLI/e2e callers and will tell the model the run came from WebUI when it did not. Safer options here arerun_origin: Noneuntil a dedicated CLI origin exists, or adding that explicit origin upstream and using it here.Suggested fix
requested_run_id: None, parent_run_id: None, subagent_depth: 0, spawn_tree_root_run_id: None, - run_origin: Some(TurnRunOrigin::WebUiChat), + run_origin: None, })As per coding guidelines, "When changing behavior in a function, re-read its docstring and adjacent comments; update or delete them in the same change to keep documentation in sync with code".
🤖 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_reborn_composition/src/runtime.rs` around lines 1299 - 1313, The SubmitTurnRequest in send_user_message* incorrectly sets run_origin to Some(TurnRunOrigin::WebUiChat), which misclassifies CLI/task/e2e submissions; change run_origin to None (or use an explicit CLI run origin if added upstream) when calling submit_turn so runtime-context gets the correct origin, and update the surrounding docstring/comments in this file to reflect the new behavior. Ensure you edit the SubmitTurnRequest construction (reference: SubmitTurnRequest, run_origin, TurnRunOrigin::WebUiChat, send_user_message*) to remove the hardcoded WebUiChat value and keep documentation in sync.Source: Coding guidelines
2150-2228: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftProduction silently drops the new communication-context feature.
This provider is only built from
local_runtime, butproduction_runtime_partshard-codeslocal_runtime: None. That means every production launch passescommunication_context_provider: None, so the host loop never renders the new communication slice on the production path. If production wiring is not ready yet, fail closed here; otherwise this needs a production-backed outbound/lifecycle provider instead of silently degrading away the feature.As per coding guidelines, "Production and migration-dry-run profiles must fail closed on local-only or missing required handles".
🤖 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_reborn_composition/src/runtime.rs` around lines 2150 - 2228, The code builds communication_context_provider only when local_runtime is Some, but production_runtime_parts sets local_runtime: None causing production runs to silently drop this feature; update the production wiring so DefaultPlannedRuntimeParts.communication_context_provider is either constructed for production or the runtime creation fails fast. Concretely, locate the communication_context_provider construction and the production Runtime/DefaultPlannedRuntimeParts creation (symbols: communication_context_provider, local_runtime, production_runtime_parts, DefaultPlannedRuntimeParts) and either (A) add a production-backed implementation of RuntimeCommunicationContextProvider/outbound preferences and lifecycle facade and use it when local_runtime is None, or (B) return an Err(RebornRuntimeError::InvalidArgument { reason: ... }) during production runtime build when local_runtime is None to fail closed.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_conversations/src/inbound.rs`:
- Around line 95-101: The run_origin assignment currently derives
TurnRunOrigin::ScheduledTrigger solely from adapter_kind.is_trusted_trigger(),
allowing an untrusted inbound with adapter=="trigger" to be treated as trusted;
change the logic to require the binding resolution policy to be trusted (e.g.,
only set ScheduledTrigger when binding_resolution_policy indicates trusted AND
adapter_kind.is_trusted_trigger()), otherwise use TurnRunOrigin::ProductInbound;
update the code paths referencing run_origin (the run_origin variable,
adapter_kind.is_trusted_trigger(), and TurnRunOrigin::ScheduledTrigger)
accordingly and add a regression test that constructs an inbound request with
BindingResolutionPolicy::Untrusted and adapter_kind "trigger" and asserts the
recorded origin is ProductInbound.
In `@crates/ironclaw_product_workflow/tests/inbound_turn_contract.rs`:
- Around line 1290-1296: Change the ProductInbound variant to store the
ProductAdapterId newtype rather than a raw String: update the enum variant
TurnRunOrigin::ProductInbound(adapter: ProductAdapterId) in the origin
definition and adjust any construction sites to pass the existing
ProductAdapterId (the envelope already provides it). Then update this test
assertion to compare against Some(TurnRunOrigin::ProductInbound { adapter:
ProductAdapterId::from("test_adapter") }) (or the actual constructor used in
tests) instead of a String, and ensure request.run_origin is constructed with
the ProductAdapterId value from the envelope.
In `@crates/ironclaw_reborn_composition/src/communication_context.rs`:
- Around line 112-135: The code currently filters extensions with
extension_is_channel_surface and returns ConnectedChannelsState::Known(channels)
even when classification data is unavailable (causing false-negative "none"
state); change the logic in the block that builds channels (the iterator that
calls extension_is_channel_surface, maps to ConnectedChannelSummary, and returns
ConnectedChannelsState::Known) to first detect whether any extension could not
be classified (i.e., the classification/projection is absent or
extension_is_channel_surface is indeterminate) and if so return a non-definitive
state (e.g., ConnectedChannelsState::Unknown or a new Uncertain variant) instead
of Known(vec![]); additionally emit a warning/log (using the existing logger)
when classification data is missing so callers can see the uncertainty rather
than silently reporting no connected channels.
In `@crates/ironclaw_reborn/src/loop_driver_host.rs`:
- Around line 1511-1514: The code currently calls surface_state.current().ok()
when computing delivery_tools_visible, which silently drops
CapabilitySurfaceState/AgentLoopHostError; change this to propagate or handle
the error explicitly: replace the .ok() usage by handling the Result from
surface_state.current() (e.g., ?-propagate the AgentLoopHostError up from the
function or match and log the error), or if a silent degradation is intentional
add a clear comment `// silent-ok: <reason>` and emit a processLogger.warn with
the error before defaulting; locate the usage by the delivery_tools_visible
variable and the surface_state.current() call to implement the fix.
- Around line 2035-2037: The code now propagates claimed.state.run_origin into
loop context in create_host, but validate_claimed_run_context does not verify
that claimed.state.run_origin matches the incoming run_context.run_origin;
update validate_claimed_run_context to assert/return an error when
claimed.state.run_origin (from the claimed argument) is Some and does not equal
the run_context.run_origin (or when one is Some and the other is None),
preventing caller-minting of trusted ingress metadata; reference the
functions/values validate_claimed_run_context, create_host,
claimed.state.run_origin, run_context.run_origin, and
loop_run_context.with_run_origin when making the check and returning a clear
validation error.
In `@crates/ironclaw_reborn/tests/loop_driver_host.rs`:
- Around line 1039-1072: The RecordingCommunicationContextProvider mock only
records delivery_tools_visible via recorded_delivery_tools_visible; update the
mock to capture all communication_context inputs (scope, actor,
delivery_tools_visible, run_origin) by adding fields (e.g., recorded_scope:
Mutex<Option<TurnScope or Arc/cloneable wrapper>>, recorded_actor:
Mutex<Option<TurnActor or identifier>>, recorded_run_origin:
Mutex<Option<ironclaw_turns::TurnRunOrigin>>) and set them inside the
communication_context implementation before returning the
CommunicationRuntimeContext; keep the existing recorded_delivery_tools_visible
behavior and provide accessor methods (e.g., delivery_tools_visible(),
recorded_scope(), recorded_actor(), recorded_run_origin()) so tests can assert
on all four inputs while preserving thread-safety with the existing Mutex/Arc
pattern on RecordingCommunicationContextProvider.
---
Outside diff comments:
In `@crates/ironclaw_reborn_composition/src/runtime.rs`:
- Around line 1299-1313: The SubmitTurnRequest in send_user_message* incorrectly
sets run_origin to Some(TurnRunOrigin::WebUiChat), which misclassifies
CLI/task/e2e submissions; change run_origin to None (or use an explicit CLI run
origin if added upstream) when calling submit_turn so runtime-context gets the
correct origin, and update the surrounding docstring/comments in this file to
reflect the new behavior. Ensure you edit the SubmitTurnRequest construction
(reference: SubmitTurnRequest, run_origin, TurnRunOrigin::WebUiChat,
send_user_message*) to remove the hardcoded WebUiChat value and keep
documentation in sync.
- Around line 2150-2228: The code builds communication_context_provider only
when local_runtime is Some, but production_runtime_parts sets local_runtime:
None causing production runs to silently drop this feature; update the
production wiring so DefaultPlannedRuntimeParts.communication_context_provider
is either constructed for production or the runtime creation fails fast.
Concretely, locate the communication_context_provider construction and the
production Runtime/DefaultPlannedRuntimeParts creation (symbols:
communication_context_provider, local_runtime, production_runtime_parts,
DefaultPlannedRuntimeParts) and either (A) add a production-backed
implementation of RuntimeCommunicationContextProvider/outbound preferences and
lifecycle facade and use it when local_runtime is None, or (B) return an
Err(RebornRuntimeError::InvalidArgument { reason: ... }) during production
runtime build when local_runtime is None to fail closed.
🪄 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: 0b284281-30f4-4c11-b026-4dd1de1bf6bf
📒 Files selected for processing (53)
.gitignorecrates/ironclaw_conversations/src/ids.rscrates/ironclaw_conversations/src/inbound.rscrates/ironclaw_host_runtime/tests/host_runtime_services_contract.rscrates/ironclaw_host_runtime/tests/turn_scheduler_contract.rscrates/ironclaw_loop_support/src/cancellation_port.rscrates/ironclaw_loop_support/src/subagent_spawn_port/tests.rscrates/ironclaw_loop_support/tests/turn_event_publisher_contract.rscrates/ironclaw_product_workflow/src/auth_continuation.rscrates/ironclaw_product_workflow/src/inbound_turn.rscrates/ironclaw_product_workflow/src/reborn_services.rscrates/ironclaw_product_workflow/tests/approval_interaction_contract.rscrates/ironclaw_product_workflow/tests/auth_interaction_contract.rscrates/ironclaw_product_workflow/tests/inbound_turn_contract.rscrates/ironclaw_product_workflow/tests/reborn_services_contract.rscrates/ironclaw_product_workflow/tests/support/planned_agent_loop.rscrates/ironclaw_reborn/src/loop_driver_host.rscrates/ironclaw_reborn/src/loop_exit_applier/tests/support.rscrates/ironclaw_reborn/src/runtime.rscrates/ironclaw_reborn/src/subagent/completion_observer.rscrates/ironclaw_reborn/src/turn_runner/tests/mod.rscrates/ironclaw_reborn/tests/hooks_integration.rscrates/ironclaw_reborn/tests/llm_gateway.rscrates/ironclaw_reborn/tests/loop_driver_host.rscrates/ironclaw_reborn/tests/loop_milestone_event_projection.rscrates/ironclaw_reborn_composition/src/communication_context.rscrates/ironclaw_reborn_composition/src/factory/auth_tests.rscrates/ironclaw_reborn_composition/src/lib.rscrates/ironclaw_reborn_composition/src/projection/tests.rscrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_reborn_composition/src/runtime/tests/auth_interaction.rscrates/ironclaw_reborn_composition/src/runtime/tests/default_system_prompt.rscrates/ironclaw_reborn_composition/src/slack_delivery.rscrates/ironclaw_reborn_composition/src/slack_serve/e2e_tests.rscrates/ironclaw_reborn_composition/src/trigger_poller.rscrates/ironclaw_reborn_composition/tests/product_live_adapters.rscrates/ironclaw_turns/src/events.rscrates/ironclaw_turns/src/lib.rscrates/ironclaw_turns/src/memory.rscrates/ironclaw_turns/src/origin.rscrates/ironclaw_turns/src/request.rscrates/ironclaw_turns/src/run_profile/host.rscrates/ironclaw_turns/src/run_profile/mod.rscrates/ironclaw_turns/src/run_profile/prompt.rscrates/ironclaw_turns/src/run_profile/runtime_context.rscrates/ironclaw_turns/src/status.rscrates/ironclaw_turns/src/store.rscrates/ironclaw_turns/tests/active_run_ref_state_contract.rscrates/ironclaw_turns/tests/agent_loop_host_contract.rscrates/ironclaw_turns/tests/checkpoint_state_store_contract.rscrates/ironclaw_turns/tests/filesystem_turn_state_contract.rscrates/ironclaw_turns/tests/turn_coordinator_contract.rstests/support/reborn/harness.rs
henrypark133
commented
Jun 13, 2026
henrypark133
left a comment
henrypark133
left a comment
Collaborator
Author
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Add a runtime-context slice that exposes connected channels, outbound delivery state, and typed run origin at loop start.
Stats: 9 findings (from 14 raw, 9 after dedup) across 7 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0
security
- Medium Untrusted communication labels enter a system prompt as instructions (
crates/ironclaw_turns/src/run_profile/runtime_context.rs:111-145, confidence 75) — anchor: crates/ironclaw_turns/src/run_profile/runtime_context.rs:111. Also flagged by: tests/Medium.
The new runtime-context slice interpolates channel names and outbound target display strings into a system-role message. Channel names are not sanitized at all, and delivery target strings are only character-mapped, so semantic payloads such as 'Ignore previous instructions' remain as ordinary system text. A user/admin-controlled channel or target label can therefore inject instructions into the model-visible system context, or trip the model-safe text denylist and block prompt construction.
bugs
- Medium Connected-channel detection always produces none (
crates/ironclaw_reborn_composition/src/communication_context.rs:114-159, confidence 75) — anchor: crates/ironclaw_reborn_composition/src/communication_context.rs:156. Also flagged by: approach/Medium.
The lifecycle-backed provider filters channel extensions through extension_is_channel_surface, but that predicate unconditionally returns false. When lifecycle listing succeeds, every active Slack/Telegram-style extension is dropped and the runtime context reports Connected channels: none, which contradicts the feature goal of surfacing connected channels at loop start.
performance
- Medium Sequential timeouts can add 1s to every loop start (
crates/ironclaw_reborn_composition/src/communication_context.rs:62-136, confidence 88) — anchor: crates/ironclaw_reborn/src/loop_driver_host.rs:1522. Also flagged by: tests/Medium, local-patterns/Low.
This provider runs on the per-loop prompt path and awaits the outbound-preferences fetch before starting the lifecycle fetch. Each call has its own 500ms timeout, so two slow backends stall model dispatch for about 1s even though the data is independent and failures degrade to unknown.
tests
- Medium Ordinary inbound origin branch is not asserted (
crates/ironclaw_conversations/src/inbound.rs:95-100, confidence 75) — anchor: crates/ironclaw_conversations/src/inbound.rs:95.
The new classifier maps non-trigger adapters to TurnRunOrigin::ProductInbound, but adjacent inbound_contract tests only inspect the trusted-trigger ScheduledTrigger branch or other submission fields. A regression that leaves ordinary conversation inbound runs with no origin or the wrong adapter would not fail. - Medium WebUI origin is not asserted in the model request (
crates/ironclaw_reborn_composition/src/runtime.rs:1297-1313, confidence 75) — anchor: crates/ironclaw_reborn_composition/src/runtime.rs:1312.
send_user_message now submits turns with TurnRunOrigin::WebUiChat, and the host copies claimed state into the prompt runtime context, but the full local-dev runtime test only checks for an outbound-delivery line. It does not assert that the WebUI origin line reaches the actual model request after submit, claim, host creation, and prompt materialization.
conventions
- Medium Run origin stores adapter identity as a raw String (
crates/ironclaw_turns/src/origin.rs:11-11, confidence 100) — anchor: .claude/rules/types.md:15.
The new typed run-origin contract carries ProductInbound { adapter: String }, and callers convert existing typed adapter IDs into strings before crossing into SubmitTurnRequest. That violates the typed-internals rule that identifiers flowing between internal modules use specialized types, not raw String; this field is persisted through turn state and then consumed by Reborn prompt composition, so the compiler can no longer distinguish a product adapter id from any other string.
local-patterns
- Low Document the new public communication provider contract (
crates/ironclaw_turns/src/run_profile/runtime_context.rs:207-214, confidence 75) — anchor: crates/ironclaw_turns/src/run_profile/compaction.rs:139.
CommunicationContextProvider is a newly re-exported run-profile port, but unlike comparable public run-profile contracts it has no doc-comment explaining ownership, None, or degradation semantics. Nearby exported ports document those contracts, which helps implementers preserve the prompt-safety and backend-failure behavior.
maintainability
- Medium Delivery visibility is cached before prompt-level filtering (
crates/ironclaw_reborn/src/loop_driver_host.rs:1511-1521, confidence 75) — anchor: crates/ironclaw_reborn/src/loop_driver_host.rs:1511.
The host snapshots delivery_tools_visible from the raw capability surface, then HostManagedLoopPromptPort later applies request.capability_view filtering before building the actual prompt. That makes delivery-tool visibility two separate truths: the runtime-context slice can mention tool names based on a capability that the final prompt surface filtered out. - Low Trusted trigger adapter kind is hardcoded twice (
crates/ironclaw_conversations/src/ids.rs:43-51, confidence 100) — anchor: crates/ironclaw_conversations/src/ids.rs:45. Also flagged by: local-patterns/Low.
AdapterKind::is_trusted_trigger hand-copies the "trigger" value already owned by ironclaw_triggers::TRIGGER_TRUSTED_ADAPTER_KIND. If that canonical adapter kind changes, origin classification silently diverges from the trusted submitter.
henrypark133
changed the title
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>
…el 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-#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>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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_conversations/src/inbound.rs (1)
95-103:⚠️ Potential issue | 🔴 CriticalBorrow
binding_policyinmatches!guard to avoid move-after-use (compile blocker)
BindingResolutionPolicyisn’tCopy(it’s only#[derive(Debug, Clone, PartialEq, Eq)]), somatches!(binding_policy, ...)moves it and the latermatch &binding_policy/match binding_policyuses become invalid, breaking the trusted-ingress classification flow.Suggested fix
- let run_origin = if matches!(binding_policy, BindingResolutionPolicy::Trusted { .. }) + let run_origin = if matches!(&binding_policy, BindingResolutionPolicy::Trusted { .. }) && adapter_kind.is_trusted_trigger() { Some(TurnRunOrigin::ScheduledTrigger) } else { Some(TurnRunOrigin::ProductInbound { adapter: adapter_kind.as_str().to_string(), }) };🤖 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_conversations/src/inbound.rs` around lines 95 - 103, The matches! call is moving binding_policy (BindingResolutionPolicy) which breaks later uses; change the guard to borrow it instead: use matches!(&binding_policy, BindingResolutionPolicy::Trusted { .. }) so binding_policy is not moved, keeping the logic that sets run_origin to Some(TurnRunOrigin::ScheduledTrigger) when adapter_kind.is_trusted_trigger(), otherwise to Some(TurnRunOrigin::ProductInbound { adapter: adapter_kind.as_str().to_string() }), and allowing subsequent match &binding_policy / match binding_policy usages to compile.
♻️ Duplicate comments (1)
crates/ironclaw_turns/src/run_profile/runtime_context.rs (1)
111-111:⚠️ Potential issue | 🟠 MajorCharacter replacement still leaves hostile instructions in system-role text.
This sanitizer only strips separators. The added test proves
Ignore previous instructionsstill survives as ordinary model-visible text, so channel / adapter / delivery labels can still steer the prompt from an untrusted source. Render opaque ids or fully quoted structured data instead of free-form labels.As per coding guidelines, "Every new ingress point (user messages, webhook payloads, memory writes, URL fetches, file ingestion) must run the matching safety scan on the pre-transform, pre-injection payload before reaching the LLM or database."
Also applies to: 336-360
🤖 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_turns/src/run_profile/runtime_context.rs` at line 111, The code uses sanitize_prompt_string in the format! call to produce free-form system-role text (e.g., format!("{} ({auth}, {active})", sanitize_prompt_string(&ch.name))) which still allows injected instructions like "Ignore previous instructions"; update the logic so that instead of embedding raw or lightly-sanitized labels you render opaque IDs or JSON-quoted structured data (for example replace inline "{auth}, {active}" labels with a stable opaque identifier or a fully-quoted/escaped JSON blob) and ensure the original channel/name input runs the established safety scan on the pre-transform, pre-injection payload (i.e., validate/sanitize the raw &ch.name before any formatting) by calling the project safety check function used elsewhere; change references to sanitize_prompt_string and the format! call to use the safe/opaque output and add the safety-scan invocation on the original input.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_reborn_composition/src/communication_context.rs`:
- Around line 94-99: The timeout/error branches are silently converting backend
failures to Unknown; change the Err(_) arms in the communication context logic
to bind the error (e.g., Err(e)) and emit a debug! with a short contextual
message and the error (e.g., debug!("communication context lookup failed: {:?}",
e)) immediately before returning CommunicationRuntimeContext with
ConnectedChannelsState::Unknown and DeliveryTargetState::Unknown; do the same
for the other Err(_) mapping in the same function so both failure paths log the
underlying error/timeout details.
---
Outside diff comments:
In `@crates/ironclaw_conversations/src/inbound.rs`:
- Around line 95-103: The matches! call is moving binding_policy
(BindingResolutionPolicy) which breaks later uses; change the guard to borrow it
instead: use matches!(&binding_policy, BindingResolutionPolicy::Trusted { .. })
so binding_policy is not moved, keeping the logic that sets run_origin to
Some(TurnRunOrigin::ScheduledTrigger) when adapter_kind.is_trusted_trigger(),
otherwise to Some(TurnRunOrigin::ProductInbound { adapter:
adapter_kind.as_str().to_string() }), and allowing subsequent match
&binding_policy / match binding_policy usages to compile.
---
Duplicate comments:
In `@crates/ironclaw_turns/src/run_profile/runtime_context.rs`:
- Line 111: The code uses sanitize_prompt_string in the format! call to produce
free-form system-role text (e.g., format!("{} ({auth}, {active})",
sanitize_prompt_string(&ch.name))) which still allows injected instructions like
"Ignore previous instructions"; update the logic so that instead of embedding
raw or lightly-sanitized labels you render opaque IDs or JSON-quoted structured
data (for example replace inline "{auth}, {active}" labels with a stable opaque
identifier or a fully-quoted/escaped JSON blob) and ensure the original
channel/name input runs the established safety scan on the pre-transform,
pre-injection payload (i.e., validate/sanitize the raw &ch.name before any
formatting) by calling the project safety check function used elsewhere; change
references to sanitize_prompt_string and the format! call to use the safe/opaque
output and add the safety-scan invocation on the original input.
🪄 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: 875635a3-7af7-4c98-aa87-3516b225a454
📒 Files selected for processing (9)
crates/ironclaw_conversations/src/ids.rscrates/ironclaw_conversations/src/inbound.rscrates/ironclaw_reborn/src/loop_driver_host.rscrates/ironclaw_reborn/tests/loop_driver_host.rscrates/ironclaw_reborn_composition/src/communication_context.rscrates/ironclaw_reborn_composition/src/runtime/tests/default_system_prompt.rscrates/ironclaw_turns/src/memory.rscrates/ironclaw_turns/src/run_profile/runtime_context.rsdocs/superpowers/specs/2026-06-13-product-context-factory-design.md
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e_web_ui) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…roduct_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>
github-actions
Bot
added
scope: dependencies
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 (6)
crates/ironclaw_product_workflow/src/inbound_turn.rs (1)
624-650: 🧹 Nitpick | 🔵 TrivialProductAdapterId → RunOriginAdapter round-trip is total (today); refactor to avoid validator drift
ProductAdapterId::newrejects empty,>256bytes, and control chars;RunOriginAdapter::newonly rejects empty and>256bytes. SinceAcceptedProductInboundTurnalready carries a validProductAdapterId,RunOriginAdapter::new(adapter_id.as_str())can’t fail due to validator mismatch, so it won’t introduce the “accepted without run” failure mode from this path.Refactor to avoid the
as_str()re-parse (invariant: keep identifier newtype validation coupled): add a typed conversion (e.g.,TryFrom<&ProductAdapterId>/ shared constructor) so future validator divergence can’t reintroduce this risk.🤖 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_product_workflow/src/inbound_turn.rs` around lines 624 - 650, ProductAdapterId and RunOriginAdapter validations are implicitly re-parsed via RunOriginAdapter::new(adapter_id.as_str()), which risks validator drift; implement a typed conversion to tie them together: add impl TryFrom<&ProductAdapterId> for RunOriginAdapter that performs the same checks as RunOriginAdapter::new (returning the same error type) or provide a shared constructor like RunOriginAdapter::try_from_product_adapter_id(&ProductAdapterId), and update the creation site in inbound_turn.rs to use that typed conversion (e.g., RunOriginAdapter::try_from(&adapter_id)?) instead of RunOriginAdapter::new(adapter_id.as_str()) so no string re-parse occurs.Source: Coding guidelines
crates/ironclaw_reborn_composition/src/runtime.rs (1)
2152-2175:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftProduction still drops the new communication runtime context.
communication_context_provideris built fromlocal_runtime.map(...), so every production build flowsNoneintoDefaultPlannedRuntimeParts. Since the new slice renders byte-identical to today's prompt whencommunicationis absent, the connected-channels / outbound-target context introduced by this PR never reaches the production runtime path. Either wire a production-safe provider here, or explicitly scope the rollout/docs to local-dev-only behavior in the same PR.🤖 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_reborn_composition/src/runtime.rs` around lines 2152 - 2175, communication_context_provider is only set when local_runtime is Some, so production (where local_runtime is None) passes None into DefaultPlannedRuntimeParts and loses the new communication context; instead, ensure a production-safe provider is constructed for the None case. Replace the local_runtime.map(...) usage for communication_context_provider with a match or map_or_else that builds a RuntimeCommunicationContextProvider in the None branch (using a RebornLocalLifecycleFacade constructed from safe/default skill_management or a no-op lifecycle facade, and a RebornOutboundPreferencesFacade with an empty/outbound registry), or alternatively add an explicit feature/gate and document that this code is local-dev-only; update the creation logic around communication_context_provider, RebornLocalLifecycleFacade, RuntimeCommunicationContextProvider, RebornOutboundPreferencesFacade, and OutboundDeliveryTargetRegistry so the production path receives a valid provider rather than None.crates/ironclaw_reborn_composition/src/communication_context.rs (3)
105-124:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreferences error degradation still lacks diagnostic logging.
Line 123 silently maps fetch errors to
DeliveryTargetState::Unknown. When the facade is down or rejecting requests, no diagnostic trace is left.Proposed fix
let delivery_target = match pref_result { Ok(response) => match ( response.final_reply_target, response.final_reply_target_status, ) { (Some(target), _) => DeliveryTargetState::Set(DeliveryTargetSummary { display_name: target.display_name.as_str().to_string(), channel: target.channel.as_str().to_string(), }), // A target is stored but the resolving registry in this // composition cannot produce its summary (e.g. no delivery // target providers wired). Never report "none set" here — a // preference exists and triggered delivery will use it. (None, RebornOutboundDeliveryTargetStatus::Unavailable) => { DeliveryTargetState::SetUnresolved } (None, _) => DeliveryTargetState::NoneSet, }, - Err(_) => DeliveryTargetState::Unknown, + Err(e) => { + debug!("outbound preferences fetch failed: {:?}", e); + DeliveryTargetState::Unknown + } };As per coding guidelines, "Fail loud: flag silent-failure patterns" and "Use
debug!for internal diagnostics."🤖 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_reborn_composition/src/communication_context.rs` around lines 105 - 124, The match arm that maps pref_result Err(_) to DeliveryTargetState::Unknown is silently swallowing the error; capture the error and emit a diagnostic debug log before returning Unknown. Change the Err(_) arm that handles pref_result to bind the error (e.g., Err(err)) and call debug! with a concise context string and the error (for example: debug!("failed to fetch delivery preference: {:?}", err)), then return DeliveryTargetState::Unknown; reference the pref_result match and the DeliveryTargetState::Unknown arm to locate where to apply this change.Source: Coding guidelines
92-103:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTimeout degradation still lacks diagnostic logging.
Line 94 collapses the shared budget timeout into
Unknownstate with nodebug!()emission. Operators cannot tell "classification unavailable" from "repeated 500ms miss" or facade outage.Proposed fix
let (pref_result, lifecycle_result) = match combined_result { Ok(pair) => pair, Err(_) => { + debug!("communication context fetch timed out after {:?}", OUTBOUND_PREFERENCES_TIMEOUT); // Shared budget expired — both are unknown. return Some(CommunicationRuntimeContext {As per coding guidelines, "Fail loud: flag silent-failure patterns" and "Use
debug!for internal diagnostics."🤖 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_reborn_composition/src/communication_context.rs` around lines 92 - 103, The Err(_) arm that collapses the shared budget into Unknown currently returns silently; add a debug log there to record the timeout/expiration diagnostic before returning. Specifically, inside the Err(_) match arm (the block that returns CommunicationRuntimeContext with ConnectedChannelsState::Unknown and DeliveryTargetState::Unknown), call debug!() with a clear message like "shared budget expired / combined_result timed out" and include any local context available (e.g., delivery_tools_visible, product_context, and any timing or retry counters you can access) so operators can distinguish a timeout from other failures; keep the rest of the return unchanged.Source: Coding guidelines
126-156:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLifecycle fetch error/None degradation still lacks diagnostic logging.
Line 155 silently collapses lifecycle fetch failures and missing facades into
ConnectedChannelsState::Unknown. No diagnostic trace when the lifecycle facade is rejecting calls.Proposed fix
let connected_channels = match lifecycle_result { Some(Ok(response)) => { if !CHANNEL_CLASSIFICATION_AVAILABLE { // Channel-surface classification is a stub until `#4778`'s // ProductAdapter surface projection lands. Returning Known([]) // would be false certainty ("none connected") when the predicate // cannot yet distinguish channel extensions from tool extensions. ConnectedChannelsState::Unknown } else { let extensions = match response.payload { Some(LifecycleProductPayload::ExtensionList { extensions, .. }) => { extensions } _ => Vec::new(), }; let channels: Vec<ConnectedChannelSummary> = extensions .into_iter() .filter(|ext| { extension_is_channel_surface(ext) && ext.phase == LifecyclePhase::Active }) .map(|ext| ConnectedChannelSummary { name: ext.summary.name.clone(), authenticated: true, active: true, }) .collect(); ConnectedChannelsState::Known(channels) } } - Some(Err(_)) | None => ConnectedChannelsState::Unknown, + Some(Err(e)) => { + debug!("lifecycle extension list fetch failed: {:?}", e); + ConnectedChannelsState::Unknown + } + None => ConnectedChannelsState::Unknown, };As per coding guidelines, "Fail loud: flag silent-failure patterns" and "Use
debug!for internal diagnostics."🤖 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_reborn_composition/src/communication_context.rs` around lines 126 - 156, The code silently maps lifecycle_result failures and missing facades to ConnectedChannelsState::Unknown; update the match to emit diagnostic debug logs: when matching Some(Err(e)) log debug!("lifecycle fetch failed: {:?}", e) (or include the error via debug!), and when matching None log debug!("lifecycle facade missing or returned None"); additionally, inside the Some(Ok(response)) branch, emit a debug when response.payload is None or not the expected LifecycleProductPayload::ExtensionList so you can see unexpected payloads while still returning ConnectedChannelsState::Unknown as before.Source: Coding guidelines
crates/ironclaw_turns/src/run_profile/runtime_context.rs (1)
101-115:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBound model-visible communication labels and channel fan-out.
These lines render user/admin-controlled labels and channel lists with no hard limits. A large channel set or oversized labels can blow up prompt size and degrade loop availability. Add explicit caps (entry count + per-label bytes/chars + aggregate rendered bytes) before interpolation.
As per coding guidelines, "User-controlled inputs must not grow unbounded; apply hard size limits (entries + total bytes) with documented eviction policy on interners, caches, and accumulators."
Suggested bounded rendering patch
+const MAX_CONNECTED_CHANNELS: usize = 16; +const MAX_LABEL_CHARS: usize = 64; +const MAX_COMM_SLICE_CHARS: usize = 1024; + +fn bounded_prompt_label(s: &str) -> String { + sanitize_prompt_string(s).chars().take(MAX_LABEL_CHARS).collect() +} + @@ - let joined = channels + let joined = channels .iter() + .take(MAX_CONNECTED_CHANNELS) .map(|ch| { @@ - format!("{} ({auth}, {active})", sanitize_prompt_string(&ch.name)) + format!("{} ({auth}, {active})", bounded_prompt_label(&ch.name)) }) .collect::<Vec<_>>() .join(", "); @@ - sanitize_prompt_string(&summary.display_name), - sanitize_prompt_string(&summary.channel) + bounded_prompt_label(&summary.display_name), + bounded_prompt_label(&summary.channel) ), @@ - .map(|a| sanitize_prompt_string(a.as_str())) + .map(|a| bounded_prompt_label(a.as_str())) .unwrap_or_else(|| "unknown".to_string()); @@ - parts.join("\n") + let rendered = parts.join("\n"); + rendered.chars().take(MAX_COMM_SLICE_CHARS).collect()Also applies to: 141-146, 157-164
🤖 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_turns/src/run_profile/runtime_context.rs` around lines 101 - 115, The ConnectedChannelsState::Known rendering currently interpolates unbounded user-controlled channel names via sanitize_prompt_string and can blow up prompts; modify the rendering to enforce hard limits: cap the number of entries (e.g., MAX_RENDER_CHANNELS), cap each sanitized label to a max byte/char length (e.g., MAX_CHANNEL_LABEL_BYTES) by truncating with an ellipsis, and enforce a global aggregate rendered bytes cap (e.g., MAX_CHANNEL_RENDER_BYTES) that stops adding entries once reached; implement deterministic eviction/truncation as the policy and apply the same bounded logic to the other similar render sites referenced (the other ConnectedChannelsState rendering blocks around the comments for lines 141-146 and 157-164) so all channel-list outputs use the same constants and truncation behavior.Source: Coding guidelines
♻️ Duplicate comments (2)
crates/ironclaw_reborn/src/loop_driver_host.rs (2)
1511-1535:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCompute delivery-tool visibility from the same prompt-visible surface.
delivery_tools_visibleis frozen here before any prompt bundle request exists, then baked intoLoopRuntimeContext. That leaves the runtime-context slice with a different source of truth than the prompt surface, so a narrower capability view can still produce “call outbound delivery target” guidance even when those tools are filtered out of the actual prompt. The PR goal says tool names should render only when outbound delivery capabilities are visible; this needs to move to the prompt boundary and use the same filtered surface that drives the final bundle.🤖 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_reborn/src/loop_driver_host.rs` around lines 1511 - 1535, Currently delivery_tools_visible is derived before the prompt bundle is requested and then baked into the runtime context, causing a mismatch with the prompt-visible surface; change this by removing the early computation of delivery_tools_visible and instead derive it from the same prompt-visible surface used to build the prompt bundle, then pass that derived value into provider.communication_context(...) (or compute it inside communication_context using the provided prompt surface) so the visibility originates from the prompt boundary and not from the earlier surface_state.current() snapshot; update any uses in LoopRuntimeContext to consume the visibility from the prompt-surface-derived value rather than the previous pre-bundle variable.
1938-2012:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSeal
product_contextinvalidate_claimed_run_context().
create_host()now forwardsclaimed.state.product_context, but this validator still never checksclaimed_run.state.product_context == run_context.product_context. Any directbuild_text_only_host*caller can therefore mint different origin metadata than the persisted claim, and the new communication slice will render that forged context into the prompt.Suggested fix
match ( &claimed_run.state.resolved_model_route, &run_context.resolved_model_route, ) { @@ _ => {} } + if claimed_run.state.product_context != run_context.product_context { + return Err(RebornLoopDriverHostError::ScopeMismatch { + reason: "loop run context product context does not match claimed run".to_string(), + }); + } let expected_profile_id = persisted_profile_id(&run_context.resolved_run_profile.profile_id);Please add a regression test that mutates only
LoopRunContext::product_contextand expectsScopeMismatch. As per coding guidelines, the “Trusted-ingress seal” invariant forbids caller-minted trusted ingress metadata across runtime boundaries, and every bug fix must include a regression test.🤖 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_reborn/src/loop_driver_host.rs` around lines 1938 - 2012, validate_claimed_run_context currently omits checking product_context, allowing callers to forge trusted ingress metadata; update validate_claimed_run_context to compare claimed_run.state.product_context == run_context.product_context and return RebornLoopDriverHostError::ScopeMismatch with an appropriate reason when they differ, and ensure create_host still forwards claimed.state.product_context unchanged; add a regression test that creates a valid ClaimedTurnRun and LoopRunContext, mutates only LoopRunContext::product_context, calls validate_claimed_run_context (or the public flow that invokes it), and asserts it errors with ScopeMismatch to prevent future regressions.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_reborn_composition/src/runtime.rs`:
- Around line 1312-1314: The send path currently hard-codes WebUI origin by
setting product_context: Some(ironclaw_product_context::resolve_web_ui(...))
inside RebornRuntime::send_user_message* (using
TurnActor::new(self.actor_user_id.clone())), which makes every submission appear
as WebUiChat; remove that hard-coded call and instead accept an explicit
product_context parameter on the send_user_message* APIs (or move resolve_web_ui
into the specific ingress/handler that knows the caller origin), update all call
sites to pass the correct product_context (or perform resolution in the
ingress), and add a caller-driven regression test that exercises the real
handler/factory/manager entrypoint to assert the produced prompt/rendering
contains the correct origin rather than always WebUiChat.
---
Outside diff comments:
In `@crates/ironclaw_product_workflow/src/inbound_turn.rs`:
- Around line 624-650: ProductAdapterId and RunOriginAdapter validations are
implicitly re-parsed via RunOriginAdapter::new(adapter_id.as_str()), which risks
validator drift; implement a typed conversion to tie them together: add impl
TryFrom<&ProductAdapterId> for RunOriginAdapter that performs the same checks as
RunOriginAdapter::new (returning the same error type) or provide a shared
constructor like
RunOriginAdapter::try_from_product_adapter_id(&ProductAdapterId), and update the
creation site in inbound_turn.rs to use that typed conversion (e.g.,
RunOriginAdapter::try_from(&adapter_id)?) instead of
RunOriginAdapter::new(adapter_id.as_str()) so no string re-parse occurs.
In `@crates/ironclaw_reborn_composition/src/communication_context.rs`:
- Around line 105-124: The match arm that maps pref_result Err(_) to
DeliveryTargetState::Unknown is silently swallowing the error; capture the error
and emit a diagnostic debug log before returning Unknown. Change the Err(_) arm
that handles pref_result to bind the error (e.g., Err(err)) and call debug! with
a concise context string and the error (for example: debug!("failed to fetch
delivery preference: {:?}", err)), then return DeliveryTargetState::Unknown;
reference the pref_result match and the DeliveryTargetState::Unknown arm to
locate where to apply this change.
- Around line 92-103: The Err(_) arm that collapses the shared budget into
Unknown currently returns silently; add a debug log there to record the
timeout/expiration diagnostic before returning. Specifically, inside the Err(_)
match arm (the block that returns CommunicationRuntimeContext with
ConnectedChannelsState::Unknown and DeliveryTargetState::Unknown), call debug!()
with a clear message like "shared budget expired / combined_result timed out"
and include any local context available (e.g., delivery_tools_visible,
product_context, and any timing or retry counters you can access) so operators
can distinguish a timeout from other failures; keep the rest of the return
unchanged.
- Around line 126-156: The code silently maps lifecycle_result failures and
missing facades to ConnectedChannelsState::Unknown; update the match to emit
diagnostic debug logs: when matching Some(Err(e)) log debug!("lifecycle fetch
failed: {:?}", e) (or include the error via debug!), and when matching None log
debug!("lifecycle facade missing or returned None"); additionally, inside the
Some(Ok(response)) branch, emit a debug when response.payload is None or not the
expected LifecycleProductPayload::ExtensionList so you can see unexpected
payloads while still returning ConnectedChannelsState::Unknown as before.
In `@crates/ironclaw_reborn_composition/src/runtime.rs`:
- Around line 2152-2175: communication_context_provider is only set when
local_runtime is Some, so production (where local_runtime is None) passes None
into DefaultPlannedRuntimeParts and loses the new communication context;
instead, ensure a production-safe provider is constructed for the None case.
Replace the local_runtime.map(...) usage for communication_context_provider with
a match or map_or_else that builds a RuntimeCommunicationContextProvider in the
None branch (using a RebornLocalLifecycleFacade constructed from safe/default
skill_management or a no-op lifecycle facade, and a
RebornOutboundPreferencesFacade with an empty/outbound registry), or
alternatively add an explicit feature/gate and document that this code is
local-dev-only; update the creation logic around communication_context_provider,
RebornLocalLifecycleFacade, RuntimeCommunicationContextProvider,
RebornOutboundPreferencesFacade, and OutboundDeliveryTargetRegistry so the
production path receives a valid provider rather than None.
In `@crates/ironclaw_turns/src/run_profile/runtime_context.rs`:
- Around line 101-115: The ConnectedChannelsState::Known rendering currently
interpolates unbounded user-controlled channel names via sanitize_prompt_string
and can blow up prompts; modify the rendering to enforce hard limits: cap the
number of entries (e.g., MAX_RENDER_CHANNELS), cap each sanitized label to a max
byte/char length (e.g., MAX_CHANNEL_LABEL_BYTES) by truncating with an ellipsis,
and enforce a global aggregate rendered bytes cap (e.g.,
MAX_CHANNEL_RENDER_BYTES) that stops adding entries once reached; implement
deterministic eviction/truncation as the policy and apply the same bounded logic
to the other similar render sites referenced (the other ConnectedChannelsState
rendering blocks around the comments for lines 141-146 and 157-164) so all
channel-list outputs use the same constants and truncation behavior.
---
Duplicate comments:
In `@crates/ironclaw_reborn/src/loop_driver_host.rs`:
- Around line 1511-1535: Currently delivery_tools_visible is derived before the
prompt bundle is requested and then baked into the runtime context, causing a
mismatch with the prompt-visible surface; change this by removing the early
computation of delivery_tools_visible and instead derive it from the same
prompt-visible surface used to build the prompt bundle, then pass that derived
value into provider.communication_context(...) (or compute it inside
communication_context using the provided prompt surface) so the visibility
originates from the prompt boundary and not from the earlier
surface_state.current() snapshot; update any uses in LoopRuntimeContext to
consume the visibility from the prompt-surface-derived value rather than the
previous pre-bundle variable.
- Around line 1938-2012: validate_claimed_run_context currently omits checking
product_context, allowing callers to forge trusted ingress metadata; update
validate_claimed_run_context to compare claimed_run.state.product_context ==
run_context.product_context and return RebornLoopDriverHostError::ScopeMismatch
with an appropriate reason when they differ, and ensure create_host still
forwards claimed.state.product_context unchanged; add a regression test that
creates a valid ClaimedTurnRun and LoopRunContext, mutates only
LoopRunContext::product_context, calls validate_claimed_run_context (or the
public flow that invokes it), and asserts it errors with ScopeMismatch to
prevent future regressions.
🪄 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: aa930155-d07b-4de3-b35f-e8415b81438c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (53)
Cargo.tomlcrates/ironclaw_conversations/Cargo.tomlcrates/ironclaw_conversations/src/inbound.rscrates/ironclaw_conversations/src/trusted_trigger.rscrates/ironclaw_host_runtime/tests/host_runtime_services_contract.rscrates/ironclaw_host_runtime/tests/turn_scheduler_contract.rscrates/ironclaw_loop_support/src/cancellation_port.rscrates/ironclaw_loop_support/src/subagent_spawn_port/tests.rscrates/ironclaw_loop_support/tests/turn_event_publisher_contract.rscrates/ironclaw_product_context/Cargo.tomlcrates/ironclaw_product_context/src/lib.rscrates/ironclaw_product_workflow/Cargo.tomlcrates/ironclaw_product_workflow/src/auth_continuation.rscrates/ironclaw_product_workflow/src/inbound_turn.rscrates/ironclaw_product_workflow/src/reborn_services.rscrates/ironclaw_product_workflow/tests/approval_interaction_contract.rscrates/ironclaw_product_workflow/tests/auth_interaction_contract.rscrates/ironclaw_product_workflow/tests/inbound_turn_contract.rscrates/ironclaw_product_workflow/tests/reborn_services_contract.rscrates/ironclaw_reborn/src/loop_driver_host.rscrates/ironclaw_reborn/src/loop_exit_applier/tests/support.rscrates/ironclaw_reborn/src/subagent/completion_observer.rscrates/ironclaw_reborn/src/turn_runner/tests/mod.rscrates/ironclaw_reborn/tests/hooks_integration.rscrates/ironclaw_reborn/tests/loop_driver_host.rscrates/ironclaw_reborn/tests/loop_milestone_event_projection.rscrates/ironclaw_reborn_composition/Cargo.tomlcrates/ironclaw_reborn_composition/src/communication_context.rscrates/ironclaw_reborn_composition/src/factory/auth_tests.rscrates/ironclaw_reborn_composition/src/projection/tests.rscrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_reborn_composition/src/runtime/tests/auth_interaction.rscrates/ironclaw_reborn_composition/src/slack_delivery.rscrates/ironclaw_reborn_composition/src/slack_serve/e2e_tests.rscrates/ironclaw_reborn_composition/src/trigger_poller.rscrates/ironclaw_reborn_composition/src/trigger_poller_trusted_submit.rscrates/ironclaw_turns/src/events.rscrates/ironclaw_turns/src/lib.rscrates/ironclaw_turns/src/memory.rscrates/ironclaw_turns/src/origin.rscrates/ironclaw_turns/src/request.rscrates/ironclaw_turns/src/run_profile/host.rscrates/ironclaw_turns/src/run_profile/mod.rscrates/ironclaw_turns/src/run_profile/runtime_context.rscrates/ironclaw_turns/src/scope.rscrates/ironclaw_turns/src/status.rscrates/ironclaw_turns/src/store.rscrates/ironclaw_turns/tests/active_run_ref_state_contract.rscrates/ironclaw_turns/tests/agent_loop_host_contract.rscrates/ironclaw_turns/tests/checkpoint_state_store_contract.rscrates/ironclaw_turns/tests/filesystem_turn_state_contract.rscrates/ironclaw_turns/tests/turn_coordinator_contract.rsdocs/superpowers/plans/2026-06-13-product-context-factory.md
henrypark133
commented
Jun 13, 2026
henrypark133
left a comment
henrypark133
left a comment
Collaborator
Author
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Add runtime context surfaced to the model at every loop start, covering connected channels, outbound delivery state, and run origin.
Stats: 13 findings (from 13 raw, 13 after dedup) across 8 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0
security
- Medium ScheduledTrigger origin is publicly forgeable (
crates/ironclaw_turns/src/origin.rs:56-63, confidence 75) — anchor: crates/ironclaw_turns/src/origin.rs:57
The PR claims ScheduledTrigger can only be minted by the product-context resolver under a trusted binding policy, but ProductTurnContext has public fields and SubmitTurnRequest accepts it directly. Any crate that can submit a turn can construct ProductTurnContext { origin: ScheduledTrigger, .. } without passing through TrustLevel::Trusted, spoofing trusted trigger semantics in the model-visible runtime context.
bugs
- Medium Product inbound loses direct-vs-channel surface (
crates/ironclaw_product_workflow/src/inbound_turn.rs:630-635, confidence 75) — anchor: crates/ironclaw_product_workflow/src/inbound_turn.rs:634
prepare_user_message derives the route kind from the inbound trigger, but that value is not carried into PreparedUserMessage; every product inbound submission then calls resolve_inbound(..., None, ...). DirectChat, BotMention, BotCommand, ReplyToBot, and LinkedThreadAction therefore all persist surface_type: None, so downstream runtime context cannot distinguish DMs from shared channels even though this PR adds typed surface plumbing. - Medium Runtime label sanitizer can still make prompts fail (
crates/ironclaw_turns/src/run_profile/runtime_context.rs:200-209, confidence 75) — anchor: crates/ironclaw_turns/src/run_profile/runtime_context.rs:200
sanitize_prompt_string preserves ordinary words such as secret, api key, and authorization. The rendered runtime context is later passed through validate_model_safe_text, which rejects those phrases, while outbound target display names and channels only reject control/format characters. A legitimate Slack target such as #secret-alerts or api key rotation can therefore make prompt bundle construction fail instead of degrading the communication slice.
performance
- Medium Loop start waits on lifecycle data it always discards (
crates/ironclaw_reborn_composition/src/communication_context.rs:78-88, confidence 88) — anchor: crates/ironclaw_reborn_composition/src/communication_context.rs:128
Every loop spawn now runs LifecycleProductAction::ExtensionList under the shared 500ms budget whenever a lifecycle facade is wired, but CHANNEL_CLASSIFICATION_AVAILABLE is currently hardcoded false, so the response is ignored and connected channels always render Unknown. If extension listing is slow, each per-message/per-run loop start can stall up to 500ms for work that cannot affect the prompt.
tests
- Low RunOriginAdapter oversized input is untested (
crates/ironclaw_turns/src/origin.rs:28-31, confidence 100) — anchor: crates/ironclaw_turns/src/origin.rs:30
RunOriginAdapter::new rejects both empty and >256-byte adapter IDs, but the adjacent tests only cover the empty case. The boundary/oversized external-input path is unexercised. - Medium Delivery target prompt sanitization is untested (
crates/ironclaw_turns/src/run_profile/runtime_context.rs:141-145, confidence 75) — anchor: crates/ironclaw_turns/src/run_profile/runtime_context.rs:144
The Set delivery-target branch sanitizes display_name and channel before rendering model-visible text, but tests only cover a benign #alerts (slack) target. Hostile target strings with newlines/control characters are not exercised through this branch. - Medium Communication provider timeout path is untested (
crates/ironclaw_reborn_composition/src/communication_context.rs:92-101, confidence 100) — anchor: crates/ironclaw_reborn_composition/src/communication_context.rs:94
The provider has a shared 500ms timeout that should degrade both connected_channels and delivery_target to Unknown, but the adjacent tests cover service errors and normal responses only, not the timeout branch.
conventions
- Medium Validated newtype bypasses validation on deserialize (
crates/ironclaw_turns/src/origin.rs:24-24, confidence 100) — anchor: .claude/rules/types.md:114
RunOriginAdapter is a newly added validated newtype, but deriving Deserialize lets persisted ProductTurnContext payloads rehydrate empty or >256-byte adapter values that RunOriginAdapter::new rejects. The typed-internals rule requires #[serde(try_from = "String")] so wire validation matches construction.
local-patterns
- Low Plan points reviewers at a nonexistent turns error file (
docs/superpowers/plans/2026-06-13-product-context-factory.md:139-146, confidence 75) — anchor: changed line docs/superpowers/plans/2026-06-13-product-context-factory.md:139; actual enum at crates/ironclaw_turns/src/status.rs:366
The implementation plan tells follow-up workers to add TurnError in crates/ironclaw_turns/src/error.rs, but this crate keeps TurnError in crates/ironclaw_turns/src/status.rs. That broken file trail makes the plan harder to execute and can send future fixes to the wrong path. - Low New context crate lacks the local crate guardrail trail (
crates/ironclaw_product_context/Cargo.toml:2-2, confidence 75) — anchor: sibling examples crates/ironclaw_product_workflow/AGENTS.md:1 and crates/ironclaw_turns/AGENTS.md:1
Nearby product/turn crates carry crate-local AGENTS.md/CLAUDE.md files that document ownership and boundaries, but the new single-owner ironclaw_product_context crate has neither. Because this crate owns the trust rule for minting ScheduledTrigger, future agents have to infer that boundary from code or PR docs instead of the repo's normal per-crate navigation path.
maintainability
- Medium Trigger origin minting still depends on caller-owned boolean state (
crates/ironclaw_product_context/src/lib.rs:18-25, confidence 75) — anchor: crates/ironclaw_product_context/src/lib.rs:20
The new resolver is meant to be the single owner of ScheduledTrigger classification, but its contract still asks callers to pass both TrustLevel and a separate is_trigger_adapter boolean. That splits one invariant across the resolver and each ingress caller, so a future caller can pass a mismatched adapter/bool pair and the central owner will still mint the wrong origin. - Medium Communication provider is now a pass-through for run origin (
crates/ironclaw_turns/src/run_profile/runtime_context.rs:220-227, confidence 75) — anchor: crates/ironclaw_turns/src/run_profile/runtime_context.rs:226
CommunicationContextProvider fetches channel and delivery state, but the new signature also takes persisted ProductTurnContext and returns it unchanged inside CommunicationRuntimeContext. That makes every provider and fake learn about state it does not own, and ties origin rendering to whether the communication provider exists rather than to the run context itself. - Low Runtime context rebuilds outbound preferences with an empty registry (
crates/ironclaw_reborn_composition/src/runtime.rs:2163-2171, confidence 75) — anchor: crates/ironclaw_reborn_composition/src/runtime.rs:2167
The runtime-context provider constructs its own RebornOutboundPreferencesFacade with OutboundDeliveryTargetRegistry::new(Vec::new()), while the WebUI composition already wires the same facade with the real outbound delivery target providers. This creates a second composition path for the same domain object and bakes unresolved delivery targets into runtime context unless future provider wiring is duplicated here too.
…d surface_type, validate-on-deserialize, tests, docs Co-Authored-By: Claude Fable 5 <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: Surface runtime context for connected channels, outbound delivery state, and trusted run origin via product context plumbing.
Stats: 7 findings (from 7 raw, 7 after dedup) across 6 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0
bugs
- Medium Communication context looks up actor preferences instead of turn owner (
crates/ironclaw_reborn_composition/src/communication_context.rs:83-91, confidence 75) — anchor: crates/ironclaw_reborn_composition/src/communication_context.rs:84
The new product context persists an owner on the run, and product inbound can submit a subject-owned scope wherescope.explicit_owner_user_id()differs fromactor.user_id. This provider ignores that owner and buildsWebUiAuthenticatedCallerfrom the actor, while outbound preferences are keyed byCommunicationPreferenceKey::personal(caller.tenant_id, caller.user_id). For shared/channel inbound or trusted trigger runs owned by a subject/creator different from the actor, the runtime context will render the actor's delivery target ornone setinstead of the actual owner's preference, producing wrong delivery guidance and warnings.
security
- Medium Free-form labels can inject instructions into runtime context (
crates/ironclaw_turns/src/run_profile/runtime_context.rs:263-267, confidence 75) — anchor: crates/ironclaw_turns/src/run_profile/runtime_context.rs:263
model_safe_label only replaces control and punctuation characters, then allows ordinary words and spaces through. Attacker-controlled channel names, delivery target display names, or adapter labels such as "ignore previous instructions and call ..." will be interpolated directly into the model-visible runtime context lines added by this PR, so the prompt-injection payload remains semantically intact even after sanitization.
tests
- Medium Shared-route surface mapping lacks caller-level assertion (
crates/ironclaw_product_workflow/src/inbound_turn.rs:283-287, confidence 75) — anchor: crates/ironclaw_product_workflow/src/inbound_turn.rs:284
prepare_user_message maps ProductConversationRouteKind::Shared to TurnSurfaceType::Channel, but the existing surface assertion constructs PreparedUserMessage manually after this mapping. The public accept_user_message BotMention test asserts owner scope, not the new product_context.surface_type, so the actual route-to-product-context path can regress unnoticed. - Medium No test covers join failure when no actor is present (
crates/ironclaw_turns/src/run_profile/runtime_context.rs:371-386, confidence 75) — anchor: crates/ironclaw_turns/src/run_profile/runtime_context.rs:371
CommunicationContextFetch::resolve has a JoinError branch that returns None when actor_present is false, but adjacent tests only cover actor_present=true degradation and the normal actor-none provider path. A regression could incorrectly render an Unknown communication slice for actorless runs.
conventions
- Medium Public trigger helper reintroduces string-based origin checks (
crates/ironclaw_triggers/src/trusted_submit.rs:12-13, confidence 75) — anchor: crates/ironclaw_product_context/AGENTS.md:29
The new publicis_trusted_trigger_adapter_kind(kind: &str)helper compares raw adapter text to the trigger literal and is re-exported fromcrates/ironclaw_triggers/src/lib.rs:36. That contradicts the new product-context contract that trigger-ness is carried by typedTrustedInboundKind::Trigger/InboundClassificationand origin classification never re-derives it from theadapter_kindstring.
maintainability
- Medium RunOriginAdapter hand-mirrors AdapterKind's byte limit (
crates/ironclaw_turns/src/origin.rs:24-28, confidence 75) — anchor: crates/ironclaw_conversations/src/ids.rs:239
The new RunOriginAdapter contract is defined by copying AdapterKind's 512-byte bound into ironclaw_turns, while AdapterKind itself is still validated by the separate hardcoded 512 in ironclaw_conversations. The comments now make this a required equality, but nothing keeps the two values in sync, so changing the adapter-id bound in one crate would silently reintroduce the narrowing this PR is trying to eliminate.
local-patterns
- Low Implementation plan still documents the old adapter bound (
docs/superpowers/plans/2026-06-13-product-context-factory.md:100-143, confidence 75) — anchor: crates/ironclaw_turns/src/origin.rs:24
The committed plan tells future agents to implementRunOriginAdapterwith a 256-byte cap and an error message saying1..=256 bytes, but the implemented type now explicitly mirrorsAdapterKindat 512 bytes. That stale plan can send follow-up work back to the rejected narrower bound.
| }); | ||
| }; | ||
| let (route_kind, creation_policy) = binding_profile_for_trigger(payload.trigger); | ||
| let surface_type = match route_kind { |
Collaborator
Author
There was a problem hiding this comment.
Medium — Shared-route surface mapping lacks caller-level assertion.
prepare_user_message maps ProductConversationRouteKind::Shared to TurnSurfaceType::Channel, but the existing surface assertion constructs PreparedUserMessage manually after this mapping. The public accept_user_message BotMention test asserts owner scope, not the new product_context.surface_type, so the actual route-to-product-context path can regress unnoticed.
Fix: tests::inbound_turn_contract::bot_mention_accept_user_message_records_channel_surface_type covering accept_user_message with ProductTriggerReason::BotMention and submitted product_context.surface_type == Channel
| scope: TurnScope, | ||
| actor: Option<TurnActor>, | ||
| ) -> Option<CommunicationRuntimeContext> { | ||
| let actor = actor?; |
Collaborator
Author
There was a problem hiding this comment.
Medium — Communication context looks up actor preferences instead of turn owner.
The new product context persists an owner on the run, and product inbound can submit a subject-owned scope where scope.explicit_owner_user_id() differs from actor.user_id. This provider ignores that owner and builds WebUiAuthenticatedCaller from the actor, while outbound preferences are keyed by CommunicationPreferenceKey::personal(caller.tenant_id, caller.user_id). For shared/channel inbound or trusted trigger runs owned by a subject/creator different from the actor, the runtime context will render the actor's delivery target or none set instead of the actual owner's preference, producing wrong delivery guidance and warnings.
Fix: Derive the outbound preference lookup identity from the run's ProductTurnContext.owner or explicit TurnScope owner, not from the actor alone.
| /// This is the trigger-owned authority on the predicate — callers in other | ||
| /// crates must use this function rather than comparing to `TRIGGER_TRUSTED_ADAPTER_KIND` | ||
| /// directly or carrying the check in a generic identifier type. | ||
| pub fn is_trusted_trigger_adapter_kind(kind: &str) -> bool { |
Collaborator
Author
There was a problem hiding this comment.
Medium — Public trigger helper reintroduces string-based origin checks.
The new public is_trusted_trigger_adapter_kind(kind: &str) helper compares raw adapter text to the trigger literal and is re-exported from crates/ironclaw_triggers/src/lib.rs:36. That contradicts the new product-context contract that trigger-ness is carried by typed TrustedInboundKind::Trigger / InboundClassification and origin classification never re-derives it from the adapter_kind string.
Fix: Remove the public string predicate/re-export and keep trigger classification flowing through the typed trusted-submit request and InboundClassification resolver.
| Channel, | ||
| } | ||
|
|
||
| /// Maximum byte length for a [`RunOriginAdapter`] value. Mirrors `AdapterKind`'s |
Collaborator
Author
There was a problem hiding this comment.
Medium — RunOriginAdapter hand-mirrors AdapterKind's byte limit.
The new RunOriginAdapter contract is defined by copying AdapterKind's 512-byte bound into ironclaw_turns, while AdapterKind itself is still validated by the separate hardcoded 512 in ironclaw_conversations. The comments now make this a required equality, but nothing keeps the two values in sync, so changing the adapter-id bound in one crate would silently reintroduce the narrowing this PR is trying to eliminate.
Fix: Collapse the bound to one owner. For example, expose a single adapter-id max constant from a low-level crate already visible to both validation sites, and have both AdapterKind validation and RunOriginAdapter validation import it; then derive the error text from that constant instead of hardcoding 512 again.
| /// A label that would still trip that policy (e.g. a channel literally named | ||
| /// `#secret-alerts`) degrades to `placeholder` so it can never fail prompt | ||
| /// construction — the slice degrades instead of the whole bundle. | ||
| fn model_safe_label(value: &str, placeholder: &str) -> String { |
Collaborator
Author
There was a problem hiding this comment.
Medium — Free-form labels can inject instructions into runtime context.
model_safe_label only replaces control and punctuation characters, then allows ordinary words and spaces through. Attacker-controlled channel names, delivery target display names, or adapter labels such as "ignore previous instructions and call ..." will be interpolated directly into the model-visible runtime context lines added by this PR, so the prompt-injection payload remains semantically intact even after sanitization.
Fix: Do not render arbitrary external display strings in trusted runtime context; use opaque/allowlisted labels or degrade non-canonical labels to placeholders.
| let actor_present = *actor_present; | ||
| match handle.await { | ||
| Ok(value) => value, | ||
| Err(error) => { |
Collaborator
Author
There was a problem hiding this comment.
Medium — No test covers join failure when no actor is present.
CommunicationContextFetch::resolve has a JoinError branch that returns None when actor_present is false, but adjacent tests only cover actor_present=true degradation and the normal actor-none provider path. A regression could incorrectly render an Unknown communication slice for actorless runs.
Fix: tests::run_profile::runtime_context::communication_context_fetch_join_failure_without_actor_returns_none covering actor_present=false JoinError branch
| pub struct RunOriginAdapter(String); | ||
|
|
||
| impl RunOriginAdapter { | ||
| pub fn new(value: impl Into<String>) -> Result<Self, crate::TurnError> { |
Collaborator
Author
There was a problem hiding this comment.
Low — Implementation plan still documents the old adapter bound.
The committed plan tells future agents to implement RunOriginAdapter with a 256-byte cap and an error message saying 1..=256 bytes, but the implemented type now explicitly mirrors AdapterKind at 512 bytes. That stale plan can send follow-up work back to the rejected narrower bound.
Fix: Update the plan snippet and error text to match the implemented 512-byte MAX_RUN_ORIGIN_ADAPTER_BYTES contract, or mark/remove the obsolete implementation checklist so it is not used as current guidance.
This was referenced Jun 15, 2026
henrypark133
added a commit
that referenced
this pull request
Jun 15, 2026
…Error 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>
7 tasks
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>
This was referenced Jun 15, 2026
elliotBraem
added a commit
to NEARBuilders/ironclaw
that referenced
this pull request
Jun 17, 2026
* 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.…
theredspoon
pushed a commit
to theredspoon/ironclaw
that referenced
this pull request
Jun 21, 2026
…d 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>
theredspoon
pushed a commit
to theredspoon/ironclaw
that referenced
this pull request
Jun 21, 2026
…Error gaps (nearai#4895) Addresses post-merge review findings on nearai#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 nearai#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>
This was referenced Jun 26, 2026
18 of 29 tasks
This was referenced Jul 31, 2026
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.
Implements #4828 (both phases): a runtime-context slice that tells the model, at every loop start, which channels are connected, where outbound delivery currently points, and how the run originated.
What the model sees (new
msg:runtime.*lines)Tool names render only when the outbound delivery capabilities are on the run's visible surface; the no-delivery warning for trigger-fired runs renders unconditionally.
Phase 1 — connected channels + delivery state
CommunicationContextProvidertrait stamped once per loop spawn (extends the feat(reborn): surface loop-start runtime context (time) in prompt bundles #4795LoopRuntimeContextmechanism;communication: Nonerenders byte-identical to today, so fingerprints are unaffected).get_outbound_preferencesand the lifecycle extension list under a single 500ms budget; degrades tounknown, never blocks loop start.DeliveryTargetState::SetUnresolvedkeeps a stored preference from ever rendering as "none set" when the resolving registry isn't wired.ProductAdaptersurface projection lands, so the slice rendersunknown(not a false "none") in the interim.Phase 2 — typed run origin
TurnRunOrigin { WebUiChat, ProductInbound { adapter }, ScheduledTrigger }set at the three submit sites, persisted onTurnRunRecord(serde-default, no migration), and carried throughLoopRunContextinto the rendered slice.ScheduledTriggeris only minted from a trusted binding policy. No parsing ofsource_binding_refencodings.Change Type
Validation
cargo fmtcargo clippy --all --benches --tests --examples --all-features— zero warningscargo buildcargo test(ironclaw_turns, ironclaw_conversations, ironclaw_product_workflow, ironclaw_reborn, ironclaw_reborn_composition)Security Impact
External adapter/channel/target display strings are sanitized at prompt-render time before entering the model-visible runtime-context section.
ScheduledTriggerorigin is derived from the trusted binding policy, not caller-supplied adapter text, so untrusted inbound cannot mint trusted origin semantics.Database Impact
None.
run_originis added toTurnRunRecordas a#[serde(default)]field on the existing JSON snapshot — no schema migration; legacy records deserialize withrun_origin = None.Review track
Track B (no DB schema change, no new external surface).
Relates to #4828. Independent of the #4778/#4779/#4780 stack — channel listing and tool-name guidance light up automatically as those land.
🤖 Generated with Claude Code
Update — ProductContextFactory refactor (addresses review of the run_origin plumbing)
Code review flagged the original
run_originfield as scattered/duplicated. This update replaces it with a single-owner design (spec + plan indocs/superpowers/):ironclaw_product_contextcrate — the single ingress resolver (resolve_inbound/resolve_web_ui). It is the only place aScheduledTriggerorigin can be minted, and only under aTrustedbinding policy, so the untrusted-"trigger" mislabel is now structurally impossible.ProductTurnContext(origin/surface_typeDirect|Channel/adapternewtype /owner) lives inironclaw_turns; the thinrun_origin: Option<TurnRunOrigin>is deleted. Persisted serde-default; no DB migration.RunOriginAdapter(no rawString); sharedTurnScope::product_ownerderives the owner.Local gate:
cargo fmtclean,cargo clippy --all --benches --tests --examples --all-featureszero warnings; per-crate suites green (turns,product_context,conversations,product_workflow,reborn,reborn_composition). Fullcargo testleft to CI.Tracked follow-ups (out of scope here): (b) outbound delivery engine consuming the persisted
TurnOwner; real channel-surface classification when #4778 lands;delivery_tools_visibleat the prompt boundary; untrusted-label prompt hardening.