Steer routine delivery through outbound targets - #4780
Conversation
github-actions
Bot
added
scope: docs
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the system prompts, tool descriptions, tests, and documentation to guide the LLM to discover and select outbound delivery targets before creating triggers or routines. The reviewer noted that the tool description in outbound_delivery.rs incorrectly uses dot notation (builtin.trigger_create) instead of the double underscore convention (builtin__trigger_create) used for model-facing tools, which could confuse the LLM. Consequently, the corresponding test assertion in tests.rs should also be updated to prevent test failures.
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.
| const OUTBOUND_DELIVERY_TARGETS_LIST_DESCRIPTION: &str = "List available outbound delivery targets for final replies and routine/trigger results, such as Slack DMs or Slack channels. When the user asks to send routine or trigger results through Slack or another product/channel, call this before builtin.trigger_create and before saying a delivery product is unavailable or asking the user to reconnect it."; | ||
| const OUTBOUND_DELIVERY_TARGET_SET_DESCRIPTION: &str = "Set the current user's final-reply delivery target to an id returned by builtin__outbound_delivery_targets_list. Use after the user asks to send replies or routine/trigger results through that product or channel, and before creating the routine or trigger."; |
There was a problem hiding this comment.
The description for OUTBOUND_DELIVERY_TARGETS_LIST_DESCRIPTION refers to builtin.trigger_create using dot notation, whereas model-facing tool names in Reborn use double underscores (e.g., builtin__trigger_create), as seen in builtin__outbound_delivery_targets_list in the very next line. Referring to the tool with dot notation can confuse the LLM since it will only see builtin__trigger_create in its tool list. We should update this to builtin__trigger_create.
| const OUTBOUND_DELIVERY_TARGETS_LIST_DESCRIPTION: &str = "List available outbound delivery targets for final replies and routine/trigger results, such as Slack DMs or Slack channels. When the user asks to send routine or trigger results through Slack or another product/channel, call this before builtin.trigger_create and before saying a delivery product is unavailable or asking the user to reconnect it."; | |
| const OUTBOUND_DELIVERY_TARGET_SET_DESCRIPTION: &str = "Set the current user's final-reply delivery target to an id returned by builtin__outbound_delivery_targets_list. Use after the user asks to send replies or routine/trigger results through that product or channel, and before creating the routine or trigger."; | |
| const OUTBOUND_DELIVERY_TARGETS_LIST_DESCRIPTION: &str = "List available outbound delivery targets for final replies and routine/trigger results, such as Slack DMs or Slack channels. When the user asks to send routine or trigger results through Slack or another product/channel, call this before builtin__trigger_create and before saying a delivery product is unavailable or asking the user to reconnect it."; | |
| const OUTBOUND_DELIVERY_TARGET_SET_DESCRIPTION: &str = "Set the current user's final-reply delivery target to an id returned by builtin__outbound_delivery_targets_list. Use after the user asks to send replies or routine/trigger results through that product or channel, and before creating the routine or trigger."; |
References
- Keep tool-specific guidance, such as parameter formats, in both the main system prompt for LLM planning and within the tool's own description (tool_info) to ensure it's exposed directly.
| assert!( | ||
| list_tool | ||
| .description | ||
| .contains("before builtin.trigger_create"), | ||
| "list tool description should steer delivery requests before trigger creation" | ||
| ); |
There was a problem hiding this comment.
Since we should update builtin.trigger_create to builtin__trigger_create in the tool description to match the model-facing tool naming convention, this test assertion should be updated accordingly to prevent test failures.
| assert!( | |
| list_tool | |
| .description | |
| .contains("before builtin.trigger_create"), | |
| "list tool description should steer delivery requests before trigger creation" | |
| ); | |
| assert!( | |
| list_tool | |
| .description | |
| .contains("before builtin__trigger_create"), | |
| "list tool description should steer delivery requests before trigger creation" | |
| ); |
References
- Keep tool-specific guidance, such as parameter formats, in both the main system prompt for LLM planning and within the tool's own description (tool_info) to ensure it's exposed directly.
|
|
||
| When a tool result is partial, truncated, failed, or otherwise shows the requested work is unfinished, adapt and continue autonomously. Ask the user only when progress requires external information, approval, or a product decision. | ||
|
|
||
| ## Delivery Targets |
There was a problem hiding this comment.
I don't think we should add any system prompts for this.
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Guide routine and trigger creation toward outbound delivery targets via model-visible tool guidance, prompts, docs, and regression coverage.
Stats: 1 finding (from 5 raw, 1 after dedup) across 1 file. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.
Bugs
- Medium Delivery steering was added to the default system prompt (
crates/ironclaw_reborn_composition/assets/prompts/default-system.md:13-16, confidence 75) — anchor:crates/ironclaw_reborn_composition/assets/prompts/default-system.md:13
The new block moves delivery-target workflow steering into the seeded default system prompt even though this PR already exposes that guidance through tool/schema/runtime surfaces. That broadens the behavior into baseline assistant identity and creates drift risk with the capability-owned descriptions.
|
|
||
| When a tool result is partial, truncated, failed, or otherwise shows the requested work is unfinished, adapt and continue autonomously. Ask the user only when progress requires external information, approval, or a product decision. | ||
|
|
||
| ## Delivery Targets |
There was a problem hiding this comment.
Medium — Delivery steering was added to the default system prompt.
This behavior should stay in tool/schema/runtime guidance rather than the seeded default system prompt. The new Delivery Targets block changes the baseline prompt for every new local-dev runtime, while the PR already adds the steering to the trigger schema/manifest and outbound delivery tool descriptions where it is scoped to the visible capability surface.
Fix: Remove this default-system.md section and the test assertion that requires the guidance in the system prompt; keep the guidance in the trigger_create description/schema plus the outbound delivery list/set tool descriptions.
Also flagged by: conventions/Medium, approach/Low, local-patterns/Medium, maintainability/Medium
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Guide routine and trigger creation to discover and choose outbound delivery targets before declaring delivery products unavailable.
Stats: 2 findings (from 3 raw, 2 after dedup) across 2 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0
-
Medium No integration test covers delivery target selection before triggers (
crates/ironclaw_reborn_composition/assets/prompts/default-system.md:15-16, confidence 75) — anchor: crates/ironclaw_reborn_composition/assets/prompts/default-system.md:15
The PR adds model-visible behavior requiring outbound delivery target discovery and selection before creating a routine or trigger, but the added tests only assert prompt/descriptor text and exercise outbound list/set separately. There is no integration or trace test for a user request to send trigger or routine results to Slack that verifies the ordered flow list targets -> set target -> trigger_create. Also flagged by: maintainability/Low. -
Low Model-visible description names the non-callable trigger id (
crates/ironclaw_reborn_composition/src/runtime/local_dev/outbound_delivery.rs:28-28, confidence 75) — anchor: crates/ironclaw_reborn_composition/assets/prompts/default-system.md:15
The outbound list tool description tells the model to call this beforebuiltin.trigger_create, but local-dev model-facing instructions use provider tool names with double underscores. In the same PR, the default system prompt namesbuiltin__outbound_delivery_targets_listandbuiltin__outbound_delivery_target_set; using the dotted capability id here introduces a second name for the model-visible trigger tool and can make the sequencing hint less actionable.
|
|
||
| ## Delivery Targets | ||
|
|
||
| - When visible outbound delivery target tools exist and the user asks to send final replies, routine results, or trigger results through a product or channel such as Slack, call `builtin__outbound_delivery_targets_list` first, then call `builtin__outbound_delivery_target_set` with a returned `target_id` before creating the routine or trigger. |
There was a problem hiding this comment.
Medium — No integration test covers delivery target selection before triggers.
The PR adds model-visible behavior requiring outbound delivery target discovery and selection before creating a routine or trigger, but the added tests only assert prompt/descriptor text and exercise outbound list/set separately. There is no integration or trace test for a user request to send trigger or routine results to Slack that verifies the ordered flow list targets -> set target -> trigger_create.
Fix: Add a caller-level integration/trace test for a user request to send routine or trigger results to Slack with an available outbound target, asserting outbound target list and set run before trigger_create.
Also flagged by: maintainability/Low
| "builtin__outbound_delivery_target_set"; | ||
| const OUTBOUND_DELIVERY_TARGETS_LIST_DESCRIPTION: &str = "List available outbound delivery targets for final replies and routine/trigger results, such as Slack DMs or Slack channels. Use before saying a delivery product is unavailable or asking the user to reconnect it."; | ||
| const OUTBOUND_DELIVERY_TARGET_SET_DESCRIPTION: &str = "Set the current user's final-reply delivery target to an id returned by builtin__outbound_delivery_targets_list. Use only after the user asks to send replies or routine/trigger results through that product or channel."; | ||
| const OUTBOUND_DELIVERY_TARGETS_LIST_DESCRIPTION: &str = "List available outbound delivery targets for final replies and routine/trigger results, such as Slack DMs or Slack channels. When the user asks to send routine or trigger results through Slack or another product/channel, call this before builtin.trigger_create and before saying a delivery product is unavailable or asking the user to reconnect it."; |
There was a problem hiding this comment.
Low — Model-visible description names the non-callable trigger id.
The outbound list tool description tells the model to call this before builtin.trigger_create, but local-dev model-facing instructions use provider tool names with double underscores. In the same PR, the default system prompt names builtin__outbound_delivery_targets_list and builtin__outbound_delivery_target_set; using the dotted capability id here introduces a second name for the model-visible trigger tool and can make the sequencing hint less actionable.
Fix: Use the provider tool spelling for the model-facing trigger tool, or avoid naming it and match the set-tool wording: before creating the routine or trigger.
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughEnforces a three-step sequencing contract for outbound delivery before trigger creation via extracted capability surface and updated capability descriptions; introduces thread-safe mutable provider registry with RwLock-backed map; wires registry into runtime with task-level accessors and communication context integration; refactors local-dev to use shared capability surface; Slack host-beta registers providers directly on runtime; webui and connectable-channel validate local-runtime presence; integration tests verify the enforced three-call sequence. Additionally, Slack multi-message rendering is introduced via new ChangesOutbound Delivery Target Sequencing Before Trigger Create
Slack Multi-Message Rendering and Delivery
Sequence Diagram(s)sequenceDiagram
participant User
participant LocalDevRuntime
participant OutboundDeliveryTriggerGateway
participant ListTargets as list targets
participant SetTarget as set target
participant TriggerCreate as trigger_create
User->>LocalDevRuntime: "create daily Slack DM trigger"
LocalDevRuntime->>OutboundDeliveryTriggerGateway: stream_model_with_capabilities (call 1)
OutboundDeliveryTriggerGateway-->>LocalDevRuntime: select list tool call
LocalDevRuntime->>ListTargets: execute
ListTargets-->>LocalDevRuntime: tool result 1
LocalDevRuntime->>OutboundDeliveryTriggerGateway: stream_model_with_capabilities (call 2)
OutboundDeliveryTriggerGateway-->>LocalDevRuntime: select set tool call
LocalDevRuntime->>SetTarget: execute
SetTarget-->>LocalDevRuntime: tool result 2
LocalDevRuntime->>OutboundDeliveryTriggerGateway: stream_model_with_capabilities (call 3, assert 3 tool results present)
OutboundDeliveryTriggerGateway-->>LocalDevRuntime: select trigger_create tool call
LocalDevRuntime->>TriggerCreate: execute
TriggerCreate-->>LocalDevRuntime: tool result 3
LocalDevRuntime-->>User: run complete, 3 tool-result refs persisted with enforced capability IDs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~68 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
github-actions
Bot
added
size: L
c334a1f to
6b4efab
Compare
6b4efab to
7b2f622
Compare
8eea976 to
d3c2fcd
Compare
github-actions
Bot
added
size: XL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_reborn_composition/src/slack_host_beta.rs`:
- Around line 469-486: The current code accepts replacement of the outbound
delivery target provider without ensuring the delivery hook is also atomically
replaced, which violates the first-writer-wins semantics. Modify the match
statement handling OutboundDeliveryTargetRegistrationOutcome::Replaced to
either: (1) return an error to fail closed on replacement attempts instead of
just logging and continuing, or (2) verify that the existing registration uses
the same Slack config before allowing replacement. This ensures target selection
and triggered delivery remain consistent and preserves the scope of outbound
records as per coding guidelines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ae8952d9-6489-4ba9-9c23-66e66f24a797
📒 Files selected for processing (4)
crates/ironclaw_reborn_composition/src/outbound_preferences.rscrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_reborn_composition/src/runtime/tests/outbound_delivery.rscrates/ironclaw_reborn_composition/src/slack_host_beta.rs
github-actions
Bot
added
size: XL
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_slack_v2_adapter/src/adapter.rs`:
- Around line 190-196: The loop processing multiple requests fails fast
per-part, returning a retryable error if any part fails, even after previous
parts have succeeded. This causes the entire envelope to be eligible for replay,
reposting already-delivered parts and creating duplicates for non-idempotent
sends. Track whether any part has been successfully delivered before the error,
and if so, convert retryable errors from send_slack_post_message into
non-retryable errors rather than propagating them as-is. Apply this same logic
to both occurrences of this pattern (the one shown and the additional location
mentioned in the comment).
In `@crates/ironclaw_slack_v2_adapter/src/delivery.rs`:
- Around line 157-164: The from_http_status function currently classifies HTTP
408 (Request Timeout) as a permanent error in the else branch, but it should be
treated as a retryable transient error. Add 408 to the condition that returns
Self::Retryable alongside the existing checks for status >= 500 and status ==
429, so that timeout errors will be retried instead of being dropped.
In `@crates/ironclaw_slack_v2_adapter/src/mrkdwn.rs`:
- Around line 309-313: The code slices `&str` using character counts and byte
offsets without validation, which violates string safety invariants and can
panic on multi-byte UTF-8 characters. In the strip_heading_marker function, the
hash_count variable (a character count) is used directly as a byte offset in the
slice &trimmed[hash_count..], and in the convert_markdown_links function,
slicing is performed at computed offsets without confirming character
boundaries. Refactor both functions to iterate using char_indices() instead of
maintaining direct byte or character indices, ensuring each slice operation
respects UTF-8 character boundaries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 60567f66-ced7-4d90-89f2-30b74264ca00
📒 Files selected for processing (6)
crates/ironclaw_reborn_composition/src/slack_delivery.rscrates/ironclaw_slack_v2_adapter/src/adapter.rscrates/ironclaw_slack_v2_adapter/src/delivery.rscrates/ironclaw_slack_v2_adapter/src/lib.rscrates/ironclaw_slack_v2_adapter/src/mrkdwn.rscrates/ironclaw_slack_v2_adapter/src/render.rs
d36ddb5 to
e2e33af
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_reborn_composition/src/runtime.rs (1)
1576-1585: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUpdate the shutdown contract for the trace flush worker.
shutdown()now stopstrace_flush_worker, but the doc still only mentions the turn-runner and budget projection.Suggested doc update
- /// Stop the turn-runner worker and the budget-event projection. - /// Awaits both tasks before returning so background state is fully - /// drained when the runtime drops. + /// Stop runtime background workers, including the trigger poller, trace + /// flush worker, turn-runner, and budget-event projection. + /// Awaits shutdown before returning so background state is fully drained + /// when the runtime drops.As per coding guidelines, “When you change behavior in a function, re-read its docstring and adjacent comments — update or delete them in the same change.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_reborn_composition/src/runtime.rs` around lines 1576 - 1585, The docstring for the shutdown method in runtime.rs is outdated and does not reflect the current implementation. The method now stops the trace_flush_worker in addition to the turn-runner worker and budget-event projection. Update the docstring (the documentation comment above the shutdown function) to include the trace_flush_worker in the list of components that are stopped and awaited during shutdown, ensuring the documentation accurately describes the behavior of the implementation.Source: Coding guidelines
♻️ Duplicate comments (1)
crates/ironclaw_reborn_composition/src/slack_host_beta.rs (1)
469-485:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftReject replacement before mutating the runtime registry.
Replacedis handled as a build error, but the registry operation has already replaced the provider by then. A second build can returnErrwhile leaving runtime target discovery on the new Slack config and the first-writer trigger delivery hook on the old config. Make the registry insert-if-absent/non-mutating on existing keys, or prove same-config idempotency before replacing. Also extend the regression test to assert the runtime provider remains unchanged after the rejected second build.As per coding guidelines, “Fail closed for auth, approvals, trust, filesystem containment, network policy, secret leases, runtime selection, and adapter identity”; the PR context says the registry inserts/replaces providers before returning
Replaced.🤖 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_host_beta.rs` around lines 469 - 485, The register_outbound_delivery_target_provider call mutates the runtime registry before the OutboundDeliveryTargetRegistrationOutcome::Replaced case is evaluated and rejected as an error, leaving the registry in an inconsistent state. Either modify the registry operation to be non-mutating/atomic on existing keys (check-then-insert semantics), or validate that a provider with the same configuration already exists and fail before calling register_outbound_delivery_target_provider with SLACK_V2_ADAPTER_ID. Additionally, extend the regression test to assert that after a rejected second build attempt, the runtime provider for SLACK_V2_ADAPTER_ID remains unchanged from the first build.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_reborn_composition/src/runtime.rs`:
- Around line 1094-1110: The provider_key parameter in
register_outbound_delivery_target_provider uses impl Into<String>, which allows
string typos to create duplicate registrations instead of triggering replacement
detection. Create a new newtype OutboundDeliveryTargetProviderKey to wrap the
provider identifier and replace the provider_key parameter type from impl
Into<String> to OutboundDeliveryTargetProviderKey. Update the
registry.register_provider call to pass the typed key, and ensure the underlying
registry method accepts this newtype rather than raw strings, following the
coding guideline to use newtypes for identifiers instead of raw String types.
In `@crates/ironclaw_reborn_composition/src/runtime/local_dev.rs`:
- Around line 542-545: The `tracing::warn!` macro call that logs the trajectory
observer on_capability_result panic is using the wrong log level and can corrupt
the REPL/TUI display. Change `tracing::warn!` to `tracing::debug!` for this
internal diagnostic call, keeping the same capability_id field and message
content, to ensure it does not interfere with the terminal UI while still
providing diagnostic information at the appropriate debug level.
---
Outside diff comments:
In `@crates/ironclaw_reborn_composition/src/runtime.rs`:
- Around line 1576-1585: The docstring for the shutdown method in runtime.rs is
outdated and does not reflect the current implementation. The method now stops
the trace_flush_worker in addition to the turn-runner worker and budget-event
projection. Update the docstring (the documentation comment above the shutdown
function) to include the trace_flush_worker in the list of components that are
stopped and awaited during shutdown, ensuring the documentation accurately
describes the behavior of the implementation.
---
Duplicate comments:
In `@crates/ironclaw_reborn_composition/src/slack_host_beta.rs`:
- Around line 469-485: The register_outbound_delivery_target_provider call
mutates the runtime registry before the
OutboundDeliveryTargetRegistrationOutcome::Replaced case is evaluated and
rejected as an error, leaving the registry in an inconsistent state. Either
modify the registry operation to be non-mutating/atomic on existing keys
(check-then-insert semantics), or validate that a provider with the same
configuration already exists and fail before calling
register_outbound_delivery_target_provider with SLACK_V2_ADAPTER_ID.
Additionally, extend the regression test to assert that after a rejected second
build attempt, the runtime provider for SLACK_V2_ADAPTER_ID remains unchanged
from the first build.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 88b769b2-c340-4fa8-b2e1-cd34eb9c4f0f
📒 Files selected for processing (21)
crates/ironclaw_host_runtime/src/first_party_tools/schemas.rscrates/ironclaw_host_runtime/src/first_party_tools/trigger_management.rscrates/ironclaw_host_runtime/tests/tool_surface_contract.rscrates/ironclaw_reborn_composition/src/lib.rscrates/ironclaw_reborn_composition/src/outbound_delivery_capability_surface.rscrates/ironclaw_reborn_composition/src/outbound_preferences.rscrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_reborn_composition/src/runtime/local_dev.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/outbound_delivery.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/tests.rscrates/ironclaw_reborn_composition/src/runtime/tests/outbound_delivery.rscrates/ironclaw_reborn_composition/src/slack_connectable_channel.rscrates/ironclaw_reborn_composition/src/slack_delivery.rscrates/ironclaw_reborn_composition/src/slack_host_beta.rscrates/ironclaw_reborn_composition/src/webui.rscrates/ironclaw_slack_v2_adapter/src/adapter.rscrates/ironclaw_slack_v2_adapter/src/delivery.rscrates/ironclaw_slack_v2_adapter/src/lib.rscrates/ironclaw_slack_v2_adapter/src/mrkdwn.rscrates/ironclaw_slack_v2_adapter/src/render.rsdocs/reborn/contracts/triggers.md
💤 Files with no reviewable changes (1)
- docs/reborn/contracts/triggers.md
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_reborn_composition/src/runtime.rs (1)
1576-1585: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUpdate the shutdown contract for the trace flush worker.
shutdown()now stopstrace_flush_worker, but the doc still only mentions the turn-runner and budget projection.Suggested doc update
- /// Stop the turn-runner worker and the budget-event projection. - /// Awaits both tasks before returning so background state is fully - /// drained when the runtime drops. + /// Stop runtime background workers, including the trigger poller, trace + /// flush worker, turn-runner, and budget-event projection. + /// Awaits shutdown before returning so background state is fully drained + /// when the runtime drops.As per coding guidelines, “When you change behavior in a function, re-read its docstring and adjacent comments — update or delete them in the same change.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_reborn_composition/src/runtime.rs` around lines 1576 - 1585, The docstring for the shutdown method in runtime.rs is outdated and does not reflect the current implementation. The method now stops the trace_flush_worker in addition to the turn-runner worker and budget-event projection. Update the docstring (the documentation comment above the shutdown function) to include the trace_flush_worker in the list of components that are stopped and awaited during shutdown, ensuring the documentation accurately describes the behavior of the implementation.Source: Coding guidelines
♻️ Duplicate comments (1)
crates/ironclaw_reborn_composition/src/slack_host_beta.rs (1)
469-485:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftReject replacement before mutating the runtime registry.
Replacedis handled as a build error, but the registry operation has already replaced the provider by then. A second build can returnErrwhile leaving runtime target discovery on the new Slack config and the first-writer trigger delivery hook on the old config. Make the registry insert-if-absent/non-mutating on existing keys, or prove same-config idempotency before replacing. Also extend the regression test to assert the runtime provider remains unchanged after the rejected second build.As per coding guidelines, “Fail closed for auth, approvals, trust, filesystem containment, network policy, secret leases, runtime selection, and adapter identity”; the PR context says the registry inserts/replaces providers before returning
Replaced.🤖 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_host_beta.rs` around lines 469 - 485, The register_outbound_delivery_target_provider call mutates the runtime registry before the OutboundDeliveryTargetRegistrationOutcome::Replaced case is evaluated and rejected as an error, leaving the registry in an inconsistent state. Either modify the registry operation to be non-mutating/atomic on existing keys (check-then-insert semantics), or validate that a provider with the same configuration already exists and fail before calling register_outbound_delivery_target_provider with SLACK_V2_ADAPTER_ID. Additionally, extend the regression test to assert that after a rejected second build attempt, the runtime provider for SLACK_V2_ADAPTER_ID remains unchanged from the first build.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_reborn_composition/src/runtime.rs`:
- Around line 1094-1110: The provider_key parameter in
register_outbound_delivery_target_provider uses impl Into<String>, which allows
string typos to create duplicate registrations instead of triggering replacement
detection. Create a new newtype OutboundDeliveryTargetProviderKey to wrap the
provider identifier and replace the provider_key parameter type from impl
Into<String> to OutboundDeliveryTargetProviderKey. Update the
registry.register_provider call to pass the typed key, and ensure the underlying
registry method accepts this newtype rather than raw strings, following the
coding guideline to use newtypes for identifiers instead of raw String types.
In `@crates/ironclaw_reborn_composition/src/runtime/local_dev.rs`:
- Around line 542-545: The `tracing::warn!` macro call that logs the trajectory
observer on_capability_result panic is using the wrong log level and can corrupt
the REPL/TUI display. Change `tracing::warn!` to `tracing::debug!` for this
internal diagnostic call, keeping the same capability_id field and message
content, to ensure it does not interfere with the terminal UI while still
providing diagnostic information at the appropriate debug level.
---
Outside diff comments:
In `@crates/ironclaw_reborn_composition/src/runtime.rs`:
- Around line 1576-1585: The docstring for the shutdown method in runtime.rs is
outdated and does not reflect the current implementation. The method now stops
the trace_flush_worker in addition to the turn-runner worker and budget-event
projection. Update the docstring (the documentation comment above the shutdown
function) to include the trace_flush_worker in the list of components that are
stopped and awaited during shutdown, ensuring the documentation accurately
describes the behavior of the implementation.
---
Duplicate comments:
In `@crates/ironclaw_reborn_composition/src/slack_host_beta.rs`:
- Around line 469-485: The register_outbound_delivery_target_provider call
mutates the runtime registry before the
OutboundDeliveryTargetRegistrationOutcome::Replaced case is evaluated and
rejected as an error, leaving the registry in an inconsistent state. Either
modify the registry operation to be non-mutating/atomic on existing keys
(check-then-insert semantics), or validate that a provider with the same
configuration already exists and fail before calling
register_outbound_delivery_target_provider with SLACK_V2_ADAPTER_ID.
Additionally, extend the regression test to assert that after a rejected second
build attempt, the runtime provider for SLACK_V2_ADAPTER_ID remains unchanged
from the first build.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 88b769b2-c340-4fa8-b2e1-cd34eb9c4f0f
📒 Files selected for processing (21)
crates/ironclaw_host_runtime/src/first_party_tools/schemas.rscrates/ironclaw_host_runtime/src/first_party_tools/trigger_management.rscrates/ironclaw_host_runtime/tests/tool_surface_contract.rscrates/ironclaw_reborn_composition/src/lib.rscrates/ironclaw_reborn_composition/src/outbound_delivery_capability_surface.rscrates/ironclaw_reborn_composition/src/outbound_preferences.rscrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_reborn_composition/src/runtime/local_dev.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/outbound_delivery.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/tests.rscrates/ironclaw_reborn_composition/src/runtime/tests/outbound_delivery.rscrates/ironclaw_reborn_composition/src/slack_connectable_channel.rscrates/ironclaw_reborn_composition/src/slack_delivery.rscrates/ironclaw_reborn_composition/src/slack_host_beta.rscrates/ironclaw_reborn_composition/src/webui.rscrates/ironclaw_slack_v2_adapter/src/adapter.rscrates/ironclaw_slack_v2_adapter/src/delivery.rscrates/ironclaw_slack_v2_adapter/src/lib.rscrates/ironclaw_slack_v2_adapter/src/mrkdwn.rscrates/ironclaw_slack_v2_adapter/src/render.rsdocs/reborn/contracts/triggers.md
💤 Files with no reviewable changes (1)
- docs/reborn/contracts/triggers.md
🛑 Comments failed to post (2)
crates/ironclaw_reborn_composition/src/runtime.rs (1)
1094-1110: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Use a typed provider key at this registration seam.
provider_key: impl Into<String>is the cross-layer identity that drivesRegisteredvsReplaced; a target-id/empty/string typo can register a second provider instead of tripping replacement detection. Introduce/use a validatedOutboundDeliveryTargetProviderKeyand have the registry/runtime/Slack caller pass that type.As per coding guidelines, “Use newtypes for identifiers … instead of raw
String,&str, oruuid::Uuid.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_reborn_composition/src/runtime.rs` around lines 1094 - 1110, The provider_key parameter in register_outbound_delivery_target_provider uses impl Into<String>, which allows string typos to create duplicate registrations instead of triggering replacement detection. Create a new newtype OutboundDeliveryTargetProviderKey to wrap the provider identifier and replace the provider_key parameter type from impl Into<String> to OutboundDeliveryTargetProviderKey. Update the registry.register_provider call to pass the typed key, and ensure the underlying registry method accepts this newtype rather than raw strings, following the coding guideline to use newtypes for identifiers instead of raw String types.Source: Coding guidelines
crates/ironclaw_reborn_composition/src/runtime/local_dev.rs (1)
542-545:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
debug!for observer-panic diagnostics.Line 542 emits an internal diagnostic with
tracing::warn!; this can corrupt the REPL/TUI display. Keep capability staging best-effort, but log this atdebug!.Minimal fix
- tracing::warn!( + tracing::debug!( capability_id = capability_id.as_str(), "trajectory observer on_capability_result panicked; dropping event" );As per coding guidelines, “REPL/TUI logging: info!/warn! corrupt the terminal UI — internal diagnostics use debug!”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_reborn_composition/src/runtime/local_dev.rs` around lines 542 - 545, The `tracing::warn!` macro call that logs the trajectory observer on_capability_result panic is using the wrong log level and can corrupt the REPL/TUI display. Change `tracing::warn!` to `tracing::debug!` for this internal diagnostic call, keeping the same capability_id field and message content, to ensure it does not interfere with the terminal UI while still providing diagnostic information at the appropriate debug level.Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_reborn_composition/src/slack_host_beta.rs (1)
543-607:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMake hook/provider wiring atomic.
set_trigger_post_submit_hookmutates runtime state before provider registration can fail or reportReplaced. If registration errors after Line 543, the runtime can be left with the Slack delivery hook wired but no matching runtime outbound-target provider; ifReplacedmeans the registry already swapped the provider, returningErrat Line 604 does not undo the mutation. Use one runtime API that claims the hook and inserts the provider atomically, or make provider registration insert-only with rollback on any later hook conflict. Downstream WebUI target discovery consumesruntime.outbound_delivery_target_provider(), so partial wiring directly drifts the visible target surface. As per coding guidelines, “Fail closed for auth, approvals, trust, filesystem containment, network policy, secret leases, runtime selection, and adapter identity” and “Preserve tenant/user/agent/project/mission/thread scope on authority, state, memory, process, network, outbound, resource, and event records.”🤖 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_host_beta.rs` around lines 543 - 607, The `set_trigger_post_submit_hook(hook)` call at line 543 mutates runtime state independently from the subsequent `register_outbound_delivery_target_provider()` call around line 595. If provider registration fails or reports `Replaced`, the runtime is left in an inconsistent state with the hook wired but no matching provider. Refactor to make these operations atomic: either consolidate into a single runtime API that claims the hook and registers the provider together, or implement rollback logic where if `register_outbound_delivery_target_provider()` fails or returns `OutboundDeliveryTargetRegistrationOutcome::Replaced`, undo the hook mutation to restore consistency. This prevents partial state corruption that would be visible to downstream consumers like `runtime.outbound_delivery_target_provider()`.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_reborn_composition/src/slack_host_beta.rs`:
- Line 37: The runtime state mutations for hook and provider registration must
be atomic to prevent leaving the runtime in an inconsistent state. In the code
block around lines 543–607, the calls to set_trigger_post_submit_hook() and
register_outbound_delivery_target_provider() are currently sequential and can
fail independently. Either wrap both mutations in a single transaction so they
succeed or fail together, or pre-validate that provider registration will
succeed before calling set_trigger_post_submit_hook(). This ensures adapter
identity and outbound delivery state remain synchronized per the runtime wiring
contract, preventing hook-set/provider-missing states that cause permanent build
failures.
---
Outside diff comments:
In `@crates/ironclaw_reborn_composition/src/slack_host_beta.rs`:
- Around line 543-607: The `set_trigger_post_submit_hook(hook)` call at line 543
mutates runtime state independently from the subsequent
`register_outbound_delivery_target_provider()` call around line 595. If provider
registration fails or reports `Replaced`, the runtime is left in an inconsistent
state with the hook wired but no matching provider. Refactor to make these
operations atomic: either consolidate into a single runtime API that claims the
hook and registers the provider together, or implement rollback logic where if
`register_outbound_delivery_target_provider()` fails or returns
`OutboundDeliveryTargetRegistrationOutcome::Replaced`, undo the hook mutation to
restore consistency. This prevents partial state corruption that would be
visible to downstream consumers like
`runtime.outbound_delivery_target_provider()`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 722af1d2-941d-4d4b-8d3b-51a67b1227f1
📒 Files selected for processing (3)
crates/ironclaw_reborn_composition/src/outbound_preferences.rscrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_reborn_composition/src/slack_host_beta.rs
| WebhookAuth, | ||
| }; | ||
| use secrecy::{ExposeSecret, SecretString}; | ||
| use sha2::{Digest, Sha256}; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify `sha2` is available to production code in this crate.
set -euo pipefail
manifest="$(fd Cargo.toml crates/ironclaw_reborn_composition | head -n 1)"
test -n "$manifest"
echo "Manifest: $manifest"
rg -n -C 3 '^\s*sha2\s*=' "$manifest"Repository: nearai/ironclaw
Length of output: 750
Hook and provider registration must be atomic.
Lines 543–607 mutate runtime state in two steps: set_trigger_post_submit_hook() succeeds, then register_outbound_delivery_target_provider() can fail or report Replaced. If the second call fails after the hook is set, the runtime is left with a dangling hook and no provider — future builds will see hook-set/provider-missing and fail closed permanently.
Wrap both mutations in a single transaction or pre-validate provider registration before setting the hook. This is a fail-loud/fail-closed invariant: adapter identity and outbound delivery state must not split (CLAUDE.md runtime wiring contract).
Note: sha2 is correctly declared in [dependencies] at line 161 of Cargo.toml; production import is valid.
🤖 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_host_beta.rs` at line 37, The
runtime state mutations for hook and provider registration must be atomic to
prevent leaving the runtime in an inconsistent state. In the code block around
lines 543–607, the calls to set_trigger_post_submit_hook() and
register_outbound_delivery_target_provider() are currently sequential and can
fail independently. Either wrap both mutations in a single transaction so they
succeed or fail together, or pre-validate that provider registration will
succeed before calling set_trigger_post_submit_hook(). This ensures adapter
identity and outbound delivery state remain synchronized per the runtime wiring
contract, preventing hook-set/provider-missing states that cause permanent build
failures.
* fix(reborn): steer routine delivery through outbound targets * fix(reborn): address outbound delivery review feedback (nearai#4780) * fix(reborn): fail loud on outbound target registration (nearai#4780) * fix(slack): harden outbound delivery rendering * fix(slack): address outbound delivery review feedback * fix(slack): allow idempotent host remounts
2 participants
Summary
Tests
Stacked on #4779.