From 809790b6d3403f217b6b1b9734e8971c5bd34942 Mon Sep 17 00:00:00 2001 From: Illia Polosukhin Date: Fri, 17 Jul 2026 20:12:17 +0000 Subject: [PATCH 1/3] =?UTF-8?q?test(reborn):=20freeze=20the=20InMemory*Sto?= =?UTF-8?q?re=20allowlist=20ratchet=20(=C2=A710,=20arch-simplification)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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` 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) --- .../tests/reborn_inmemory_store_ratchet.rs | 155 ++++++++++++++++++ 1 file changed, 155 insertions(+) create mode 100644 crates/ironclaw_architecture/tests/reborn_inmemory_store_ratchet.rs diff --git a/crates/ironclaw_architecture/tests/reborn_inmemory_store_ratchet.rs b/crates/ironclaw_architecture/tests/reborn_inmemory_store_ratchet.rs new file mode 100644 index 00000000000..da2794c1621 --- /dev/null +++ b/crates/ironclaw_architecture/tests/reborn_inmemory_store_ratchet.rs @@ -0,0 +1,155 @@ +//! Anti-slippage ratchet for the store-consolidation axis (§10 of +//! `docs/reborn/2026-07-17-architecture-simplification-dto-dyn-local.md`). +//! +//! §4.3 replaces every hand-written `InMemory*Store` with the one production +//! `Filesystem*Store` exercised over `InMemoryBackend`. That migration is +//! incremental (per domain), so this test **freezes the current inventory of +//! `pub struct InMemory*Store` definitions** and fails on any change: +//! +//! - a **new** `InMemory*Store` (not in the allowlist) fails — the debt can only +//! shrink, never grow; +//! - **deleting** a store without removing it from [`FROZEN_INMEMORY_STORES`] +//! also fails — so the allowlist is forced to shrink in lock-step as each +//! domain lands, and a reviewer sees the list get shorter (§10: "compare set +//! membership, never an aggregate count"). +//! +//! Definition of done for this axis (§10): the allowlist reaches the empty set — +//! every store is `Filesystem*Store` in tests. Until then this +//! frozen set is the contract. + +use std::collections::BTreeSet; +use std::path::{Path, PathBuf}; + +/// The frozen inventory of `pub struct InMemory*Store` definitions under +/// `crates/`, as of the run-state slice (§4.3, A4). Remove an entry in the same +/// PR that deletes its store; never add one. Grouped by whether it is a +/// remaining Slice-A consolidation target or a peripheral store outside the +/// domain-store scope of §4.3 (the axis is still complete only when the whole +/// list is empty). +const FROZEN_INMEMORY_STORES: &[&str] = &[ + // --- remaining Slice-A domain store: turns (do last, reconcile-then-delete; + // `InMemoryTurnStateStore` is also the `inmemory-turn-state` production + // runtime authority, so it is not a mechanical delete) --- + "InMemoryTurnStateStore", + "InMemoryCheckpointStateStore", + "InMemoryLoopCheckpointStore", + "InMemoryInstructionMaterializationStore", + // --- peripheral stores (outside §4.3's five core domains; listed so the + // ratchet stays exhaustive and no new InMemory store slips in) --- + "InMemoryBoundedSubagentGoalStore", + "InMemoryBudgetGateStore", + "InMemoryDeliveredGateRouteStore", + "InMemoryExtensionInstallationStore", + "InMemoryOpenAiCompatRefStore", + "InMemoryOutboundStateStore", + "InMemorySecretStore", + "InMemorySessionStore", + "InMemoryTriggeredRunDeliveryStore", +]; + +#[test] +fn reborn_inmemory_store_allowlist_is_frozen_and_only_shrinks() { + let crates_dir = workspace_root().join("crates"); + let mut found = BTreeSet::new(); + collect_inmemory_store_defs(&crates_dir, &mut found); + + let frozen: BTreeSet<&str> = FROZEN_INMEMORY_STORES.iter().copied().collect(); + let found_refs: BTreeSet<&str> = found.iter().map(String::as_str).collect(); + + let added: Vec<&&str> = found_refs.difference(&frozen).collect(); + assert!( + added.is_empty(), + "New `pub struct InMemory*Store` definitions are banned (arch-simplification \ + §4.3/§10): every domain store must be `Filesystem*Store`. \ + Offending new stores: {added:?}. If this is a genuine new local store, justify \ + it in review and add it to FROZEN_INMEMORY_STORES — but the intended direction \ + is to delete these, not add them." + ); + + let removed: Vec<&&str> = frozen.difference(&found_refs).collect(); + assert!( + removed.is_empty(), + "FROZEN_INMEMORY_STORES lists stores that no longer exist: {removed:?}. A store \ + was deleted (good!) — trim it from the allowlist in the same PR so the ratchet \ + keeps shrinking toward empty (§10)." + ); +} + +/// Self-test for the scanner: it must extract exactly the `pub struct InMemory*Store` +/// definitions and ignore string literals, comments, and non-`Store` structs. +#[test] +fn inmemory_store_def_scanner_self_test() { + let sample = r#" + pub struct InMemoryWidgetStore { field: u8 } + struct InMemoryPrivateStore; // not `pub` -> ignored + pub struct InMemoryWidget { x: u8 } // not a *Store -> ignored + // pub struct InMemoryCommentedStore -> in a comment, but scanner is line-based + let name = "InMemoryStringLiteralStore"; // string literal, no `pub struct` -> ignored + pub struct InMemorySpacedStore(u8); // extra spaces tolerated + "#; + let mut out = BTreeSet::new(); + scan_source_for_inmemory_store_defs(sample, &mut out); + let got: Vec<&str> = out.iter().map(String::as_str).collect(); + assert_eq!( + got, + vec!["InMemorySpacedStore", "InMemoryWidgetStore"], + "scanner must match only `pub struct InMemory*Store` definitions" + ); +} + +fn collect_inmemory_store_defs(dir: &Path, out: &mut BTreeSet) { + let entries = std::fs::read_dir(dir) + .unwrap_or_else(|err| panic!("failed to read {}: {err}", dir.display())); + for entry in entries { + let entry = entry.unwrap_or_else(|err| panic!("failed to read dir entry: {err}")); + let path = entry.path(); + if path.is_dir() { + // Skip build artifacts. + if path.file_name().and_then(|n| n.to_str()) == Some("target") { + continue; + } + collect_inmemory_store_defs(&path, out); + continue; + } + if path.extension().and_then(|ext| ext.to_str()) != Some("rs") { + continue; + } + // This ratchet file's own self-test fixture contains sample + // `pub struct InMemory*Store` lines; don't count them. + if path.file_name().and_then(|n| n.to_str()) == Some("reborn_inmemory_store_ratchet.rs") { + continue; + } + let contents = std::fs::read_to_string(&path) + .unwrap_or_else(|err| panic!("failed to read {}: {err}", path.display())); + scan_source_for_inmemory_store_defs(&contents, out); + } +} + +/// Line-based scan: pull the identifier from any `pub struct InMemory*Store` +/// definition. Intentionally simple (no regex dep); matches the definition form, +/// not references, string literals, or comments. +fn scan_source_for_inmemory_store_defs(source: &str, out: &mut BTreeSet) { + const MARKER: &str = "pub struct "; + for line in source.lines() { + let trimmed = line.trim_start(); + let Some(rest) = trimmed.strip_prefix(MARKER) else { + continue; + }; + let ident: String = rest + .trim_start() + .chars() + .take_while(|c| c.is_alphanumeric() || *c == '_') + .collect(); + if ident.starts_with("InMemory") && ident.ends_with("Store") { + out.insert(ident); + } + } +} + +fn workspace_root() -> PathBuf { + PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .parent() + .and_then(|path| path.parent()) + .expect("architecture crate must live under crates/ironclaw_architecture") + .to_path_buf() +} From 661b020be8a31e8385616394fbb5659b5cf37da5 Mon Sep 17 00:00:00 2001 From: Illia Polosukhin Date: Fri, 17 Jul 2026 22:19:31 +0000 Subject: [PATCH 2/3] test(reborn): harden the InMemory*Store ratchet scanner (visibility, multiplicity, comments/strings) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../tests/reborn_inmemory_store_ratchet.rs | 268 +++++++++++++++--- 1 file changed, 236 insertions(+), 32 deletions(-) diff --git a/crates/ironclaw_architecture/tests/reborn_inmemory_store_ratchet.rs b/crates/ironclaw_architecture/tests/reborn_inmemory_store_ratchet.rs index da2794c1621..5a72a0fe935 100644 --- a/crates/ironclaw_architecture/tests/reborn_inmemory_store_ratchet.rs +++ b/crates/ironclaw_architecture/tests/reborn_inmemory_store_ratchet.rs @@ -4,23 +4,30 @@ //! §4.3 replaces every hand-written `InMemory*Store` with the one production //! `Filesystem*Store` exercised over `InMemoryBackend`. That migration is //! incremental (per domain), so this test **freezes the current inventory of -//! `pub struct InMemory*Store` definitions** and fails on any change: +//! pub-visible `struct InMemory*Store` definitions** and fails on any change: //! //! - a **new** `InMemory*Store` (not in the allowlist) fails — the debt can only //! shrink, never grow; +//! - a **second definition** of a frozen name (in another module/crate) fails — +//! the set is keyed by identifier, so multiplicity is checked explicitly and +//! duplicate-name debt cannot hide behind an existing entry; //! - **deleting** a store without removing it from [`FROZEN_INMEMORY_STORES`] //! also fails — so the allowlist is forced to shrink in lock-step as each //! domain lands, and a reviewer sees the list get shorter (§10: "compare set //! membership, never an aggregate count"). //! +//! The scanner strips comments and string literals before matching, and scans +//! production code only (a crate's `tests/` tree holds test doubles, not the +//! §4.3 debt this ratchet inventories). +//! //! Definition of done for this axis (§10): the allowlist reaches the empty set — //! every store is `Filesystem*Store` in tests. Until then this //! frozen set is the contract. -use std::collections::BTreeSet; +use std::collections::{BTreeMap, BTreeSet}; use std::path::{Path, PathBuf}; -/// The frozen inventory of `pub struct InMemory*Store` definitions under +/// The frozen inventory of pub-visible `struct InMemory*Store` definitions under /// `crates/`, as of the run-state slice (§4.3, A4). Remove an entry in the same /// PR that deletes its store; never add one. Grouped by whether it is a /// remaining Slice-A consolidation target or a peripheral store outside the @@ -45,25 +52,43 @@ const FROZEN_INMEMORY_STORES: &[&str] = &[ "InMemorySecretStore", "InMemorySessionStore", "InMemoryTriggeredRunDeliveryStore", + // --- pub(crate) stores the visibility-aware scanner also inventories + // (same debt class, just crate-private) --- + "InMemorySecretsStore", + "InMemorySlackChannelRouteStore", + "InMemorySlackPersonalDmTargetStore", ]; #[test] fn reborn_inmemory_store_allowlist_is_frozen_and_only_shrinks() { let crates_dir = workspace_root().join("crates"); - let mut found = BTreeSet::new(); + let mut found: BTreeMap> = BTreeMap::new(); collect_inmemory_store_defs(&crates_dir, &mut found); let frozen: BTreeSet<&str> = FROZEN_INMEMORY_STORES.iter().copied().collect(); - let found_refs: BTreeSet<&str> = found.iter().map(String::as_str).collect(); + let found_refs: BTreeSet<&str> = found.keys().map(String::as_str).collect(); - let added: Vec<&&str> = found_refs.difference(&frozen).collect(); + let added: Vec<(&str, &Vec)> = found + .iter() + .filter(|(name, _)| !frozen.contains(name.as_str())) + .map(|(name, paths)| (name.as_str(), paths)) + .collect(); assert!( added.is_empty(), - "New `pub struct InMemory*Store` definitions are banned (arch-simplification \ - §4.3/§10): every domain store must be `Filesystem*Store`. \ - Offending new stores: {added:?}. If this is a genuine new local store, justify \ - it in review and add it to FROZEN_INMEMORY_STORES — but the intended direction \ - is to delete these, not add them." + "New pub-visible `struct InMemory*Store` definitions are banned \ + (arch-simplification §4.3/§10): every domain store must be \ + `Filesystem*Store`. Offending new stores: {added:?}. If this \ + is a genuine new local store, justify it in review and add it to \ + FROZEN_INMEMORY_STORES — but the intended direction is to delete these, not \ + add them." + ); + + let duplicated = duplicate_definitions(&found); + assert!( + duplicated.is_empty(), + "Each frozen InMemory*Store name must have exactly one definition; a second \ + same-named definition elsewhere is new debt hiding behind an allowlist entry \ + (§10): {duplicated:?}" ); let removed: Vec<&&str> = frozen.difference(&found_refs).collect(); @@ -75,37 +100,92 @@ fn reborn_inmemory_store_allowlist_is_frozen_and_only_shrinks() { ); } -/// Self-test for the scanner: it must extract exactly the `pub struct InMemory*Store` -/// definitions and ignore string literals, comments, and non-`Store` structs. +/// Self-test for the scanner: it must extract exactly the pub-visible +/// (including `pub(crate)`/`pub(super)`/`pub(in ...)`) `struct InMemory*Store` +/// definitions and ignore private structs, non-`Store` structs, and — because +/// comments and strings are stripped before matching — definition-shaped text in +/// line comments, block comments (nested and multiline), plain string literals, +/// and raw string literals. #[test] fn inmemory_store_def_scanner_self_test() { - let sample = r#" + let sample = r##" pub struct InMemoryWidgetStore { field: u8 } - struct InMemoryPrivateStore; // not `pub` -> ignored + struct InMemoryPrivateStore; // not pub-visible -> ignored pub struct InMemoryWidget { x: u8 } // not a *Store -> ignored - // pub struct InMemoryCommentedStore -> in a comment, but scanner is line-based - let name = "InMemoryStringLiteralStore"; // string literal, no `pub struct` -> ignored + // pub struct InMemoryLineCommentedStore -> ignored + /* pub struct InMemoryBlockCommentedStore */ + /* + pub struct InMemoryMultilineCommentedStore { x: u8 } + /* pub struct InMemoryNestedCommentedStore */ + */ + let name = "pub struct InMemoryStringLiteralStore"; + let raw = r#" + pub struct InMemoryRawStringStore; + "#; pub struct InMemorySpacedStore(u8); // extra spaces tolerated - "#; + pub(crate) struct InMemoryCrateStore; // restricted visibility -> still inventoried + pub(in crate::foo) struct InMemoryInPathStore; // restricted visibility -> still inventoried + pub(crate)fn not_a_struct() {} // no `struct` after visibility -> ignored + "##; let mut out = BTreeSet::new(); scan_source_for_inmemory_store_defs(sample, &mut out); let got: Vec<&str> = out.iter().map(String::as_str).collect(); assert_eq!( got, - vec!["InMemorySpacedStore", "InMemoryWidgetStore"], - "scanner must match only `pub struct InMemory*Store` definitions" + vec![ + "InMemoryCrateStore", + "InMemoryInPathStore", + "InMemorySpacedStore", + "InMemoryWidgetStore" + ], + "scanner must match only pub-visible `struct InMemory*Store` definitions \ + outside comments and strings" + ); +} + +/// Self-test for the multiplicity check: the same identifier defined in two +/// files must be reported as a duplicate. +#[test] +fn inmemory_store_duplicate_detection_self_test() { + let mut found: BTreeMap> = BTreeMap::new(); + for path in ["crate_a/src/lib.rs", "crate_b/src/lib.rs"] { + let mut idents = BTreeSet::new(); + scan_source_for_inmemory_store_defs("pub struct InMemoryDupStore;", &mut idents); + for ident in idents { + found.entry(ident).or_default().push(PathBuf::from(path)); + } + } + let duplicated = duplicate_definitions(&found); + assert_eq!( + duplicated.len(), + 1, + "two same-named definitions must be flagged" ); + assert_eq!(duplicated[0].0, "InMemoryDupStore"); + assert_eq!(duplicated[0].1.len(), 2); +} + +fn duplicate_definitions(found: &BTreeMap>) -> Vec<(&str, &Vec)> { + found + .iter() + .filter(|(_, paths)| paths.len() > 1) + .map(|(name, paths)| (name.as_str(), paths)) + .collect() } -fn collect_inmemory_store_defs(dir: &Path, out: &mut BTreeSet) { +fn collect_inmemory_store_defs(dir: &Path, out: &mut BTreeMap>) { let entries = std::fs::read_dir(dir) .unwrap_or_else(|err| panic!("failed to read {}: {err}", dir.display())); for entry in entries { let entry = entry.unwrap_or_else(|err| panic!("failed to read dir entry: {err}")); let path = entry.path(); if path.is_dir() { - // Skip build artifacts. - if path.file_name().and_then(|n| n.to_str()) == Some("target") { + // Skip build artifacts, and integration-test trees: doubles defined + // under a crate's `tests/` directory (e.g. recording stores in + // tests/support/) are test seams, not the §4.3 production-store debt + // this ratchet inventories. + let dir_name = path.file_name().and_then(|n| n.to_str()); + if dir_name == Some("target") || dir_name == Some("tests") { continue; } collect_inmemory_store_defs(&path, out); @@ -114,25 +194,43 @@ fn collect_inmemory_store_defs(dir: &Path, out: &mut BTreeSet) { if path.extension().and_then(|ext| ext.to_str()) != Some("rs") { continue; } - // This ratchet file's own self-test fixture contains sample - // `pub struct InMemory*Store` lines; don't count them. + // This ratchet file's own allowlist and self-test fixtures mention + // `InMemory*Store` names; string/comment stripping already excludes + // them, but skip the file entirely as defense in depth. if path.file_name().and_then(|n| n.to_str()) == Some("reborn_inmemory_store_ratchet.rs") { continue; } let contents = std::fs::read_to_string(&path) .unwrap_or_else(|err| panic!("failed to read {}: {err}", path.display())); - scan_source_for_inmemory_store_defs(&contents, out); + let mut idents = BTreeSet::new(); + scan_source_for_inmemory_store_defs(&contents, &mut idents); + for ident in idents { + out.entry(ident).or_default().push(path.clone()); + } } } -/// Line-based scan: pull the identifier from any `pub struct InMemory*Store` -/// definition. Intentionally simple (no regex dep); matches the definition form, -/// not references, string literals, or comments. +/// Extract the identifier from every pub-visible `struct InMemory*Store` +/// definition — `pub`, `pub(crate)`, `pub(super)`, or `pub(in path)` (a +/// restricted-visibility store is the same debt class, just crate-private). +/// Comments and string literals are stripped first, so definition-shaped text +/// inside them is not matched. Matches the definition form, not references. fn scan_source_for_inmemory_store_defs(source: &str, out: &mut BTreeSet) { - const MARKER: &str = "pub struct "; - for line in source.lines() { + let stripped = strip_comments_and_strings(source); + for line in stripped.lines() { let trimmed = line.trim_start(); - let Some(rest) = trimmed.strip_prefix(MARKER) else { + let Some(after_pub) = trimmed.strip_prefix("pub") else { + continue; + }; + // Optional restricted-visibility qualifier: `(crate)`, `(super)`, `(in path)`. + let after_vis = match after_pub.trim_start().strip_prefix('(') { + Some(rest) => match rest.split_once(')') { + Some((_, tail)) => tail, + None => continue, + }, + None => after_pub, + }; + let Some(rest) = after_vis.trim_start().strip_prefix("struct ") else { continue; }; let ident: String = rest @@ -146,6 +244,112 @@ fn scan_source_for_inmemory_store_defs(source: &str, out: &mut BTreeSet) } } +/// Replace line comments, block comments (nested), plain/raw string literal +/// contents, and char literals with blanks, preserving newlines so the +/// line-based matcher keeps operating on real code lines only. A minimal +/// lexer — good enough for rustfmt'd source; it intentionally errs on the side +/// of stripping (a mis-lex would surface loudly as a frozen-set mismatch). +fn strip_comments_and_strings(source: &str) -> String { + let chars: Vec = source.chars().collect(); + let mut out = String::with_capacity(source.len()); + let mut i = 0; + while i < chars.len() { + let c = chars[i]; + // Line comment. + if c == '/' && chars.get(i + 1) == Some(&'/') { + while i < chars.len() && chars[i] != '\n' { + i += 1; + } + continue; + } + // Block comment (Rust block comments nest). + if c == '/' && chars.get(i + 1) == Some(&'*') { + let mut depth = 1usize; + i += 2; + while i < chars.len() && depth > 0 { + if chars[i] == '/' && chars.get(i + 1) == Some(&'*') { + depth += 1; + i += 2; + } else if chars[i] == '*' && chars.get(i + 1) == Some(&'/') { + depth -= 1; + i += 2; + } else { + if chars[i] == '\n' { + out.push('\n'); + } + i += 1; + } + } + continue; + } + // Raw string literal: r"..." / r#"..."# (optionally b/c-prefixed). + if c == 'r' || ((c == 'b' || c == 'c') && chars.get(i + 1) == Some(&'r')) { + let hash_start = if c == 'r' { i + 1 } else { i + 2 }; + let mut j = hash_start; + while chars.get(j) == Some(&'#') { + j += 1; + } + if chars.get(j) == Some(&'"') { + let hashes = j - hash_start; + let mut k = j + 1; + while k < chars.len() { + if chars[k] == '"' && (0..hashes).all(|h| chars.get(k + 1 + h) == Some(&'#')) { + k += 1 + hashes; + break; + } + if chars[k] == '\n' { + out.push('\n'); + } + k += 1; + } + i = k; + continue; + } + } + // Plain string literal (handles escapes). + if c == '"' { + i += 1; + while i < chars.len() { + if chars[i] == '\\' { + i += 2; + continue; + } + if chars[i] == '"' { + i += 1; + break; + } + if chars[i] == '\n' { + out.push('\n'); + } + i += 1; + } + continue; + } + // Char literal vs lifetime: only consume when it closes as a literal. + if c == '\'' { + if chars.get(i + 1) == Some(&'\\') { + let mut k = i + 2; + while k < chars.len() && chars[k] != '\'' { + k += 1; + } + i = k + 1; + continue; + } + if chars.get(i + 2) == Some(&'\'') { + i += 3; + continue; + } + // A lifetime (`'a`) — emit and move on. + out.push(c); + i += 1; + continue; + } + out.push(c); + i += 1; + } + out +} + fn workspace_root() -> PathBuf { PathBuf::from(env!("CARGO_MANIFEST_DIR")) .parent() From 93a8c39b5c98ff4ad61758f12f93196799a453f3 Mon Sep 17 00:00:00 2001 From: Illia Polosukhin Date: Fri, 17 Jul 2026 22:35:23 +0000 Subject: [PATCH 3/3] test(reborn): ratchet scan preserves same-file duplicates; honest production-scope contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../tests/reborn_inmemory_store_ratchet.rs | 92 ++++++++++++++----- 1 file changed, 69 insertions(+), 23 deletions(-) diff --git a/crates/ironclaw_architecture/tests/reborn_inmemory_store_ratchet.rs b/crates/ironclaw_architecture/tests/reborn_inmemory_store_ratchet.rs index 5a72a0fe935..4e03dc537eb 100644 --- a/crates/ironclaw_architecture/tests/reborn_inmemory_store_ratchet.rs +++ b/crates/ironclaw_architecture/tests/reborn_inmemory_store_ratchet.rs @@ -16,9 +16,11 @@ //! domain lands, and a reviewer sees the list get shorter (§10: "compare set //! membership, never an aggregate count"). //! -//! The scanner strips comments and string literals before matching, and scans -//! production code only (a crate's `tests/` tree holds test doubles, not the -//! §4.3 debt this ratchet inventories). +//! The scanner strips comments and string literals before matching. It skips +//! `tests/`, `examples/`, and `benches/` trees (test doubles there are not §4.3 +//! debt) but is line-based, not cfg-aware: a pub-visible store defined in an +//! inline `#[cfg(test)]` module in src IS inventoried — keep test doubles under +//! `tests/` (or justify an allowlist entry in review). //! //! Definition of done for this axis (§10): the allowlist reaches the empty set — //! every store is `Filesystem*Store` in tests. Until then this @@ -126,20 +128,25 @@ fn inmemory_store_def_scanner_self_test() { pub(crate) struct InMemoryCrateStore; // restricted visibility -> still inventoried pub(in crate::foo) struct InMemoryInPathStore; // restricted visibility -> still inventoried pub(crate)fn not_a_struct() {} // no `struct` after visibility -> ignored + #[cfg(test)] + mod tests { + // The scanner is line-based, not cfg-aware: an inline cfg(test) + // double in src IS inventoried (keep doubles under `tests/`). + pub struct InMemoryCfgTestStore; + } "##; - let mut out = BTreeSet::new(); - scan_source_for_inmemory_store_defs(sample, &mut out); - let got: Vec<&str> = out.iter().map(String::as_str).collect(); + let got = scan_source_for_inmemory_store_defs(sample); assert_eq!( got, vec![ + "InMemoryWidgetStore", + "InMemorySpacedStore", "InMemoryCrateStore", "InMemoryInPathStore", - "InMemorySpacedStore", - "InMemoryWidgetStore" + "InMemoryCfgTestStore" ], - "scanner must match only pub-visible `struct InMemory*Store` definitions \ - outside comments and strings" + "scanner must match pub-visible `struct InMemory*Store` definitions \ + outside comments and strings, in source order" ); } @@ -149,9 +156,7 @@ fn inmemory_store_def_scanner_self_test() { fn inmemory_store_duplicate_detection_self_test() { let mut found: BTreeMap> = BTreeMap::new(); for path in ["crate_a/src/lib.rs", "crate_b/src/lib.rs"] { - let mut idents = BTreeSet::new(); - scan_source_for_inmemory_store_defs("pub struct InMemoryDupStore;", &mut idents); - for ident in idents { + for ident in scan_source_for_inmemory_store_defs("pub struct InMemoryDupStore;") { found.entry(ident).or_default().push(PathBuf::from(path)); } } @@ -165,6 +170,42 @@ fn inmemory_store_duplicate_detection_self_test() { assert_eq!(duplicated[0].1.len(), 2); } +/// Self-test for same-file multiplicity: two same-named definitions in +/// different modules of ONE file must also be reported — the scan preserves +/// occurrences instead of deduplicating per file. +#[test] +fn inmemory_store_same_file_duplicate_detection_self_test() { + let sample = r#" + mod first { + pub struct InMemoryDupStore; + } + mod second { + pub struct InMemoryDupStore; + } + "#; + let occurrences = scan_source_for_inmemory_store_defs(sample); + assert_eq!( + occurrences, + vec!["InMemoryDupStore", "InMemoryDupStore"], + "same-file duplicates must be preserved by the scan" + ); + + let mut found: BTreeMap> = BTreeMap::new(); + for ident in occurrences { + found + .entry(ident) + .or_default() + .push(PathBuf::from("crate_a/src/lib.rs")); + } + let duplicated = duplicate_definitions(&found); + assert_eq!( + duplicated.len(), + 1, + "a same-file duplicate must be flagged by the multiplicity check" + ); + assert_eq!(duplicated[0].1.len(), 2); +} + fn duplicate_definitions(found: &BTreeMap>) -> Vec<(&str, &Vec)> { found .iter() @@ -180,12 +221,15 @@ fn collect_inmemory_store_defs(dir: &Path, out: &mut BTreeMap) { +/// Returns every occurrence in source order (no dedup) so same-file duplicate +/// definitions in different modules stay visible to the multiplicity check. +fn scan_source_for_inmemory_store_defs(source: &str) -> Vec { let stripped = strip_comments_and_strings(source); + let mut out = Vec::new(); for line in stripped.lines() { let trimmed = line.trim_start(); let Some(after_pub) = trimmed.strip_prefix("pub") else { @@ -239,9 +284,10 @@ fn scan_source_for_inmemory_store_defs(source: &str, out: &mut BTreeSet) .take_while(|c| c.is_alphanumeric() || *c == '_') .collect(); if ident.starts_with("InMemory") && ident.ends_with("Store") { - out.insert(ident); + out.push(ident); } } + out } /// Replace line comments, block comments (nested), plain/raw string literal