feat(product-workflow): prepare WebUI binding slice - #3727
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 implements a concrete conversation binding layer that bridges product adapter requests to the canonical ironclaw_conversations service, utilizing a trusted installation resolver to derive TenantId and default scopes from host configuration. It also introduces an InMemoryIdempotencyLedger for local development and testing, and completes the implementation of projection subscription resolution within the DefaultProductWorkflow. Feedback was provided regarding the use of a wildcard arm in an enum mapping function, recommending an exhaustive match statement to improve compile-time safety.
| match payload { | ||
| ProductInboundPayload::UserMessage(message) => match message.trigger { | ||
| ProductTriggerReason::DirectChat => ProductConversationRouteKind::Direct, | ||
| ProductTriggerReason::BotMention | ||
| | ProductTriggerReason::ReplyToBot | ||
| | ProductTriggerReason::BotCommand | ||
| | ProductTriggerReason::LinkedThreadAction => ProductConversationRouteKind::Shared, | ||
| }, | ||
| ProductInboundPayload::Command(command) => match command.trigger { | ||
| ProductTriggerReason::DirectChat => ProductConversationRouteKind::Direct, | ||
| ProductTriggerReason::BotMention | ||
| | ProductTriggerReason::ReplyToBot | ||
| | ProductTriggerReason::BotCommand | ||
| | ProductTriggerReason::LinkedThreadAction => ProductConversationRouteKind::Shared, | ||
| }, | ||
| _ => ProductConversationRouteKind::Direct, | ||
| } |
There was a problem hiding this comment.
The match statement in route_kind_for_payload uses a wildcard _ arm. Per the general rules, mapping between enums should be exhaustive to ensure all cases are explicitly handled and to catch new variants at compile time.
match payload {
ProductInboundPayload::UserMessage(message) => match message.trigger {
ProductTriggerReason::DirectChat => ProductConversationRouteKind::Direct,
ProductTriggerReason::BotMention
| ProductTriggerReason::ReplyToBot
| ProductTriggerReason::BotCommand
| ProductTriggerReason::LinkedThreadAction => ProductConversationRouteKind::Shared,
},
ProductInboundPayload::Command(command) => match command.trigger {
ProductTriggerReason::DirectChat => ProductConversationRouteKind::Direct,
ProductTriggerReason::BotMention
| ProductTriggerReason::ReplyToBot
| ProductTriggerReason::BotCommand
| ProductTriggerReason::LinkedThreadAction => ProductConversationRouteKind::Shared,
},
ProductInboundPayload::ApprovalResolution(_)
| ProductInboundPayload::AuthResolution(_)
| ProductInboundPayload::SubscriptionRequest(_)
| ProductInboundPayload::LinkedThreadAction(_)
| ProductInboundPayload::NoOp => ProductConversationRouteKind::Direct,
}References
- In functions that map one enum to another, use an exhaustive match statement instead of a wildcard _ arm. This forces a compile-time error when new variants are added, ensuring all cases are explicitly handled.
|
Security/correctness review findings have been addressed in What changed:
Regression coverage added:
Validation run:
|
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
Multi-agent code review
Reviewed with Security, Bugs, Performance/Concurrency, Tests, and Conventions lenses.
Findings: 10 total
- Critical/High: 0
- Medium: 10
- Reviewers failed: 0
Main risk areas: trusted ProductWorkflow scope persistence, stale idempotency settlement, durable-state refresh concurrency, and missing regression coverage for the new projection/idempotency branches.
| }; | ||
|
|
||
| let mut changed = false; | ||
| if binding.agent_id.is_none() && trusted_agent_id.is_some() { |
There was a problem hiding this comment.
When this path resolves an existing binding that still has no persisted agent/project scope, it mutates the binding to whatever trusted defaults are configured today. That can silently re-scope an old external conversation route after installation config changes or after a legacy unscoped bind was created, which conflicts with the new guardrail that first-contact defaults are persisted rather than later overlaid. Please apply trusted defaults only when creating the binding, or reject/migrate existing unscoped bindings explicitly.
|
|
||
| async fn settle(&self, action: ProductInboundAction) -> Result<(), ProductWorkflowError> { | ||
| let mut state = self.lock_state()?; | ||
| if state.settled.contains_key(&action.fingerprint) { |
There was a problem hiding this comment.
This returns Ok(()) for any later settle once the fingerprint exists in settled, even if the caller is an older expired action whose reservation was reclaimed and settled by a different action_id. In that race the stale dispatch believes its terminal outcome was durably accepted, violating the stale-reservation protection invariant. Please only treat settle as idempotent when the settled action_id matches; otherwise return the superseded-reservation transient error.
| request: ResolveConversationRequest, | ||
| ) -> Result<ConversationBindingResolution, InboundTurnError> { | ||
| #[cfg(any(feature = "libsql", feature = "postgres"))] | ||
| self.refresh_state_from_repository().await?; |
There was a problem hiding this comment.
With libsql/postgres enabled, lookup_binding refreshes the shared in-memory snapshot without taking mutation_lock, while all mutating paths hold that lock across refresh and persist. A lookup can load an older revision while a writer is saving, then install that stale snapshot after the writer commits, causing projection lookups to miss fresh bindings under concurrent traffic. Please take the same mutation lock around this refresh/read path, or make refresh revision-aware so it cannot move the cache backward.
| request: ResolveConversationRequest, | ||
| trusted_agent_id: Option<ironclaw_host_api::AgentId>, | ||
| trusted_project_id: Option<ironclaw_host_api::ProjectId>, | ||
| ) -> Result<ConversationBindingResolution, InboundTurnError> { |
There was a problem hiding this comment.
The default implementation accepts trusted host-owned scope and then discards it, falling back to the legacy unscoped resolver. Since this is a public trait method, any implementer that does not override it will compile while silently violating the ProductWorkflow boundary that default agent/project scope must be persisted on first bind. Please make this method required, or have the default return an explicit unsupported error instead of ignoring the scope.
| Err(ProductAdapterError::Internal { | ||
| detail: ironclaw_product_adapters::RedactedString::new( | ||
| "projection subscription resolution not yet implemented", | ||
| let ProductInboundPayload::SubscriptionRequest(payload) = envelope.payload() else { |
There was a problem hiding this comment.
This new public projection resolver has a distinct MalformedInboundPayload branch for non-subscription envelopes, but the contract tests only exercise subscription payloads. Please add projection_subscription_rejects_non_subscription_payload or equivalent coverage so this caller-facing error contract does not regress.
| thread_id_hint: Option<&str>, | ||
| ) -> Result<ironclaw_host_api::ThreadId, ProductAdapterError> { | ||
| if let Some(thread_id_hint) = thread_id_hint { | ||
| let hinted = ironclaw_host_api::ThreadId::new(thread_id_hint).map_err(|_| { |
There was a problem hiding this comment.
Projection tests cover a well-formed but mismatched thread_id_hint, but not the malformed external-input path here. Please add coverage such as projection_subscription_rejects_malformed_thread_hint to assert invalid hints return MalformedInboundPayload rather than being treated as binding access failures.
| fingerprint: ActionFingerprintKey, | ||
| received_at: DateTime<Utc>, | ||
| ) -> Result<IdempotencyDecision, ProductWorkflowError> { | ||
| let mut state = self.lock_state()?; |
There was a problem hiding this comment.
The ledger uses a mutex to make same-fingerprint reservation atomic, but the current tests only exercise this sequentially. Please add a contention test, for example in_memory_idempotency_ledger_allows_only_one_concurrent_reservation, that races two begin_or_replay calls on the same fingerprint and asserts only one gets New.
| "unknown adapter installation", | ||
| ))) | ||
| } | ||
| ProductWorkflowError::BindingRequired { reason } => Some(ProductInboundAck::Rejected( |
There was a problem hiding this comment.
The unpaired-actor workflow test checks the first BindingRequired rejection and absence of turn submission, but not the new terminal idempotency behavior for this branch. Please add a retry assertion or a dedicated concrete_product_workflow_replays_unpaired_actor_terminal_rejection test so duplicate deliveries replay the settled permanent rejection.
| ProductWorkflowError::BindingRequired { reason } => Some(ProductInboundAck::Rejected( | ||
| ProductRejection::permanent(ProductRejectionKind::BindingRequired, reason.clone()), | ||
| )), | ||
| ProductWorkflowError::BindingAccessDenied => { |
There was a problem hiding this comment.
BindingAccessDenied is now settled as a permanent terminal rejection, but the product workflow tests do not cover that branch or its duplicate replay behavior. Please add a workflow-level access-denied case, not only a conversation-layer test, so this facade mapping stays protected.
| ironclaw_product_adapters::ProductTriggerReason::DirectChat => { | ||
| ProductConversationRouteKind::Direct | ||
| } | ||
| ironclaw_product_adapters::ProductTriggerReason::BotMention |
There was a problem hiding this comment.
The concrete ProductWorkflow path only submits DirectChat messages in tests, but this new mapping gives BotMention, ReplyToBot, BotCommand, and LinkedThreadAction shared-route semantics. Please add a concrete workflow test, for example concrete_product_workflow_bot_mention_uses_shared_route, to prove shared triggers reach binding resolution with shared access.
|
Addressed the code-review feedback in ab996fa. ✓ Fixed trusted-scope drift for existing unscoped conversation bindings: later trusted defaults now reject instead of silently re-scoping legacy bindings. Verification run: Note: scripts/pre-commit-safety.sh still reports existing PR-wide findings outside this fix set (UTF8/TMPDIR/PANIC/PROJECTION matches in files not touched by ab996fa), so I did not fold unrelated cleanup into this review-fix commit. |
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
Multi-agent review for PR #3727 at ab996fa812b0d26372cecd65bc76fb8b8d4b86fa.
Result: blocking findings found. GitHub does not allow this account to request changes on its own PR, so this is posted as a review comment. Findings kept to the high-confidence blockers plus one regression-test gap:
- High: failed trusted-scope resolves can still mutate route access before returning an error.
- High: terminal idempotency replay for binding/install rejections is keyed without actor validation.
- High: accepted-message replay can run before validating the current envelope identity.
- Medium: add regression coverage that lookup-only / rejected resolve paths do not reserve or widen route state.
| thread_id: binding.thread_id.to_string(), | ||
| }); | ||
| } | ||
| if request.route_kind == ConversationRouteKind::Shared { |
There was a problem hiding this comment.
[High] This mutates route_access before the trusted-scope compatibility check below can fail. Because the function returns immediately on ensure_trusted_scope_not_reinterpreted, the live in-memory state is not rolled back, so a rejected Shared resolve can leave a previously direct binding widened. Please run all fallible validation before widen_binding_route_access, or stage changes in a cloned state and commit only after validation succeeds.
|
|
||
| fn terminal_ack_for_error(error: &ProductWorkflowError) -> Option<ProductInboundAck> { | ||
| match error { | ||
| ProductWorkflowError::UnknownInstallation => { |
There was a problem hiding this comment.
[High] Settling binding/install rejections as terminal outcomes is useful, but the workflow replay key is still adapter + installation + source binding + external event, with no actor identity. A failed delivery from an unpaired actor can poison that idempotency record for a later valid actor using the same event, and replay returns before the binding service can validate the current envelope. Please either include actor identity in the replay key or validate the replaying envelope against the stored resolved/rejected binding before returning Duplicate.
| installation_id: envelope.installation_id().clone(), | ||
| external_actor_ref: envelope.external_actor_ref().clone(), | ||
| external_conversation_ref: envelope.external_conversation_ref().clone(), | ||
| external_event_id: envelope.external_event_id().clone(), |
There was a problem hiding this comment.
[High] This adds event/route validation to the fresh binding path, but accepted-message replay is checked before this block and is still keyed only by source binding plus external event id. For non-terminal/released workflow actions, a matching retry can submit or replay the previously accepted message without validating the current actor, installation mapping, route access, or conversation identity. Please validate the binding before replay, or extend the replay request/store key to include the accepted actor and conversation identity.
| )?; | ||
| let binding_key = BindingKey::from_request(&request); | ||
| let external_conversation_identity = request.external_conversation_ref.identity(); | ||
| state.ensure_external_event_route( |
There was a problem hiding this comment.
[Medium] Please add regression coverage proving lookup-only and failed lookup paths do not reserve or widen route state. A targeted case like lookup_binding_miss_does_not_reserve_external_event_route should do a lookup miss, then create with the same external_event_id on a different valid conversation so an accidental reservation would fail the test.
|
Addressed the review findings in 2a04ee4. Summary:
Local verification:
Note: scripts/pre-commit-safety.sh still reports pre-existing warnings elsewhere in the PR diff outside the files changed for this review fix. |
…versationBindingService Drops the Telegram-specific product_bindings table and its libsql/postgres implementations in favor of the shared ProductConversationBindingService (PR #3727) backed by ironclaw_conversations' filesystem store (PR #3679). The shared facade fails closed on unpaired actors, so the host now reads REBORN_TELEGRAM_PAIRINGS at boot and installs the operator-trusted external-user → Reborn-user pairings idempotently before serving traffic. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ui-readiness feat(product-workflow): prepare WebUI binding slice
1 participant
Summary
Builds out the initial ProductWorkflow services needed for the WebUI rollout slice on top of the composition-root work in #3725.
ironclaw_conversationsvia a trusted adapter installation registrySecurity / correctness notes
thread_id_hintis only accepted when it exactly matches the resolved conversation binding; it cannot switch thread/tenant authority.Validation
cargo test -p ironclaw_product_workflowcargo test -p ironclaw_product_workflow in_memory_idempotency_ledger_ignores_stale_releases_after_reclaimcargo test -p ironclaw_conversationscargo test -p ironclaw_product_adapters --features test-support,host-auth-mintcargo test -p ironclaw_architecture reborn_crate_dependency_boundaries_holdcargo clippy -p ironclaw_product_workflow --all-targets -- -D warningscargo fmt --all -- --checkgit diff --checkFEATURE_PARITY.mdwas checked; it does not currently track ProductWorkflow explicitly.