[jussielo-amd]

Closes ROCMAI-865, ROCMAI-884 and ROCMAI-886.

Where the seed re-home of ROCMAI-865 landed. The ticket also asked to re-home the orphaned proptest seed apps/rocm/proptest-regressions/main.txt. That is already on main: #892 renamed it to apps/rocm/proptest-regressions/model_catalog_text.txt (a 100 percent rename) when it moved the property test into its own file, and main.txt no longer exists. What this PR adds for the ticket is the guard that keeps a seed from being orphaned again, so with it both halves of the ticket are delivered.

Summary

This PR adds two source-scan guards, a clean-up of the JSONL store tests, and a rule that makes CI run the first guard when a pull request moves a property test. All of it is test, CI and documentation tooling: no production code changes and nothing a user of the CLI can observe changes. The three changes are bundled on purpose to cut review overhead; each has its own section below and its own commits.

The two guards are #[cfg(test)] source-scan contract tests in xtask, in the same style as env_mutation_contract.rs and ascii_output_contract.rs, run by cargo test -p xtask with no new subcommand. They share one literal-and-comment stripper (xtask/src/rust_source.rs) so each gets the same answer to "is this text code?".

ROCMAI-865: no orphaned proptest regression seeds

proptest finds a property test's saved failure cases by the source file the proptest! block is written in: a block in <crate>/src/<path>.rs reads and writes <crate>/proptest-regressions/<path>.txt. When a test moves to another file, its seed file is no longer read, and proptest says nothing: the recorded counterexample just stops being replayed. This already happened once during a module split in apps/rocm (a seed named main.txt outlived the format_bytes property test that had moved out of main.rs).

The contract test in xtask/src/proptest_regressions_contract.rs walks the tree (skipping target/ and dot-directories, like its siblings), maps every file under a proptest-regressions/ directory back to the source file proptest derives it from, and fails when that source file is missing or holds no proptest! block in code. A proptest! named in a comment of any kind (including nested block comments) or in a string literal does not count, so a note about where a test moved cannot keep an unread seed alive; the block itself is recognised as the name, the ! and an opening delimiter taken as separate tokens, so proptest ! { .. } and proptest /* note */ ! { .. } count while my_proptest! and proptest != x do not. A stray file that is not a .txt seed is reported too. The failure names the seed, the source file it expected, the likely cause and the fix (git mv the seed to the new file's name when nothing is there yet, or append its lines to the file that exists, prove it is read, or delete it if the test is gone). On current main no seed is orphaned, so the check passes with no data change.

The mapping was checked against proptest itself: for each of the six seed files on main a deliberately unparsable line was appended, the owning tests were run with --nocapture, and proptest printed unparsable line, ignoring for that exact path. The rule is documented in the module organization convention in docs/architecture.md and under Property tests in docs/testing.md, including how to prove a moved seed is read and how a complete move (git mv the seed) differs from a split (copy the cases, append when the file exists).

Limits, stated in the module docs and in docs/testing.md. The mapping assumes src/ is where proptest finds main.rs or lib.rs: a crate with a src/bin/<x>/main.rs that has a property test would be reported as an orphan by mistake, and a property test under a crate's tests/ directory is invisible to the guard; nothing in this tree has either. The guard cannot see a split that git mvd the seed away from tests that still need it, since no file is orphaned then, only missing. It assumes proptest's default persistence (a test with failure_persistence: None writes no seed file), and it confirms that something reads a seed file, not that the seed still reproduces a failure.

Running it on pull requests

The guard lives in xtask, and the affected-crates selection used by Test (affected crates) reaches xtask only through the crates it depends on, so a property test moved inside apps/rocm alone would meet the guard only in the merge queue. cargo xtask affected now also selects xtask when a changed file is under a proptest-regressions directory, or when the diff of a changed Rust file adds or removes a line containing proptest! (git's -G pickaxe with --no-renames, so both ends of a move are found, including a move whose seed was forgotten and so changes no seed file). With no xtask member in the graph the answer is the whole workspace. The pre-push hook's selection goes through the same select function, so it picks up the seed-path rule as well. The rust paths-filter in ci.yml gains **/proptest-regressions/**, since a seed-only rename or deletion moves no Rust file and would otherwise skip every rust-gated job. docs/testing.md describes both rules under CI test selection and under Property tests.

ROCMAI-884: the freeform planner never imports chat

The freeform planner and chat.rs used to import each other, and the cycle was broken by moving what the planner used into leaf modules (prompt_text.rs, rocm_argv.rs, tool_policy.rs) that import nothing from either side. Nothing enforced that: cargo xtask check-crate-edges compares crates, not modules inside apps/rocm, so a later change could add use crate::chat; back to the planner and nothing would fail until someone noticed the cycle by hand.

The contract test in xtask/src/chat_import_contract.rs reads freeform_planner.rs and the three leaf modules and fails, naming file:line, if any of them reaches chat in non-test code. It flags crate::chat, super::chat and self::chat as a path (a use or an inline call), chat as a member of a use crate::{..} group, and mod chat, and treats a raw identifier (r#chat) as chat. It does not flag identifiers or prose that merely contain the word (chat_cli_arg_value, comments, string literals). The one exemption is a #[cfg(test)] mod name { .. } block (or mod name;): test modules are removed by bracket matching over the text the shared stripper has already blanked, so a brace inside a string cannot move where a module appears to end, and a #[cfg(test)] on anything else (a use, a function, a field) exempts nothing and is scanned like production code, which can only fail toward a false alarm. If one of the four files is renamed, the test fails on the missing path with a message to update GUARDED. On current main none of the four files imports chat in any code. The rule is described in a new section of docs/testing.md and in one clause of the freeform_planner.rs entry of docs/architecture.md.

Limits, stated in the module docs. A route to chat that never writes the path is not seen: an alias (use crate as root; root::chat::f()), a glob (use crate::*; then chat::f()) or a #[path = ".."] mod pointing at chat.rs. Only #[cfg(test)] exactly is recognised as a test gate (cfg(all(test, ..)) reads as production code, which fails toward a false alarm). Only the four listed files are scanned, not the modules they import.

ROCMAI-886: the JSONL store tests use their own fixtures

The tests in crates/rocm-core/src/jsonl_store.rs drove the generic JSONL store engine through the automation audit store (AuditEventRecord, append_audit_event, load_recent_audit_events), a test-only edge from the generic engine back to one of its users. They now carry their own small record type (Entry, a tag plus padding so a serialized line is about as long as a real store's) and local helpers (entry, entry_line, entry_tags, temp_store, seed_store, append_entry, load_recent_entries, repair_store_with), and name nothing from automation.rs. The automation tests are untouched and keep covering the automation-specific stores through the audit fixtures in test_support.rs, whose module comment now says so. Every previous leaf test name is kept and each test asserts the same behaviour. The padding in Entry is deliberate: the rotation tests size their byte caps against a realistic line length and the torn-tail tests cut the first 20 bytes of a line, so a minimal record would have changed what they exercise; the store is named events.jsonl because one test asserts the rotation warning names that file.

Two things are new in the tests. The load_recent_entries helper reads through 4 generations, which mirrors EVENT_LOG_ROTATED_GENERATIONS in automation.rs; it stays a literal so the engine's tests keep no edge to automation.rs, and its comment names the coupling so a change to the production constant is a conscious decision. And a_torn_live_file_at_the_cap_is_rotated_whole_and_the_new_record_starts_a_clean_file covers what the documented append order says: a live file at the cap that ends in torn debris is rotated aside whole, before its tail is inspected, so the rotated copy keeps its debris, the live file starts clean with only the new record, and a reader spans both. The torn-tail repair tests append with no rotation policy and the rotation tests never seed debris, so this is the test where the two meet.

docs/architecture.md no longer describes a test-only edge for jsonl_store.rs, and docs/testing.md notes that the engine's tests use a record type of their own.

What changed

Item	Where
Seed guard every_proptest_regression_file_belongs_to_a_property_test_that_reads_it and its fixture tests	new xtask/src/proptest_regressions_contract.rs, registered in xtask/src/main.rs
The literal and comment stripper, shared by the contracts	new xtask/src/rust_source.rs (#[cfg(test)]), used by env_mutation_contract.rs, proptest_regressions_contract.rs and chat_import_contract.rs
Planner guard the_freeform_planner_and_its_leaf_modules_never_import_chat and its fixtures	new xtask/src/chat_import_contract.rs, registered in xtask/src/main.rs
xtask selected for a changed seed file or a changed proptest! line, with tests over the pure rule and a throwaway git repository	xtask/src/affected.rs, the rust filter in .github/workflows/ci.yml
JSONL store tests over a local record type, the torn-file-at-the-cap test, the generations comment	crates/rocm-core/src/jsonl_store.rs, module comment in crates/rocm-core/src/test_support.rs
The carry-the-seeds rule, the planner import rule, the selection rules, the JSONL fixtures note	docs/testing.md, docs/architecture.md

Verification

Run on the final branch: cargo fmt --all --check; cargo clippy --locked -p xtask -p rocm-core --all-targets -- -D warnings; cargo test -p xtask --no-fail-fast (600 passed); cargo test -p rocm-core --lib jsonl (26 passed); cargo xtask manifest --check; cargo xtask check-crate-edges; cargo xtask lychee; the leak scan of the diff; a conflict-marker grep. cargo xtask affected --base origin/main runs end to end.

Shown red when the target is broken. For the selection rule: removing the seed-path condition turns a_changed_proptest_seed_file_selects_the_xtask_guard and a_proptest_change_without_an_xtask_member_runs_the_workspace red; removing the source-list condition turns a_changed_proptest_source_selects_the_xtask_guard red; dropping the -G option or the *.rs pathspec from the git call turns proptest_source_files_lists_rust_files_whose_diff_touches_a_proptest_block red. For the torn-file test: skipping the rotation when the live file ends in debris turns it red and no other JSONL test. For the seed guard, adding a copy of the historical apps/rocm/proptest-regressions/main.txt makes the contract test print proptest would read it for apps/rocm/src/main.rs, which has no proptest! block, and removing it turns it green; replacing the tokenized proptest! recognition with a plain line check turns the comment and string-literal tests red. For the planner guard, adding use crate::chat; to freeform_planner.rs fails the contract test with apps/rocm/src/freeform_planner.rs:<line>: a path through crate::chat, and not removing test modules, letting a non-module #[cfg(test)] gate swallow the rest of the file, and skipping the stripper each turn named tests red. The earlier JSONL engine tests were mutation-checked (skipping rotation, dropping the store lock, changing the lock file name, disabling the shrink retry, dropping the generation shift, cutting the torn tail one byte too far) and each turned the covering tests red.

Not run: the full workspace gate, cargo xtask e2e, cargo xtask coverage and scripts/smoke_local.py (every change is test or documentation tooling; CI carries them). The Windows cross-check of the cfg(windows) JSONL test append_repairs_a_torn_tail_while_another_handle_blocks_replacing_the_store could not run on a Linux host (ring's build script needs lib.exe); it was edited the same way as its Unix sibling and the Windows CI job exercises it. cargo xtask check-module-size reports chat.rs and service_launch.rs over the limit; neither is touched here and the check is report-only today.

User-visible behaviour

None, so there is no Gherkin scenario: this is test, CI and documentation tooling plus a rewrite of a #[cfg(test)] module. No command, output, exit code, file or runtime selection changes, and no printed message is added or edited.

Notes for reviewers

Reading order: the proptest guard (the guard, the shared stripper, the documentation), the planner guard, the JSONL fixtures commit, then the selection rule and the torn-file test as the last two commits, with merges of main in between where it moved. docs/architecture.md and docs/testing.md are the files other open modularization and docs pull requests tend to touch, so expect a conflict on the lines this change adds there if another PR edits them.

[Copilot]

🟡 Changes recommended

The guard can mistake commented-out macros for active property tests, and the seed-migration guidance can remove saved-case replay from tests left behind during partial moves.

2 open issues

• Property macro detection ignores trailing and block comments · New
• Seed migration advice breaks partial property-test splits · New

What changed in this PR

Adds an xtask contract test to detect proptest regression files left unread after property tests move or source files are deleted. This extends the repository’s existing source-scan checks without changing CLI behavior.

Changes:

• Maps regression files to their expected sources and reports missing sources, missing property-test blocks, and stray files.
• Adds fixtures covering path mapping, orphan detection, comments, diagnostics, and filesystem traversal.
• Documents moving saved cases with property tests and verifying that proptest reads them.

Verification was limited to source inspection; tests were not run.

File	Description
xtask/​src/​proptest_regressions_contract.rs	Adds the regression-file contract check and fixture tests.
xtask/​src/​main.rs	Registers the new contract module.
docs/​testing.md	Explains the check, its CI scope, and seed-replay verification.
docs/​architecture.md	Adds saved-case handling guidance for module moves.

_{Review effort: Balanced}

[Copilot]

🟡 Changes recommended

Two scanner defects remain: function-pointer fields can hide production chat dependencies, and whitespace or comments before a proptest macro’s ! can cause live seeds to be reported as orphaned.

3 open issues

• Cfg(test) function-pointer fields hide the following production field · New
• Proptest orphan detection misses whitespace or comments before ! · New
• Seed migration advice breaks partial property-test splits

1 resolved since last review

• Property macro detection ignores trailing and block comments

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

The scanner misses production dependencies when cfg-gated generic parameters incorrectly extend exemptions and when nested import groups lose the crate root context.

1 open issue

• Seed migration advice breaks partial property-test splits

2 previously missed issues

In code that hasn't changed since last review

Stop cfg exemption at the generic parameter list's closing >

xtask/​src/​chat_import_contract.rs:354

For fn production<#[cfg(test)] T>() { crate::chat::f(); }, the function body remains production code, but the scanner hides it because the generic parameter's closing > does not end the exemption. Track the enclosing generic-parameter list and stop at its closing >, including when the gated parameter has no trailing comma. Add function and struct fixtures to ensure gated generic parameters cannot hide production dependencies.

Preserve crate root context in nested grouped imports

xtask/​src/​chat_import_contract.rs:421

A valid grouped import such as use crate::{{chat::T}}; clears the root flag at the inner {, so the scanner reports no offense even though it imports directly from crate::chat. This lets the dependency this contract is meant to prevent pass unnoticed. Preserve the parent root context for nested groups without a path prefix, while still resetting it for groups such as providers::{...}, and add a fixture for this spelling.

2 resolved since last review

• Proptest orphan detection misses whitespace or comments before !
• Cfg(test) function-pointer fields hide the following production field

_{Review effort: Balanced}

Addressed in 74cf6a3 and f24acb6 (smaller planner guard, seed-layout wording and limits), with replies on the threads and on the PR for the bundling and ticket questions.

[juhovainio]

No blocking findings from my read of the diff. Approving, with a few non-blocking notes:

• The PR bundles three separate changes (the proptest seed guard, the planner chat-import guard, and the JSONL test rewrite). Splitting them would make each easier to review and revert; worth considering next time.
• The proptest seed guard only runs on PRs that touch xtask or its dependencies, so a test moved inside apps/rocm alone would be caught only in the merge queue. Consider having the affected-crates selector include xtask when proptest-regressions/** changes.
• The PR description narrates review history ("an earlier version...") rather than describing the final diff; a short rewrite from the final state would help future readers.
• Low-confidence observation: the torn-tail JSONL tests now append with no rotation policy, so repair and rotation are no longer exercised together there.

One small inline nit below. This reflects a read of the diff plus the CI results at the time of review.

[juhovainio]

Nit: 4 generations matches EVENT_LOG_ROTATED_GENERATIONS in automation.rs; consider naming that coupling in the comment so a change to the production constant is a conscious decision here.

[jussielo-amd]

Good point, thanks. I kept the 4 as a literal rather than referencing EVENT_LOG_ROTATED_GENERATIONS, because the point of the rewrite is that the engine's tests carry no edge to automation.rs, but the doc comment on load_recent_entries now names the coupling to that constant and says a change to the production value should be matched here on purpose. The change is in a329b35.

[jussielo-amd]

Thanks for the approval and the notes.

On bundling the three changes: agreed that three separate PRs would be easier to revert one by one. I bundled them to cut review overhead, since all three are test and documentation tooling with no production change, and the commits stay grouped per change so each is still readable on its own. I have left it bundled.

On the proptest seed guard only running when xtask is affected: fixed in 2fc383e. cargo xtask affected now selects xtask when a changed file is under a proptest-regressions directory, and also when the diff of a changed Rust file adds or removes a line containing proptest! (found with git diff -G, so both ends of a move are listed, including a move whose seed was forgotten and so changes no seed file). The rust paths-filter in ci.yml gains **/proptest-regressions/**, because a seed-only rename or deletion moves no Rust file and would otherwise have skipped the rust-gated jobs altogether. Unit tests cover the pure rule and the git call (against a throwaway repository), and each turns red when its rule is removed. The paragraph in docs/testing.md that said such a move is caught only in the merge queue now describes the new rules instead.

On the PR description: rewritten from the final diff, without the earlier-version narration; it now has a section for each of the three changes, one for the selection rule, the verification and the known limits.

On repair and rotation no longer being exercised together: you are right that nothing covered it. The torn-tail tests never reached the rotation cap even before the rewrite (the audit store's policy was passed but never triggered), and the rotation tests never seeded debris, so the documented order, where a live file at the cap that ends in torn debris is rotated aside whole before its tail is inspected, had no test. a329b35 adds a_torn_live_file_at_the_cap_is_rotated_whole_and_the_new_record_starts_a_clean_file, which asserts that the rotated copy keeps its debris untouched, the live file holds only the new record, and a reader spans both. Skipping the rotation for a torn file turns it red and no other JSONL test.

[Copilot]

🟡 Changes recommended

The affected-crate pickaxe misses valid proptest macro spellings with whitespace or comments before !, allowing moved unseeded blocks to bypass the new test-selection safeguard.

3 open issues

• Seed guard misses spaced or commented proptest macro syntax · New
• Preserve root context in nested use groups
• Clarify that only cfg(test) modules are exempt

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

The affected-crates pickaxe misses supported separated-token proptest invocations and skips the seed guard, while the import scanner permits prohibited root chat imports through nested use groups.

3 open issues

• Seed guard misses spaced or commented proptest macro syntax
• Preserve root context in nested use groups
• Clarify that only cfg(test) modules are exempt

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

The affected-source pickaxe misses supported proptest invocations with whitespace or comments before !, allowing test moves that leave orphaned seeds to skip the xtask orphan check.

3 open issues

• Seed guard misses spaced or commented proptest macro syntax
• Preserve root context in nested use groups
• Clarify that only cfg(test) modules are exempt

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

The affected-crates pickaxe misses valid noncontiguous proptest invocations, so moving a block without its seed can omit xtask from CI and bypass the orphaned-seed guard.

3 open issues

• Seed guard misses spaced or commented proptest macro syntax
• Preserve root context in nested use groups
• Clarify that only cfg(test) modules are exempt

_{Review effort: Balanced}