test(reborn): freeze the RebornServicesApi facade method set (§5.2.5/§10) - #6292
Conversation
…§10) §5.2.5 step 1 of the architecture-simplification note: freeze the product facade now so any *new* method fails CI and the §5.2 migration stops the bleeding before it starts. The product surface is turn-lifecycle + `invoke` (commands) + `query` (reads); a new product operation is a matrix-declared capability descriptor or a view descriptor, never a facade method. Adds `reborn_facade_method_freeze_ratchet.rs`, the sixth §10 anti-slippage ratchet, alongside the InMemory-store / LocalDev-typename / deployment-mode / capability-DTO-collapse / Authorized-seal ratchets. It freezes the current 88-method `RebornServicesApi` trait block (`crates/ironclaw_product_workflow/src/reborn_services.rs`) as a set-membership allowlist (§10: set membership, never a count) and fails on: - a new trait method not in the allowlist (the freeze); - a removed method not trimmed from the allowlist (so the list shrinks in lock-step toward the ~8-method turn-lifecycle + invoke/query end-state); - a duplicate method name (defensive). Unlike the sibling type-name ratchets, this one extracts the method set of one trait block via a brace-depth-aware scan (a `fn` inside a default-method body is ignored) over comment-/string-stripped source, reusing `ratchet_support::strip_comments_and_strings`. Ships with self-tests covering async/multi-line signatures, default bodies with nested fns and inner braces, impl-block/free fns, and comment/string decoys; verified it bites by injecting a method and observing the failure name it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🔎 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. |
github-actions
Bot
added
size: XS
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a Rust architecture test that freezes the ChangesReborn facade ratchet
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 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 |
![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 test file, reborn_facade_method_freeze_ratchet.rs, which implements an anti-slippage ratchet to freeze the RebornServicesApi trait block and prevent the facade method surface from growing. The reviewer identified an issue in the trait extraction logic where a simple substring search could match traits sharing a common prefix (e.g., RebornServicesApiExt instead of RebornServicesApi). They recommended using boundary-aware matching to ensure robust and accurate extraction.
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.
| let decl = format!("trait {trait_name}"); | ||
| let Some(decl_pos) = stripped.find(&decl) else { | ||
| return Vec::new(); | ||
| }; |
There was a problem hiding this comment.
The current substring matching logic stripped.find(&decl) can incorrectly match traits that share a common prefix with the target trait (for example, RebornServicesApiExt would be matched if it is defined before RebornServicesApi). To prevent this, we should ensure that the character immediately following the matched trait name is not a word character (i.e., not alphanumeric or underscore), adhering to boundary-aware matching principles.
let decl = format!("trait {trait_name}");
let mut start = 0;
let decl_pos = loop {
if let Some(pos) = stripped[start..].find(&decl) {
let absolute_pos = start + pos;
let next_byte = stripped.as_bytes().get(absolute_pos + decl.len());
if next_byte.map_or(true, |&b| !b.is_ascii_alphanumeric() && b != b'_') {
break Some(absolute_pos);
}
start = absolute_pos + 1;
} else {
break None;
}
};
let Some(decl_pos) = decl_pos else {
return Vec::new();
};References
- When scanning or matching terms in CamelCase identifiers, prefer structural or boundary-aware matching (e.g., requiring the term to be followed by an uppercase letter, digit, underscore, or end of string) over maintaining explicit prefix exception lists to prevent enumeration drift.
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 1 | 0 | 1 | 3e97e8f93c69 |
Head: 3e97e8f93c691ddea5f91e739c205124a251092f
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 new facade-freeze ratchet can silently bind to a renamed/prefixed trait instead of RebornServicesApi, defeating its stated rename guard.
Findings
Blocking: 1 / Notes: 0
Blocking findings
1. ❌ [MEDIUM] Require an exact trait-name match
Location: crates/ironclaw_architecture/tests/reborn_facade_method_freeze_ratchet.rs:166
find("trait {trait_name}") accepts a prefix match. If RebornServicesApi is renamed to RebornServicesApiV2 (or _legacy) with the same method set, this extractor scans that different trait and the ratchet passes, despite its promised fail-loud behavior on a rename. Require a non-identifier boundary after trait_name (and add that prefixed-name case to the missing-trait self-test).
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.
| fn extract_trait_methods(source: &str, trait_name: &str) -> Vec<String> { | ||
| let stripped = strip_comments_and_strings(source); | ||
| let decl = format!("trait {trait_name}"); | ||
| let Some(decl_pos) = stripped.find(&decl) else { |
There was a problem hiding this comment.
This substring search also matches trait RebornServicesApiV2 / _legacy. A rename retaining the same method set would therefore pass while scanning the wrong trait. Require an identifier boundary after the name and self-test that prefix case.
|
🚅 Deployed to the ironclaw-pr-6292 environment in ironclaw-ci-preview
|
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.63% — 313644 / 366296 lines Per-crate breakdown (65 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)
|
…ndaries (#6292 IronLoop/Gemini) The trait extractor did a plain substring `find("trait RebornServicesApi")`, so a rename that keeps the same method set — `trait RebornServicesApiV2`, `RebornServicesApi_legacy`, or a `subtrait`-like prefix — would silently bind to the renamed trait and defeat the freeze's stated rename guard. Require a word boundary on both sides of the trait name: `trait` must start a word and the character right after the name must not be an identifier char. Also fixes the formatting/Code-Style CI failures on the file. Regression: `extract_trait_methods_rejects_renamed_or_prefixed_trait_self_test` asserts the renamed/prefixed variants extract no methods while the exact `trait RebornServicesApi` (incl. a supertrait bound / generics after the name) still binds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed IronLoop + Gemini's blocking finding (substring trait match could bind to a renamed/prefixed trait) plus the Code-Style/Formatting CI failures — fixed in The extractor did a plain |
|
@ironloopai review |
There was a problem hiding this comment.
✅ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ✅ Approved | 0 | 0 | 0 | b3050d72412c |
Head: b3050d72412c1c0afa575d65d158abeece28d27b
Next: No reviewer action needed.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Reviewed a normal, focused PR: one new 382-line architecture ratchet test and no production changes. The extractor is scoped to the exact RebornServicesApi trait, and the frozen set matches all 88 current direct trait methods.
Findings
None.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_architecture/tests/reborn_facade_method_freeze_ratchet.rs`:
- Around line 262-265: Update the duplicate filtering around the `duplicated`
collection to store and check references in the existing `BTreeSet` instead of
cloning each extracted method name. Preserve the current duplicate detection and
collected-result behavior while changing the set and `seen.insert` types to use
string references.
🪄 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: 574253ea-e9d8-44ad-9e21-164f301acb19
📒 Files selected for processing (1)
crates/ironclaw_architecture/tests/reborn_facade_method_freeze_ratchet.rs
| let duplicated: Vec<&String> = found | ||
| .iter() | ||
| .filter(|m| !seen.insert((*m).clone())) | ||
| .collect(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Avoid unnecessary string allocations in the duplicate filter.
You can store references in the BTreeSet to avoid cloning every extracted method name during the duplicate check.
♻️ Proposed refactor
- let duplicated: Vec<&String> = found
- .iter()
- .filter(|m| !seen.insert((*m).clone()))
- .collect();
+ let duplicated: Vec<&String> = found
+ .iter()
+ .filter(|&m| !seen.insert(m))
+ .collect();🤖 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 `@crates/ironclaw_architecture/tests/reborn_facade_method_freeze_ratchet.rs`
around lines 262 - 265, Update the duplicate filtering around the `duplicated`
collection to store and check references in the existing `BTreeSet` instead of
cloning each extracted method name. Preserve the current duplicate detection and
collected-result behavior while changing the set and `seen.insert` types to use
string references.
|
✅ Ready for merge — facade-method-freeze ratchet (§5.2.5/§10). IronLoop: approved, 0 blocking. Fixed the earlier blocking finding (substring trait match could bind to a renamed/prefixed trait) with a word-boundary match + regression test, and the Code-Style/Formatting CI failures. CI green. |
Reconcile the capability-result collapse stack (host_api::Resolution as the single loop-facing capability result; CapabilityOutcome deleted; GateRecordStore + ReplayPayloadStore host-private stores) with 16 advancing main commits — DeploymentConfig owns every deployment axis (#6279), enforcement axes become resolved policy values (#6277), RebornServicesApi facade method-set freeze (#6292), hermetic NEARAI env tests (#6272), checkpoint stores over production impls (#6260), and the turn-state row store work (#6263). Single conflict: tests/integration/support/harness/mod.rs — both sides added fields to the same refresh-input struct literal. Resolved by unioning them: main's `trajectory_observer: None` + `extension_management` block AND the collapse's `gate_record_store` + `replay_payload_store`. Verified: `cargo build --workspace --all-features [--tests]` clean (no API drift), all 12 `ironclaw_architecture` ratchet binaries pass (collapse ratchet coexists with main's new deployment-mode-branching and facade-method-freeze ratchets). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…uest-side) collapse plan (#6306) §14 status log was stale — it listed the §5.3 five-channel flip as "in flight on integration/reborn-flip-base" and said "CapabilityOutcome is retained for Stage 2b to delete", but that work has landed on main: - Move the §5.3 flip stack from "In flight" to "Merged"; record #6293 (Stage 2b — CapabilityOutcome + all result mirrors DELETED), #6299 (the stack squash-landed on main, reconciled with #6279/#6277/#6292/#6296), and #6303 (auth-gate setup fingerprint fix + injective encoding). - Fix the Slice C.1 bullet: the Resolution/Blocked/Suspension/HostFailure channel enums are now merged too. - Add the remaining work under "Not started": the Slice C down-path (request-side) collapse — the 9 request mirrors still frozen in FROZEN_COLLAPSE_DTOS — with the concrete risk-ordered slice sequence (D1 dispatch→Authorized, D2 authorize(&Invocation), D3 loop membrane mints Invocation, D4 resume/auth-resume, D5 security-milestone seal inline, D6 ratchet-to-empty + measure). Docs-only; the frozen contract (§1–§13) is unchanged, only the mutable §14 log. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This branch was successfully deployed
1 participant
Implements §5.2.5 step 1 ("freeze the facade now") of
docs/reborn/2026-07-17-architecture-simplification-dto-dyn-local.md— the obvious unblocked next step on the §5.2 axis, independent of the in-flight §5.3Resolutionflip stack and Slice 0.What
Adds
crates/ironclaw_architecture/tests/reborn_facade_method_freeze_ratchet.rs— the sixth §10 anti-slippage ratchet, joining the InMemory-store, LocalDev-typename, deployment-mode-typename, deployment-mode-branching, capability-DTO-collapse, and Authorized-seal ratchets already onmain.It freezes the current 88-method
RebornServicesApitrait block (crates/ironclaw_product_workflow/src/reborn_services.rs) as a set-membership allowlist (§10: set membership, never a count) and fails on:FROZEN_REBORN_SERVICES_METHODS— the freeze. A new product operation is a matrix-declared capability descriptor or a view descriptor (§5.2), never a facade method.invoke/queryend-state (§5.2.5 step 5).How it differs from the sibling ratchets
The sibling ratchets scan type definitions (
struct/enum/trait/type) via the sharedratchet_supportscanners. §10 requires the facade freeze be "generated from the trait block itself, not a file-wide grep," so this ratchet adds a brace-depth-aware trait-method extractor: it reads only theRebornServicesApiblock, at trait-declaration depth (afninside a default-method body is ignored), over comment-/string-stripped source (reusingratchet_support::strip_comments_and_strings).Testing
Ships with self-tests (per "guardrails are code"):
extract_trait_methods_self_test— async + multi-line signatures, default bodies with nestedfns and inner braces, impl-block/freefns after the block, and comment/string decoys.extract_trait_methods_missing_trait_self_test— a renamed/absent trait yields no methods, so the main test's non-empty guard fires loudly rather than passing silently.reborn_facade_method_allowlist_is_frozen_and_only_shrinks— asserts against live source.Verified the ratchet bites: injecting
brand_new_feature_methodinto the trait producedFAILED … Offending new methods: ["brand_new_feature_method"].cargo test -p ironclaw_architecturefully green;cargo clippy -p ironclaw_architecture --test reborn_facade_method_freeze_ratchet --all-features -- -D warningsclean.Follow-ups (not this PR)
🤖 Generated with Claude Code