feat(reborn): add approval interaction service

[hanakannzashi]

Summary

• Added a Reborn approval interaction service for listing scoped, redacted pending approval gates and resolving click approve/deny decisions.
• Read pending approvals from canonical run-state and approval-request records instead of introducing a product/UI-owned approval queue.
• Routed WebUI gate resolution and ProductWorkflow approval-resolution payloads through the approval interaction service.
• Added contract coverage for approve, deny, missing/stale gates, cross-scope rejection, redaction/no-exposure sentinels, and audited resolver port wiring.

Change Type

• Bug fix
• New feature
• Refactor
• Documentation
• CI/Infrastructure
• Security
• Dependencies

Linked Issue

Closes #3889

Validation

• cargo fmt --all -- --check
• cargo clippy --all --benches --tests --examples --all-features -- -D warnings
• cargo build
• Relevant tests pass: cargo test -p ironclaw_product_workflow, cargo test -p ironclaw_product_workflow --test approval_interaction_contract, cargo test -p ironclaw_architecture reborn_crate_dependency_boundaries_hold
• cargo test --features integration if database-backed or integration behavior changed
• Manual testing: Not applicable; service/facade behavior covered by contract tests.
• If a coding agent was used and supports it, review-pr or pr-shepherd --fix was run before requesting review

Additional validation:

• cargo clippy -p ironclaw_product_workflow --all-targets -- -D warnings
• git diff --check && git diff --cached --check

Security Impact

Yes. This adds the product/WebUI approval interaction boundary for click approvals. The service validates caller scope before resolving approval gates, returns redacted DTOs only, rejects missing/stale/cross-scope gates deterministically, rejects AlwaysAllow, routes approve/deny through canonical approval resolver and turn coordinator ports, and preserves audit sink wiring for the concrete resolver port. No new network listeners, secrets access, direct tool execution, or sandbox policy changes.

Database Impact

None. This reads and mutates through existing run-state, approval-request, capability-lease, idempotency, and turn coordinator contracts. No migrations or schema changes.

Blast Radius

Limited to ironclaw_product_workflow approval interaction contracts plus WebUI/ProductWorkflow routing for approval gate resolution. Existing non-approval gate handling remains on the prior path.

Rollback Plan

Revert this PR to remove the approval interaction service, WebUI/ProductWorkflow approval routing, and associated tests/docs. Reborn approval gates would return to the prior unresolved/blocked behavior until a replacement service lands.

Review Follow-Through

Reviewer judgment requested on the click-approval DTO shape, redaction boundary, and whether production composition should inject ApprovalResolverPort directly or wrap ApprovalResolutionPort from host runtime services.

---

Review track: C

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[serrrfirat]

Thermo-nuclear code-quality pass: the behavior is heading in the right direction, but the implementation adds a structurally risky approval interaction boundary. The main issue is that approval record mutation and turn continuation are split into separate operations without an obvious recovery model, and the caller-facing service shape forces list-then-resolve orchestration into product workflow. I think this needs a cleaner decomposition before merging.

[serrrfirat]

Multi-agent code review complete for f6d884b45218317d9f4e440dc89ff5f73f228a90.

Reviewers: security, bugs, performance/concurrency, tests, conventions. Failed reviewers: none.

Findings after confidence filtering and dedupe: 7 total. Severity: 1 High, 6 Medium.

The blocking issue is the non-atomic approval decision plus turn transition. Several additional medium findings cover leakage of invocation fingerprint material, duplicated read-model scans, optional service wiring that violates local architecture rules, and missing contract coverage for new approval branches.

[serrrfirat]

Addressed the review findings in 5f6cf9c.\n\nSummary:\n- Made approval resolution recover across resolver-success/coordinator-failure retries by recognizing already-approved/already-denied gates and resuming/cancelling idempotently.\n- Removed the product workflow list-then-resolve scan; product approvals now call the approval interaction service directly by gate ref.\n- Redacted invocation fingerprints from pending approval DTOs.\n- Replaced the optional product approval service with a rejecting default.\n- Added coverage for spawn approval routing, unsupported approval actions, persistent approval rejection from product/WebUI paths, DTO redaction, and resolved-gate retry recovery.\n- Split the approval interaction monolith into focused modules.\n\nVerification:\n- cargo test -p ironclaw_product_workflow\n- cargo clippy -p ironclaw_product_workflow --all-targets -- -D warnings

[serrrfirat]

Thermo loop follow-up pushed in 3f1c936.\n\nLoop findings fixed:\n- Renamed the status-bearing gate record from pending-only naming to ApprovalGateRecord.\n- Clarified ResolveApprovalInteractionRequest.run_id as run_id_hint for callers that already know the run.\n- Collapsed duplicated persistent-approval rejection in the WebUI facade.\n- Tightened approved-gate recovery so it ensures the capability lease exists before resume, retries missing leases, and avoids duplicating an existing active/claimed lease.\n- Preserved stale-gate ordering so denied/expired gates do not ask for lease terms before returning StaleGate.\n\nValidation:\n- cargo test -p ironclaw_product_workflow\n- cargo clippy -p ironclaw_product_workflow --all-targets -- -D warnings\n- git diff --check

[serrrfirat]

Review completed with 5 reviewer agents plus intent analysis.

Result: REQUEST_CHANGES

Findings: 11 total

• High: 1
• Medium: 10

The blocking issue is the run-state/turn-run id mismatch in the approval read model. The remaining findings are security/audit, performance, architecture-contract, and missing caller-facing coverage risks.

[serrrfirat]

Addressed the active approval interaction findings in 81d23465.

Summary:

• Stopped deriving turn run ids from capability invocation ids. Approval reads now use approval records plus parked turn state, and resolve uses a targeted gate lookup with the caller run hint when present.
• Removed raw approval reasons from product/WebUI DTO summaries; pending approval summaries now use the fixed redacted fallback.
• Wired WebUI approval resolution with an audit sink and added a runtime contract test that observes a redacted approval audit event.
• Made the WebUI approval interaction facade dependency required with a rejecting default, removing the optional production branch.
• Updated the approval contract docs for one-shot spawn approval semantics.
• Added missing coverage for no-hint resolve, pending-list filtering/order, product deny routing, WebUI approval deny routing, spawn lease retry, and the run-id correlation regression.

Validation:

• cargo test -p ironclaw_product_workflow
• cargo test -p ironclaw_reborn_composition
• cargo clippy -p ironclaw_product_workflow --all-targets -- -D warnings
• cargo clippy -p ironclaw_reborn_composition --all-targets -- -D warnings
• cargo test -p ironclaw_architecture reborn_crate_dependency_boundaries_hold
• git diff --check

[serrrfirat]

Follow-up forced $code-review --force pass is fixed and pushed in 9156a452.

What changed:

• Avoided cloning the full turn-state persistence snapshot for approval lookups; the locator now queries scoped blocked approval runs directly and can recover a gate's original run id from approval checkpoints for replay.
• Changed approval listing to load only approvals referenced by parked approval gates instead of materializing all historical approval records for the thread.
• Fixed approval replay/idempotency edge cases:
  • nested turn idempotency keys are derived from stable inbound fingerprint fields, excluding mutable actor display names
  • already-approved/denied gate replays can reach the turn coordinator's idempotency cache when the run is no longer parked
  • claimed approval leases no longer satisfy recovery checks; active leases only
• Added regression coverage for malformed approval refs, unwired fallback, spawn approval lease effects, spawn lease dedupe, claimed-lease reissue, and replay paths after a run leaves the approval gate.
• Updated approval docs/contracts to match approve-first/lease-retry semantics and status-bearing approval gate read model behavior.

Validation run:

• cargo test -p ironclaw_turns
• cargo test -p ironclaw_product_workflow
• cargo test -p ironclaw_reborn_composition
• cargo clippy -p ironclaw_turns --all-targets -- -D warnings
• cargo clippy -p ironclaw_product_workflow --all-targets -- -D warnings
• cargo clippy -p ironclaw_reborn_composition --all-targets -- -D warnings
• git diff --check

Final targeted reviewer retries for Bugs, Performance/Concurrency, Tests, and Conventions are clean for Medium-or-higher findings.

[zmanian]

Review — approval interaction service (#4029)

Reviewed at head 9156a45. This builds on @serrrfirat's three prior review rounds (commits f6d884b4, 430785a3, 9156a45). I re-verified each earlier finding against the current head rather than re-flagging; the headline is that the security-relevant ones are resolved. Notes below lead with what I independently confirmed, then residual items.

Security — isolation & redaction (verified sound at head)

• Cross-scope/cross-tenant isolation is enforced server-side, not from caller claims. The chain is: (1) the read model resolves the approval record only within the caller's owner scope and drops it via same_interaction_owner (read_model.rs:98,128); (2) the turn-run locator queries are actor+scope-bound at the store (runtime.rs:218-220 → blocked_approval_runs_for_actor / approval_run_for_actor_and_gate); (3) turn_gate_state reloads the run via get_run_state and rejects on state.actor != request.actor before the gate-ref check, returning CrossScopeDenied (service.rs:129-133). A forged run_id_hint pointing at another owner's run fails the actor check; one pointing at an unrelated run of the caller's own fails the gate_ref match → NotParkedOnGate. The decision is rebound to authoritative scope on every path including the replay branches. Covered by cross_scope_actor_is_rejected_before_resolution. No isolation gap found.
• Redaction holds. PendingApprovalInteractionView no longer carries InvocationFingerprint (prior Medium) — the fingerprint stays server-side, used only inside ApprovalResolverPort::matching_lease_exists (resolver.rs:124). display_safe_summary() returns a fixed "Approval required" constant and no longer derives from ApprovalRequest.reason (prior Medium). The DTO exposes only capability_id + scope + gate/run ids. Locked by list_pending_does_not_expose_invocation_fingerprint and list_pending_never_derives_summary_from_raw_approval_reason. (Minor: the fixed summary is now information-free for the UI — acceptable as a fail-safe, but a typed display-safe summary would be more useful later.)
• Gate-resolution authority is bound unforgeably. The approve/deny decision is keyed to (authoritative scope, actor, gate_ref, request_id) re-derived from the gate record, not the wire payload; gate_ref must round-trip to approval_gate_ref(request.id) (types.rs:180-185).

Correctness — idempotency / double-resolve (prior High, now mitigated)

The original blocking finding (mutate-approval-then-resume is non-atomic; a failed resume hides the gate on retry) is mitigated at head by explicit recovery branches: NotParkedOnGate + Approved/Denied routes to replay_approved_gate / replay_denied_gate, and the already-approved-but-no-lease case routes to ensure_dispatch_lease / ensure_spawn_lease (service.rs:168-177,241-280,316-339). This is still mutate-then-resume rather than one atomic op, but a retry is now idempotent and recoverable, which was the reviewer's accepted alternative. Covered by already_approved_gate_retries_lease_issue_then_resumes, already_approved_replay_reaches_turn_coordinator_when_run_is_not_parked, and the denied analogues. One residual edge: the replay path depends on get_run_state still returning the run; if the run is GC'd between the failed first attempt and the retry, the retry returns MissingGate. Acceptable, but worth a one-line comment noting replay recovery is bounded by run-state retention.

Conventions (prior findings resolved)

• Optional-Arc / architecture.md #2 — both runtime structs now hold a required Arc<dyn ApprovalInteractionService> defaulting to RejectingApprovalInteractionService (reborn_services.rs:145,159; workflow.rs:52,68). This is exactly the prescribed pattern (required field + rejecting default + test-only with_*); the dead-503-branch concern is gone.
• Module split — the single approval_interaction.rs is now a module tree (types/read_model/resolver/service/gate_ref/mod), addressing the "mini-subsystem in one file" finding.
• Audit sink — composition now wires ApprovalResolverPort::new(...).with_audit_sink(...) (runtime.rs:942-947) with a contract test observing the emitted audit record (webui_approval_audit_sink().records()). No silent-authority-change path remains.
• No .unwrap()/.expect() in the production module; errors map through ProductWorkflowError with sanitized reasons. RejectingApprovalInteractionService fails closed. Good.

Performance (Low, residual)

resolve → find_gate now does a targeted approval_gate(scope, run_id_hint, gate_ref) lookup rather than the full-scan approval_gates, so the prior O(all approvals + all runs)-per-click finding is largely addressed for the resolve path. list_pending still scans (correct for a list op). With run_id_hint = None (ProductWorkflow path) the locator still does an actor-scoped blocked-run scan to recover the run id — bounded by parked runs per actor, fine for now; revisit if parked-gate counts grow.

Test coverage — strong

The previously-flagged gaps are all closed: spawn routing, unsupported-action, no-run-hint, multi-gate filter+stable-sort, fingerprint/reason non-exposure, audit observation, caller-level deny routing (approval_resolution_deny_routes_through_approval_interaction_service), always-allow rejection at both the ProductWorkflow and WebUI boundaries (approval_resolution_always_allow_is_rejected_without_approval_interaction, approval_gate_resolution_with_persistent_flag_is_rejected_without_approval_interaction), WebUI deny→Cancelled (approval_gate_denial_uses_approval_interaction_service_and_returns_cancelled), and the run-state-uses-parked-turn-run-id regression (run_state_read_model_uses_parked_turn_run_id_for_pending_approvals). Tests drive the real caller boundaries (RebornServices / ProductWorkflow dispatch), not just helpers — satisfies "test through the caller." Note: PR checklist leaves clippy --all-features and cargo build unchecked; please confirm the full clippy gate is green before merge.

Attested-stack interaction (flag)

This PR shares its WebUI gate/resolve ingress surface with the open attested-signing stack, specifically #3995 ("reborn webui attested gate/resolve ingress"). Both modify the same files: ironclaw_product_workflow/src/reborn_services.rs, .../lib.rs, ironclaw_reborn_composition/src/runtime.rs, and .../webui.rs. #4029 routes an approval gate family by gate:approval- prefix inside RebornServices::resolve_gate; #3995 adds an attested gate/resolve ingress through the same dispatch surface and RebornServices runtime struct (plus #4015 adds the attested gate-model and also edits composition/runtime.rs). They sit on different base branches, so whichever lands second will hit merge conflicts in resolve_gate's gate-family branching and in the RebornServices/runtime composition fields. More importantly, two independently-added gate-family dispatch branches on one resolve_gate is the "duplicate dispatch pipeline" shape (architecture.md #4) — the integrator should reconcile approval-prefix routing and the attested gate model into one coherent gate-family resolver rather than stacking two prefix checks. Recommend coordinating the merge order with the attested stack owner and adding a single gate-family routing test that exercises both families.

Recommendation

The implementation has converged well across three review rounds — the High and security/redaction Mediums are resolved with targeted tests. I'd treat the remaining items as non-blocking: (1) confirm the full clippy --all-features gate is green, (2) coordinate the resolve_gate/composition merge with attested-stack #3995/#4015 and unify the gate-family dispatch, (3) optionally note the run-state-retention bound on replay recovery. No new blocking security or correctness issues found at this head.

Reviewed on behalf of @zmanian.

[zmanian]

Merge-order coordination (attested-signing stack overlap)

Heads up @hanakannzashi: this PR and the attested-signing stack (#3995 webui gate/resolve ingress, #4015) both add gate-family dispatch to the same RebornServices::resolve_gate surface and touch reborn_services.rs / composition/runtime.rs / composition/webui.rs.

Agreed order: #4029 lands first, and the attested stack rebases onto your merged changes (it's a 14-PR chain — far cheaper for it to absorb 2 PRs than the reverse). So no rebasing burden on you from this.

Two asks to keep that rebase clean and avoid the duplicate-dispatch-pipeline shape (architecture.md #4):

1. Keep the approval-prefix routing in resolve_gate as a single, extensible gate-family branch (so the attested gate family slots in alongside it, not as a parallel pipeline).
2. Please drop a comment here when this merges, so the attested stack can rebase promptly.

(Review feedback is separate — the CHANGES_REQUESTED items still apply before merge. This is purely about sequencing.)

Coordinating on behalf of @zmanian (attested-signing stack owner).

[serrrfirat]

Re-reviewed current head after the resolved review threads and the green full CI pass. Previous blocking findings are addressed; approving for merge.