fix(reborn): stabilize Reborn Playwright nightly matrix - #6777
Merged
Merged
Conversation
italic-jinxin
added
size: L
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6777
July 28, 2026 10:42
Destroyed
github-actions
Bot
added
scope: ci
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds typed Trace Hold authorization input handling and contract coverage, bounded Reborn Playwright diagnostics, workflow updates, deterministic scenario setup, and refreshed WebUI E2E assertions. ChangesReborn authorization and Playwright coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PlaywrightWorkflow
participant RebornWebUIHarness
participant BrowserContext
participant RebornServer
participant ArtifactUpload
PlaywrightWorkflow->>RebornWebUIHarness: set artifact directory and byte budget
RebornWebUIHarness->>RebornServer: start server with workspace and logs
RebornWebUIHarness->>BrowserContext: capture screenshots, trace, and video
RebornServer->>RebornWebUIHarness: write stdout and stderr logs
PlaywrightWorkflow->>ArtifactUpload: upload shard diagnostics
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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_reborn_webui_v2_legacy_pending_messages.py`:
- Around line 867-872: Update the retry assertions in the test around
_submitted_response() to verify the expected successful WebUI state after
clicking “Retry message.” Retain the existing checks that the failed message is
resolved, but also assert the rendered content or state produced by
_submitted_response() so a different terminal error cannot satisfy the test.
🪄 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: e1223989-3099-474e-a961-b3ed87d055d8
📒 Files selected for processing (12)
.github/workflows/README.md.github/workflows/reborn-playwright.ymlcrates/ironclaw_product/src/reborn_services/product_capability_handlers.rscrates/ironclaw_product/tests/reborn_services_contract.rstests/e2e/reborn_webui_harness.pytests/e2e/scenarios/test_reborn_v2_file_download.pytests/e2e/scenarios/test_reborn_webui_v2_extensions_api.pytests/e2e/scenarios/test_reborn_webui_v2_legacy_extensions.pytests/e2e/scenarios/test_reborn_webui_v2_legacy_pending_messages.pytests/e2e/scenarios/test_reborn_webui_v2_legacy_settings_search.pytests/e2e/scenarios/test_reborn_webui_v2_legacy_tool_execution.pytests/e2e/scenarios/test_reborn_webui_v2_legacy_tool_permissions.py
Contributor
🔎 Review · PR #6777
The target changed before this Run could finish. Automatic · PR opened · attempt 0 of 3 · cancelled after 1s Run details
|
Contributor
Author
|
@claude review |
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6777
July 28, 2026 10:51
Destroyed
This comment was marked as resolved.
This comment was marked as resolved.
|
🚅 Deployed to the ironclaw-pr-6777 environment in ironclaw-ci-preview
|
Contributor
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.73% — 313854 / 366095 lines Per-crate breakdown (60 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)
|
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6777
July 28, 2026 11:19
Destroyed
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6777
July 28, 2026 11:22
Destroyed
…playwright # Conflicts: # crates/ironclaw_product/tests/reborn_services_contract.rs
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6777
July 29, 2026 07:41
Destroyed
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)
crates/ironclaw_product/tests/reborn_services_contract.rs (1)
2330-2360: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCover successful submission-ID forwarding.
This test proves malformed input is rejected, but not that
request.submission_idreachesauthorize_trace_hold; an implementation that drops the field and supplies another invalid value could produce the same assertion. Keep this validation case and add a valid-ID caller test that verifies the authorization result or captures the forwarded argument through a test seam.As per path instructions, changed production-wired behavior requires a caller-level regression test that exercises the affected behavior, not only validation failure.
🤖 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_product/tests/reborn_services_contract.rs` around lines 2330 - 2360, Add a caller-level success test alongside trace_hold_authorize_capability_decodes_typed_product_input that supplies a valid submission_id, invokes ProductSurface through RebornServices, and verifies the authorization result or captured argument passed to authorize_trace_hold. Preserve the existing malformed-input validation test and use a test seam or fake to confirm the exact request.submission_id is forwarded.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.
Outside diff comments:
In `@crates/ironclaw_product/tests/reborn_services_contract.rs`:
- Around line 2330-2360: Add a caller-level success test alongside
trace_hold_authorize_capability_decodes_typed_product_input that supplies a
valid submission_id, invokes ProductSurface through RebornServices, and verifies
the authorization result or captured argument passed to authorize_trace_hold.
Preserve the existing malformed-input validation test and use a test seam or
fake to confirm the exact request.submission_id is forwarded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1e439e8a-7d25-4070-b2d1-e4c8172b62d5
📒 Files selected for processing (2)
crates/ironclaw_product/src/reborn_services/product_capability_handlers.rscrates/ironclaw_product/tests/reborn_services_contract.rs
hanakannzashi
left a comment
hanakannzashi
left a comment
Contributor
There was a problem hiding this comment.
Static review only; tests were not run.
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6777
July 29, 2026 09:16
Destroyed
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/reborn_webui_harness.py (1)
259-271: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAnchor the phase-suffix split instead of splitting on the first
" (".
node_idis derived by taking everything before the first" ("inPYTEST_CURRENT_TEST. If a parametrize id contains" (", this truncates to the wrong value and no longer matchesitem.nodeidused byconftest.py'spytest_runtest_makereport. The bundle then never gets located by_mark_registered_artifact_bundles_failed/_finalize_registered_artifact_bundles, so its pending sentinel is never cleared — it stays budget-protected forever for the rest of the shard run. Pytest's own docs on this variable are explicit: "the actual format can be changed between releases (even bug fixes) so it shouldn't be relied on for scripting or automation."Anchor the split on the known phase suffix instead of the first occurrence:
🔧 Proposed fix
- node_id = os.environ.get("PYTEST_CURRENT_TEST", "browser-context").split( - " (", 1 - )[0] + raw_node_id = os.environ.get("PYTEST_CURRENT_TEST", "browser-context") + node_id = re.sub(r" \((?:setup|call|teardown)\)$", "", raw_node_id)#!/bin/bash # Check whether any parametrize ids in tests/e2e embed a space followed by "(", # which would break the current-first-occurrence split. rg -n 'pytest\.mark\.parametrize' -A5 tests/e2e | rg -n 'ids='🤖 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/reborn_webui_harness.py` around lines 259 - 271, Update the node_id parsing in the artifact-registration flow to remove only pytest’s known phase suffix, rather than splitting at the first " (". Preserve any " (" text contained in parametrization IDs so the resulting value matches conftest.py’s item.nodeid and remains discoverable by _mark_registered_artifact_bundles_failed and _finalize_registered_artifact_bundles.
🤖 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/reborn_webui_harness.py`:
- Around line 259-271: Update the node_id parsing in the artifact-registration
flow to remove only pytest’s known phase suffix, rather than splitting at the
first " (". Preserve any " (" text contained in parametrization IDs so the
resulting value matches conftest.py’s item.nodeid and remains discoverable by
_mark_registered_artifact_bundles_failed and
_finalize_registered_artifact_bundles.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: facb03e0-c910-43ce-bd9f-8d075c247f6f
📒 Files selected for processing (4)
tests/e2e/CLAUDE.mdtests/e2e/conftest.pytests/e2e/reborn_webui_harness.pytests/e2e/scenarios/test_reborn_webui_harness_artifacts.py
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6777
July 29, 2026 09:27
Destroyed
This was referenced Jul 29, 2026
hanakannzashi
approved these changes
Jul 29, 2026
hanakannzashi
left a comment
hanakannzashi
left a comment
Contributor
There was a problem hiding this comment.
Static re-review complete; prior comments are addressed. Approved without running tests, per request.
This was referenced Jul 30, 2026
l3ocifer
pushed a commit
to l3ocifer/frick-ironclaw
that referenced
this pull request
Sep 3, 2026
* fix(product): decode trace hold authorization input * test(playwright): stabilize Reborn nightly matrix * test(playwright): harden nightly diagnostics * fix(e2e): preserve failed artifact bundles
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Linked Issue
Closes #6765
Validation
cargo fmt --all -- --checkcargo clippy -p ironclaw_product --all-targets -- -D warningscargo build -p ironclaw --bin ironclawcargo test -p ironclaw_product --test reborn_services_contract— 246 passedscripts/ci/check-e2e-matrix-files.sh .github/workflows/reborn-playwright.ymlSecurity Impact
None. Trace authorization remains scoped to the authenticated tenant and user. The change only corrects typed input decoding.
Database Impact
None.
Blast Radius
Limited to the Trace Commons hold-authorization product command and the shared Reborn Playwright harness/nightly workflow.
Rollback Plan
Revert the E2E commit to restore the previous nightly harness behavior, and revert the product commit independently if the trace authorization dispatch change causes a regression.
Review track: C