Repository navigation
test(e2e): isolate mutable Emulate provider worlds - #6525
Merged
Merged
Conversation
Contributor
🔎 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. |
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6525
July 22, 2026 21:37
Destroyed
github-actions
Bot
added
size: XS
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughWalkthroughEmulate-backed QA journeys reuse a module-scoped Reborn runtime, run providers on stable ports, reset mutated provider state between cases, and assert clean provider baselines before replay. Repeated isolation cases and supporting E2E documentation were added. ChangesProvider journey isolation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant QAJourney
participant ProviderFixture
participant ProviderWorld
participant RebornRuntime
participant EmulateProviders
QAJourney->>ProviderFixture: load trace and identify mutations
ProviderFixture->>ProviderWorld: reset mutated services
ProviderWorld->>EmulateProviders: restart or clean provider state
ProviderFixture->>RebornRuntime: reuse shared runtime
RebornRuntime->>EmulateProviders: replay journey
ProviderFixture->>EmulateProviders: assert baseline and clean Slack mutations
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 |
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
⚠️ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| 0 | 0 | 0 | e0a598d59f4d |
Head: e0a598d59f4dc53cff9cb8f4b64227761654dcdb
Next: Human review or validation is required before merging.
Run details
Status: Current
Needs human: no
Needs validation: yes
Summary
Scoped review of the 3-file, 332-addition E2E fixture change found no concrete correctness or security defect. Runtime validation is still required because this environment lacks Python, the E2E virtualenv, and the built Reborn binary.
Findings
None.
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.
![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_qa_trace_full_path.py`:
- Around line 982-1016: The Slack replay isolation logic in
_cleanup_slack_provider_mutations must include threaded messages: at
tests/e2e/scenarios/test_reborn_qa_trace_full_path.py:982-1016, enumerate thread
sends via the same conversations.replies pagination path used at
tests/e2e/scenarios/test_reborn_qa_trace_full_path.py:952-979, then match and
remove them with chat.delete; update the baseline-absence checks in the sibling
range as needed, while preserving existing non-thread history handling.
🪄 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: ff3c4f0c-d1f4-4e92-93c0-dbf27f8ce436
📒 Files selected for processing (3)
tests/e2e/CLAUDE.mdtests/e2e/conftest.pytests/e2e/scenarios/test_reborn_qa_trace_full_path.py
Contributor
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 86.32% — 309582 / 358644 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)
|
8 of 21 tasks
|
🚅 Deployed to the ironclaw-pr-6525 environment in ironclaw-ci-preview
|
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6525
July 22, 2026 22:27
Destroyed
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6525
July 23, 2026 07:23
Destroyed
78 of 84 tasks
This was referenced Jul 23, 2026
5 of 17 tasks
serrrfirat
added a commit
that referenced
this pull request
Jul 28, 2026
* test(e2e): replay the provider journeys in reverse order nightly Workstream 3 of #6524 owes four isolation proofs: a journey must pass alone, twice consecutively, after another mutating journey, and in reversed order. #6525 landed the doubled-repeat arm for two journeys. This adds the reversed arm. Reversing is the cheapest arrangement that puts every case somewhere it has never run, so it is the arm that actually catches state leaking between journeys through the shared Google/Slack/GitHub worlds — a case that only passes because an earlier one left the world in a convenient shape fails here and nowhere else. Ordering is a parameter of `provider_journey_runs(reverse=...)` rather than a hidden environment read, because the failure mode to design out is a "reversed" lane that silently runs forward: it would pass exactly like the ordinary lane and quietly retire the proof it was added to provide. So the reversal is asserted directly — order flipped, multiset unchanged, and more than one run so the comparison cannot pass vacuously — plus the env parsing that CI actually sets. Verified by sabotage: making `reverse=True` a no-op fails the new test. Wired through the existing `reborn-e2e` workflow_call rather than a new job, so the Emulate setup is reused; off by default and enabled only by nightly-deep-ci, since it doubles the slowest lane. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(e2e): make the reverse replay share one provider world Review caught that the original lane could not fail. The ordinary parametrized replay runs behind `reborn_qa_emulate_provider_server`, a function-scoped fixture whose `finally` cleans Slack and resets every other mutated provider after each case. Every journey therefore starts from seed state regardless of position, so reversing that list could not expose cross-journey leakage — there was no surviving state to leak. The lane was a no-op wearing the shape of a proof, which is exactly the failure the guards in this PR were supposed to rule out; they proved the reversal was configured, not that reversing could detect anything. The reverse proof now lives in its own scenario that drives `reborn_qa_emulate_runtime` directly, bypassing the per-case reset fixture: it replays only the mutating journeys, back to front, keeping every prior provider effect in place, and cleans up once after the whole sequence. A journey that only passes because an earlier one left the world in a convenient shape now fails there and nowhere else. The scenario also fails closed when `IRONCLAW_JOURNEY_ORDER` is not set to reverse, so a workflow edit cannot quietly downgrade the expensive lane back into a forward replay. Ordinary replay ordering is unchanged and stays per-case isolated — reversing it was never meaningful. `pytest.mark.parametrize` values are a list of tuples per the repo lint invariant (PT007). Sabotage-verified: making the shared-world reversal a no-op fails `test_shared_world_replay_reverses_each_mutating_journey_once`. Reverted; 33 pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(e2e): keep the shared-world replay out of the ordinary lane Two CI failures from the previous commit, both real. The shared-world scenario lives in test_reborn_qa_trace_full_path.py, which the ordinary PR step runs whole. That step does not set IRONCLAW_JOURNEY_ORDER, so the scenario's fail-closed assert fired on every PR. Deselect it there by node id rather than softening the assert to a skip: the assert is what stops a workflow edit from quietly downgrading the nightly proof into a forward replay, and a skip would look identical to a pass in exactly that case. Extracting the replay body into `_replay_qa_journey_provider_leg` also broke the journey-evidence checker, which requires the declared test to call its readback helper — the calls now sit one hop away. The checker now follows a single level of module-level delegation, but only through an *awaited* call: an un-awaited coroutine never runs, and arbitrary depth would let a readback buried behind several hops vouch for itself where no reviewer can see it at the call site. Following nothing is the default, so an omitted argument makes the check stricter, not looser. Three self-tests pin that: an awaited delegate is accepted, an un-awaited one is rejected, and two levels are rejected. 54 tests pass across the two affected files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(e2e): filter the shared-world replay by marker, not --deselect The `--deselect` added in the previous commit silently did nothing. It matched when the step passed a single file, which is how I checked it, and stopped matching once the real step passed six — so CI ran the shared-world scenario in the ordinary lane anyway and it failed its fail-closed assert exactly as before. That is the same failure shape this PR keeps circling: a guard that looks applied, reports nothing, and changes no behaviour. The local check was too narrow to see it. A marker does not depend on nodeid path resolution. The scenario is marked `shared_world`, the marker is registered in pyproject so an unknown-marker typo cannot pass silently, and the ordinary lane filters with `-m "not shared_world"`. The nightly lane still selects it by node id, and its fail-closed assert is untouched. Verified both directions with the full six-file list the step actually uses: the ordinary lane collects zero shared-world tests, and the nightly node id still collects exactly one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This was referenced Jul 28, 2026
l3ocifer
pushed a commit
to l3ocifer/frick-ironclaw
that referenced
this pull request
Sep 3, 2026
…rai#6728) * test(e2e): replay the provider journeys in reverse order nightly Workstream 3 of nearai#6524 owes four isolation proofs: a journey must pass alone, twice consecutively, after another mutating journey, and in reversed order. nearai#6525 landed the doubled-repeat arm for two journeys. This adds the reversed arm. Reversing is the cheapest arrangement that puts every case somewhere it has never run, so it is the arm that actually catches state leaking between journeys through the shared Google/Slack/GitHub worlds — a case that only passes because an earlier one left the world in a convenient shape fails here and nowhere else. Ordering is a parameter of `provider_journey_runs(reverse=...)` rather than a hidden environment read, because the failure mode to design out is a "reversed" lane that silently runs forward: it would pass exactly like the ordinary lane and quietly retire the proof it was added to provide. So the reversal is asserted directly — order flipped, multiset unchanged, and more than one run so the comparison cannot pass vacuously — plus the env parsing that CI actually sets. Verified by sabotage: making `reverse=True` a no-op fails the new test. Wired through the existing `reborn-e2e` workflow_call rather than a new job, so the Emulate setup is reused; off by default and enabled only by nightly-deep-ci, since it doubles the slowest lane. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(e2e): make the reverse replay share one provider world Review caught that the original lane could not fail. The ordinary parametrized replay runs behind `reborn_qa_emulate_provider_server`, a function-scoped fixture whose `finally` cleans Slack and resets every other mutated provider after each case. Every journey therefore starts from seed state regardless of position, so reversing that list could not expose cross-journey leakage — there was no surviving state to leak. The lane was a no-op wearing the shape of a proof, which is exactly the failure the guards in this PR were supposed to rule out; they proved the reversal was configured, not that reversing could detect anything. The reverse proof now lives in its own scenario that drives `reborn_qa_emulate_runtime` directly, bypassing the per-case reset fixture: it replays only the mutating journeys, back to front, keeping every prior provider effect in place, and cleans up once after the whole sequence. A journey that only passes because an earlier one left the world in a convenient shape now fails there and nowhere else. The scenario also fails closed when `IRONCLAW_JOURNEY_ORDER` is not set to reverse, so a workflow edit cannot quietly downgrade the expensive lane back into a forward replay. Ordinary replay ordering is unchanged and stays per-case isolated — reversing it was never meaningful. `pytest.mark.parametrize` values are a list of tuples per the repo lint invariant (PT007). Sabotage-verified: making the shared-world reversal a no-op fails `test_shared_world_replay_reverses_each_mutating_journey_once`. Reverted; 33 pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(e2e): keep the shared-world replay out of the ordinary lane Two CI failures from the previous commit, both real. The shared-world scenario lives in test_reborn_qa_trace_full_path.py, which the ordinary PR step runs whole. That step does not set IRONCLAW_JOURNEY_ORDER, so the scenario's fail-closed assert fired on every PR. Deselect it there by node id rather than softening the assert to a skip: the assert is what stops a workflow edit from quietly downgrading the nightly proof into a forward replay, and a skip would look identical to a pass in exactly that case. Extracting the replay body into `_replay_qa_journey_provider_leg` also broke the journey-evidence checker, which requires the declared test to call its readback helper — the calls now sit one hop away. The checker now follows a single level of module-level delegation, but only through an *awaited* call: an un-awaited coroutine never runs, and arbitrary depth would let a readback buried behind several hops vouch for itself where no reviewer can see it at the call site. Following nothing is the default, so an omitted argument makes the check stricter, not looser. Three self-tests pin that: an awaited delegate is accepted, an un-awaited one is rejected, and two levels are rejected. 54 tests pass across the two affected files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(e2e): filter the shared-world replay by marker, not --deselect The `--deselect` added in the previous commit silently did nothing. It matched when the step passed a single file, which is how I checked it, and stopped matching once the real step passed six — so CI ran the shared-world scenario in the ordinary lane anyway and it failed its fail-closed assert exactly as before. That is the same failure shape this PR keeps circling: a guard that looks applied, reports nothing, and changes no behaviour. The local check was too narrow to see it. A marker does not depend on nodeid path resolution. The scenario is marked `shared_world`, the marker is registered in pyproject so an unknown-marker typo cannot pass silently, and the ordinary lane filters with `-m "not shared_world"`. The nightly lane still selects it by node id, and its fail-closed assert is untouched. Verified both directions with the full six-file list the step actually uses: the ordinary lane collects zero shared-world tests, and the nightly node id still collects exactly one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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
Change Type
Linked Issue
Part of #6524
Validation
cargo fmt --all -- --check— Not applicable: no Rust files changed.cargo clippy --all --benches --tests --examples --all-features -- -D warnings— Not applicable: no Rust files changed.cargo build—cargo build -p ironclaw --bin ironclawcargo test --features integrationif database-backed or integration behavior changed — Not applicable: test harness only; no database or product behavior changed.review-prorpr-shepherd --fixwas run before requesting review — iterativeautoreviewcompleted with no actionable findings.Test Strategy
User behavior: Full-path recorded QA journeys run hermetically without inheriting mutable provider state from a previous case, while CI retains one built binary and one module-scoped Reborn process.
Risk areas:
Tests added or updated:
What the tests prove: Provider mutation cases start clean, produce the expected provider-observable side effect, and cannot pass only because an earlier case left matching data behind. Read-only provider processes remain reused.
Commands run:
Result:
96 passed in 107.35s; fixture validation passed for 61 files.Security Impact
None. The change only controls local Emulate processes and test data cleanup; it does not alter product permissions, secrets, network policy, file access, tool execution, or sandbox behavior.
Reborn Trust-Boundary Checklist
N/A: test-harness-only change; no Reborn policy, runtime, persistence, status, error, serialization, or trust-bearing production types changed.
Database Impact
None.
Blast Radius
Limited to the Emulate-backed E2E fixtures and full-path QA trace matrix. A fixture lifecycle bug could cause provider tests to fail or leak local test state; product binaries and runtime behavior are unchanged.
Rollback Plan
Revert this commit to restore the previous module-scoped provider fixtures. No data migration or compatibility step is required.
Review Follow-Through
CodeRabbit thread 3634027367 was addressed in
de7e32be6: threaded Slack sends now useconversations.repliesfor outcome, baseline, and exact-timestamp cleanup, with a real Emulate regression test. No known review follow-up remains.Review track: C (CI/test infrastructure)