§1: natural-language /plan (Ask → Plan → Review → Run)

[michaelroy-amd]

Stack 7/10 · base: supergoal/phase-6-automations-permissions

• /plan <request> reuses the deterministic planner (freeform_plan_next_action_with_context); renders the structured plan for review.
• A complete mutating action (approval_required && !has_placeholders && !provider_assisted) hands off to the Phase-4 approval modal; placeholder and provider-assisted plans stay plan-only (mirrors the legacy validate_freeform_execution_action guard).
• Provider-assist stays off by default.

Reviewed: rust + maintainability. All cargo gates green.

[rominf]

LGTM. The Run stage is correctly gated: /plan only sets a rocm_command edge that drains through the same approval seam — a mutating plan returns ApprovalRequired and executes only after operator approval. natural_language_plan is correctly in the read-only set. Robust against malformed LLM output (no unwraps).

One optional note: the placeholder/provider-assisted "review-only" guard is enforced only in the reducer, not re-checked at the seam — fine today, worth a comment that it's deliberately not duplicated.

[rominf]

Requesting changes on signing grounds — this is separate from the code review above, which still stands.

Every commit on this PR is unsigned: GitHub reports verification.verified = false (reason unsigned) for all of them. Signed commits are required before this can merge.

To fix, configure commit signing and re-sign the branch history, e.g. with SSH signing:

git config gpg.format ssh
git config user.signingkey <your-signing-key.pub>
git config commit.gpgsign true
git rebase --exec 'git commit --amend --no-edit -S' <base-branch>
git push --force-with-lease

Since this is a stacked PR, re-signing is easiest from the bottom of the stack upward — re-signing a base branch rewrites its SHAs and the dependent branches will need rebasing/re-pushing on top.

Once the commits show as Verified I'll clear this.

[rominf]

Signing gate cleared — all commits on this PR now show Verified (verification.verified = true, reason valid). Re-reviewed the current diff against this PR's own base after the re-sign; it's substantively unchanged from my prior review.

Approving on both grounds: code is sound and commits are signed.

[Copilot]

Pull request overview

Adds Phase-7 “Ask → Plan → Review → Run” support to rocm-dash TUI via a new /plan <request> slash command that calls the read-only natural_language_plan tool off-thread, renders the plan for review, and (when safe/complete) forwards the planned argv into the existing Phase-4 approval flow.

Changes:

• Introduces a /plan edge (plan_request) and a ClientMsg::PlanReady message to render plan output and optionally hand off a complete mutating action to the approval modal.
• Registers a new read-only dash tool natural_language_plan and updates the bin-side tool implementation to return both rendered plan text and a structured next action.
• Updates docs + adds/extends tests to cover /plan dispatch and handoff behavior.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
docs/dash-parity-map.md	Marks /plan as covered in Phase 7 and documents the dash mechanism/tests.
crates/rocm-dash-tui/src/ui/tabs/instances.rs	Updates test state initialization to include the new plan_request field.
crates/rocm-dash-tui/src/client.rs	Adds ClientMsg::PlanReady to carry rendered plan text + optional structured action.
crates/rocm-dash-tui/src/app.rs	Implements /plan parsing, plan execution (spawn_blocking), plan result parsing, and approval handoff logic + tests.
crates/rocm-dash-tui/src/agent.rs	Registers the natural_language_plan read tool and updates the read-tool list/tests.
apps/rocm/src/main.rs	Extends the natural_language_plan tool payload to include a structured action and adds bin-side tests.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[rominf]

Re-approving the current head after the cascade rebase onto the updated base — refreshing my prior approval onto this exact commit.

Re-reviewed: natural-language /plan (Ask→Plan→Review→Run): read-only plan tool off-thread via spawn_blocking, complete mutating actions handed to the approval modal.

Copilot re-reviewed this same commit (COMMENTED, no blocking findings). No clear issues; LGTM.

[rominf]

Requesting changes — retracting my earlier approval. One real concurrency bug (confirmed reachable against this head), plus two non-blocking doc nits.

Blocking: on_plan_ready unconditionally clobbers a pending slash_tool (app.rs:1146-1160).
/plan is dispatched off-thread: the plan_request drain in the event loop (app.rs:1804) runs the planner via spawn_blocking and — unlike the chat path — does not set chat_sending. So while a plan is in flight, submit_chat's in-flight guard (app.rs:669) is open and the user can issue another slash command, which sets self.slash_tool. When PlanReady arrives, on_plan_ready then does self.slash_tool = Some(...) unconditionally for a complete mutating action, silently dropping the user's pending command and replacing it with an approval flow they didn't just request.

This is exactly the case open_approval already guards (if self.approval.is_some() { … discard new; return }). on_plan_ready should do the same: if self.slash_tool is already set (or an approval is open), surface a message and leave the existing request intact instead of overwriting. A unit test driving on_plan_ready with a pre-set slash_tool would lock it in.

Non-blocking (doc accuracy, carryover from Copilot):

• docs/dash-parity-map.md:41 and the client.rs:82 doc comment both describe /plan handoff without the !provider_assisted guard the reducer actually applies — provider-assisted plans are review-only. Worth aligning the wording so future readers don't think provider-assisted plans auto-forward.

Once the clobber guard is in I'll re-review promptly.

[michaelroy-amd]

Pruned one committed Copilot suggestion (simplicity-first / ponytail pass). Force-pushed; kept the two doc-accuracy fixes.

Kept

• parity-map /plan row noting the !provider_assisted guard — doc accuracy.
• client.rs doc comment clarifying mutating plans require !provider_assisted — doc accuracy.

Dropped (with reason)

• on_plan_ready race guard — speculative concurrency hardening. The interleaving (issuing another slash while a /plan computes off-thread) is unlikely and the outcome is last-write-wins on a UI field with no data corruption; not worth the added guard complexity.

[rominf]

Thanks for the update — the two doc nits are resolved (the client.rs PlanReady comment and the parity-map row now both state the !provider_assisted guard). However, the blocking issue from my previous review is still open, so I'm keeping changes requested.

The new commit only touches client.rs and docs/dash-parity-map.md; app.rs is unchanged, so on_plan_ready (app.rs:1146-1159) still does self.slash_tool = Some(...) unconditionally for a complete mutating action. Because the /plan drain (app.rs:1804) runs the planner off-thread without setting chat_sending, a slash command issued while a plan is in flight can still be clobbered (and replaced by an approval flow). The fix is to guard on_plan_ready against an already-set slash_tool (or open approval) — mirroring open_approval's existing guard — and surface a message instead of overwriting, with a unit test driving on_plan_ready while a slash_tool is pending.

Once that guard is in I'll approve.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

[michaelroy-amd]

@rominf addressed — added the guard. on_plan_ready now bails when slash_tool or approval is already set: it pushes an error turn ("A command is already in progress; the planned action was discarded…") and drops the planned action instead of overwriting, mirroring open_approval's single-in-flight guard. New unit test on_plan_ready_guards_against_pending_slash_tool drives on_plan_ready with a pending slash_tool and asserts it's preserved + an error turn is surfaced. Ready for re-review.

[rominf]

Approving at 3bb2df4.

The in-flight guard added to on_plan_ready resolves the earlier clobber concern: /plan drains off-thread without setting chat_sending, and the guard now matches open_approval's single-in-flight contract (checks both slash_tool and approval).

Verified the producer↔consumer contract: the four action fields map 1:1 from FreeformPlanAction, has_placeholders uses the same <…> predicate on both sides, and the provider-assisted / placeholder review-only guards hold independently in the bin and the TUI. CI green on both platforms.

Two open threads remain, both non-blocking:

• Empty-args in parse_plan_result is defense-in-depth only — the sole producer cannot emit a complete mutating action with empty argv (provider planner rejects empty args; deterministic actions are hardcoded non-empty). A !args.is_empty() guard would close the thread but isn't required.
• Bare /plan uses the agent role instead of error, unlike sibling slash commands — a minor consistency nit.

Both are fine as follow-ups.