From e97e463dfc85497bd270470507468ab6693c8857 Mon Sep 17 00:00:00 2001 From: Roman Sirokov Date: Tue, 29 Sep 2026 12:44:02 +0300 Subject: [PATCH] docs: assert a CLI message together with the behavior it describes (ROCMAI-383) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four times in a month a command printed a claim about its own actions that the code did not perform: a "no existing SDK" line over an install that displaced the active default, an "Approved by --yes" line on a path with no such flag, and uninstall advice naming a command that re-hit the same unparseable record. All four are now fixed, but nothing stops the next one — the message and the code it describes sit in different functions, and no convention asks for the two to be asserted together. The fourth instance is the one that shapes this rule. Its wording was pinned by a unit test, and the pin held the false claim in place while CI reported green, so "add a pinning test" on its own is not the remedy. AGENTS.md §3 now names the three shapes that need it, and the e2e README carries the same rule for Then steps that match printed output. Signed-off-by: Roman Sirokov --- .github/pull_request_template.md | 1 + AGENTS.md | 25 +++++++++++++++++++++++++ tests/e2e-cucumber/README.md | 1 + 3 files changed, 27 insertions(+) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 7fb8e45ae..a5f60e1a2 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -6,3 +6,4 @@ SPDX-License-Identifier: MIT - [ ] If this PR fixes a bug, searched `tests/e2e-cucumber/expectations.toml` for the fixed ticket ID and removed/narrowed any now-stale xfail rows. - [ ] If this PR adds a new subcommand or subsystem, its domain implementation lives in its own file per `docs/architecture.md` (the clap declaration and dispatch wiring staying in `main.rs`/`lib.rs` is expected, not a violation). +- [ ] Every new or changed user-facing message was read against the code path that runs after it, and its test asserts the resulting state — not only the wording — per AGENTS.md §3. Covers claims of an outcome, remediation advice naming a command, and promises that something will *not* happen. diff --git a/AGENTS.md b/AGENTS.md index 72c03279a..842f2ab19 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -101,6 +101,31 @@ definitions when no existing scenario already covers it: - purely internal changes (refactors, CI plumbing, docs) do not need one; say why in the PR text rather than leaving it unexplained +**A message about the CLI's own behavior is asserted together with the behavior.** When a +change adds or edits a line the CLI prints about what it just did, what it will do next, +or what the user must do to recover, the covering test asserts the message *and* the +resulting state in the same test. Three shapes need this: + +- claims of an outcome ("this becomes the active default runtime", "nothing was saved") +- remediation advice naming a command — the named command must exist, accept those flags, + and actually clear the condition that printed it +- promises that something will *not* happen ("no driver commands will be executed", + "never stops servers automatically"), which no happy-path test exercises + +A test that only pins the wording certifies the string, not the truth of it, and a pin +over a false claim holds the claim in place. Where the text genuinely has to be pinned on +its own, the assertion carries a comment naming the test that proves the behavior — see +`setup_reset_cli_output_is_plain_and_persists_first_time_prompt` in `apps/rocm/src/main.rs`, +which pins the onboarding line and points at +`startup_focus_gate_only_opens_onboarding_for_explicit_setup_focus` for the behavior +itself. + +The message and the code it describes are usually in different functions and often +different files, so nothing links them by construction. Where the printed line can be +derived from the same value the branch is taken on — as `preapproved_install_line` in +`apps/rocm/src/therock.rs` derives it from the approval source — prefer that: a message +computed from the decision cannot disagree with it. + ## 4) Live State Verification Before Any External Claim Before each stateful decision or public status update: diff --git a/tests/e2e-cucumber/README.md b/tests/e2e-cucumber/README.md index 751f7b0cf..06acf0848 100644 --- a/tests/e2e-cucumber/README.md +++ b/tests/e2e-cucumber/README.md @@ -241,3 +241,4 @@ The `.feature` file is both the spec and the test input — cucumber reads it at - **Isolated state.** Each scenario uses isolated config, data, and cache directories. Tests never touch `~/.rocm`. - **Behavioral language.** Feature files describe what users care about, not implementation details. How steps are implemented (mock vs real, which port, which API) stays in the step functions. - **OS-assigned ports.** The mock server binds to `127.0.0.1:0` to avoid port conflicts between tests. +- **A message is asserted with the state it describes.** A `Then` step matching printed output ("no existing SDK was found", "becomes the active default runtime", "stop them with `rocm services stop `") asserts the resulting state in the same scenario — the file that was or was not written, the runtime that is or is not active, the advice's named command actually clearing the condition. Matching the text alone certifies the wording, not the truth of it, which is how a scenario passes over a CLI that is wrong about what it did. See AGENTS.md §3.