Repository navigation
test(reborn): freeze the InMemory*Store allowlist ratchet (§10) - #6204
Merged
Merged
Conversation
Contributor
🔎 IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted reviewer state before this projection. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds an architecture test that scans workspace Rust sources for public ChangesInMemory store inventory ratchet
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 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 |
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6204
July 17, 2026 20:12
Destroyed
github-actions
Bot
added
size: XS
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 1 | 1 | 2 | d0a8e978e6f7 |
Head: d0a8e978e6f77fcbf75d4b7b5d316e4852556c5f
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 stack layer is small and reviewable: one 155-line architecture test, and the 13-entry allowlist matches the current inventory. However, the ratchet collapses definitions by bare type name, allowing a new same-named store in another module to pass undetected. The scanner also misclassifies definition-shaped text inside multiline literals or block comments.
Findings
Blocking: 1 / Notes: 1
Blocking findings
1. ❌ [MEDIUM] Key the ratchet by qualified definition, not bare name
Location: crates/ironclaw_architecture/tests/reborn_inmemory_store_ratchet.rs:144
found is keyed only by the struct identifier, so multiple definitions with the same name collapse into one BTreeSet entry. Rust permits the same type name in different modules or crates; adding another InMemorySessionStore, for example, would leave the discovered set unchanged and pass this ratchet despite introducing a new store. Freeze a qualified key such as relative source path plus identifier, and add a regression test covering same-named definitions in different files.
Non-blocking notes (1)
1. 💬 [LOW] Scanner counts definition-shaped text inside raw strings and block comments
Location: crates/ironclaw_architecture/tests/reborn_inmemory_store_ratchet.rs:134-136
The prefix scan has no awareness of Rust comments or literals. A raw multiline fixture or block comment containing a line beginning pub struct InMemoryPhantomStore is reported as a real definition, contradicting the scanner contract and potentially breaking CI for legitimate fixtures. The current string-literal self-test does not exercise this because its literal contains no pub struct text. Parse or lex Rust source, or strip comments/literals, and add raw-string and block-comment cases.
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.
| .take_while(|c| c.is_alphanumeric() || *c == '_') | ||
| .collect(); | ||
| if ident.starts_with("InMemory") && ident.ends_with("Store") { | ||
| out.insert(ident); |
Contributor
There was a problem hiding this comment.
This set is keyed only by the bare identifier, so same-named definitions in different modules collapse. Adding another InMemorySessionStore, for example, leaves found unchanged and passes the ratchet. Please freeze a qualified key such as relative path plus identifier and test the duplicate-name case.
Member
Author
There was a problem hiding this comment.
Addressed in 661b020: definitions are now collected as identifier → defining-file paths, and any name with more than one definition fails the ratchet with the files listed (duplicate_definitions + a dedicated self-test covering the two-files-same-name case). I kept identifiers (rather than frozen path+name keys) as the allowlist key so a pure file move doesn't trip the ratchet — the debt is the type's existence, not its location — while the multiplicity check closes the collapse hole you describe.
| const MARKER: &str = "pub struct "; | ||
| for line in source.lines() { | ||
| let trimmed = line.trim_start(); | ||
| let Some(rest) = trimmed.strip_prefix(MARKER) else { |
Contributor
There was a problem hiding this comment.
This line-based prefix check also matches definition-shaped lines inside raw multiline strings or block comments. The self-test's string case does not contain pub struct, so it does not verify the documented exclusion. Please add raw-string/block-comment cases and make the scanner token-aware or strip those regions.
Member
Author
There was a problem hiding this comment.
Addressed in 661b020: the scanner now strips line comments, nested/multiline block comments, plain string literals (with escapes), raw strings (r#\"…\"#, incl. b/c prefixes), and char literals before the line scan, and the self-test now exercises marker-bearing cases for each region (block comment, nested block comment, plain string, raw string) — the previous string case indeed never contained the marker. Bonus from the same review pass: the matcher also covers pub(crate)/pub(super)/pub(in …) visibility (three previously-invisible production stores are now frozen) and skips tests/ trees so test doubles stay out of scope.
ilblackdragon
force-pushed
the
refactor/reborn-runstate-stores-over-rootfs
branch
from
July 17, 2026 20:29
1d293c8 to
d163cbc
Compare
ilblackdragon
force-pushed
the
refactor/reborn-inmemory-store-ratchet
branch
from
July 17, 2026 20:29
d0a8e97 to
92dbd8b
Compare
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6204
July 17, 2026 20:29
Destroyed
|
🚅 Deployed to the ironclaw-pr-6204 environment in ironclaw-ci-preview
|
ilblackdragon
force-pushed
the
refactor/reborn-runstate-stores-over-rootfs
branch
from
July 17, 2026 20:45
d163cbc to
f8a43b4
Compare
ilblackdragon
force-pushed
the
refactor/reborn-inmemory-store-ratchet
branch
from
July 17, 2026 20:45
92dbd8b to
5d950f6
Compare
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6204
July 17, 2026 20:45
Destroyed
Base automatically changed from
refactor/reborn-runstate-stores-over-rootfs
to
main
July 17, 2026 22:06
…simplification) Adds the anti-slippage ratchet §10 of docs/reborn/2026-07-17-architecture-simplification-dto-dyn-local.md calls for on the store-consolidation axis, locking in the Slice-A progress (approvals #6195, authorization-lease #6197, processes #6200, run-state #6203 all deleted their bespoke `InMemory*Store`s). `crates/ironclaw_architecture/tests/reborn_inmemory_store_ratchet.rs` scans `crates/` for `pub struct InMemory*Store` definitions and asserts the set exactly equals a checked-in frozen allowlist (the current 13, post-A4): - a NEW `InMemory*Store` fails the test — the debt can only shrink, never grow; - deleting a store without trimming the allowlist also fails — so the list is forced to shrink in lock-step as each domain lands (§10: compare set membership, never a count), and reviewers watch it get shorter. Definition of done for the axis (§10): the allowlist reaches empty — every store is `Filesystem*Store<InMemoryBackend>` in tests. The remaining Slice-A target is the turns domain (`InMemoryTurnStateStore` + checkpoint/instruction stores), which is reconcile-then-delete and also the `inmemory-turn-state` production runtime authority — deliberately left for a dedicated, carefully-validated effort, not a mechanical delete. Ships with its own scanner self-test (per the "guardrails are code" rule). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…multiplicity, comments/strings) Addresses the IronLoop findings on #6204 plus one more gap found in review: - Visibility: the scanner matched only `pub struct`, so `pub(crate)` InMemory stores evaded the ratchet entirely. It now matches `pub`/`pub(crate)`/`pub(super)`/`pub(in path)` and the frozen list gains the three production pub(crate) stores that were invisible before (`InMemorySecretsStore`, `InMemorySlackChannelRouteStore`, `InMemorySlackPersonalDmTargetStore`); `tests/` trees are skipped so test doubles (e.g. tests/support recording stores) stay out of scope. - Multiplicity: the set was keyed by bare identifier, so a second same-named store in another module collapsed into the allowlist entry. Definitions are now collected per path and any duplicate name fails with the defining files listed; covered by a dedicated self-test. - Comments/strings: definition-shaped text inside block comments (incl. nested/multiline) and plain/raw string literals no longer matches — a minimal lexer strips them before the line scan. The self-test now exercises marker-bearing comment and string cases, which the previous string-literal case never did. Also rebased onto main post-#6203 merge (drops the stacked A4 commit), resolving the merge conflict. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ilblackdragon
force-pushed
the
refactor/reborn-inmemory-store-ratchet
branch
from
July 17, 2026 22:19
5d950f6 to
661b020
Compare
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6204
July 17, 2026 22:19
Destroyed
ilblackdragon
force-pushed
the
refactor/reborn-inmemory-store-ratchet
branch
from
July 17, 2026 22:20
661b020 to
47bc5b4
Compare
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6204
July 17, 2026 22:20
Destroyed
ilblackdragon
force-pushed
the
refactor/reborn-inmemory-store-ratchet
branch
from
July 17, 2026 22:24
47bc5b4 to
661b020
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@crates/ironclaw_architecture/tests/reborn_inmemory_store_ratchet.rs`:
- Around line 150-166: Update the source-scanning and duplicate-tracking logic
used by scan_source_for_inmemory_store_defs and duplicate_definitions so
multiple definitions of the same store in different modules of one file remain
distinct occurrences rather than being collapsed by the per-file BTreeSet. Add a
regression test covering two module-level InMemoryDupStore definitions in a
single file, asserting both occurrences are preserved and flagged as duplicates,
while retaining the existing separate-file coverage.
- Around line 19-21: Align the collector’s production-only behavior with its
guarantee-bearing documentation: exclude inline #[cfg(test)] modules as well as
examples/ and benches, or narrow the documented contract to match actual
scanning. Update the collector and its related assertions, including a
regression fixture containing an inline #[cfg(test)] store, and ensure test
doubles outside production code are not reported as debt.
🪄 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: aa51c692-0963-4cbf-8d02-dba549ab76be
📒 Files selected for processing (1)
crates/ironclaw_architecture/tests/reborn_inmemory_store_ratchet.rs
…duction-scope contract Addresses the two CodeRabbit findings on #6204: - Same-file multiplicity: the per-file BTreeSet collapsed two same-named definitions in different modules of one file before paths were recorded, so the duplicate check could not see them. The scan now returns occurrences in source order (no dedup) and a dedicated self-test covers the same-file/two-module case. - Scope contract: the docs claimed "production code only", but the line-based scanner is not cfg-aware. The walk now also skips examples/ and benches/ alongside tests/, and the docs + a self-test fixture state the actual semantics: an inline #[cfg(test)] store in src IS inventoried — keep test doubles under tests/ or justify an allowlist entry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6204
July 17, 2026 22:35
Destroyed
Contributor
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.94% — 304827 / 354688 lines Per-crate breakdown (62 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)
|
Member
Author
✅ Ready for mergeReviewed, all findings addressed, CI fully green (56 pass / 0 fail) on head What was done:
Follow-up queued for #6205 (stacked): extract the hardened scanner into a shared test-support module so the LocalDev* ratchet reuses it instead of duplicating a weaker copy. 🤖 Generated with Claude Code |
ilblackdragon
added a commit
that referenced
this pull request
Jul 17, 2026
…for LocalDev ratchet Addresses the IronLoop finding on #6205 (line-oriented scan matches comment/string text and misses `pub unsafe trait`) by consolidating both §10 ratchets onto one hardened scanner instead of duplicating a weaker copy: - New `tests/ratchet_support/mod.rs`: comment/string-stripping lexer, `pub`/`pub(crate)`/`pub(super)`/`pub(in ...)` visibility, `unsafe`/`auto` modifiers, occurrence-preserving scans, production-scoped walk (skips tests/, examples/, benches/), and a multiplicity check. - `reborn_inmemory_store_ratchet.rs` refactored onto the shared module (behavior identical; self-tests preserved). - `reborn_localdev_typename_ratchet.rs` gains everything above plus cfg-aware multiplicity: the composition factory defines durable/ no-durable alias pairs for the same `LocalDev*` name under mutually exclusive `#[cfg(...)]` gates (including rustfmt-split multi-line gates) — those are exempt from the duplicate check only when every occurrence is cfg-gated; a mixed pair still fails. Regression tests cover the cfg pair (single- and multi-line), the mixed pair, same-file duplicates, and marker-bearing comment/string fixtures. Also rebased onto #6204's current head. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ilblackdragon
added a commit
that referenced
this pull request
Jul 17, 2026
…for LocalDev ratchet Addresses the IronLoop finding on #6205 (line-oriented scan matches comment/string text and misses `pub unsafe trait`) by consolidating both §10 ratchets onto one hardened scanner instead of duplicating a weaker copy: - New `tests/ratchet_support/mod.rs`: comment/string-stripping lexer, `pub`/`pub(crate)`/`pub(super)`/`pub(in ...)` visibility, `unsafe`/`auto` modifiers, occurrence-preserving scans, production-scoped walk (skips tests/, examples/, benches/), and a multiplicity check. - `reborn_inmemory_store_ratchet.rs` refactored onto the shared module (behavior identical; self-tests preserved). - `reborn_localdev_typename_ratchet.rs` gains everything above plus cfg-aware multiplicity: the composition factory defines durable/ no-durable alias pairs for the same `LocalDev*` name under mutually exclusive `#[cfg(...)]` gates (including rustfmt-split multi-line gates) — those are exempt from the duplicate check only when every occurrence is cfg-gated; a mixed pair still fails. Regression tests cover the cfg pair (single- and multi-line), the mixed pair, same-file duplicates, and marker-bearing comment/string fixtures. Also rebased onto #6204's current head. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ilblackdragon
added a commit
that referenced
this pull request
Jul 17, 2026
* test(reborn): freeze the LocalDev* type-name ratchet (§4.4/§10, arch-simplification) Adds the deployment-mode-as-type ratchet §4.4/§10 of docs/reborn/2026-07-17-architecture-simplification-dto-dyn-local.md calls for on the `Local*` axis, ahead of Slice B (collapsing the `LocalDev*` shadow runtime to a `DeploymentConfig` value). `crates/ironclaw_architecture/tests/reborn_localdev_typename_ratchet.rs` scans `crates/` for `pub`/`pub(crate)` `LocalDev*` type definitions (struct/enum/trait/ type alias) and asserts the set exactly equals a checked-in frozen allowlist (the current 31): - a NEW `LocalDev*` type fails — a deployment mode must resolve to policy data at the composition edge, never grow another type; - deleting one without trimming the allowlist also fails — so the list shrinks in lock-step as Slice B lands (§10: compare set membership, never a count). Definition of done for this axis: the allowlist reaches empty — local-dev is one `DeploymentConfig` constant, no `LocalDev*` type remains. Scoped to `LocalDev*` specifically (clean empty-set goal); the broader §4.4 name audit — Bucket 2 renames (`LocalFilesystem`->`DiskFilesystem`, `LocalHostProcessPort`->`HostProcessPort`) and Bucket 3 false positives (`Locale`, `HostedMcp*`, `LocalTraceSubmission*`) — is a separate concern. Ships with its own scanner self-test (per "guardrails are code"). The scanner is exhaustive: it surfaced 9 `LocalDev*` types a line-oriented grep had missed (synthetic-capability + extension-surface + auth-read-model families). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(reborn): share hardened ratchet scanner; cfg-aware multiplicity for LocalDev ratchet Addresses the IronLoop finding on #6205 (line-oriented scan matches comment/string text and misses `pub unsafe trait`) by consolidating both §10 ratchets onto one hardened scanner instead of duplicating a weaker copy: - New `tests/ratchet_support/mod.rs`: comment/string-stripping lexer, `pub`/`pub(crate)`/`pub(super)`/`pub(in ...)` visibility, `unsafe`/`auto` modifiers, occurrence-preserving scans, production-scoped walk (skips tests/, examples/, benches/), and a multiplicity check. - `reborn_inmemory_store_ratchet.rs` refactored onto the shared module (behavior identical; self-tests preserved). - `reborn_localdev_typename_ratchet.rs` gains everything above plus cfg-aware multiplicity: the composition factory defines durable/ no-durable alias pairs for the same `LocalDev*` name under mutually exclusive `#[cfg(...)]` gates (including rustfmt-split multi-line gates) — those are exempt from the duplicate check only when every occurrence is cfg-gated; a mixed pair still fails. Regression tests cover the cfg pair (single- and multi-line), the mixed pair, same-file duplicates, and marker-bearing comment/string fixtures. Also rebased onto #6204's current head. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
7 tasks done
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
§10 anti-slippage ratchet — freeze the
InMemory*StoreallowlistStacked on #6203. Implements the store-consolidation ratchet §10 of
docs/reborn/2026-07-17-architecture-simplification-dto-dyn-local.md— the enforcement the doc says each axis needs during the migration, not only after.Slice A has deleted the bespoke
InMemory*Storefor four domains (approvals #6195, authorization-lease #6197, processes #6200, run-state #6203). This test locks that in and prevents backsliding.What it does
crates/ironclaw_architecture/tests/reborn_inmemory_store_ratchet.rsscanscrates/forpub struct InMemory*Storedefinitions and asserts the set exactly equals a checked-in frozen allowlist (the current 13, post-A4):InMemory*Store(not in the allowlist) → test fails: the debt can only shrink, never grow.Definition of done for this axis (§10): the allowlist reaches the empty set — every store is
Filesystem*Store<InMemoryBackend>in tests.Remaining after this
The one remaining Slice-A domain is turns (
InMemoryTurnStateStore+InMemory{Checkpoint,LoopCheckpoint,InstructionMaterialization}Store). Unlike A2–A4 it is not a mechanical delete:InMemoryTurnStateStoreis 4,258 LOC (larger than the filesystem impl) and theinmemory-turn-stateproduction runtime authority (the in-process turn-state coordinator that avoids per-userstate.jsonCAS livelock). The allowlist documents it explicitly and it is deliberately left for a dedicated, stress-validated effort. The peripheral stores (budget/session/outbound/etc.) are listed to keep the ratchet exhaustive; whether each is in §4.3's domain-store scope is a separate call.Safety
Test-only. Ships with its own scanner self-test (per "guardrails are code"). No production code touched.
🤖 Generated with Claude Code