test(reborn): integration-suite restructure — tests/integration/ home, framework disentanglement, single-run coverage - #5633
Conversation
…keleton + renames) Commit 1/8 of the integration-suite restructure. Byte-pure moves: tests/support/reborn/ -> tests/integration/support/, 27 reborn_integration_*.rs bins -> tests/integration/<name>.rs, 7 reborn_group_* dirs -> tests/integration/group_<name>/. Cargo [[test]] names stay identical; only paths move (27 entries added, 7 retargeted). Only content edits: the #[path] mount lines each move forces (flat bins, group mains, 33 parity/QA bins) — wiring folded into this commit so every commit builds green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…embly} + doubles/, extract binary-E2E family to tests/support/reborn_parity_qa/ Commit 2/8. Symbol-preserving split of the 5530-line harness.rs at its final home: recorder.rs (capability recorder), options.rs (HostRuntimeHarnessOptions, verbatim), assembly.rs (local_dev_* runtime/filesystem/policy/mount helpers), doubles/ (14 files, one per substituted production port, rustdoc header names the production seam), residual core in harness/mod.rs with pub(crate) use re-exports preserving every surviving reborn_support::harness::<Name> path. RebornBinaryE2EHarness + SubmittedTurn + RebornHarnessSharedStorage + HarnessLoopExitEvidencePort + assert_milestone_order + trace_tool_call_response and model_replay.rs leave to tests/support/reborn_parity_qa/ (their consumers are exclusively parity/QA); the 33 parity/QA bins mount the new parity_qa_support tree and repoint only those imports. Only content changes beyond the moves: use/mod lines, visibility bumps the new module boundaries force, and per-file dead_code allows replacing the old blanket allow. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ile + harness/profiles/ domains Commit 3/8 — the design commit. HostRuntimeHarnessOptions gains Default; new ToolsProfile value type (options.rs) captures the shared new_with_options seven-arg shape plus the four observed post-construct steps (network-policy override, provider-trust override, asset copy, auto-approve default), applied in the constructors' existing order by the one shared ToolsProfile::build() path. Every tools constructor leaves harness/mod.rs for a per-domain profiles/<domain>.rs file (16 domains): ToolsProfile rows become <name>_profile() + a thin build wrapper; bespoke constructors (qa_smoke, core_builtin, github issue trio, mock_mcp, web_access) move verbatim. The 4-deep core_builtin suffix-variant chain folds into one CoreBuiltinOptions; the four variants are deleted. project_tools_with_fault_injection (new since the plan's table) rides in profiles/project.rs. Thick group pairs (live_approvals, live_auth_and_approval, profile_tools, outbound_target_tools) adopt ToolsProfile::build_group_capability_with_base over GroupBaseData with auto-approve disables kept explicit at call sites; capability_backend::install() now selects via the same profile fns, deleting its duplicate constructor-selection table. The six group-builder runtime setters move to group_options.rs (private child module of group.rs, group_constructors precedent). Constructor doc comments carried verbatim. Flip-checked: falsifying the trigger profile's capability set turns reborn_group_triggers red for the right reason. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…support to reborn_parity_qa/, drop dead approval.rs Commit 4/8. git-mv qa_trace.rs, qa_scenarios.rs, delivery.rs, network.rs from tests/integration/support/ to tests/support/reborn_parity_qa/ (their consumers are exclusively parity/QA bins) and rewrite those consumers' imports (qa_recorded_behavior, qa_smoke_scenarios_e2e, qa_doc_grounding, qa_web_fetch, qa_channel_delivery, outbound_reply_target parity, support_unit_tests). Delete approval.rs — zero consumers anywhere (pub use GateRef + an unused alias). classify-test-scope.sh's reborn path arm now matches tests/integration/* and tests/support/reborn_parity_qa/* instead of the removed tests/support/reborn/*; fixture updated. New tests/support/reborn_parity_qa/CLAUDE.md documents the tier split and the one-way import direction (parity/QA imports FROM tests/integration/support/, never the reverse — direction grep clean). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ry guard Commit 6/8. Kills the run-twice shape: the 34 tests/integration/ suites now run ONCE, instrumented, in a 5-lane reborn-integration-coverage matrix (4 modulo partitions of the 27 flat bins + 1 group lane), features unified to libsql. Each lane produces its lcov from one combined 'cargo llvm-cov --workspace --features libsql test --test ...' invocation — the split --no-report + 'report --lcov' shape silently drops all crates/ironclaw_* files because the standalone report subcommand has no --workspace flag (verified empirically). New coverage-report job merges lane lcovs (checked-in Python merger; sums DA per file:line, recomputes LF/LH, filters to crates/ironclaw_*), applies tests/integration/ coverage-exemptions.toml (seeded empty; every entry needs reason + issue link), and renders a per-crate table to the job summary + a sticky PR comment. Informational only — pass/fail gating rides the instrumented lanes; coverage-report failures warn, never red, the roll-up. reborn-coverage.yml (the duplicate compile+run) is deleted. Parity/QA root-partition lanes stay uninstrumented — the harness-only coverage boundary is enforced by job topology. Instrumented lanes use a dedicated reborn-integration-cov cache key. Discovery scripts retarget to tests/integration/ (group and int-tier enumeration; root partitions now match only the parity/QA bins by construction). New scripts/ci/check-test-suite-boundaries.sh (invoked next to classify-test-scope.sh) enforces the one-way support-tree direction, the parity/QA mount rule, and that tests/support/reborn/ never reappears. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Commit 7/8. tests/integration/support/CLAUDE.md moves to tests/integration/CLAUDE.md with surgical fixes for the new reality (harness/ split + profiles/ + doubles/ file map, new mount-line boilerplate for flat bins and group mains, [[test]] naming convention, binary-E2E family pointer to tests/support/reborn_parity_qa/CLAUDE.md). Root CLAUDE.md spec table + testing pointers, the ironclaw-reborn-testing and reborn-feature skills, and the two living docs/reborn/ pages repoint the same way. Historical dated plans/specs under docs/superpowers/ deliberately untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Commit 8/8. Comment-only pass over all 126 tests/integration/ files (zero code changes — verified by non-comment diff-line scan). Deleted: narration/play-by-play, migration/wave/lane provenance, restated signatures, duplicated profile/wrapper doc blocks, history essays. Kept, compressed to 1-2 lines: why-pins, mutation-verified notes, doubles seam contracts, product decisions — invariant + issue/PR ref; every C-*/E-*/T0-* seam id and DEFERRED/COVERED pointer retained (regex-checked per lane). ~1,800 comment lines removed net. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
github-actions
Bot
added
scope: ci
|
Important Review skippedToo many files! This PR contains 200 files, which is 50 over the limit of 150. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (7)
📒 Files selected for processing (200)
You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request restructures the test suite by moving roadmap integration tests to tests/integration/ and parity/QA tests to tests/support/reborn_parity_qa/, updating configuration files and CI scripts accordingly. It also introduces new CI scripts for test suite boundary checks and LCOV coverage merging and reporting. Feedback on the changes highlights opportunities to make the CI scripts more robust, such as updating regex patterns to support relative paths, validating that exempted module paths are repo-relative, and ensuring bash pipelines do not fail under set -eo pipefail when grep finds no matches.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| # a superset of the historical Reborn-family-only allowlist (the int-tier | ||
| # suites now exercise the whole workspace closure, not just the Reborn crate | ||
| # families). | ||
| crate_re = re.compile(r"/crates/(ironclaw_[A-Za-z0-9_]+)/") |
There was a problem hiding this comment.
The regex r"/crates/(ironclaw_[A-Za-z0-9_]+)/" expects a leading slash before crates/. If cargo llvm-cov produces relative paths (e.g., starting with crates/...), this regex will fail to match, silently dropping coverage for all files. Consider using (?:^|/) to support both absolute and relative paths.
| crate_re = re.compile(r"/crates/(ironclaw_[A-Za-z0-9_]+)/") | |
| crate_re = re.compile(r"(?:^|/)crates/(ironclaw_[A-Za-z0-9_]+)/") |
There was a problem hiding this comment.
Fixed in ff6dc6b — (?:^|/)crates/ in both lcov scripts.
|
|
||
| mode, lcov_path, exemptions_path = sys.argv[1], sys.argv[2], sys.argv[3] | ||
|
|
||
| crate_re = re.compile(r"/crates/(ironclaw_[A-Za-z0-9_]+)/") |
There was a problem hiding this comment.
The regex r"/crates/(ironclaw_[A-Za-z0-9_]+)/" expects a leading slash before crates/. If cargo llvm-cov produces relative paths (e.g., starting with crates/...), this regex will fail to match, silently dropping coverage for all files. Consider using (?:^|/) to support both absolute and relative paths.
| crate_re = re.compile(r"/crates/(ironclaw_[A-Za-z0-9_]+)/") | |
| crate_re = re.compile(r"(?:^|/)crates/(ironclaw_[A-Za-z0-9_]+)/") |
There was a problem hiding this comment.
Fixed in ff6dc6b — same (?:^|/) hardening applied here.
| module = entry.get("module") | ||
| if not module: | ||
| print(f"malformed exemption entry (missing 'module'): {entry}", file=sys.stderr) | ||
| sys.exit(1) |
There was a problem hiding this comment.
In the exemptions parser, there is no validation that the module path is actually repo-relative and starts with crates/. If a user accidentally specifies a generic suffix (like src/a.rs or a.rs), it could match and exempt files across multiple different crates, leading to silent over-exemption. Adding a validation check ensures that exemptions are precise and robust.
| module = entry.get("module") | |
| if not module: | |
| print(f"malformed exemption entry (missing 'module'): {entry}", file=sys.stderr) | |
| sys.exit(1) | |
| module = entry.get("module") | |
| if not module: | |
| print(f"malformed exemption entry (missing 'module'): {entry}", file=sys.stderr) | |
| sys.exit(1) | |
| if not module.startswith("crates/"): | |
| print(f"exemption module path '{module}' must be repo-relative and start with 'crates/'", file=sys.stderr) | |
| sys.exit(1) |
There was a problem hiding this comment.
Fixed in ff6dc6b — validation added (module must start with crates/), plus regression case A9 in test-reborn-coverage.sh pinning the rejection. The suffix-match over-exemption risk was real.
| mapfile -t direction_hits < <( | ||
| grep -rl -E 'parity_qa_support|reborn_parity_qa' tests/integration/ 2>/dev/null | LC_ALL=C sort | ||
| ) |
There was a problem hiding this comment.
Under set -eo pipefail, if grep finds no matches, it exits with status 1, which will cause the pipeline grep | sort to exit with 1. In some bash environments or configurations, this can cause the script to terminate unexpectedly. Wrapping grep or appending || true ensures the pipeline exits with 0 when no matches are found.
| mapfile -t direction_hits < <( | |
| grep -rl -E 'parity_qa_support|reborn_parity_qa' tests/integration/ 2>/dev/null | LC_ALL=C sort | |
| ) | |
| mapfile -t direction_hits < <( | |
| (grep -rl -E 'parity_qa_support|reborn_parity_qa' tests/integration/ 2>/dev/null || true) | LC_ALL=C sort | |
| ) |
References
- A fragile implementation is acceptable for non-critical, advisory scripts where the failure mode is benign (e.g., false negatives leading to reduced test coverage, not a hard failure).
There was a problem hiding this comment.
Declined — the grep runs inside a <(...) process substitution feeding mapfile; bash discards the substitution's exit status, so a no-match grep cannot terminate the script (set -e/pipefail apply to the outer mapfile, which succeeds). The empty case is handled by the array-length check. Verified by running the guard on a tree with zero matches.
| mapfile -t mount_in_integration < <( | ||
| grep -rlE "${mount_pattern}" tests/integration/ 2>/dev/null | LC_ALL=C sort | ||
| ) |
There was a problem hiding this comment.
Under set -eo pipefail, if grep finds no matches, it exits with status 1, which will cause the pipeline grep | sort to exit with 1. In some bash environments or configurations, this can cause the script to terminate unexpectedly. Wrapping grep or appending || true ensures the pipeline exits with 0 when no matches are found.
| mapfile -t mount_in_integration < <( | |
| grep -rlE "${mount_pattern}" tests/integration/ 2>/dev/null | LC_ALL=C sort | |
| ) | |
| mapfile -t mount_in_integration < <( | |
| (grep -rlE "${mount_pattern}" tests/integration/ 2>/dev/null || true) | LC_ALL=C sort | |
| ) |
References
- A fragile implementation is acceptable for non-critical, advisory scripts where the failure mode is benign (e.g., false negatives leading to reduced test coverage, not a hard failure).
There was a problem hiding this comment.
Declined — same reasoning as the check-1 comment above: process substitution exit status is discarded by bash, so this pipeline cannot kill the script on no-match.
|
🚅 Deployed to the ironclaw-pr-5633 environment in ironclaw-ci-preview
|
Reborn integration-tier coverageLine coverage (Reborn crates): 26.31% — 45017 / 171123 lines Per-crate breakdown (62 crates, lowest-covered first)
This signal is informational: coverage never gates the PR — not the percentage, not the per-crate holes, not the 0-coverage callout. Exemptions (0 file(s) excluded from the accounting above)No exemptions configured. |
…ser_id, const dedup, stale-path sweep + guard Review follow-up (thermo-nuclear + code-review passes on #5633): - ToolsProfile::new now takes user_id explicitly — the service_label-seeded placeholder was a silently-valid wrong value if a profile ever forgot to override it; all 15 profile call sites pass their fixed domain user id. - TEST_CAPABILITY_ID/TEST_CAPABILITY_SURFACE_VERSION deduplicated: binary_e2e now imports the doubles-tree constants instead of carrying drift-prone copies. - Stale tests/support/reborn/ references swept from tests/** and Cargo.toml (snapshot source headers included); check-test-suite-boundaries.sh gains check #4 failing on any reappearance of the retired path under tests/. (Doc-comment stragglers inside crates/ are deliberately left for a separate docs-only PR — production files stay outside this PR's diff surface.) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f95657973e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| local path="$1" | ||
| case "$path" in | ||
| docs/reborn/*|scripts/reborn-e2e-rust.sh|scripts/ci/run-reborn-root-partition.sh|scripts/ci/run-reborn-group-tests.sh|tests/reborn_*|tests/support/reborn/*|tests/fixtures/llm_traces/reborn_qa/*|tests/e2e/scenarios/test_reborn_*) | ||
| docs/reborn/*|scripts/reborn-e2e-rust.sh|scripts/ci/run-reborn-root-partition.sh|scripts/ci/run-reborn-group-tests.sh|tests/reborn_*|tests/integration/*|tests/support/reborn_parity_qa/*|tests/fixtures/llm_traces/reborn_qa/*|tests/e2e/scenarios/test_reborn_*) |
There was a problem hiding this comment.
When a PR only changes the new coverage pipeline scripts such as scripts/ci/reborn-coverage-lane-run.sh, reborn-coverage-merge-lcov.sh, or reborn-coverage-summary.sh, this classifier does not mark it as Reborn-scoped; it falls through to the generic scripts/ci/* code path and emits has_reborn_tests=false. Since the old standalone coverage workflow was removed and reborn-tests.yml skips the coverage jobs when that output is false, future regressions in these scripts can fast-pass without running the Reborn coverage/test pipeline.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in ff6dc6b — is_reborn_test_path() now includes scripts/ci/reborn-coverage-*.sh, test-reborn-coverage.sh, check-test-suite-boundaries.sh, and test-classify-test-scope.sh, with 6 new classifier regression cases asserting has_reborn_tests=true for each.
|
Scoped MOVE-lane review complete. I did not read the whole PR diff. No actionable findings from this lane. Bugs, Conventions, and Local Patterns reviewers all returned Evidence checked:
One claim-hygiene note, not a blocker: |
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
L6 docs/comment lane review: found three stale documentation pointers after the tests/integration support guide moved. Mechanical checks passed: comment-only scan for f956579, suite-boundary guard, and src/crates cross-cutting check.
| Support tree for the **parity and QA suites** (`tests/reborn_*_parity.rs`, | ||
| `tests/reborn_qa_*.rs`, `tests/reborn_*_e2e.rs`). These suites are current and | ||
| maintained, but they are **not coverage-bearing** — the coverage program runs | ||
| exclusively over `tests/integration/` (see `tests/integration/support/CLAUDE.md`). |
There was a problem hiding this comment.
Medium — Parity QA guide points at a nonexistent integration support guide.
This line sends readers to tests/integration/support/CLAUDE.md, but the final tree has the integration-tier guide at tests/integration/CLAUDE.md and no support-local CLAUDE file. Root CLAUDE.md now points to tests/integration/CLAUDE.md, so this breaks the new documentation trail.
Fix: Change the reference to tests/integration/CLAUDE.md.
There was a problem hiding this comment.
Fixed in ff6dc6b — now points at tests/integration/CLAUDE.md.
| ## 2. The scripted-model harness seam | ||
|
|
||
| The in-process harness (`tests/support/reborn/`, spec in its `CLAUDE.md`) fakes exactly one thing: the vendor SDK at the bottom (`TraceLlm`). Everything else — product workflow, coordinator, scheduler, agent loop, the real `ironclaw_llm` retry/failover/circuit-breaker chain — executes for real, and assertions read *persisted state* (filesystem, thread history), never internals. | ||
| The in-process harness (`tests/integration/support/`, spec in its `CLAUDE.md`) fakes exactly one thing: the vendor SDK at the bottom (`TraceLlm`). Everything else — product workflow, coordinator, scheduler, agent loop, the real `ironclaw_llm` retry/failover/circuit-breaker chain — executes for real, and assertions read *persisted state* (filesystem, thread history), never internals. |
There was a problem hiding this comment.
Medium — Exemplar guide implies a missing support-local CLAUDE file.
The sentence says the tests/integration/support/ harness has its spec in its CLAUDE.md, which implies tests/integration/support/CLAUDE.md. After the restructure, the authoritative spec is tests/integration/CLAUDE.md; there is no support-local CLAUDE file.
Fix: Rewrite the parenthetical to point at tests/integration/CLAUDE.md while keeping tests/integration/support/ as the harness code location.
There was a problem hiding this comment.
Fixed in ff6dc6b — parenthetical now reads: code in tests/integration/support/, spec in tests/integration/CLAUDE.md.
| //! group's own subdir and fails to compile: | ||
| //! | ||
| //! ```rust,no_run | ||
| //! #[allow(dead_code)] #[path = "../support/reborn/mod.rs"] mod reborn_support; |
There was a problem hiding this comment.
Medium — Group boilerplate still mounts the retired support/reborn path.
The required group-main boilerplate tells authors to mount #[path = "../support/reborn/mod.rs"] mod reborn_support;, but the final tree has no tests/integration/support/reborn/mod.rs. Existing group mains mount reborn_support from ../support/mod.rs, so copying this documented boilerplate would fail to compile.
Fix: Change the sample line to #[allow(dead_code)] #[path = "../support/mod.rs"] mod reborn_support;.
There was a problem hiding this comment.
Fixed in ff6dc6b — boilerplate doc now matches group_approvals/main.rs exactly (../support/mod.rs + ../../support/mod.rs mounts).
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (L3 — ToolsProfile design)
Scoped review of PR #5633 lane: design commits 02b56bc67 + 4d7f54774 only, comparing the migrated ToolsProfile / harness/profiles/* shape against origin/main:tests/support/reborn/harness.rs.
Reviewers run: Bugs, Approach, Maintainability, Tests.
Cross-cutting guard checked: git diff --name-only origin/main...HEAD | grep -E '^(src|crates)/' returned empty.
Findings
-
Medium — Group-specific alignment leaks into the profile layer (
tests/integration/support/harness/options.rs:309, confidence 75)ToolsProfile::build_group_capability_with_basemakes the generic harness profile module depend ongroup::GroupBaseDatasolely to perform group constructor user alignment. That reverses the ownership boundary: profile construction now knows about group assembly, andGroupBaseData/canonical_subject_userhad to be widened topub(crate)for this helper.Suggested fix: move this helper into
tests/integration/support/group_constructors.rsas a private local helper. KeepToolsProfilelimited to profile construction, remove theGroupBaseDataimport fromharness/options.rs, and narrowGroupBaseDatapluscanonical_subject_userback to module-private. -
Medium — Live-shell
CoreBuiltinOptionspath is not covered (tests/integration/support/capability_backend.rs:83, confidence 75)The migration replaces the deleted live-shell constructor with
CoreBuiltinOptions::default().with_live_shell(), but the integration tests do not drive theShellMode::Livecaller path. A regression that routes live-shell requests throughcore_builtin_tools_default()would still leave the existing inert/scripted shell tests passing.Suggested fix: add
tests::integration::process_port::live_shell_uses_local_process_portcoveringRebornIntegrationHarness::test_default().with_builtin_http_tools().with_live_shell()dispatchingbuiltin.shellwithout the recording process port.
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (L5: CI workflows + scripts)
Posted the one surviving scoped finding from the L5 pass. Security and Performance returned no findings; local checks were green (test-reborn-coverage.sh, test-classify-test-scope.sh, check-test-suite-boundaries.sh, actionlint).
| local path="$1" | ||
| case "$path" in | ||
| docs/reborn/*|scripts/reborn-e2e-rust.sh|scripts/ci/run-reborn-root-partition.sh|scripts/ci/run-reborn-group-tests.sh|tests/reborn_*|tests/support/reborn/*|tests/fixtures/llm_traces/reborn_qa/*|tests/e2e/scenarios/test_reborn_*) | ||
| docs/reborn/*|scripts/reborn-e2e-rust.sh|scripts/ci/run-reborn-root-partition.sh|scripts/ci/run-reborn-group-tests.sh|tests/reborn_*|tests/integration/*|tests/support/reborn_parity_qa/*|tests/fixtures/llm_traces/reborn_qa/*|tests/e2e/scenarios/test_reborn_*) |
There was a problem hiding this comment.
Medium — New Reborn coverage helpers are not classified as Reborn test scope.
This classifier still only treats the older Reborn CI scripts (run-reborn-root-partition.sh, run-reborn-group-tests.sh) as Reborn scope. The new coverage path now depends on scripts/ci/reborn-coverage-*.sh, scripts/ci/test-reborn-coverage.sh, and scripts/ci/check-test-suite-boundaries.sh, but a future PR that touches only those helpers currently classifies as has_reborn_tests=false. The roll-up then exits through the fast-pass path in reborn-tests.yml, so the coverage helper regression suite would not run for changes to the helpers themselves.
I verified this directly with the classifier on the current PR head: those helper paths produce docs_only=false, has_core_code=true, has_legacy_tests=true, has_reborn_tests=false.
Fix: Add the new coverage/boundary helper paths to the Reborn/shared classifier list and add classifier regression cases for them.
There was a problem hiding this comment.
Fixed in ff6dc6b (same fix as the codex P2 thread) — classifier arm + 6 regression cases added.
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (scoped lane)
Reviewed one lane of PR #5633: commit 47083d383 / current head 4d7f54774, scoped to tests/integration/support/harness, tests/integration/support/doubles, and tests/support/reborn_parity_qa.
Stats: 4 findings across 3 files. Reviewers run: Bugs, Maintainability, Local Patterns, Conventions. Bugs returned clean. Cross-cutting guard passed: no src/ or crates/ files changed in origin/main...HEAD.
I also spot-checked the requested focus areas: TEST_CAPABILITY_* dedupe is complete in this lane, the two near-name capability-port factories are distinct rather than mis-merged, and the sampled doubles/ production-substitute headers mostly line up with the real production traits/types aside from the breadcrumb issues called out inline.
| @@ -0,0 +1,1223 @@ | |||
| //! Reborn binary-E2E harness. | |||
There was a problem hiding this comment.
Medium — harness/mod.rs still documents the old binary-E2E harness.
The split module header still says this is the Reborn binary-E2E harness with scripted model-gateway substitutions, but that contract now lives in tests/support/reborn_parity_qa/binary_e2e.rs. tests/integration/CLAUDE.md describes the integration tier as distinct from RebornBinaryE2EHarness, so this copied header is stale after the refactor.
Fix: Replace the copied binary-E2E module header and stale arch-exempt note with a HostRuntimeCapabilityHarness-specific module description.
There was a problem hiding this comment.
Fixed in ff6dc6b — header rewritten to describe HostRuntimeCapabilityHarness (integration-tier, real host-runtime wiring over recorded doubles); stale binary-E2E/arch-exempt notes removed.
| } | ||
|
|
||
| pub(crate) struct HostRuntimeCapabilityHarness { | ||
| pub(crate) runtime: Arc<dyn HostRuntime>, |
There was a problem hiding this comment.
Medium — Factory split exposes most harness internals.
Moving HostRuntimeHarnessCapabilityPortFactory into the sibling doubles module forces HostRuntimeCapabilityHarness to publish runtime, IO, mounts, grants, trust, policy, secrets, and recording buffers as crate-visible state. The cross-module edge is self-created by the split: old harness.rs kept the factory and these fields private in one module, while the new sibling factory reads the raw fields directly.
Fix: Keep the authority/grant/port assembly owned by HostRuntimeCapabilityHarness. Either move HostRuntimeHarnessCapabilityPortFactory back into the harness module, or make the doubles factory call a harness-owned method so these fields can return to private.
There was a problem hiding this comment.
Fixed in ff6dc6b via the harness-owned-method variant — port/grant/authority assembly moved into HostRuntimeCapabilityHarness::create_recording_capability_port (with capability_grants + error helper), the doubles factory is now a thin trait adapter, and all 13 exposed fields plus dispatch_user_for_run/apply_synthetic_capability_wrappers narrowed back from pub(crate) to module-private.
| @@ -0,0 +1,146 @@ | |||
| /// Test double substituting the production `LoopCapabilityPortFactory` wiring | |||
There was a problem hiding this comment.
Low — Factory header points one symbol at the wrong file.
The header names both LocalDevLoopCapabilityPortFactory and HostRuntimeLoopCapabilityPortFactory but cites only crates/ironclaw_reborn_composition/src/runtime/local_dev.rs. HostRuntimeLoopCapabilityPortFactory is defined in crates/ironclaw_loop_support/src/capability_port.rs, so the split-file breadcrumb sends readers to the wrong module for half of the substituted wiring.
Fix: Name both production locations in the header: local_dev.rs for LocalDevLoopCapabilityPortFactory and crates/ironclaw_loop_support/src/capability_port.rs for HostRuntimeLoopCapabilityPortFactory.
There was a problem hiding this comment.
Fixed in ff6dc6b — each factory symbol now cites its actual file (local_dev.rs / ironclaw_loop_support/src/capability_port.rs).
| @@ -0,0 +1,74 @@ | |||
| /// Test double substituting the production `NetworkHttpEgress` impl | |||
There was a problem hiding this comment.
Low — Network egress header omits the transport location.
The header says this double substitutes PolicyNetworkHttpEgress over ReqwestNetworkTransport, but cites only crates/ironclaw_network/src/egress.rs. PolicyNetworkHttpEgress lives there, while ReqwestNetworkTransport lives in crates/ironclaw_network/src/transport.rs, so the production-impl breadcrumb is incomplete.
Fix: Either include crates/ironclaw_network/src/transport.rs next to ReqwestNetworkTransport, or narrow the header to cite only PolicyNetworkHttpEgress.
There was a problem hiding this comment.
Fixed in ff6dc6b — crates/ironclaw_network/src/transport.rs cited next to ReqwestNetworkTransport.
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
L4 — Parity/QA bin imports
Scoped review of the parity/QA bin import repoints after the binary-E2E family moved to tests/support/reborn_parity_qa/.
Findings: 2 low/nit items. Bugs reviewer returned clean; deterministic checks for moved-symbol imports, mount boilerplate, integration direction, and src//crates/ changed files were otherwise clean.
| #[allow(dead_code)] | ||
| #[path = "support/reborn_parity_qa/mod.rs"] | ||
| mod parity_qa_support; | ||
| #[path = "integration/support/mod.rs"] |
There was a problem hiding this comment.
Low — Add dead_code allow to the reborn_support mount.
reborn_support is remounted to tests/integration/support/mod.rs without the #[allow(dead_code)] used by the documented two-tree parity/QA consumer shape. Every other live lane consumer carries the allow before both support mounts, which keeps shared support helpers from surfacing as dead-code warnings under warning-deny checks.
Fix: Add #[allow(dead_code)] immediately before this integration/support/mod.rs mount.
There was a problem hiding this comment.
Fixed in ff6dc6b — #[allow(dead_code)] added, matching the other 9 root parity bins.
| #[allow(dead_code)] | ||
| #[path = "integration/support/mod.rs"] | ||
| mod reborn_support; | ||
| // Required by reborn_support::model_replay through crate::support::trace_llm. |
There was a problem hiding this comment.
Nit — Stale support-module path in comment.
The comment still says mod support is required by reborn_support::model_replay, but this file now imports model_replay from parity_qa_support. That points readers at the wrong support tree when tracing why the top-level support module is mounted.
Fix: Update the comment to reference parity_qa_support::model_replay, or delete it if the dependency is now obvious from the imports.
There was a problem hiding this comment.
Fixed in ff6dc6b — comment now references parity_qa_support::model_replay.
…epoints, harness re-encapsulation CI scripts: lcov crate regex accepts relative paths ((?:^|/)crates/) in merge+summary; exemption module paths must start with crates/ (suffix match could over-exempt; +A9 regression case); classify-test-scope now marks the coverage/boundary helper scripts as Reborn scope so helper-only PRs can't fast-pass the pipeline they drive (+6 regression cases). Docs/comments: stale pointers to the removed tests/integration/support/ CLAUDE.md now target tests/integration/CLAUDE.md; group-main boilerplate doc shows the real ../support/mod.rs mount; harness/mod.rs header rewritten for HostRuntimeCapabilityHarness (binary-E2E contract moved); doubles headers cite each substituted symbol's actual production file; parity-bin comment and missing #[allow(dead_code)] mount attr fixed. Structure (review-accepted): build_group_capability_with_base moved from ToolsProfile into group_constructors.rs — profile layer no longer imports group::GroupBaseData, and GroupBaseData/canonical_subject_user return to module-private. Capability-port assembly moved into harness-owned create_recording_capability_port; HostRuntimeHarnessCapabilityPortFactory is a thin trait adapter and 13 harness fields + 2 methods narrowed from pub(crate) to module-private. Coverage: live_shell_uses_local_process_port pins the ShellMode::Live caller path — real LocalHostProcessPort output surfaces in the tool result while the inert recording port stays untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review-response commit
|
… + #5389/#5390/#5403/#5613) (#5692) * reborn: add failure explanations and retryable failed runs * test(loop_support): set inline_messages on the contract-test LoopModelRequest #4841 added the `inline_messages` field (serde default) to LoopModelRequest but missed this one construction in the thread_loop_support_contract integration test, breaking that test target's compile. Production builds default it to Vec::new(); match that. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(reborn): resolve main-merge CI breaks on #4841 (retry_turn stub + finer failure categories) The main→#4841 merge surfaced two semantic conflicts the auto-merge missed: - Clippy: main added `TurnCoordinator::retry_turn`; the StaticTurnCoordinator test stub in openai_compat_serve/tests.rs didn't implement it. Add the stub (returns Unavailable, matching its other methods). - Test ironclaw_reborn: main's chaos tests (#5296) assert the coarse failure categories "driver_unavailable"/"model_error", but #4841 refined production to finer, accurate categories — a full checkpoint-state disk now yields "host_stage_unavailable_checkpoint" and an offline model provider yields "model_unavailable" (both deliberate named categories with dedicated failure_summary messages). Update the two stale assertions to match #4841's intended categorization. Verified: the two turn_runner_worker_full_reborn_fails_* tests pass; clippy ironclaw_reborn_composition --all-features clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Persist retry busy idempotency records * test(reborn): expect specific model unavailable failure * fix(reborn): preserve same-run checkpoint refs * fix(ci): update reborn compile drift * fix(ci): update composition test drift * feat(reborn): make model-fixable capability failures recoverable (batch 1) Turns recoverable→bork mis-mappings into model-visible tool errors so the agent self-corrects instead of the run dying. On top of #4841. - agent_loop keystone: capability_error_class re-buckets Dispatcher / InvalidOutput / Unknown(_) / non-exhaustive default from Permanent (Abort) to OperationFailed (ToolErrorResult), aligning with the host_runtime disposition layer (which never intends a capability failure to abort). Cancelled / Permanent stay terminal. This makes "model called a nonexistent tool" (UnknownCapability/UnknownProvider -> InvalidOutput) recoverable. - outbound_delivery: outbound_delivery_outcome routes recoverable RebornServicesErrorCode to Ok(Failed/Denied) (only Internal -> Err); fixed the safe_summary that interpolated the model-supplied target_id (Invariant 2); expired/not-yet-approved approval-lease arms -> Ok(Denied) instead of terminal. - host_runtime: malformed model-supplied SandboxProcessPlan -> recoverable Failed{InvalidInput} outcome (defense-in-depth; the live gate in loop_support's host_runtime_input_for_capability is fixed in a follow-up). Lib tests green: agent_loop 354, host_runtime 297, loop_support 337, reborn 259; outbound_delivery 26. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(loop_support): malformed sandbox plan is recoverable, not run-ending Completes the sandbox-plan fix. The live terminal gate is loop_support's host_runtime_input_for_capability: a malformed/invalid model-supplied SandboxProcessPlan returned AgentLoopHostError::InvalidInvocation, which capability_host_error maps to terminal HostUnavailable{Capability} (run dies). Now the invoke path downgrades that InvalidInvocation to a model-visible Ok(CapabilityOutcome::Failed{InvalidInput}) so the agent can correct the plan and the run continues. The helper only emits InvalidInvocation for the sandbox-plan parse/validation case; its host-internal serialization failure keeps its Internal Err. Updated both locked tests to assert the recoverable outcome. loop_support lib: 337 passed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(llm): provider error fidelity for accurate recover/explain (batch 2) Maps provider failures to the right LlmError variant so model-call errors are explained accurately and context overflow recovers via context-shrink instead of borking. - rig_adapter (OpenAI/Anthropic/Ollama/Tinfoil/openai_compatible): map_rig_error now detects auth failures (401/403/invalid key) -> AuthFailed (non-retryable, non-breaker-tripping) instead of generic RequestFailed, so a bad key surfaces as a credentials problem rather than wasted retries + opaque run-bork. - Codex (openai_codex_provider + codex_chatgpt): a stream ending without response.completed is now a retryable InvalidResponse/EmptyResponse instead of a silent successful Stop; codex_chatgpt now maps SSE error/response.failed events; both detect 413/context-overflow -> ContextLengthExceeded. - github_copilot + anthropic_oauth: detect 413 (and 400+context body) -> ContextLengthExceeded so context-shrink recovery fires (401/429/5xx untouched). ironclaw_llm lib: 915 passed, 0 failed. clippy + fmt clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(host_runtime): unknown method/capability is immediate model-visible error (batch 3) Sweep finding: a method/capability the model named that does not exist (RuntimeDispatchErrorKind::MethodMissing / UndeclaredCapability) mapped to RuntimeFailureKind::Backend -> RetrySameCall, so it burned the retry budget before becoming model-visible. Retrying never resolves a nonexistent target. Now maps to InvalidInput -> ModelVisibleToolError: the model gets an immediate "no such method/capability" tool error and self-corrects. Updated the pinning table entries. host_runtime lib: 297 passed. Batch-3 sweep conclusion: after the keystone + batches 1-2, no remaining recoverable->bork CORRECTNESS defects exist (tool backends fully clean; no hard-Err bypass on model-fixable conditions; nothing mapped to terminal Cancelled/Permanent). This was the last shape-#3 quality nit worth fixing now. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(reborn): FailureLane classifier + two-bucket enforcement test Item #2 foundation, built ON TOP of #4841's failure-surfacing machinery (reuses category + FailureExplanationProvider + retryable rather than a parallel RunFailureReason taxonomy). - FailureLane enum (Retriable | Explainable | Security), wire-stable snake_case. - failure_lane(category, retryable): retryable -> Retriable, else Explainable. Security is reserved for the ingress safety/leak refusal path (minimal security-stop policy) and is never produced at the run boundary; the match on category is the seam for a future mid-run safety-abort category. - ALL_RUN_FAILURE_CATEGORIES: canonical list of every category the run boundary can produce. - ENFORCEMENT TEST (every_failure_category_is_explainable_and_classified): locks the two-bucket invariant — every failure category resolves to a SPECIFIC user explanation (never the generic fallback) AND a definite lane. A new category that forgets its sentence, or regresses to the generic fallback, fails here. Plus canonical_list_covers_loop_failure_kinds guards against list drift. reborn_composition failure_lane: 5 passed. clippy clean (the one pre-existing needless_return in local_runtime_profile.rs is unrelated). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(reborn): retry-disposition policy (hybrid retry core) Operationalizes the hybrid retry decision on top of the FailureLane classifier. Pure decision function; the auto-redrive scheduler is its consumer. - RetryDisposition { Auto | UserInitiated | NoRetry }, wire-stable snake_case. - retry_disposition(category, retryable): no checkpoint -> NoRetry; transient host/lease/store/provider/tool faults -> Auto (silent re-drive from checkpoint, bounded by the scheduler); model/provider/config/model-fixable faults -> UserInitiated (retry affordance; a silent re-drive would just re-fail). Conservative Auto allowlist (anything not clearly transient -> UserInitiated). - RetryDisposition::failure_lane() ties it back to FailureLane; a test asserts the two layers agree for every category in ALL_RUN_FAILURE_CATEGORIES. reborn_composition retry_disposition: 5 passed. clippy + fmt clean. Follow-up: the scheduler wiring that calls retry_disposition() to auto-requeue (the behavior-flipping "Auto" half) — this is its tested decision core. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(reborn): carry secret-scrubbed raw cause to the model Reborn over-sanitized capability/host failures: a real cause like `missing input_schema_ref at /system/extensions/.../list_calendars.input.v1.json` was collapsed to the generic "host runtime rejected capability request" because there was no model-visible field to carry the raw cause and the summary validator rejected any string containing `/`. Policy shift: redact secret VALUES only; let paths, codes, schema refs, and raw error text reach the model so it can retry or explain. Foundation + Tier-1 vertical: - AgentLoopHostError gains an optional model-visible `detail: Option<String>` channel (+ `with_detail`). - CapabilityFailureDetail gains a free-text `Diagnostic { text }` variant. - ToolObservationDetail::GenericFailure gains a bounded, leniently-validated `detail` (allows `/ { } [ ] < >`, rejects NUL/control + caps length) — the channel that already reaches the model and bypasses the strict summary validator. - Relax ONLY the false-positive word bans in validate_loop_safe_summary and validate_tool_result_safe_summary (drop "provider error", "stack trace", "tool input", "traceback", "host path", "raw runtime", "invalid api key"); keep the delimiter ban, control-char ban, length cap, and credential markers. - Tier-1 producers stop dropping the cause: raw_agent_loop_host_error threads the value-scrubbed raw_detail into AgentLoopHostError.detail; the runtime model-visible failure path carries a value-scrubbed Diagnostic when the strict summary validator drops the reason; capability_helpers forwards the diagnostic into the model-visible observation. - Boxed ProviderArgumentError.error to keep result_large_err quiet after the AgentLoopHostError/CapabilityFailureDetail size growth. Tests cover the anchor (path string reaches the model-visible detail), secret value redaction, the relaxed/retained summary markers, and legacy GenericFailure JSON round-trip. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(reborn): MCP per-cause error tokens + explainer detail plumbing (tiers 2a, 3.2) - ironclaw_mcp: replace the flat "response_error"/"request_denied" literals with per-cause diagnostic tokens (mcp_http_status_<code>, mcp_jsonrpc_error code=..., mcp_parse_failed, ...), bounded + control-char-stripped, no public signature change. The model now learns the real HTTP status / JSON-RPC code. - reborn_composition: FailureExplanationInput gains a `detail` field rendered into the failure-explanation prompt (secret-scrubbed via sanitize_model_visible_text). Wired end-to-end in the projection; sourced once TurnLifecycleEvent carries detail (upstream chain in a follow-up commit). mcp --lib 18 passed; reborn_composition --lib failure_explanation tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(reborn): thread secret-scrubbed failure detail to the explainer (tiers 2b, 3.1) Complete the model-error-detail chain so the failure explainer (and the model) receive the real cause of a model/provider/driver fault instead of only a sanitized category. Only secret VALUES are withheld (scrubbed via the existing value-level redactors); the descriptive cause now flows end-to-end. Carrier `detail: Option<String>` (serde default + skip_serializing_if, so pre-detail persisted rows rehydrate as None) added and threaded through: - ironclaw_loop_support: HostManagedModelError.detail + with_detail; threaded in model_gateway_error into AgentLoopHostError.detail. - ironclaw_agent_loop: AgentLoopExecutorError::HostUnavailableWithDiagnostics gains detail; model-stage construction carries error.detail. - ironclaw_turns: AgentLoopDriverError::Failed.detail; TurnLifecycleEvent.detail (Failed events only, via failure_detail_for_event in the runner/memory path). - ironclaw_reborn: map_provider_error puts the scrubbed provider reason into HostManagedModelError.detail; planned_driver carries HostUnavailable detail into AgentLoopDriverError::Failed; turn_runner/turn_run_executor carry it into the failure record. - ironclaw_reborn_composition: detail_for_turn_event sources from event.detail, feeding the FailureExplanationInput.detail already rendered in the explainer prompt. Construction-site churn: detail added to TurnLifecycleEvent / AgentLoopDriverError test fixtures and the event_projections pending-gate test support. Verified per crate (--lib): turns 355, loop_support 342, agent_loop 261, reborn 175, event_projections 26 — all green; composition --lib 1042 passed (1 pre-existing live_progress_stream failure, unrelated). turns integration contracts compile; clippy clean across the chain. Pre-existing base-branch breakage in loop_support thread_loop_support_contract (inline_messages) is unrelated to this change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(turns): allow secret-scrubbed model-visible detail on Failed events The detail channel (TurnLifecycleEvent.detail) intentionally carries a secret-scrubbed description of the real failure cause to the model/explainer; update the guardrail so the spec matches the behavior. Only secret values are withheld; raw unscrubbed backend strings still stay behind host adapters. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(reborn): carry detail on AgentLoopDriverError::Failed in integration tests The tier-2b/3.1 detail field on AgentLoopDriverError::Failed broke construction and pattern sites in reborn's integration test targets (concurrent_workers, loop_driver_host) that the original --lib gate never compiled. Constructions get detail: None; the driver_host_error helper carries error.detail; exhaustive match patterns bind detail: _. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(reborn): update integration contracts for per-cause MCP tokens + detail channel Two integration test targets asserted pre-refinement model-visible strings that the --lib gate never compiled (test-through-the-caller gap): - mcp_adapter_contract: 5 assertions expected the flat "response_error"/ "request_denied" tokens; update them to the per-cause tokens the Tier 2a change now emits (mcp_invalid_protocol_version, mcp_jsonrpc_id_mismatch, mcp_invalid_session_id, mcp_http_status_500, mcp_denied_credential_source). - llm_gateway: the offline-provider test asserted the error Debug leaked NO provider detail at all. Tier 2b deliberately surfaces the secret-scrubbed non-secret cause on the detail channel. Rewrite (and rename) the test to the current policy: assert the non-secret reason ("connection refused", endpoint URL) reaches the model via `detail`, while the credential token (sk-provider-secret) is scrubbed from both `detail` and the full Debug. This strengthens the secret-scrubbing guard. mcp full suite green; llm_gateway full suite green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(host_runtime): missing first-party handler is InvalidInput, not Backend first_party_missing_handler_fails_closed_without_side_effect_handler asserted the dispatch failure kind was Backend, but #5389 deliberately reclassified an UndeclaredCapability/MethodMissing dispatch failure (a capability the model named that has no registered handler) to InvalidInput — a model-fixable, model-visible tool error that must not burn the retry budget on a call that can never resolve by retrying (see the From<DispatchFailureKind> mapping in production.rs). The test still fails closed (Failed outcome) and still carries "dispatch failed: UndeclaredCapability"; only the kind assertion is updated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(reborn): LoopFailureKind fault matrix — executor layer + exhaustive category table Phase 1 of the per-error coverage harness (docs/plans/2026-07-03-loop-failure-matrix.md): - turns: all_failure_kinds category table now exhaustive (13/13 — adds the previously-missing CheckpointUnavailable + CompactionUnavailable) with a same-crate exhaustive-match guard so a new variant breaks compilation. - agent_loop: new table-driven executor failure matrix (executor/tests/failure_matrix.rs) driving every executor-reachable LoopFailureKind at its real origin via MockHost seams, asserting per row: P1 (reason_kind + sanitized category/safe_summary), P3 (no fabricated final assistant reply), and explanation_message_refs presence per the explainable set. New fail_transcript_with test knob on MockHost + DriverMockHost (test code only). - Four divergences found and documented (doc §5a), asserted as actual behavior, none silently fixed: Approval+SkipAndContinue completes (gate enforcement gap), NoProgressDetected missing its explanation attach, single Denied recovers-and-completes (no-borking working as designed), TranscriptWriteFailed/CheckpointRejected legacy-only enum origins. Validated (bounded): cargo test -p ironclaw_turns all_failure_kinds; cargo test -p ironclaw_agent_loop failure_matrix; check/clippy -D warnings/ fmt on ironclaw_agent_loop — all green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(reborn): LoopFailureKind fault matrix — binary/driver layer rows Phase 2 of the per-error coverage harness (docs/plans/2026-07-03-loop-failure-matrix.md): - planned_driver: resume with missing checkpoint payload asserts LoopFailureKind::CheckpointUnavailable + "checkpoint_unavailable"; in-flight model Cancelled (no cooperative cancel signal) asserts map_executor_error yields "interrupted_unexpectedly". - e2e: binary-level divergence lock — the same in-flight Cancelled run projects "driver_failed" at the runner boundary (category overwritten; doc §5a.5, candidate follow-up to preserve the driver-mapped category). - e2e: non-model P4 row — capability-stage invocation failure is retryable ("host_stage_unavailable_capability", checkpoint preserved, no fabricated reply) and retry_run resumes to completion, so P4 is no longer proven only through the model stage. New scripted capability-invocation-error mode in the test harness (test support only). Validated (bounded): targeted planned_driver tests + full reborn_failure_retry_resume_e2e (15 passed) + clippy -D warnings + fmt. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(reborn): reconcile fault matrix to the recoverability stack (#5389/#5390) This matrix PR sits on top of the recoverability stack (main←#4841←#5389←#5390←#5403), so it validates the FIXED + classified system rather than #4841's base behavior: - Add executor rows for #5389's model-fixable capability failures that are now RECOVERABLE (InvalidInput / InvalidOutput / PolicyDenied): where the base branch terminated the run, the stack now surfaces a model-visible tool error and the loop completes. Rows assert the recovered outcome. - Add FailureLane / RetryDisposition alignment (binary/e2e layer, since ironclaw_agent_loop can't depend on ironclaw_reborn_composition): each real failure path's (category, retryable) is asserted to map to the expected #5390 FailureLane bucket + RetryDisposition — proving the real paths feed the classifier correctly. Complements #5390's classifier unit tests (which test the functions directly) rather than duplicating them. - Doc: §6 relationship to #5390; §5a marks divergences RESOLVED by the stack (capability recoverability) vs still-open (Approval SkipAndContinue completes; NoProgressDetected lacks explanation; Transcript/Checkpoint are planned-executor host errors; interrupted_unexpectedly projected as driver_failed at the runner boundary). The cherry-picked base matrix assertions still passed unchanged on the stack — they assert actual behavior, so the additive fixes did not break them; this commit adds the stack-specific coverage on top. Validated (bounded, on the stack): cargo test ironclaw_turns all_failure_kinds; cargo test ironclaw_agent_loop failure_matrix; cargo test --test reborn_failure_retry_resume_e2e (19 passed); clippy -D warnings; fmt --check. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(reborn): address gemini failure matrix comments (#5613) * style: cargo fmt on batch-1 recoverable-error changes Reproduces the stack's skipped fmt commit (c19db45) against the restacked base. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(tests): restack reconciliation — retry_run stub + superseded IssueCode import - webui_v2_router_smoke's MinimalWebuiServices gained the retry_run rejecting stub the trait now requires (base #4841 break: #5633's smoke fake predates #4841's retry_run addition; every other RebornServicesApi fake already has it). - drop CapabilityInputIssueCode from the ironclaw_turns re-export and ironclaw_loop_support import: the restacked base's CapabilityInputIssue carries DispatchInputIssueCode instead, and the stack's name was import-only on this branch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(restack): outbound set-target routes through outbound_delivery_outcome; MCP tests use Option error info - set-target handler: replace the superseded #5445 NotFound special-case + outbound_delivery_host_error (deleted by the recoverability batch) with the outbound_delivery_outcome disposition the stack pins in unit tests; matches the list handler. - parse_mcp_response framing tests (base-side, written against the old `error: bool`): assert against the stack's richer `Option<JsonRpcErrorInfo>` — same intent, error presence still pinned. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(restack): turn_stream_auth fake event carries the new detail field Base-side projection test predates the stack's TurnLifecycleEvent.detail addition; None matches the auth-gate fixture's intent. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(reborn): align failure_category_demasked pin to the fidelity taxonomy TraceLlm exhaustion (gateway cannot serve the call) maps to ModelErrorClass::Unavailable -> "model_unavailable" under the batch-2 provider-error fidelity mapping; the scenario's "model_error" pin predated it. The scenario's intent — the de-masked TRUE category survives, never the "driver_protocol_violation" sentinel — is unchanged and still asserted exactly. Capability-surface direction (not a security boundary): both categories are classified, retriable lanes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(agent_loop): run fault-matrix rows on 16MiB threads The restacked executor's future (failure explanations + digests + the recovery re-entry) outgrows the 2MiB default test-thread stack in debug on the in-run recovery rows. Production loop threads run 8MiB stacks (ironclaw_reborn_cli serve runtime); mirror the repo's big-stack test-thread pattern (traces/tests.rs, process_port.rs) per row. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(host_runtime): egress contract pins the per-cause MCP denial token The SecretStoreLease-over-production-egress denial now surfaces mcp_denied_credential_source (McpRequestDeniedCause::DeniedCredentialSource) instead of the flat request_denied. Deny-before-transport is unchanged and still asserted (zero recorded requests); ironclaw_mcp's own adapter contract pins the same token. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(review): strip model-visible failure detail from public run-state + align feature-gated tests Security (IronLoop HIGH): RebornGetRunStateResponse forwarded SanitizedFailure.detail — free-form, model-visible backend cause text, scrubbed only for secret VALUES — straight to the browser. Add SanitizedFailure::public_projection() (keeps category, drops detail) and project the public WebUI shape through it. Regression tests: the strip helper (status.rs) and the caller (get_run_state contract test now programs a detail-bearing failure and asserts the DTO omits it). Inherent to the stack, not the reconciliation (original top-of-stack forwarded it raw too). Feature-gated test alignments (only run under --all-features/libsql, so the default-feature local sweep missed them; CI crate buckets caught them): - factory web-access: missing first-party handler is InvalidInput, not Backend (#5389 reclassify; capability still fails closed, only disposition changed). - outbound local_dev (x2): set-target routes through outbound_delivery_outcome (matches original top-of-stack), so the missing-target summary is the fixed "invalid outbound delivery request"; error_kind stays recoverable InvalidInput. - ironclaw_mcp parse_mcp_response_rejects_empty: per-cause tokens replace the flat "response_error" (mcp_parse_failed / mcp_no_payload). Also: cargo fmt (capability_port import reflow from the restack edit). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(review): drop untrusted MCP server error message from model-visible reason + align tool_call category Security (IronLoop HIGH): parse_json_rpc_error_info copied the untrusted MCP server JSON-RPC error.message into the McpClientError::Client reason (model-visible, 'stable sanitized reason') with only length-bounding — not redaction. MCP servers can echo request args, paths, provider diagnostics, or credential-shaped values. Remove message end-to-end (JsonRpcErrorInfo field, parse, cause variant, render); keep only the standardized protocol code=<n>, which is the safe diagnostic. No redaction util is reachable from this leaf crate, and the stable-reason surface should carry stable tokens, not free text. Regression: the former 'reason carries message' test now asserts the message does NOT leak. Feature-gated integration test (ran only under --features libsql): tests/integration/tool_call.rs disabled-spawn-subagent-called-anyway now asserts 'model_unavailable' (InvalidOutput -> Unavailable fidelity category), not the stale 'model_error'. The security property (disabled capability never dispatched) is unchanged and still asserted. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(reborn): cancel-path provider error is model_context_overflow, not model_error fail_model() -> ErrLlm -> LlmError::ContextLengthExceeded, which the batch-2 provider fidelity mapping now categorizes as the accurate model_context_overflow (was the generic model_error). Both cancel tests still pin the load-bearing behavior: reaches Failed after bounded context-shrink recovery (no retry-forever), and the per-thread busy lock releases on Failed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(merge): implement retry_turn on TurnCoordinator doubles added by main Merging origin/main brought two new TurnCoordinator test doubles (UnusedTurnCoordinator in src/runtime.rs, SpyTurnCoordinator in tests/runtime.rs) that predate this stack's retry_turn addition to the TurnCoordinator trait. Add the impls (unimplemented!/delegate, matching each double's existing method style) so the merged tree compiles. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(deny): ignore RUSTSEC-2026-0204 (crossbeam Debug-fmt invalid deref) Newly published advisory (after this branch and main), transitive via crossbeam-epoch. The affected path is the `fmt::Pointer`/`Debug` impl for `Atomic`/`Shared` when the pointer is already invalid — a formatting path we do not exercise. Ignore with justification per the existing advisories convention; remove when the fixed crossbeam-utils release propagates. Verified `cargo deny check advisories` = ok locally. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(reborn): preserve scrubbed failure detail into TurnRunExecutorError (IronLoop) The driver-failed Err path in execute_claimed_run converted the computed SanitizedFailure back to TurnRunExecutorError::new(category), dropping the scrubbed model-visible detail. The scheduler records error.failure(), so production driver failures persisted only the category and TurnLifecycleEvent.detail stayed empty — the failure explainer got the fallback summary instead of the real provider/model cause. Add TurnRunExecutorError::from_failure(SanitizedFailure) (the struct already holds a full SanitizedFailure) and use it at the call site so detail survives across the host-runtime boundary. Caller regression test: driver Failed{detail: Some(..)} -> execute_claimed_run -> err.failure().detail() is preserved. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(reborn): JSON-frame untrusted failure detail in the explainer prompt (IronLoop) detail is untrusted provider/tool/runtime error text (e.g. MCP server or provider bodies). sanitize_model_visible_text redacts credential tokens but keeps newlines/instructions, so appending it raw let a crafted error inject extra prompt fields or directives (a fake fallback_summary:, an 'ignore previous instructions') into the failure explainer — whose output becomes the public failure_summary. That is a prompt-injection path into user-visible messaging, widened by the detail-preservation fix. Frame detail as data: JSON-string-escape it so newlines/quotes are escaped and it stays a single quoted 'detail: "..."' value. failure_category and fallback_summary are host-authored (category-derived) and unchanged. Regression test: a detail embedding newline+fallback_summary+directive is neutralized (exactly one real fallback_summary line, no directive line, detail present as an escaped quoted value). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Summary
Lands the reviewed integration-suite restructure as one PR with 7 ordered commits (per the plan's ONE-PR decision; per-stage sections of the reviewed plan are the content spec).
9417972a5) — roadmap suite to its dedicated home:tests/support/reborn/→tests/integration/support/, 27 flat bins →tests/integration/<name>.rs, 7 group dirs →tests/integration/group_<x>/.[[test]]names stay identical (reborn_integration_*,reborn_group_*); only paths move. Only content edits are the#[path]mount lines each move forces (folded here, not a later commit, so every commit builds green).47083d383) — 5,530-lineharness.rs→harness/{mod,recorder,options,assembly}.rs+doubles/(one file per substituted production port, header names the production seam). The binary-E2E family (RebornBinaryE2EHarness,model_replay, milestone asserts) leaves totests/support/reborn_parity_qa/— its consumers are exclusively parity/QA. 33 parity/QA bins mount the new tree; every survivingreborn_support::harness::<Name>path re-exported.02b56bc67) — the constructor table dissolves intoToolsProfile(typed capture of the sharednew_with_options+ 4 post-construct steps) +harness/profiles/<16 domains>.core_builtin4-variant suffix chain folds into oneCoreBuiltinOptions. Thick group pairs adoptbuild_group_capability_with_base;capability_backend::install()reuses the same profile fns. Flip-checked (falsified trigger profile → domain tests red for the right reason → reverted).1e77c0d68) —qa_trace/qa_scenarios/delivery/network→reborn_parity_qa/; deadapproval.rsdeleted; direction invariant documented + grep-clean.1f292b520) — single-run coverage: the 34 integration suites run once, instrumented (5 lanes, libsql), per-lane lcov → merged per-crate report + exemption manifest (tests/integration/coverage-exemptions.toml, seeded empty) + sticky PR comment (informational; gating rides the lanes). Duplicatereborn-coverage.ymldeleted. Newscripts/ci/check-test-suite-boundaries.shenforces the one-way support direction.faab5e1d1) — living docs repointed; authoring guide nowtests/integration/CLAUDE.md; newtests/support/reborn_parity_qa/CLAUDE.md.f95657973) — comment-only pass overtests/integration/(~1,800 lines): narration and migration provenance die; why-pins/seam contracts compressed to invariant + issue ref; all seam ids and DEFERRED/COVERED pointers retained.Plan deviations (all justified in commits): mount/
[[test]]wiring folded into commit 1 (per-commit green gates beat deferred wiring);model_replay.rsmoved withbinary_e2e.rsin commit 2 (atomic with its sole co-located consumer); commit 5 became a no-diff audit as a result;project_tools_with_fault_injection(landed after the plan's table) treated as a standard profile row.Test plan
cargo test --features libsql --no-fail-fast, 162 bins) green at every stacked commit; all 68 reborn-family bins green. Residual failures during sweeps were pre-existing load flakes (--libunder high parallelism,reborn_qa_connect_flows), each green isolated/rerun.--listdiff vs pre-restructure baseline — identical test fns modulo (a)qa_trace's 5 unit tests no longer ride along into the 34 integration bins (they still compile in every parity/QA bin) and (b) upstream fix(ci): stabilize main-equivalent clippy and coverage checks #5591 renames.cargo clippy --all --tests --examples --all-features -- -D warnings,cargo fmt --check,scripts/check_no_panics.pyclean.test-reborn-coverage.sh(78/78) pass locally; coverage lanes' lcov pipeline validated locally against real instrumented runs.reborn-integration-coveragelanes produce part lcovs andcoverage-reportrender the merged per-crate table on this PR.🤖 Generated with Claude Code