feat(btw): switch a coding CLI's own provider from the dashboard - #219
Conversation
d68f543 to
3622486
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
BegoniaHe
left a comment
BegoniaHe
left a comment
There was a problem hiding this comment.
AI-assisted review
Stacked delta btw-settings-page...cli-provider-switch only. This is not an approval.
The host-file write path (backup, atomic replace, 0o600, no credential in responses) is the right shape. The Dashboard page as wired cannot be used safely: GET is gated on the high-risk write action, and the provider list round-trips the config redaction placeholder as if it were a live key.
Blockers
GET /coding-cli/global-configrequirescoding_cli.config.write+ step-up;load()does not prompt for step-up, so the page fails closed on first open.configProfileApi.getredactsapi_keyto__ASTRBOT_REDACTED__. This page stores that string and posts it back. Duplicate/delete then save can persist the placeholder or attach another row's secret (restore is by list index).
Also
- Invalid Claude
settings.jsonis backed up, then replaced with a new object — the test name claims otherwise. - “Add” in the Claude section stores an unscoped entry, so it appears under Codex too; backend
normalize_cli_providersalso drops duplicate ids. - Truncated Codex managed section: apply prepends a new block instead of refusing.
- New sidebar route has no
docs/zh+docs/en(includingwebui.mdmapping). process_tree.pyis unrelated drive-by; the innerimport ctypesis already flagged by CodeQL.- Helper tests never hit FastAPI, so the GET/step-up miss was invisible.
Please add a ## Human note (AI_POLICY).
| auth: AuthContext = Depends(require_coding_cli_scope), | ||
| ): | ||
| """Report each CLI's own configuration and the providers it can use.""" | ||
| await authorize_coding_cli(request, auth) |
There was a problem hiding this comment.
Blocker: this GET uses authorize_coding_cli, which demands high-risk coding_cli.config.write and therefore a step-up token (HIGH_RISK_ACTIONS → _requires_step_up).
CliConfigPage.load() calls codingCliApi.state() with no step-up headers and treats any failure as loadFailed. Opening the page is a read of file metadata; it should use a normal read action (e.g. platform.read / a new coding_cli.config.read). Keep the write action on PUT/DELETE only.
x-astrbot-scope: config on the OpenAPI GET also does not declare this write action, so the contract hides the gate.
| loadFailed.value = false; | ||
| try { | ||
| const [stateResponse, profileResponse] = await Promise.all([ | ||
| codingCliApi.state(), |
There was a problem hiding this comment.
Blocker: codingCliApi.state() is not wrapped in runMutationWithStepUp. The matching GET currently requires coding_cli.config.write, so the first visit 403s with requires_step_up and this catch only flips loadFailed. Retry calls load() again and cannot succeed.
Even after the GET is a read, this path should not swallow a structured step-up response as a generic load error.
| }; | ||
| if (provider.cli) entry.cli = provider.cli; | ||
| else delete entry.cli; | ||
| if (provider.api_key) entry.api_key = provider.api_key; |
There was a problem hiding this comment.
High: get_profile redacts nested api_key to __ASTRBOT_REDACTED__. load() then does api_key: text(entry.api_key), so this truthy check posts the placeholder as the stored secret.
save_config_async restores redacted values by list index, not by id. After delete/reorder, a later row can receive an earlier row's key. After duplicate (new id, extra index), restore does not match and the placeholder itself is persisted — a later Switch writes that string into ~/.claude/settings.json / auth.json.
Treat the placeholder as “key present, field empty”; never copy it onto a new id; omit api_key on unchanged rows only if restore is id-keyed. Vitest currently mocks the raw secret, so this path is untested.
| name: `${provider.name} (copy)`, | ||
| current: false, | ||
| cli: source?.cli ?? '', | ||
| api_key: source?.api_key ?? '', |
There was a problem hiding this comment.
High: Duplicate copies source.api_key. After a real GET that value is the redaction placeholder, not a key. The copy is a new id, so config restore cannot map it back to the original secret (index past len(current) is left as posted). Saving the list then stores the placeholder; Switching the copy writes it into the CLI file.
Copy metadata only. Leave api_key empty on the duplicate and has_api_key: false until the operator pastes a key.
| formError.value = tm('cliConfigPage.endpointOrKeyRequired'); | ||
| return; | ||
| } | ||
| const scope = form.value.cli === 'claude_code' ? '' : form.value.cli; |
There was a problem hiding this comment.
Bug: Add from the Claude section sets cli to ''. providersFor and providers_for_cli treat a missing cli as “every CLI”, so the card also appears under Codex. Add from Codex is scoped correctly.
normalize_cli_providers also dedupes by id globally, so two same-id entries (allowed here via per-scope clash) silently drop the second on save/reload.
Set cli: form.value.cli for both sections, and reject a duplicate id across the whole list.
| _back_up_once(path) | ||
|
|
||
| if cli == "claude_code": | ||
| payload = _read_json(path) |
There was a problem hiding this comment.
Bug: _read_json documents “treated as absent rather than replaced”, but this caller still _atomic_writes payload after a {} fallback. Invalid JSON, JSONC comments, or a non-object file is backed up and then overwritten with {"env": ...}.
Refuse the switch when the existing file cannot be parsed as an object (backup-only is not enough). Same for a JSON array.
| # A backup is taken, so the damaged file is recoverable... | ||
| cg.apply_provider("claude_code", {**PROVIDER, "api_key": ""}) | ||
| backup = path.with_name(path.name + cg.BACKUP_SUFFIX) | ||
| assert backup.read_text(encoding="utf-8") == "{ this is not json" |
There was a problem hiding this comment.
This test name says the file is not replaced, but it only asserts the backup. After apply_provider, path is a new JSON object and the original text is gone. Please assert path.read_text() is unchanged (once apply refuses) or drop the name so this does not lock in the overwrite.
| ) | ||
| block = _managed_block(base_url, model, env_key if api_key else "") | ||
| rest = _strip_managed_block(existing) | ||
| _atomic_write(path, f"{block}\n" + (f"\n{rest}" if rest else ""), private=False) |
There was a problem hiding this comment.
Bug: _strip_managed_block correctly leaves a truncated section alone, but apply_provider then prepends a complete block in front of it. The file keeps the orphan # >>> astrbot btw plus user tables, and the next rewrite only strips the new complete section.
If CODEX_MANAGED_BEGIN is present without CODEX_MANAGED_END, refuse the write (same policy as “do not guess”). read_state already reports managed is False for this case; apply should match.
| { | ||
| title: 'core.navigation.cliConfig', | ||
| icon: 'mdi-pencil-ruler', | ||
| to: '/cli-config', |
There was a problem hiding this comment.
New default-sidebar route. docs/zh/use/webui.md and docs/en/use/webui.md still have no entry (and no old→new mapping row). The page's ConfigDocsLink points at dev/astrbot-config.html, which also does not describe btw.cli_providers or the host-file switch.
AGENTS.md: navigation/config changes need matching zh+en docs in this change.
| ) | ||
|
|
||
|
|
||
| def test_the_switch_action_is_high_risk(): |
There was a problem hiding this comment.
This asserts the write action is high-risk, but nothing exercises the FastAPI routes. GET/PUT/DELETE all share authorize_coding_cli, so the “page load needs step-up” bug cannot fail this file.
Please add TestClient coverage: GET without step-up is allowed (once split), PUT/DELETE without step-up is 403 requires_step_up, and responses never contain the raw key.
3622486 to
278263e
Compare
The provider list could never be shown. Two faults sat under one symptom, and the first hid the second. The state route asked for `coding_cli.config.write`, which is high-risk. The Dashboard answers a high-risk action with a step-up challenge until the operator proves themself, and a plain GET has no way to answer one, so every load of the page was denied -- the audit log holds nothing but `step_up_required` for that action. Reading a path and whether a key is stored is not the write that action guards, so the read now asks for `platform.read` and the switch keeps the high-risk action to itself. That also puts the route back inside the `config` API scope the OpenAPI document declares for it, which the write action was never part of. Behind that denial, `_config_of` looked for the config profile service on the core runtime's services, where it does not exist: it is on the Dashboard's own `app.state.services`. The route therefore answered "Configuration is unavailable" with a 503 even once it was authorized, which is why fixing the action alone changed nothing visible. The editor now answers a challenge instead of dead-ending on it: the state call travels the step-up path, so a session that still has to prove itself gets the dialog and the request is retried with the token. The tests go through the ASGI app rather than calling the route functions directly, because the fault was in which action a route asks for and nothing below the app can see that. AI-Generated: true Generated-At: 2026-09-18T08:53:00Z
The profile reports a stored key as `__ASTRBOT_REDACTED__`, and the page read that marker into the field as if it were the key. Saving then carried it whichever way the entry moved: a rename put it in front of a different provider's key, a copy under a new id kept the marker itself, and an added provider was given no scope at all, so it appeared under both CLIs while the profile -- which keys its list by id -- would drop one of the two on the next load. The marker now means what it says: a key is stored, the field is empty, and empty means "leave it alone". The marker goes back only for the entry it came from, whose id the save still carries; a copy is metadata only and starts with no key. An entry added from a section is scoped to that section, and an id another entry already uses is refused across the whole list rather than per CLI. The copy that only this page reads comes back with it. The branch below drops it because nothing there renders it; the page that needs it is here. AI-Generated: true Generated-At: 2026-09-18T08:53:30Z
`_read_json` fell back to an empty object for a file it could not parse, and its docstring said that meant the file was "treated as absent rather than replaced". The switch then replaced it: a backup of a settings.json holding JSONC comments -- or anything else the parser refuses -- only made the loss recoverable, and a JSON array was replaced by an object just as quietly. A file that exists and is not a JSON object is now an error the switch reports, and the backup is not taken, because nothing was going to change. The Codex section had the same shape of gap one file over. Stripping a section whose closing marker is missing is right, but applying a provider then wrote a complete section in front of the orphan, so the file kept the orphan for good and the next switch stripped only the block it had written. Reading already reported that file as unmanaged; writing now refuses it too, on the same grounds: where the section ends is not knowable from what is left. The credential file is read before the configuration file is written, so a refusal leaves the CLI switched in no respect rather than in half. AI-Generated: true Generated-At: 2026-09-18T08:54:00Z
The module already imports `ctypes` at module level, guards `WinDLL` behind a `sys.platform` check, and reads `get_last_error` through `getattr`, so importing the names directly and guarding the lookup again changed nothing -- and the second `import ctypes`, inside `_last_error`, is a redundant import the linter and CodeQL both flag. It arrived with the delegation work in the base branch and has nothing to do with switching a CLI's provider. AI-Generated: true Generated-At: 2026-09-18T08:54:30Z
The new page had no entry in the user guide and the settings it edits had none in the developer guide, which is where the page's own documentation link points. The developer guide gains what `btw.cli_providers` holds, that a switch is the one write AstrBot makes outside its data directory, what is written into each CLI's own file, and which files are refused rather than replaced. The user guide gains the entry so an operator can find the page, together with the two things it is easy to get wrong: the switch rewrites what the CLI reads by hand, and it asks for a step-up. AI-Generated: true Generated-At: 2026-09-18T08:55:00Z
CodeQL reads `"https://gw.example" in settings` as an attempt to sanitize a URL by substring and flags it as incomplete, which is fair: the check would pass on a file that merely mentions the endpoint somewhere. The switch writes named fields, so the test reads those fields and compares them, which says what it meant and stops matching by accident. The explicit import of the `asgi_app` fixture is redundant -- the harness exports it with everything else -- and CodeQL counts it as unused. AI-Generated: true Generated-At: 2026-09-18T09:30:33Z
278263e to
051cb0a
Compare
3 participants
Stacked on #206. Review
btw-settings-pagefirst.What this does
A preset configured for a coding agent is layered over the CLI's own
configuration for the length of a delegated run and never touches it, which is
what keeps a run from rewriting what the operator runs interactively — but it
also means there was no way to point the CLI itself at a provider. This adds
that second, deliberate path, in the shape cc-switch uses: a card list grouped by
CLI, each card carrying an endpoint and an API key, with enable / edit /
duplicate / delete per card and a per-CLI restore.
The list (
btw.cli_providers) is ordinary configuration, saved through theprofile like any other setting. Only the switch touches a file AstrBot does not
own.
Guards
It replaces the operator's settings and can persist a credential in a file
AstrBot does not own, so:
.astrbot-backup) before the first write;mkstemp+fsync+os.replace+ directory fsync), anda file holding a key is
0o600;coding_cli.config.writeaction, whichmeans step-up.
Codex
The standard library reads TOML but cannot write it, and rewriting a file we
cannot parse faithfully would drop the user's comments, so AstrBot's keys go in a
comment-delimited section at the top of the file — TOML requires top-level
keys to precede every table — and the rest is left byte for byte as it was. A
section with no closing marker is left alone rather than guessed at. The
credential goes in
auth.jsonunder the name that section points at.Notes for review
btw.cli_providersdeliberately has no entry in the config metadata: thegeneric renderer's list control would flatten each entry to
"[object Object]". The guard that every BTW field reaches a control now names that oneexemption, with a test that it stays a real setting.
never sends a key back, so empty has to mean unchanged rather than erased.
mdi-pencil-ruler, already in the generated MDI subset;no new icon dependency.
Verification
Local gates on this host: dashboard
typecheck,eslint --max-warnings=0,i18n check,
vitest(317 passed; the 4favicon.svgsuite failures predate thischange),
pnpm build,pyright --pythonplatform Linux(0 errors), and the Pythonunit suite including the OpenAPI contract test.
uv runtargets in the Makefile do not run on this host (.python-versionpins3.14.6); the equivalent commands above were run directly.