test(reborn): wiring-parity guard — harness runtime shape vs production local-dev (#5637) - #5642
Conversation
…s shape vs production local-dev (#5637) Adds a test-tree-only tripwire so the integration harness's DefaultPlannedRuntimeParts construction cannot silently drift from production's local-dev build: - support/planned_runtime_parts_shape.rs: exhaustive no-`..` destructure of all 32 fields into a 13-bool Some/None shape — a new/removed field in ironclaw_reborn fails this file to compile. - into_group captures the shape at the real construction site and stashes it on GroupSharedStorage; harness/group accessors expose it. - wiring_parity.rs asserts parity against a hand-derived EXPECTED_PRODUCTION_SHAPE modulo 5 named ALLOWED_DIVERGENCES rows (each with a reason), plus a smoke build of the local-dev profile. - Companion check: every harness/profiles/*.rs capability id is a subset of the production capability surface, modulo a documented synthetic-id skip list. All four falsifications verified red (allowlist-row removal, destructure field removal E0027, capability-id rename, bogus-id injection); suite green with zero production-crate edits. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a new integration test target that asserts planned-runtime shape parity and capability-id subset parity between harness profiles and production-derived surfaces. ChangesWiring-parity integration test
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
size: XS
![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 introduces a new integration test, reborn_integration_wiring_parity, to ensure that the test harness's runtime configuration matches production's local-dev configuration and that capability IDs do not drift. It adds a helper module to extract the Some/None shape of DefaultPlannedRuntimeParts and updates the integration group and harness builders to capture this shape. Feedback on the changes points out an unused support module declaration in the new test file that should be verified and cleaned up.
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.
![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: 154d9d0050
ℹ️ 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".
Reborn integration-tier coverageLine coverage (Reborn crates): 28.54% — 49242 / 172553 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/wiring_parity.rs`:
- Around line 62-131: Replace the stringly-typed field identifiers used by
ALLOWED_DIVERGENCES and mask() with a dedicated enum for the planned-runtime
fields, and update the allowlist to store enum values instead of raw strings.
Keep the exhaustive behavior by matching on the enum variants in mask(), and
make sure the enum stays aligned with DefaultPlannedRuntimePartsShape so field
additions/removals fail loudly at compile time.
🪄 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: c25e3a9e-6196-41ed-bcf9-9a64f5a28c9e
📒 Files selected for processing (6)
Cargo.tomltests/integration/support/builder.rstests/integration/support/group.rstests/integration/support/mod.rstests/integration/support/planned_runtime_parts_shape.rstests/integration/wiring_parity.rs
|
🚅 Deployed to the ironclaw-pr-5642 environment in ironclaw-ci-preview
|
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Add test-only Reborn wiring parity guards to catch integration harness drift from production local-dev runtime shape.
Stats: 2 findings (from 5 raw, 2 after dedup) across 1 file. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.
Maintainability
- Medium Capability parity test hand-copies the profile surfaces (
tests/integration/wiring_parity.rs:274-281, confidence 100) — anchor: tests/integration/wiring_parity.rs:274
The new subset check says each row is hand-transcribed fromharness/profiles/*.rs, so the test now has a second manually maintained copy of every harness capability list. If a profile adds or removes an id and this table is not edited, the drift guard still passes against the stale copy, which makes the test harder to trust precisely where it is meant to prevent drift.
Local Patterns
- Low Allowlist reason points at the wrong group.rs line (
tests/integration/wiring_parity.rs:89-92, confidence 75) — anchor: tests/integration/support/group.rs:726
The hook-security-audit-sink allowlist reason sends readers to group.rs:704, but in the checked-out file line 704 is the skill-context-source comment and the actualhook_security_audit_sink: Noneassignment is at line 726. This makes the re-derive trail misleading in a test whose comments are meant to be the maintenance map.
…fixes on PR #5642 Addresses 4 accepted review findings on wiring_parity.rs: 1. Capability subset check restructured (henrypark133 Medium 3524106126, codex P2 3523787910). LHS now reads each harness/profiles/*.rs domain's REAL capability_ids off its actual ToolsProfile/harness constructor instead of a hand-transcribed table; three bespoke (non-ToolsProfile) constructors (core_builtin, qa_smoke, web_access) got a small pure accessor shared with their own harness-literal builder, so there's one source, not two. RHS (production_capability_surface()) now unions only production-derived sources: builtin_first_party_package() + github_support::capability_ids() + a new extension_surface::bundled_extension_manifest_capability_ids(), which parses the other 9 bundled extensions' real manifest.toml assets the same way github's is already parsed. BUNDLED_EXTENSION_CAPABILITY_IDS (hand-typed, verified byte-identical to the real manifests — no drift found) is no longer unioned into RHS. STOP/report (not papered over): EXTENSION_LIFECYCLE_CAPABILITY_IDS' real values live in ironclaw_reborn_composition::extension_lifecycle_capabilities as pub(crate) with no public accessor — visibility-blocked from this test crate without a crates/ change, which is out of scope here (ZERO crates/ changes on this branch). The "extension" domain row now skips those 4 ids explicitly (documented) rather than silently re-including the old hand-list to make the check pass. Falsification: added a permanent regression test (harness_profile_capability_ids_subset_falsifies_on_narrowed_surface) that removes one real id from a copy of the surface and asserts the subset check would catch it on the right domain — proving the restructured shape still detects drift instead of two hand-lists trivially agreeing with each other. Verified by hand first: an over-strict version of this assertion caught the removal on "core_builtin" (which also grants that id) before being loosened to "profile is among the failing domains", proving the mechanism fires. 2. model_budget_accountant scope (codex P2 3523787909): doc-commented that EXPECTED_PRODUCTION_SHAPE's `false` models the no-LLM local-dev shape, and verified (by reading runtime.rs's model_gateway_override match arm) that the harness's scripted TraceLlm path forces the same None cost-table outcome unconditionally — so the comparison is genuinely like-for-like, not two different scopes glossed over. A real resolved-LLM production build would set this Some instead; noted as the field's variance boundary. 3. Stale line ref (henrypark133 Low 3524106127): hook_security_audit_sink's ALLOWED_DIVERGENCES reason no longer cites group.rs:704 (now :726, and volatile); replaced with the field name, plus a same-line comment at the assignment site itself. 4. mod support" necessity (gemini 3523784389): verified by actually removing it and running cargo check — compile fails (5 errors, "cannot find `support` in `crate`") because reborn_support's builder.rs/group.rs/reply.rs/scripted_provider.rs/triggered_submit.rs all reference `crate::support::trace_llm::{TraceLlm, ...}`. Restored; recommendation is keep as-is (dual-mount is required, not boilerplate). Verified: cargo test --all-features --test reborn_integration_wiring_parity (11/11 green, including the new falsification test), cargo check --tests --all-features (clean), cargo clippy on the touched target (clean), cargo fmt, and scripts/ci/check-test-suite-boundaries.sh (pass). Also spot-ran reborn_integration_web_access, reborn_integration_process_port, and reborn_integration_secret_injection to confirm the core_builtin/ qa_smoke/web_access accessor extraction is behavior-preserving. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/wiring_parity.rs`:
- Around line 371-379: The "extension" domain in wiring_parity is using an
order-dependent skip over capability_ids, which can exclude the wrong entries if
the concatenation order changes. Update the logic in the extension branch to
filter capability_ids by membership against EXTENSION_LIFECYCLE_CAPABILITY_IDS
instead of relying on position, using the existing
extension_lifecycle_tools_profile() and CapabilityId symbols to keep the
intended lifecycle set excluded regardless of ordering. Confirm CapabilityId
supports equality checks before switching to the membership-based filter.
🪄 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: fdf73e47-21b6-4efb-8c7a-0bbfd6d55335
📒 Files selected for processing (6)
tests/integration/support/extension_surface.rstests/integration/support/group.rstests/integration/support/harness/profiles/core_builtin.rstests/integration/support/harness/profiles/qa_smoke.rstests/integration/support/harness/profiles/web_access.rstests/integration/wiring_parity.rs
…n subset row Positional .skip(4) assumed the profile concatenates lifecycle ids first; a set-based filter is robust to internal reordering. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # Cargo.toml
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/support/builder.rs`:
- Around line 119-124: The tool disclosure plumbing is duplicated across the
harness and group builders, with mirrored bridged/off setters and a match in
build(); consolidate this into a single passthrough setter on
RebornIntegrationGroupBuilder that accepts Option<ToolDisclosureMode> and
forward the harness field directly to it. Update RebornIntegrationHarnessBuilder
and RebornIntegrationGroupBuilder to use the shared tool_disclosure path,
removing the separate with_tool_disclosure_bridged/with_tool_disclosure_off
duplication and the match-based re-dispatch in build().
In `@tests/integration/support/group.rs`:
- Around line 850-879: The FanOutTurnEventSink::publish implementation preserves
only the first sink error and silently discards any later failures, so add an
explicit log for each discarded error while keeping the existing “return first
error after trying all sinks” behavior. Update FanOutTurnEventSink::publish to
record/log subsequent sink.publish(event.clone()) errors (for example via a
debug-level message) before continuing the loop, so failures from both the
in-memory recorder and trace-capture sink remain observable without changing the
caller-facing Result contract.
🪄 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: 53c369a9-dec2-46be-8a14-d66a94439400
📒 Files selected for processing (3)
Cargo.tomltests/integration/support/builder.rstests/integration/support/group.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/support/builder.rs`:
- Around line 119-124: The tool disclosure plumbing is duplicated across the
harness and group builders, with mirrored bridged/off setters and a match in
build(); consolidate this into a single passthrough setter on
RebornIntegrationGroupBuilder that accepts Option<ToolDisclosureMode> and
forward the harness field directly to it. Update RebornIntegrationHarnessBuilder
and RebornIntegrationGroupBuilder to use the shared tool_disclosure path,
removing the separate with_tool_disclosure_bridged/with_tool_disclosure_off
duplication and the match-based re-dispatch in build().
In `@tests/integration/support/group.rs`:
- Around line 850-879: The FanOutTurnEventSink::publish implementation preserves
only the first sink error and silently discards any later failures, so add an
explicit log for each discarded error while keeping the existing “return first
error after trying all sinks” behavior. Update FanOutTurnEventSink::publish to
record/log subsequent sink.publish(event.clone()) errors (for example via a
debug-level message) before continuing the loop, so failures from both the
in-memory recorder and trace-capture sink remain observable without changing the
caller-facing Result contract.
🪄 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: 53c369a9-dec2-46be-8a14-d66a94439400
📒 Files selected for processing (3)
Cargo.tomltests/integration/support/builder.rstests/integration/support/group.rs
🛑 Comments failed to post (2)
tests/integration/support/builder.rs (1)
119-124: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Consider a single passthrough setter instead of two mirrored methods + match.
with_tool_disclosure_bridged/with_tool_disclosure_offon the harness builder each just setself.tool_disclosure, andbuild()then re-dispatches via a 3-armmatchinto the group builder's own equivalent pair of setters. A singlefn tool_disclosure(mut self, mode: Option<ToolDisclosureMode>) -> SelfonRebornIntegrationGroupBuilder, forwarded directly asgroup_builder.tool_disclosure(self.tool_disclosure), would drop the match and the duplicated method pair on the group side.Not blocking — the current shape is explicit and well-documented — but it's straightforward duplication across two builders for a single enum field.
Also applies to: 221-252, 411-419
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/support/builder.rs` around lines 119 - 124, The tool disclosure plumbing is duplicated across the harness and group builders, with mirrored bridged/off setters and a match in build(); consolidate this into a single passthrough setter on RebornIntegrationGroupBuilder that accepts Option<ToolDisclosureMode> and forward the harness field directly to it. Update RebornIntegrationHarnessBuilder and RebornIntegrationGroupBuilder to use the shared tool_disclosure path, removing the separate with_tool_disclosure_bridged/with_tool_disclosure_off duplication and the match-based re-dispatch in build().tests/integration/support/group.rs (1)
850-879: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
Fan-out sink silently drops every error after the first.
publishattempts all sinks but only ever surfaces the first error — any later sink's failure (e.g. a broken trace-capture write while the in-memory recorder also errors) is thrown away with no trace at all, not even a log line. This is a mild instance of the repo's "fail loud" concern: errors should propagate or at minimum be observable, not vanish.Since this is test-support infra with at most two sinks today, impact is low, but a
debug!("sink publish failed: {error}")for the discarded errors would make double-failure debugging possible without changing the single-error contract callers rely on.As per path instructions: "Fail loud: flag silent-failure patterns — .unwrap_or_default() on a Result, .ok()? dropping errors, let-else returning None to swallow failures, warn-and-continue that poisons state."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/support/group.rs` around lines 850 - 879, The FanOutTurnEventSink::publish implementation preserves only the first sink error and silently discards any later failures, so add an explicit log for each discarded error while keeping the existing “return first error after trying all sinks” behavior. Update FanOutTurnEventSink::publish to record/log subsequent sink.publish(event.clone()) errors (for example via a debug-level message) before continuing the loop, so failures from both the in-memory recorder and trace-capture sink remain observable without changing the caller-facing Result contract.Source: Path instructions
Closes #5637 (W5-WIRING-PARITY).
What
A wiring-drift tripwire for the integration harness, test-tree only (zero
src//crates/changes):tests/integration/support/planned_runtime_parts_shape.rs: exhaustive no-..destructure overDefaultPlannedRuntimeParts(32 fields, 13Option). Production adding/removing a field breaks this compile the day it lands, forcing a conscious wire-or-allowlist decision instead of a silentNone.tests/integration/wiring_parity.rs: the harness-built shape (bothtest_default()andbuiltin_tools()configs) compared againstEXPECTED_PRODUCTION_SHAPE, hand-derived frombuild_reborn_runtime's literal with a pinned re-derive instruction.ALLOWED_DIVERGENCESdocuments the 5 deliberate substitutions, one row per double/seam;mask()validates every row against the real field set and panics on unknown names, so renames can't leave stale entries inert.harness/profiles/<domain>capability-id list ⊆ the production registry surface (builtin_first_party_package()∪ extension ∪ github id sets), with an evidence-backed skip-list for synthetic ids. Ids reference production constants, so renames fail compile.Falsification evidence (all fired red, then reverted)
E0027compile error (tripwire proof).Known accepted gap
A silent production rewiring of an existing field isn't auto-detected (the expected shape is a hand-derived constant); #5641 tracks the machine-derived variant, which needs a production-crate test-support accessor and was deliberately excluded here. #5640 tracks the
hook_security_audit_sinkdouble gap behind one allowlist row.Verification
reborn_integration_wiring_parity10/10;cargo check --testsclean; clippy zero warnings (full workspace, all features); suite-boundary guard OK;git diff origin/main --name-only | grep -E '^(src|crates)/'empty.🤖 Generated with Claude Code