Repository navigation
test(xtask): fail when the freeform planner or its leaf modules import chat - #925
Closed
jussielo-amd wants to merge 4 commits into
Closed
jussielo-amd wants to merge 4 commits into
jussielo-amd wants to merge 4 commits into
Conversation
…t chat
chat.rs calls the freeform planner, so a planner that imported chat back
would re-form a module cycle. The cycle was broken by moving what the
planner used into leaf modules (prompt_text.rs, rocm_argv.rs,
tool_policy.rs), but check-crate-edges compares crates, so nothing
stopped a later change from putting the import back.
Add a source-scan contract test in the style of the environment-mutation
and ASCII-output contracts. It reads freeform_planner.rs and the three
leaf modules, blanks comments and literals with the environment contract's
existing stripper, drops #[cfg(test)] items by brace matching, and reports
file:line for any remaining crate::chat / super::chat / self::chat path,
chat member of a use crate::{..} group, or mod chat. The stripper is made
pub(crate) for this; nothing else in that file changes.
Tooling only: no change to the shipped binaries.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
jussielo-amd
marked this pull request as ready for review
November 5, 2026 17:44
jussielo-amd
requested review from
siloteemu
and
a balanced review from Copilot
November 5, 2026 17:44
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The contract check can miss forbidden planner-to-chat dependencies because raw identifiers bypass the guard and test-only struct fields can incorrectly exempt subsequent production code.
2 open issues
What changed in this PR
Adds an xtask contract test to protect the dependency boundary between the freeform planner, its shared leaf modules, and chat. No shipped CLI behavior changes.
Changes:
- Scans four guarded files for production references to
chat, with file-and-line diagnostics and regression fixtures. - Reuses the existing comment/literal stripping helper.
- Documents the dependency rule and how to run the guard.
Verification was limited to source inspection; project checks were not run.
| File | Description |
|---|---|
xtask/src/main.rs |
Registers the new contract module. |
xtask/src/env_mutation_contract.rs |
Exposes the existing stripping helper for reuse. |
xtask/src/chat_import_contract.rs |
Adds the dependency scanner and regression tests. |
docs/testing.md |
Documents the guard, scope, and test command. |
docs/architecture.md |
Notes enforcement of the planner’s dependency boundary. |
Review effort: Balanced
Comment on lines
+94
to
+95
| } else if c.is_alphanumeric() || c == '_' { | ||
| let start = i; |
Comment on lines
+194
to
+195
| if let Some(len) = cfg_test_attribute_len(tokens, i) { | ||
| i = item_end(tokens, i + len); |
…ports-chat Signed-off-by: Jussi Elo <jussi.elo@amd.com> # Conflicts: # docs/architecture.md
…ort guard
The guard's tokenizer split r#chat into the identifier r, a # and the identifier chat, so use crate::r#chat;, use crate::{r#chat}; and mod r#chat; named the chat module and passed. A raw identifier is now one token carrying the plain name, so every form the guard flags has a raw twin that fails it too. A raw identifier that spells a keyword (r#type) keeps its prefix, so it is never mistaken for the keyword.
The fixtures cover the raw forms of a path, a group member, a nested group, a module declaration and an inline call, each expecting an offense; the look-alikes that are not the module (a raw chat under another path, a local variable, a raw identifier that merely starts with chat) stay unflagged; and a tokenizer test pins the single-token and raw-keyword behaviour. The raw-form fixtures are red without the change.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
…ment it gates
The planner import guard removed everything a #[cfg(test)] attribute covered by treating it as a gate on an item: it ran to the first semicolon or to the end of the first brace block that followed. On a struct field, an enum variant, a parameter, a tail expression, a struct-literal field or a match arm there is neither, so the exemption ran on past the closing brace of the struct or the parenthesis of the parameter list and swallowed the next function. struct State { #[cfg(test)] marker: () } followed by fn production() { crate::chat::f(); } produced no offense.
A gate now covers one thing and ends with it: at a semicolon, at the comma that separates a list element from the next (not a comma inside generic arguments), at the end of the first brace block, or, without consuming it, at a closer that has no opener inside the gate, which is the closing brace of the struct, enum, block or match the element sits in or the closing parenthesis of its parameter list. Whether the gate is on a list element or on an item or statement is read from the first token after the attributes and visibility, because the commas in an item's generics and where clause are not its end.
The fixtures cover a test-only field (last, with a comma, with stacked attributes and a visibility, with generic arguments, in a tuple struct), a variant (bare, tuple, struct-like), a parameter, a tail expression, a struct-literal field and a match arm, each followed by production code that must still be reported; the item and statement forms that must stay exempt despite commas in their generics; and a field called union. Eleven of the fifteen field and variant cases are red on the previous scanner. The module docs and docs/testing.md say what a gate covers and list the routes to chat that a path scan does not see (an alias, a glob import, a path attribute).
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
Collaborator
Author
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
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.
Summary
This delivers ROCMAI-884. The freeform planner and
chat.rsused 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 enforces that rule today:cargo xtask check-crate-edgescompares crates, not modules insideapps/rocm, so a later change could adduse crate::chat;back to the planner and nothing would fail until someone noticed the cycle by hand.This adds a source-scan contract test in
xtask, in the same shape as the environment-mutation and ASCII-output contracts (a#[cfg(test)]module run bycargo test -p xtask, no subcommand and no new machinery). It readsfreeform_planner.rsand the three leaf modules and fails, namingfile:line, if any of them reacheschatin non-test code. It flagscrate::chat,super::chatandself::chatas a path (auseor an inline call),chatas a member of ause crate::{..}group, andmod chat. It does not flag identifiers or prose that merely contain the word (chat_cli_arg_value, comments, string literals), and it ignores#[cfg(test)]items, so amod testsblock may still usechat.On current main the premise holds: none of the four files imports
chatin any code (none of their test modules does either), so the guard passes on the real tree.What moved
Nothing moved. New and changed files:
xtask/src/chat_import_contract.rs, registered inxtask/src/main.rsstrip_literals_and_comments(comment and literal blanking that preserves line numbers)xtask/src/env_mutation_contract.rs; itstestsmodule and that one function becomepub(crate), nothing else in the file changesdocs/testing.md, and one clause in thefreeform_planner.rsentry ofdocs/architecture.mdTest items are removed by brace matching over the stripped text, so a brace inside a string literal cannot change where a
#[cfg(test)]item appears to end. No existing helper did that for a file as a whole (the two existing contracts each track test regions inside their own scanners), so it is a small function in the new module.Verification
Ran:
cargo fmt --all --check;cargo clippy -p xtask --all-targets -- -D warnings;cargo test -p xtask --no-fail-fast(472 passed, 0 failed, of which 8 are the new tests);cargo xtask manifest --check;cargo xtask check-crate-edges; the lychee pre-commit hook on the changed docs; the leak scan againstorigin/main(the only hits are the word "module-private" in an existing architecture.md sentence this change touches);git merge-tree --write-tree origin/main HEADexits 0.Red on the real tree: temporarily adding
use crate::chat;tofreeform_planner.rsmakesthe_freeform_planner_and_its_leaf_modules_never_import_chatfail withapps/rocm/src/freeform_planner.rs:54: a path through crate::chat ..., and addingchatto theuse crate::{..}group intool_policy.rsfails it withapps/rocm/src/tool_policy.rs:33: chat as a member of a use crate::{..} group. Both edits were reverted.Red when the guard is broken: each of these single edits to the scanner turns at least one new test red (and was reverted): skipping the removal of test items, ignoring
crate::chatpaths, ignoringchatin ause crate::{..}group, treating every{group as a root group, ignoringmod chat, droppingselfas a path root, and ignoring bracket nesting when finding the end of an item. Two other edits I tried (keeping or dropping the;after a test item's closing brace, and skipping stacked attributes) left the tests green because they do not change what the scan reports, so I deleted that code instead of keeping it.The fixtures cover: the forbidden import reported with its file and line, every spelling of it, a
#[cfg(test)] mod testsor single#[cfg(test)]item referencingchatpassing, production code after a test item still being scanned, braces inside literals not moving the end of a test item, and look-alikes (comments, strings,chat_cli_arg_value,providers::chat, a local namedchat) not being flagged. A test also runs the scanner over the realfreeform_planner.rstext with an import prepended and appended, so it is proven on the real file's structure and not only on small strings.Not run:
cargo clippyagainst the Windows target (xtask has no platform-gated code and is not built for it), the full workspace gate,cargo xtask e2eandscripts/smoke_local.py(CI carries them).User-visible behaviour
None, so no Gherkin scenario applies: this is tooling only. No shipped binary or CLI output changes, and no printed message is added or edited.
Notes for reviewers
One commit. Suggested reading order: the module docs of
xtask/src/chat_import_contract.rs(what is and is not scanned, and the known limits), thenchat_imports_in_production_code, then the fixtures, then the docs. Known limits are written in the module docs: only#[cfg(test)]exactly is recognised as a test gate (cfg(all(test, ..))reads as production code, which fails toward a false alarm), and only the four listed files are scanned, not the modules they import (none oftherock,service_launch,services_text,shell_quote,providersordriver_installimportschattoday; they can be added toGUARDEDif that ever needs pinning). If one of the four files is renamed the test fails on the missing path with a message to updateGUARDED, instead of silently dropping the file from the guard.The change touches
docs/architecture.md, which modularization PRs touch often, so expect a conflict on thefreeform_planner.rsentry if another PR edits that line.