Post Slack feedback when a message is deferred behind a pending gate - #4811
Merged
Merged
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughObserver adds an early A2b path: on plain DeferredBusy + UserMessage it authorizes, non-blockingly acquires a hint-post permit, derives an approval/auth/generic hint from run state, spawns a detached best-effort Slack chat.postMessage holding the permit, and returns early (silently dropping on saturation/failure). ChangesDeferred-busy Slack hint
Sequence Diagram(s)sequenceDiagram
participant observe_workflow_ack
participant ConversationBindingLookup
participant Semaphore
participant RunStateStore
participant Slack_chat_postMessage
observe_workflow_ack->>ConversationBindingLookup: lookup conversation binding (authorize)
ConversationBindingLookup-->>observe_workflow_ack: binding or Unauthorized
observe_workflow_ack->>Semaphore: try_acquire_owned (non-blocking)
Semaphore-->>observe_workflow_ack: permit or deny
observe_workflow_ack->>RunStateStore: get_run_state(active run id)
RunStateStore-->>observe_workflow_ack: run state (BlockedApproval/BlockedAuth/other) or Err
observe_workflow_ack->>Slack_chat_postMessage: spawn detached postMessage(hint) holding permit
Slack_chat_postMessage-->>Semaphore: release permit when done
Repo invariant (.claude/rules): background tasks performing external egress must not leak credentials or perform unaudited egress — ensure the detached post respects egress/auth invariants. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
github-actions
Bot
added
risk: low
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a deferred-busy feedback mechanism to Slack delivery, posting a hint to users when their messages are silently dropped because a run is blocked on a pending gate. This includes helper logic to filter out duplicate and non-user messages, along with corresponding unit tests. Feedback suggests adding an authorization check on the originating conversation before posting the hint to align with existing security boundaries.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+803
to
+818
| if let Some(hint) = deferred_busy_hint_for_user_message(&envelope, &ack) { | ||
| if let Err(post_err) = post_slack_message( | ||
| self.services.egress.as_ref(), | ||
| envelope.external_conversation_ref(), | ||
| hint, | ||
| ) | ||
| .await | ||
| { | ||
| tracing::debug!( | ||
| target = "ironclaw::reborn::slack_delivery", | ||
| error = %post_err, | ||
| "failed to post deferred-busy hint to Slack (best-effort)" | ||
| ); | ||
| } | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
To prevent posting feedback hints to unauthorized or arbitrary Slack channels, we should verify that the originating conversation is authorized before posting the DeferredBusy hint. This aligns with the security boundary checks performed in post_rejection_hint_if_authorized and deliver_final_reply.
if let Some(hint) = deferred_busy_hint_for_user_message(&envelope, &ack) {
if self
.services
.binding_service
.lookup_binding(ResolveBindingRequest::from_envelope(&envelope))
.await
.is_ok()
{
if let Err(post_err) = post_slack_message(
self.services.egress.as_ref(),
envelope.external_conversation_ref(),
hint,
)
.await
{
tracing::debug!(
target = "ironclaw::reborn::slack_delivery",
error = %post_err,
"failed to post deferred-busy hint to Slack (best-effort)"
);
}
}
return;
}![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: 237c612228
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+1041
to
+1051
| if !matches!(ack, ProductInboundAck::DeferredBusy { .. }) { | ||
| return None; | ||
| } | ||
| // Only reply to user messages — resolution/control/noop payloads must stay silent. | ||
| if !matches!(envelope.payload(), ProductInboundPayload::UserMessage(_)) { | ||
| return None; | ||
| } | ||
| // Each delivery of a DeferredBusy ack posts the hint: a user sending multiple | ||
| // messages while blocked gets a hint for each one (desired feedback). Rare | ||
| // transport retries may double-post — accepted as benign best-effort. | ||
| Some(SLACK_DEFERRED_BUSY_MESSAGE) |
There was a problem hiding this comment.
This posts the approval-specific hint for every DeferredBusy user message, but DeferredBusy is returned whenever the thread has any non-terminal active run, not just an approval gate; inspected crates/ironclaw_turns/src/memory.rs (thread_busy) and crates/ironclaw_turns/src/status.rs (keeps_active_lock is !is_terminal()). In the common queued/running/auth-blocked/resource-blocked cases, Slack users are now told to reply approve/deny, which is misleading and may not unblock anything. Use the active_run_id to check the current run state before sending approval wording, or make this a generic busy message.
Useful? React with 👍 / 👎.
henrypark133
commented
Jun 12, 2026
henrypark133
left a comment
henrypark133
left a comment
Collaborator
Author
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Post Slack feedback when Reborn defers user messages behind a pending approval or auth gate.
Stats: 5 findings (from 7 raw, 5 after dedup/filter) across 1 file. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 2.
Bugs
- Medium DeferredBusy hint assumes every busy run is an approval gate (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:803-807, confidence 75) — anchor:crates/ironclaw_reborn_composition/src/slack_delivery.rs:803
This branch posts approval-specific guidance for everyDeferredBusyuser message, butDeferredBusycomes from any same-thread active lock and only carriesactive_run_id. That can include a normal running turn orBlockedAuth, so users may be told toapprove/denywhen there is no approval gate or when the required action is authentication.
Performance / Concurrency
- Medium DeferredBusy hints bypass delivery backpressure (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:803-809, confidence 82) — anchor:crates/ironclaw_wasm_product_adapters/src/runner_immediate_ack.rs:100
The new path awaits Slackchat.postMessagebefore taking the delivery semaphore, and the immediate-ack runner holds its admission permit until observer follow-up completes. A burst of deferred messages can therefore pin post-ACK workers on external Slack I/O and emit one Slack API call per message without per-thread coalescing.
Tests
- Medium No integration test covers Slack DeferredBusy feedback flow (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:803-817, confidence 75) — anchor:crates/ironclaw_reborn_composition/src/slack_delivery.rs:803(no diff position — body only)
The new user-visible behavior is tested by directly callingobserve_workflow_ackwith a syntheticDeferredBusyack. There is no caller-level Slack ingress/workflow test that sends a second Slack message while the first run is parked on a pending gate and verifies that the fake Slack API receives the hint. - Low DeferredBusy post failure branch is untested (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:804-817, confidence 75) — anchor:crates/ironclaw_reborn_composition/src/slack_delivery.rs:804(no diff position — body only)
The new best-effort failure path logs at debug and returns before generic delivery, but no test programs Slack egress failure for aDeferredBusyuser message to prove it does not fall through into final delivery or error-feedback posting.
Conventions
- Low Large file grows past architecture budget without justification (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:3058-3058, confidence 75) — anchor:.claude/rules/architecture.md:136
This PR adds 213 lines to a file now around 3,955 lines. The architecture rule says files over 3,000 lines need a decomposition tracking issue, and PRs adding more than 200 lines need an inline justification.
| { | ||
| return; | ||
| } | ||
| // A2b: DeferredBusy feedback — the user's message was silently dropped |
Collaborator
Author
There was a problem hiding this comment.
Medium — DeferredBusy hint assumes every busy run is an approval gate.
This branch posts approval-specific guidance for every DeferredBusy user message, but DeferredBusy is returned for any same-thread active lock and only carries active_run_id. That can include a normal running turn or BlockedAuth, so users may be told to approve/deny when there is no approval gate or when the required action is authentication.
There is also a backpressure concern here: this path awaits Slack chat.postMessage before the delivery semaphore, while the immediate-ack runner holds its admission permit until observer follow-up completes. A burst of deferred messages can pin post-ACK workers on external Slack I/O and emit one Slack API call per message without coalescing.
Fix: Check the active run state before posting and choose approval/auth/running feedback accordingly, or use gate-neutral wording; also put this hint behind a bounded/coalesced best-effort path so repeated deferred messages cannot bypass delivery backpressure.
Also flagged by: local-patterns/Medium, performance/Medium, security/Medium
| ); | ||
| } | ||
|
|
||
| // ── DeferredBusy ack feedback tests ─────────────────────────────────────── |
Collaborator
Author
There was a problem hiding this comment.
Low — Large file grows past architecture budget without justification.
This PR adds 213 lines to slack_delivery.rs, which is now around 3,955 lines. The architecture rule says files over 3,000 lines need a decomposition tracking issue, and PRs adding more than 200 lines need an inline justification.
Fix: Move the new tests/helper into an existing narrower module, or add the required inline large-file justification with a decomposition tracking issue.
This was referenced Jun 12, 2026
Collaborator
Author
|
Review comments addressed in 8782ec5:
New tests: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/ironclaw_reborn_composition/src/slack_delivery.rs (1)
815-818: 💤 Low valueConsider defensive handling instead of
unreachable!().The
unreachable!()at line 817 is safe given thatis_deferred_busy_user_messageguarantees the ack isDeferredBusy. However, for defensive coding, you could handle the unexpected case gracefully (e.g., log and return early) rather than panicking in production.♻️ Defensive alternative
- let active_run_id = match &ack { - ProductInboundAck::DeferredBusy { active_run_id, .. } => *active_run_id, - _ => unreachable!("is_deferred_busy_user_message ensures DeferredBusy"), - }; + let ProductInboundAck::DeferredBusy { active_run_id, .. } = &ack else { + tracing::debug!( + target = "ironclaw::reborn::slack_delivery", + "deferred-busy hint path received non-DeferredBusy ack (logic bug); skipping" + ); + return; + };🤖 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/slack_delivery.rs` around lines 815 - 818, The match on ack currently uses unreachable!() for the non-DeferredBusy branch; change this to defensive handling: in the block where you compute active_run_id (matching ProductInboundAck::DeferredBusy { active_run_id, .. } => *active_run_id), replace the unreachable arm with code that logs an unexpected ack variant (including the ack debug) and returns early (or returns a suitable Result/Option) instead of panicking; use the same surrounding context where is_deferred_busy_user_message was checked so the function (or caller) can exit gracefully when the invariant is violated.
🤖 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.
Nitpick comments:
In `@crates/ironclaw_reborn_composition/src/slack_delivery.rs`:
- Around line 815-818: The match on ack currently uses unreachable!() for the
non-DeferredBusy branch; change this to defensive handling: in the block where
you compute active_run_id (matching ProductInboundAck::DeferredBusy {
active_run_id, .. } => *active_run_id), replace the unreachable arm with code
that logs an unexpected ack variant (including the ack debug) and returns early
(or returns a suitable Result/Option) instead of panicking; use the same
surrounding context where is_deferred_busy_user_message was checked so the
function (or caller) can exit gracefully when the invariant is violated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 666a3d05-1293-4a8a-9f5c-c17af56f41a9
📒 Files selected for processing (1)
crates/ironclaw_reborn_composition/src/slack_delivery.rs
henrypark133
commented
Jun 12, 2026
henrypark133
left a comment
henrypark133
left a comment
Collaborator
Author
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Post best-effort Slack feedback when user messages are deferred because a Reborn run is blocked on a pending gate.
Stats: 6 findings (from 7 raw, 6 after dedup) across 1 file. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.
Performance / Concurrency
- Medium Deferred-busy hints bypass delivery backpressure (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:847-857, confidence 88) — anchor:crates/ironclaw_reborn_composition/src/slack_delivery.rs:860
Every DeferredBusy user message spawns a detached Slack post before the observer acquires any delivery permit. A burst of messages while a thread is blocked can therefore create unbounded Tokio tasks and concurrent Slack egress calls, bypassing the existingmax_concurrent_deliveriesbackpressure path. Also flagged by: security/Medium.
Bugs
- Medium Generic deferred hint promises a replay that does not exist (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:69-69, confidence 75) — anchor:crates/ironclaw_reborn_composition/src/slack_delivery.rs:69
The generic DeferredBusy message says the new message is queued and will run when the current task finishes, but the PR explicitly leaves deferred-message drain/resubmission for a separate change.
Tests
-
Medium Missing test for scope derivation fallback (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:1101-1113, confidence 100) — anchor:crates/ironclaw_reborn_composition/src/slack_delivery.rs:1102
The new deferred-busy hint helper falls back to generic copy when deriving aTurnScopefrom the resolved binding fails, but the added tests only use bindings that derive a scope successfully. -
Medium Missing test for run-state lookup failure (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:1115-1134, confidence 100) — anchor:crates/ironclaw_reborn_composition/src/slack_delivery.rs:1127
The state-aware hint path degrades to generic copy whenTurnCoordinator::get_run_statereturns an error, but tests only cover successful lookups. -
Low Missing test for deferred-busy post failure (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:847-855, confidence 75) — anchor:crates/ironclaw_reborn_composition/src/slack_delivery.rs:848
The spawned best-effort Slack post logs on failure, but the deferred-busy tests only program successful posts or no post.
Maintainability
- Low Boolean helper leaves DeferredBusy run id as a hidden invariant (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:814-818, confidence 75) — anchor:crates/ironclaw_reborn_composition/src/slack_delivery.rs:814
is_deferred_busy_user_messagereturns a boolean, then the caller immediately re-matchesackand relies onunreachable!to recoveractive_run_id; returningOption<TurnRunId>would let the type system carry that invariant.
| .await; | ||
| let egress = Arc::clone(&self.services.egress); | ||
| let conversation = envelope.external_conversation_ref().clone(); | ||
| tokio::spawn(async move { |
Collaborator
Author
There was a problem hiding this comment.
Medium — Deferred-busy hints bypass delivery backpressure.
Every DeferredBusy user message spawns a detached Slack post before the observer acquires any delivery permit. A burst of messages while a thread is blocked can therefore create unbounded Tokio tasks and concurrent Slack egress calls, bypassing the existing max_concurrent_deliveries/backpressure path.
Fix: Gate deferred-busy hint posting with a bounded semaphore or reuse the existing delivery/backpressure queue before spawning the Slack egress task.
Also flagged by: security/Medium
| /// Posted when the blocking run is `BlockedAuth`. | ||
| const SLACK_DEFERRED_BUSY_AUTH_MESSAGE: &str = "Ironclaw is waiting on an authentication step before taking new messages — complete the authentication prompt (or reply `auth deny <auth-request-ref>` to decline)."; | ||
| /// Posted for any other non-terminal blocking state, or when the state lookup fails. | ||
| const SLACK_DEFERRED_BUSY_GENERIC_MESSAGE: &str = "Ironclaw is still working on a previous message — this one is queued and will run when the current task finishes."; |
Collaborator
Author
There was a problem hiding this comment.
Medium — Generic deferred hint promises a replay that does not exist.
The generic DeferredBusy message tells Slack users that the new message is queued and will run when the current task finishes. This PR explicitly leaves deferred-message drain/resubmission for a separate change, and the current path only marks the inbound message DeferredBusy and posts this hint. When the active run is Running or the state lookup falls back to generic copy, users will wait for a message that is not automatically submitted.
Fix: Change the generic copy to say the previous message is still running and ask the user to retry after it finishes, or only promise queuing after the drain is implemented.
| active_run_id: TurnRunId, | ||
| ) -> &'static str { | ||
| let scope = match (|| -> Result<TurnScope, ProductWorkflowError> { | ||
| let thread_scope = thread_scope_from_binding(binding)?; |
Collaborator
Author
There was a problem hiding this comment.
Medium — Missing test for scope derivation fallback.
The new deferred-busy hint helper explicitly falls back to generic copy when deriving a TurnScope from the resolved binding fails, including the path for missing agent_id, but the added tests only use bindings that derive a scope successfully.
Fix: tests::deferred_busy_missing_agent_binding_posts_generic_hint covering missing agent_id scope derivation fallback
| TurnStatus::BlockedAuth => SLACK_DEFERRED_BUSY_AUTH_MESSAGE, | ||
| _ => SLACK_DEFERRED_BUSY_GENERIC_MESSAGE, | ||
| }, | ||
| Err(err) => { |
Collaborator
Author
There was a problem hiding this comment.
Medium — Missing test for run-state lookup failure.
The new state-aware hint path degrades to generic copy when TurnCoordinator::get_run_state returns an error, but the inline deferred-busy tests only cover successful lookups for BlockedApproval, BlockedAuth, and Running.
Fix: tests::deferred_busy_run_state_lookup_error_posts_generic_hint covering get_run_state error fallback
| let egress = Arc::clone(&self.services.egress); | ||
| let conversation = envelope.external_conversation_ref().clone(); | ||
| tokio::spawn(async move { | ||
| if let Err(post_err) = |
Collaborator
Author
There was a problem hiding this comment.
Low — Missing test for deferred-busy post failure.
The new deferred-busy Slack post is spawned best-effort and logs on post_slack_message failure, but the deferred-busy tests only program successful posts or no post; they do not exercise the new failure branch that must not panic or block the ACK path.
Fix: tests::deferred_busy_post_failure_is_best_effort covering Slack post error in spawned hint task
| // | ||
| // Backpressure: the hint post is spawned detached so a burst of deferred | ||
| // messages does not pin post-ack workers on Slack I/O. | ||
| if is_deferred_busy_user_message(&envelope, &ack) { |
Collaborator
Author
There was a problem hiding this comment.
Low — Boolean helper leaves DeferredBusy run id as a hidden invariant.
The new path first asks is_deferred_busy_user_message for a boolean, then immediately re-matches ack and relies on unreachable! to recover the active_run_id. That splits one contract across two places: callers must remember that true implies a plain DeferredBusy ack, while the type system still exposes the impossible branch.
Fix: Have the helper return the run id directly, e.g. deferred_busy_user_message_run_id(envelope, ack) -> Option, and drive the branch with if let Some(active_run_id) = .... That removes the second match and the unreachable! invariant.
github-actions
Bot
added
size: XL
Collaborator
Author
|
Second-round comments addressed in dc41bb0 (the 14:3x–14:5x batch re-reported pre-push findings already fixed in 8782ec5; the 15:50 batch was fresh):
39/39 slack_delivery tests pass, clippy zero warnings. |
There was a problem hiding this comment.
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/slack_delivery.rs (2)
71-79:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't rely on inline-code formatting in these hint strings.
These messages are sent through
post_slack_message, which hardcodesmrkdwn: false, so Slack will render the backticks literally instead of as command formatting. Either remove the backticks from the copy or let this path opt into Markdown so the commands display as intended.🤖 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/slack_delivery.rs` around lines 71 - 79, The three hint constants SLACK_DEFERRED_BUSY_APPROVAL_MESSAGE, SLACK_DEFERRED_BUSY_AUTH_MESSAGE, and SLACK_DEFERRED_BUSY_GENERIC_MESSAGE include inline backticks but messages are posted with mrkdwn: false; update the implementation so either (A) remove the backticks from these strings and reword the command examples to plain text, or (B) change the call-site that posts these messages (the function that invokes post_slack_message) to enable mrkdwn/Markdown for these specific messages; pick one approach and apply it consistently so commands render correctly in Slack.
3172-3176:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse one
active_run_idin the "two hints" regression.This test currently calls
deferred_busy_ack()twice, which generates two different blocking run IDs. That misses the actual contract here — multiple messages while the same run stays blocked should each get a hint — and it would not catch a future dedupe keyed onactive_run_id.🧪 Proposed fix
- fn deferred_busy_ack() -> ProductInboundAck { + fn deferred_busy_ack(active_run_id: TurnRunId) -> ProductInboundAck { ProductInboundAck::DeferredBusy { accepted_message_ref: AcceptedMessageRef::new("slack:deferred").expect("ref"), - active_run_id: TurnRunId::new(), + active_run_id, } }+ let active_run_id = TurnRunId::new(); observer - .observe_workflow_ack(envelope(user_message_payload()), deferred_busy_ack()) + .observe_workflow_ack( + envelope(user_message_payload()), + deferred_busy_ack(active_run_id), + ) .await; // Second distinct user message while blocked (different event, new ack). observer - .observe_workflow_ack(envelope(user_message_payload()), deferred_busy_ack()) + .observe_workflow_ack( + envelope(user_message_payload()), + deferred_busy_ack(active_run_id), + ) .await;Also applies to: 3294-3340
🤖 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/slack_delivery.rs` around lines 3172 - 3176, The test erroneously generates two different TurnRunId values by calling deferred_busy_ack() twice; update the test to use a single active_run_id for both hints: create one TurnRunId (e.g., let run_id = TurnRunId::new()) and either change deferred_busy_ack() to accept an active_run_id parameter or inline the ProductInboundAck::DeferredBusy construction twice using the same run_id, ensuring both ProductInboundAck::DeferredBusy instances share the identical active_run_id; apply the same change for the other occurrence referenced (lines ~3294-3340) so both hints use the same active_run_id.
🤖 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.
Outside diff comments:
In `@crates/ironclaw_reborn_composition/src/slack_delivery.rs`:
- Around line 71-79: The three hint constants
SLACK_DEFERRED_BUSY_APPROVAL_MESSAGE, SLACK_DEFERRED_BUSY_AUTH_MESSAGE, and
SLACK_DEFERRED_BUSY_GENERIC_MESSAGE include inline backticks but messages are
posted with mrkdwn: false; update the implementation so either (A) remove the
backticks from these strings and reword the command examples to plain text, or
(B) change the call-site that posts these messages (the function that invokes
post_slack_message) to enable mrkdwn/Markdown for these specific messages; pick
one approach and apply it consistently so commands render correctly in Slack.
- Around line 3172-3176: The test erroneously generates two different TurnRunId
values by calling deferred_busy_ack() twice; update the test to use a single
active_run_id for both hints: create one TurnRunId (e.g., let run_id =
TurnRunId::new()) and either change deferred_busy_ack() to accept an
active_run_id parameter or inline the ProductInboundAck::DeferredBusy
construction twice using the same run_id, ensuring both
ProductInboundAck::DeferredBusy instances share the identical active_run_id;
apply the same change for the other occurrence referenced (lines ~3294-3340) so
both hints use the same active_run_id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 12e67d07-7f23-42f5-a0b9-33c1c70c6961
📒 Files selected for processing (1)
crates/ironclaw_reborn_composition/src/slack_delivery.rs
… pending gate
When a user sends a Slack message while a run is blocked on a pending
approval/auth gate, the inbound ack is DeferredBusy and the delivery
observer silently returned — the user got no indication why their
message was ignored.
Add a best-effort one-shot hint posted before acquiring the delivery
semaphore (same pattern as the A2 rejection-hint block). The post only
fires for UserMessage payloads — resolution/control payloads stay
silent. Duplicate{DeferredBusy} unwraps to the inner ack so Slack
transport retries also post the hint exactly once.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ssage voice
- Return None for ALL Duplicate acks in deferred_busy_hint_for_user_message
(DeferredBusy is never settled by the idempotency ledger, so
Duplicate{DeferredBusy} is unreachable; None-for-all-Duplicate is the safe
invariant matching rejection_hint_for_resolution)
- Document that each distinct plain DeferredBusy delivery posts a hint (desired
feedback for users sending multiple messages while blocked; rare transport
retries may double-post — accepted as benign best-effort)
- Replace duplicate_deferred_busy_with_user_message_posts_hint test with
duplicate_deferred_busy_with_user_message_posts_nothing asserting the new
None-for-all-Duplicate behavior
- Add two_distinct_deferred_busy_user_messages_post_two_hints test asserting
the deliberate per-delivery retry semantics
- Change SLACK_DEFERRED_BUSY_MESSAGE to impersonal voice matching sibling
constants ("Ironclaw is waiting..." instead of "I'm waiting...")
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tached post Look up the blocking run's TurnStatus via turn_coordinator.get_run_state before choosing copy for the DeferredBusy hint: - BlockedApproval → approval wording (approve / deny) - BlockedAuth → auth wording (complete auth or auth deny) - anything else / lookup failure → generic queued-task wording Add binding authorization check (mirrors post_rejection_hint_if_authorized) so the hint is silently skipped for unauthorized conversations. Spawn the Slack post in a detached task so a burst of deferred messages does not pin post-ack workers on external I/O. Failure handling is debug!-only inside the spawned task. Replace SLACK_DEFERRED_BUSY_MESSAGE with three typed constants. Replace deferred_busy_hint_for_user_message with is_deferred_busy_user_message predicate and new async deferred_busy_hint_from_run_state helper. Tests: update 2 existing DeferredBusy tests to use BlockedApproval state and yield_now drain; add 3 new tests: BlockedAuth copy, Running/generic copy, and unauthorized-binding suppression. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…new tests 1. Unbounded detached posts: add `hint_post_permits` semaphore (MAX_CONCURRENT_HINT_POSTS = 4) to `SlackFinalReplyDeliveryObserver`. `try_acquire_owned()` before spawning; drop hint with debug! on saturation so a burst cannot create unbounded tasks or bypass max_concurrent_deliveries. Permit held for the full duration of the Slack I/O inside the spawn. 2. Generic copy honesty: rewrite SLACK_DEFERRED_BUSY_GENERIC_MESSAGE to "...can't take this one yet — please resend it once the current task finishes." (no false queue promise); add comment to revisit when deferred-drain PR #4812 lands. 3. Helper API: replace `is_deferred_busy_user_message(…) -> bool` + unreachable! extraction with `deferred_busy_user_message_run_id(…) -> Option<TurnRunId>`; caller uses `if let Some(run_id) = …`. 4. New test: deferred_busy_missing_agent_binding_posts_generic_hint — binding with agent_id=None → scope derivation fails → generic copy. 5. New test: deferred_busy_run_state_lookup_error_posts_generic_hint — ErroringTurnCoordinator always returns Err → generic copy. 6. New test: deferred_busy_post_failure_is_best_effort — egress programmed with Err(Timeout) → no panic, ack path returns normally. Bonus: deferred_busy_semaphore_saturated_drops_hint_without_panic — all permits pre-held → no post, no panic. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
henrypark133
force-pushed
the
feat/slack-deferred-busy-feedback
branch
from
June 12, 2026 18:21
dc41bb0 to
fb7f982
Compare
henrypark133
commented
Jun 12, 2026
henrypark133
left a comment
henrypark133
left a comment
Collaborator
Author
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Post Slack feedback when Reborn messages are deferred behind pending approval or auth gates.
Stats: 5 findings (from 7 raw, 5 after dedup/filter) across 1 file. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0
Bugs
- Medium Auth busy hint omits the actual auth request ref (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:1301-1302, confidence 75) — anchor:crates/ironclaw_reborn_composition/src/slack_delivery.rs:1302
For BlockedAuth states the code returns placeholderauth deny <auth-request-ref>even though Slack auth denial needs a concrete ref.
Security
- Medium Deferred-busy hints can be spammed without a rate limit (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:1023-1024, confidence 75) — anchor:crates/ironclaw_reborn_composition/src/slack_delivery.rs:1024
The semaphore caps concurrent posts, not total hint volume over time for an authorized participant flooding a blocked thread.
Approach
- Medium Detached hint posts bypass the immediate-ACK task lifecycle (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:1019-1032, confidence 75) — anchor:crates/ironclaw_wasm_product_adapters/src/runner_immediate_ack.rs:100
The runner already owns post-ACK observer backpressure/drain behavior; the detached Slack task creates a second lifecycle outside that boundary. Also flagged by: maintainability/Low.
Tests
- Medium DeferredBusy state lookup request fields are not asserted (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:1294-1297, confidence 75) — anchor:crates/ironclaw_reborn_composition/src/slack_delivery.rs:1294
The tests assert resulting Slack text/count but do not capture the computedGetRunStateRequestscope andactive_run_id.
Conventions
- Medium Large-file growth lacks required decomposition justification (
crates/ironclaw_reborn_composition/src/slack_delivery.rs:3317-3317, confidence 100) — anchor:.claude/rules/architecture.md:136
slack_delivery.rsis already over 3,000 lines, and this PR adds over 200 lines without an inline large-file/decomposition justification.
| /// Posted when the blocking run is `BlockedApproval`. | ||
| const SLACK_DEFERRED_BUSY_APPROVAL_MESSAGE: &str = "Ironclaw is waiting on a pending approval before taking new messages — reply `approve` or `deny` (or `approve gate:<ref>`) to resume."; | ||
| /// Posted when the blocking run is `BlockedAuth`. | ||
| const SLACK_DEFERRED_BUSY_AUTH_MESSAGE: &str = "Ironclaw is waiting on an authentication step before taking new messages — complete the authentication prompt (or reply `auth deny <auth-request-ref>` to decline)."; |
Collaborator
Author
There was a problem hiding this comment.
Medium — Auth busy hint omits the actual auth request ref.
For BlockedAuth states the code has the blocking run state available, including gate_ref, but returns a static message containing the placeholder auth deny <auth-request-ref>. The Slack parser expects a concrete target such as auth deny gate:auth-slack, so a user following only this new deferred-busy hint cannot decline the auth gate from Slack.
Fix: Return formatted auth hint text that includes state.gate_ref when present, or remove the unusable auth deny command from the static fallback.
| // is accurate for the full duration of the Slack I/O. | ||
| let _permit = permit; | ||
| if let Err(post_err) = | ||
| post_slack_message(egress.as_ref(), &conversation, hint).await |
Collaborator
Author
There was a problem hiding this comment.
Medium — Deferred-busy hints can be spammed without a rate limit.
Every distinct DeferredBusy user message now triggers a Slack chat.postMessage side effect. The semaphore caps only concurrent in-flight posts; it does not cap total posts over time, so an authorized Slack participant can flood messages while a run is blocked and make the bot spam the channel and consume Slack API quota, potentially rate-limiting later important delivery or gate feedback.
Fix: Throttle deferred-busy hints per conversation and active_run_id, for example posting at most once per blocked run or once per short TTL window.
| .await; | ||
| let egress = Arc::clone(&self.services.egress); | ||
| let conversation = envelope.external_conversation_ref().clone(); | ||
| tokio::spawn(async move { |
Collaborator
Author
There was a problem hiding this comment.
Medium — Detached hint posts bypass the immediate-ACK task lifecycle.
The DeferredBusy hint path spawns its own detached Slack post task and adds a Slack-local semaphore to replace the runner's existing post-ACK backpressure. The immediate-ACK runner already documents that its admission permit bounds the whole post-ACK task, including observer follow-up, specifically so long-running observers do not accumulate outside that lifecycle. Since the protocol ACK has already been returned before the observer runs, awaiting this best-effort post in the observer would not delay Slack's webhook ACK and would keep shutdown, drain, and backpressure behavior in the established runner path.
Fix: Remove the detached tokio::spawn and hint_post_permits path; post the DeferredBusy hint inline in observe_workflow_ack like post_rejection_hint_if_authorized, relying on the immediate-ACK runner's existing admission permit and drain lifecycle.
Also flagged by: maintainability/Low
| } | ||
| }; | ||
| match coordinator | ||
| .get_run_state(GetRunStateRequest { |
Collaborator
Author
There was a problem hiding this comment.
Medium — DeferredBusy state lookup request fields are not asserted.
The new DeferredBusy hint path computes both the TurnScope from the resolved binding and the run_id from the ack before calling get_run_state, but the added tests only assert the resulting Slack message text/count. The scripted coordinator echoes or ignores the GetRunStateRequest, so those tests would still pass if the lookup used the wrong active_run_id or a scope unrelated to the binding, violating the repo rule that runtime API mocks capture every production argument.
Fix: Add tests::slack_delivery::deferred_busy_uses_ack_active_run_id_and_binding_scope_for_state_lookup covering GetRunStateRequest.run_id and scope derived from the authorized binding.
| ); | ||
| } | ||
|
|
||
| // ── DeferredBusy ack feedback tests ─────────────────────────────────────── |
Collaborator
Author
There was a problem hiding this comment.
Medium — Large-file growth lacks required decomposition justification.
This PR adds hundreds of lines to slack_delivery.rs, which is already over 3,000 lines. The architecture rule says files over 3,000 lines need a decomposition tracking issue, and PRs adding more than 200 lines need an inline justification. I found no large_file arch-exempt or decomposition justification near the new DeferredBusy implementation or test blocks.
Fix: Move the new DeferredBusy coverage into a smaller test module/file or add an inline large_file arch-exempt with a tracking issue/plan for decomposing slack_delivery.rs.
…re (#4811) - Fix 1: remove detached tokio::spawn + hint_post_permits semaphore; await post_slack_message inline in observe_workflow_ack; update block comment to explain why inline is safe (protocol ACK already returned, runner admission permit bounds the whole post-ACK task per runner_immediate_ack.rs contract); delete the now-meaningless semaphore-saturation test - Fix 2: add per-observer in-memory throttle (hint_seen: Mutex<...>) bounded at HINT_SEEN_CAP=256 entries with FIFO eviction; check before binding lookup to avoid coordinator round-trip on repeats; add tests deferred_busy_same_conversation_same_run_id_posts_once and deferred_busy_same_conversation_different_run_id_posts_twice - Fix 3: change deferred_busy_hint_from_run_state to return String; format concrete gate ref into BlockedApproval and BlockedAuth hints (`approve {ref}` / `auth deny {ref}`); fall back to static copy when gate_ref is None; update existing BlockedApproval and BlockedAuth tests to use scripted gate refs and assert concrete text; add deferred_busy_blocked_approval_no_gate_ref_posts_fallback_hint for None path - Fix 4: add deferred_busy_uses_ack_active_run_id_and_binding_scope_for_state_lookup using RecordingTurnCoordinator to assert the forwarded GetRunStateRequest carries the ack's active_run_id and a non-empty tenant scope - Fix 5: add arch-exempt: large_file annotation near top of file referencing decomposition issue #4818 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
Author
|
Third-round comments addressed in 33b0c7a:
585/585 composition lib tests pass, clippy clean. |
henrypark133
added a commit
that referenced
this pull request
Jun 15, 2026
* 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>
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
…earai#4811) * feat(reborn): post Slack feedback when a message is deferred behind a pending gate When a user sends a Slack message while a run is blocked on a pending approval/auth gate, the inbound ack is DeferredBusy and the delivery observer silently returned — the user got no indication why their message was ignored. Add a best-effort one-shot hint posted before acquiring the delivery semaphore (same pattern as the A2 rejection-hint block). The post only fires for UserMessage payloads — resolution/control payloads stay silent. Duplicate{DeferredBusy} unwraps to the inner ack so Slack transport retries also post the hint exactly once. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(reborn): correct DeferredBusy hint duplicate-ack semantics and message voice - Return None for ALL Duplicate acks in deferred_busy_hint_for_user_message (DeferredBusy is never settled by the idempotency ledger, so Duplicate{DeferredBusy} is unreachable; None-for-all-Duplicate is the safe invariant matching rejection_hint_for_resolution) - Document that each distinct plain DeferredBusy delivery posts a hint (desired feedback for users sending multiple messages while blocked; rare transport retries may double-post — accepted as benign best-effort) - Replace duplicate_deferred_busy_with_user_message_posts_hint test with duplicate_deferred_busy_with_user_message_posts_nothing asserting the new None-for-all-Duplicate behavior - Add two_distinct_deferred_busy_user_messages_post_two_hints test asserting the deliberate per-delivery retry semantics - Change SLACK_DEFERRED_BUSY_MESSAGE to impersonal voice matching sibling constants ("Ironclaw is waiting..." instead of "I'm waiting...") Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(reborn): state-aware deferred-busy hint with authorization and detached post Look up the blocking run's TurnStatus via turn_coordinator.get_run_state before choosing copy for the DeferredBusy hint: - BlockedApproval → approval wording (approve / deny) - BlockedAuth → auth wording (complete auth or auth deny) - anything else / lookup failure → generic queued-task wording Add binding authorization check (mirrors post_rejection_hint_if_authorized) so the hint is silently skipped for unauthorized conversations. Spawn the Slack post in a detached task so a burst of deferred messages does not pin post-ack workers on external I/O. Failure handling is debug!-only inside the spawned task. Replace SLACK_DEFERRED_BUSY_MESSAGE with three typed constants. Replace deferred_busy_hint_for_user_message with is_deferred_busy_user_message predicate and new async deferred_busy_hint_from_run_state helper. Tests: update 2 existing DeferredBusy tests to use BlockedApproval state and yield_now drain; add 3 new tests: BlockedAuth copy, Running/generic copy, and unauthorized-binding suppression. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(reborn): bounded hint-posts, honest copy, no unreachable!, three new tests 1. Unbounded detached posts: add `hint_post_permits` semaphore (MAX_CONCURRENT_HINT_POSTS = 4) to `SlackFinalReplyDeliveryObserver`. `try_acquire_owned()` before spawning; drop hint with debug! on saturation so a burst cannot create unbounded tasks or bypass max_concurrent_deliveries. Permit held for the full duration of the Slack I/O inside the spawn. 2. Generic copy honesty: rewrite SLACK_DEFERRED_BUSY_GENERIC_MESSAGE to "...can't take this one yet — please resend it once the current task finishes." (no false queue promise); add comment to revisit when deferred-drain PR nearai#4812 lands. 3. Helper API: replace `is_deferred_busy_user_message(…) -> bool` + unreachable! extraction with `deferred_busy_user_message_run_id(…) -> Option<TurnRunId>`; caller uses `if let Some(run_id) = …`. 4. New test: deferred_busy_missing_agent_binding_posts_generic_hint — binding with agent_id=None → scope derivation fails → generic copy. 5. New test: deferred_busy_run_state_lookup_error_posts_generic_hint — ErroringTurnCoordinator always returns Err → generic copy. 6. New test: deferred_busy_post_failure_is_best_effort — egress programmed with Err(Timeout) → no panic, ack path returns normally. Bonus: deferred_busy_semaphore_saturated_drops_hint_without_panic — all permits pre-held → no post, no panic. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(reborn): address five review findings on deferred-busy hint feature (nearai#4811) - Fix 1: remove detached tokio::spawn + hint_post_permits semaphore; await post_slack_message inline in observe_workflow_ack; update block comment to explain why inline is safe (protocol ACK already returned, runner admission permit bounds the whole post-ACK task per runner_immediate_ack.rs contract); delete the now-meaningless semaphore-saturation test - Fix 2: add per-observer in-memory throttle (hint_seen: Mutex<...>) bounded at HINT_SEEN_CAP=256 entries with FIFO eviction; check before binding lookup to avoid coordinator round-trip on repeats; add tests deferred_busy_same_conversation_same_run_id_posts_once and deferred_busy_same_conversation_different_run_id_posts_twice - Fix 3: change deferred_busy_hint_from_run_state to return String; format concrete gate ref into BlockedApproval and BlockedAuth hints (`approve {ref}` / `auth deny {ref}`); fall back to static copy when gate_ref is None; update existing BlockedApproval and BlockedAuth tests to use scripted gate refs and assert concrete text; add deferred_busy_blocked_approval_no_gate_ref_posts_fallback_hint for None path - Fix 4: add deferred_busy_uses_ack_active_run_id_and_binding_scope_for_state_lookup using RecordingTurnCoordinator to assert the forwarded GetRunStateRequest carries the ack's active_run_id and a non-empty tenant scope - Fix 5: add arch-exempt: large_file annotation near top of file referencing decomposition issue nearai#4818 Co-Authored-By: Claude Fable 5 <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
* feat(product): explicit gate-open feedback for busy threads, no parking Design decision (supersedes the closed defer-and-drain PR nearai#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 nearai#4811; stale nearai#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>
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.
Fixes the dead-air UX when a Reborn run is blocked on a pending gate.
Problem
While a run sits in
BlockedApproval/BlockedAuth, it holds the thread's active lock; every new Slack message on that thread is accepted asMessageStatus::DeferredBusywith no new run created.SlackFinalReplyDeliveryObserverearly-returns onDeferredBusyacks (nosubmitted_run_id), so the user gets complete silence — no reply, no error, no hint.Change
observe_workflow_acknow posts one best-effort message when aDeferredBusyack arrives for aUserMessagepayload:UserMessagepayloads get the hint (resolution payloads already have rejected-ack feedback; control/projection/noop stay silent).Duplicateacks returnNone—DeferredBusyis never settled by the idempotency ledger, soDuplicate{DeferredBusy}is unreachable; None-for-all-Duplicate is the safe invariant (matches the rejection-hint block).debug!, never recurse, never block the ack path.Tests
deferred_busy_ack_with_user_message_posts_hint,deferred_busy_ack_with_resolution_payload_posts_nothing,duplicate_deferred_busy_with_user_message_posts_nothing,two_distinct_deferred_busy_user_messages_post_two_hints— 32/32slack_deliverytests pass, clippy clean.Related: #4799 (gate routing makes the approve actually work); the deferred-message drain (resubmission after the gate resolves) ships separately.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests