diff --git a/.claude/skills/create-agent-issue/SKILL.md b/.claude/skills/create-agent-issue/SKILL.md index 8710098b..12c614ec 100644 --- a/.claude/skills/create-agent-issue/SKILL.md +++ b/.claude/skills/create-agent-issue/SKILL.md @@ -32,11 +32,11 @@ The caller passes (free-form for ad-hoc use, structured for subagent use): - **TODO text** — the line(s) describing the work. - **Priority** — `P0` / `P1` / `P2` / `P3`. -- **Folded sub-items** *(optional)* — secondary bullets that should appear as a +- **Folded sub-items** _(optional)_ — secondary bullets that should appear as a checklist in the issue body. -- **Source file + line range** *(optional)* — file path and 1-based line range +- **Source file + line range** _(optional)_ — file path and 1-based line range whose lines should be replaced with a link to the new issue after the push. -- **`chunk-id`** *(optional)* — string used to name the draft artifact file. +- **`chunk-id`** _(optional)_ — string used to name the draft artifact file. Required when `stage=draft` or when running under a reviewer flow. - **`stage`** — one of: - `stage=draft` — investigate + write draft to @@ -44,9 +44,9 @@ The caller passes (free-form for ad-hoc use, structured for subagent use): - `stage=review-and-push` — load the draft, independently verify, revise if needed, push via `gh`, edit the source file (or return the replacement line). - - `stage=all` *(default for ad-hoc use)* — do both back-to-back in the same + - `stage=all` _(default for ad-hoc use)_ — do both back-to-back in the same context. Used when no reviewer step is required. -- **`source-edit`** *(optional, only with `review-and-push` / `all`)* — +- **`source-edit`** _(optional, only with `review-and-push` / `all`)_ — `apply` (default; skill edits the file) or `return-only` (skill reports the proposed replacement line and does not touch the file). @@ -73,6 +73,7 @@ Before drafting: (e.g. `designdocs/SKILLS_DISCOVERY_REDESIGN.md` is referenced today but not yet written), flag this in the issue's Open questions section and proceed with the TODO text alone. + 3. **Note the current behavior in one or two sentences.** This anchors the `## Context` section. The sentence must reference at least one real file path or symbol the investigator just read. @@ -80,7 +81,7 @@ Before drafting: anchors `## Proposed behavior` and `## Success criteria`. 5. **Look for "design needed" markers** in the TODO sub-bullets. When present, the issue body should call this out in `## Open questions / - Risks` and `## Proposed behavior` should remain intentionally open-ended +Risks` and `## Proposed behavior` should remain intentionally open-ended ("design a UI that …" rather than prescribing pixels). ## Title standard @@ -155,7 +156,7 @@ missing label via `gh label create --repo logancyang/obsidian-copilot-preview --color `. Palette: | Label | Color | -|--------------|-----------| +| ------------ | --------- | | `agent-mode` | `#5319e7` | | `P0` | `#b60205` | | `P1` | `#d93f0b` | @@ -183,15 +184,19 @@ investigated: --- ## Context + ... ## Proposed behavior + ... ## Success criteria + ... ## Open questions / Risks + ... ``` @@ -243,7 +248,7 @@ After all rubric items pass: `/tmp/create-agent-issue-.md`) — passing a body file is more robust than `--body` for multi-line markdown. 3. `gh issue create --repo logancyang/obsidian-copilot-preview --title "" - --label "<comma-separated labels>" --body-file <tmpfile>`. Capture the +--label "<comma-separated labels>" --body-file <tmpfile>`. Capture the returned URL. 4. Parse the issue number from the URL. @@ -281,11 +286,11 @@ If no source file is passed, skip this step entirely. Return a structured summary the caller can parse: - `Created #<N>: <title> → <url>` -- `Source edit: <path>:<lines>` *or* `Source edit: returned <line>` *or* +- `Source edit: <path>:<lines>` _or_ `Source edit: returned <line>` _or_ `Source edit: none` -- `Revision passes: <N>` *(only in `stage=review-and-push` / `stage=all`)* -- `Rubric: pass` *or* `Rubric: abort — <reason>` *(only in - `stage=review-and-push` / `stage=all`)* +- `Revision passes: <N>` _(only in `stage=review-and-push` / `stage=all`)_ +- `Rubric: pass` _or_ `Rubric: abort — <reason>` _(only in + `stage=review-and-push` / `stage=all`)_ If `Rubric: abort`, no issue was created and no source edit was made. diff --git a/designdocs/SKILLS_DISCOVERY_REDESIGN.md b/designdocs/SKILLS_DISCOVERY_REDESIGN.md index 3869ccf2..225ef0b8 100644 --- a/designdocs/SKILLS_DISCOVERY_REDESIGN.md +++ b/designdocs/SKILLS_DISCOVERY_REDESIGN.md @@ -38,10 +38,10 @@ This doc references [`SKILLS_MANAGEMENT.md`](./SKILLS_MANAGEMENT.md) as the foun A managed skill is in exactly one of three states based on where its `SKILL.md` lives: -| State | Where SKILL.md lives | Enabled-agents source of truth | Notes | -|---|---|---|---| -| **Canonical** | `<vault>/copilot/skills/<name>/SKILL.md` | `metadata.copilot-enabled-agents` in frontmatter | Today's only state. Agent folders hold symlinks. | -| **Project (single)** | `<vault>/.<agent>/skills/<name>/SKILL.md` for exactly one agent | Inferred from the folder path | The skill belongs to one agent; no canonical copy, no symlinks. | +| State | Where SKILL.md lives | Enabled-agents source of truth | Notes | +| ---------------------- | ------------------------------------------------------------------------------------- | ------------------------------------------------------------ | ------------------------------------------------------------------------------------------- | +| **Canonical** | `<vault>/copilot/skills/<name>/SKILL.md` | `metadata.copilot-enabled-agents` in frontmatter | Today's only state. Agent folders hold symlinks. | +| **Project (single)** | `<vault>/.<agent>/skills/<name>/SKILL.md` for exactly one agent | Inferred from the folder path | The skill belongs to one agent; no canonical copy, no symlinks. | | **Project (mirrored)** | Identical `SKILL.md` directory trees in two or more `<vault>/.<agent>/skills/<name>/` | Inferred from which agent folders contain the identical copy | Common after a user manually copied a skill between agents; collapses to one row in the UI. | Identity rule: skills are identified by name. Two skills with the same name and identical directory contents (full recursive content hash, SKILL.md plus every support file) merge. Same name with **different** contents do not merge; see §10 (Edge cases). @@ -75,21 +75,19 @@ After all four walks, results are merged by `(name, dirContentHash)`: Extend the existing `Skill` interface (`src/agentMode/skills/types.ts`) with a `location` discriminator: ```ts -export type SkillLocation = - | { kind: "canonical" } - | { kind: "project"; agentDirs: BackendId[] }; +export type SkillLocation = { kind: "canonical" } | { kind: "project"; agentDirs: BackendId[] }; export interface Skill { name: string; description: string; - filePath: string; // absolute path to the SKILL.md actually used for display/open - dirPath: string; // absolute path to the directory holding that SKILL.md + filePath: string; // absolute path to the SKILL.md actually used for display/open + dirPath: string; // absolute path to the directory holding that SKILL.md body: string; // ...existing optional spec fields unchanged enabledAgents: BackendId[]; // For canonical skills: parsed from frontmatter. - // For project skills: equals location.agentDirs. + // For project skills: equals location.agentDirs. location: SkillLocation; - contentHash?: string; // present for project skills; used to detect mirrored duplicates + contentHash?: string; // present for project skills; used to detect mirrored duplicates } ``` @@ -99,17 +97,17 @@ For project-mirrored skills the chosen `filePath` / `dirPath` is the alphabetica The per-row agent toggle has three flavors based on current `Skill.location`: -| Current location | Toggle action | Effect | Confirmation? | -|---|---|---|---| -| Canonical | Toggle ON any agent | Update frontmatter; create symlink in `.<agent>/skills/`. | No (today's behavior). | -| Canonical | Toggle OFF any agent | Update frontmatter; remove symlink. SKILL.md stays in canonical. | No (today's behavior). | -| Project (single, agent A) | Toggle ON agent A | No-op (already enabled). | No. | -| **Project (single, agent A)** | **Toggle ON agent B** | **Migrate to canonical; create symlinks at A and B.** | **Yes** (unless suppressed). | -| **Project (single, agent A)** | **Toggle OFF agent A** | **Migrate to canonical with 0 enabled agents; SKILL.md is preserved under `copilot/skills/`; no symlinks.** | **Yes** (unless suppressed) — the dialog explains the file would otherwise have nowhere to live. | -| Project (mirrored, A+B) | Toggle ON A or B | No-op (already enabled). | No. | -| **Project (mirrored, A+B)** | **Toggle ON agent C** | **Migrate to canonical; delete the duplicate project folders; create symlinks at A, B, C.** | **Yes** (unless suppressed). | -| Project (mirrored, A+B) | Toggle OFF agent A (with B still enabled) | Delete `.A/skills/<name>/`. The skill remains a project row, now single-agent B. | No. Show a brief `Notice` (`Removed foo from Claude project folder; still active in Codex.`). | -| **Project (mirrored, A+B)** | **Toggle OFF the last remaining agent** | **Migrate the surviving copy to canonical with 0 enabled agents; delete the source project folder.** | **Yes** (unless suppressed) — same dialog as the project-single off case, body lists the one surviving copy. | +| Current location | Toggle action | Effect | Confirmation? | +| ----------------------------- | ----------------------------------------- | ----------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------ | +| Canonical | Toggle ON any agent | Update frontmatter; create symlink in `.<agent>/skills/`. | No (today's behavior). | +| Canonical | Toggle OFF any agent | Update frontmatter; remove symlink. SKILL.md stays in canonical. | No (today's behavior). | +| Project (single, agent A) | Toggle ON agent A | No-op (already enabled). | No. | +| **Project (single, agent A)** | **Toggle ON agent B** | **Migrate to canonical; create symlinks at A and B.** | **Yes** (unless suppressed). | +| **Project (single, agent A)** | **Toggle OFF agent A** | **Migrate to canonical with 0 enabled agents; SKILL.md is preserved under `copilot/skills/`; no symlinks.** | **Yes** (unless suppressed) — the dialog explains the file would otherwise have nowhere to live. | +| Project (mirrored, A+B) | Toggle ON A or B | No-op (already enabled). | No. | +| **Project (mirrored, A+B)** | **Toggle ON agent C** | **Migrate to canonical; delete the duplicate project folders; create symlinks at A, B, C.** | **Yes** (unless suppressed). | +| Project (mirrored, A+B) | Toggle OFF agent A (with B still enabled) | Delete `.A/skills/<name>/`. The skill remains a project row, now single-agent B. | No. Show a brief `Notice` (`Removed foo from Claude project folder; still active in Codex.`). | +| **Project (mirrored, A+B)** | **Toggle OFF the last remaining agent** | **Migrate the surviving copy to canonical with 0 enabled agents; delete the source project folder.** | **Yes** (unless suppressed) — same dialog as the project-single off case, body lists the one surviving copy. | Migration always lands the SKILL.md in `<vault>/<configured-skills-folder>/<name>/`. Name collisions with an existing canonical skill use today's suffix-on-collision helper (`foo`, `foo-2`, `foo-3`, …) and the resulting name is shown in the confirmation before commit. @@ -245,14 +243,15 @@ External edit propagation: if the user edits `.claude/skills/foo/SKILL.md` direc **User opens the overflow menu on a project-mirrored skill — lockdown.** The mirrored state is fragile: modifying one copy diverges it from the others, breaking the merge silently. To prevent that, the overflow menu collapses to a single actionable entry when `skill.location.kind === "project"` and `skill.location.agentDirs.length >= 2`: -- The four normal entries — *Edit SKILL.md*, *Edit properties*, *Rename*, *Delete* — render in their muted/disabled state (greyed text, no hover affordance, `aria-disabled="true"`). Clicking them does nothing. +- The four normal entries — _Edit SKILL.md_, _Edit properties_, _Rename_, _Delete_ — render in their muted/disabled state (greyed text, no hover affordance, `aria-disabled="true"`). Clicking them does nothing. - A single new entry, **Migrate to shared folder**, renders at the top of the menu in its enabled state. Clicking it opens the proactive-consolidate variant of `MigrateSkillConfirmModal` described in §8. - Hovering anywhere on the menu (or specifically on any muted entry) reveals a tooltip with the verbatim copy: > This skill is duplicated in {N} agent folders. Editing one copy would silently diverge from the others. Migrate it to your shared folder first to enable edits. ({N} is the live count, e.g. `2 agent folders`.) -- *Reveal in vault* is the one read-only action that stays enabled. It opens whichever copy `Skill.dirPath` resolved to (alphabetically-first agent dir, deterministic). + +- _Reveal in vault_ is the one read-only action that stays enabled. It opens whichever copy `Skill.dirPath` resolved to (alphabetically-first agent dir, deterministic). Once migration completes, the skill is canonical and the overflow menu returns to its normal full state on the next render. No menu state needs to be remembered across renders — the lockdown is a pure function of `Skill.location`. @@ -417,7 +416,7 @@ Reference paths the design doc points at: - **"Don't ask again" is a single global flag.** Per-source-agent or per-pair variants were considered but rejected: the dialog body is the same shape regardless of which agent the source is, and the user's mental model is "I trust Copilot to do migrations" or "I want to be asked each time." More granular suppression complicates the settings surface without adding clarity. - **The migration dialog is a confirmation, not a wizard.** No source-selection UI; the rows that need migration are already disambiguated by the discovery merge. - **Project-managed skills are not part of reconciliation.** Reconciliation is about keeping agent symlinks in sync with canonical frontmatter. A project-managed skill has no canonical SKILL.md to disagree with. -- **Project-mirrored skills lock down their overflow menu to a single Migrate action.** Alternative: apply the user's edit/rename/delete to every copy atomically. Rejected because it hides the duplicate-state from the user (the next external edit to one copy still diverges them silently, and there is no UI signal that they were ever in a fragile state). The lockdown teaches the user that the duplicates need to be resolved before edits, and the Migrate entry is one click away. The tooltip explains *why* on hover so the lockdown does not feel arbitrary. +- **Project-mirrored skills lock down their overflow menu to a single Migrate action.** Alternative: apply the user's edit/rename/delete to every copy atomically. Rejected because it hides the duplicate-state from the user (the next external edit to one copy still diverges them silently, and there is no UI signal that they were ever in a fragile state). The lockdown teaches the user that the duplicates need to be resolved before edits, and the Migrate entry is one click away. The tooltip explains _why_ on hover so the lockdown does not feel arbitrary. - **The spawn-time skill-creation directive is removed entirely** (see §11b). With unified discovery, an agent writing to its own native skills directory (`.claude/skills/`, `.agents/skills/`, `.opencode/skills/`) is the desired behavior — discovery will pick the new skill up as project-managed. Telling each agent to override its native convention and write into `copilot/skills/` was a workaround for the canonical-only discovery model and serves no purpose under the new model. ## 17. References diff --git a/designdocs/agents/TESTING_GUIDE.md b/designdocs/agents/TESTING_GUIDE.md index 16b91087..2c2d5831 100644 --- a/designdocs/agents/TESTING_GUIDE.md +++ b/designdocs/agents/TESTING_GUIDE.md @@ -74,6 +74,35 @@ fixture omits `_keychainOnly`, so the plugin loads in disk mode and ignores any API keys left in the OS keychain from prior runs — a deterministic clean state. The source file is validated as JSON before the target is touched. +### `data.legacy-byok.json` — one-time BYOK migration + +`npm run test:reset-data -- scripts/test-fixtures/data.legacy-byok.json` loads a +pre-versioned legacy install (no `settingsVersion`; legacy `activeModels` + +top-level provider keys; empty new slices apart from one pre-seeded BYOK +Anthropic provider that proves dedup). On the next plugin load, +`runSettingsMigrations` converts the legacy BYOK providers/models into the new +`providers` / `configuredModels` / `backends` shape so they work in OpenCode, and +stamps `settingsVersion`. It runs in disk mode with obvious fake `sk-test-…` +keys. After reset, read back and verify the migrated shape: + +```bash +$OBS vault=$VAULT eval code='app.plugins.plugins.copilot.loadData().then(d=>JSON.stringify({ + settingsVersion: d.settingsVersion, + providers: Object.values(d.providers).map(p=>({type:p.providerType, origin:p.origin.kind, catalog:p.origin.catalogProviderId, hasKey: p.apiKeyKeychainId!=null})), + configuredModels: d.configuredModels.map(m=>m.info.id), + chat: d.backends.chat?.enabledModels?.length, + opencode: d.backends.opencode?.enabledModels?.length, + legacyUntouched: { anthropicApiKey: d.anthropicApiKey, activeModels: d.activeModels.length } +}))' +``` + +Expect: `settingsVersion` stamped; routable providers carry a `catalogProviderId` +and a key; `backends.opencode.enabledModels` is non-empty and excludes the +embedding (`BAAI/bge-m3`) and Bedrock rows; exactly one Anthropic BYOK provider +(the pre-seeded one — its descriptor deduped, not re-created); legacy +`anthropicApiKey` and `activeModels` untouched. A second reload must not change +`settingsVersion` or create duplicates (the version gate holds). + ## 0. Golden rule: pick the right window first Obsidian is a single Electron app with one renderer per open vault. The CLI diff --git a/designdocs/todo/AGENT_MODE_TODOS.md b/designdocs/todo/AGENT_MODE_TODOS.md index 1cd4908b..222c0322 100644 --- a/designdocs/todo/AGENT_MODE_TODOS.md +++ b/designdocs/todo/AGENT_MODE_TODOS.md @@ -4,99 +4,67 @@ - [x] Load chat history from agents - [x] Save chat history to notes - [x] P0: Thoroughly assess whether migrating to Anthropic agent SDK is worth it. -- [ ] P0: Test Windows devices +- [ ] P0: [Validate Agent Mode end-to-end on Windows](https://github.com/logancyang/obsidian-copilot-preview/issues/58) - [x] P0: Make sure bash command shows what command it runs (visibility) -- [ ] P0: Provide copilot specific system prompt - - [ ] Allow users to share system prompt - - [ ] Do a quick spike on the concrete behavior - - [ ] Investigate opencode provider specific prompt -- [ ] P0: Test sandbox mode +- [ ] P0: [Upgrade pinned opencode to latest version](https://github.com/logancyang/obsidian-copilot-preview/issues/59) +- [ ] P0: [Pass copilot built-in system prompt / user provided system prompt to agents](https://github.com/logancyang/obsidian-copilot-preview/issues/61) +- [ ] P0: [Test sandbox mode](https://github.com/logancyang/obsidian-copilot-preview/issues/60) - [x] P0: Skills - [x] Check out cc-switch to understand how to make skills compatible cross other agents https://github.com/farion1231/cc-switch - [x] P0: Permission management - [x] Permission UI improvement -- [ ] P0: How to design the settings to configure the provider? +- [x] P0: How to design the settings to configure the provider? - [x] Redesign the model settings - discussed on May 7 group meeting - [x] support self host model - [x] remove built-in models - - [ ] migrate existing models to the new format -- [ ] P0: Test opencode works well with local models -- [ ] P0: Test migration (skill, model) to make sure no unrecoverable migration is introduced in pre-release -- [ ] P0: 非migrate的skill最好能直接引用,同名的skill如果内容一样migrate可以做得更好 + - [x] migrate existing models to the new format +- [ ] P0: [Test opencode works well with local models](https://github.com/logancyang/obsidian-copilot-preview/issues/63) +- [ ] P0: [Test migration (skill, model) to make sure no unrecoverable migration is introduced in pre-release](https://github.com/logancyang/obsidian-copilot-preview/issues/64) +- [ ] P0: [非migrate的skill最好能直接引用,同名的skill如果内容一样migrate可以做得更好](https://github.com/logancyang/obsidian-copilot-preview/issues/62) - [x] P0: Auto-save chat history controls - [x] P0: Support image context -- [ ] P1: Better binary detection -- [ ] P1: MCP - - Basic functionality is ready - - [ ] P1: Surface externally-managed MCP servers (claude.ai remote, plugin-provided) — see [MCP_EXTERNALLY_MANAGED_SERVERS.md](./MCP_EXTERNALLY_MANAGED_SERVERS.md) - - [ ] P1: Support oauth for MCP servers (the one example that I tested didn't work) - - [ ] P1: Support setting MCP by copy pasting JSON blobs -- [ ] P1: Fix broken legacy agent mode - - [ ] Can we only support basic chat - not now but eventually yes +- [ ] P1: [Support claude code authentication](https://github.com/logancyang/obsidian-copilot-preview/issues/65) +- [ ] P1: [Better binary detection](https://github.com/logancyang/obsidian-copilot-preview/issues/66) +- [ ] P1: [MCP management](https://github.com/logancyang/obsidian-copilot-preview/issues/67) +- [ ] P1: [Support oauth for MCP servers](https://github.com/logancyang/obsidian-copilot-preview/issues/68) +- [ ] P1: [Support setting MCP by copy pasting JSON blobs](https://github.com/logancyang/obsidian-copilot-preview/issues/70) +- [ ] P1: [Fix broken legacy agent mode](https://github.com/logancyang/obsidian-copilot-preview/issues/69) - [x] P1: [BUG] Check active note path. Sometimes the agent will start from a path that does not exist - [x] P1: [Performance] Updating skills settings is laggy -- [ ] P1: Make askuserquestion tool show questions inline in the chat. -- [ ] P1: Improve binary detection -- [ ] P1: Citation - - [ ] a tool to let agent call citation -- [ ] P1: Content type support (image, audio) - https://agentclientprotocol.com/protocol/content -- [ ] P1: Edit diff UI - https://agentclientprotocol.com/protocol/tool-calls#diffs - - The edit diff should be based on well rendered markdown, not raw markdown file. For example, table is impossible to understand the diff with the raw format -- [ ] P1: Thoroughly test opencode, codex, and claude code with different test cases - - [ ] Create sample vaults for test cases. -- [ ] P1: Fix session title for claude code agent -- [ ] P1: Agent upgrade detection and helper UI in settings -- [ ] P1: Forward web-source context to the agent - - The agent should be able to access the content of the rendered tab - - Right-click "Add to Copilot context" excerpts from web tabs and the - "include active web tab" toggle currently surface a Notice and are - dropped before the prompt is built. Wire them into the - `<copilot-context>` envelope (e.g. a `Web excerpts:` section with - `title (url): content`) so the agent can actually read them. -- [ ] P1: Move PDF parsing into an agent tool - - Inline PDF parsing during send can take too long and blocks the user - before the agent starts. Keep PDF attachments as vault paths for the - built-in Read tool for now, then expose Copilot PDF parsing as an - explicit tool the agent can call when it needs extracted PDF text. -- [ ] P1: Token counter - - To know how many context is left in the current session - - Nice-to-have: Cost estimate -- [ ] P1: Integrate copilot plus tool calls (convert them to skills) - - [ ] vault search (make it work with Miyo) - - may want to rename - - challenge - how to enable it in an agent - - challenge - agentic search often is better than RAG, how does the agent know when to trigger it - - we don't need index-free search - - [ ] web search (paid feature only) - - [ ] ~~edit~~ - - [ ] youtube transcription (paid feature only) - - [ ] obsidian CLI -- [ ] P1: Opencode plan mode fine-tuning -- [ ] P1: compaction - - [ ] manual trigger - - [ ] configure when to auto compact -- [ ] P1: Project mode -- [ ] P2: Inherit model effort from previous model when selecting a new model in the picker - - It's quite tedious today that sometimes the effort will drop to low. -- [ ] P2: Support subscriptions that work with opencode (kimi code) -- [ ] P2: Claude vscode plugin add comment to plan capability - - It makes iterating on plan a lot easier +- [ ] P1: [Make AskUserQuestion tool show questions inline in the chat like permission instead of in a dialog](https://github.com/logancyang/obsidian-copilot-preview/issues/71) +- [ ] P1: [Citation](https://github.com/logancyang/obsidian-copilot-preview/issues/72) +- [ ] P1: [Edit diff UI](https://github.com/logancyang/obsidian-copilot-preview/issues/73) +- [ ] P1: [Fix session title for claude code agent](https://github.com/logancyang/obsidian-copilot-preview/issues/75) +- [ ] P1: [Agent upgrade detection and helper UI in settings](https://github.com/logancyang/obsidian-copilot-preview/issues/76) +- [ ] P1: [Forward web-source context to the agent](https://github.com/logancyang/obsidian-copilot-preview/issues/77) +- [ ] P1: [Move PDF parsing into an agent tool](https://github.com/logancyang/obsidian-copilot-preview/issues/79) +- [ ] P1: [Token counter](https://github.com/logancyang/obsidian-copilot-preview/issues/78) +- P1: Integrate copilot plus tool calls (convert them to skills) + - [ ] [vault search (make it work with Miyo)](https://github.com/logancyang/obsidian-copilot-preview/issues/82) + - [ ] [web search (paid feature only)](https://github.com/logancyang/obsidian-copilot-preview/issues/80) + - [ ] [deprecate "edit" and "composer"](https://github.com/logancyang/obsidian-copilot-preview/issues/81) + - [ ] [youtube transcription (paid feature only)](https://github.com/logancyang/obsidian-copilot-preview/issues/83) + - [ ] [obsidian CLI](https://github.com/logancyang/obsidian-copilot-preview/issues/84) +- [ ] P1: [support compaction](https://github.com/logancyang/obsidian-copilot-preview/issues/87) +- [ ] P1: [Allow comment in plan mode](https://github.com/logancyang/obsidian-copilot-preview/issues/86) +- [ ] P1: [Project mode](https://github.com/logancyang/obsidian-copilot-preview/issues/85) +- [ ] P1: [Move relevant notes to its own view](https://github.com/logancyang/obsidian-copilot-preview/issues/88) +- [ ] P1: [Settings page revamp](https://github.com/logancyang/obsidian-copilot-preview/issues/89) +- [ ] P2: [Support subscriptions that work with opencode (kimi code)](https://github.com/logancyang/obsidian-copilot-preview/issues/91) +- [ ] P2: [Claude vscode plugin add comment to plan capability](https://github.com/logancyang/obsidian-copilot-preview/issues/90) - [x] P2: [UX] Make mode more obvious - idea: consider change the chat border color for different modes - [x] P2: [UX] fix the brief moment of "Read Read" tool call message - Also made every tool-card verb status-aware ("Reading…" / "Read foo.md", "Editing draft.md" / "Edited draft.md", "Fetching url" / "Fetched url", etc.) so in-flight calls no longer render with past-tense verbs. -- [ ] P2: Edit previous user message -- [ ] P2: Support batch editing skills -- [ ] P2: New agent command (/new, /usage) -- [ ] P2: Claude code / Codex authentication -- [ ] P2: Steering conversation (instead of queue) -- [ ] P2: Rollback everything to the state of previous message -- [ ] P2: In-product QA agent to debug for users -- [ ] P3: Rerun agent response -- [ ] P3: Agent todo list - - [ ] make sure in case it renders, it won't be buggy +- [ ] P2: [Edit previous user message](https://github.com/logancyang/obsidian-copilot-preview/issues/92) +- [ ] P2: [Agent command (/new)](https://github.com/logancyang/obsidian-copilot-preview/issues/93) +- [ ] P2: [Steering conversation (instead of queue)](https://github.com/logancyang/obsidian-copilot-preview/issues/94) +- [ ] P2: [Rollback everything to the state of previous message](https://github.com/logancyang/obsidian-copilot-preview/issues/95) +- [ ] P2: [In-product QA agent to debug for users](https://github.com/logancyang/obsidian-copilot-preview/issues/96) +- [ ] P3: [Rerun agent response](https://github.com/logancyang/obsidian-copilot-preview/issues/97) +- [ ] P3: [Copy agent message](https://github.com/logancyang/obsidian-copilot-preview/issues/98) - [x] P1: Queue messages - [x] P1: Only include the provided models - hide openrouter models behind a modal selector diff --git a/designdocs/todo/AGENT_TODOS_TO_ISSUES_PLAYBOOK.md b/designdocs/todo/AGENT_TODOS_TO_ISSUES_PLAYBOOK.md index 2da23dee..8481f01f 100644 --- a/designdocs/todo/AGENT_TODOS_TO_ISSUES_PLAYBOOK.md +++ b/designdocs/todo/AGENT_TODOS_TO_ISSUES_PLAYBOOK.md @@ -6,7 +6,7 @@ in `logancyang/obsidian-copilot-preview`, replacing each migrated TODO line with a link to its issue. The orchestrator does **not** write issues itself. It triages, then spawns a -pair of subagents per chunk — an *author* that drafts, and a *reviewer* that +pair of subagents per chunk — an _author_ that drafts, and a _reviewer_ that re-verifies, revises, and pushes. The orchestrator owns the source-file write to avoid races. @@ -76,9 +76,10 @@ a chunk record. **Record the triage** in `.context/issue-migration-plan.md` as a table: ```md -| chunk-id | parent line | priority | folded sub-lines | split-out sub-lines | source-line-range | -|----------|-------------|----------|------------------|---------------------|-------------------| -| windows-test | 7 | P0 | 8 | — | 7-8 | +| chunk-id | parent line | priority | folded sub-lines | split-out sub-lines | source-line-range | +| ------------ | ----------- | -------- | ---------------- | ------------------- | ----------------- | +| windows-test | 7 | P0 | 8 | — | 7-8 | + | ... ``` diff --git a/scripts/test-fixtures/data.legacy-byok.json b/scripts/test-fixtures/data.legacy-byok.json new file mode 100644 index 00000000..d562518c --- /dev/null +++ b/scripts/test-fixtures/data.legacy-byok.json @@ -0,0 +1,74 @@ +{ + "userId": "test-legacy-byok", + "anthropicApiKey": "sk-test-anthropic-0000", + "openAIApiKey": "sk-test-openai-0000", + "openAIOrgId": "org-test-0000", + "openRouterAiApiKey": "sk-test-openrouter-toplevel", + "siliconflowApiKey": "sk-test-siliconflow-0000", + "googleApiKey": "", + "amazonBedrockApiKey": "sk-test-bedrock-0000", + "amazonBedrockRegion": "us-east-1", + "activeModels": [ + { "name": "claude-sonnet-4-5", "provider": "anthropic", "enabled": true, "isBuiltIn": false }, + { "name": "gpt-4o", "provider": "openai", "enabled": true, "isBuiltIn": false }, + { "name": "gpt-5.5", "provider": "openai", "enabled": true, "isBuiltIn": true, "core": true }, + { "name": "x-ai/grok-4.3", "provider": "openrouterai", "enabled": true, "isBuiltIn": false }, + { + "name": "qwen/qwen-2.5-coder", + "provider": "openrouterai", + "enabled": true, + "isBuiltIn": false, + "apiKey": "sk-test-openrouter-per-model" + }, + { + "name": "deepseek-ai/DeepSeek-V3", + "provider": "siliconflow", + "enabled": true, + "isBuiltIn": false, + "baseUrl": "https://api.siliconflow.com/v1" + }, + { + "name": "BAAI/bge-m3", + "provider": "siliconflow", + "enabled": true, + "isBuiltIn": false, + "isEmbeddingModel": true + }, + { + "name": "anthropic.claude-3-5-sonnet-v2", + "provider": "amazon-bedrock", + "enabled": true, + "isBuiltIn": false + }, + { "name": "gemini-2.5-pro", "provider": "google", "enabled": true, "isBuiltIn": false }, + { + "name": "deepseek/deepseek-chat", + "provider": "openrouterai", + "enabled": false, + "isBuiltIn": false + }, + { "name": "copilot-plus-flash", "provider": "copilot-plus", "enabled": true, "isBuiltIn": true } + ], + "providers": { + "existing-anthropic-1": { + "providerId": "existing-anthropic-1", + "providerType": "anthropic", + "displayName": "My Anthropic (pre-existing)", + "baseUrl": "https://api.anthropic.com", + "origin": { "kind": "byok", "catalogProviderId": "anthropic" }, + "apiKeyKeychainId": null, + "addedAt": 1735689600000 + } + }, + "configuredModels": [ + { + "configuredModelId": "existing-cm-1", + "providerId": "existing-anthropic-1", + "info": { "id": "claude-sonnet-4-5", "displayName": "Claude Sonnet 4.5" }, + "configuredAt": 1735689600000 + } + ], + "backends": { + "chat": { "enabledModels": ["existing-cm-1"] } + } +} diff --git a/src/agentMode/index.ts b/src/agentMode/index.ts index a4edc97e..4e58a7d2 100644 --- a/src/agentMode/index.ts +++ b/src/agentMode/index.ts @@ -29,6 +29,7 @@ export type { EnrolledModelRef } from "./session/agentDefaultEnable"; export { partitionOpencodeOnlyWireIds } from "./backends/opencode/opencodeProbePartition"; export { mapProviderToOpencodeId } from "./backends/opencode/opencodeModelResolve"; export type { OpencodeProviderMapping } from "./backends/opencode/opencodeModelResolve"; +export { installBadge, InstallBadge, InstallStatusLine } from "./backends/shared/installStatus"; export type { BackendState, CopilotMode, diff --git a/src/main.ts b/src/main.ts index 6cab9caa..99d3821e 100644 --- a/src/main.ts +++ b/src/main.ts @@ -66,6 +66,7 @@ import { } from "@/services/webViewerService/webViewerServiceSingleton"; import { WebSelectionTracker } from "@/services/webViewerService/webViewerServiceSelection"; import VectorStoreManager from "@/search/vectorStoreManager"; +import { runSettingsMigrations } from "@/settings/migrations"; import { CopilotSettingTab } from "@/settings/SettingsPage"; import { getModelKeyFromModel, @@ -195,6 +196,12 @@ export default class CopilotPlugin extends Plugin { } })(); }); + // One-time settings migrations. Runs after the persist subscriber is wired + // (so every mutation is saved) and after createModelManagement, and before + // agent/model-discovery init below — so migrated BYOK providers are present + // when OpenCode first enumerates models. Awaited for deterministic ordering; + // it's a fast, one-time, no-op for already-migrated/fresh vaults. + await runSettingsMigrations(this.modelManagement); this.addSettingTab(new CopilotSettingTab(this.app, this)); // Core plugin initialization diff --git a/src/services/settingsPersistence.test.ts b/src/services/settingsPersistence.test.ts index 460e9201..bd8cc464 100644 --- a/src/services/settingsPersistence.test.ts +++ b/src/services/settingsPersistence.test.ts @@ -8,6 +8,7 @@ if (typeof window.structuredClone === "undefined") { import { DEFAULT_SETTINGS } from "@/constants"; import type { CustomModel } from "@/aiParams"; +import { CURRENT_SETTINGS_VERSION } from "@/settings/migrations/version"; import type { CopilotSettings } from "@/settings/model"; /** Match the production secret-key heuristic without importing the real module. */ @@ -168,6 +169,11 @@ describe("loadSettingsWithKeychain", () => { const loaded = await mod.loadSettingsWithKeychain(null, saveData); expect((loaded as unknown as Record<string, unknown>)._keychainOnly).toBe(true); + // Fresh installs are stamped at the current schema version so the one-time + // migrations never run for them. + expect((loaded as unknown as Record<string, unknown>).settingsVersion).toBe( + CURRENT_SETTINGS_VERSION + ); // Reason: even on fresh install, hydrateFromKeychain still runs so // tombstones / pre-existing keychain entries are honored. expect(keychain.hydrateFromKeychain).toHaveBeenCalled(); diff --git a/src/services/settingsPersistence.ts b/src/services/settingsPersistence.ts index be82afbf..58489692 100644 --- a/src/services/settingsPersistence.ts +++ b/src/services/settingsPersistence.ts @@ -39,6 +39,7 @@ import { // but only used in code paths that run AFTER settings are loaded. Functions that // run DURING settings loading (loadSettingsWithKeychain) use console.* directly. import { logError, logWarn } from "@/logger"; +import { CURRENT_SETTINGS_VERSION } from "@/settings/migrations/version"; import { Notice } from "obsidian"; // --------------------------------------------------------------------------- @@ -357,6 +358,9 @@ export async function loadSettingsWithKeychain( // nothing changes" rule for users who manually deleted their keys. if (isFreshInstall) { (settings as unknown as Record<string, unknown>)._keychainOnly = true; + // Stamp the current schema version so brand-new vaults skip the one-time + // migrations entirely (nothing to migrate; avoids a needless save). + settings.settingsVersion = CURRENT_SETTINGS_VERSION; } // ---- Vault namespace ID bootstrap (shared by both modes). ---- @@ -399,6 +403,7 @@ export async function loadSettingsWithKeychain( }; if (isFreshInstall) { currentDisk._keychainOnly = true; + currentDisk.settingsVersion = CURRENT_SETTINGS_VERSION; } await saveData(currentDisk as unknown as CopilotSettings); rawDiskData = currentDisk; diff --git a/src/settings/migrations/byokMigration.test.ts b/src/settings/migrations/byokMigration.test.ts new file mode 100644 index 00000000..3f056b81 --- /dev/null +++ b/src/settings/migrations/byokMigration.test.ts @@ -0,0 +1,405 @@ +/** + * Unit tests for the BYOK migration. `planByokMigration` is pure, so the bulk + * of the coverage builds a legacy settings object and asserts the resulting + * `SetupProviderInput[]`. `executeByokMigration` is exercised against a fake + * `ModelManagementApi` (the real enrollment behavior of `setupProvider` is + * covered by `ByokSetupApi.test.ts`; here we only assert which descriptors flow + * through and that dedup / per-provider-failure handling hold). + * + * All `@/modelManagement` imports are type-only so the model-management barrel + * (and its UI deps) never loads in this unit test. + */ + +import type { CustomModel } from "@/aiParams"; +import { ChatModelProviders, DEFAULT_SETTINGS } from "@/constants"; +import type { ModelManagementApi, Provider, SetupProviderInput } from "@/modelManagement"; +import type { CopilotSettings } from "@/settings/model"; + +import { executeByokMigration, planByokMigration } from "./byokMigration"; + +jest.mock("@/logger", () => ({ + logInfo: jest.fn(), + logWarn: jest.fn(), + logError: jest.fn(), +})); + +function model(overrides: Partial<CustomModel>): CustomModel { + return { + name: "test-model", + provider: ChatModelProviders.OPENAI, + enabled: true, + isBuiltIn: false, + ...overrides, + }; +} + +function settingsWith( + models: CustomModel[], + overrides: Partial<CopilotSettings> = {} +): CopilotSettings { + return { ...DEFAULT_SETTINGS, ...overrides, activeModels: models }; +} + +const normalizeUrl = (url: string | undefined) => + (url ?? "").trim().replace(/\/+$/, "").toLowerCase(); + +const byCatalog = (plan: SetupProviderInput[], catalogProviderId: string) => + plan.find((p) => p.catalogProviderId === catalogProviderId); + +const byBaseUrl = (plan: SetupProviderInput[], baseUrl: string) => + plan.find((p) => !p.catalogProviderId && normalizeUrl(p.baseUrl) === normalizeUrl(baseUrl)); + +describe("planByokMigration — provider mapping", () => { + it("maps catalog-backed providers to the right providerType + catalogProviderId", () => { + const plan = planByokMigration( + settingsWith( + [ + model({ name: "claude-sonnet-4-5", provider: ChatModelProviders.ANTHROPIC }), + model({ name: "gpt-4o", provider: ChatModelProviders.OPENAI }), + model({ name: "gemini-2.5-pro", provider: ChatModelProviders.GOOGLE }), + model({ name: "x-ai/grok-4.3", provider: ChatModelProviders.OPENROUTERAI }), + model({ name: "grok-4", provider: ChatModelProviders.XAI }), + model({ name: "llama-3.3-70b", provider: ChatModelProviders.GROQ }), + model({ name: "mistral-large", provider: ChatModelProviders.MISTRAL }), + model({ name: "deepseek-chat", provider: ChatModelProviders.DEEPSEEK }), + ], + { + anthropicApiKey: "k", + openAIApiKey: "k", + googleApiKey: "k", + openRouterAiApiKey: "k", + xaiApiKey: "k", + groqApiKey: "k", + mistralApiKey: "k", + deepseekApiKey: "k", + } + ) + ); + + expect(byCatalog(plan, "anthropic")).toMatchObject({ providerType: "anthropic" }); + expect(byCatalog(plan, "openai")).toMatchObject({ providerType: "openai-compatible" }); + expect(byCatalog(plan, "google")).toMatchObject({ providerType: "google" }); + expect(byCatalog(plan, "openrouter")).toMatchObject({ providerType: "openai-compatible" }); + expect(byCatalog(plan, "xai")).toMatchObject({ providerType: "openai-compatible" }); + expect(byCatalog(plan, "groq")).toMatchObject({ providerType: "openai-compatible" }); + expect(byCatalog(plan, "mistral")).toMatchObject({ providerType: "openai-compatible" }); + expect(byCatalog(plan, "deepseek")).toMatchObject({ providerType: "openai-compatible" }); + }); + + it("maps catalog-less SiliconFlow to openai-compatible with its default base URL", () => { + const plan = planByokMigration( + settingsWith( + [model({ name: "deepseek-ai/DeepSeek-V3", provider: ChatModelProviders.SILICONFLOW })], + { + siliconflowApiKey: "k", + } + ) + ); + const sf = byBaseUrl(plan, "https://api.siliconflow.com/v1"); + expect(sf).toMatchObject({ providerType: "openai-compatible" }); + expect(sf?.catalogProviderId).toBeUndefined(); + }); + + it("enrolls routable providers into chat + opencode", () => { + const plan = planByokMigration( + settingsWith([model({ name: "x", provider: ChatModelProviders.OPENROUTERAI })], { + openRouterAiApiKey: "k", + }) + ); + expect(byCatalog(plan, "openrouter")?.autoEnrollIn).toEqual(["chat", "opencode"]); + }); +}); + +describe("planByokMigration — grouping, keys, base URLs", () => { + it("groups multiple models of one provider under a single descriptor", () => { + const plan = planByokMigration( + settingsWith( + [ + model({ name: "x-ai/grok-4.3", provider: ChatModelProviders.OPENROUTERAI }), + model({ name: "deepseek/deepseek-chat", provider: ChatModelProviders.OPENROUTERAI }), + model({ name: "qwen/qwen-2.5", provider: ChatModelProviders.OPENROUTERAI }), + ], + { openRouterAiApiKey: "k" } + ) + ); + expect(plan).toHaveLength(1); + expect(plan[0].models.map((m) => m.id).sort()).toEqual([ + "deepseek/deepseek-chat", + "qwen/qwen-2.5", + "x-ai/grok-4.3", + ]); + }); + + it("prefers a per-model apiKey over the top-level provider key", () => { + const plan = planByokMigration( + settingsWith( + [model({ name: "x", provider: ChatModelProviders.OPENROUTERAI, apiKey: "sk-per-model" })], + { openRouterAiApiKey: "sk-top-level" } + ) + ); + expect(byCatalog(plan, "openrouter")?.apiKey).toBe("sk-per-model"); + }); + + it("falls back to the top-level provider key", () => { + const plan = planByokMigration( + settingsWith([model({ name: "x", provider: ChatModelProviders.OPENROUTERAI })], { + openRouterAiApiKey: "sk-top-level", + }) + ); + expect(byCatalog(plan, "openrouter")?.apiKey).toBe("sk-top-level"); + }); + + it("uses the model's baseUrl when set, else the provider default", () => { + const custom = planByokMigration( + settingsWith( + [ + model({ + name: "x", + provider: ChatModelProviders.OPENROUTERAI, + baseUrl: "https://proxy.local/v1", + }), + ], + { openRouterAiApiKey: "k" } + ) + ); + expect(byCatalog(custom, "openrouter")?.baseUrl).toBe("https://proxy.local/v1"); + + const fallback = planByokMigration( + settingsWith([model({ name: "claude", provider: ChatModelProviders.ANTHROPIC })], { + anthropicApiKey: "k", + }) + ); + expect(byCatalog(fallback, "anthropic")?.baseUrl).toBe("https://api.anthropic.com"); + }); + + it("captures openAIOrgId in extras for OpenAI", () => { + const plan = planByokMigration( + settingsWith([model({ name: "gpt-4o", provider: ChatModelProviders.OPENAI })], { + openAIApiKey: "k", + openAIOrgId: "org-123", + }) + ); + expect(byCatalog(plan, "openai")?.extras).toEqual({ openAIOrgId: "org-123" }); + }); +}); + +describe("planByokMigration — scope filters", () => { + it("skips embedding models (flag or id heuristic) but keeps chat models", () => { + const plan = planByokMigration( + settingsWith( + [ + model({ name: "gpt-4o", provider: ChatModelProviders.OPENAI }), + model({ + name: "text-embedding-3-large", + provider: ChatModelProviders.OPENAI, + isEmbeddingModel: true, + }), + model({ name: "nomic-embed-text", provider: ChatModelProviders.OPENAI }), + ], + { openAIApiKey: "k" } + ) + ); + expect(byCatalog(plan, "openai")?.models.map((m) => m.id)).toEqual(["gpt-4o"]); + }); + + it("skips disabled models", () => { + const plan = planByokMigration( + settingsWith( + [model({ name: "gpt-4o", provider: ChatModelProviders.OPENAI, enabled: false })], + { + openAIApiKey: "k", + } + ) + ); + expect(plan).toEqual([]); + }); + + it("skips copilot-plus and github-copilot", () => { + const plan = planByokMigration( + settingsWith( + [ + model({ + name: "copilot-plus-flash", + provider: ChatModelProviders.COPILOT_PLUS, + isBuiltIn: true, + }), + model({ name: "gpt-5", provider: ChatModelProviders.GITHUB_COPILOT }), + ], + { plusLicenseKey: "lic", githubCopilotToken: "tok" } + ) + ); + expect(plan).toEqual([]); + }); + + it("skips providers without a usable key", () => { + const plan = planByokMigration( + settingsWith([model({ name: "gpt-4o", provider: ChatModelProviders.OPENAI })], { + openAIApiKey: "", + }) + ); + expect(plan).toEqual([]); + }); + + it("includes enabled built-in models when the provider has a key", () => { + const plan = planByokMigration( + settingsWith( + [ + model({ + name: "gpt-5.5", + provider: ChatModelProviders.OPENAI, + isBuiltIn: true, + core: true, + }), + ], + { openAIApiKey: "k" } + ) + ); + expect(byCatalog(plan, "openai")?.models.map((m) => m.id)).toEqual(["gpt-5.5"]); + }); + + it("returns [] for empty activeModels", () => { + expect(planByokMigration(settingsWith([]))).toEqual([]); + }); +}); + +describe("planByokMigration — Azure / Bedrock (chat only)", () => { + it("maps Azure to chat-only with azure extras", () => { + const plan = planByokMigration( + settingsWith([model({ name: "gpt-4o", provider: ChatModelProviders.AZURE_OPENAI })], { + azureOpenAIApiKey: "k", + azureOpenAIApiInstanceName: "my-instance", + azureOpenAIApiDeploymentName: "my-deploy", + azureOpenAIApiVersion: "2024-06-01", + }) + ); + const azure = plan.find((p) => p.providerType === "azure"); + expect(azure).toBeDefined(); + expect(azure?.catalogProviderId).toBeUndefined(); + expect(azure?.autoEnrollIn).toEqual(["chat"]); + expect(azure?.extras).toEqual({ + azureInstanceName: "my-instance", + azureDeploymentName: "my-deploy", + azureApiVersion: "2024-06-01", + }); + }); + + it("maps Bedrock to chat-only with the region in extras", () => { + const plan = planByokMigration( + settingsWith([model({ name: "claude-3-5", provider: ChatModelProviders.AMAZON_BEDROCK })], { + amazonBedrockApiKey: "k", + amazonBedrockRegion: "us-east-1", + }) + ); + const bedrock = plan.find((p) => p.providerType === "bedrock"); + expect(bedrock?.autoEnrollIn).toEqual(["chat"]); + expect(bedrock?.extras).toEqual({ bedrockRegion: "us-east-1" }); + }); +}); + +describe("planByokMigration — local providers (custom URL required)", () => { + it("migrates Ollama / LM Studio only when an explicit baseUrl is set", () => { + const withUrl = planByokMigration( + settingsWith([ + model({ + name: "llama3.2", + provider: ChatModelProviders.OLLAMA, + baseUrl: "http://192.168.1.5:11434/v1", + }), + ]) + ); + expect(withUrl).toHaveLength(1); + expect(withUrl[0]).toMatchObject({ + providerType: "openai-compatible", + baseUrl: "http://192.168.1.5:11434/v1", + autoEnrollIn: ["chat", "opencode"], + }); + + const withoutUrl = planByokMigration( + settingsWith([model({ name: "llama3.2", provider: ChatModelProviders.LM_STUDIO })]) + ); + expect(withoutUrl).toEqual([]); + }); +}); + +describe("executeByokMigration", () => { + let idSeq = 0; + function makeApi(existing: Provider[] = []) { + const setupProvider = jest.fn(async (input: SetupProviderInput) => ({ + providerId: `prov-${++idSeq}`, + configuredModelIds: input.models.map((_, i) => `cm-${idSeq}-${i}`), + })); + const listByOrigin = jest.fn((kind: string) => existing.filter((p) => p.origin.kind === kind)); + const api = { + providerRegistry: { listByOrigin }, + setup: { byok: { setupProvider } }, + } as unknown as ModelManagementApi; + return { api, setupProvider }; + } + + function byokProvider(overrides: Partial<Provider>): Provider { + return { + providerId: "existing", + providerType: "anthropic", + displayName: "Existing", + origin: { kind: "byok", catalogProviderId: "anthropic" }, + addedAt: 0, + apiKeyKeychainId: null, + ...overrides, + }; + } + + it("creates one provider per planned descriptor", async () => { + const { api, setupProvider } = makeApi(); + await executeByokMigration( + api, + settingsWith( + [ + model({ name: "claude", provider: ChatModelProviders.ANTHROPIC }), + model({ name: "or-x", provider: ChatModelProviders.OPENROUTERAI }), + ], + { anthropicApiKey: "k", openRouterAiApiKey: "k" } + ) + ); + expect(setupProvider).toHaveBeenCalledTimes(2); + }); + + it("skips a descriptor that duplicates an existing BYOK provider", async () => { + const existing = byokProvider({ + providerType: "anthropic", + baseUrl: "https://api.anthropic.com", + origin: { kind: "byok", catalogProviderId: "anthropic" }, + }); + const { api, setupProvider } = makeApi([existing]); + await executeByokMigration( + api, + settingsWith( + [ + model({ name: "claude", provider: ChatModelProviders.ANTHROPIC }), + model({ name: "gpt-4o", provider: ChatModelProviders.OPENAI }), + ], + { anthropicApiKey: "k", openAIApiKey: "k" } + ) + ); + // Anthropic deduped; only OpenAI created. + expect(setupProvider).toHaveBeenCalledTimes(1); + expect(setupProvider.mock.calls[0][0]).toMatchObject({ catalogProviderId: "openai" }); + }); + + it("continues after a per-provider failure", async () => { + const { api, setupProvider } = makeApi(); + setupProvider.mockRejectedValueOnce(new Error("keychain unavailable")); + await expect( + executeByokMigration( + api, + settingsWith( + [ + model({ name: "claude", provider: ChatModelProviders.ANTHROPIC }), + model({ name: "or-x", provider: ChatModelProviders.OPENROUTERAI }), + ], + { anthropicApiKey: "k", openRouterAiApiKey: "k" } + ) + ) + ).resolves.toBeUndefined(); + expect(setupProvider).toHaveBeenCalledTimes(2); + }); +}); diff --git a/src/settings/migrations/byokMigration.ts b/src/settings/migrations/byokMigration.ts new file mode 100644 index 00000000..ecdb35df --- /dev/null +++ b/src/settings/migrations/byokMigration.ts @@ -0,0 +1,335 @@ +/** + * One-time migration: legacy BYOK models + provider keys → the new + * provider / configured-model / backend data model, so a user's own keys keep + * working in OpenCode (and Simple Chat) when agent mode lands. + * + * Two halves so the mapping logic is trivially unit-testable: + * - `planByokMigration` is PURE — legacy settings in, `SetupProviderInput[]` + * out, no side effects (reads keys from the already-hydrated in-memory + * settings, never disk). + * - `executeByokMigration` is the thin side-effecting wrapper that dedups + * against existing BYOK providers and feeds each descriptor through the + * battle-tested `ByokSetupApi.setupProvider` (provider row → keychain → + * configured models → backend enrollment, with its own rollback). + * + * Scope (locked with the product owner): + * - Credential-driven: every legacy provider with a user-supplied key (or, for + * local/openai-format providers, an explicit base URL) and its ENABLED + * models — built-in and custom. + * - Azure / Bedrock migrate to Simple Chat only (OpenCode can't route them). + * - Skip embeddings, disabled models, and copilot-plus / github-copilot + * (owned by Plus sign-in and agent setup). + * - Non-destructive: legacy keys and `activeModels` are left untouched. + */ + +import type { CustomModel } from "@/aiParams"; +import { + ChatModelProviders, + ProviderInfo, + type ProviderMetadata, + ProviderSettingsKeyMap, + type SettingKeyProviders, +} from "@/constants"; +import { logError, logInfo } from "@/logger"; +// Type-only imports: nothing here pulls the model-management barrel at runtime, +// keeping this settings-layer module (and its unit tests) light. +import type { + BackendType, + ModelInfo, + ModelManagementApi, + Provider, + ProviderType, + SetupProviderInput, +} from "@/modelManagement"; +import type { CopilotSettings } from "@/settings/model"; + +// Token-bounded "embed"/"embedding" id match. Mirrors `looksLikeEmbeddingModel` +// in `@/modelManagement/catalog/catalogTransform`; kept local so this module +// stays type-only against the model-management barrel. +const EMBEDDING_ID = /(^|[-_/.\s])embed(ding)?($|[-_/.\s])/i; + +interface LegacyProviderMapping { + providerType: ProviderType; + /** models.dev / OpenCode provider id; absent for catalog-less providers. */ + catalogProviderId?: string; + /** OpenCode can route this provider → enroll in `opencode` too. */ + opencodeRoutable: boolean; + /** + * Keyless providers (Ollama / LM Studio / generic OpenAI-format) that are + * only migrated when the model carries an explicit `baseUrl` — i.e. the user + * actually pointed them somewhere, not a bare unconfigured default. + */ + requiresBaseUrl?: boolean; +} + +/** + * Legacy `CustomModel.provider` → new-format mapping. Providers absent here + * (copilot-plus, github-copilot, anything unrecognized) are skipped. The + * top-level API-key field is derived from `ProviderSettingsKeyMap`, not + * duplicated here. + */ +const LEGACY_PROVIDER_MAP: Partial<Record<string, LegacyProviderMapping>> = { + [ChatModelProviders.ANTHROPIC]: { + providerType: "anthropic", + catalogProviderId: "anthropic", + opencodeRoutable: true, + }, + [ChatModelProviders.OPENAI]: { + providerType: "openai-compatible", + catalogProviderId: "openai", + opencodeRoutable: true, + }, + [ChatModelProviders.GOOGLE]: { + providerType: "google", + catalogProviderId: "google", + opencodeRoutable: true, + }, + [ChatModelProviders.OPENROUTERAI]: { + providerType: "openai-compatible", + catalogProviderId: "openrouter", + opencodeRoutable: true, + }, + [ChatModelProviders.XAI]: { + providerType: "openai-compatible", + catalogProviderId: "xai", + opencodeRoutable: true, + }, + [ChatModelProviders.GROQ]: { + providerType: "openai-compatible", + catalogProviderId: "groq", + opencodeRoutable: true, + }, + [ChatModelProviders.MISTRAL]: { + providerType: "openai-compatible", + catalogProviderId: "mistral", + opencodeRoutable: true, + }, + [ChatModelProviders.DEEPSEEK]: { + providerType: "openai-compatible", + catalogProviderId: "deepseek", + opencodeRoutable: true, + }, + // Catalog-less but OpenAI-compatible: routable via their base URL. + [ChatModelProviders.SILICONFLOW]: { providerType: "openai-compatible", opencodeRoutable: true }, + [ChatModelProviders.COHEREAI]: { providerType: "openai-compatible", opencodeRoutable: true }, + [ChatModelProviders.OLLAMA]: { + providerType: "openai-compatible", + opencodeRoutable: true, + requiresBaseUrl: true, + }, + [ChatModelProviders.LM_STUDIO]: { + providerType: "openai-compatible", + opencodeRoutable: true, + requiresBaseUrl: true, + }, + [ChatModelProviders.OPENAI_FORMAT]: { + providerType: "openai-compatible", + opencodeRoutable: true, + requiresBaseUrl: true, + }, + // Not OpenCode-routable → Simple Chat only. + [ChatModelProviders.AZURE_OPENAI]: { providerType: "azure", opencodeRoutable: false }, + [ChatModelProviders.AMAZON_BEDROCK]: { providerType: "bedrock", opencodeRoutable: false }, +}; + +// Frozen enrollment targets — referential stability (see AGENTS.md). +const ENROLL_CHAT_AND_OPENCODE: readonly BackendType[] = Object.freeze(["chat", "opencode"]); +const ENROLL_CHAT_ONLY: readonly BackendType[] = Object.freeze(["chat"]); + +/** Trim, drop a trailing slash, lowercase — for grouping / dedup comparison. */ +function normalizeUrl(url: string | undefined): string { + return (url ?? "").trim().replace(/\/+$/, "").toLowerCase(); +} + +/** Runtime-safe lookup: `ProviderInfo`'s typed keys are the provider enum, but + * `model.provider` is a raw string, so widen the index to allow `undefined`. */ +function providerMetaFor(provider: string): ProviderMetadata | undefined { + return (ProviderInfo as unknown as Record<string, ProviderMetadata | undefined>)[provider]; +} + +/** Catalog default base URL for a provider, or `undefined` for placeholder + * URLs (azure `<resource>`, bedrock `{region}`) that aren't real endpoints. */ +function defaultBaseUrlFor(provider: string): string | undefined { + const url = providerMetaFor(provider)?.curlBaseURL; + if (!url || url.includes("<") || url.includes("{")) return undefined; + return url; +} + +function displayNameFor(provider: string): string { + return providerMetaFor(provider)?.label ?? provider; +} + +/** Per-`providerType` opaque payload the adapters can't function without. */ +function buildExtras( + model: CustomModel, + settings: CopilotSettings, + providerType: ProviderType +): Record<string, unknown> | undefined { + if (providerType === "azure") { + const extras: Record<string, unknown> = {}; + const instance = model.azureOpenAIApiInstanceName || settings.azureOpenAIApiInstanceName; + const deployment = model.azureOpenAIApiDeploymentName || settings.azureOpenAIApiDeploymentName; + const apiVersion = model.azureOpenAIApiVersion || settings.azureOpenAIApiVersion; + if (instance) extras.azureInstanceName = instance; + if (deployment) extras.azureDeploymentName = deployment; + if (apiVersion) extras.azureApiVersion = apiVersion; + return Object.keys(extras).length > 0 ? extras : undefined; + } + if (providerType === "bedrock") { + const region = model.bedrockRegion || settings.amazonBedrockRegion; + return region ? { bedrockRegion: region } : undefined; + } + if (model.provider === (ChatModelProviders.OPENAI as string)) { + const orgId = model.openAIOrgId || settings.openAIOrgId; + return orgId ? { openAIOrgId: orgId } : undefined; + } + return undefined; +} + +interface ResolvedCandidate { + mapping: LegacyProviderMapping; + apiKey?: string; + baseUrl?: string; + extras?: Record<string, unknown>; +} + +/** + * Decide whether a single legacy model migrates, and resolve its credential / + * base URL / extras. Returns `null` for anything out of scope. + */ +function resolveCandidate(model: CustomModel, settings: CopilotSettings): ResolvedCandidate | null { + const mapping = LEGACY_PROVIDER_MAP[model.provider]; + if (!mapping) return null; // unknown / copilot-plus / github-copilot + if (!model.enabled) return null; // disabled models skipped per scope + if (model.isEmbeddingModel ?? EMBEDDING_ID.test(model.name)) return null; // embeddings skipped + + const keyField = ProviderSettingsKeyMap[model.provider as SettingKeyProviders]; + const rawKey = keyField ? settings[keyField] : undefined; + const topLevelKey = typeof rawKey === "string" ? rawKey.trim() : ""; + const apiKey = model.apiKey?.trim() || topLevelKey || undefined; + + let baseUrl: string | undefined; + if (mapping.requiresBaseUrl) { + // Local / generic OpenAI-format: only migrate an explicitly-pointed endpoint. + baseUrl = model.baseUrl?.trim() || undefined; + if (!baseUrl) return null; + } else { + baseUrl = model.baseUrl?.trim() || defaultBaseUrlFor(model.provider); + if (!apiKey) return null; // key-based providers need a usable key + } + + return { mapping, apiKey, baseUrl, extras: buildExtras(model, settings, mapping.providerType) }; +} + +function toModelInfo(model: CustomModel): ModelInfo { + return { id: model.name, displayName: model.displayName?.trim() || model.name }; +} + +/** + * Pure: legacy settings → BYOK provider-setup descriptors. Models are grouped + * into one provider per `(providerType, catalogProviderId, baseUrl, apiKey)` so + * distinct credentials become distinct provider instances; model ids are + * de-duped within a group (last wins) to satisfy `bulkSet`. + */ +export function planByokMigration(settings: CopilotSettings): SetupProviderInput[] { + const groups = new Map< + string, + { input: SetupProviderInput; modelsById: Map<string, ModelInfo> } + >(); + + for (const model of settings.activeModels ?? []) { + const candidate = resolveCandidate(model, settings); + if (!candidate) continue; + const { mapping, apiKey, baseUrl, extras } = candidate; + + const groupKey = [ + mapping.providerType, + mapping.catalogProviderId ?? "", + normalizeUrl(baseUrl), + apiKey ?? "", + ].join("�"); + + let group = groups.get(groupKey); + if (!group) { + const input: SetupProviderInput = { + providerType: mapping.providerType, + displayName: displayNameFor(model.provider), + models: [], + autoEnrollIn: mapping.opencodeRoutable ? ENROLL_CHAT_AND_OPENCODE : ENROLL_CHAT_ONLY, + }; + if (mapping.catalogProviderId) input.catalogProviderId = mapping.catalogProviderId; + if (baseUrl) input.baseUrl = baseUrl; + if (apiKey) input.apiKey = apiKey; + if (extras) input.extras = extras; + group = { input, modelsById: new Map() }; + groups.set(groupKey, group); + } + + const info = toModelInfo(model); + group.modelsById.set(info.id, info); + } + + return [...groups.values()].map(({ input, modelsById }) => ({ + ...input, + models: [...modelsById.values()], + })); +} + +/** A pre-existing BYOK provider equivalent to a planned descriptor (same + * identity: type + catalog id + base URL). Key is intentionally NOT part of + * the match — a keyless existing row still counts as "already present". */ +function isDuplicateByok(provider: Provider, descriptor: SetupProviderInput): boolean { + if (provider.origin.kind !== "byok") return false; + return ( + provider.providerType === descriptor.providerType && + (provider.origin.catalogProviderId ?? "") === (descriptor.catalogProviderId ?? "") && + normalizeUrl(provider.baseUrl) === normalizeUrl(descriptor.baseUrl) + ); +} + +/** + * Side-effecting executor. Builds a one-shot plan from `settings`, skips + * descriptors that match an existing BYOK provider, and creates the rest via + * `ByokSetupApi.setupProvider`. Never throws: a single provider failure is + * logged and the rest proceed (the version bump in the caller is unconditional). + */ +export async function executeByokMigration( + api: ModelManagementApi, + settings: CopilotSettings +): Promise<void> { + const descriptors = planByokMigration(settings); + if (descriptors.length === 0) { + logInfo("[byok-migration] no legacy BYOK providers to migrate"); + return; + } + + // Snapshot existing BYOK providers once; the planner already de-dups within + // this run, so a stale snapshot only matters for pre-existing rows. + const existing = api.providerRegistry.listByOrigin("byok"); + let created = 0; + let skipped = 0; + let failed = 0; + + for (const descriptor of descriptors) { + if (existing.some((provider) => isDuplicateByok(provider, descriptor))) { + skipped++; + logInfo(`[byok-migration] skipping already-present provider "${descriptor.displayName}"`); + continue; + } + try { + const result = await api.setup.byok.setupProvider(descriptor); + created++; + logInfo( + `[byok-migration] migrated "${descriptor.displayName}" ` + + `(${result.configuredModelIds.length} models, enroll=${descriptor.autoEnrollIn?.join("+")})` + ); + } catch (err) { + failed++; + logError(`[byok-migration] failed to migrate "${descriptor.displayName}"; continuing`, err); + } + } + + logInfo( + `[byok-migration] done: ${created} migrated, ${skipped} already present, ${failed} failed` + ); +} diff --git a/src/settings/migrations/index.ts b/src/settings/migrations/index.ts new file mode 100644 index 00000000..9cb9ea64 --- /dev/null +++ b/src/settings/migrations/index.ts @@ -0,0 +1,34 @@ +/** + * One-time settings migrations, run once on plugin load (from `Plugin.onload`, + * after the settings-persistence subscriber is wired so every mutation is + * persisted, and before agent/model-discovery init so OpenCode first sees the + * migrated backends). + * + * The gate is deliberately a plain version comparison — there is a single + * one-time migration today (legacy BYOK → model-management). Promote this to an + * ordered runner only if a second migration ever accrues. + */ + +import { logInfo } from "@/logger"; +import type { ModelManagementApi } from "@/modelManagement"; +import { getSettings, setSettings } from "@/settings/model"; + +import { executeByokMigration } from "./byokMigration"; +import { CURRENT_SETTINGS_VERSION } from "./version"; + +export { CURRENT_SETTINGS_VERSION } from "./version"; + +/** + * Run pending one-time migrations and stamp the new version. No-op when + * settings are already at/above the target (migrated vaults, fresh installs). + */ +export async function runSettingsMigrations(api: ModelManagementApi): Promise<void> { + const fromVersion = getSettings().settingsVersion ?? 0; + if (fromVersion >= CURRENT_SETTINGS_VERSION) return; + + logInfo(`[settings-migration] migrating from v${fromVersion} to v${CURRENT_SETTINGS_VERSION}`); + await executeByokMigration(api, getSettings()); + // Bump unconditionally after the migration so a per-provider failure can't + // wedge the gate into re-running on every load. Persists via the subscriber. + setSettings({ settingsVersion: CURRENT_SETTINGS_VERSION }); +} diff --git a/src/settings/migrations/runSettingsMigrations.test.ts b/src/settings/migrations/runSettingsMigrations.test.ts new file mode 100644 index 00000000..85132a5a --- /dev/null +++ b/src/settings/migrations/runSettingsMigrations.test.ts @@ -0,0 +1,106 @@ +/** + * Version-gate tests for `runSettingsMigrations`. `@/settings/model` is mocked + * (getSettings / setSettings) and a fake `ModelManagementApi` stands in, so the + * gate logic is exercised in isolation from the real store and registries. + */ + +import type { CustomModel } from "@/aiParams"; +import { ChatModelProviders, DEFAULT_SETTINGS } from "@/constants"; +import type { ModelManagementApi } from "@/modelManagement"; +import { getSettings, setSettings, type CopilotSettings } from "@/settings/model"; + +import { CURRENT_SETTINGS_VERSION, runSettingsMigrations } from "./index"; + +jest.mock("@/logger", () => ({ + logInfo: jest.fn(), + logWarn: jest.fn(), + logError: jest.fn(), +})); + +jest.mock("@/settings/model", () => { + const actual = jest.requireActual<typeof import("@/settings/model")>("@/settings/model"); + return { ...actual, getSettings: jest.fn(), setSettings: jest.fn() }; +}); + +const mockGetSettings = getSettings as jest.MockedFunction<typeof getSettings>; +const mockSetSettings = setSettings as jest.MockedFunction<typeof setSettings>; + +function settings( + overrides: Partial<CopilotSettings>, + models: CustomModel[] = [] +): CopilotSettings { + return { ...DEFAULT_SETTINGS, activeModels: models, ...overrides }; +} + +function makeApi() { + const setupProvider = jest.fn(async () => ({ providerId: "p1", configuredModelIds: ["cm1"] })); + const api = { + providerRegistry: { listByOrigin: jest.fn(() => []) }, + setup: { byok: { setupProvider } }, + } as unknown as ModelManagementApi; + return { api, setupProvider }; +} + +const keyedAnthropic = () => + settings({ settingsVersion: undefined, anthropicApiKey: "sk-ant" }, [ + { + name: "claude-sonnet-4-5", + provider: ChatModelProviders.ANTHROPIC, + enabled: true, + isBuiltIn: false, + }, + ]); + +beforeEach(() => { + jest.clearAllMocks(); +}); + +it("runs when settingsVersion is undefined (pre-versioned install)", async () => { + mockGetSettings.mockReturnValue(keyedAnthropic()); + const { api, setupProvider } = makeApi(); + + await runSettingsMigrations(api); + + expect(setupProvider).toHaveBeenCalledTimes(1); + expect(mockSetSettings).toHaveBeenCalledWith({ settingsVersion: CURRENT_SETTINGS_VERSION }); +}); + +it("runs when settingsVersion is the orphaned prototype value 2", async () => { + mockGetSettings.mockReturnValue({ ...keyedAnthropic(), settingsVersion: 2 }); + const { api, setupProvider } = makeApi(); + + await runSettingsMigrations(api); + + expect(setupProvider).toHaveBeenCalledTimes(1); + expect(mockSetSettings).toHaveBeenCalledWith({ settingsVersion: CURRENT_SETTINGS_VERSION }); +}); + +it("bumps the version even when there is nothing to migrate", async () => { + mockGetSettings.mockReturnValue(settings({ settingsVersion: undefined }, [])); + const { api, setupProvider } = makeApi(); + + await runSettingsMigrations(api); + + expect(setupProvider).not.toHaveBeenCalled(); + expect(mockSetSettings).toHaveBeenCalledWith({ settingsVersion: CURRENT_SETTINGS_VERSION }); +}); + +it("skips when already at the current version", async () => { + mockGetSettings.mockReturnValue(settings({ settingsVersion: CURRENT_SETTINGS_VERSION })); + const { api, setupProvider } = makeApi(); + + await runSettingsMigrations(api); + + expect(setupProvider).not.toHaveBeenCalled(); + expect(mockSetSettings).not.toHaveBeenCalled(); +}); + +it("skips a future version", async () => { + mockGetSettings.mockReturnValue(settings({ settingsVersion: CURRENT_SETTINGS_VERSION + 1 })); + const { api, setupProvider } = makeApi(); + + await runSettingsMigrations(api); + + expect(setupProvider).not.toHaveBeenCalled(); + expect(mockSetSettings).not.toHaveBeenCalled(); +}); diff --git a/src/settings/migrations/version.ts b/src/settings/migrations/version.ts new file mode 100644 index 00000000..6be1c38f --- /dev/null +++ b/src/settings/migrations/version.ts @@ -0,0 +1,12 @@ +/** + * Settings schema version. Bumped by the one-time migrations in this folder. + * + * Kept in its own leaf module (no heavy imports) so low-level code like the + * settings-persistence layer can stamp fresh installs without pulling in the + * model-management barrel that `runSettingsMigrations` depends on. + * + * `= 4` matches the v4 launch. Gate is `(settingsVersion ?? 0) < CURRENT`, so + * pre-versioned installs (real users → `0`) and the orphaned prototype `2` + * both run; migrated vaults (`4`) and freshly-stamped installs skip. + */ +export const CURRENT_SETTINGS_VERSION = 4; diff --git a/src/settings/model.ts b/src/settings/model.ts index 0a23560d..eefcd1c8 100644 --- a/src/settings/model.ts +++ b/src/settings/model.ts @@ -221,6 +221,13 @@ export interface CopilotSettings { * renaming or moving the vault folder does not orphan keychain entries. */ _keychainVaultId?: string; + /** + * Schema version, bumped by one-time migrations in `src/settings/migrations`. + * Absent on pre-versioned installs (treated as `0`). Deliberately NOT in + * `DEFAULT_SETTINGS` — a default value would defeat the migration gate via + * the `setSettings` merge. See `runSettingsMigrations`. + */ + settingsVersion?: number; /** Agent Mode (ACP-backed BYOK agent harness). Desktop only. */ agentMode: { enabled: boolean; diff --git a/src/settings/v2/components/AgentSettings.tsx b/src/settings/v2/components/AgentSettings.tsx index 6c0ce363..f451a916 100644 --- a/src/settings/v2/components/AgentSettings.tsx +++ b/src/settings/v2/components/AgentSettings.tsx @@ -1,9 +1,11 @@ import { + InstallBadge, listBackendDescriptors, McpServersPanel, type BackendDescriptor, type BackendId, } from "@/agentMode"; +import { Button } from "@/components/ui/button"; import { SettingItem } from "@/components/ui/setting-item"; import { usePlugin } from "@/contexts/PluginContext"; import { logError } from "@/logger"; @@ -108,13 +110,28 @@ const BackendSection: React.FC<{ .catch((e) => logError(`[AgentMode] preload ${descriptor.id} failed`, e)); }, [manager, descriptor.id, installState.kind]); + const Icon = descriptor.Icon; + return ( <div className="tw-space-y-3 tw-rounded-md tw-border tw-border-solid tw-border-border tw-p-3"> - <div className="tw-text-base tw-font-semibold">{descriptor.displayName}</div> - - {Panel && <Panel plugin={plugin} app={plugin.app} />} + <div className="tw-flex tw-items-center tw-justify-between tw-gap-2"> + <div className="tw-flex tw-items-center tw-gap-2"> + <Icon className="tw-size-4" /> + <span className="tw-text-base tw-font-semibold">{descriptor.displayName}</span> + <InstallBadge state={installState} /> + </div> + <Button + size="default" + variant={installState.kind === "ready" ? "secondary" : "default"} + onClick={() => descriptor.openInstallUI(plugin)} + > + Configure + </Button> + </div> {installState.kind === "ready" && <ConfiguredModelEnableList descriptor={descriptor} />} + + {Panel && <Panel plugin={plugin} app={plugin.app} />} </div> ); };