Repository navigation
Fix main Reborn Playwright failures - #6554
ilblackdragon wants to merge 5 commits into
Conversation
🔎 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. |
github-actions
Bot
added
size: XL
📝 WalkthroughSummary by CodeRabbit
WalkthroughWebUI extension mutations now carry client action identifiers through frontend requests, handlers, activity IDs, and tests. Extension removal uses active package snapshots and supports already-absent cleanup. Lifecycle mounts, command parsing, frontend presentation, attachment limits, and E2E contracts are also updated. ChangesWebUI action identity propagation
Extension lifecycle removal and mounting
WebUI contract and presentation updates
Estimated code review effort: 4 (Complex) | ~45 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 |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![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 | 1 | 0 | 1 | 3d648f4f1289 |
Head: 3d648f4f12897c2397c10fda9d8c3a85fc13c085
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 backend preserves supplied lifecycle action IDs, but the browser generates a new ID on each mutation call, so response-lost retries cannot reuse the ProductSurface activity ID.
Findings
Blocking: 1 / Notes: 0
Blocking findings
1. ❌ [MEDIUM] Lifecycle retries mint a new client action ID
Location: crates/ironclaw_webui/frontend/src/pages/extensions/lib/extensions-api.ts:22
The ID is generated inside each helper call, and install/activate/remove do not accept an ID from a retry layer; setup similarly omits one when calling setupExtension. Retrying after a lost response therefore creates a different extension_lifecycle_activity_id, so the backend's retry identity guarantee is unreachable from the browser. Generate and retain one ID per user gesture, pass it through every retry, and cover that behavior in a frontend 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.
| body: JSON.stringify({ package_ref: packageRef }), | ||
| body: JSON.stringify({ | ||
| package_ref: packageRef, | ||
| client_action_id: clientActionId(), |
There was a problem hiding this comment.
This ID is minted per helper invocation, not per user gesture. A response-lost retry calls the helper again and gets a new ID, producing a different ProductSurface activity instead of the stable retry identity tested by the handler contract. Retain/pass an action ID from the mutation or retry layer, including setup.
|
🚅 Deployed to the ironclaw-pr-6554 environment in ironclaw-ci-preview
|
![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 (2)
crates/ironclaw_reborn_composition/src/webui/product_capability.rs (1)
252-267: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRoute extension-lifecycle mounts through the Production/LocalDev branch.
product_invocation_mounts()only branchesProductCapabilityMountsfor skill management, while extension install/activate/remove always callscrate::local_dev_mounts::system_extensions_lifecycle_mount_view()from a local-dev module and tests onlyProductCapabilityMounts::LocalDev. Add/cover the intended production mount path so production capability invocations do not get an unscopable or stale mount view.Compliance: violation of codebase invariant from
crates/ironclaw_reborn_composition/src/webui/product_capability.rsthat production capability grants useProductCapabilityMounts::Productionand fail closed for missing/incorrect mounts.🤖 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 252 - 267, Update product_invocation_mounts so extension-lifecycle capabilities also branch on ProductCapabilityMounts::LocalDev versus ProductCapabilityMounts::Production instead of always calling system_extensions_lifecycle_mount_view. Add or reuse the production-specific mount-view function for the Production branch, preserve scoped local-dev behavior, and ensure missing or incorrect production mounts fail closed.crates/ironclaw_webui/src/webui_v2/handlers.rs (1)
2267-2292: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated parse-then-derive-activity-id boilerplate across all four lifecycle handlers.
install_extension,activate_extension,remove_extension,setup_extensioneach repeat:let client_action_id = parse_webui_client_action_id(body.client_action_id...).map_err(RebornServicesError::from)?; let activity_id = extension_lifecycle_activity_id(&caller, CAPABILITY, &package_ref, client_action_id.as_str())?;As per coding guidelines, "Keep functions focused and extract helpers when logic is reused." Extracting a single helper reduces the risk of one call site drifting (e.g., forgetting the
.map_errmapping) as this pattern grows.♻️ Proposed helper
fn parse_extension_lifecycle_activity_id( caller: &WebUiAuthenticatedCaller, capability: ProductCapabilityDescriptor, package_ref: &LifecyclePackageRef, client_action_id: Option<String>, ) -> Result<ActivityId, RebornServicesError> { let client_action_id = parse_webui_client_action_id(client_action_id).map_err(RebornServicesError::from)?; extension_lifecycle_activity_id(caller, capability, package_ref, client_action_id.as_str()) }Each handler then calls
parse_extension_lifecycle_activity_id(&caller, CAPABILITY, &package_ref, body.client_action_id)?.Also applies to: 2318-2347, 2350-2379, 2555-2605
🤖 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 2267 - 2292, Extract the repeated client-action parsing and lifecycle activity-ID derivation from install_extension, activate_extension, remove_extension, and setup_extension into a shared parse_extension_lifecycle_activity_id helper. Preserve the existing parse_webui_client_action_id error mapping and pass each handler’s caller, capability, package_ref, and body.client_action_id through the helper.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@crates/ironclaw_reborn_composition/src/extension_host/extension_lifecycle.rs`:
- Around line 2184-2189: In the active package selection near
active_extensions.snapshot().get_extension(), add a concise inline comment
documenting that hosted-MCP discovery can make the active-registry package
differ from lifecycle_package, so the snapshot lookup is preferred and
lifecycle_package is only the fallback.
In
`@crates/ironclaw_webui/frontend/src/pages/chat/components/auth-oauth-card.tsx`:
- Around line 45-54: Update PROVIDER_DISPLAY_NAMES to include the
github-to-GitHub override, then remove the separate providerId === ["git",
"hub"].join("") branch from providerDisplayName so all provider display-name
overrides use the map.
In `@crates/ironclaw_webui/frontend/src/pages/extensions/lib/extensions-api.ts`:
- Around line 17-43: Update installExtension, activateExtension, and
removeExtension to accept an optional caller-supplied client action ID and use
it instead of always invoking clientActionId(), matching setupExtension’s
override behavior. Propagate this stable ID through the related hook and
mutation paths so retries reuse the same activity identity while preserving
generated IDs when no override is provided.
---
Outside diff comments:
In `@crates/ironclaw_reborn_composition/src/webui/product_capability.rs`:
- Around line 252-267: Update product_invocation_mounts so extension-lifecycle
capabilities also branch on ProductCapabilityMounts::LocalDev versus
ProductCapabilityMounts::Production instead of always calling
system_extensions_lifecycle_mount_view. Add or reuse the production-specific
mount-view function for the Production branch, preserve scoped local-dev
behavior, and ensure missing or incorrect production mounts fail closed.
In `@crates/ironclaw_webui/src/webui_v2/handlers.rs`:
- Around line 2267-2292: Extract the repeated client-action parsing and
lifecycle activity-ID derivation from install_extension, activate_extension,
remove_extension, and setup_extension into a shared
parse_extension_lifecycle_activity_id helper. Preserve the existing
parse_webui_client_action_id error mapping and pass each handler’s caller,
capability, package_ref, and body.client_action_id through the helper.
🪄 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: b4181f0f-dec3-4be4-a62a-4c4ea7b794f1
📒 Files selected for processing (30)
crates/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/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/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_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/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/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
|
@ironloopai review --agent reviewer |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/e2e/scenarios/test_emulate_reborn_provider_contracts.py`:
- Around line 231-232: Restrict the 404 skip branches around the Google Docs and
corresponding provider readback checks to the fixture’s declared Emulate 0.7.0
version or explicit unsupported-capability flag. Allow all other 404 responses
to continue through the normal assertions so incorrect routes, authentication
failures, fixture issues, and regressions fail the tests; preserve provider
readback/state assertions for supported capabilities.
🪄 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: 7aa6e9c6-ae54-4196-8ca4-244c22af4208
📒 Files selected for processing (11)
crates/ironclaw_product_workflow/src/commands.rscrates/ironclaw_product_workflow/tests/product_commands_contract.rscrates/ironclaw_reborn_composition/src/factory.rscrates/ironclaw_reborn_composition/src/webui/product_capability.rscrates/ironclaw_reborn_composition/tests/webui_v2_e2e.rscrates/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.tstests/e2e/scenarios/test_emulate_reborn_provider_contracts.pytests/e2e/scenarios/test_reborn_qa_trace_full_path.py
| if created.status_code == 404: | ||
| pytest.skip("Emulate 0.7.0 does not expose the Google Docs API") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Narrow the 404 skip to the known unsupported Emulate capability.
These branches skip on any 404, so wrong routes, fixture misconfiguration, auth failures, or emulator regressions become green skipped tests. Gate the skip on the fixture’s declared Emulate version/capability; let unrelated 404 responses fail normally.
As per coding guidelines, Emulate-backed provider tests must assert provider readback/state; this broad skip weakens that contract.
Also applies to: 277-278
🤖 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_emulate_reborn_provider_contracts.py` around lines
231 - 232, Restrict the 404 skip branches around the Google Docs and
corresponding provider readback checks to the fixture’s declared Emulate 0.7.0
version or explicit unsupported-capability flag. Allow all other 404 responses
to continue through the normal assertions so incorrect routes, authentication
failures, fixture issues, and regressions fail the tests; preserve provider
readback/state assertions for supported capabilities.
Source: Coding guidelines
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 (1)
tests/e2e/scenarios/test_reborn_qa_trace_full_path.py (1)
787-799: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSingle-caller wrapper — inline into its only caller.
_install_inline_traceonly wraps a POST to/__mock/llm_traceand is called once, fromtest_provider_operation_case_executes_with_provider_readback(line 1365). Based on learnings, prefer a cohesive single-operation implementation over extracting a helper used by only one caller when it doesn't materially reduce complexity — this is exactly the "wrapper around a fetch flow" case called out.♻️ Inline into the single caller
-async def _install_inline_trace( - mock_llm_server: str, - source: str, - trace: dict, -) -> None: - async with httpx.AsyncClient() as client: - response = await client.post( - f"{mock_llm_server}/__mock/llm_trace", - json={"source": source, "trace": trace}, - timeout=15, - ) - response.raise_for_status() - - def _provider_operation_trace(case: ProviderOperationCase) -> dict:And at the call site:
await operation_case.assert_baseline(emulate_url) - await _install_inline_trace(mock_llm_server, source, trace) + async with httpx.AsyncClient() as client: + response = await client.post( + f"{mock_llm_server}/__mock/llm_trace", + json={"source": source, "trace": trace}, + timeout=15, + ) + response.raise_for_status()🤖 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 787 - 799, Remove the single-use _install_inline_trace helper and inline its HTTP POST and response.raise_for_status flow directly into test_provider_operation_case_executes_with_provider_readback, preserving the existing request payload, timeout, and async client behavior.Source: Learnings
🤖 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 `@tests/e2e/scenarios/test_reborn_qa_trace_full_path.py`:
- Around line 787-799: Remove the single-use _install_inline_trace helper and
inline its HTTP POST and response.raise_for_status flow directly into
test_provider_operation_case_executes_with_provider_readback, preserving the
existing request payload, timeout, and async client behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f6b4e9f3-ca2f-477c-bf20-b2d8ea998e59
📒 Files selected for processing (5)
crates/ironclaw_reborn_composition/src/extension_host/extension_lifecycle.rscrates/ironclaw_webui/frontend/src/pages/chat/components/auth-oauth-card.tsxtests/e2e/conftest.pytests/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 | 9b11181e37fd |
Head: 9b11181e37fdf8fcb8a1ee1a5ba8ba89768f8331
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 a blocking lifecycle-retry defect: stable activity IDs are not backed by replay/deduplication in the real Reborn execution path.
Findings
Blocking: 1 / Notes: 0
Blocking findings
1. ❌ [HIGH] Stable lifecycle IDs do not replay retries
Location: crates/ironclaw_webui/src/webui_v2/handlers.rs:2281
Passing a stable activity ID here does not make a retry idempotent. The runtime invoker calls HostRuntime::invoke_capability again; its durable run-state store rejects the same invocation ID as InvocationAlreadyExists instead of replaying the first outcome. Extension setup takes the other branch and executes its direct product operation again because that branch ignores activity_id. A response-lost retry therefore fails or repeats the side effect. Add a ProductSurface-level replay/deduplication path for all four lifecycle operations and cover it with a real-runtime retry 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.
| &package_ref, | ||
| client_action_id.as_str(), | ||
| )?; | ||
| let resolution = invoke_product_capability_with_activity_id( |
There was a problem hiding this comment.
The stable activity ID is not sufficient by itself: the real runtime re-enters HostRuntime::invoke_capability, whose run-state store rejects the already-used invocation ID rather than replaying the first outcome. Setup instead re-executes because its direct ProductOperation path ignores activity_id. A response-lost lifecycle retry therefore fails or duplicates work; add ProductSurface-level replay/deduplication and test a real-runtime retry.
|
Replaced by #6558, which carries the latest main merge and CI/review fixes on a fresh pull ref. |
This branch was successfully deployed
1 participant
Summary
client_action_idthrough install/activate/remove/setup and deriving stable scoped activity IDs without the old channel envelope.Change Type
Linked Issue
None.
Validation
cargo fmt --all -- --checkcargo clippy --all --benches --tests --examples --all-features -- -D warnings: Not run as full workspace; targeted touched-crate clippy command passed.cargo build: covered by earliercargo build -p ironclaw --bin ironclawbefore review-fix amendments.cargo test --features integrationif database-backed or integration behavior changed: targeted Reborn integration test passed.review-prorpr-shepherd --fixwas run before requesting review: Not applicable; review comments were inspected and addressed manually.Test Strategy
User behavior:
WebUI and CLI/channel ingress use ProductSurface as the product-facing boundary. Extension lifecycle gestures carry stable client action IDs so browser retries resolve to the same ProductSurface activity instead of dropping responses or colliding with unrelated requests.
Risk areas:
Tests added or updated:
reborn_integration_webui_v2_product_apiupdated to send lifecycleclient_action_idthrough the production WebUI facade.What the tests prove:
Extension lifecycle requests validate and preserve the client action ID from frontend/e2e/integration ingress, deterministic activity IDs are stable for retried install gestures, setup still forwards the expected product capability input, and the browser legacy extension flow matches the new API contract.
Commands run:
cargo fmt --all -- --checkgit diff --checkcargo build -p ironclaw --bin ironclawcargo test -p ironclaw_product_workflow --test product_commands_contractcargo test -p ironclaw_reborn_composition --test webui_v2_e2ecargo test -p ironclaw_reborn_composition product_invocation_mounts_grants_extension_lifecycle_mountscorepack pnpm --dir crates/ironclaw_webui/frontend lint:conventions && corepack pnpm --dir crates/ironclaw_webui/frontend typecheckcorepack pnpm --dir crates/ironclaw_webui/frontend test -- src/pages/extensions/lib/extensions-api.test.ts src/pages/extensions/hooks/useExtensions-catalog.test.tspython3 -m pytest scenarios/test_emulate_reborn_provider_contracts.py::test_emulate_google_covers_reborn_gsuite_read_inputs scenarios/test_emulate_reborn_provider_contracts.py::test_emulate_google_covers_reborn_docs_contract scenarios/test_emulate_reborn_provider_contracts.py::test_emulate_google_covers_reborn_sheets_contract -qfromtests/e2e(1 passed, 2 skipped because Emulate 0.7.0 lacks Docs/Sheets endpoints)python3 -m pytest scenarios/test_reborn_qa_trace_full_path.py -qfromtests/e2e(13 passed, 17 skipped for Emulate 0.7.0 unsupported Docs/Sheets/Slack GET routes)tests/e2e:python3 -m pytest <reborn-playwright matrix files> -k 'not test_reborn_legacy_always_approve_survives_reborn_restart' -q --timeout=120 --durations=25(204 passed, 1 deselected)Security Impact
No new authentication bypass, network access, or secret exposure. The LLM provider activity seed now records secret presence without hashing raw API-key bytes. Lifecycle action IDs are validated as WebUI client action IDs before being converted into scoped ProductSurface activity IDs.
Reborn Trust-Boundary Checklist
serde(default)fields fail closed or have migration tests. Optional client action fields are accepted only at compatibility seams; route validation still requires them for lifecycle POSTs.Transient,Permanent,Misconfigured,PolicyDeniedor equivalent). Existing validation error mapping retained.Database Impact
None.
Blast Radius
Touches WebUI extension lifecycle handlers, ProductSurface inbound request shaping, frontend/e2e lifecycle calls, and production WebUI integration fixtures. A regression would most likely appear as install/activate/remove validation failure, retry idempotency mismatch, or stale extension setup input shape.
Rollback Plan
Revert this PR and continue using the prior extension lifecycle request contract while the ProductSurface migration is revisited. This is source-compatible for persistence because no database schema changes are included.
Review Follow-Through
Review comments from #6546 were addressed here. #6546 is closed and #6553 will be closed because GitHub refused to reopen it after its pull ref stopped tracking the branch.
Review track: C (runtime/permissions/browser CI)