Conversation
PATCH /config hot-reload w/ selective cache invalidation
* Fix cache corruption bug in refresh function
* adding favorite model picker
Resolved conflicts: - dialog-model.tsx: Merged favorites system with provider connection dialog - state.ts: Combined cleanup approaches (entries.clear + recordsByKey.delete) - provider.ts: Kept State import, removed unused Global import
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRefactors many modules to use a named State.register lifecycle, adds a robust config persistence layer with locking, backups, atomic JSONC writes and diffs, implements config hot-reload and invalidation orchestration, introduces model favorites in the CLI, and expands keybinds and tests to cover the new flows. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as TUI
participant Dialog as Model Dialog
participant Local as Local Store
participant Disk as Disk
participant Config as Config Service
participant State as State Registry
participant Invalidate as ConfigInvalidation
User->>CLI: Open model picker / use favorite key
CLI->>Dialog: request options
Dialog->>Local: get favorites & recents
Local-->>Dialog: enriched options
User->>Dialog: toggle favorite (Ctrl+F)
Dialog->>Local: toggleFavorite(model)
Local->>Disk: save() -> persist favorites
Disk-->>Local: persisted
User->>Config: PATCH /config (update)
Config->>Disk: acquireLock(filepath)
Config->>Disk: createBackup && read existing
Config->>Config: merge & validate
Config->>Disk: writeConfigFile (atomic JSONC)
Config->>Disk: releaseLock && remove backup
Config->>State: State.invalidate(matched names)
State->>Invalidate: emits Config.Updated
Invalidate->>State: apply(diff) -> trigger per-subsystem refresh
State-->>User: return { before, after, diff, filepath }
sequenceDiagram
participant Bootstrap
participant State as State Registry
participant Plugin as Plugin State
participant Provider as Provider State
participant Invalidate as ConfigInvalidation
Bootstrap->>Invalidate: ConfigInvalidation.setup()
Bootstrap->>State: register("plugin", root, init, dispose)
Bootstrap->>State: register("provider", root, init, dispose)
Note right of State: registrations tracked in namedRegistry
User->>Config: update(...)
Config->>State: invalidate("provider:*")
State->>Provider: dispose + re-init on next access
State->>Invalidate: notify Config.Updated -> apply()
Invalidate->>Provider: request provider refresh
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/opencode/src/project/state.ts (1)
9-137: Tighten invalidate semantics: doc mismatch and potentialnamedRegistryleakageTwo small issues worth addressing:
- Wildcard comment vs implementation
The docstring says the wildcard prefix is “before the
:*” (e.g."cache:*"→"cache"), but the implementation usesname.slice(0, -1), which yields"cache:". That’s still a usable prefix for names like"cache:foo", but the comment is misleading.If you want the documented behavior, change the slice to remove both characters:
- export async function invalidate(name: string, key?: string) { - const pattern = name.endsWith(":*") ? name.slice(0, -1) : null + export async function invalidate(name: string, key?: string) { + const pattern = name.endsWith(":*") ? name.slice(0, -2) : nullIf the current
"cache:"-style prefix is intentional, I’d instead update the comment to say so.
namedRegistryentries never removedNeither
invalidatenordisposeprunesnamedRegistry, so over time you can accumulate(name, key, init)tuples for directories and states that no longer exist. Functionally it’s harmless (subsequent invalidations just find norecordsByKeyentry), but in a long‑lived process hopping across many directories this will leak some memory.You can clean this up with minimal changes, for example:
- In
invalidate, track and remove theNamedEntryobjects you actually invalidated, and delete the name entirely when empty:- const tasks: Promise<void>[] = [] - for (const entry of entries) { + const tasks: Promise<void>[] = [] + const toRemove: NamedEntry[] = [] + for (const entry of entries) { if (key && entry.key !== key) continue @@ if (stateEntry.dispose) { @@ tasks.push(task) } - - keyRecords.delete(entry.init) + keyRecords.delete(entry.init) + toRemove.push(entry) + if (keyRecords.size === 0) { + recordsByKey.delete(entry.key) + } } + + for (const entry of toRemove) { + entries.delete(entry) + } + if (entries.size === 0) { + namedRegistry.delete(name) + }
- In
dispose(key), also strip out anyNamedEntryfor that key:- entries.clear() - recordsByKey.delete(key) + entries.clear() + recordsByKey.delete(key) + + for (const [name, namedEntries] of namedRegistry) { + for (const entry of Array.from(namedEntries)) { + if (entry.key === key) { + namedEntries.delete(entry) + } + } + if (namedEntries.size === 0) { + namedRegistry.delete(name) + } + }These changes keep behavior the same while preventing stale registry entries from building up.
Also applies to: 168-168
🧹 Nitpick comments (15)
packages/opencode/src/session/prompt.ts (1)
903-928: Read tool multi-part handling looks solid; a couple of minor robustness nitsThe new flow (always emitting a synthetic text part from
result.outputand then either mapped attachments or the originalpart) matches the described behavior and should integrate well with existing message handling. A couple of small, optional improvements:
- When mapping
result.attachments, consider ensuring each attachment has anidto avoid any downstream assumptions about presence/uniqueness ofid:...result.attachments.map((attachment) => ({ ...attachment, id: attachment.id ?? Identifier.ascending("part"), synthetic: true, filename: attachment.filename ?? part.filename, messageID: info.id, sessionID: input.sessionID, }))- If there are cases where
result.outputcan be an empty string while attachments carry the real payload, you may want to skip pushing the synthetic text part when!result.outputto avoid extra empty bubbles.These are polish-level suggestions; the core behavior change looks correct.
packages/opencode/src/file/index.ts (2)
144-153: Tighten ENOENT guard for TypeScript type-safetyThe ENOENT handling is fine logically, but the current narrowing:
.catch((error) => { if (error instanceof Error && "code" in error && error.code === "ENOENT") return log.error("failed to refresh files", { error }) })may be brittle under
useUnknownInCatchVariablesand strict mode, sinceErrordoesn’t declarecode. A small type guard avoids potential TS errors:- fetchEntries() - .catch((error) => { - if (error instanceof Error && "code" in error && error.code === "ENOENT") return - log.error("failed to refresh files", { error }) - }) + fetchEntries() + .catch((error) => { + const code = typeof error === "object" && error !== null && "code" in error ? (error as any).code : undefined + if (code === "ENOENT") return + log.error("failed to refresh files", { error }) + })This keeps runtime behavior the same while staying friendly to stricter TS configs.
155-162: Firstfiles()call likely returns empty cache; confirm non-blocking semantics are intendedWith the current flow:
refresh()is fired once in the initializer (Line 155) but not awaited.fetchEntries()begins the ripgrep scan asynchronously and eventually setscache = temp.files()(Lines 158-162) triggers anotherrefresh()only when!fetching, but always returns the currentcacheimmediately without waiting.This means the very first
files()call after state creation will almost certainly return the initial{ files: [], dirs: [] }snapshot while the first scan is still running, and subsequent calls will always see the previous snapshot (never block for the latest scan).If that “eventually consistent, never-blocking” behavior is intentional, this is fine; if callers expect the first
search()to see a warmed list, you may want a one-time awaited warm-up, e.g.:- let fetching = false + let fetching = false + let initialized = false @@ - const refresh = () => { - fetching = true - fetchEntries() + const refresh = async () => { + fetching = true + await fetchEntries() .catch((error) => { // ENOENT handling as above }) .finally(() => { fetching = false }) } - refresh() + // prime the cache once + const initPromise = refresh().finally(() => { + initialized = true + }) @@ - async files() { - if (!fetching) { - refresh() - } - return cache - }, + async files() { + if (!initialized) { + await initPromise + } else if (!fetching) { + void refresh() + } + return cache + },This preserves non-blocking refreshes after the first call but guarantees the initial
files()result is based on a completed scan. Please confirm which behavior you want.packages/opencode/test/config/write.test.ts (1)
9-75: JSONC write tests look solid; consider a couple of extra assertionsThese tests nicely cover comment preservation and JSONC validity, including trailing commas. To tighten coverage further, you might also assert that the top‑level
modeland nestedagent.build/planmodels are updated as expected in the incremental case (not justtheme), to guard against partial-update regressions.packages/opencode/src/server/server.ts (2)
83-101: LastConfigUpdate cache and timer are fine, but consider lifecycle/cleanupThe per‑directory
LastConfigUpdatecache with a 60s TTL and a 60ssetIntervalsweeper works for a long‑running server, but the interval is never cleared. In embedded/test scenarios that importServerwithout actuallylisten‑ing, this can keep the process alive longer than intended and makes the timer effectively global.If that’s a concern for you, consider either:
- Clearing the interval as part of a server teardown, or
- Dropping the periodic timer and doing opportunistic cleanup on reads (since you already check freshness with
isFresh).Functionally this is fine; this is more about process lifecycle hygiene.
1938-1967: Toast enrichment logic works; cross‑directory fallback may surprise slightlyThe enrichment of “Saved (global|project) config …” toasts based on
LastConfigUpdateis robust (directory‑scoped first, 10s freshness window, safe try/catch, and only altering messages that match the pattern). That should get rid of the-> undefinedUX without risking hard failures.One behavioral detail: when there’s no fresh entry for the current directory, you fall back to the freshest entry for the same scope across all directories. That means a toast in one project can occasionally show sections from another project if they save around the same time. If you’d prefer stricter correctness over “best available” enrichment, you could skip the cross‑directory fallback and only enrich when there’s a fresh entry for the current directory.
packages/opencode/src/plugin/index.ts (1)
15-67: Plugin state registration is good; watch for duplicate/default plugins and export shapeThe State‑backed lifecycle for plugins (init + cleanup) looks solid and the teardown path with per‑hook
cleanup()is a nice improvement.Two minor things to consider here:
Duplicate default plugins
If a user explicitly lists"opencode-copilot-auth@0.0.5"or"opencode-anthropic-auth@0.0.2"inconfig.plugin, the current logic will load that plugin twice (once from config, once from the default pushes). A simple dedupe avoids surprising double registration, e.g.:
const plugins = [...(config.plugin ?? [])]if (!Flag.OPENCODE_DISABLE_DEFAULT_PLUGINS) {plugins.push("opencode-copilot-auth@0.0.5")plugins.push("opencode-anthropic-auth@0.0.2")}for (let plugin of plugins) {
const plugins = new Set(config.plugin ?? [])if (!Flag.OPENCODE_DISABLE_DEFAULT_PLUGINS) {plugins.add("opencode-copilot-auth@0.0.5")plugins.add("opencode-anthropic-auth@0.0.2")}for (let plugin of plugins) {
- Assuming every export is a plugin function
Object.entries(mod)treats every export as aPluginInstanceand calls it. If a plugin module ever exposes additional values (e.g. constants or helpers), this will throw at runtime. If you anticipate that, you could defensively filter exports bytypeof fn === "function"before calling.Neither is a blocker, but tightening these would make plugin loading more robust.
packages/opencode/src/config/lock.ts (1)
1-133: Filesystem locking looks solid; consider two small robustness tweaksThe combination of an in‑memory mutex (
fileLocks) and anO_CREAT|O_EXCLlockfile is clean and should work well. A couple of things to consider:
- The stale-lock removal is purely
mtimeMs-based with a hard default of 60s. If any config operation can legitimately hold the lock for longer (e.g., slow I/O on network filesystems), this will aggressively delete a still-valid lock. You already exposestaleAfter; it may be worth:
- Bumping the default for safety, and/or
- Using the stored
{ pid, createdAt }to optionally check whether the process is still alive before forcibly removing the lock (even a best-effortprocess.kill(pid, 0)check when on the same host).acquireLocknormalizes but does not resolve the path. If callers ever pass a mix of relative and absolute paths for the same underlying file, you could end up with two independent in‑process locks. If that’s a realistic risk, switching topath.resolve(filepath)instead ofnormalizewould harden this.None of these are correctness blockers, but tightening them would make the lock more resilient in edge cases.
packages/opencode/test/config/config.test.ts (1)
465-505: Cross‑platform nit: build expected file URL viapathToFileURLinstead of string concatenationThe
"appends plugins discovered from directories after merging config files"test expectation currently builds the plugin URL as:expect(config.plugin).toEqual([ "local-plugin", `file://${path.join(globalTmp.path, "plugin", "custom.ts")}`, ])On POSIX this happens to match
file:///tmp/..., but on Windows this would yield something likefile://C:\path\plugin\custom.ts, which does not match a canonical file URL and may differ from whatever the implementation produces.For portability and consistency with the earlier test, consider:
- expect(config.plugin).toEqual(["local-plugin", `file://${path.join(globalTmp.path, "plugin", "custom.ts")}`]) + const globalPluginUrl = pathToFileURL( + path.join(globalTmp.path, "plugin", "custom.ts"), + ).href + expect(config.plugin).toEqual(["local-plugin", globalPluginUrl])This keeps the intent while avoiding OS-specific path quirks.
packages/opencode/test/config/hot-reload.test.ts (2)
93-148: Test names vs asserted behavior (full dispose vs hot-reload)The first three tests exercise config update semantics correctly, but the “without full dispose” / “uses full dispose” behavior isn’t actually asserted (e.g., by checking instance lifecycle or cache reuse vs re-init). If distinguishing full dispose vs hot-reload behavior is important, consider adding explicit assertions (e.g., on invalidation calls or instance state) or renaming tests to reflect what’s concretely verified.
Also applies to: 150-172
174-201: Strengthen assertions around local overrides vs global updatesThe “global updates propagate despite local overrides” and “project updates remain scoped to the initiator” tests correctly verify observer/global behavior, but they don’t assert that the writer’s local project config remains effective in the presence of a global model. Adding explicit checks that the writer still sees the project model (and not the global one) would better lock in the intended precedence rules.
Also applies to: 302-336
packages/opencode/src/cli/cmd/tui/context/local.tsx (2)
102-125: Persistence shape change formodel.json—consider backward compatibilityThe switch to persisting
{ recent, favorite }viasave()and then reading viax.recent/x.favoriteassumesmodel.jsonis an object with those properties. If earlier versions ever wrote a bare array or a different shape, existing users could silently lose their recent history on upgrade. It may be worth tolerating both formats (e.g.,if (Array.isArray(x)) setModelStore("recent", x)as a migration path) to avoid discarding prior recents.Also applies to: 129-137, 141-148
254-272:toggleFavoriteandsetsemantics are reasonable, but persistence is best-effort
set()only persists whenoptions?.recentis truthy, andtoggleFavoritealways persists after updating the favorites list. GivenBun.writeis not awaited, persistence is intentionally best-effort and asynchronous—fine for a non-critical MRU/favorites file. If you ever need stronger guarantees (e.g., for debugging flaky state), consider awaitingsave()in these paths.Also applies to: 273-291
packages/opencode/src/provider/provider.ts (1)
392-399: Defensive check arounddatabase[providerID]in custom loader loopIn the custom loader block you assume every
CUSTOM_LOADERSkey has a corresponding entry indatabase:for (const [providerID, fn] of Object.entries(CUSTOM_LOADERS)) { if (disabled.has(providerID)) continue const result = await fn(database[providerID]) ... }If a loader is added for a provider that doesn’t exist in
database(or is removed from models.dev), loaders likeopencodethat dereferenceinput.env/input.modelswill throw.A small guard makes this more robust without changing behavior when the database is complete:
- for (const [providerID, fn] of Object.entries(CUSTOM_LOADERS)) { - if (disabled.has(providerID)) continue - const result = await fn(database[providerID]) + for (const [providerID, fn] of Object.entries(CUSTOM_LOADERS)) { + if (disabled.has(providerID)) continue + const providerInfo = database[providerID] + if (!providerInfo) continue + const result = await fn(providerInfo) if (result && (result.autoload || providers[providerID])) { mergeProvider(providerID, result.options ?? {}, "custom", result.getModel) } }packages/opencode/src/config/write.ts (1)
25-49: Incremental JSONC updates + atomic writes look solid; consider minor no-op optimizationThe overall flow in
writeConfigFile+applyIncrementalUpdatesis well-structured:
- Existing JSON/JSONC content is patched via
jsonc-parser.modifybased onConfigDiffand deep equality, then validated withvalidateJsonc.- Non-JSONC paths fall back to a full JSON rewrite.
writeFileAtomicallycorrectly uses a temp file in the same directory, fsyncs the file and then the directory (on non-Windows), and cleans up temp files on failure.A couple of non-blocking tweaks you might consider:
Avoid no-op writes: if
applyIncrementalUpdatesreturns a string identical toexistingContent, you could skipwriteFileAtomicallyto reduce churn on disk and avoid triggering unnecessary watchers.Clarify
.jsonhandling:isJsonccurrently treats both.jsoncand.jsonas JSONC for incremental edits. That’s perfectly valid (JSON ⊂ JSONC) but is a subtle behavior change; a short code comment explaining the intention (preserve formatting/comments for both) would make this clearer to future readers.Also applies to: 84-175
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/sdk/js/src/gen/types.gen.tsis excluded by!**/gen/**
📒 Files selected for processing (37)
packages/opencode/src/agent/agent.ts(2 hunks)packages/opencode/src/cli/cmd/tui/app.tsx(1 hunks)packages/opencode/src/cli/cmd/tui/component/dialog-model.tsx(4 hunks)packages/opencode/src/cli/cmd/tui/context/local.tsx(4 hunks)packages/opencode/src/command/index.ts(2 hunks)packages/opencode/src/config/backup.ts(1 hunks)packages/opencode/src/config/config.ts(10 hunks)packages/opencode/src/config/diff.ts(1 hunks)packages/opencode/src/config/error.ts(1 hunks)packages/opencode/src/config/global-file.ts(1 hunks)packages/opencode/src/config/hot-reload.ts(1 hunks)packages/opencode/src/config/invalidation.ts(1 hunks)packages/opencode/src/config/lock.ts(1 hunks)packages/opencode/src/config/persist.ts(1 hunks)packages/opencode/src/config/write.ts(1 hunks)packages/opencode/src/file/index.ts(2 hunks)packages/opencode/src/file/watcher.ts(2 hunks)packages/opencode/src/format/index.ts(2 hunks)packages/opencode/src/lsp/index.ts(2 hunks)packages/opencode/src/mcp/index.ts(2 hunks)packages/opencode/src/permission/index.ts(2 hunks)packages/opencode/src/plugin/index.ts(1 hunks)packages/opencode/src/project/bootstrap.ts(1 hunks)packages/opencode/src/project/instance.ts(1 hunks)packages/opencode/src/project/state.ts(3 hunks)packages/opencode/src/provider/provider.ts(2 hunks)packages/opencode/src/server/server.ts(4 hunks)packages/opencode/src/session/prompt.ts(1 hunks)packages/opencode/src/tool/registry.ts(2 hunks)packages/opencode/test/config/config.test.ts(4 hunks)packages/opencode/test/config/hot-reload.test.ts(1 hunks)packages/opencode/test/config/write.test.ts(1 hunks)packages/sdk/go/config.go(2 hunks)packages/sdk/python/src/opencode_ai/models/keybinds_config.py(6 hunks)packages/ui/src/components/select-dialog.tsx(2 hunks)packages/web/src/content/docs/keybinds.mdx(1 hunks)specs/config-spec.md(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (25)
packages/opencode/src/tool/registry.ts (2)
packages/opencode/src/project/instance.ts (2)
state(48-50)Instance(15-93)packages/opencode/src/config/config.ts (1)
state(197-197)
packages/opencode/src/config/invalidation.ts (3)
packages/opencode/src/config/diff.ts (1)
ConfigDiff(4-26)packages/opencode/src/project/instance.ts (1)
Instance(15-93)packages/opencode/src/config/hot-reload.ts (1)
isConfigHotReloadEnabled(1-3)
packages/opencode/test/config/write.test.ts (1)
packages/opencode/src/config/write.ts (1)
writeConfigFile(25-49)
packages/opencode/src/server/server.ts (3)
packages/opencode/src/project/instance.ts (2)
directory(39-41)Instance(15-93)packages/opencode/src/config/hot-reload.ts (1)
isConfigHotReloadEnabled(1-3)packages/opencode/src/cli/cmd/tui/event.ts (1)
TuiEvent(4-39)
packages/opencode/src/config/diff.ts (1)
packages/opencode/src/config/config.ts (2)
Info(529-714)Info(716-716)
packages/opencode/src/project/bootstrap.ts (1)
packages/opencode/src/project/instance.ts (1)
Instance(15-93)
packages/opencode/src/config/backup.ts (1)
packages/opencode/script/postinstall.mjs (1)
targetPath(73-73)
packages/opencode/src/permission/index.ts (3)
packages/opencode/src/project/instance.ts (2)
state(48-50)Instance(15-93)packages/opencode/src/config/config.ts (1)
state(197-197)packages/opencode/src/tool/registry.ts (1)
state(27-58)
packages/opencode/src/command/index.ts (3)
packages/opencode/src/project/instance.ts (2)
state(48-50)Instance(15-93)packages/opencode/src/config/config.ts (3)
state(197-197)Info(529-714)Info(716-716)packages/opencode/src/tool/registry.ts (1)
state(27-58)
packages/opencode/src/file/watcher.ts (4)
packages/opencode/src/project/instance.ts (2)
state(48-50)Instance(15-93)packages/opencode/src/config/config.ts (1)
state(197-197)packages/opencode/src/tool/registry.ts (1)
state(27-58)packages/opencode/src/file/time.ts (1)
state(6-15)
packages/opencode/src/lsp/index.ts (3)
packages/opencode/src/project/instance.ts (2)
state(48-50)Instance(15-93)packages/opencode/src/config/config.ts (1)
state(197-197)packages/opencode/src/tool/registry.ts (1)
state(27-58)
packages/opencode/src/project/instance.ts (2)
packages/opencode/src/cli/cmd/agent.ts (2)
fn(17-132)fn(143-155)packages/opencode/src/cli/cmd/github.ts (1)
fn(142-358)
packages/opencode/test/config/config.test.ts (2)
packages/opencode/test/fixture/fixture.ts (1)
tmpdir(11-28)packages/opencode/src/project/instance.ts (1)
Instance(15-93)
packages/opencode/src/plugin/index.ts (3)
packages/plugin/src/index.ts (2)
Plugin(27-27)PluginInput(19-25)packages/opencode/src/project/instance.ts (2)
state(48-50)Instance(15-93)packages/opencode/src/config/config.ts (1)
state(197-197)
packages/opencode/test/config/hot-reload.test.ts (2)
packages/opencode/src/project/instance.ts (2)
directory(39-41)Instance(15-93)packages/opencode/src/project/bootstrap.ts (1)
InstanceBootstrap(15-30)
packages/opencode/src/config/persist.ts (9)
packages/opencode/src/project/instance.ts (2)
directory(39-41)Instance(15-93)packages/opencode/src/config/global-file.ts (1)
resolveGlobalFile(5-8)packages/opencode/src/config/config.ts (3)
Info(529-714)Info(716-716)update(881-897)packages/opencode/src/config/diff.ts (2)
ConfigDiff(4-26)computeDiff(45-123)packages/opencode/src/config/lock.ts (1)
acquireLock(90-133)packages/opencode/src/config/backup.ts (2)
createBackup(3-12)restoreBackup(14-17)packages/opencode/src/config/write.ts (2)
writeConfigFile(25-49)writeFileAtomically(265-279)packages/opencode/src/config/hot-reload.ts (1)
isConfigHotReloadEnabled(1-3)packages/opencode/src/config/error.ts (3)
ConfigWriteError(38-45)ConfigValidationError(14-27)ConfigUpdateError(4-12)
packages/opencode/src/config/write.ts (2)
packages/opencode/src/config/diff.ts (1)
ConfigDiff(4-26)packages/opencode/src/config/config.ts (2)
Info(529-714)Info(716-716)
packages/opencode/src/mcp/index.ts (3)
packages/opencode/src/project/instance.ts (2)
state(48-50)Instance(15-93)packages/opencode/src/config/config.ts (1)
state(197-197)packages/opencode/src/tool/registry.ts (1)
state(27-58)
packages/opencode/src/file/index.ts (2)
packages/opencode/src/util/log.ts (2)
file(50-52)error(135-139)packages/opencode/src/project/instance.ts (1)
Instance(15-93)
packages/opencode/src/cli/cmd/tui/component/dialog-model.tsx (1)
packages/opencode/src/cli/cmd/tui/ui/dialog-select.tsx (1)
filter(149-151)
packages/opencode/src/format/index.ts (3)
packages/opencode/src/project/instance.ts (2)
state(48-50)Instance(15-93)packages/opencode/src/config/config.ts (3)
state(197-197)Info(529-714)Info(716-716)packages/opencode/src/format/formatter.ts (14)
enabled(18-20)enabled(27-29)enabled(66-74)enabled(111-120)enabled(127-129)enabled(136-139)enabled(146-148)enabled(155-178)enabled(185-205)enabled(212-220)enabled(227-229)enabled(236-238)enabled(245-247)Info(6-12)
packages/opencode/src/agent/agent.ts (3)
packages/opencode/src/project/instance.ts (2)
state(48-50)Instance(15-93)packages/opencode/src/config/config.ts (3)
state(197-197)Info(529-714)Info(716-716)packages/opencode/src/tool/registry.ts (1)
state(27-58)
packages/opencode/src/config/config.ts (4)
packages/opencode/src/project/instance.ts (4)
directory(39-41)Instance(15-93)worktree(42-44)state(48-50)packages/opencode/src/config/global-file.ts (1)
resolveGlobalFile(5-8)packages/opencode/src/config/persist.ts (1)
update(61-183)packages/opencode/src/config/diff.ts (1)
ConfigDiff(4-26)
packages/opencode/src/provider/provider.ts (3)
packages/opencode/src/project/instance.ts (2)
state(48-50)Instance(15-93)packages/opencode/src/config/config.ts (1)
state(197-197)packages/opencode/src/tool/registry.ts (1)
state(27-58)
packages/opencode/src/project/state.ts (3)
packages/opencode/src/project/instance.ts (3)
state(48-50)dispose(76-80)invalidate(51-53)packages/opencode/src/config/config.ts (1)
state(197-197)packages/opencode/src/tool/registry.ts (2)
state(27-58)register(78-86)
🔇 Additional comments (43)
packages/ui/src/components/select-dialog.tsx (2)
14-14: Good addition for extensibility.The new optional
onKeyEventprop enables custom keyboard event handling while maintaining backward compatibility.
66-72: LGTM! Clean implementation with good reuse.The selected item is computed once and reused, which is more efficient and consistent. Note that
onKeyEventreceives the item state before the key is processed—for navigation keys, this means it gets the current selection before the active item updates. This ordering is appropriate for enabling custom keybind handling that can intercept default behavior.packages/opencode/src/file/index.ts (1)
126-143: Atomic cache rebuild viatemplooks correctBuilding into a local
tempand swappingcache = tempat the end avoids partially-populated snapshots leaking to callers, and the directory walk guards (dir === "."/dir === current) plus theSetfor de-duping look sound across platforms. No functional issues spotted here.packages/sdk/go/config.go (2)
1938-1941: LGTM: Keybind fields added correctly.The new
ModelCycleFavoriteandModelCycleFavoriteReversefields are properly declared and follow the existing pattern for keybind configuration fields.
2015-2016: LGTM: JSON metadata fields added correctly.The corresponding
apijson.Fieldentries for the new keybind fields are properly included in the JSON metadata struct, maintaining consistency with the struct definition.packages/opencode/src/config/diff.ts (1)
1-123: LGTM: Well-structured config diff implementation.The module provides a clean API for computing configuration differences:
ConfigDiffinterface clearly defines tracked fieldscomputeKeysChangedhelper efficiently identifies added/removed/modified keyscomputeDiffsystematically compares all relevant config fields using deep equality- Plugin diffing correctly handles array differences
The implementation is comprehensive and follows good patterns for change detection.
packages/sdk/python/src/opencode_ai/models/keybinds_config.py (1)
46-47: LGTM: Complete keybind field integration.The new
model_cycle_favoriteandmodel_cycle_favorite_reversefields are correctly integrated throughout the class:
- Properly declared with default value "none"
- Included in docstring documentation
- Serialization in
to_dict()is complete- Deserialization in
from_dict()is complete- Constructor properly handles both fields
The implementation follows the established pattern for all other keybind fields.
Also applies to: 100-101, 183-185, 288-291, 396-398, 469-470
packages/opencode/src/config/error.ts (1)
1-45: LGTM: Well-designed error types.The new error types provide comprehensive coverage for config operations:
ConfigUpdateErrorcaptures update context (filepath, scope, directory)ConfigValidationErrorprovides detailed validation errors with field-level informationConfigWriteConflictErrorincludes timeout tracking for conflict scenariosConfigWriteErrorcovers all write operations (create, write, backup, restore)All schemas are properly structured with appropriate field types and constraints.
packages/web/src/content/docs/keybinds.mdx (1)
41-42: LGTM: Documentation updated correctly.The new keybind entries are properly documented with default value "none", consistent with the SDK definitions in Go and Python.
packages/opencode/src/cli/cmd/tui/app.tsx (1)
251-268: LGTM: Favorite cycling commands added correctly.The new command entries follow the established pattern:
- Properly categorized under "Agent"
- Keybind names match the config field names (
model_cycle_favorite,model_cycle_favorite_reverse)- Direction parameters (1, -1) are consistent with existing cycle commands
- Integration with
local.model.cycleFavorite()is cleanpackages/opencode/src/config/global-file.ts (1)
1-8: LGTM: Clean global config path resolver.The implementation is straightforward and correct:
- Creates the config directory recursively (safe if already exists)
- Returns the full path to the global config file
- Simple and focused functionality
packages/opencode/src/lsp/index.ts (1)
9-9: LGTM: State registration refactor applied correctly.The migration from
Instance.state()toState.register()maintains the existing logic while adopting the new state management pattern:
- Named registration ("lsp") enables targeted invalidation
- Root resolver remains the same
- Initialization and cleanup logic are unchanged
This aligns with the broader state lifecycle restructuring across the codebase.
Also applies to: 62-64
packages/opencode/src/config/hot-reload.ts (1)
1-3: LGTM—clean feature flag implementation.The explicit string comparison
=== "true"is clear and unambiguous, preventing accidental truthy values from enabling hot-reload.packages/opencode/src/command/index.ts (1)
40-69: LGTM—consistent state lifecycle migration.The migration from
Instance.state()toState.register()aligns with the broader refactoring across the codebase. The initialization logic is preserved, and the default command fallback remains intact.packages/opencode/src/mcp/index.ts (1)
57-94: LGTM—proper resource lifecycle with teardown.The migration to
State.register()correctly pairs client initialization with teardown. The teardown function properly closes all MCP clients and handles errors gracefully, preventing resource leaks.specs/config-spec.md (1)
1-100: LGTM—comprehensive API specification.The spec clearly documents the PATCH /config endpoint behavior, including:
- Scope selection and file targeting
- Locking and backup/restore flow
- Hot-reload vs. legacy behavior toggle
- Error cases and client workflow examples
The backward-compatibility guidance (lines 97-100) is particularly valuable for integrators.
packages/opencode/src/tool/registry.ts (1)
27-58: LGTM—consistent state lifecycle migration.The migration from
Instance.state()toState.register()maintains the existing tool discovery logic while aligning with the centralized state management pattern.packages/opencode/src/permission/index.ts (1)
53-85: LGTM—proper cleanup of pending permissions.The migration to
State.register()includes a teardown function that rejects all pending permission requests, preventing hanging promises when the state is disposed. This is crucial for graceful shutdown.packages/opencode/src/project/instance.ts (2)
51-53: LGTM—clean delegation to State API.The
invalidate()method provides a convenient Instance-level wrapper for state invalidation, maintaining API consistency with the existingInstance.state()pattern.
54-75: LGTM—robust per-directory iteration with error isolation.The
forEach()implementation correctly:
- Awaits each context before invoking the callback
- Isolates errors per directory using
.catch()on each iteration- Normalizes non-Error exceptions to Error instances
- Logs a warning summary when any errors occur
- Returns detailed error information for caller inspection
This pattern enables safe broadcast operations (e.g., config invalidation across all tracked directories) without one failure blocking others.
packages/opencode/src/project/bootstrap.ts (1)
13-17: ConfigInvalidation.setup() is correctly idempotent. No action needed.The implementation uses a promise-caching pattern that guarantees idempotency:
setupPromiseis declared as a module-level variable initialized toundefined(line 18). The setup function guards against re-execution with an early return ifsetupPromisealready exists (lines 173-174). The event subscription toBus.subscribe()is registered inside the IIFE (line 179), which only executes on the first invocation. Subsequent calls return the cached promise immediately without re-running setup logic, preventing duplicate event listeners and resource leaks.packages/opencode/src/server/server.ts (1)
253-303: Config PATCH hot‑reload gating and error aggregation look consistentThe new PATCH
/configflow with:
- explicit
scope(project/global),- a single
Config.updatecall,diffForPublish‑based section extraction,- hot‑reload gating via
isConfigHotReloadEnabled, and- aggregated publish errors for global scope
is coherent and lines up with the hot‑reload toggle semantics. The per‑directory publish in the global branch plus detailed error message construction (
dir: message) should make partial failures debuggable.The only thing to double‑check is that
diffForPublish’s contract really is “flat record of sections → boolean”, sincesectionsis built from keys with value=== true; if nested or non‑boolean shapes ever appear there, toast enrichment will become misleading.packages/opencode/src/file/watcher.ts (1)
5-72: State.register integration for the file watcher looks correctUsing
State.register("filewatcher", () => Instance.directory, …)aligns with the new invalidate API and keeps per-directory watcher lifecycle nicely scoped. The initializer handles unsupported platforms and non‑git projects gracefully, and the disposer safely no‑ops when no subscription exists.No changes needed here.
packages/opencode/test/config/config.test.ts (4)
4-26: Helpers for hot-reload flag and scoped plugin fixture look good
withHotReloadFlagcorrectly snapshots and restoresOPENCODE_CONFIG_HOT_RELOADeven whenfnthrows, andscopedPluginFixtureusestmpdir+await usingto keep filesystem state isolated. The minimalpackage.json/plugin setup is enough to exercise plugin resolution without pulling in real dependencies.No changes needed here; this is a nice pattern for future env-dependent tests.
Also applies to: 28-61
271-297: Scoped + relative plugin path test aligns with expected resolutionThe
"preserves scoped plugin specifiers and resolves relative plugin paths"test is well-structured:
- It covers both a scoped npm plugin and a relative path plugin in the same config.
- It asserts the resolved config uses a proper
file://URL (pathToFileURL(...).href), which is portable across platforms.This gives good regression coverage for the new plugin path handling logic.
418-421: UsingConfig.update’s returned filepath strengthens this testSwitching to:
const result = await Config.update({ update: newConfig as any }) const writtenConfig = JSON.parse(await Bun.file(result.filepath).text())is a nice improvement: the test no longer has to guess where the config ended up and instead relies on the API contract. This should make it more robust against future changes in persistence layout.
437-463: Hot-reload on/off behavior for scoped plugins is well-coveredThe two tests around scoped plugins and
OPENCODE_CONFIG_HOT_RELOADexercise:
- Enabled hot reload (
"true") withwithHotReloadFlag, verifying the plugin specifier remains"@scope/plugin".- Explicitly disabled hot reload (env var unset), verifying the same outcome.
Together with
scopedPluginFixture, this should catch regressions where scoped npm plugin identifiers are rewritten differently under hot reload vs non‑hot‑reload modes.packages/opencode/test/config/hot-reload.test.ts (4)
13-24: Helper setup for temp globals and workspaces looks solidThe helper utilities (
withFreshGlobalPath,createWorkspace,patchConfig,getConfig,subscribeWithContext,ensureInstance,cleanup) are well-structured, restore global state infinally, and keep tests relatively self-contained. No functional issues stand out here.Also applies to: 26-89
13-24: Global env/config mutation may be fragile under parallel test executionThese tests mutate
process.env.OPENCODE_CONFIG_HOT_RELOADandGlobal.Path.configin multiple places, restored withfinally, which is good. If Bun’s test runner ever executes tests in this file in parallel, though, this shared global mutation could introduce flakiness. Consider enforcing serial execution for this suite or centralizing env/global overrides in a single, serialized harness.Also applies to: 93-368
264-300: Fan-out error handling test is nicely targetedThe fan-out test that subscribes on a target instance, forces a publish failure, and then still asserts that the target’s config was updated (despite a 500 response) is a good coverage point for “partial success + aggregated error” semantics. No issues here.
338-368: Theme-only invalidation test clearly pins desired invalidation setMonkey-patching
Instance.invalidateto assert only["config", "theme"]are hit for a theme-only diff is a clean way to guard against future changes accidentally broadening invalidation scope. The test logic and restoration of the original invalidate function look correct.packages/opencode/src/cli/cmd/tui/context/local.tsx (1)
202-205: Favorite list APIs andcycleFavoritebehavior look coherentThe new
favorite()getter pluscycleFavoritebehavior (valid-model filtering, wraparound, and auto-seeding recents with a cap of 5) form a consistent UX and keep persistence in sync viasave(). The guard that shows an informational toast when no favorites exist is a nice touch.Also applies to: 227-253
packages/opencode/src/cli/cmd/tui/component/dialog-model.tsx (1)
34-75: Options orchestration for recent/favorite/core models is well structuredThe composition of
favoriteOptions,recentOptions, and core provider model options (including favorite stars, “Free” footer, and category labels) is clear and keeps duplicates out of the unfiltered list. Usingconnected()to gate provider descriptions/categories and leaving Free models visible even when not connected is a sensible UX tradeoff.Also applies to: 76-99, 100-159
packages/opencode/src/config/persist.ts (2)
20-41: Target file resolution and normalization behavior look correct
determineTargetFile’s precedence for project files plus creation of a default.opencode/opencode.jsoncunder.opencode/is sensible, andloadFileContent/normalizeConfiggive you a clean base ($schema,agent,mode,plugin) for subsequent diffing and validation. No issues here.Also applies to: 43-59
143-179: Error handling and Zod validation mapping look appropriateThe backup-restore logic plus
ConfigValidationErrorandConfigUpdateErrorwrapping provide good diagnostics without leaking raw Zod errors. MappingZodIssueentries into{ field, message, expected, received }is helpful for surfacing schema issues to callers. Overall error handling here looks correct.packages/opencode/src/agent/agent.ts (1)
43-105: Agent state registration and default agents are consistent and clearUsing
State.register("agent", () => Instance.directory, async () => { ... })to derive a map of agents fromcfg.agentplus built-ins (general,build,plan) is in line with the broader State-based lifecycle. The default agents’ tools and modes, and the way you layer user-defined agents on top (includingdisablesupport anddefaultToolsmerging), all look coherent.Also applies to: 106-139
packages/opencode/src/provider/provider.ts (1)
240-487: State.register integration for provider state looks coherentThe migration to
State.register("provider", () => Instance.directory, async () => { ... })keeps provider/model data scoped per directory and lines up with the new config/tool state patterns. The internal shape{ models, providers, sdk, realIdByKey }is consistent with downstream uses (list,getProvider,getModel,getSDK), and the GitHub Copilot Enterprise augmentation plus post-filtering of models appear well-contained.No blocking issues here beyond the more specific concerns called out in other comments.
packages/opencode/src/config/write.ts (2)
117-221: Nested update handling for provider/mcp keys is a good balance of precision and simplicity
buildUpdateInstructions+buildNestedInstructionsdo a nice job of:
- Respecting
ConfigDiffflags while still falling back to deep equality when no diff is provided.- Minimizing edits for nested records like
providerandmcpby emitting per-child updates or deletions ([key, childKey]), while still replacing whole sub-objects for deeper changes.The use of
isDeepEqual,hasOwn, andsortInstructionskeeps the behavior predictable and deterministic. No issues from a correctness standpoint here.
247-295: Atomic writer and directory sync behavior are appropriately cautious
writeFileAtomicallyandsyncDirectoryare implemented in a robust, POSIX-friendly way:
- Temp file in the target directory, mode
0o600, fsync + close before rename.- Rename wrapped in a try/catch that cleans up the temp file on failure.
- Directory fsync is attempted on non-Windows platforms with reasonable error logging and graceful degradation when
EISDIR/ENOENToccurs.This is suitable for config persistence and should behave well across platforms.
packages/opencode/src/config/config.ts (4)
27-73: Plugin path-like spec detection and resolution look correct and cross-platform awareThe combination of
WINDOWS_RELATIVE_PREFIXES,isPathLikePluginSpecifier, andresolvePluginFileReferencegives you:
- Support for
file://,~/,./,../, absolute paths, and Windows-style relative/absolute paths.- Resolution of relative paths against the config file location.
- Normalization via
pathToFileURL(andnormalizeWindowsPathfor URL-style separators).Applying this in
load()only whendata.plugin?.lengthand leaving non-path-like specifiers (e.g., package names) untouched is a good balance. The logic should behave well on both POSIX and Windows.One small note: since plugin URLs are only normalized in-memory and not written back (unlike
$schema), it’s intentional that user config on disk stays as originally authored.Also applies to: 835-841
89-165: Config state loader and State.register wiring are consistent with the new state model
loadStateFromDisknow centralizes config aggregation:
- Starts from
global()and merges project/up-stackopencode.jsonc/opencode.json.- Honors
OPENCODE_CONFIG,OPENCODE_CONFIG_CONTENT, and well-known auth providers.- Collects a
directoriesarray includingGlobal.Path.config, discovered.opencodedirs, andOPENCODE_CONFIG_DIR.- Loads commands/agents/modes/plugins per directory and appends discovered plugin files.
Registering this via:
export const state = State.register("config", () => Instance.directory, loadStateFromDisk)and then exposing
get()/directories()throughstate()gives you per-directory caching and aligns nicely with other modules that useState.register.
readFreshConfig()delegating directly toloadStateFromDisk()is also a clear escape hatch for bypassing the cache when needed.Also applies to: 191-203
718-748: Global config loading now uses resolveGlobalFile, keeping all sources consistentSwitching
loadGlobalConfigto:const globalFile = await resolveGlobalFile() let result: Info = pipe( {}, mergeDeep(await loadFile(path.join(Global.Path.config, "config.json"))), mergeDeep(await loadFile(path.join(Global.Path.config, "opencode.json"))), mergeDeep(await loadFile(globalFile)), )ensures the canonical global JSONC path is used consistently, while still honoring legacy
config.json/opencode.jsonfiles and TOML migration. This looks correct and keeps the behavior centralized inresolveGlobalFile.
881-897: Config.update delegation to persist module matches the new persistence flowThe new
Config.updatesignature and implementation:export async function update(input: { scope?: "project" | "global"; update: Info; directory?: string }) { const scope = input.scope ?? "project" const directory = input.directory ?? Instance.directory const { update: persistUpdate } = await import("./persist") return persistUpdate({ scope, update: input.update, directory }) }correctly:
- Defaults
scopeto"project"anddirectorytoInstance.directory.- Forwards the update to
config/persist.ts, which now owns diff computation, writing, backup, and cache invalidation.- Returns the enriched
{ before, after, diff, diffForPublish, filepath }structure expected by callers.This looks consistent with the rest of the refactor.
| for (const [key, value] of Object.entries(cfg.agent ?? {})) { | ||
| if (value.disable) { | ||
| delete result[key] | ||
| continue | ||
| } | ||
| let item = result[key] | ||
| if (!item) | ||
| item = result[key] = { | ||
| name: key, | ||
| mode: "all", | ||
| permission: agentPermission, | ||
| options: {}, | ||
| tools: {}, | ||
| builtIn: false, | ||
| } | ||
| const { name, model, prompt, tools, description, temperature, top_p, mode, color, permission, ...extra } = value | ||
| item.options = { | ||
| ...item.options, | ||
| ...extra, | ||
| } | ||
| if (model) item.model = Provider.parseModel(model) | ||
| if (prompt) item.prompt = prompt | ||
| if (tools) | ||
| item.tools = { | ||
| ...item.tools, | ||
| ...tools, | ||
| } | ||
| item.tools = { | ||
| ...defaultTools, | ||
| ...item.tools, | ||
| ...tools, | ||
| } | ||
| item.tools = { | ||
| ...defaultTools, | ||
| ...item.tools, | ||
| } | ||
| if (description) item.description = description | ||
| if (temperature != undefined) item.temperature = temperature | ||
| if (top_p != undefined) item.topP = top_p | ||
| if (mode) item.mode = mode | ||
| if (color) item.color = color | ||
| // just here for consistency & to prevent it from being added as an option | ||
| if (name) item.name = name | ||
| if (description) item.description = description | ||
| if (temperature != undefined) item.temperature = temperature | ||
| if (top_p != undefined) item.topP = top_p | ||
| if (mode) item.mode = mode | ||
| if (color) item.color = color | ||
| // just here for consistency & to prevent it from being added as an option | ||
| if (name) item.name = name | ||
|
|
||
| if (permission ?? cfg.permission) { | ||
| item.permission = mergeAgentPermissions(cfg.permission ?? {}, permission ?? {}) | ||
| if (permission ?? cfg.permission) { | ||
| item.permission = mergeAgentPermissions(cfg.permission ?? {}, permission ?? {}) | ||
| } | ||
| } |
There was a problem hiding this comment.
Permission merging can drop safe defaults and mutates config objects
Two subtle issues in the permission path:
-
Loss of
doom_loop/external_directorydefaultsagentPermissionandplanPermissioncorrectly start fromdefaultPermission/planPermission(withdoom_loop/external_directorydefaulting to"ask"), merged withcfg.permission.- Inside the
for (const [key, value] of Object.entries(cfg.agent ?? {}))loop, the finalif (permission ?? cfg.permission)branch overwritesitem.permissionwithmergeAgentPermissions(cfg.permission ?? {}, permission ?? {}), which ignoresdefaultPermission. - For agents that have an entry in
cfg.agent(including overrides of built-ins) and a globalcfg.permissionthat doesn’t mentiondoom_loop/external_directory, the merged result can change those fields from"ask"toundefined. This weakens the default safety posture for those overrides.
Consider basing this merge on the agent’s existing permission (
item.permission) instead, e.g.mergeAgentPermissions(item.permission, permission ?? {}), so that per-agent overrides extend the default rather than replacing it. -
mergeAgentPermissionsmutates its arguments- The normalization logic rewrites
basePermission.bashandoverridePermission.bashwhen they’re strings. Ifcfg.permissionis passed in, this mutates the shared config object and may have surprising side effects elsewhere that readcfg.permission. - A shallow clone of the inputs before normalization (or working on local temporaries) would avoid this kind of in-place mutation.
- The normalization logic rewrites
Addressing both would make the permission model safer and easier to reason about.
Also applies to: 226-263
| { | ||
| keybind: Keybind.parse("ctrl+f")[0], | ||
| title: "favorite", | ||
| onTrigger: (option) => { | ||
| local.model.toggleFavorite(option.value as ModelValue) | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Guard ctrl+f favorite keybind against non-model options / undefined selection
onTrigger and onSelect both cast option.value to ModelValue. Two edge cases to consider:
- If
DialogSelectever callsonTriggerwithoptionundefined (e.g., empty list or no current selection), this will throw. - When
!connected(), you append provider options fromproviders(), whosevalueshape may not match{ providerID; modelID }. Pressingctrl+fon such an option will flow a malformed object intotoggleFavorite, leading to confusing “Model undefined/undefined is not valid” toasts.
A small defensive check like if (!option || !("providerID" in option.value && "modelID" in option.value)) return; before calling toggleFavorite / set would avoid these edge cases without changing the happy path.
Also applies to: 184-187
🤖 Prompt for AI Agents
In packages/opencode/src/cli/cmd/tui/component/dialog-model.tsx around lines
172-178 (and similarly for 184-187), the callback casts option.value to
ModelValue unguarded which can throw if option is undefined or when provider
options (added when disconnected) have a different shape; add a defensive
early-return that verifies option is truthy and that option.value is an object
containing both providerID and modelID (e.g., check option && typeof
option.value === "object" && "providerID" in option.value && "modelID" in
option.value) before calling local.model.toggleFavorite or local.model.set so
malformed or empty selections are ignored.
| export async function createBackup(filepath: string): Promise<string> { | ||
| const timestamp = new Date().toISOString().replace(/[:.]/g, "-") | ||
| const backupPath = `${filepath}.bak-${timestamp}` | ||
|
|
||
| if (await Bun.file(filepath).exists()) { | ||
| await fs.copyFile(filepath, backupPath) | ||
| } | ||
|
|
||
| return backupPath | ||
| } | ||
|
|
||
| export async function restoreBackup(backupPath: string, targetPath: string): Promise<void> { | ||
| await fs.copyFile(backupPath, targetPath) | ||
| await fs.unlink(backupPath) | ||
| } |
There was a problem hiding this comment.
Clarify behavior when the source file doesn’t exist to avoid failing restore paths
createBackup always returns a backup path, even when the source file is missing and no backup was actually written. If callers unconditionally call restoreBackup with that path, fs.copyFile(backupPath, targetPath) will throw when backupPath doesn’t exist, which is likely to happen on first‑run or “no config yet” scenarios.
You may want to make the “no backup created” case explicit, e.g.:
- Return
Promise<string | null>fromcreateBackupand only callrestoreBackupwhen non‑null, or - Keep the current return type but make
restoreBackupa no‑op when the backup file is missing (optionally logging).
For example, one option on the backup side:
-export async function createBackup(filepath: string): Promise<string> {
+export async function createBackup(filepath: string): Promise<string | null> {
const timestamp = new Date().toISOString().replace(/[:.]/g, "-")
const backupPath = `${filepath}.bak-${timestamp}`
- if (await Bun.file(filepath).exists()) {
- await fs.copyFile(filepath, backupPath)
- }
-
- return backupPath
+ if (await Bun.file(filepath).exists()) {
+ await fs.copyFile(filepath, backupPath)
+ return backupPath
+ }
+
+ return null
}(With corresponding call‑site updates.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function createBackup(filepath: string): Promise<string> { | |
| const timestamp = new Date().toISOString().replace(/[:.]/g, "-") | |
| const backupPath = `${filepath}.bak-${timestamp}` | |
| if (await Bun.file(filepath).exists()) { | |
| await fs.copyFile(filepath, backupPath) | |
| } | |
| return backupPath | |
| } | |
| export async function restoreBackup(backupPath: string, targetPath: string): Promise<void> { | |
| await fs.copyFile(backupPath, targetPath) | |
| await fs.unlink(backupPath) | |
| } | |
| export async function createBackup(filepath: string): Promise<string | null> { | |
| const timestamp = new Date().toISOString().replace(/[:.]/g, "-") | |
| const backupPath = `${filepath}.bak-${timestamp}` | |
| if (await Bun.file(filepath).exists()) { | |
| await fs.copyFile(filepath, backupPath) | |
| return backupPath | |
| } | |
| return null | |
| } | |
| export async function restoreBackup(backupPath: string, targetPath: string): Promise<void> { | |
| await fs.copyFile(backupPath, targetPath) | |
| await fs.unlink(backupPath) | |
| } |
| async function invalidateProvider(diff: ConfigDiff): Promise<void> { | ||
| await Instance.invalidate("provider") | ||
| } | ||
|
|
||
| async function invalidateMCP(diff: ConfigDiff): Promise<void> { | ||
| await Instance.invalidate("mcp") | ||
| } | ||
|
|
||
| async function invalidateLSP(diff: ConfigDiff): Promise<void> { | ||
| await Instance.invalidate("lsp") | ||
| } | ||
|
|
||
| async function invalidateFileWatcher(): Promise<void> { | ||
| await Instance.invalidate("filewatcher") | ||
| } | ||
|
|
||
| async function invalidatePlugin(diff: ConfigDiff): Promise<void> { | ||
| await Instance.invalidate("plugin") | ||
| } | ||
|
|
||
| async function invalidateToolRegistry(): Promise<void> { | ||
| await Instance.invalidate("tool-registry") | ||
| } | ||
|
|
||
| async function invalidatePermission(): Promise<void> { | ||
| await Instance.invalidate("permission") | ||
| } | ||
|
|
||
| async function invalidateCommandAgentFormat(diff: ConfigDiff): Promise<void> { | ||
| if (diff.command) await Instance.invalidate("command") | ||
| if (diff.agent) await Instance.invalidate("agent") | ||
| if (diff.formatter) await Instance.invalidate("format") | ||
| } | ||
|
|
||
| async function invalidateUIAndPrompts(diff: ConfigDiff): Promise<void> { | ||
| if (diff.instructions) await Instance.invalidate("instructions") | ||
| if (diff.theme) await Instance.invalidate("theme") | ||
| } | ||
|
|
There was a problem hiding this comment.
Ensure share/autoshare changes actually invalidate their state
The invalidation orchestration looks good overall (nice use of targets + tasks and guarded diff checks), but there’s a small inconsistency around share/autoshare:
shareSettingsChangedis computed and, when true,"share-settings"is added totargets:
const shareSettingsChanged = diff.share || diff.autoshare
const uiChanged = diff.theme || diff.instructions || shareSettingsChanged
if (uiChanged) {
if (diff.theme) targets.add("theme")
if (diff.instructions) targets.add("instructions")
if (shareSettingsChanged) targets.add("share-settings")
tasks.push(invalidateUIAndPrompts(diff))
}- However,
invalidateUIAndPromptsonly invalidates"instructions"and"theme":
async function invalidateUIAndPrompts(diff: ConfigDiff): Promise<void> {
if (diff.instructions) await Instance.invalidate("instructions")
if (diff.theme) await Instance.invalidate("theme")
}So when only share/autoshare flips, you log "share-settings" as a target but never call Instance.invalidate("share-settings"). If there’s state registered under that name, it won’t be refreshed.
Two options:
- If you intend a real share-settings state, add a dedicated invalidation:
-async function invalidateUIAndPrompts(diff: ConfigDiff): Promise<void> {
- if (diff.instructions) await Instance.invalidate("instructions")
- if (diff.theme) await Instance.invalidate("theme")
-}
+async function invalidateUIAndPrompts(diff: ConfigDiff): Promise<void> {
+ if (diff.instructions) await Instance.invalidate("instructions")
+ if (diff.theme) await Instance.invalidate("theme")
+ if (diff.share || diff.autoshare) await Instance.invalidate("share-settings")
+}- If there is no such state (and you only log it for now), consider dropping
"share-settings"fromtargetsto avoid misleading logs.
Also, several helper functions (invalidateProvider, invalidateMCP, invalidatePlugin, etc.) accept diff but don’t use it. Trimming those unused parameters would simplify the signatures a bit.
Also applies to: 118-133
🤖 Prompt for AI Agents
In packages/opencode/src/config/invalidation.ts around lines 19 to 57 (and also
118 to 133), the code adds "share-settings" to targets when diff.share or
diff.autoshare changes but invalidateUIAndPrompts never invalidates
"share-settings", and several invalidateX functions accept an unused diff
parameter; fix by either adding a call to await
Instance.invalidate("share-settings") inside invalidateUIAndPrompts when
diff.share || diff.autoshare is true, or remove "share-settings" from the
targets generation if no state exists, and remove the unused diff parameters
from helper functions (invalidateProvider, invalidateMCP, invalidatePlugin,
etc.) to simplify signatures and update any call sites accordingly.
| export async function update(input: { scope: "project" | "global"; update: Config.Info; directory: string }): Promise<{ | ||
| before: Config.Info | ||
| after: Config.Info | ||
| diff: ConfigDiff | ||
| diffForPublish: ConfigDiff | ||
| filepath: string | ||
| }> { | ||
| const filepath = await determineTargetFile(input.scope, input.directory) | ||
| const release = await acquireLock(filepath) | ||
|
|
||
| log.info("config.update.start", { | ||
| scope: input.scope, | ||
| directory: input.directory, | ||
| filepath, | ||
| }) | ||
|
|
||
| const beforeGlobal = input.scope === "global" ? await Config.global() : undefined | ||
|
|
||
| try { | ||
| const backupPath = await createBackup(filepath) | ||
|
|
||
| try { | ||
| const before = await Config.get() | ||
|
|
||
| const existingContent = await loadFileContent(filepath) | ||
| const fileContent = existingContent ? parseJsonc(existingContent) : {} | ||
| const previousParsed = existingContent ? Config.Info.safeParse(fileContent) : undefined | ||
| const previousNormalized = previousParsed?.success ? normalizeConfig(previousParsed.data) : undefined | ||
|
|
||
| const merged = mergeDeep(fileContent, input.update) | ||
|
|
||
| const validated = Config.Info.parse(merged) | ||
|
|
||
| const normalized = normalizeConfig(validated) | ||
| const writerDiff = previousNormalized ? computeDiff(previousNormalized, normalized) : undefined | ||
|
|
||
| await writeConfigFile(filepath, normalized, existingContent, { | ||
| diff: writerDiff, | ||
| previous: previousNormalized, | ||
| }).catch((error) => { | ||
| log.error("JSONC write failed, attempting fallback", { | ||
| filepath, | ||
| error: String(error), | ||
| }) | ||
|
|
||
| const content = JSON.stringify(normalized, null, 2) + "\n" | ||
| return writeFileAtomically(filepath, content) | ||
| }) | ||
|
|
||
| const hotReloadEnabled = isConfigHotReloadEnabled() | ||
| if (hotReloadEnabled && input.scope === "global") { | ||
| await State.invalidate("config") | ||
| } | ||
| if (hotReloadEnabled && input.scope === "project") { | ||
| await Instance.invalidate("config") | ||
| } | ||
|
|
||
| log.info("config.update.cacheInvalidated", { | ||
| scope: input.scope, | ||
| directory: input.directory, | ||
| filepath, | ||
| cacheInvalidated: hotReloadEnabled && input.scope === "global", | ||
| hotReloadEnabled, | ||
| }) | ||
|
|
||
| const after = hotReloadEnabled ? await Config.get() : await Config.readFreshConfig() | ||
| const afterGlobal = input.scope === "global" ? await Config.global() : undefined | ||
|
|
||
| const diff = computeDiff(before, after) | ||
| const diffForPublish = input.scope === "global" ? computeDiff(beforeGlobal!, afterGlobal!) : diff | ||
|
|
||
| if (await Bun.file(backupPath).exists()) { | ||
| await fs.unlink(backupPath) | ||
| } | ||
|
|
||
| log.info("config.update.persisted", { | ||
| scope: input.scope, | ||
| directory: input.directory, | ||
| filepath, | ||
| }) | ||
|
|
||
| return { before, after, diff, diffForPublish, filepath } |
There was a problem hiding this comment.
Update flow is robust; small logging inconsistency around cache invalidation
The update pipeline—lock, backup, JSONC-aware merge+validate, atomic write with JSON fallback, hot-reload-aware invalidation, then diff computation—looks solid and handles both project and global scopes cleanly, including diffForPublish based on global-only diffs. One minor nit: in the cache-invalidated log, cacheInvalidated is set to hotReloadEnabled && input.scope === "global", even though you also invalidate Instance for project scope when hot reload is enabled. If that field is consumed downstream, consider either renaming it (e.g., globalCacheInvalidated) or including project-scope invalidation in the flag.
🤖 Prompt for AI Agents
In packages/opencode/src/config/persist.ts around lines 61-142, the log entry
"config.update.cacheInvalidated" sets cacheInvalidated to hotReloadEnabled &&
input.scope === "global" which is inconsistent because
Instance.invalidate("config") is called for project scope; update the log to
accurately reflect what was invalidated — either replace cacheInvalidated with
two booleans (globalCacheInvalidated: hotReloadEnabled && input.scope ===
"global", projectCacheInvalidated: hotReloadEnabled && input.scope ===
"project") or rename to globalCacheInvalidated and add a separate
projectCacheInvalidated flag; ensure the values are computed from
hotReloadEnabled and input.scope and update any downstream consumers if
necessary.
| const state = State.register( | ||
| "format", | ||
| () => Instance.directory, | ||
| async () => { | ||
| const enabled: Record<string, boolean> = {} | ||
| const cfg = await Config.get() | ||
|
|
||
| const formatters: Record<string, Formatter.Info> = {} | ||
| if (cfg.formatter === false) { | ||
| log.info("all formatters are disabled") | ||
| return { | ||
| enabled, | ||
| formatters, | ||
| const formatters: Record<string, Formatter.Info> = {} | ||
| if (cfg.formatter === false) { | ||
| log.info("all formatters are disabled") | ||
| return { | ||
| enabled, | ||
| formatters, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for (const item of Object.values(Formatter)) { | ||
| formatters[item.name] = item | ||
| } | ||
| for (const [name, item] of Object.entries(cfg.formatter ?? {})) { | ||
| if (item.disabled) { | ||
| delete formatters[name] | ||
| continue | ||
| for (const item of Object.values(Formatter)) { | ||
| formatters[item.name] = item | ||
| } | ||
| for (const [name, item] of Object.entries(cfg.formatter ?? {})) { | ||
| if (item.disabled) { | ||
| delete formatters[name] | ||
| continue | ||
| } | ||
| const result: Formatter.Info = mergeDeep(formatters[name] ?? {}, { | ||
| command: [], | ||
| extensions: [], | ||
| ...item, | ||
| }) | ||
| if (result.command.length === 0) continue | ||
| result.enabled = async () => true | ||
| result.name = name | ||
| formatters[name] = result | ||
| } | ||
| const result: Formatter.Info = mergeDeep(formatters[name] ?? {}, { | ||
| command: [], | ||
| extensions: [], | ||
| ...item, | ||
| }) | ||
|
|
||
| if (result.command.length === 0) continue | ||
|
|
||
| result.enabled = async () => true | ||
| result.name = name | ||
| formatters[name] = result | ||
| } | ||
|
|
||
| return { | ||
| enabled, | ||
| formatters, | ||
| } | ||
| }) | ||
| return { | ||
| enabled, | ||
| formatters, | ||
| } | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Guard against non‑formatter exports when iterating Object.values(Formatter)
The new state initializer assumes every runtime export from ./formatter is a Formatter.Info:
for (const item of Object.values(Formatter)) {
formatters[item.name] = item
}If formatter.ts ever exports anything that isn’t an Info object (e.g. a helper function or constant), this will:
- Insert it into
formatterswithitem.name(e.g. function name), and - Later cause runtime failures when
isEnabledcallsitem.enabled()or when code expects anextensionsarray.
To make this more future‑proof, add a small runtime guard so only Info‑shaped values are picked up, e.g.:
- for (const item of Object.values(Formatter)) {
- formatters[item.name] = item
- }
+ for (const value of Object.values(Formatter)) {
+ if (
+ value &&
+ typeof value === "object" &&
+ typeof (value as Formatter.Info).name === "string" &&
+ Array.isArray((value as Formatter.Info).extensions) &&
+ typeof (value as Formatter.Info).enabled === "function"
+ ) {
+ const item = value as Formatter.Info
+ formatters[item.name] = item
+ }
+ }That keeps the auto‑discovery benefit while preventing accidental registration of non‑formatter exports. The rest of the merge/override logic (honoring disabled, requiring non‑empty command, and forcing enabled = async () => true for config‑defined formatters) looks good.
| const disabled = await Config.get().then((cfg) => new Set(cfg.disabled_providers ?? [])) | ||
| // load env | ||
| for (const [providerID, provider] of Object.entries(database)) { | ||
| if (disabled.has(providerID)) continue | ||
| const apiKey = provider.env.map((item) => process.env[item]).at(0) | ||
| if (!apiKey) continue | ||
| mergeProvider( | ||
| providerID, | ||
| // only include apiKey if there's only one potential option | ||
| provider.env.length === 1 ? { apiKey } : {}, | ||
| "env", | ||
| ) |
There was a problem hiding this comment.
Env API key resolution ignores all but the first env var and recomputes config unnecessarily
The env-loading block has two problems:
- It only inspects the first env var in
provider.env, so if the first is unset but a later one is set, the provider will be skipped. - It calls
Config.get()again even thoughconfigis already available earlier in the initializer.
You can fix both by reusing config and properly searching for the first set env var:
- const disabled = await Config.get().then((cfg) => new Set(cfg.disabled_providers ?? []))
+ const disabled = new Set(config.disabled_providers ?? [])
// load env
for (const [providerID, provider] of Object.entries(database)) {
if (disabled.has(providerID)) continue
- const apiKey = provider.env.map((item) => process.env[item]).at(0)
+ const envVars = provider.env ?? []
+ const apiKey = envVars
+ .map((name) => process.env[name])
+ .find((value) => value !== undefined && value !== "")
if (!apiKey) continue
mergeProvider(
providerID,
// only include apiKey if there's only one potential option
- provider.env.length === 1 ? { apiKey } : {},
+ envVars.length === 1 ? { apiKey } : {},
"env",
)
}🤖 Prompt for AI Agents
In packages/opencode/src/provider/provider.ts around lines 370 to 381, the
env-loading loop incorrectly only reads the first declared env var and
re-queries Config.get(); update it to reuse the existing config variable for
disabled providers and to find the first set environment variable by iterating
provider.env (or using find) and checking process.env for each entry, assign
that value to apiKey if found, and keep the existing behavior of passing {
apiKey } to mergeProvider only when provider.env.length === 1.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const existingContent = await loadFileContent(filepath) | ||
| const fileContent = existingContent ? parseJsonc(existingContent) : {} | ||
| const previousParsed = existingContent ? Config.Info.safeParse(fileContent) : undefined |
There was a problem hiding this comment.
The persistence path parses the existing config with parseJsonc(existingContent) but does not enable allowTrailingComma like the reader (load in config.ts) does. Valid JSONC files that contain trailing commas (e.g., { "model": "foo", }) are successfully loaded by Config.get(), but a PATCH /config will hit this parser and throw before writing or emitting events, making config updates fail for otherwise valid files.
Useful? React with 👍 / 👎.
…r config with State.register preserved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/opencode/src/config/config.ts (1)
181-187: Remove duplicate autoshare logic.Lines 181-183 and 185-187 contain identical code that checks
autoshare === trueand setsshare = "auto". This is a copy-paste error.Apply this diff to remove the duplication:
if (result.autoshare === true && !result.share) { result.share = "auto" } - if (result.autoshare === true && !result.share) { - result.share = "auto" - } - if (!result.keybinds) result.keybinds = Info.shape.keybinds.parse({})
♻️ Duplicate comments (1)
packages/opencode/src/provider/provider.ts (1)
386-396: Env API key resolution still only inspects the first env var.The env-loading block still has the issue previously flagged: it only inspects the first env var in
provider.envusing.at(0), so if the first is unset but a later one is set, the provider will be skipped.Apply this diff to properly search for the first set env var:
// load env for (const [providerID, provider] of Object.entries(database)) { if (disabled.has(providerID)) continue - const apiKey = provider.env.map((item) => process.env[item]).at(0) + const envVars = provider.env ?? [] + const apiKey = envVars + .map((name) => process.env[name]) + .find((value) => value !== undefined && value !== "") if (!apiKey) continue mergeProvider( providerID, // only include apiKey if there's only one potential option - provider.env.length === 1 ? { apiKey } : {}, + envVars.length === 1 ? { apiKey } : {}, "env", ) }
🧹 Nitpick comments (2)
packages/opencode/src/server/server.ts (2)
85-101: Config update cache is fine; consider extracting TTL constants and unref-ing the cleanup timerThe LastConfigUpdate map and cleanup loop look correct for the enrichment use case, but there are a couple of small lifecycle nits:
- The 60 s expiry and sweep interval are hard-coded; extracting them into named constants would make it easier to keep them in sync and tune later.
- The
setIntervalat Line 89 will keep the process alive in short‑lived tools/tests that import this module (e.g., just to runopenapi()), because the timer is neverunref’d or cleared.You could wrap the interval in a variable, pull the TTL into a constant, and unref the timer when supported:
- const LastConfigUpdate: Map<string, { scope: "project" | "global"; sections: string[]; at: number }> = new Map() - - // Periodically clean up stale entries from LastConfigUpdate - setInterval(() => { - const now = Date.now() - for (const [dir, entry] of LastConfigUpdate.entries()) { - if (now - entry.at > 60_000) { - LastConfigUpdate.delete(dir) - } - } - }, 60_000) + const LastConfigUpdate: Map<string, { scope: "project" | "global"; sections: string[]; at: number }> = new Map() + const LAST_CONFIG_UPDATE_TTL_MS = 60_000 + const LAST_CONFIG_UPDATE_SWEEP_MS = 60_000 + + // Periodically clean up stale entries from LastConfigUpdate + const lastConfigUpdateTimer = setInterval(() => { + const now = Date.now() + for (const [dir, entry] of LastConfigUpdate.entries()) { + if (now - entry.at > LAST_CONFIG_UPDATE_TTL_MS) { + LastConfigUpdate.delete(dir) + } + } + }, LAST_CONFIG_UPDATE_SWEEP_MS) + if (typeof (lastConfigUpdateTimer as any).unref === "function") { + ;(lastConfigUpdateTimer as any).unref() + }
43-43: Config PATCH + hot‑reload fan‑out looks solid; consider tightening scope handling and partial‑failure semanticsThe new flow to:
- take
scope+directory,- call
Config.updateand useresult.before/after/diffForPublish,- gate
Config.Event.Updatedpublishing onisConfigHotReloadEnabled(), and- remember sections via
rememberConfigUpdateis coherent and matches the intended hot‑reload behavior.
Two refinements you might want to consider:
Scope validation
scopeis currently taken fromc.req.query("scope")and cast to"project" | "global"(Line 253). Any unexpected value (e.g.?scope=foo) will silently flow intoConfig.updateand the subsequent conditionals. If the public API is meant to be strict, a small Zod enum validator on the query (or an explicit runtime guard that falls back to"project"and/or returns 400 on invalid values) would make this safer and self‑documenting.Global fan‑out error semantics
In the global branch, any failure inInstance.forEachresults in aggregatingpublishErrorsand throwing a newError(Lines 291‑300). By that point, the config has already been persisted, so the client will see a 500 even though the write succeeded and only some listeners failed to be notified. If you expect transient notification issues, you may want to:
- keep the logging you have,
- but either return 200 with an
errorsfield in the response, or downgrade to a non‑fatal warning, depending on how critical immediate hot‑reload is for callers.No changes are strictly required, but tightening these two behaviors would make this endpoint more predictable from an API consumer’s perspective.
Also applies to: 253-304
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/sdk/js/src/gen/types.gen.tsis excluded by!**/gen/**
📒 Files selected for processing (4)
packages/opencode/src/config/config.ts(10 hunks)packages/opencode/src/provider/provider.ts(2 hunks)packages/opencode/src/server/server.ts(4 hunks)packages/opencode/src/session/prompt.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/opencode/src/session/prompt.ts
🧰 Additional context used
🧬 Code graph analysis (3)
packages/opencode/src/server/server.ts (3)
packages/opencode/src/project/instance.ts (2)
directory(39-41)Instance(15-93)packages/opencode/src/config/hot-reload.ts (1)
isConfigHotReloadEnabled(1-3)packages/opencode/src/cli/cmd/tui/event.ts (1)
TuiEvent(4-39)
packages/opencode/src/provider/provider.ts (3)
packages/opencode/src/config/config.ts (1)
state(197-197)packages/opencode/src/project/instance.ts (2)
state(48-50)Instance(15-93)packages/opencode/src/tool/registry.ts (1)
state(27-58)
packages/opencode/src/config/config.ts (4)
packages/opencode/src/project/instance.ts (4)
directory(39-41)Instance(15-93)worktree(42-44)state(48-50)packages/opencode/src/config/global-file.ts (1)
resolveGlobalFile(5-8)packages/opencode/src/config/persist.ts (1)
update(61-183)packages/opencode/src/config/diff.ts (1)
ConfigDiff(4-26)
🔇 Additional comments (16)
packages/opencode/src/provider/provider.ts (4)
241-515: State.register pattern correctly applied.The refactoring properly migrates provider initialization to a State.register lifecycle, matching the pattern used in other modules. The scoped timer, async initialization, and returned state structure are all correct.
415-454: Complex GitHub Copilot Enterprise handling is correct.The plugin auth loading properly handles both
github-copilotandgithub-copilot-enterpriseproviders separately. The logic correctly:
- Checks for auth on both variants
- Loads main provider only if main auth exists
- Loads enterprise provider only if enterprise auth exists
- Calls the plugin loader with the appropriate Auth.get function for each
461-506: Model filtering and provider cleanup logic is comprehensive.The filtering correctly:
- Removes disallowed providers
- Blacklists specific problematic models
- Filters experimental/alpha/deprecated models (with flag override)
- Applies provider-specific whitelist/blacklist from config
- Removes providers with no models after filtering
- Handles openrouter npm package special case
308-318: Verify the github-copilot auth plugin provides API endpoint for enterprise provider.The code structure appears sound—
database["github-copilot-enterprise"]is created withapi: undefinedand relies on the externalopencode-copilot-authplugin'sauth.loaderfunction to return the API endpoint via options at line 446-450. However, verification of the external plugin implementation is required to confirm it properly handles the enterprise provider. Ensure the plugin's loader returns the appropriate API endpoint in the returned options forgithub-copilot-enterprise.packages/opencode/src/server/server.ts (1)
1948-1977: Toast enrichment around config saves is defensive and matches the intended UXThe enrichment for
/tui/show-toastlooks good:
- It’s gated on a clear regex (
Saved (global|project) config), so unrelated toasts are untouched.- It prefers the current directory’s fresh entry, with a bounded 10 s window, and has a sane “freshest by scope” fallback.
- All of it is wrapped in a best‑effort
try { … } catch {}and falls back to the original payload on any error, so it can’t break toast delivery.- The
"no changes"fallback when there are no sections is a nice UX improvement over-> undefined.I don’t see any functional issues here; behavior is consistent with the new LastConfigUpdate tracking.
packages/opencode/src/config/config.ts (11)
27-41: LGTM! Comprehensive path detection.The
isPathLikePluginSpecifierfunction correctly identifies various path formats including Windows-specific prefixes, relative paths, absolute paths, and file:// URLs.
43-73: LGTM! Robust plugin reference resolution.The function handles multiple path formats correctly including Unix and Windows paths, relative and absolute paths, and home directory expansion. The try-catch block provides a safe fallback for edge cases.
75-87: LGTM! Well-structured config update event.The event definition appropriately captures the scope, directory, and state changes (before/after/diff) needed for config update notifications.
140-164: LGTM! Plugin accumulation logic is sound.The code correctly accumulates plugin files from multiple directories and defensively ensures
result.pluginis an array before pushing accumulated plugin files.
197-197: LGTM! Proper State lifecycle registration.The state export correctly uses the new
State.registerlifecycle pattern, binding config state to the instance directory with theloadStateFromDiskloader function.
199-202: LGTM! Clean fresh config accessor.The
readFreshConfigfunction provides a straightforward way to bypass caching and load configuration directly from disk.
724-754: LGTM! Improved global config loading.The refactoring to use
resolveGlobalFile()improves modularity and theglobal()function correctly delegates toloadGlobalConfig().
841-846: LGTM! Plugin resolution integrated correctly.The code properly detects and resolves path-like plugin references while preserving npm package names, allowing both types of plugin specifications to coexist.
493-494: LGTM! New favorite model keybinds.The new keybinds for cycling favorite models are properly defined with "none" as defaults, allowing users to opt-in by configuring them.
874-881: The code correctly referencesz.core.$ZodIssue[], which is the documented way to type an array of Zod issue objects in Zod v4. The syntax used in line 878 is valid and compatible with Zod 4.Likely an incorrect or invalid review comment.
887-903: LGTM! Clean separation of concerns.The
updatefunction correctly delegates to the persist module, maintaining a clean separation between the config API surface and persistence implementation. Verification confirms thatConfig.Updatedevents are properly emitted in the server layer after config updates complete, with all relevant context (before, after, diff) included in the event payload.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.