Remove ProductWorkflow from channel ingress - #6558
Conversation
…surface-review-fixes
github-actions
Bot
added
size: XL
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
🔎 IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted reviewer state before this projection. |
📝 WalkthroughWalkthroughChangesThe PR propagates Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 86.24% — 310399 / 359935 lines Per-crate breakdown (62 crates, lowest-covered first)
This table itself is informational and never gates the PR on its own — not the percentage, not the per-crate holes, not the 0-coverage callout. A separate coverage ratchet (dry-run until enforce=true; see tests/integration/coverage-floor.toml) can fail the build on specific configured floors. Exemptions (3 entry/entries excluded from the accounting above)
|
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 3 | 0 | 3 | c9cb601b92a9 |
Head: c9cb601b92a9b19ab74f6cc152c61acf158f327b
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Changes requested: lifecycle retry handling is not idempotent, setup retries can re-run side effects, and the sidebar loses access to paginated threads.
Findings
Blocking: 3 / Notes: 0
Blocking findings
1. ❌ [MEDIUM] Lifecycle retries fail instead of replaying
Location: crates/ironclaw_webui/src/webui_v2/handlers.rs:2277
Reusing a client action ID recreates the same ActivityId, which RuntimeProductCapabilityInvoker converts directly to the host InvocationId. The host run-state rejects a second invocation ID with InvocationAlreadyExists (the runtime's own tests explicitly require a fresh context for a second invocation), so a response-lost retry of install/activate/remove becomes a backend failure rather than an idempotent replay. Add durable replay/lookup behavior for completed lifecycle invocations, and cover a real runtime duplicate POST rather than only the stubbed handler test.
2. ❌ [MEDIUM] Setup action IDs do not deduplicate setup side effects
Location: crates/ironclaw_webui/src/webui_v2/handlers.rs:2582
EXTENSION_SETUP_SUBMIT_CAPABILITY is handled by ProductOperationHandler directly, so this activity ID never reaches the runtime invocation/run-state path. The request's client_action_id is removed from input and a duplicate POST executes setup again. In particular, duplicate channel-secret setup calls ChannelConfigService::save, which treats every nonblank secret as stored and reactivates an active channel again. Persist and replay setup actions by client action ID (or route setup through an idempotent capability path), with a duplicate-POST test proving only one save/reactivation.
3. ❌ [MEDIUM] Removing pagination hides conversations after the first page
Location: crates/ironclaw_webui/frontend/src/pages/chat/hooks/useThreads.ts:20
This fetches only the default thread page, while the server default is 50 and returns next_cursor for additional pages. This change removes the hook's page appending and all sidebar load-more/search messaging, leaving the cursor unused. Users with more than 50 threads cannot select or search their older conversations from the sidebar. Restore cursor paging (and its UI/test coverage) or otherwise fetch and expose all accessible threads.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas.
Inline review fallback
Inline comment projection fell back to a body-only PR Review because GitHub rejected the inline payload.
Reason: Unprocessable Entity: "Path could not be resolved" - https://docs.github.com/rest/pulls/reviews#create-a-review-for-a-pull-request
IronLoop preserved the inline review comment payloads below instead of dropping them.
Inline fallback 1: crates/ironclaw_webui/src/webui_v2/handlers.rs:2277
Reusing this derived activity ID is not a retry today: RuntimeProductCapabilityInvoker turns it into the host invocation ID, and the run-state rejects a second invocation with InvocationAlreadyExists. A response-lost install/activate/remove retry therefore fails instead of replaying. Please add durable replay/lookup semantics and exercise a duplicate POST against the real runtime.
Inline fallback 2: crates/ironclaw_webui/src/webui_v2/handlers.rs:2582
Setup is a direct ProductOperationHandler operation, so this activity ID never provides deduplication. The same client action can rerun credential/config writes; duplicate channel-secret setup also triggers another active-channel reactivation. Please persist/replay setup actions by client action ID (or route them through an idempotent path).
Inline fallback 3: crates/ironclaw_webui/frontend/src/pages/chat/hooks/useThreads.ts:20
The server defaults thread lists to 50 and returns next_cursor, but this change removes the page appender and all sidebar load-more wiring. Older threads become unselectable and unsearchable in the sidebar. Please retain cursor pagination and coverage.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_webui/src/webui_v2/handlers.rs (1)
2729-2747: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRestore deterministic activity IDs for generic capability invocations.
invoke_product_capability()now passesActivityId::new()— a fresh v4 ID per call — toinvoke_product_capability_with_activity_id(). There are still 21 bare generic call sites, so retrying the same capability request (for example,set_settings_tools_auto_approve()) cannot reuse the same activity ID and can break the stable activity IDs needed for retry handling.🤖 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_webui/src/webui_v2/handlers.rs` around lines 2729 - 2747, Update invoke_product_capability to derive and pass a deterministic ActivityId for the serialized capability input instead of ActivityId::new(). Preserve the existing invoke_product_capability_with_activity_id flow and ensure identical generic capability requests produce the same activity ID across retries.
🤖 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_webui/frontend/src/pages/chat/components/auth-oauth-card.tsx`:
- Around line 43-52: The PROVIDER_DISPLAY_NAMES allowlist uses the wrong key for
the GitHub provider. Update the entry in providerDisplayName’s allowlist to use
the canonical “git_hub” provider ID while preserving the intended “GitHub”
display label.
In `@tests/e2e/scenarios/test_reborn_qa_trace_full_path.py`:
- Around line 915-937: Remove the single-use helpers
_emulate_google_supports_sheets and _emulate_slack_supports_extension_reads, and
inline their HTTP probe logic at their respective call sites in the retry/fetch
flow. Preserve the existing headers, URLs, timeout, and status-code checks while
eliminating the unnecessary indirection.
- Around line 638-650: Update the document rewrite logic around
created_document_content so content is derived per Google Docs document/upload
pair rather than once from the first google-docs__insert_text call in the trace.
Associate each google-docs__create_document and corresponding
google-drive__upload_file with its own inserted text, preserving distinct
content when multiple documents are created.
---
Outside diff comments:
In `@crates/ironclaw_webui/src/webui_v2/handlers.rs`:
- Around line 2729-2747: Update invoke_product_capability to derive and pass a
deterministic ActivityId for the serialized capability input instead of
ActivityId::new(). Preserve the existing
invoke_product_capability_with_activity_id flow and ensure identical generic
capability requests produce the same activity ID across retries.
🪄 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: 29b0b3d0-5680-47b5-a64f-8e483eb7ca5c
📒 Files selected for processing (39)
crates/ironclaw_product_workflow/src/commands.rscrates/ironclaw_product_workflow/src/lib.rscrates/ironclaw_product_workflow/src/reborn_services/extension_setup_credentials.rscrates/ironclaw_product_workflow/src/webui_inbound.rscrates/ironclaw_product_workflow/tests/product_commands_contract.rscrates/ironclaw_product_workflow/tests/reborn_services_contract.rscrates/ironclaw_reborn_composition/src/extension_host/extension_lifecycle.rscrates/ironclaw_reborn_composition/src/extension_host/extension_lifecycle_capabilities.rscrates/ironclaw_reborn_composition/src/factory.rscrates/ironclaw_reborn_composition/src/outbound/mod.rscrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_reborn_composition/src/runtime/tests/core.rscrates/ironclaw_reborn_composition/src/runtime/tests/outbound_delivery.rscrates/ironclaw_reborn_composition/src/webui/facade/tests.rscrates/ironclaw_reborn_composition/src/webui/product_capability.rscrates/ironclaw_reborn_composition/tests/webui_v2_e2e.rscrates/ironclaw_reborn_composition/tests/webui_v2_serve.rscrates/ironclaw_webui/frontend/src/lib/api.test.tscrates/ironclaw_webui/frontend/src/lib/api.tscrates/ironclaw_webui/frontend/src/pages/chat/components/auth-oauth-card.tsxcrates/ironclaw_webui/frontend/src/pages/chat/lib/attachments.tscrates/ironclaw_webui/frontend/src/pages/extensions/hooks/useExtensions-catalog.test.tscrates/ironclaw_webui/frontend/src/pages/extensions/hooks/useExtensions.tscrates/ironclaw_webui/frontend/src/pages/extensions/lib/extensions-api.test.tscrates/ironclaw_webui/frontend/src/pages/extensions/lib/extensions-api.tscrates/ironclaw_webui/src/webui_v2/handlers.rscrates/ironclaw_webui/tests/webui_v2_handlers_contract.rstests/e2e/conftest.pytests/e2e/scenarios/test_emulate_reborn_provider_contracts.pytests/e2e/scenarios/test_reborn_qa_trace_full_path.pytests/e2e/scenarios/test_reborn_slack_channel_e2e.pytests/e2e/scenarios/test_reborn_webui_v2_automation_trace_outbound_api.pytests/e2e/scenarios/test_reborn_webui_v2_extensions_api.pytests/e2e/scenarios/test_reborn_webui_v2_legacy_approval.pytests/e2e/scenarios/test_reborn_webui_v2_legacy_attachments.pytests/e2e/scenarios/test_reborn_webui_v2_legacy_auth_flows.pytests/e2e/scenarios/test_reborn_webui_v2_legacy_extensions.pytests/e2e/scenarios/test_reborn_webui_v2_legacy_settings_search.pytests/integration/webui_v2_product_api.rs
💤 Files with no reviewable changes (1)
- tests/e2e/scenarios/test_reborn_webui_v2_automation_trace_outbound_api.py
|
🚅 Deployed to the ironclaw-pr-6558 environment in ironclaw-ci-preview
|
…surface-review-fixes-refresh-2 # Conflicts: # crates/ironclaw_reborn_composition/src/factory.rs # crates/ironclaw_reborn_composition/src/runtime.rs # crates/ironclaw_reborn_composition/src/webui/product_capability.rs
|
@ironloopai review --agent reviewer |
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 2 | 0 | 2 | d04cba851ab4 |
Head: d04cba851ab4ae7bc66b914a988bbb330afe9485
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Two retry/idempotency regressions remain in ProductSurface mutations.
Findings
Blocking: 2 / Notes: 0
Blocking findings
1. ❌ [HIGH] Value-derived IDs turn repeat mutations into permanent replays
Location: crates/ironclaw_webui/src/webui_v2/handlers.rs:2866
The new durable result lookup replays every previously successful ActivityId, while this helper derives that ID solely from the requested target. A user who sets preference A, then B, then A will have the third request replay the old A result without invoking the capability; the following query still returns B. generic_product_capability_activity_id has the same issue for skill and other generic mutations. Use a per-action retry key (or bounded/expiring replay semantics), and add an A→B→A regression test.
2. ❌ [HIGH] Extension setup retries bypass the durable replay path
Location: crates/ironclaw_webui/src/webui_v2/handlers.rs:2583-2588
This passes the stable activity ID to ProductSurface, but builtin.extension_setup_submit is handled by ProductOperationHandler before RuntimeProductCapabilityInvoker is called. That path ignores activity_id, so a response-lost retry repeats manual-token/channel-config side effects instead of replaying the first result. Add operation-level durable idempotency or route setup through the replaying invoker, with a real-runtime duplicate-submit test.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas.
| ))) | ||
| } | ||
|
|
||
| fn outbound_preferences_activity_id( |
There was a problem hiding this comment.
This activity ID is a permanent value-derived cache key now that successful ProductSurface calls replay /product-results. Setting A, then B, then A replays the first A result and skips the final mutation, leaving B active. The generic helper has the same issue; use a per-action retry key or bounded replay retention.
| serde_json::Value::String(package_ref.id.as_str().to_string()), | ||
| ); | ||
| let resolution = invoke_product_capability( | ||
| let resolution = invoke_product_capability_with_activity_id( |
There was a problem hiding this comment.
builtin.extension_setup_submit is dispatched by ProductOperationHandler, which bypasses RuntimeProductCapabilityInvoker and ignores activity_id. A retry with this client action ID therefore re-submits setup side effects rather than replaying the durable result.
d04cba8 to
bf82ea7
Compare
|
@ironloopai review --agent reviewer |
There was a problem hiding this comment.
Actionable comments posted: 3
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/webui/product_capability.rs (1)
100-152: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftHold the activity lock only while mutating in-memory state.
_activity_guardcrossesHostRuntime::invoke_capability(...)and filesystem reads before the latercast_updatewrite, which violatescrates/ironclaw_reborn_composition/**/*.rs: “Read-modify-write operations must use the shared bounded CAS helper rather than a process-local mutex held across backend I/O.” Build the capability request first, acquire the lock only around the persistence claim/replay path, and add a timeout to the whole idempotency attempt rather than a process-local unscopedAsyncMutexbehind slow capability dispatch.🤖 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/webui/product_capability.rs` around lines 100 - 152, Refactor the capability invocation flow in the surrounding method so `HostRuntime::invoke_capability` and filesystem/backend reads occur before acquiring the activity lock. Use the shared bounded CAS helper for the persistence claim/replay update, scope the lock only to the required in-memory mutation, and apply a timeout across the complete idempotency attempt; remove the current lock held across dispatch and `product_resolution`.Source: Path instructions
🤖 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_webui/frontend/src/lib/api.test.ts`:
- Around line 198-210: Extend the setupExtension test to cover the missing
clientActionId case: call setupExtension without clientActionId, then verify the
request body includes the generated client-action ID along with the action and
payload. Keep the existing caller-supplied ID coverage intact and assert the
generated ID’s expected format or value using the test’s available request data.
In `@crates/ironclaw_webui/src/webui_v2/handlers.rs`:
- Around line 2835-2840: Update extension_lifecycle_activity_id to accept a
reference to the parsed, strongly typed client action ID instead of &str, and
adjust its callers to pass that validated domain value without re-parsing or
accepting raw input.
- Around line 2820-2828: Update the activity identity generation around the
request presence flags so different api_key rotations produce distinct replay
identities without including raw key bytes, using an opaque action identity or
secret-store revision/fingerprint. Preserve the existing behavior for non-secret
fields, and add a handler-level test covering otherwise-identical updates with
different API keys.
---
Outside diff comments:
In `@crates/ironclaw_reborn_composition/src/webui/product_capability.rs`:
- Around line 100-152: Refactor the capability invocation flow in the
surrounding method so `HostRuntime::invoke_capability` and filesystem/backend
reads occur before acquiring the activity lock. Use the shared bounded CAS
helper for the persistence claim/replay update, scope the lock only to the
required in-memory mutation, and apply a timeout across the complete idempotency
attempt; remove the current lock held across dispatch and `product_resolution`.
🪄 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: 4e13badd-5bfa-4751-98ba-ff20bbe4a9ad
📒 Files selected for processing (19)
crates/ironclaw_host_api/src/path.rscrates/ironclaw_reborn_composition/src/extension_host/extension_lifecycle.rscrates/ironclaw_reborn_composition/src/extension_host/extension_lifecycle_capabilities.rscrates/ironclaw_reborn_composition/src/factory.rscrates/ironclaw_reborn_composition/src/local_dev_mounts.rscrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_reborn_composition/src/runtime/tests/core.rscrates/ironclaw_reborn_composition/src/runtime/tests/outbound_delivery.rscrates/ironclaw_reborn_composition/src/webui/facade/tests.rscrates/ironclaw_reborn_composition/src/webui/product_capability.rscrates/ironclaw_reborn_composition/tests/webui_v2_e2e.rscrates/ironclaw_webui/frontend/src/lib/api.test.tscrates/ironclaw_webui/frontend/src/lib/api.tscrates/ironclaw_webui/frontend/src/lib/sidebar-active-thread.test.tscrates/ironclaw_webui/frontend/src/pages/chat/components/auth-oauth-card.tsxcrates/ironclaw_webui/src/webui_v2/handlers.rscrates/ironclaw_webui/tests/webui_v2_handlers_contract.rstests/e2e/scenarios/test_reborn_qa_trace_full_path.pytests/integration/webui_v2_product_api.rs
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 3 | 0 | 3 | bf82ea70d543 |
Head: bf82ea70d543fb53e67b2c97bd3d7c92eb53cfab
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Found three blocking issues in retry handling and E2E verification.
Findings
Blocking: 3 / Notes: 0
Blocking findings
1. ❌ [HIGH] Setup submissions bypass durable replay
Location: crates/ironclaw_webui/src/webui_v2/handlers.rs:2586
EXTENSION_SETUP_SUBMIT_CAPABILITY is handled as a ProductOperationHandler, so RebornServices::invoke executes it before RuntimeProductCapabilityInvoker, the sole /product-results replay path. Retrying the same client action after a lost response reruns channel configuration and credential submission rather than replaying the completed result. Add durable idempotency to this operation or route it through the replaying path.
2. ❌ [HIGH] Queued retries can split the per-activity lock
Location: crates/ironclaw_reborn_composition/src/webui/product_capability.rs:86
Removing the map entry does not account for callers that already cloned this mutex and are waiting. After an unpersisted invocation, such as a runtime failure or result-persistence error, a waiting retry proceeds with the old lock while a new retry creates a fresh lock and invokes the same activity concurrently. This can duplicate external side effects. Retain entries until no holders or waiters remain, or use a durable per-activity lease.
3. ❌ [MEDIUM] Docs-to-Drive trace rewriting no longer verifies uploaded content
Location: tests/e2e/scenarios/test_reborn_qa_trace_full_path.py:673
After this rewrite, the baseline and outcome checks recognize only Docs and Sheets creation. They therefore return before checking a normalized google-drive__upload_file, allowing the changed journey to pass even if the uploaded document name or content was not persisted. Update the provider readback checks to recognize Drive uploads and assert their content.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas.
| let resolution = invoke_product_capability_with_activity_id( | ||
| state.services(), | ||
| caller.clone(), | ||
| EXTENSION_SETUP_SUBMIT_CAPABILITY, |
There was a problem hiding this comment.
EXTENSION_SETUP_SUBMIT_CAPABILITY is handled by ProductOperationHandler, so it never reaches RuntimeProductCapabilityInvoker or its /product-results replay store. Retrying this client action after a lost response re-executes save_values and credential submission. Please add durable idempotency here or route setup through the replaying path.
| .get(&activity_id) | ||
| .is_some_and(|current| Arc::ptr_eq(current, lock)) | ||
| { | ||
| locks.remove(&activity_id); |
There was a problem hiding this comment.
Removing the entry solely because it is the current Arc races with queued waiters. After an unpersisted error, a waiter can use this old mutex while a later retry creates a fresh mutex and invokes the same activity concurrently. Retain entries until no holders/waiters remain, or use a durable activity lease.
| ) | ||
| document_upload_index += 1 | ||
| call["arguments"] = { | ||
| "name": arguments["title"], |
There was a problem hiding this comment.
The baseline/outcome helpers only recognize Docs and Sheets creation, so after this rewrite they return without checking the Drive upload. Please add google-drive__upload_file readback and assert the uploaded name/content; otherwise this normalized journey can pass with missing or incorrect document content.
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)
tests/e2e/scenarios/test_reborn_qa_trace_full_path.py (1)
639-676: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve all text chunks for each rewritten document.
This assigns only the first
google-docs__insert_textto each positional placeholder. Multiple inserts for one document are either discarded or assigned to another document, so the Drive upload no longer represents the recorded Docs operation. Associate inserts by their document reference and append chunks in trace order before rewriting the matching create call.🤖 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 `@tests/e2e/scenarios/test_reborn_qa_trace_full_path.py` around lines 639 - 676, Update the document content reconstruction around document_upload_contents and the google-docs__create_document rewrite to associate google-docs__insert_text calls with their document reference rather than positional empty placeholders. Accumulate and append every text chunk in trace order for each document, then use the complete accumulated content when rewriting the matching create call as google-drive__upload_file.
🤖 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 `@tests/e2e/scenarios/test_reborn_qa_trace_full_path.py`:
- Around line 663-676: Update _assert_google_provider_outcome() to recognize
rewritten google-drive__upload_file calls alongside
google-docs__create_document, then use the upload result to download the created
file and assert its provider state and expected content. Ensure _load_trace()
normalization still leads to read-back validation rather than returning when
create_call is absent.
---
Outside diff comments:
In `@tests/e2e/scenarios/test_reborn_qa_trace_full_path.py`:
- Around line 639-676: Update the document content reconstruction around
document_upload_contents and the google-docs__create_document rewrite to
associate google-docs__insert_text calls with their document reference rather
than positional empty placeholders. Accumulate and append every text chunk in
trace order for each document, then use the complete accumulated content when
rewriting the matching create call as google-drive__upload_file.
🪄 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: 66aa041a-fdf2-46f6-9d54-a6fd301b5c77
📒 Files selected for processing (4)
crates/ironclaw_webui/frontend/src/pages/chat/components/auth-oauth-card.tsxcrates/ironclaw_webui/src/webui_v2/handlers.rscrates/ironclaw_webui/tests/webui_v2_handlers_contract.rstests/e2e/scenarios/test_reborn_qa_trace_full_path.py
|
@ironloopai review --agent reviewer |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/e2e/scenarios/test_reborn_qa_trace_full_path.py (1)
644-692: 🎯 Functional Correctness | 🟠 MajorRestore provider readback for rewritten Docs uploads.
This change emits
google-drive__upload_file, but_assert_google_provider_outcome()still searches only forgoogle-docs__create_documentandgoogle-sheets__create_spreadsheet(Line 969-982). The normalized Docs flow can therefore return without checking the uploaded file, name, or content.As per coding guidelines, provider tests must assert Emulate provider state/readback rather than only replay output.
🤖 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 `@tests/e2e/scenarios/test_reborn_qa_trace_full_path.py` around lines 644 - 692, Update _assert_google_provider_outcome() to recognize the normalized google-drive__upload_file call produced by the Docs replay flow, alongside the existing document and spreadsheet cases. Assert the Emulate provider readback for the uploaded file’s identifier, name, and content so rewritten Docs uploads are validated rather than skipped.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 `@tests/e2e/scenarios/test_emulate_reborn_provider_contracts.py`:
- Around line 29-47: Replace the mutating release POST in
_skip_if_github_release_writes_unavailable and the corresponding probe in
tests/e2e/scenarios/test_reborn_qa_trace_full_path.py:938-948 with a
guaranteed-invalid, non-mutating capability request. Accept only 422 as
supported, skip on 403 or 404, and fail all other statuses; ensure this shared
capability contract runs before provider-operation baselines in both sites.
---
Duplicate comments:
In `@tests/e2e/scenarios/test_reborn_qa_trace_full_path.py`:
- Around line 644-692: Update _assert_google_provider_outcome() to recognize the
normalized google-drive__upload_file call produced by the Docs replay flow,
alongside the existing document and spreadsheet cases. Assert the Emulate
provider readback for the uploaded file’s identifier, name, and content so
rewritten Docs uploads are validated rather than skipped.
🪄 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: 40682f00-d6ad-4e59-b852-63419603fdbe
📒 Files selected for processing (2)
tests/e2e/scenarios/test_emulate_reborn_provider_contracts.pytests/e2e/scenarios/test_reborn_qa_trace_full_path.py
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 1 | 0 | 1 | 8a289536d97d |
Head: 8a289536d97d0678b43753e3d169e0fa26385623
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
The setup retry path still re-executes side effects despite the new stable client action ID.
Findings
Blocking: 1 / Notes: 0
Blocking findings
1. ❌ [HIGH] Replay API-only extension setup by activity ID
Location: crates/ironclaw_webui/src/webui_v2/handlers.rs:2583-2589
This now supplies a stable activity ID, but builtin.extension_setup_submit is handled as an API-only ProductOperationHandler operation. That path invokes setup directly and returns a synthetic success resolution without consulting the durable /product-results replay store. A response-lost retry with the same client action ID therefore runs ChannelConfigFacade::save_values again (which can reactivate an active extension) and resubmits manual credentials. Put API-only setup behind durable activity-ID replay, or route it through the replaying runtime path, and add a real duplicate-request test that proves these side effects execute once.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas.
| caller.clone(), | ||
| EXTENSION_SETUP_SUBMIT_CAPABILITY, | ||
| input, | ||
| activity_id, |
There was a problem hiding this comment.
extension_setup_submit is API-only, so RebornServices::invoke dispatches it directly and returns a synthetic success without consulting the durable runtime replay store. The same client action ID therefore reruns setup after a lost response, including config saves (which can reactivate an active extension) and manual-token submission. Please add durable replay for this path and a duplicate real-runtime request test.
…surface-review-fixes-refresh-2
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 (2)
crates/ironclaw_webui/tests/webui_v2_handlers_contract.rs (1)
5366-5419: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAdd a retry/replay assertion for
setup_extension, matchinginstall_extension's.
install_extension_invokes_lifecycle_capability_with_body_package_ref(5056-5109) andset_outbound_preferences_dispatches_body_through_invoke(3248-3319) both assert that an identicalclient_action_idretry reuses the sameProductSurfaceactivity id.setup_extension_invokes_product_surface_capabilityonly sends one request, so it doesn't prove the previously-flaggedEXTENSION_SETUP_SUBMIT_CAPABILITYreplay-bypass is actually fixed by the newinvoke_product_operationdispatch path. Please add a second request +invoke_calls[0].2 == invoke_calls[1].2assertion here.🤖 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_webui/tests/webui_v2_handlers_contract.rs` around lines 5366 - 5419, Extend setup_extension_invokes_product_surface_capability with a second identical POST using the same client_action_id, then assert both requests succeed and invoke_calls contains two entries whose activity IDs (the third tuple element) are equal. Preserve the existing payload, capability, response, and view-query assertions while adding explicit retry/replay coverage for setup_extension.crates/ironclaw_webui/src/webui_v2/handlers.rs (1)
2566-2616: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAdd an HTTP-level retry regression test for
setup_extension.
setup_extension_invokes_product_surface_capabilitysends only one request and checks that the capability call shape is correct, unlikeinstall_extension_invokes_lifecycle_capability_with_body_package_ref. Per CLAUDE.md’s bug-fix regression-test invariant, add a two-request setup retry test that asserts the same activity/action/request shape survives the retry so saved setup work is replayed, not re-run.🤖 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_webui/src/webui_v2/handlers.rs` around lines 2566 - 2616, Add an HTTP-level two-request retry regression test for setup_extension, modeled on install_extension_invokes_lifecycle_capability_with_body_package_ref. Send the same setup request twice and assert both capability invocations preserve the identical activity ID, action/capability, package reference, and request body shape, confirming the saved setup work is replayed rather than re-executed.
🤖 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/webui/product_capability.rs`:
- Around line 154-186: Update invoke_product_operation to use the
caller-supplied summary when constructing the successful resolution, rather than
ignoring it. Thread summary into product_operation_resolution and replace its
hardcoded “capability completed” text with that value, ensuring the resulting
resolution persisted for replay retains each ProductOperation's success_summary.
---
Outside diff comments:
In `@crates/ironclaw_webui/src/webui_v2/handlers.rs`:
- Around line 2566-2616: Add an HTTP-level two-request retry regression test for
setup_extension, modeled on
install_extension_invokes_lifecycle_capability_with_body_package_ref. Send the
same setup request twice and assert both capability invocations preserve the
identical activity ID, action/capability, package reference, and request body
shape, confirming the saved setup work is replayed rather than re-executed.
In `@crates/ironclaw_webui/tests/webui_v2_handlers_contract.rs`:
- Around line 5366-5419: Extend
setup_extension_invokes_product_surface_capability with a second identical POST
using the same client_action_id, then assert both requests succeed and
invoke_calls contains two entries whose activity IDs (the third tuple element)
are equal. Preserve the existing payload, capability, response, and view-query
assertions while adding explicit retry/replay coverage for setup_extension.
🪄 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: 5f9873dc-855f-43e7-901b-2eb06896c8f7
📒 Files selected for processing (15)
crates/ironclaw_product_workflow/src/lib.rscrates/ironclaw_product_workflow/src/reborn_services.rscrates/ironclaw_product_workflow/src/reborn_services/llm_config.rscrates/ironclaw_product_workflow/tests/reborn_services_contract.rscrates/ironclaw_reborn_composition/src/llm_admin/llm_config_service.rscrates/ironclaw_reborn_composition/src/webui/product_capability.rscrates/ironclaw_webui/frontend/src/lib/api.test.tscrates/ironclaw_webui/frontend/src/lib/api.tscrates/ironclaw_webui/frontend/src/pages/settings/lib/settings-api.test.tscrates/ironclaw_webui/frontend/src/pages/settings/lib/settings-api.tscrates/ironclaw_webui/src/webui_v2/handlers.rscrates/ironclaw_webui/tests/webui_v2_handlers_contract.rstests/e2e/conftest.pytests/e2e/scenarios/test_emulate_reborn_provider_contracts.pytests/e2e/scenarios/test_reborn_qa_trace_full_path.py
…surface-review-fixes-refresh-2
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 (3)
crates/ironclaw_webui/tests/webui_v2_handlers_contract.rs (1)
5063-5111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRetry assertions don't check the retry actually resent the same capability/input.
Only
invoke_calls[0].0/.1are checked against the expected capability id and input;invoke_calls[1].0/.1are never asserted, so a regression that changes what the retry POST sends toinvoke()(wrong capability, or a mutated input body) would pass this test silently. Thesetup_extension_invokes_product_surface_capabilitytest right below (lines 5430-5433) checks both calls for capability and input — this test should mirror that.♻️ Proposed fix
assert_eq!( invoke_calls[0].0, CapabilityId::new(EXTENSION_INSTALL_CAPABILITY_ID).expect("capability id") ); assert_eq!( invoke_calls[0].1, serde_json::json!({ "extension_id": "nearai-mcp" }) ); + assert_eq!(invoke_calls[1].0, invoke_calls[0].0); + assert_eq!(invoke_calls[1].1, invoke_calls[0].1); assert_eq!( invoke_calls[0].2, invoke_calls[1].2, "the client action id must survive response-lost retries as the ProductSurface activity id" );🤖 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_webui/tests/webui_v2_handlers_contract.rs` around lines 5063 - 5111, Extend the retry assertions in the test around the two entries in services.invoke_calls so invoke_calls[1].0 matches EXTENSION_INSTALL_CAPABILITY_ID and invoke_calls[1].1 matches the expected {"extension_id": "nearai-mcp"} input, mirroring the capability and input checks used by setup_extension_invokes_product_surface_capability.crates/ironclaw_reborn_composition/src/webui/product_capability.rs (2)
445-453: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSummary-persist failure surfaces a genuinely-successful, already-persisted operation as a hard error.
results.persist(...)(the durable success marker) succeeds, thenresults.persist_summary(...)runs with?. If only the summary write fails,product_operation_resolutionreturnsErreven though the operation completed and its result is durably recorded via the firstpersistcall. The caller sees a failure and — per this PR's own client-action-id retry design — may mint a newclient_action_idbelieving the prior attempt failed, causing the (possibly non-idempotent) underlying operation to run again. This defeats the retry-idempotency guarantee this PR is adding.🐛 Proposed fix: don't let an auxiliary summary-write failure invalidate an already-durable result
let result_ref = ResultRef::from_uuid(invocation_id.as_uuid()); results.persist(scope, result_ref, Vec::new()).await?; - results.persist_summary(scope, result_ref, summary).await?; + // silent-ok: the durable success marker above already recorded this + // operation as done; replay falls back to a generic summary if this + // auxiliary write is missing, so failing here would wrongly report a + // succeeded, durably-persisted operation as failed to the caller. + if let Err(error) = results.persist_summary(scope, result_ref, summary).await { + tracing::warn!(%error, "product operation summary persist failed after result was persisted"); + }Based on coding guidelines,
crates/**/*.rs: "justified fallbacks must include an inline// silent-ok: <reason>comment naming the operation."🤖 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/webui/product_capability.rs` around lines 445 - 453, Update product_operation_resolution so persist_summary failures do not propagate as errors after the durable results.persist succeeds; treat the summary write as a best-effort auxiliary operation while preserving the successful Resolution return. Add the required inline // silent-ok: comment explaining that summary persistence must not invalidate an already-durable operation result.Source: Coding guidelines
55-56: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDon’t hold the per-activity async mutex across capability execution and result persistence
crates/**/*.rsrequires not holding process-local/per-record async mutexes across filesystem/backend I/O. In bothinvokeandinvoke_product_operation, the activity lock is acquired beforeresults.replay(...)/host_runtime.invoke_capability(...)/operation execution and persists throughproduct_resolution(...)orproduct_operation_resolution(...)persist calls. Scope the lock to the fast single-flight guard only, or move the locking down after the replay check so it is never held during capability/result I/O.🤖 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/webui/product_capability.rs` around lines 55 - 56, Update invoke and invoke_product_operation so activity_locks is not held across results.replay, host_runtime.invoke_capability, operation execution, or product resolution persistence. Perform replay checks before acquiring the lock, and scope each lock only around the fast in-memory single-flight guard; release it before all capability, backend, filesystem, and result I/O.Source: Path instructions
🤖 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/webui/product_capability.rs`:
- Around line 620-622: Update the summary handling in replay_product_result so
errors from replay_product_result_summary are treated like missing summaries:
fall back to fixed_summary("capability completed") instead of propagating them.
Preserve successful decoded summaries, and add an inline // silent-ok: comment
explaining that persisted successful results must remain replayable when summary
decoding or validation fails.
---
Outside diff comments:
In `@crates/ironclaw_reborn_composition/src/webui/product_capability.rs`:
- Around line 445-453: Update product_operation_resolution so persist_summary
failures do not propagate as errors after the durable results.persist succeeds;
treat the summary write as a best-effort auxiliary operation while preserving
the successful Resolution return. Add the required inline // silent-ok: comment
explaining that summary persistence must not invalidate an already-durable
operation result.
- Around line 55-56: Update invoke and invoke_product_operation so
activity_locks is not held across results.replay,
host_runtime.invoke_capability, operation execution, or product resolution
persistence. Perform replay checks before acquiring the lock, and scope each
lock only around the fast in-memory single-flight guard; release it before all
capability, backend, filesystem, and result I/O.
In `@crates/ironclaw_webui/tests/webui_v2_handlers_contract.rs`:
- Around line 5063-5111: Extend the retry assertions in the test around the two
entries in services.invoke_calls so invoke_calls[1].0 matches
EXTENSION_INSTALL_CAPABILITY_ID and invoke_calls[1].1 matches the expected
{"extension_id": "nearai-mcp"} input, mirroring the capability and input checks
used by setup_extension_invokes_product_surface_capability.
🪄 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: 94727d14-7b82-41ca-b82d-8fced7d8cf38
📒 Files selected for processing (2)
crates/ironclaw_reborn_composition/src/webui/product_capability.rscrates/ironclaw_webui/tests/webui_v2_handlers_contract.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/ironclaw_reborn_composition/src/webui/product_capability.rs (3)
345-353: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInclude extension search in lifecycle mount selection.
is_extension_lifecycle_capabilityrecognizes install/activate/remove but excludesEXTENSION_SEARCH_CAPABILITY_ID. The canonical search manifest declaresReadFilesystem, so product invocation gives it empty mounts and breaks lifecycle catalog search. Include search and extend the test through the product invocation caller, not onlyproduct_invocation_mounts.As per coding guidelines, “Test through the caller when a predicate, classifier, or transform gates a side effect”; path instructions require the same.
🤖 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/webui/product_capability.rs` around lines 345 - 353, Update is_extension_lifecycle_capability to also recognize EXTENSION_SEARCH_CAPABILITY_ID, ensuring extension search receives the lifecycle mounts required by its manifest. Add or extend coverage through the product invocation caller, verifying the search capability gets the expected filesystem mount rather than testing only product_invocation_mounts.Sources: Coding guidelines, Path instructions
177-191: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMake activity-lock cleanup cancellation-safe.
Cancellation during
activity_lock.lock().awaitoroperation.awaitdrops the mutex guard but bypassesrelease_activity_lock; the map retains that activity ID forever. Use an RAII cleanup mechanism and add an aborted-invocation regression test.🤖 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/webui/product_capability.rs` around lines 177 - 191, Make the activity-lock lifecycle in the invocation flow cancellation-safe by introducing an RAII cleanup guard immediately after acquiring the lock, ensuring its drop path calls release_activity_lock for every exit, including cancellation during activity_lock.lock().await or operation.await. Update the replay and normal-result paths around product_operation_resolution to avoid manual cleanup that can be bypassed, and add a regression test verifying an aborted invocation removes the activity ID from the lock map.
185-186: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not persist replay state only after the side effect.
operationcan succeed whileproduct_operation_resolutionfails to persist. Retrying the same client action then re-executes the completed mutation because no durable replay record exists. Make the activity ID part of a durable idempotency/evidence protocol spanning the operation, and fault-test persistence failure followed by retry.As per coding guidelines, “Side-effecting success requires durable or provider-issued evidence plus read-back verification.”
🤖 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/webui/product_capability.rs` around lines 185 - 186, Update the operation flow around operation.await and product_operation_resolution so activity ID durability and idempotency span the side effect, rather than persisting replay state only after resolution succeeds. Record or obtain durable/provider-issued evidence and verify it by read-back before treating the mutation as successful, allowing retries to reuse the completed result without re-executing it. Add a fault test covering persistence failure followed by retry.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.
Outside diff comments:
In `@crates/ironclaw_reborn_composition/src/webui/product_capability.rs`:
- Around line 345-353: Update is_extension_lifecycle_capability to also
recognize EXTENSION_SEARCH_CAPABILITY_ID, ensuring extension search receives the
lifecycle mounts required by its manifest. Add or extend coverage through the
product invocation caller, verifying the search capability gets the expected
filesystem mount rather than testing only product_invocation_mounts.
- Around line 177-191: Make the activity-lock lifecycle in the invocation flow
cancellation-safe by introducing an RAII cleanup guard immediately after
acquiring the lock, ensuring its drop path calls release_activity_lock for every
exit, including cancellation during activity_lock.lock().await or
operation.await. Update the replay and normal-result paths around
product_operation_resolution to avoid manual cleanup that can be bypassed, and
add a regression test verifying an aborted invocation removes the activity ID
from the lock map.
- Around line 185-186: Update the operation flow around operation.await and
product_operation_resolution so activity ID durability and idempotency span the
side effect, rather than persisting replay state only after resolution succeeds.
Record or obtain durable/provider-issued evidence and verify it by read-back
before treating the mutation as successful, allowing retries to reuse the
completed result without re-executing it. Add a fault test covering persistence
failure followed by retry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0c1de0d3-f962-4c21-b5d5-a0ce965e0d44
📒 Files selected for processing (1)
crates/ironclaw_reborn_composition/src/webui/product_capability.rs
…6520 Conflict resolution keeps main's structure and this PR's lifecycle model: the no-Activate contract wins everywhere activate reappeared (handlers, frontend api/hooks/tests, e2e scenarios, facade/product-capability tests, product command parsing), while main's client-action-id mutation contract replaces this branch's idempotency_key wire end-to-end (install handler derives its activity id from the validated client action id and still enters through install_extension_on_surface). The removal path keeps main's snapshot-fallback unpublish for hosted-MCP republished packages without resurrecting activation-state writes; the gesture- idempotency contract test now pins both distinct-gesture divergence and response-lost-retry stability of the client action id. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WF1EiatUVLjKKs3eYGMbKV
This branch was successfully deployed
1 participant
Summary
origin/main(62a4907338a5f0d5a0cbad4001b3858e15077c38) into this PR branch.ProductSurfaceand moving API-only product operations through durable ProductSurface replay by activity/invocation ID.Change Type
Linked Issue
None.
Validation
cargo fmt --all -- --checkcargo clippy --all --benches --tests --examples --all-features -- -D warningscargo buildcargo test --features integrationif database-backed or integration behavior changedorigin/main.review-prorpr-shepherd --fixwas run before requesting review: Not applicable; review comments were inspected and addressed manually.Test Strategy
User behavior:
Product ingress uses
ProductSurfaceas the product-facing boundary. Browser retries for extension setup/lifecycle, provider upsert, and outbound preference updates replay by opaque client action ID instead of request values or secrets. Generic product mutations without client action IDs avoid permanent value-derived replay caching.Risk areas:
Tests added or updated:
ironclaw_product_workflow,ironclaw_reborn_composition, andironclaw_webuipassed.What the tests prove:
API-only ProductSurface operations can persist/replay operation-specific success summaries, unreadable optional summary sidecars do not fail replay of already-persisted product results, activity IDs are scoped to validated client action IDs where retry semantics are needed, generic mutations do not permanently cache repeated value payloads, frontend callers serialize generated/explicit client action IDs, Google Drive upload rewrites are verified by provider readback, and GitHub release probes do not seed provider state.
Commands run:
cargo fmt --allcargo fmt --all -- --checkgit diff --checkcargo test -p ironclaw_webui activity_id -- --nocapturecargo test -p ironclaw_webui skill_content_and_mutations_use_product_surface -- --nocapturecargo test -p ironclaw_webui set_outbound_preferences_dispatches_body_through_invoke -- --nocapturecargo test -p ironclaw_webui --features test-support --test webui_v2_handlers_contract -- --nocapturecargo test -p ironclaw_reborn_composition product_result_replay_ignores_unreadable_summary_sidecar -- --nocapturecargo test -p ironclaw_reborn_composition product_operation_resolution_persists_replayable_success -- --nocapturecargo test -p ironclaw_reborn_composition --features test-support --test webui_v2_e2e operator_llm_config::nearai_provider_save_persists_key_and_survives_resave -- --nocapturecargo test -p ironclaw_product_workflow upsert_llm_provider_allows_loopback_base_url_for_self_hosted -- --nocapturecargo clippy -p ironclaw_product_workflow -p ironclaw_reborn_composition -p ironclaw_webui --all-targets --all-features -- -D warningscargo clippy -p ironclaw_reborn_composition --all-targets --all-features -- -D warningscorepack pnpm vitest run src/lib/api.test.ts src/pages/settings/lib/settings-api.test.tscorepack pnpm lint:conventionscorepack pnpm typecheckpython3 -m py_compile tests/e2e/scenarios/test_emulate_reborn_provider_contracts.py tests/e2e/scenarios/test_reborn_qa_trace_full_path.pytests/e2e/.venv/bin/python -m pytest tests/e2e/scenarios/test_reborn_webui_v2_smoke.py -q(29 passed)Note:
corepack pnpm lintwas attempted, but the package script shells out topnpmand this environment exposes pnpm only through Corepack. The underlying script steps,lint:conventionsandtypecheck, both passed viacorepack pnpm.Security Impact
No new authentication bypass, network access, or secret exposure. Retry identities now use validated opaque client action IDs for product mutations that need replay, and raw API key values are not seeded into activity IDs. Product replay stores bounded result bytes under the existing scoped
/product-resultsroot.Reborn Trust-Boundary Checklist
serde(default)fields fail closed or have migration tests. Optional client action fields are validated at ingress and covered by handler/frontend tests.PRODUCT_RESULT_MAX_BYTES; per-activity locks now avoid removing entries while waiters may still exist.Transient,Permanent,Misconfigured,PolicyDeniedor equivalent). Existing validation/error mapping retained.Database Impact
None. No schema migration is included. Durable ProductSurface replay uses the existing filesystem-backed scoped product result storage.
Blast Radius
Touches ProductSurface capability/operation invocation, WebUI product mutation activity IDs, frontend request serialization for setup/outbound/provider settings, and E2E provider trace/readback helpers. A regression would most likely appear as duplicate product mutation behavior, extension setup/lifecycle retry behavior, provider upsert retries, outbound preferences saves, or provider fixture readback/probe failures.
Rollback Plan
Revert this PR and restore the prior ProductSurface/ProductWorkflow ingress contract while replay semantics are revisited. No database schema rollback is required.
Review Follow-Through
extension_setup_submitdurable replay, permanent value-derived activity keys, ProductSurface lock cleanup races, and Google Drive upload readback.Review track: C (runtime/permissions/browser CI)