feat(reborn/product-workflow): finish dispatch arms, hooks, and ack taxonomy (#3280) - #3558
Closed
nickpismenkov wants to merge 2 commits into
Closed
nickpismenkov wants to merge 2 commits into
nickpismenkov wants to merge 2 commits into
Conversation
…axonomy (#3280) Closes the remaining acceptance criteria of issue #3280 (ProductWorkflow facade). Builds on PR #3428's first slice and PR #3542's outbound policy service. Stacked onto #3542's branch so the projection-subscription arm can wrap `OutboundPolicyService::authorize_subscription` end to end. ## New port traits (`crates/ironclaw_product_workflow/src/services.rs`) Eight new port traits — implementor-side facades that production wiring will satisfy by wrapping host-layer services. Names align with the contract sketches in #3094, #3278, #3280, and #3286. - `BeforeInboundPolicy` — pre-staging hook with `Continue { rewritten_text }` / `Reject { reason }` outcomes mirroring v1 `HookPoint::BeforeInbound`. - `ProductCommandRouter` — slash-command seam (full matrix owned by #3286). - `ApprovalInteractionService` — wraps `ironclaw_approvals::ApprovalResolver`. - `AuthInteractionService` — wraps `ironclaw_authorization` capability dispatch authorizer + lease store. - `LinkedThreadActionService` — typed click-through action routing. - `MissionService` — explicit-intent mission firing per #3278 with `MissionFireRequest` / `MissionFireOutcome::{Submitted, DeferredBusy, Suppressed, Rejected}` shapes; no `OnEvent` pattern-match path. - `SystemActionService` — typed system action with mandatory accountable `system_actor_ref` + `kind`. No `is_internal` bypass (AC #15). - `ProjectionSubscriptionAuthority` — wraps `ironclaw_outbound::OutboundPolicyService::authorize_subscription` (PR #3542) to return the canonical adapter-facing `ProjectionSubscriptionRequest`. ## Workflow dispatch (`crates/ironclaw_product_workflow/src/workflow.rs`) - `DefaultProductWorkflow` keeps the 2-arg `new(inbound_turn_service, idempotency_ledger)` constructor for backwards compatibility with #3428's tests; adds `with_*` builder methods for each new port. - Every non-`UserMessage` payload now has its own dispatch arm (`dispatch_command`, `dispatch_approval`, `dispatch_auth`, `dispatch_linked_thread_action`, `dispatch_mission_action`, `dispatch_system_action`). When a port is unset, the arm returns a redacted permanent `Rejected` ack via the existing `terminal_ack_for_error` taxonomy. - `accept_inbound` short-circuits `SubscriptionRequest` before the idempotency ledger touches anything — read paths never take a `ProductInboundAction` row (AC #14). Adapters must use `resolve_projection_subscription` instead. - `resolve_projection_subscription` now reaches the `ProjectionSubscriptionAuthority` port (replaces the prior "not yet implemented" stub). ## New payload variants (`crates/ironclaw_product_adapters/src/inbound.rs`) - `MissionActionPayload { mission_intent, mission_id_hint, data }` — explicit mission intent only; ordinary chat never auto-attaches. - `SystemActionPayload { system_actor_ref, kind, scope_thread_id, data }` — typed system action with accountable actor/kind. No `is_internal`, no boolean bypass. Plus five new wire-stable typed wrappers used by the new ack variants: `MissionFireRef`, `MissionFireSuppressionReason`, `LoopGateRef`, `ProductCommandName`, `LinkedThreadActionId`. They live in the adapter crate (one direction of the dependency edge) so acks serialize without the workflow crate appearing in the wire surface. ## New ack variants (`ProductInboundAck`) - `CommandRouted { command }` - `GateHandled { gate_ref }` — used by both approval and auth resolutions (auth_request_ref projects into the wire-stable `LoopGateRef` container) - `LinkedThreadActionRouted { action_id }` - `MissionSubmitted { mission_fire_ref, submitted_run_id }` - `MissionSuppressed { mission_fire_ref, reason }` All five are durable outcomes (`is_durable_outcome() == true`); none are retryable. `terminal_ack_for_error` extends to cover `TurnResumeRejected` for the gate arms. ## BeforeInbound integration (`crates/ironclaw_product_workflow/src/inbound_turn.rs`) `DefaultInboundTurnService` gains an optional `Arc<dyn BeforeInboundPolicy>` field via the `with_before_inbound_policy` builder. The hook fires only on the genuine new-message path (after the replay-check, before binding resolution and staging). Rewrite replaces the staged content; rejection short-circuits before any `accept_inbound_message` or `submit_turn` call, so rejected input never becomes accepted transcript content (AC #8, AC #9). ## No mission auto-attach guarantee The regression test `ordinary_user_message_text_never_reaches_mission_service` drives four mission-keyword `UserMessage` envelopes through a fully-wired workflow and asserts `MissionService::fire_count() == 0` after each. This locks in the AC #12 invariant and forecloses the v1 `src/bridge/router.rs::fire_event_missions_for_message` bug shape where `MissionCadence::OnEvent { event_pattern }` pattern-matched against inbound text and side-fired missions in parallel to the turn. ## Test surface `crates/ironclaw_product_workflow/tests/`: - 9 → 12 tests in `inbound_turn_contract.rs` (+3 BeforeInbound) - 15 → 48 tests in `product_workflow_contract.rs` (+33 across all new dispatch arms, ack mappings, ledger lifecycle, projection auth, and the no-auto-attach guard) - 2 unit tests in `error.rs` unchanged Total in `ironclaw_product_workflow --features test-support`: **62 tests pass, 0 fail**. ## Acceptance criteria coverage (#3280) | AC | Status | |----|--------| | 1. UserMessage dispatches through InboundTurnService | unchanged from #3428 | | 2. Adapter-supplied internal refs non-authoritative | unchanged from #3428 | | 3. ProductInboundAction ledger row before staging | unchanged from #3428 | | 4. Duplicate replays prior outcome | unchanged from #3428 | | 5. No duplicate message/run on replay | unchanged from #3428 | | 6. Crash recovery retries with same accepted message ref | unchanged from #3428 | | 7. Pre-side-effect transient failure stays retryable | unchanged from #3428 | | 8. BeforeInbound rewrite stages rewritten content | **NEW** | | 9. BeforeInbound rejection creates no transcript content | **NEW** | | 10. Adapter-supplied internal user/thread/scope ignored | unchanged from #3428 | | 11. Refs-only TurnCoordinator submit | unchanged from #3428 | | 12. ThreadBusy → DeferredBusy | unchanged from #3428 | | 13. Pipeline acknowledgement only | unchanged from #3428 | | 14. Slash-command via ProductCommandRouter | **NEW** | | 15. Gate/auth via interaction services, no direct resume_turn | **NEW** | | 16. Mission via MissionService, explicit intent only | **NEW** | | 17. Ordinary chat never auto-attaches to a mission | **NEW (regression test)** | | 18. Product-safe rejection taxonomy redacts internals | extended for new arms | | 19. Subscription dispatch creates no ledger row | **NEW** | | 20. Typed system actions require accountable actor/scope | **NEW** | | 21. Architecture boundary tests | confirmed intact | ## Verified ``` cargo fmt --all -- --check cargo clippy -p ironclaw_product_workflow --features test-support --all-targets -- -D warnings cargo clippy -p ironclaw_product_adapters --all-targets --all-features -- -D warnings cargo clippy -p ironclaw_outbound --all-targets --all-features -- -D warnings cargo test -p ironclaw_product_workflow --features test-support cargo test -p ironclaw_product_adapters cargo test -p ironclaw_outbound cargo test -p ironclaw_architecture cargo build --workspace ``` All clean. Architecture boundary tests confirm `ironclaw_product_workflow` still has no normal-build dependency on `ironclaw_dispatcher`, `ironclaw_extensions`, `ironclaw_host_runtime`, `ironclaw_mcp`, `ironclaw_wasm`, `ironclaw_scripts`, `ironclaw_network`, `ironclaw_engine`, or `ironclaw_gateway`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
4 tasks
Closes the 4 findings from gemini-code-assist's review:
- HIGH: TurnSubmissionRejected was falling into the transient catch-all.
BeforeInboundPolicy::Reject uses this variant; rejections must settle
as permanent so retries replay the prior rejection instead of
re-evaluating the policy. Add the variant to terminal_ack_for_error
with PolicyDenied disposition.
- HIGH: MissionFireOutcome::DeferredBusy was mapped to terminal
MissionSuppressed{BusyThread}, which settled the ledger and prevented
retry when the thread freed. Add a new non-terminal MissionDeferred
ack mirroring the UserMessage DeferredBusy retry pattern; busy thread
outcomes now release the ledger for retry.
- MEDIUM: MissionFireRef doc said the workflow minted it, but the impl
has the service mint it. Updated the doc to reflect service-side
minting (the correct design — the service owns mission-fire identity).
- MEDIUM: resolve_projection_subscription silently defaulted hints to
None for non-SubscriptionRequest payloads, masking caller bugs.
Replaced with an explicit non_subscription_payload_for_resolution
rejection.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
serrrfirat
reviewed
May 13, 2026
| /// `"fire"`, `"cancel"`, `"status"`), an optional explicit mission id hint, | ||
| /// and optional per-intent data. | ||
| #[derive(Debug, Clone, PartialEq, Eq, Serialize)] | ||
| pub struct MissionActionPayload { |
Collaborator
There was a problem hiding this comment.
how did we end up implementing missions here?
Collaborator
|
This PR has too many assumptions about things that don't exist. We should be first implementing things that do exist. Please review this PR and come back with a slice instead of implementing a solution that depends on non existing parts of the architecture. |
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Waiting on PR #3542 to merge first
This PR provides the
ProjectionSubscriptionAuthorityport trait that production wiring will satisfy by wrappingironclaw_outbound::OutboundPolicyService::authorize_subscription.OutboundPolicyServiceitself lives in PR #3542 (currently CHANGES_REQUESTED; the seal-types fix is already pushed ata5819c50and CI is green). This PR's source has no compile-time dependency on #3542 — the port is a pure abstraction with aFakeProjectionSubscriptionAuthorityimpl for tests — but the production wiring that satisfies the port (in step 4 of the rollout plan) requires #3542 to merge first. Please review and approve, but hold the merge button until #3542 lands.Summary
Closes the remaining 8 acceptance criteria of issue #3280 (ProductWorkflow facade). Builds on PR #3428's first slice.
Adds eight new port traits, two new payload variants, five new ack variants, a BeforeInbound hook seam, and 38 new contract tests including the critical regression test that closes the v1 mission auto-attach bug shape.
What lands in this PR
New port traits (
crates/ironclaw_product_workflow/src/services.rs)Implementor-side facades that production wiring will satisfy by wrapping host-layer services. Names align with the contract sketches in #3094, #3278, #3280, and #3286.
BeforeInboundPolicyHookPoint::BeforeInboundportedProductCommandRouterAgentCommandService(full matrix)ApprovalInteractionServiceironclaw_approvals::ApprovalResolver(#3094)AuthInteractionServiceironclaw_authorization::CapabilityDispatchAuthorizer(#3094)LinkedThreadActionServiceMissionServiceMissionFireRequest/Outcomeper #3278SystemActionServiceProjectionSubscriptionAuthorityOutboundPolicyService::authorize_subscription(#3542)DefaultProductWorkflow::new(inbound_turn_service, idempotency_ledger)stays a 2-arg constructor for back-compat with #3428's tests;with_*builder methods plug in the new ports. When a port is unset, the corresponding arm returns a redacted permanentRejectedack via the existingterminal_ack_for_errortaxonomy.New payload variants (
crates/ironclaw_product_adapters/src/inbound.rs)MissionActionPayload { mission_intent, mission_id_hint, data }— explicit intent only.SystemActionPayload { system_actor_ref, kind, scope_thread_id, data }— typed system action with mandatory accountable actor/kind. Nois_internalbypass (AC feat: Support direct API key auth and cheap model routing #20).Plus five wire-stable typed wrappers used by the new ack variants (
MissionFireRef,MissionFireSuppressionReason,LoopGateRef,ProductCommandName,LinkedThreadActionId). They live in the adapter crate so acks serialize without the workflow crate appearing in the wire surface.New ack variants (
ProductInboundAck)CommandRouted { command }GateHandled { gate_ref }— used by both approval and auth resolutions (auth_request_ref projects into the wire-stableLoopGateRefcontainer)LinkedThreadActionRouted { action_id }MissionSubmitted { mission_fire_ref, submitted_run_id }MissionSuppressed { mission_fire_ref, reason }All five are durable outcomes; none are retryable.
BeforeInbound integration (
inbound_turn.rs)DefaultInboundTurnServicegains an optionalArc<dyn BeforeInboundPolicy>field viawith_before_inbound_policy. The hook fires only on the genuine new-message path (after the replay-check, before binding resolution). Rewrite replaces the staged content; rejection short-circuits before anyaccept_inbound_messageorsubmit_turncall so rejected input never becomes accepted transcript content.Subscription read-path bypass
accept_inboundshort-circuitsSubscriptionRequestbefore any idempotency-ledger touch — read paths never take aProductInboundActionrow (AC #19). Adapters must useresolve_projection_subscriptionfor the read path; that method now reaches theProjectionSubscriptionAuthorityport and returns the canonical adapter-facingProjectionSubscriptionRequest.No mission auto-attach guarantee
ordinary_user_message_text_never_reaches_mission_serviceis the regression closure for AC #17 / the v1src/bridge/router.rs::fire_event_missions_for_messagebug shape whereMissionCadence::OnEvent { event_pattern }pattern-matched inbound text and side-fired missions in parallel to the turn.The test drives four mission-keyword
UserMessageenvelopes through a fully-wired workflow (every port configured) and assertsMissionService::fire_count() == 0after each. Missions only fire via explicitMissionActionPayloadfrom the adapter.Acceptance criteria coverage (#3280)
Test surface
tests/inbound_turn_contract.rstests/product_workflow_contract.rssrc/error.rsunit testsironclaw_product_workflow --features test-supportIn
ironclaw_product_adapters: 11 → 16 tests (+5 covering the new payload variants and wire-stable wrappers).Out of scope (by design)
MissionServicereal implementation — issue [Reborn] Define MissionService integration with TurnCoordinator #3278 owns it; this PR ships the port shape only.AgentCommandServicefull command compatibility matrix — issue [Reborn] Preserve agent command behavior through Reborn loops and services #3286 owns it; this PR ships theProductCommandRouterseam only.Verification
cargo fmt --all -- --check: cleancargo clippy -p ironclaw_product_workflow --features test-support --all-targets -- -D warnings: cleancargo clippy -p ironclaw_product_adapters --all-targets --all-features -- -D warnings: cleancargo test -p ironclaw_product_workflow --features test-support: 62 pass, 0 failcargo test -p ironclaw_product_adapters: 16 passcargo test -p ironclaw_architecture: 13 boundary tests pass;reborn_crate_dependency_boundaries_holdconfirmsironclaw_product_workflowstill has no normal-build dependency onironclaw_dispatcher,ironclaw_extensions,ironclaw_host_runtime,ironclaw_mcp,ironclaw_wasm,ironclaw_scripts,ironclaw_network,ironclaw_engine, orironclaw_gatewaycargo build --workspace: cleanTest plan
ordinary_user_message_text_never_reaches_mission_serviceregression test really exercises all the v1 bug-shape patternssubscription_request_via_accept_inbound_is_rejected_without_ledger_rowcorrectly asserts the read-path bypass (no ledger lock, no authority call)ProductInboundAckvariants are wire-stable and serialise round-trip🤖 Generated with Claude Code