test(reborn): complete WS9 generated state machines - #6886
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesThe PR adds typed state-machine coverage inventories and fail-loud gap tests, exposes runtime resource governors to integration harnesses, adds LibSQL planned-runtime restart testing for approval outcomes, and strengthens orphan ownership/reservation and double-submit invariant checks. Reborn lifecycle validation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant PreRestartHarness
participant RebornIntegrationGroup
participant LibSQL
participant PostRestartHarness
PreRestartHarness->>LibSQL: persist parked approval-gated run
PreRestartHarness->>RebornIntegrationGroup: request planned runtime restart
RebornIntegrationGroup->>LibSQL: reopen durable composite
RebornIntegrationGroup->>PostRestartHarness: rebuild runtime and thread
PostRestartHarness->>LibSQL: apply approve, deny, or cancel
PostRestartHarness-->>PreRestartHarness: verify terminal status and effect count
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 |
|
🚅 Deployed to the ironclaw-pr-6886 environment in ironclaw-ci-preview
|
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
🔎 Review · PR #6886
1 actionable findings →Reviewed the complete trusted base-to-head comparison. The restart, concurrent-admission, harness, composition seam, workflow, and coverage-registry changes are generally coherent, but the registry overstates one journey’s executable invariant evidence. Automatic · PR opened + CI failed · attempt 1 of 3 · completed in 1m 56s Run details
|
There was a problem hiding this comment.
🔍 Review complete · PR #6886
💬 1 finding
Reviewed the complete trusted base-to-head comparison. The restart, concurrent-admission, harness, composition seam, workflow, and coverage-registry changes are generally coherent, but the registry overstates one journey’s executable invariant evidence.
Findings
- 🟠 Medium · Admission-only webhook evidence is credited with terminal stability —
tests/e2e/state_machine_coverage.py:161-164
Details are attached to the relevant diff.
Validation and technical details
- Inspected all 17 changed files and surrounding composition, resource-governor, process-journal, integration-harness, journey-registry, and evidence-checker code.
git diff --check refs/ironloop/base...refs/ironloop/headpassed.- Repository knowledge graph was unavailable; followed the required guidance-and-targeted-search fallback.
- Rust and Python tests could not be executed because
cargoanduvare not installed in the review environment. - Base:
main - Head:
codex/ws9-equivalence-state-machinesatc91a54b - Run:
cb549eef-89e6-4ddf-a7c9-aa5a357eb542
c91a54b to
df5081a
Compare
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
🔎 Review · PR #6886
1 actionable findings →Reviewed the complete trusted base-to-head comparison. The runtime test-support seam and restart/double-submit harness changes are coherent, but the new coverage registry overstates its mechanically proven lifecycle coverage: a required denial row is backed by a test that never verifies the claimed denied state. Automatic · PR opened · attempt 1 of 3 · completed in 2m 4s Run details
|
There was a problem hiding this comment.
🔍 Review complete · PR #6886
Reviewed the complete trusted base-to-head comparison. The runtime test-support seam and restart/double-submit harness changes are coherent, but the new coverage registry overstates its mechanically proven lifecycle coverage: a required denial row is backed by a test that never verifies the claimed denied state.
Findings
- 🟠 Medium · Denial coverage is credited without observing a denied lifecycle state —
tests/e2e/state_machine_coverage.py:277-290
Details are attached to the relevant diff.
Validation and technical details
- Compared refs/ironloop/base a378b39 through refs/ironloop/head df5081a across all 18 changed files.
- Inspected composition governor wiring, all test-support consumers, group restart reconstruction, generated gate/restart sequences, process/resource assertions, Python coverage registry/meta-tests, workflow, and Cargo target registration.
python3 -m py_compile tests/e2e/state_machine_coverage.py tests/e2e/scenarios/test_state_machine_coverage.pypassed.git diff --check refs/ironloop/base..refs/ironloop/headpassed.- Rust and pytest suites could not be executed because this review environment does not provide
cargooruv. - Base:
main - Head:
codex/ws9-equivalence-state-machinesatdf5081a - Run:
8a1dd829-716b-430f-b118-783e8490e047
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/state_machine_coverage.py`:
- Around line 105-121: Annotate _cargo, _operation, and _fault with their
appropriate return types. Import the provider operation and fault case types
from their respective modules, using those types for the selector helpers and
the existing CargoEvidence type for _cargo.
- Around line 178-195: Update _provider_case to derive invariants from
operation_class instead of unconditionally assigning
StateMachineInvariant.AT_MOST_ONCE_EFFECT. Include the effect invariant only for
write-capable operations, while READ cases such as github_get_issue and
google_sheets_read_values_empty use no effect invariant.
🪄 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: bf12ae04-8488-4ee6-ad11-731eda5107fd
📒 Files selected for processing (18)
.github/workflows/reborn-tests.ymlCargo.tomlcrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_reborn_composition/src/test_support/mod.rstests/e2e/scenarios/test_state_machine_coverage.pytests/e2e/state_machine_coverage.pytests/integration/generated_gate_sequences.rstests/integration/generated_restart_sequences.rstests/integration/support/assertions.rstests/integration/support/builder.rstests/integration/support/group.rstests/integration/support/harness/mod.rstests/integration/support/harness/profiles/core_builtin.rstests/integration/support/harness/profiles/github.rstests/integration/support/harness/profiles/mock_mcp.rstests/integration/support/harness/profiles/qa_smoke.rstests/integration/support/harness/profiles/web_access.rstests/integration/support/harness/recorder.rs
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.55% — 315466 / 368757 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)
|
df5081a to
7f78b10
Compare
* test(reborn): complete WS9 generated state machines * test(reborn): strengthen WS9 invariant evidence * test(reborn): align registry claims with evidence
Summary
lease_wedgetruthful uncertain-outcome proof and map it into the mechanical dimension × sequence × invariant registry. The registry reports 0 remaining WS9 gaps.Overlap reconciliation: #6794 remains the source of the initial lifecycle/focused-boundary slice and is not duplicated. This branch was rebased and adapted after #6696 made the row-native process journal the sole lifecycle authority. Open #5981 and #6096 do not currently implement this same-thread generated restart/double-submit scope; #5981 may intentionally change the current busy-admission outcome later.
Change Type
Linked Issue
Related #6524
Validation
cargo fmt --all -- --checkcargo clippy -p ironclaw_reborn_composition -p ironclaw_reborn_integration_tests --tests --all-features -- -D warningscargo build— not applicable: compilation is covered by both integration targets and scoped all-feature clippy; no shipping build behavior changed.cargo test --features integrationif database-backed or integration behavior changed — not applicable: no database implementation/schema behavior changed; the targeted LibSQL restart integration target exercises the affected persistence seam.pr-shepherdmaintainer-quality review completed before requesting review; its architecture/evidence findings were fixed and the focused validation was repeated on the final rebased head.Test Strategy
User behavior:
Generated Reborn runs retain one authoritative process tree and no leaked capability resource holds across trigger, retry, cancel, duplicate, restart, and concurrent same-thread double-submit interleavings. Durable approval outcomes remain recoverable after a real runtime restart.
Risk areas:
Tests added or updated:
tests/e2e/scenarios/test_state_machine_coverage.pymechanically checks all 7 dimensions, 37 representative rows, 10 required high-risk pairs, all 6 sequences, all 5 invariants, duplicate case-id rejection, and 0 retained gaps. Journey-derived claims require matching declared observable evidence, preventing admission-only journeys from being credited with terminal stability.reborn_generated_gate_sequenceschecks per-transition ownership/resource invariants and all same-thread double-submit schedules;reborn_generated_restart_sequencesrebuilds the coordinator/executor/scheduler/scope gateway/process journal on a fresh LibSQL connection for approve/deny/cancel after restart.What the tests prove:
trigger,retry,cancel,duplicate,restart,concurrent_double_submit).AgentTurn, every parent exists, no unexpected agent-turn process appears, and the exact production-composed governor has no leaked holds at quiescent blocked/terminal boundaries. In-flight transitions check process ownership without misclassifying a legitimate executing-capability reservation as an orphan.reborn_integration_lease_wedgeproof is the canonical evidence and is referenced by the registry rather than weakened or duplicated.Commands run:
Additional package-suite evidence:
cargo test -p ironclaw_reborn_composition: 541 passed; four timeout-shaped failures passed when rerun individually.observability::trace_capture::tests::capture_skips_when_policy_missing_or_disabledfails identically on an untouchedorigin/mainworktree, so it is a pre-existing, non-attributable base failure.df5081ad4merged artifact: global 85.54% (315436/368757), runner 86.87% (14669/16887), processes 88.07% (5839/6630), and turns 86.21% (9453/10965). The ratchet passes against that exact artifact with the new floors.Security Impact
None. The only composition change is feature-gated to
test-supportand returns the exact already-composed resource governor alongside the runtime for read-only invariant assertions. It does not alter authorization, approvals, network mediation, secrets, sandboxing, or production capability dispatch.Reborn Trust-Boundary Checklist
rgand compilation/clippy cover the touched runtime/test-support accessors.serde(default)fields fail closed or have migration tests: not applicable; no serialized fields changed.Database Impact
None. No migration, schema, query contract, or backend implementation changes. The restart generator uses LibSQL because it is the hermetic backend with an independent reopen recipe; unsupported InMemory/Postgres restart modes fail loudly rather than pretending to restart.
Blast Radius
Test infrastructure under
tests/e2eandtests/integration, the Reborn test workflow’s documented generated counts, and a feature-gated composition test-support builder. The main review risks are keeping the representative registry synchronized with supported enums and preserving current same-thread busy-admission semantics if #5981 later changes them.Rollback Plan
Revert commits
50348a75c,7f78b108a, and513478eb1. There is no migration, persisted format change, configuration change, or production default to roll back. CI would return to #6794’s prior generated coverage.Review Follow-Through
Maintainer self-review found and fixed two issues before readiness: the governor read seam initially grew the production runtime service shape, and generic journey rows initially inferred stronger lifecycle evidence than their declared assertions proved. The final seam is owned by composition test support, and the registry now derives lifecycle/sequence/invariant claims only from explicit observable evidence. If #5981 lands first and changes busy admission into queued steering, rebase and update the double-submit expected outcome deliberately rather than weakening the invariant.
The complete eight-reviewer
code-review-multipass then found eight actionable issues, all fixed: process-kind validation, in-flight reservation timing, restart and double-submit durable effect read-back, restart denominator simplification, coverage-floor recapture, owning-guide documentation, and private-helper naming. One additional suggestion—treating equal axes/evidence with different case IDs as duplicate rows—was reproduced and rejected becausecase_idis the executable parameter identity for distinct journey cases sharing one parametrized test.Three pre-existing bot findings on the reviewed old head were also fixed and resolved: provider read rows no longer claim an effect-count invariant, approval denial now uses the actually observed
Completedlifecycle with an explicit generated assertion, and the typed registry selector helpers have concrete return annotations.Checkbox-to-test/PR map
reborn_generated_restart_sequencesin this PR.reborn_integration_lease_wedge, referenced mechanically by the registry.Review track: A (tests/CI infrastructure; production behavior unchanged)