§1: provider switch (/provider) + /chat + Anthropic backend (rig) by michaelroy-amd · Pull Request #36 · ROCm/rocm-cli

michaelroy-amd · 2026-06-21T15:45:52Z

Stack 8/10 · base: supergoal/phase-7-nl-plan

AnthropicAgentClient via rig (no Cargo feature needed; agent.rs stays the sole rig namer), registering the same read+mutating ROCm tools as the other backends.
/provider local|openai|anthropic rebuilds the active backend (local restored from a saved clone); /chat entry/passthrough.
Keys env-first → OS secure store via provider_keys, never argv/logs. ureq/reqwest partition intact (bin has no rig).

Reviewed: rust + security (no key in argv/logs — confirmed) + maintainability. All cargo gates green.

rominf

Approve with minor comments. /provider + Anthropic backend via rig looks good; default endpoint is hardcoded to https://api.anthropic.com (ignores cfg.base_url, can't be pointed elsewhere), key rides x-api-key and never hits argv/base_url, Debug redacts it.

One real bug:

Failed-switch state desync (event-loop provider_switch drain): on a failed non-Local switch the code reverts active_provider to Local but leaves agent unchanged. If a prior /provider openai succeeded and a later /provider anthropic fails, the UI shows local while chat still routes to OpenAI. Fix: also restore agent (revert to the previously-active provider, not hardcoded Local). This path is untested. (Looks like #38's "honest provider revert" addresses exactly this — worth confirming #36 doesn't ship independently of that fix.)
Minor: AnthropicAgentClient::complete triplicates the prompt/timeout/annotate tail of the other two backends; could extract a shared helper.

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 now show Verified. Re-reviewed the current diff against supergoal/phase-7-nl-plan after the re-sign.

Approving as a stacked PR. The Anthropic provider wiring is solid: key is env-first then secure store, carried in-process (never in argv), base_url pinned to the provider default, key in x-api-key (won't leak into error strings). Tool/approval parity is preserved across all three backends.

One known caveat, accepted because it's fixed downstream: the failed-switch arm reverts active_provider to Local unconditionally, so a failed switch while already on a remote provider mislabels the active provider (display + routing, not a crash or capability hole). This is exactly the desync fixed by #38's honest provider revert. Acceptable in stack order.

Copilot

Pull request overview

Adds Phase-8 dash chat provider switching and an Anthropic (Claude) backend via rig, wiring key resolution through the bin → ResolvedArgs seam and ensuring tool/approval parity across backends.

Changes:

Introduces /provider local|openai|anthropic and /chat slash commands with event-loop-driven backend rebuild and chat dispatch.
Adds AnthropicAgentClient (rig provider) and refactors shared telemetry/skill tool registration to keep all backends in parity.
Extends dash arg resolution to carry an Anthropic API key (env-first, then OS secure store) into the TUI process.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
docs/dash-parity-map.md	Marks provider/chat parity as covered and documents the new slash-command behaviors.
crates/rocm-dash-tui/src/ui/tabs/instances.rs	Updates test `AppState` construction to include the new provider fields.
crates/rocm-dash-tui/src/app.rs	Adds `ChatProvider`, `/provider` + `/chat` handling, provider-switch edge draining, and a backend factory (`build_chat_agent`).
crates/rocm-dash-tui/src/agent.rs	Adds `AnthropicAgentClient` and centralizes shared telemetry/skill tool registration across backends.
apps/rocm/src/dash.rs	Resolves Anthropic API key (env → secure store) and threads it into `ResolvedArgs` for the dash.

💡 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: provider switch /provider + /chat + Anthropic (rig) backend; API key resolved env→secure-store across the dash seam, tool/approval parity preserved.

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

rominf

Requesting changes — retracting my earlier approval. Two High-severity correctness bugs in the provider-switch path (both confirmed against the code at this head, not just the Copilot flags):

1. /provider openai with no key silently builds a dead backend (agent.rs:897-905, app.rs:1513-1520).
RigAgentClient::new falls back to a "sk-no-key" bearer when cfg.api_key is None and returns Ok, so build_chat_agent(ChatProvider::Openai, …) returns Some(...) even with OPENAI_API_KEY unset. The switch reports "switched to openai" and then 401s at request time. This violates build_chat_agent's own documented contract ("None signals 'couldn't build' (e.g. a missing key) so the caller surfaces an actionable error turn instead of switching to a dead backend"), and it's inconsistent with the Anthropic path, which correctly fails fast (AnthropicAgentClient::new returns a Build error naming the env var when the key is absent).

Fix: gate the OpenAI build on a non-empty args.chat_api_key and return None when it's missing, so the event loop surfaces the actionable error turn. (The sk-no-key dummy should remain only for the managed-gateway/custom-header case that genuinely ignores the bearer.)

2. Failed provider switch leaves active_provider and the live agent inconsistent (app.rs:2025-2035).
On build failure the code sets state.active_provider = ChatProvider::Local but leaves agent unchanged. If the previous backend was remote (e.g. switching Anthropic → OpenAI and the OpenAI build fails), the UI then claims "local" while chat requests still route to the old remote backend.

Fix: in that failure branch, also restore agent to the local agent so the displayed provider and the live backend stay consistent (or, if local can't be rebuilt here, keep active_provider on the prior backend and word the error accordingly — but the two must not diverge).

Non-blocking (carryover Copilot nits): the parity-map row (docs/dash-parity-map.md:42) says keys are "env-first then secure store" but only the Anthropic key consults the secure store — the OpenAI key is env-only; and the argv-scan assertion (app.rs:~3889) isn't a reliable "no key in argv" proof. Worth tidying but not blocking.

Apologies for the earlier approval — I'd relied on Copilot's non-blocking summary and missed these inline findings. Once (1) and (2) are addressed I'll re-review promptly.

michaelroy-amd · 2026-06-24T21:05:05Z

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

Kept

parity-map wording fix: only the Anthropic key consults the OS secure store; the OpenAI-compatible key is env-only — doc accuracy.

Dropped (with reason)

OpenAI keyless fail-fast (gate provider build on non-empty OPENAI_API_KEY, return None otherwise) — would break the intended keyless flow for local OpenAI-compatible servers (vLLM / llama.cpp), which accept the dummy bearer by design. Keeping the current keyless-tolerant build behavior.
(Also dismissed, not committed: provider-switch-failure leaves agent unchanged, and the std::env::args() argv-scan test — keeping the existing security-intent test as-is.)

rominf

Thanks for the update, but both blocking issues from my previous review are still open — the new commit only changes one line in docs/dash-parity-map.md; app.rs and agent.rs are unchanged. Keeping changes requested.

/provider openai with no key still builds a dead backend. build_chat_agent's OpenAI arm (app.rs:1514) still passes api_key: args.chat_api_key.clone(), and RigAgentClient::new still falls back to the sk-no-key bearer (agent.rs:899) and returns Ok — so the switch reports success and then 401s at request time. (Note: the chat_api_key.is_none() check around app.rs:1642 is the default-launch ChatGPT-OAuth fallback path, not the provider-switch build, so it doesn't cover this.) Please gate the OpenAI build on a non-empty key and return None when absent.
Failed provider switch still leaves state inconsistent. The failure branch (app.rs:2021-2023) sets active_provider = ChatProvider::Local but doesn't reset agent, so a failed switch from a remote backend leaves the UI claiming "local" while requests still route to the old remote backend.

Once these two are addressed I'll re-review.

Copilot

Pull request overview

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

+        ChatProvider::Openai => {
+            let cfg = crate::llm::LlmConfig {
+                base_url: OPENAI_BASE_URL.to_string(),
+                model: args
+                    .chat_model
+                    .clone()
+                    .filter(|m| !m.is_empty())
+                    .unwrap_or_else(|| OPENAI_DEFAULT_MODEL.to_string()),
+                api_key: args.chat_api_key.clone(),
+                auth_header: None,
+            };
+            crate::agent::RigAgentClient::new(cfg, executor, Some(approval_tx))
+                .ok()
+                .map(|c| std::sync::Arc::new(c) as std::sync::Arc<dyn crate::agent::AgentClient>)
+        }


+    #[test]
+    fn chat_keys_flow_only_through_resolved_args_not_argv() {
+        // The seam carries keys via ResolvedArgs (in-process), never process
+        // argv. This structurally asserts the factory reads the key from the
+        // struct field — there is no argv plumbing in the build path.
+        let args = args_with_anthropic_key(Some("sentinel-key"));
+        assert_eq!(args.anthropic_api_key.as_deref(), Some("sentinel-key"));
+        // The real process args never carry the key (no `--api-key`-style flag
+        // exists; keys are env/secure-store sourced by the bin into the struct).
+        let argv: Vec<String> = std::env::args().collect();
+        assert!(
+            !argv.iter().any(|a| a.contains("sentinel-key")),
+            "no key value is ever present in process argv"
+        );
+    }


…t on failed switch build_chat_agent's OpenAI arm now requires a non-empty key and returns None otherwise — previously it passed the (possibly None) key through to RigAgentClient::new, which substitutes a dummy sk-no-key bearer and builds, so the switch reported success then 401'd at request time. On a failed switch the event loop now also resets the live agent to local_agent (not just active_provider), so requests don't keep routing to a prior remote backend while the UI claims local. Adds build_chat_agent_openai_requires_key. Addresses review on #36.

michaelroy-amd · 2026-06-25T15:41:45Z

@rominf both addressed. (1) build_chat_agent's OpenAI arm now requires a non-empty chat_api_key and returns None otherwise, so a keyless /provider openai no longer builds the sk-no-key backend — the switch fails fast with the actionable error and stays on the current backend. New test build_chat_agent_openai_requires_key covers no-key→None and key→Some. (2) The failed-switch branch now also does agent = local_agent.clone() alongside active_provider = Local, so a failed switch from a remote backend no longer leaves requests routed to the old remote while the UI claims local. Ready for re-review.

…t on failed switch build_chat_agent's OpenAI arm now requires a non-empty key and returns None otherwise — previously it passed the (possibly None) key through to RigAgentClient::new, which substitutes a dummy sk-no-key bearer and builds, so the switch reported success then 401'd at request time. On a failed switch the event loop now also resets the live agent to local_agent (not just active_provider), so requests don't keep routing to a prior remote backend while the UI claims local. Adds build_chat_agent_openai_requires_key. Addresses review on #36.

michaelroy-amd · 2026-06-26T15:44:48Z

Rebased onto current main (post-#35 squash) and re-signed; the only conflict was docs/dash-parity-map.md (kept main's plan row + this PR's provider/chat rows).

Both blocking issues from your last review are addressed in 1dac2c7 (fix(dash): fail OpenAI provider switch fast without a key; reset agent on failed switch) — that commit landed at 08:40 PDT on 6/25, after your 06:58 re-review of the docs-only 8aff7e0, which is why they still looked open:

OpenAI build with no key — build_chat_agent's OpenAI arm now gates on a non-empty key: let api_key = args.chat_api_key.clone().filter(|k| !k.trim().is_empty())?; and passes Some(api_key) to RigAgentClient::new, returning None when absent so the switch surfaces an actionable error and stays on the current backend instead of building a dead sk-no-key one. Covered by build_chat_agent_openai_requires_key.
Failed switch left state inconsistent — the failure branch now also resets the live agent (agent = local_agent.clone();) alongside active_provider = ChatProvider::Local, so a failed switch from a remote backend no longer routes requests to the old remote while the UI claims local.

cargo fmt --check, clippy --all-targets --all-features -D warnings, and cargo test -p rocm-dash-tui all clean. Re-requesting review.

Add AnthropicAgentClient mirroring RigAgentClient: builds rig::providers::anthropic::Client from an LlmConfig (api_key required, provider-default base_url), and registers the identical telemetry/skill + ROCm read + mutating tool set per request so tool and approval parity holds across local/openai/anthropic. Anthropic ships unconditionally in rig-core 0.38.1 (no Cargo feature). Register-helper bounds unchanged. Signed-off-by: Michael Roy <michael.roy@amd.com>

…argv) Add ResolvedArgs.anthropic_api_key, carried in-process through the seam. dash.rs sources it via provider_keys::resolve_provider_api_key (env-first then OS secure store); None on absence/error so the dash still launches. Signed-off-by: Michael Roy <michael.roy@amd.com>

Add ChatProvider {Local,Openai,Anthropic} + active_provider + provider_switch edge on AppState. /provider [name] parses + raises the switch edge; the event loop rebuilds the live agent via a new build_chat_agent factory (construction only — Openai/Anthropic from ResolvedArgs keys, Local reuses the inline auto-detect build). A missing key reverts to local with an actionable error turn. /chat [prompt] passes through to the agent; bare /chat focuses the Chat tab. Mark the G parity rows (provider, chat) covered. Signed-off-by: Michael Roy <michael.roy@amd.com>

agent.rs: anthropic construct/requires-key/explicit-model, the same-tools contract across backends, and an ignored live round-trip. app.rs: /provider switch+bare+unknown, /chat passthrough+focus, the build_chat_agent factory (with/without key, local defers), and a structural assert that keys flow via ResolvedArgs not argv. Signed-off-by: Michael Roy <michael.roy@amd.com>

…aph) Split the AnthropicAgentClient doc first paragraph, rewrite the provider-switch build arm as if-let/else, and the no-key test as let-else (the Ok variant isn't Debug). Signed-off-by: Michael Roy <michael.roy@amd.com>

…lemetry_tools; doc key-sourcing FIX 1 (HIGH): /provider local now re-points agent at the auto-detected local backend. Snapshot local_agent before the event loop and restore it in the ChatProvider::Local arm — build_chat_agent(Local) returns None by design, so without the restore a prior /provider openai left requests routed to OpenAI (silent wrong-backend bug). Adds provider_local_restores_saved_local_agent test. FIX 2 (HIGH): extract register_telemetry_tools<M, P> mirroring register_rocm_read_tools, killing the 6-tool copy-paste across all three complete() bodies (RigAgentClient, ChatGptAgentClient, AnthropicAgentClient). Extends all_backends_register_same_rocm_tools to pin the telemetry set via SKILL_NAMES so it can't silently diverge. FIX 3 (MEDIUM): document the intentional key-sourcing asymmetry on chat_api_key_from_env — chat/OpenAI key is env-only; the Anthropic key additionally consults the OS secure store via provider_keys. Signed-off-by: Michael Roy <michael.roy@amd.com>

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Michael Roy <michael.roy@amd.com>

…t on failed switch build_chat_agent's OpenAI arm now requires a non-empty key and returns None otherwise — previously it passed the (possibly None) key through to RigAgentClient::new, which substitutes a dummy sk-no-key bearer and builds, so the switch reported success then 401'd at request time. On a failed switch the event loop now also resets the live agent to local_agent (not just active_provider), so requests don't keep routing to a prior remote backend while the UI claims local. Adds build_chat_agent_openai_requires_key. Addresses review on #36. Signed-off-by: Michael Roy <michael.roy@amd.com>

Comments addressed

…headers) The phase-10 app.rs split predates #36/#37 and the repo's MIT relicense, so the extracted modules carried stale logic and license headers that the rebase's "keep the split structure" resolution preserved. Restore main's behavior: - build_chat_agent's OpenAI arm requires a non-empty key and returns None otherwise (#36); the older split passed the raw Option through, building a dead sk-no-key backend that 401s at request time. - /update --apply is position-independent (scan all tokens), matching /uninstall, not only the second token. - /uninstall rejects conflicting --apply + --dry-run with a guided error. - app/{chat,slash,summary}.rs SPDX headers switched Apache-2.0 → MIT to match the relicensed tree (licenserc.toml / hawkeye gate). All dash-tui tests pass; hawkeye clean. Signed-off-by: Michael Roy <michael.roy@amd.com>

rominf reviewed Jun 22, 2026

View reviewed changes

michaelroy-amd force-pushed the supergoal/phase-7-nl-plan branch from 48f5053 to 52858a1 Compare June 23, 2026 03:41

michaelroy-amd force-pushed the supergoal/phase-8-provider-anthropic branch from de9b2e8 to c217073 Compare June 23, 2026 03:41

rominf requested changes Jun 23, 2026

View reviewed changes

michaelroy-amd force-pushed the supergoal/phase-7-nl-plan branch from 52858a1 to c89e8e6 Compare June 23, 2026 17:39

michaelroy-amd force-pushed the supergoal/phase-8-provider-anthropic branch from c217073 to ba10a67 Compare June 23, 2026 17:39

rominf approved these changes Jun 24, 2026

View reviewed changes

rominf mentioned this pull request Jun 24, 2026

§1+§2: polish, harden, invariant re-proofs + ROADMAP update #38

Merged

michaelroy-amd force-pushed the supergoal/phase-7-nl-plan branch from c89e8e6 to 02310cc Compare June 24, 2026 16:53

michaelroy-amd force-pushed the supergoal/phase-8-provider-anthropic branch from ba10a67 to 0cf26f3 Compare June 24, 2026 16:53

rominf requested a review from Copilot June 24, 2026 17:50

Copilot started reviewing on behalf of rominf June 24, 2026 17:51 View session

Copilot AI reviewed Jun 24, 2026

View reviewed changes

Comment thread crates/rocm-dash-tui/src/app.rs

Comment thread crates/rocm-dash-tui/src/app.rs

Comment thread docs/dash-parity-map.md Outdated

Comment thread crates/rocm-dash-tui/src/app.rs

rominf approved these changes Jun 24, 2026

View reviewed changes

rominf requested changes Jun 24, 2026

View reviewed changes

michaelroy-amd force-pushed the supergoal/phase-8-provider-anthropic branch from 14b06a8 to 8aff7e0 Compare June 24, 2026 21:03

rominf requested a review from Copilot June 25, 2026 06:55

Copilot started reviewing on behalf of rominf June 25, 2026 06:56 View session

rominf previously requested changes Jun 25, 2026

View reviewed changes

Copilot AI reviewed Jun 25, 2026

View reviewed changes

michaelroy-amd requested a review from rominf June 25, 2026 15:41

michaelroy-amd force-pushed the supergoal/phase-7-nl-plan branch from 33fcde0 to 3bb2df4 Compare June 25, 2026 16:31

Base automatically changed from supergoal/phase-7-nl-plan to main June 26, 2026 08:14

michaelroy-amd force-pushed the supergoal/phase-8-provider-anthropic branch from 45017d5 to 1dac2c7 Compare June 26, 2026 15:43

michaelroy-amd and others added 7 commits June 26, 2026 08:51

Potential fix for pull request finding

81f0bab

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Michael Roy <michael.roy@amd.com>

michaelroy-amd force-pushed the supergoal/phase-8-provider-anthropic branch from 1dac2c7 to ae78484 Compare June 26, 2026 15:55

michaelroy-amd enabled auto-merge June 26, 2026 15:55

juhovainio approved these changes Jun 26, 2026

View reviewed changes

michaelroy-amd added this pull request to the merge queue Jun 26, 2026

Merged via the queue into main with commit 749c0d3 Jun 26, 2026
8 checks passed

michaelroy-amd deleted the supergoal/phase-8-provider-anthropic branch June 26, 2026 17:01

Uh oh!

Conversation

michaelroy-amd commented Jun 21, 2026

Uh oh!

rominf left a comment

Choose a reason for hiding this comment

Uh oh!

rominf left a comment

Choose a reason for hiding this comment

Uh oh!

rominf left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rominf left a comment

Choose a reason for hiding this comment

Uh oh!

rominf left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelroy-amd commented Jun 24, 2026

Uh oh!

rominf left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

michaelroy-amd commented Jun 25, 2026

Uh oh!

michaelroy-amd commented Jun 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rominf left a comment •

edited

Loading