[jussielo-amd]

The exactly-once planning guarantee for rocm --yes <request> and the natural_language_plan tool was proven only by two unit regressions in providers.rs. Nothing exercised it through the real binary, which AGENTS.md section 3 requires for user-observable behavior. This adds that coverage. It is test-only apart from two unit-test fixtures and one visibility change: StructuredRequestPlan.request becomes pub(crate), like its approval and actions siblings, so the execution fixture can assert on it.

The e2e mock server can now serve a scripted sequence of chat completions, so each request gets a different tool-call reply, and it records every request it receives. A request past the end of the script gets HTTP 500 and is still recorded, so over-asking is loud and counted. ServiceRecordOptions gains an inference_verified option that plants the inference-verified latch. Without it the CLI's own readiness probe consumes scripted reply 0 and the planner receives the wrong plan. A new step file, planner_steps.rs, wires config set-planner-provider local to the mock.

Three scenarios in chat.feature cover the user-visible behavior:

• chat-10 (@id:chat-freeform-yes-plans-once): rocm --yes "serve a model" asks the planning assistant exactly once, shows the assistant's own plan (the whole next_tool_call: rocm examine line, not a substring), and is refused rather than run.
• chat-11 (@id:chat-assistant-plan-tool-plans-once): the chat assistant's plan tool asks the planner exactly once.
• chat-12 (@id:chat-plan-tool-text-matches-action): rocm mcp-call natural_language_plan returns the same first plan as structuredContent.text and as action.args, and provider_assisted is true.

All three run on the mock lane with no GPU, on both Linux and Windows e2e lanes (loopback HTTP only). --yes refuses provider-assisted plans before dispatch, so for that surface the agreement between plan shown and action executed stays covered by the two unit tests. The plan tool's text/action agreement is covered by chat-12.

While adding chat-10 it turned out that the docs used rocm --yes "start a local model" as the example of a refused provider-assisted plan. That request resolves deterministically at high confidence, never reaches the provider, and under --yes would run the serve it plans. docs/testing.md and docs/manual-testing.md now use serve a model, and say that only requests the deterministic planner cannot settle reach the provider. The two unit-test fixtures that fed the provider path that request now use serve a model, and each now asserts plan.request so the string is tied to an observable result. Two small unit tests pin what the black-box harness has to repeat: the planner prompt wording it matches on (provider_planner_prompt_keeps_the_wording_the_e2e_harness_matches) and the built-in assistant model id it plants (builtin_assistant_model_id_matches_the_e2e_harness), so drift fails in the rocm crate and not as an opaque scenario failure.

Mutation checks, each shown red and then reverted:

• Re-planning in run_freeform makes chat-10 fail with 2 planning requests.
• Rendering from a fresh plan in run_freeform makes chat-10 fail on the next_tool_call line.
• Planning twice in natural_language_plan makes chat-11 and chat-12 fail on the request count. Taking the text from the first build and the action from the second makes chat-12 fail on the text/action comparison.
• Deleting the provider_assisted refusal in validate_freeform_execution_action makes chat-10 fail at the exit-code assertion, because the command then exits 0; its stdout carries the execution header the step also checks for.
• Rewording the planner prompt's opening line or its User request: label, changing the built-in assistant model id, and not storing the request on a provider plan each fail their unit test.
• Renaming the latch key in the writer, the matcher marker, and the stub-text assertion each fail their own unit test or scenario.

Not verified: the Windows e2e lane was not run locally, and the new code is platform-neutral by reading. This is a test-only change, so docs/architecture.md and the user-facing CLI docs need no update beyond the manual-testing correction above.

[Copilot]

🟢 Approval recommended

No active review issues were identified in the CLI planning coverage changes, though tests were not run during review and Windows e2e was not verified locally.

No open issues 🎉

What changed in this PR

Adds end-to-end coverage for exactly-once provider-assisted planning through the real CLI, extending the existing unit regressions without changing production behavior.

Changes:

• Adds scripted mock chat replies, request history, and an inference-verified fixture option.
• Adds three scenarios covering freeform --yes refusal, chat-triggered planning, and plan text/action agreement.
• Corrects ambiguous-request examples in documentation and two unit-test fixtures.

Tests were not run during this review. The PR also notes that the Windows e2e lane was not verified locally.

File	Description
tests/​e2e-cucumber/​tests/​e2e/​planner_steps.rs	Adds planner setup, invocation, and assertion steps.
tests/​e2e-cucumber/​tests/​e2e.rs	Registers the planner step module.
tests/​e2e-cucumber/​src/​mock_server.rs	Adds scripted replies, request recording, latch support, and unit tests.
tests/​e2e-cucumber/​README.md	Documents scripted mocks and harness testing conventions.
tests/​e2e-cucumber/​features/​chat.feature	Adds three planning regression scenarios.
tests/​e2e-cucumber/​Cargo.toml	Clarifies the existing dev-dependency’s uses in comments only.
docs/​testing.md	Documents coverage, commands, and corrected provider-planning examples.
docs/​manual-testing.md	Corrects the request used to exercise provider-assisted planning.
apps/​rocm/​src/​freeform_planner.rs	Updates a provider-planner unit-test request fixture.
apps/​rocm/​src/​freeform_execution.rs	Updates a provider-assisted execution unit-test request fixture.

_{Review effort: Balanced}

[nowycondro]

This automated review never files a GitHub approval or change request; the merge decision stays with a human reviewer.

Review — no blocking findings

Full review of the whole change (base cc56179ced9dd1be42a3000b2592ace9394dbff1 → head 555dbff087b56c92838a929a3582acada04afb8b).
Implements ROCMAI-730 (e2e coverage for freeform plan dedup through the real CLI/MCP entry points with a sequenced mock provider) — matches the ticket's scope as refined in its comment thread; not claiming to close anything it doesn't deliver.

Blocking

None

Non-blocking

• The two edited unit-test fixtures don't exercise the routing decision their swap is framed around — apps/rocm/src/freeform_planner.rs:1798, apps/rocm/src/freeform_execution.rs:406-410
  Both tests call provider_planner_response_to_plan(request, provider, content) directly with a hand-crafted provider JSON response, bypassing build_freeform_plan_with_context/the deterministic-vs-provider routing decision entirely. Inside provider_planner_response_to_plan, request is only stored into plan.request (freeform_planner.rs:934) and never read, matched, or branched on; neither test asserts on plan.request. So the "start a local model" → "serve a model" swap is behaviorally inert for these two tests — they'd pass identically with either string. This is harmless (nothing is broken or weakened) and the PR description's claim is technically accurate ("fixtures... now use 'serve a model', with the tests unchanged otherwise"), but the implied rationale (keeping the fixture consistent with which requests actually reach the provider) doesn't apply to what these specific tests exercise.
  Confidence 70 · logic · Fix: no action required to merge; optionally drop the now-arbitrary request string from these two fixtures, or add an assertion on plan.request if the intent is to tie the test to the input text.

Decisions for the author

None

Positive signals

• The inference_verified latch on ServiceRecordOptions is well-designed: it defaults to false (preserving every existing non-scripted test's behavior unchanged) and its doc comment states exactly when to set it, so it reads as a deliberate opt-in rather than an easy-to-forget landmine.
• The mutation-testing discipline described in the PR body (re-planning, fresh-plan rendering, double-planning, latch-key rename each shown to fail their target scenario/test, then reverted) is exactly the kind of verification this repo's AGENTS.md §3 asks for, and the central claim it rests on (that "start a local model" resolves deterministically while "serve a model" reaches the provider) was independently traced through the real matcher/confidence logic and confirmed true.

Deployment notes

None

What this covered

Read completely at head: tests/e2e-cucumber/src/mock_server.rs (1253 lines), tests/e2e-cucumber/tests/e2e/planner_steps.rs (402 lines, new), tests/e2e-cucumber/features/chat.feature (152 lines), tests/e2e-cucumber/tests/e2e.rs (diff), apps/rocm/src/freeform_planner.rs (1934 lines), apps/rocm/src/freeform_execution.rs (419/422 lines), docs/testing.md, docs/manual-testing.md, tests/e2e-cucumber/README.md, tests/e2e-cucumber/Cargo.toml — plus surrounding production code these touch (crates/rocm-core/src/net.rs's readiness probe, apps/rocm/src/advised_commands.rs for a stray-fixture check that turned out not to apply, sibling step files for convention). Fanned out across three independent workers grouped by coupling (e2e mock/step/feature contract; freeform planner+execution fixtures and the determinism claim; docs+harness metadata), each reading its files in full; findings consolidated and verified against source by the driver. CI: all 24 checks green (including E2E tests, E2E consolidated report, clippy, windows-build-and-test, coverage, commit-signatures); two checks (Skill checks, Sphinx docs build) correctly skipped as this PR touches neither skills/ nor Sphinx-built docs.

Did not run: history/prior-changes passes as dedicated fan-out workers (folded into the above — git log on touched files showed planner_steps.rs is new with no prior history, and freeform_planner.rs/freeform_execution.rs history was read directly in service of the determinism-claim verification); no review threads or prior discussion were read as input. Design review (Step 7) raised no fundamental-approach or tradeoff items beyond what's noted above.
Not reconciled against any discussion on this PR.

[juhovainio]

Looks good. No blocking findings. The scripted mock's cursor is handed out atomically under the history lock, exhaustion returns 500 and is still recorded, and /models does not consume a script entry. Unit tests cover these plus the latch round-trip through ManagedServiceRecord. Docs are consistent and scope matches ROCMAI-730.

Non-blocking:

• The chat-10 refusal step's negative execution check can pass vacuously (inline). The PR body lists mutation checks for the dedup paths but not for this step; worth noting which mutation (letting --yes run a provider plan) turns chat-10 red.
• PLANNER_PROMPT_MARKER and BUILTIN_ASSISTANT_MODEL_ID are duplicated from the product with nothing tying them together. Drift fails closed, so this is minor; a small unit test in the rocm crate pinning the marker would make it fail at the source.
• The Windows e2e lane was not run (stated in the PR body). The code reads as platform neutral.

[juhovainio]

This negative check passes vacuously if the section header wording changes. Consider noting which mutation (letting --yes run a provider plan) was shown to turn chat-10 red, since the PR body lists mutations only for the dedup paths.

[jussielo-amd]

Checked: deleting the provider_assisted refusal in validate_freeform_execution_action turns chat-10 red at the exit-code assertion, because the command then exits 0, and its stdout carries the execution header this check looks for, so the needle is a real string the CLI prints when a plan runs. If the header wording ever changed, the exit-code assertion would still catch that regression. I recorded the mutation in the PR body and in a comment on the step in 693ed5f, which also pins the planner prompt wording and the built-in assistant model id that the harness repeats (the other two non-blocking points).

[nowycondro]

🤖 Automated review (Claude Code) · 04f5945

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Review — no blocking findings

Full review of the whole change (base cc56179ced9dd1be42a3000b2592ace9394dbff1 → head 04f59458d4160fdbb70672ce7f2a1dbf0ec94c05).
Implements ROCMAI-730 (e2e coverage for the exactly-once freeform-plan-dedup guarantee through the real CLI/MCP entry points, via a sequenced mock chat provider) as scoped by the ticket's own comment thread — the "shown plan equals executed action" half is explicitly left to the existing unit tests since it isn't black-box observable on the --yes surface (refused before dispatch). The PR description matches this scope accurately; it does not overclaim.

Blocking

None

Non-blocking

None

Decisions for the author

• freeform_execution.rs's edited fixture doesn't assert on plan.request, unlike its twin in freeform_planner.rs — non-blocking-improvement, apps/rocm/src/freeform_execution.rs:406-410 vs apps/rocm/src/freeform_planner.rs:1798-1799.
  Both unit tests swap their fixture string from "start a local model" to "serve a model". In freeform_planner.rs the test now also asserts assert_eq!(plan.request, "serve a model"), so the swap is tied to an observable outcome. The freeform_execution.rs copy of the same swap has no equivalent assertion — provider_planner_response_to_plan's request parameter is only stored (freeform_planner.rs:932, request.trim().to_owned()), never matched or branched on inside that function, so without an assertion the string's exact value is cosmetic there. Harmless (the test still correctly proves validate_freeform_execution_action rejects provider-assisted plans), but applying the same fix pattern to both copies would make the pairing complete.
• Black-box test harness duplicates production constants/strings (PLANNER_PROMPT_MARKER, BUILTIN_ASSISTANT_MODEL_ID in tests/e2e-cucumber/tests/e2e/planner_steps.rs) — tradeoff, already weighed in the code's own comments.
  The harness can't import apps/rocm/src/providers.rs's real constants (black-box design principle in tests/e2e-cucumber/README.md), so it repeats their literal values. If the product's wording drifts, the scenario fails closed (no match → test fails) rather than silently passing against stale expectations — a deliberate, documented choice rather than an oversight, and reasonable given the alternative (importing production code into the black-box harness) is worse.

Positive signals

• The scripted-reply cursor in handle_chat (tests/e2e-cucumber/src/mock_server.rs) assigns the script index from history.len() - 1 under the same lock acquisition that pushes the request, with no intervening .await; two concurrent requests cannot be handed the same reply or skip one. Verified by reading the function directly, not just the diff hunk.
• inference_verified is a real, non-cosmetic latch: traced end to end from ServiceRecordOptions::inference_verified → the JSON inference_verified_at_unix_ms field written by write_service_record_with → rocm_core::ManagedServiceRecord → read by production code in crates/rocm-core/src/net.rs's managed_service_endpoint_readiness, which returns EndpointReadiness::Serving immediately when the field is Some, skipping the CLI's own completion probe exactly as the field's doc comment claims. Defaults to false, so every pre-existing non-scripted scenario is unaffected.
• The central factual claim the whole fixture/docs swap rests on — that "start a local model" resolves deterministically at high confidence while "serve a model" does not — was independently traced through the real matcher (infer_recommended_assistant_model_from_request explicitly matches the former; infer_model_from_request extracts "a model" from the latter, which generic_model_phrase recognizes as unresolved) and confirmed true; a pre-existing unit test (hybrid_planner_defaults_generic_local_assistant_to_validated_qwen) already independently proves the deterministic side.
• The "exactly once" and "refused rather than run" assertions in planner_steps.rs are not vacuous: the former filters the server's real recorded-request history for a specific prompt-prefix match and asserts count == 1; the latter combines a non-zero exit code with the literal absence of the execution header text that render_freeform_execution_header only ever emits when a plan actually executes, plus the specific refusal wording in stderr.
• Impact analysis is clean: a repo-wide grep for the old "start a local model" example confirms every remaining occurrence is a different, legitimate use (the deterministic-matcher literal itself, an unrelated EXAMPLES-parsing test fixture, and explanatory contrast text in the updated docs) — nothing was left half-updated.

Deployment notes

None — test-only change plus two unit-test fixtures and documentation; no production code path is touched.

What this covered

Read completely at head: tests/e2e-cucumber/src/mock_server.rs (full diff + surrounding function bodies), tests/e2e-cucumber/tests/e2e/planner_steps.rs (new, 402 lines), tests/e2e-cucumber/features/chat.feature (diff), tests/e2e-cucumber/tests/e2e.rs (diff), apps/rocm/src/freeform_planner.rs and apps/rocm/src/freeform_execution.rs (diffs plus the full functions/tests they touch, including provider_planner_response_to_plan and the deterministic matcher helpers), docs/testing.md, docs/manual-testing.md, tests/e2e-cucumber/README.md, tests/e2e-cucumber/Cargo.toml (all diffs in full) — plus crates/rocm-core/src/net.rs's readiness-probe code and apps/rocm/src/advised_commands.rs to verify cross-file claims. History pass: git log/git show on all touched files confirmed planner_steps.rs is new and traced the production dedup fix (ROCMAI-725) this PR adds coverage for. CI: all 24 checks green; Skill checks and Sphinx docs build correctly skipped (the diff touches neither skills/ nor the Sphinx-built docs/rocm-docs/ tree, confirmed directly). Design review raised no fundamental-approach concerns; a check for provider-call retry logic that could mask the mock's "over-ask fails loudly" behavior found none.

One file-grouped pass (apps/rocm/src/freeform_planner.rs, apps/rocm/src/freeform_execution.rs, docs/testing.md, docs/manual-testing.md, tests/e2e-cucumber/README.md, tests/e2e-cucumber/Cargo.toml) was originally fanned out to an independent background worker that did not return in time; that pass was instead run directly by this reviewing context (not independently) after the timeout, so it carries a weaker independence guarantee than the other group (mock_server.rs/planner_steps.rs/chat.feature/e2e.rs), which a separate background worker did complete and return. Not reconciled against any discussion on this PR.

[jussielo-amd]

Heads-up for whoever handles the queue: on the earlier runs of this PR, the two Strix Halo Ubuntu E2E lanes (nightly and release) were cancelled at the 45 minute job limit in the "Run E2E tests on Strix Halo" step, not failed on an assertion. The same two lanes hit the same limit on every recent run of the E2E self-hosted workflow I checked, including the push to main (run 41093898821), the merge queue run (41091047116), three other PRs (41079705915, 41069373643 and 41063989926) and this one's 41072758280, so it looks like a runner or capacity problem rather than something this change causes. On that commit the MI300X, W7900, Strix Halo WSL2 and Strix Halo Windows lanes all passed, as did the hosted E2E tests job. I have not re-run the Strix Halo Ubuntu lanes.