fix(test-infra): repair the #6520 audit fallout — coverage-lane crash, blind auth suites, weakened guards - #6609
Conversation
…rflow under llvm-cov Since the #6520 merge commit, main's Coverage (default) and Coverage (all-features) jobs die in reborn_integration_extension_delivery: unbound_telegram_actor_pairs_via_web_minted_code_then_turns_attribute_to_ the_paired_user's ~400-line journey future overflows the 2 MiB test-thread stack once llvm-cov instrumentation inflates its frames (SIGABRT, 'has overflowed its stack'). Box the future via the _impl extraction the sibling telegram_update_becomes_a_turn_and_a_coordinated_reply already uses. Red first: cargo llvm-cov --no-report -p ironclaw_reborn_integration_tests --test reborn_integration_extension_delivery reproduced the exact CI crash locally; green after this change (18 passed instrumented and uninstrumented; the 2 postgres cases are filtered locally — no service). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGtnyQSPh8ouwypXioXWTc
…-tool-installs test_private_tool_installs_full_path has failed on main's E2E Coverage lane since #6520 merged: the install contract now requires the client gesture id (parse_client_action_id, product_surface_inbound.rs) and this scenario's _install helper still posted only package_ref, dying with {"field":"client_action_id","validation_code":"missing_field"}. Send one id per install gesture via the reborn_webui_harness helper — the same reconciliation #6603 applied to the extensions-api specs. Verified locally through the real serve binary: 1 passed (was the standing red on main). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGtnyQSPh8ouwypXioXWTc
…st]] registration The selector's find -maxdepth 1 walk could not see domain-folder bins, so the six tests/integration/auth/ suites (oauth_connect, oauth_popup_journeys, oauth_refresh, auth_gate, auth_failure, reopen_resume_through_gate) ran in NO PR or merge-queue lane — their only executor was the push-to-main coverage workflow, itself crashed since #6520. Discovery now selects every workspace [[test]] whose path sits under tests/integration/, so a suite cannot be registered without also being selected, whatever directory shape it uses; a registered-but-deleted file fails the lane loudly. Also drops the bash-4 mapfile so the guardrail runs on macOS dev machines (the old selector could not execute under /bin/bash 3.2 — all 12 section-D harness assertions failed locally at base), and de-stales the lane-runner's hardcoded 34/27/7 suite counts. Regression coverage: test-reborn-coverage.sh D6 pins domain-folder-bin selection and unregistered-sibling exclusion; D1-D5 pass unchanged. All six auth suites verified green locally (71 passed / 0 failed) before wiring them into the lanes; new selector output cross-checked name-by-name against Cargo.toml (56 suites, was 50). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGtnyQSPh8ouwypXioXWTc
…mber install reconciliation Two integration-tier weakenings from #6520: 1. scenario_remove_then_absent_cross_thread's phase-4 guard asserted the absence of installation_phase:setup_needed — but the same PR seeds a credential in phase 1, so the pre-remove state is active and a stale search projection would read active and pass the guard. Assert the wire contract instead: the installation_phase key is omitted entirely for a caller with no visible installation, so ANY surviving phase now fails. (Non-vacuity is anchored by scenario_install_then_active positively pinning the key for installed entries.) 2. The retired Activate action's structural successor — an existing member's idempotent install retry reconciling setup_needed -> active (extension_lifecycle.rs's Some(existing) same-caller arm) — had no integration coverage, and setup_needed was never positively observed at this tier. New scenario drives it on the shared store as a DISTINCT actor (with_actor_id): install parks the credential gate, deny leaves the membership at setup_needed (observed cross-thread), a seeded credential completes setup, and the same member's re-install reconciles to active (observed cross-thread) with no remove in between. The distinct actor kills scenario-order coupling both ways. Verified: cargo test --test reborn_group_extensions — 15 passed / 0 failed including both changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGtnyQSPh8ouwypXioXWTc
…docs AUTH_LIVE_FORCE_GOOGLE_REFRESH and the deliberate-expiry note documented the expire_secret_in_db flow #6520 removed (the canary no longer reaches into persistence); the flag is read nowhere. Point the scopes example at the full-URL form matching run_live_canary.py's GOOGLE_SCOPE_DEFAULT, and note that a product-side refresh-under-expiry proof remains a tracked follow-up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGtnyQSPh8ouwypXioXWTc
|
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. |
|
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 (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesReborn test infrastructure and lifecycle coverage
Estimated code review effort: 3 (Moderate) | ~20 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 |
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 1 | 0 | 1 | 2fe72a16c7e9 |
Head: 2fe72a16c7e928f6e6ccf82c63f5817037c7008c
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 registration-driven selector works for the current 56 targets, but its parser silently drops valid Cargo test registrations when path precedes name, undermining the PR’s coverage-selection guarantee.
Findings
Blocking: 1 / Notes: 0
Blocking findings
1. ❌ [MEDIUM] Make manifest discovery independent of key order
Location: scripts/ci/reborn-coverage-int-tier-tests.sh:48-50
Cargo TOML key order is not semantic, but this emits a target immediately when it sees path, only if name has already appeared. A valid [[test]] with path = "tests/integration/..." before name = "reborn_integration_..." is silently omitted (and can make discovery report no suites), so that suite receives no coverage-lane execution. Collect both fields for the whole block and emit at its boundary, then add a reversed-order fixture to the harness.
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.
| sub(/^name = "/, "", name) | ||
| sub(/"$/, "", name) | ||
| } | ||
| in_test && /^path = "tests\/integration\// && name != "" { |
There was a problem hiding this comment.
This assumes name precedes path, although Cargo treats TOML key order as irrelevant. A valid test block with path first is silently skipped, returning the coverage blind spot this selector is meant to prevent. Collect both fields per [[test]] block before deciding whether to emit it, and add a reversed-order fixture.
There was a problem hiding this comment.
Fixed in 4e2314f — the parser now buffers each [[test]] stanza and emits at the stanza boundary (next table header or EOF), so key order no longer matters. Harness case D6 gained a path-before-name stanza that failed against the old parser (verified red) and passes now; real-repo selector output is byte-identical (56 suites).
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 `@scripts/ci/reborn-coverage-int-tier-tests.sh`:
- Around line 40-52: The TOML registration scan in
scripts/ci/reborn-coverage-int-tier-tests.sh lines 40-52 depends on name
preceding path; buffer both fields within each [[test]] stanza and emit the name
when the stanza closes, regardless of key order. Add the requested D6 fixture
and selection assertion in scripts/ci/test-reborn-coverage.sh lines 933-937 to
cover path-before-name registrations.
In `@scripts/ci/test-reborn-coverage.sh`:
- Around line 933-937: Add a second valid [[test]] stanza to the D6 Cargo.toml
fixture with path before name, and update the corresponding assertions to verify
this entry is emitted. Keep the existing name-before-path fixture and assertions
intact.
🪄 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: a939cbab-6e82-441f-ac8e-178ebe4e10d6
📒 Files selected for processing (10)
scripts/auth_live_canary/config.example.envscripts/ci/reborn-coverage-int-tier-tests.shscripts/ci/reborn-coverage-lane-run.shscripts/ci/test-reborn-coverage.shscripts/live-canary/ACCOUNTS.mdtests/e2e/scenarios/test_reborn_private_tool_installs.pytests/integration/extension_delivery.rstests/integration/group_extensions/main.rstests/integration/group_extensions/scenario_existing_member_reinstall_reconciles_to_active.rstests/integration/group_extensions/scenario_remove_then_absent_cross_thread.rs
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 86.28% — 313353 / 363168 lines Per-crate breakdown (59 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)
|
|
🚅 Deployed to the ironclaw-pr-6609 environment in ironclaw-ci-preview
|
Review finding (ironloopai blocking + CodeRabbit major, PR #6609): the selector emitted a name only when it appeared BEFORE the stanza's path line, so a valid Cargo registration written path-first would be silently skipped — recreating the blind-spot class this selector exists to prevent. Cargo treats TOML key order as irrelevant; the parser now buffers each [[test]] stanza and emits at the stanza boundary (next table header or EOF). Red first: harness case D6 extended with a path-before-name stanza failed against the old parser, passes now; all 14 section-D assertions green and the real-repo output is byte-identical (56 suites). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGtnyQSPh8ouwypXioXWTc
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 `@scripts/ci/reborn-coverage-int-tier-tests.sh`:
- Around line 56-61: Update the TOML parsing patterns in
scripts/ci/reborn-coverage-int-tier-tests.sh at lines 56-61 to allow optional
whitespace around the name/path keys and equals sign, while preserving
quoted-value extraction. Add a compact no-space Cargo.toml assignment to the D6
fixture in scripts/ci/test-reborn-coverage.sh at lines 927-945 to verify the
parser includes such registrations.
🪄 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: b14b23d9-c338-484f-8e15-e05b2e106393
📒 Files selected for processing (2)
scripts/ci/reborn-coverage-int-tier-tests.shscripts/ci/test-reborn-coverage.sh
…regexes Second formatting-dependence finding on the selector (CodeRabbit, PR #6609): the awk accepted only 'key = "value"' spacing, so a legal compact 'name="..."' stanza would be silently dropped. Rather than harden the regex one format at a time, parse Cargo.toml with Python's stdlib tomllib — the lane already hard-depends on python3 (scripts/ci/lib/reborn_coverage_lcov.py) and ubuntu-latest + macOS both ship >=3.11 — so the selector accepts exactly what Cargo accepts, closing the whole class (key order, spacing, comments). Red first: D6 extended with a compact trailing-comment stanza failed against the regex parser, passes now; all 14 section-D assertions green; real-repo output byte-identical (56 suites); shellcheck clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGtnyQSPh8ouwypXioXWTc
…ne (#6660) * fix(coverage): declare RUST_MIN_STACK on the push-to-main coverage lane `Code Coverage` has been red on every push to main for 20+ consecutive commits, aborting with `has overflowed its stack` / `fatal runtime error: stack overflow` (SIGABRT, exit 101) — on a *different* test each time as unrelated PRs shifted which future sat deepest: 096e8f8 unbound_telegram_actor_pairs_via_web_minted_code_… (extension_delivery) 8f4d832 duplicate_and_restart_replay_converge_exactly_once::case_1 (extension_ingress) d06bde9 extension_install_survives_independent_reopen (durable) Root cause is a workflow gap, not test depth. libtest gives each test thread a 2 MiB stack. `reborn-tests.yml` splits this package's suites across two jobs and gives each the headroom it needs — `reborn-integration-coverage` carries 8 MiB (llvm-cov inflates the integration harness's async frames; #6609) and `root-reborn-parity-tests` carries 64 MiB (reborn_qa_smoke_scenarios_e2e drives whole turns on the libtest stack, ~10 MiB uninstrumented). `coverage.yml` runs `cargo llvm-cov --workspace`, i.e. BOTH tiers in one job, and declared neither. Set it to the union's requirement, 64 MiB. This also explains why the per-test fixes did not converge: the depth lives in shared harness code (group build -> submit_turn -> composition), so #6609's `Box::pin` lowered one test below the ceiling and the next-deepest test simply became the new failure. The controlled comparison at d06bde9: `Reborn integration coverage (1)` ran reborn_integration_durable instrumented with RUST_MIN_STACK=8388608 and passed, while `Coverage (all-features)`/`Coverage (default)` ran the same suite under the same instrumentation with no setting and SIGABRT'd. Same code, same instrumentation — only the stack size differed. Regression coverage: tests/coverage_lane_stack_headroom.rs pins the invariant on both workflows, sized per tier (whole-workspace lanes need 64 MiB; integration-tier-only lanes need 8 MiB). Verified red before this change (`coverage.yml:coverage … declares no job-level RUST_MIN_STACK`) and green after. Mutation-tested three ways: a below-floor value, a whole-workspace lane set to the integration tier's 8 MiB, and dropping reborn-tests.yml's own value each fail the guard. Non-vacuity assertions keep a renamed job or reworded `run:` line from silently emptying the scan. Note: coverage.yml triggers only on `push: branches: [main]`, so this PR's own CI cannot exercise the fixed lane — it is validated by the guard test plus the CI evidence above, and proven by the next push to main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(ci): route the coverage-headroom guard into the Reborn root test lanes The guard added in the previous commit never ran: `root-reborn-parity-tests` gates on `has_reborn_tests`, and `classify-test-scope.sh`'s `is_reborn_test_path` matches root suites by the `tests/reborn_*` prefix. `tests/coverage_lane_stack_headroom.rs` did not match, so a PR touching only it and a workflow classified as `has_reborn_tests=false` and skipped every Reborn test lane — the guard was dead weight on exactly the PR shape it exists to police (a workflow edit). Caught on PR CI for this branch: `Reborn root tests` reported `skipping`. Rename to `tests/reborn_coverage_lane_stack_headroom.rs`, matching the convention every other root suite already uses. Verified with the real staged file set: the classifier now reports `has_reborn_tests=true`, and the suite passes under its new target name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Summary
reborn_integration_extension_deliverySIGABRTs with a stack overflow under llvm-cov instrumentation since the fix(reborn): make extension readiness and channel delivery generic #6520 merge commit, killingCoverage (default)andCoverage (all-features)on every main push. The 400-line pairing-attribution journey's future overflows the 2 MiB test-thread stack when instrumentation inflates its frames — boxed via the sameBox::pin+_implpattern its siblingtelegram_update_becomes_a_turn_and_a_coordinated_replyalready uses.test_private_tool_installs_full_pathfails with{"field":"client_action_id","validation_code":"missing_field"}— the fix(reborn): make extension readiness and channel delivery generic #6520 install contract requires the client gesture id and this scenario was never reconciled. Now sendsclient_action_idper the SPA contract (extensions-api.ts), same as the test(playwright): reconcile suite to the merged #6520 lifecycle and setup contracts #6603-reconciled specs.scripts/ci/reborn-coverage-int-tier-tests.shdiscovered suites withfind -maxdepth 1, which cannot see domain-folder bins — so all sixtests/integration/auth/suites (oauth_connect,oauth_popup_journeys,oauth_refresh,auth_gate,auth_failure,reopen_resume_through_gate) ran only in the push-to-main coverage workflow (itself crashed by the bug above; four of the six ran nowhere at all post-merge). Discovery is now registration-driven — every workspace[[test]]whosepathsits undertests/integration/is selected — so a suite cannot be registered without also being selected, in any directory shape. Also made the selector bash-3.2-portable (mapfile→ plain string), so the guardrail runs on macOS dev machines; that fixes 12 previously-failing section-D harness assertions locally.scenario_remove_then_absent_cross_thread's phase-4 guard checked absence of"installation_phase":"setup_needed"while fix(reborn): make extension readiness and channel delivery generic #6520's edit made the pre-remove stateactive— a stale search projection would readactiveand pass. The guard now asserts the wire contract directly: theinstallation_phasekey is omitted entirely for a caller with no visible installation. And the retired Activate action's structural successor — an existing member's idempotent install retry reconcilingsetup_needed → active— had zero integration-tier coverage (setup_neededwas never positively observed at that tier); a new scenario drives it on the shared store as a distinct actor.AUTH_LIVE_FORCE_GOOGLE_REFRESHand the deliberate-expiry note documented a flow fix(reborn): make extension readiness and channel delivery generic #6520 deleted (expire_secret_in_db); the scopes example drifted fromGOOGLE_SCOPE_DEFAULT.Change Type
Linked Issue
Related: post-merge audit of #6520; follow-ups #6602/#6603 fixed the canary wire shape and the Playwright shards, this PR covers the remaining regressions. No pre-existing issue tracks them.
Validation
cargo fmt --all -- --checkcargo clippy -p ironclaw_reborn_integration_tests --tests -- -D warnings(default) and--all-features— the only crate with Rust changes; both lanes cleancargo build -p ironclawcargo test --features integration— not applicable: no database-backed behavior changed (test/CI/docs-only diff)review-pr/pr-shepherd --fix— not run (agent session; full command evidence inline)Test Strategy
User behavior: none changed — this PR touches tests, CI selection, and docs only. The production crates are untouched.
Risk areas:
Tests added or updated:
test-reborn-coverage.shsection D — new case D6 pins domain-folder-bin selection and unregistered-sibling exclusion; D1–D5 pass unchanged against the rewritten selector.scenario_existing_member_reinstall_reconciles_to_active(new, group_extensions, distinct actor viawith_actor_id; positively pins cross-threadsetup_needed, then the same-member idempotent-install reconciliation toactivewith no remove);scenario_remove_then_absent_cross_threadguard re-armed to the key-absence wire contract;extension_delivery.rsoverflowing journey boxed (_implextraction, in-file precedent).test_reborn_private_tool_installs.pyreconciled to the fix(reborn): make extension readiness and channel delivery generic #6520 install gesture contract.What the tests prove:
reborn_integration_extension_deliveryunder llvm-cov again (the exact previously-crashing invocation passes locally).installation_phasefails it).active) is pinned at the integration tier, cross-thread, on the shared store.Commands run (local, all exit 0 unless noted):
cargo llvm-cov --no-report -p ironclaw_reborn_integration_tests --test reborn_integration_extension_delivery— red first (reproduced main's crash:thread 'unbound_telegram_actor_pairs_via_web_minted_code_then_turns_attribute_to_the_paired_user::case_1_libsql' has overflowed its stack, SIGABRT), then green after the fix (-- --skip case_2_postgreslocally: no Postgres service; CI provides one — 18 passed / 0 failed).cargo test --test reborn_integration_extension_delivery -- --skip case_2_postgres— green uninstrumented.cargo test --test reborn_group_extensions— 15 passed / 0 failed, including the new scenario and the re-armed guard.cargo test --test reborn_integration_oauth_connect --test reborn_integration_oauth_popup_journeys --test reborn_integration_oauth_refresh --test reborn_integration_auth_gate --test reborn_integration_auth_failure --test reborn_integration_reopen_resume_through_gate— 71 passed / 0 failed (proven green before wiring into CI lanes).tests/e2e: pytest scenarios/test_reborn_private_tool_installs.py— red on main (the standing E2E Coverage failure), green with the fix through the realironclaw servebinary.bash scripts/ci/test-reborn-coverage.sh— all 14 section-D assertions pass (D1–D6); the 12 remaining failures are the pre-existing section-C fake-ghcases, identical on the pristine tree (which additionally fails all 12 D assertions there, because the oldmapfileselector cannot run under macOS bash 3.2 at all).bash scripts/ci/reborn-coverage-int-tier-tests.sh— emits 56 suites (was 50), including the six auth suites; every emitted name cross-checked againstCargo.toml.shellcheckon both edited scripts — clean (the two remaining findings in the harness are pre-existing and identical at base).bash scripts/pre-commit-safety.sh— pass.Security Impact
None. No production code changed. The auth suites newly running in PR CI only increase enforcement of existing OAuth/gate behavior.
Reborn Trust-Boundary Checklist
N/A — no Reborn production/runtime/DB code changed (tests, CI scripts, and docs only).
Database Impact
None.
Blast Radius
CI coverage-lane selection (
reborn-coverage-int-tier-tests.shconsumers:reborn-coverage-lane-run.sh, its 5-lane matrix inreborn-tests.yml), theCoverage (default)/Coverage (all-features)/E2E Coveragejobs on main, thereborn_group_extensionsandreborn_integration_extension_deliverysuites, and the auth-canary docs. Lane-shape note: the six auth suites join the four flat modulo-partitions (~1.5 suites/lane more); they run 0.01–4.2 s each uninstrumented locally, so lane duration impact is small.Rollback Plan
Revert the PR; every change is test/CI/docs-scoped and independently revertible per commit. Reverting the selector commit alone returns the auth suites to their pre-PR blind spot (and re-breaks local D-section harness runs on macOS); reverting the extension_delivery commit re-crashes the main coverage lanes.
Review Follow-Through
Known follow-ups deliberately NOT in this PR (from the same audit): a restoration-tracking issue for the 15 quarantined live-canary replay fixtures; a product-side Google refresh-under-expiry canary proof (the harness-side deletion is now documented in ACCOUNTS.md); the
ironclaw_productreadiness-derivation duplication cluster; a PR/merge-queue trigger for the nightly-only Playwright suite; and a CI lane for the live-QA/canary Python unit suites plus a Python lint pass (ruffF811 would have caught #6520's duplicate Playwright helper). The live-QA harness unit-suite repair (test_run_live_qa.py, red on main) is in flight separately with the operator's local changes and is intentionally untouched here.Review track: C (CI/Infrastructure)
🤖 Generated with Claude Code
https://claude.ai/code/session_01YGtnyQSPh8ouwypXioXWTc