Conversation
PATCH /config hot-reload w/ selective cache invalidation
* Fix cache corruption bug in refresh function
|
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. WalkthroughThis change migrates many modules to a named State.register lifecycle and adds a full, disk-driven config subsystem: atomic JSONC writes (incremental edits), locking, backup/restore, config diffing, hot-reload, targeted invalidation, state registry invalidate APIs, plus tests, server endpoints, and supporting utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as API
participant Persist as Persist Module
participant Lock as Lock Service
participant Write as Writer
participant Bus as EventBus
participant Inval as Invalidation
Client->>Server: PATCH /config (scope, update)
Server->>Persist: update(input)
Persist->>Lock: acquireLock(filepath)
Lock-->>Persist: release()
Persist->>Persist: validate & merge -> computeDiff(before, after)
Persist->>Write: writeConfigFile(filepath, newConfig, existingContent)
alt write fails
Persist->>Persist: restore backup
end
Persist->>Bus: publish Config.Updated(scope,directory,diff)
Bus->>Inval: apply(diff, scope, directory)
par parallel invalidations
Inval->>Inval: invalidate provider/mcp/lsp/plugins/tools/...
end
Bus-->>Server: published
Server-->>Client: 200 {before, after, diff, filepath}
sequenceDiagram
participant Module
participant State as State Registry
Module->>State: register(name, root, init, dispose)
State->>State: record entry keyed by root
Module->>Module: use state value
Module->>State: invalidate(name, key?)
State->>State: find entries, call dispose for matches
State-->>Module: entries removed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![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 hotReloadEnabled = isConfigHotReloadEnabled() | ||
| if (hotReloadEnabled && input.scope === "global") { | ||
| await State.invalidate("config") | ||
| } | ||
| if (hotReloadEnabled && input.scope === "project") { |
There was a problem hiding this comment.
The cache invalidation now runs only when OPENCODE_CONFIG_HOT_RELOAD is true. Because that flag defaults to false, the new code writes the config file but leaves the cached Config state (and dependent provider/agent/tool state) untouched, so subsequent Config.get() calls and running services continue to use the old config until a manual restart/dispose. The previous implementation always disposed the instance after an update, so this change makes config edits appear to succeed while having no effect in the default environment.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (28)
packages/opencode/test/config/config.test.ts (2)
271-297: Good coverage for mixed scoped + relative plugin resolutionThis test nicely verifies:
- Scoped plugin identifiers remain intact.
- Relative plugin paths are normalized to
file://URLs (matchingpathToFileURLbehavior).You might optionally also assert on the exact length/order of
config.pluginfor even tighter regression coverage, but it’s not strictly necessary.
418-421: Config.update filepath assertion covers new API shapeUsing
result.filepathto read back and assert on the written config validates the new return contract. Consider also asserting a couple of other fields fromresult(e.g.,difforscope) in a separate test to lock down the richer payload, but the current check is functionally sufficient.packages/opencode/src/config/diff.ts (1)
28-43: Key‑change computation is correct; set‑based variant is a future micro‑opt
computeKeysChangedcorrectly identifies added, removed, and modified keys, and gracefully handlesundefinedinputs. If provider/mcp maps ever grow large, you could switch fromincludestoSetmembership to avoid O(n²) behavior, but that’s likely premature here.packages/opencode/test/config/hot-reload.test.ts (3)
13-24: Global mutations make tests dependent on serial executionThese tests mutate
Global.Path.config,Instance.invalidate, andprocess.env.OPENCODE_CONFIG_HOT_RELOAD. If the test runner ever executes them in parallel, they will race on this shared state and may become flaky. Consider marking this suite as serial or centralizing these mutations behind a single test harness that guarantees exclusive access.Also applies to: 84-91, 93-368
57-72: ClarifysubscribeWithContextfallback directory behavior
subscribeWithContextfalls back toprocess.cwd()whenevent.properties.directoryis absent. That may not correspond to any initializedInstanceand could be surprising if an event is emitted without a directory. If that case is expected, consider documenting it in the tests; otherwise, it might be safer to ignore such events or fail fast rather than binding them toprocess.cwd().Also applies to: 264-295
240-262: Test name doesn’t match behaviorThe
"event subscriber sees refreshed config before targeted invalidations"test currently only patches the config and assertsConfig.get()returns the updated model, without exercising any subscriber or invalidation ordering. Either rename the test to reflect what it actually asserts or extend it to include a subscription and explicit ordering checks.packages/opencode/src/project/bootstrap.ts (1)
13-18: ConfigInvalidation setup ordering is good; ensure it’s idempotentRunning
ConfigInvalidation.setup()at the top ofInstanceBootstrapis a good way to guarantee invalidation wiring is in place before plugins, LSP, and watchers initialize. SinceInstanceBootstrapmay run for multiple directories, make suresetup()is idempotent (e.g., guards against double subscriptions) so repeated calls don’t cause duplicate listeners or leaks.packages/opencode/src/mcp/index.ts (2)
96-121: Clarify semantics for disabled MCP configs andadd()return shapeWhen
mcp.enabled === false,create()returnsundefined, andadd()records a"failed" / "unknown error"status and returns only that status object, whereas other branches return the fulls.statusmap. Given theStatusunion includes a"disabled"variant, it may be clearer to:
- Treat disabled configs as
"disabled"rather than"failed"with a generic error; and- Make
add()consistently return the full status map (or consistently return just the updated status), to avoid surprising callers.Also applies to: 123-221
267-291: Minor nit: reuse in‑memory state intools()In
tools(), you already haveconst s = await state(), but then fetchclientsSnapshotvia anotherstate()call throughclients(). You can instead uses.clientsdirectly to avoid a second lookup and keep everything on the same snapshot, though this is purely a minor cleanup.packages/opencode/src/permission/index.ts (1)
7-85: Permission state migration toState.registerlooks solidThe migration to
State.register("permission", () => Instance.directory, ...)plus the disposer that rejects all pending requests preserves behavior and adds clean lifecycle handling. If you want to be extra tidy, you could also clearstate.pending/state.approvedinside the disposer after rejecting to help GC, but it’s not required.specs/config-spec.md (1)
1-100: Spec matches implementation and is very clearThe PATCH /config spec lines up well with the current server behavior (scope handling, hot-reload flag, event fan-out, backup/restore semantics). If you want to go further, you could add a short note that some 500s may originate from post-persist notification failures (e.g. global publish) in addition to low-level write/validation errors, but that’s optional.
packages/opencode/src/file/index.ts (1)
122-165: Non-blockingrefresh()may return empty/stale results; consider awaiting the active scanThe new pattern (
refresh()kicking offfetchEntries()without awaiting, andfiles()returningcacheimmediately) means:
- On first use,
File.search()can see an empty{ files: [], dirs: [] }while the initial ripgrep scan is still running.- Subsequent calls trigger a new full
refresh()wheneverfetchingis false, so frequent lookups can schedule repeated scans.If that behavior is intentional (eventual consistency, fast responses), then this is fine. If you’d prefer callers to always see a fully-populated view, a small refactor would help:
- Track a shared
fetchPromiseinstead of justfetching, and haverefresh()return it.- In
files(),await refresh()before returningcache, so the first call waits for the scan and later calls get a fresh snapshot.That would also give you more control over how often you rescan (e.g., only on explicit invalidation).
Also applies to: 159-161
packages/opencode/src/project/instance.ts (1)
51-75: HardenInstance.forEachagainst rejected context promises
invalidatelooks good. InforEach, you already collect errors fromcontext.provide(...).catch(...), but a rejection ofcontextPromiseitself (e.g., a failedProject.fromDirectory) will:
- Throw out of the loop,
- Prevent remaining directories from being processed,
- Bypass your
errorsaggregation.To make
forEachconsistently robust (and matchdisposeAll), consider:for (const [directory, contextPromise] of cache) { const ctx = await contextPromise.catch((error) => { errors.push({ directory, error: error instanceof Error ? error : new Error(String(error)), }) return undefined }) if (!ctx) continue await context .provide(ctx, async () => { await fn(directory) }) .catch((error) => { errors.push({ directory, error: error instanceof Error ? error : new Error(String(error)), }) }) }This keeps
forEach’s contract (“return per-directory errors”) even when initialization fails.packages/opencode/src/server/server.ts (2)
82-100: Config-update cache for toast enrichment is reasonable; consider interval cleanup for testsThe
LastConfigUpdatemap plus the 60s cleanup interval is a straightforward way to enrich later toasts without unbounded growth. For a long-lived server process this is fine. If this module ever gets imported in short-lived test runners without starting the server, you might eventually want to keep the interval handle and expose a way to clear it in tests, but that’s a nice-to-have.
252-303:/configPATCH hot-reload + global publish flow looks consistentThe handler now:
- Honors
scope=project|globalwith a default ofproject.- Delegates to
Config.update({ scope, update, directory })and usesdiffForPublishto derivesections.- Gates
Config.Event.Updatedpublishing onisConfigHotReloadEnabled(), with per-directory fan-out viaInstance.forEachforglobalscope.- Records sections in
LastConfigUpdatefor both project and global cases.This matches the spec’s behavior (hot-reload flag controls cache invalidation + events; global scope broadcasts to all tracked directories). One behavior choice to be aware of: when global publish fails for some directories, the config file has still been updated, but the endpoint returns a 500 with an aggregated error message. That trade-off (persistence succeeds, fan-out failure surfaces as a hard error) seems acceptable, just worth keeping in mind for clients interpreting a 500 as “nothing changed”.
packages/opencode/src/plugin/index.ts (1)
9-9: Plugin lifecycle integration withState.registerlooks solidThe plugin state is now correctly keyed by
Instance.directoryand registered under the"plugin"namespace, and the dispose callback will be invoked viaState.invalidatewith the resolved state object, so hooks + input lifecycle should behave as intended. The init path still lazily loads plugins and accumulateshooks, and subsequent calls re-use the memoized state.One optional improvement: plugin cleanup currently runs sequentially; a single slow or hanging plugin
cleanup()will block the rest. If you care about resilience over strict ordering, consider running them in parallel withPromise.allwhile still logging per-hook failures.Also applies to: 15-67
packages/opencode/src/project/state.ts (1)
9-17: State.register / invalidate design is sound; minor doc/pattern nitThe new named registry +
State.register/State.invalidateflow is consistent:
registerwrapscreate, records(name, key = root(), init, dispose)the first time a given (name, key, init) is accessed, and returns a getter that continues to use the same underlyingcreatememoization.invalidate(name, key?)correctly:
- Expands
name:*into per-registered names and recurses, and- For concrete
name, finds matching entries, looks up correspondingEntryinrecordsByKey, awaits the storedstateand runsdispose(state)if present, then removes just that init from the per-key map.dispose(key)now deletes the entirerecordsByKeybucket for that key, which is appropriate for full instance teardown; any subsequent access will lazily recreate state.The only real nit is in the wildcard comment vs implementation:
- For
"cache:*",patternis computed asname.slice(0, -1)→"cache:", while the docstring says “prefix (before the:*)”, which would be"cache". Given you checkregisteredName.startsWith(pattern),"cache:"is probably what you want for names like"cache:users", but the comment can be misleading. Either adjust the comment or change the slice toslice(0, -2)to match the documented behavior.Otherwise, the lifecycle + invalidation mechanics look correct.
Also applies to: 38-67, 69-137, 167-167
packages/opencode/src/format/index.ts (1)
27-60: Config overrides force formatterenabled()totrue, potentially bypassing built‑in checksIn the state initializer, any formatter mentioned in
cfg.formattergetsresult.enabled = async () => true, even when merging over an existing built‑in formatter:const result: Formatter.Info = mergeDeep(formatters[name] ?? {}, { ...item }) ... result.enabled = async () => trueIf default formatters implement
enabled()with capability checks (e.g., probing whether a binary is available), this change means partial overrides (like just changingcommandorextensions) will now always report enabled, even when those checks would fail.Consider preserving the original
enabledfor existing formatters and only defaulting toasync () => truefor new formatters that come purely from config, e.g.:const base = formatters[name] const result: Formatter.Info = mergeDeep(base ?? {}, { command: [], extensions: [], ...item, }) if (result.command.length === 0) continue result.name = name if (!base?.enabled) { result.enabled = async () => true } formatters[name] = resultThis keeps config overrides flexible without accidentally disabling useful health checks for built‑ins.
packages/opencode/src/provider/provider.ts (1)
242-489: Provider state init is solid; a few robustness tweaks would helpThe new
State.register("provider", ...)flow looks consistent and keeps responsibilities well‑scoped. A couple of small improvements could make it more robust:
- Env key resolution only checks the first variable
const apiKey = provider.env.map((item) => process.env[item]).at(0) if (!apiKey) continueIf any provider has multiple possible env var names, this ignores all but the first. Using the first defined value would be safer:
const apiKey = provider.env.map((item) => process.env[item]).find(Boolean) if (!apiKey) continue
- Avoid duplicate
Config.get()callsYou already have
const config = await Config.get()at the top; later you recompute:const disabled = await Config.get().then((cfg) => new Set(cfg.disabled_providers ?? []))You can reuse
configinstead:const disabled = new Set(config.disabled_providers ?? [])This reduces I/O and avoids subtle inconsistencies if
Config.get()ever stops being pure.
- Defensive guard for custom loaders
Custom loaders assume a
ModelsDev.Provider:for (const [providerID, fn] of Object.entries(CUSTOM_LOADERS)) { ... const result = await fn(database[providerID])If
database[providerID]is ever missing (e.g., models.dev changes), this will passundefinedand likely throw deep inside the loader. A cheap guard avoids surprising crashes:const base = database[providerID] if (!base) continue const result = await fn(base)None of these change the intended behavior but they harden the initialization path and reduce chances of edge‑case failures.
packages/opencode/src/config/invalidation.ts (1)
58-133:diff.toolsis logged but never triggers any invalidation
applyInternalbuildssectionsfrom all boolean flags:const sections = Object.keys(diff).filter((k) => diff[k as keyof ConfigDiff] === true)but only some flags are wired to actual invalidation tasks. Notably,
ConfigDiff.toolsis never used:
- No
toolsChangedcomputed.- No corresponding
Instance.invalidate(...)call.If the
toolssection of config influences any cached state (e.g., tool registry or permissions around tools), those caches will not be refreshed on hot‑reload; only thesectionslog will mention"tools".If
toolsis expected to affect runtime behavior, consider adding something like:const toolsChanged = diff.tools if (toolsChanged) { targets.add("tool-registry") tasks.push(invalidateToolRegistry()) }(or another appropriate target) so that toggling tools via config gets picked up without a restart. If
toolsis intentionally read purely on demand, it may be worth documenting that here in a comment for future maintainers.packages/opencode/src/agent/agent.ts (1)
226-263: makemergeAgentPermissionspure and tighten its typing
mergeAgentPermissionscurrently:
- Mutates its arguments (
basePermission.bash/overridePermission.bash), which can unexpectedly normalizecfg.permissionin place.- Accepts
anyfor both parameters, even though the shapes are known (Config.Info["permission"]/Agent.Info["permission"]/ partials).To make this helper easier to reason about and safer to reuse, consider:
- Guarding with optional chaining instead of assuming objects:
if (typeof basePermission?.bash === "string") { ... } if (typeof overridePermission?.bash === "string") { ... }
- Cloning before normalization to avoid side effects:
const base = basePermission ? { ...basePermission } : {} const override = overridePermission ? { ...overridePermission } : {}
- Typing the parameters as
Partial<Config.Info["permission"]>(or a shared permission type) instead ofany.These changes wouldn’t alter behavior but would make permission handling more robust.
packages/opencode/src/config/write.ts (1)
25-49: Clarify.jsonvs.jsonchandling inwriteConfigFile
isJsoncis currently:const isJsonc = filepath.endsWith(".jsonc") || filepath.endsWith(".json")so existing
.jsonfiles go through the JSONC incremental update + validation path as well. That’s technically fine (JSON ⊂ JSONC), but:
- It differs from the usual expectation that only
.jsoncpreserves comments/formatting via incremental edits.- The variable name suggests only
.jsoncshould take that path.If the intent is to patch both
.jsonand.jsonc, consider renamingisJsoncto something likesupportsJsoncEditsfor clarity. If the intent is to only patch.jsonc, drop the.jsonbranch here so.jsonalways gets a full rewrite.packages/opencode/src/config/persist.ts (2)
51-59: Consider using nullish coalescing for$schemainnormalizeConfigCurrently:
return { $schema: config.$schema || "https://opencode.ai/schema/config.json", ...config, agent: config.agent || {}, mode: config.mode || {}, plugin: config.plugin || [], }Because
...configcomes after$schema, any explicitly set$schemavalue (including"") will overwrite the default. If you want to treat an empty string as “missing” but still prefer explicit non-empty values, a slightly clearer pattern is:return { ...config, $schema: config.$schema ?? "https://opencode.ai/schema/config.json", agent: config.agent ?? {}, mode: config.mode ?? {}, plugin: config.plugin ?? [], }This is mostly a readability/intent clarification rather than a functional change for typical JSON inputs (where
undefinedfields are omitted entirely).
110-124:cacheInvalidatedlog field doesn’t reflect project‑scope invalidationIn
update, you correctly invalidate caches for both scopes:if (hotReloadEnabled && input.scope === "global") { await State.invalidate("config") } if (hotReloadEnabled && input.scope === "project") { await Instance.invalidate("config") }but the log payload reports:
cacheInvalidated: hotReloadEnabled && input.scope === "global",So project‑scope updates with hot reload enabled still log
cacheInvalidated: false, which can be misleading when debugging. Consider either:
- Logging
cacheInvalidated: hotReloadEnabled(since you always invalidate for the active scope), or- Introducing a more explicit field like
invalidateScope: input.scopeand/orprojectCacheInvalidated/globalCacheInvalidated.This is observability-only; runtime behavior is otherwise correct.
packages/opencode/src/config/config.ts (4)
27-41: Plugin path normalization is thoughtful; confirm intended heuristics
isPathLikePluginSpecifier+resolvePluginFileReferencenicely handle:
file://URLs (no-op),- POSIX relative (
./,../) and home (~/) paths,- Windows‑style relative prefixes (
.\\,..\\,~\\),- Absolute paths via
path.isAbsolute.In
load(), only values classified as “path‑like” are rewritten to canonicalfile://URLs; others are left as raw specifiers:if (data.plugin?.length) { for (let i = 0; i < data.plugin.length; i++) { const plugin = data.plugin[i] if (isPathLikePluginSpecifier(plugin)) { data.plugin[i] = resolvePluginFileReference(plugin, configFilepath) } } }This means entries like
"./plugin/foo.js"or"C:\\path\\plugin.js"will be resolved tofile://URLs, but a bare"plugin/foo.js"will be treated as a package/module ID, not a file path.If you want any value containing a path separator to be treated as path‑like, you might extend
isPathLikePluginSpecifieraccordingly; otherwise, documenting this distinction in user‑facing docs would help avoid confusion.Also applies to: 833-838
89-195: Config aggregation and plugin discovery logic look good; small duplication in autoshare handlingThe refactored
loadStateFromDisk:
- Starts from
global()and merges per‑file configs discovered viaFilesystem.findUp.- Collects
.opencodedirectories (plusGlobal.Path.configandOPENCODE_CONFIG_DIR) and merges JSON config, markdown commands/agents/modes, and plugin.js/.tsfiles from each directory.- Accumulates discovered plugin files into
pluginFilesand appends them toresult.pluginafter all scanning.This composition is consistent and gives a clear precedence order.
Minor nit: the autoshare → share mapping appears twice with identical conditions:
if (result.autoshare === true && !result.share) { result.share = "auto" } if (result.autoshare === true && !result.share) { result.share = "auto" }You can safely drop one of these blocks to avoid redundant checks.
716-742: Global config loader correctly adds.jsoncsupport; check$schemaURL consistency
loadGlobalConfignow includes: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)), )which sensibly adds
opencode.jsoncas the preferred global config file while still honoring legacyconfig.json/opencode.json.Later, when migrating from TOML, it sets:
result["$schema"] = "https://opencode.ai/config.json"Elsewhere (e.g.,
normalizeConfig), the default$schemais"https://opencode.ai/schema/config.json". If consumers or tooling rely on a single canonical schema URL, it might be worth unifying these strings; if the difference is intentional (e.g., legacy vs current schema), consider a brief comment to document that distinction.
75-87: EnsureConfig.Event.Updatedis emitted somewhere in the update pathYou’ve defined a useful
Config.Event.UpdatedBus event:export const Event = { Updated: Bus.event( "config.updated", z.object({ scope, directory, refreshed, before, after, diff }), ), }but in the code shown here and in
persist.update, I don’t see a correspondingEvent.Updated.emit(...)call. If emitting this event is part of the hot‑reload/invalidation story, consider wiring it into the end ofpersist.update(usingdiffForPublish) or into thisConfig.updatewrapper so subscribers actually receive notifications.If event emission is handled elsewhere in the PR, feel free to ignore this; just worth double‑checking that this new event type doesn’t remain unused.
📜 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 (29)
packages/opencode/src/agent/agent.ts(2 hunks)packages/opencode/src/command/index.ts(2 hunks)packages/opencode/src/config/backup.ts(1 hunks)packages/opencode/src/config/config.ts(9 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/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)specs/config-spec.md(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (23)
packages/opencode/src/mcp/index.ts (3)
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)
state(48-50)Instance(15-92)
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-92)
packages/opencode/src/format/index.ts (4)
packages/opencode/src/config/config.ts (3)
state(197-197)Info(527-712)Info(714-714)packages/opencode/src/tool/registry.ts (2)
state(27-58)enabled(126-145)packages/opencode/src/project/instance.ts (2)
state(48-50)Instance(15-92)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/provider/provider.ts (3)
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)
state(48-50)Instance(15-92)
packages/opencode/src/agent/agent.ts (3)
packages/opencode/src/config/config.ts (3)
state(197-197)Info(527-712)Info(714-714)packages/opencode/src/tool/registry.ts (1)
state(27-58)packages/opencode/src/project/instance.ts (2)
state(48-50)Instance(15-92)
packages/opencode/src/file/index.ts (2)
packages/opencode/src/util/log.ts (1)
error(135-139)packages/opencode/src/project/instance.ts (1)
Instance(15-92)
packages/opencode/src/config/diff.ts (1)
packages/opencode/src/config/config.ts (2)
Info(527-712)Info(714-714)
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-92)packages/opencode/src/config/hot-reload.ts (1)
isConfigHotReloadEnabled(1-3)
packages/opencode/src/config/backup.ts (1)
packages/opencode/script/postinstall.mjs (1)
targetPath(73-73)
packages/opencode/src/permission/index.ts (4)
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)
state(48-50)Instance(15-92)packages/opencode/src/file/time.ts (1)
state(6-15)
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(527-712)Info(714-714)
packages/opencode/src/tool/registry.ts (2)
packages/opencode/src/config/config.ts (1)
state(197-197)packages/opencode/src/project/instance.ts (2)
state(48-50)Instance(15-92)
packages/opencode/src/project/bootstrap.ts (1)
packages/opencode/src/project/instance.ts (1)
Instance(15-92)
packages/opencode/src/lsp/index.ts (4)
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)
state(48-50)Instance(15-92)packages/opencode/src/file/time.ts (1)
state(6-15)
packages/opencode/src/plugin/index.ts (3)
packages/plugin/src/index.ts (2)
Plugin(27-27)PluginInput(19-25)packages/opencode/src/config/config.ts (1)
state(197-197)packages/opencode/src/project/instance.ts (2)
state(48-50)Instance(15-92)
packages/opencode/src/project/state.ts (4)
packages/opencode/src/config/config.ts (1)
state(197-197)packages/opencode/src/tool/registry.ts (2)
state(27-58)register(78-86)packages/opencode/src/project/instance.ts (3)
state(48-50)dispose(76-79)invalidate(51-53)packages/opencode/src/file/time.ts (1)
state(6-15)
packages/opencode/src/file/watcher.ts (4)
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)
state(48-50)Instance(15-92)packages/opencode/src/file/time.ts (1)
state(6-15)
packages/opencode/src/command/index.ts (3)
packages/opencode/src/config/config.ts (3)
state(197-197)Info(527-712)Info(714-714)packages/opencode/src/tool/registry.ts (1)
state(27-58)packages/opencode/src/project/instance.ts (2)
state(48-50)Instance(15-92)
packages/opencode/test/config/write.test.ts (1)
packages/opencode/src/config/write.ts (1)
writeConfigFile(25-49)
packages/opencode/src/config/persist.ts (9)
packages/opencode/src/project/instance.ts (2)
directory(39-41)Instance(15-92)packages/opencode/src/config/global-file.ts (1)
resolveGlobalFile(5-8)packages/opencode/src/config/config.ts (3)
Info(527-712)Info(714-714)update(879-895)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/test/config/hot-reload.test.ts (2)
packages/opencode/src/project/instance.ts (2)
directory(39-41)Instance(15-92)packages/opencode/src/project/bootstrap.ts (1)
InstanceBootstrap(15-30)
packages/opencode/src/server/server.ts (3)
packages/opencode/src/project/instance.ts (2)
directory(39-41)Instance(15-92)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/config.ts (3)
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 (24)
packages/opencode/src/config/hot-reload.ts (1)
1-3: Env flag helper is clear and explicit
isConfigHotReloadEnableddoes a simple, predictable string check against"true", which is easy to reason about and test.packages/opencode/src/config/global-file.ts (1)
5-7: Global config path resolution looks correctEnsuring
Global.Path.configexists viafs.mkdir(..., { recursive: true })before returningopencode.jsoncis appropriate for global config persistence.packages/opencode/test/config/config.test.ts (5)
4-4: Global import is appropriate for test-only path overridesImporting
Globalhere is warranted for the later test that temporarily overridesGlobal.Path.configand restores it in afinallyblock.
10-26: Env flag helper is robust with proper restoration
withHotReloadFlagsafely snapshots and restoresprocess.env.OPENCODE_CONFIG_HOT_RELOADin afinallyblock, which is important for keeping tests isolated.
28-61: Scoped plugin fixture is well‑structuredThe fixture sets up a realistic scoped plugin under
node_moduleswith minimal surface area and a correspondingopencode.json, which is a good basis for the later plugin tests.
437-449: Hot‑reload enabled test correctly guards against rewriting scoped pluginsThis test ensures that enabling hot reload via
withHotReloadFlag("true", ...)does not rewrite scoped npm plugin specifiers, which is an important invariant for plugin resolution behavior.
451-463: Complementary coverage for hot‑reload disabled behaviorThis test confirms that scoped plugin identifiers are preserved when hot reload is effectively disabled (env var unset), giving symmetric coverage with the enabled case.
packages/opencode/src/config/diff.ts (2)
4-26: ConfigDiff surface is coherent and focusedThe
ConfigDiffinterface cleanly scopes diff signals to just the sections you care about (including key‑level details for provider/mcp and plugin add/remove lists), which should make downstream invalidation logic straightforward.
45-123: Diff computation covers all relevant config facets with sensible semantics
- Uses
isDeepEqualfor structured sections (provider/mcp/lsp/formatter/watcher/etc.), and simple!==for scalar fields likeshare,theme,model, which is appropriate.- Plugin diffs include both a coarse
pluginflag andpluginAdded/pluginRemovedlists for incremental handling. Note that pure reordering will setplugin: truewith empty add/remove arrays, which is a reasonable trade‑off.Overall, this is a solid foundation for driving targeted invalidation and publish flows.
packages/opencode/src/config/error.ts (1)
1-45: Structured config error types look well‑designedThe Zod‑backed
NamedErrordefinitions for update, validation, write conflict, and write errors provide clear, typed payloads (filepath,scope,errors,operation, etc.), which should make both logging and user‑facing messages more consistent and reliable. Just ensure that call sites constructing these errors populate fields in line with these schemas.packages/opencode/test/config/hot-reload.test.ts (3)
13-31: Test helpers and cleanup look solidThe helpers for global-path isolation, workspace creation, and cleanup (including
Instance.disposeAll()and directory removal) give good test isolation and mirror real usage paths of the config system andInstanceBootstrap. No issues from a correctness or lifecycle perspective.Also applies to: 84-91
264-300: Good coverage of global fan‑out error handlingThe fan‑out test nicely verifies both the aggregated 500 response and that the target directory still observes the updated config despite a subscriber throwing. This is a good regression guard for the new invalidation/broadcast logic.
338-368: Theme‑only invalidation test is precise and non‑intrusiveMonkey‑patching
Instance.invalidateto capture invalidation names, then restoring it in afinallyblock, is a clean way to assert that only"theme"(plus the expected"config") is invalidated. The test is focused and should be stable.packages/opencode/src/mcp/index.ts (1)
57-94: MCP state registration and cleanup look correctUsing
State.register("mcp", () => Instance.directory, …)and a disposer that closes all clients centralizes MCP lifecycle per directory and ensures clients are cleaned up on state disposal. Error handling inside the disposer is also reasonable.packages/opencode/src/command/index.ts (1)
40-69: Command state registration is consistent and correctThe migration to
State.register("command", () => Instance.directory, …)cleanly scopes commands per directory, builds a stableRecord<string, Info>, and ensures a sensible defaultinitcommand when missing. UsingInstance.worktreefor templating is appropriate under the Instance-scoped state.packages/opencode/src/file/watcher.ts (1)
5-12: File watcher lifecycle integration looks goodRegistering the watcher via
State.register("filewatcher", () => Instance.directory, …)with a disposer that unsubscribes the subscription ties watcher lifetime cleanly to the instance state. Backend selection, ignore patterns, and flag‑gatedinit()behavior are preserved, so behavior should remain consistent while gaining proper cleanup on dispose.Also applies to: 33-72
packages/opencode/src/server/server.ts (1)
1784-1813: Toast enrichment for config saves is robust and non-invasiveThe
/tui/show-toasthandler now:
- Detects “Saved (global|project) config …” messages,
- Looks up a fresh
LastConfigUpdateentry, preferring the current directory but falling back to the freshest matching-scope entry,- Rewrites
-> undefinedtails or reconstructs the message with a joined section list,- Wraps everything in a
try { ... } catch {}so failures don’t break toast delivery.This is a nice UX improvement and fails gracefully. The regex and 10s freshness window are conservative enough that spurious rewrites should be rare.
packages/opencode/src/tool/registry.ts (1)
17-17: Tool registry state is correctly migrated to namedState.registerThe registry is now scoped under
"tool-registry"and keyed byInstance.directory, with the initializer still discovering filesystem tools and plugin tools into a sharedcustomarray.ToolRegistry.registerandall()correctly await the registered state and mutatecustomin place, so invalidation viaState.invalidate("tool-registry", dir)will force a full rescan on next access.No functional issues spotted with the new lifecycle wiring.
Also applies to: 27-58
packages/opencode/src/lsp/index.ts (1)
9-9: LSP lifecycle is correctly migrated to namedState.registerThe LSP module’s state is now properly registered under
"lsp"and keyed byInstance.directory, with:
- The initializer still constructing
servers,clients,broken, andspawningfromConfigandLSPServer, and- The disposer cleanly shutting down all existing clients.
Callers (
init,status,getClients,run) consistently go through the registeredstate()getter, so invalidation viaState.invalidate("lsp", dir)will fully rebuild LSP state on next access. No issues spotted with the new lifecycle wiring.Also applies to: 62-126
packages/opencode/test/config/write.test.ts (1)
9-75: Nice targeted coverage for JSONC preservation and validityBoth tests exercise the important behaviors of
writeConfigFile:
- Preserving existing JSONC comments while updating fields.
- Ensuring incremental edits remain valid JSONC (validated via
jsonc-parser).- Cleaning up temp directories in
finallyto avoid test pollution.This suite gives good confidence in the incremental write path without overreaching.
packages/opencode/src/agent/agent.ts (1)
43-139: State registration and built‑in agent defaults look consistentThe migration to
State.register("agent", () => Instance.directory, ...)and the construction of thegeneral,build, andplanagents (includingagentPermission/planPermissionand default tool wiring) are internally consistent and align with the new config/state pattern used elsewhere. I don’t see functional issues in this initializer.packages/opencode/src/config/write.ts (2)
84-175: Incremental diff application logic looks solidThe
applyIncrementalUpdates/buildUpdateInstructions/buildNestedInstructionspipeline correctly:
- Uses
ConfigDiffhints where available, but still falls back to deep equality to detect changes when no diff is provided.- Handles nested objects (provider/mcp/agent/command/permission/formatter/lsp/tools/mode) via path-based updates, including deletions through
value: undefined.- Sorts instructions so paths are applied deterministically and without root/child conflicts.
Given the surrounding validation (
validateJsonc) and the fallback to full JSON writes inpersist.update, this looks like a safe and well-structured approach.
265-295: Atomic write and directory sync strategy looks correct
writeFileAtomically+syncDirectoryfollow the standard pattern:
- Write to a temp file with
0o600, fsync, thenrenameto the final path.- Clean up the temp file on rename failure.
- Attempt to fsync the containing directory on POSIX, with sensible logging and error handling.
This should give robust durability semantics for config writes without obvious portability pitfalls.
packages/opencode/src/config/config.ts (1)
197-203: Config state registration and update delegation are aligned with the new persist flow
state = State.register("config", () => Instance.directory, loadStateFromDisk)wires config loading into the shared State mechanism, andget()/directories()correctly projectconfiganddirectoriesfrom that state.readFreshConfig()bypasses the cache by callingloadStateFromDisk()directly, which is appropriate for post‑write readbacks when hot reload is disabled.- The exported
updatewrapper delegates to./persistwhile defaultingscopeto"project"anddirectorytoInstance.directory, matching the rest of the API.This all looks consistent with the new config persistence and invalidation design.
Also applies to: 875-895
| 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.
Per‑agent permission merging may unintentionally drop built‑in defaults
In the per‑agent loop, item.permission is recomputed as:
if (permission ?? cfg.permission) {
item.permission = mergeAgentPermissions(cfg.permission ?? {}, permission ?? {})
}This has a couple of side effects:
- For built‑in agents (e.g.,
general/plan) that already haveagentPermission/planPermission(which includedoom_loop/external_directorydefaults), adding anycfg.agent.<name>entry without apermissionblock still re‑merges fromcfg.permissiononly, effectively discarding those built‑in defaults. - For
plan, a per‑agentpermissionoverride also ignores the stricterplanPermissionbaseline and instead uses only global + per‑agent config as the base.
If you want per‑agent permissions to extend the current agent baseline, a safer pattern would be along the lines of:
const base = item.permission ?? agentPermission
if (permission) {
item.permission = mergeAgentPermissions(base, permission)
}and only fall back to cfg.permission when no agent baseline exists.
Minor: disable is not destructured from value, so disable: false would end up inside item.options via extra. If you don’t want disable to appear in options, consider destructuring it alongside the other known fields.
🤖 Prompt for AI Agents
In packages/opencode/src/agent/agent.ts around lines 140–181, the per-agent
permission merge replaces any existing built-in agent baseline (losing defaults)
and also leaves `disable` inside `item.options`; change the merge to use the
current agent baseline as the base (e.g., base = item.permission ??
agentPermission, falling back to cfg.permission only if neither exists) and only
merge incoming per-agent `permission` into that base (instead of always merging
cfg.permission with permission), and also destructure `disable` from `value`
alongside the other known fields so it is not included in `extra`/options.
| import fs from "fs/promises" | ||
|
|
||
| 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/create-backup contract when source file does not exist
createBackup always returns a backupPath even if Bun.file(filepath).exists() is false and no backup file is actually created. If a caller blindly passes that path into restoreBackup, it will throw (ENOENT) on copyFile.
Two options to make this safer:
- Change the API to
Promise<string | null>(or similar) and returnnullwhen there is nothing to back up, withrestoreBackupbeing a no-op if givennull, or - Keep the API but document/ensure all call sites check
await Bun.file(backupPath).exists()(or track a boolean) before callingrestoreBackup.
Either way, making this explicit will avoid surprising restore failures when there was no original config file.
🤖 Prompt for AI Agents
In packages/opencode/src/config/backup.ts around lines 1 to 17, createBackup
currently returns a backup path even when the source file doesn't exist which
leads restoreBackup to throw ENOENT; change createBackup to return
Promise<string | null> and only compute/return the backupPath (and perform
fs.copyFile) when Bun.file(filepath).exists() is true, otherwise return null;
update restoreBackup to accept backupPath: string | null and be a no-op when
given null; update any call sites to handle the nullable return (or rely on
restoreBackup's no-op) and update function signatures/types accordingly.
| export async function acquireLock(filepath: string, options?: LockOptions): Promise<() => Promise<void>> { | ||
| const normalized = path.normalize(filepath) | ||
| const timeout = options?.timeout ?? 30000 | ||
| const staleAfter = options?.staleAfter ?? LOCKFILE_STALE_AFTER_MS | ||
| const startTime = Date.now() | ||
|
|
||
| while (fileLocks.has(normalized)) { | ||
| const waited = Date.now() - startTime | ||
|
|
||
| if (waited > 5000 && waited < 5100) { | ||
| log.warn("lock acquisition taking longer than expected", { | ||
| filepath: normalized, | ||
| waited, | ||
| }) | ||
| } | ||
|
|
||
| if (waited > timeout) { | ||
| throw new Error(`Lock timeout: could not acquire lock for ${normalized} after ${waited}ms`) | ||
| } | ||
|
|
||
| await fileLocks.get(normalized) | ||
| await Bun.sleep(10) | ||
| } | ||
|
|
||
| let releaseFn: () => void | ||
| const lockPromise = new Promise<void>((resolve) => { | ||
| releaseFn = resolve | ||
| }) | ||
|
|
||
| fileLocks.set(normalized, lockPromise) | ||
|
|
||
| const releaseFilesystem = await acquireFilesystemLock({ | ||
| filepath: normalized, | ||
| timeout, | ||
| staleAfter, | ||
| startTime, | ||
| }) | ||
|
|
||
| return async () => { | ||
| fileLocks.delete(normalized) | ||
| releaseFn!() | ||
| await releaseFilesystem() | ||
| } | ||
| } |
There was a problem hiding this comment.
Failed filesystem lock acquisition can permanently deadlock in‑process callers
In acquireLock, the in‑process lock is registered before awaiting acquireFilesystemLock:
fileLocks.set(normalized, lockPromise)
const releaseFilesystem = await acquireFilesystemLock({ ... })If acquireFilesystemLock throws (e.g., due to timeout or an unexpected fs error), control never reaches the return statement, and:
fileLocksstill containsnormalized.- The stored
lockPromiseis never resolved.
All subsequent acquireLock(normalized) calls will:
while (fileLocks.has(normalized)) {
...
await fileLocks.get(normalized)
await Bun.sleep(10)
}and will wait on a promise that never settles, effectively deadlocking that path for the lifetime of the process.
You can fix this by ensuring the map entry and promise are cleared on error:
export async function acquireLock(filepath: string, options?: LockOptions): Promise<() => Promise<void>> {
const normalized = path.normalize(filepath)
const timeout = options?.timeout ?? 30000
const staleAfter = options?.staleAfter ?? LOCKFILE_STALE_AFTER_MS
const startTime = Date.now()
while (fileLocks.has(normalized)) {
const waited = Date.now() - startTime
if (waited > 5000 && waited < 5100) {
log.warn("lock acquisition taking longer than expected", { filepath: normalized, waited })
}
if (waited > timeout) {
throw new Error(`Lock timeout: could not acquire lock for ${normalized} after ${waited}ms`)
}
await fileLocks.get(normalized)
await Bun.sleep(10)
}
let releaseFn!: () => void
const lockPromise = new Promise<void>((resolve) => {
releaseFn = resolve
})
fileLocks.set(normalized, lockPromise)
let releaseFilesystem: () => Promise<void>
try {
releaseFilesystem = await acquireFilesystemLock({
filepath: normalized,
timeout,
staleAfter,
startTime,
})
} catch (error) {
// Clear in-process lock on failure so future callers are not stuck
fileLocks.delete(normalized)
releaseFn()
throw error
}
return async () => {
fileLocks.delete(normalized)
releaseFn()
await releaseFilesystem()
}
}This preserves existing semantics while guaranteeing that failures in acquireFilesystemLock don’t poison the in‑process lock state.
🤖 Prompt for AI Agents
In packages/opencode/src/config/lock.ts around lines 90 to 133, the in‑process
lock entry is set before awaiting acquireFilesystemLock so if that call throws
the fileLocks map retains the key and the lockPromise never resolves,
permanently blocking future callers; wrap the await acquireFilesystemLock in a
try/catch, declare releaseFn as non‑null and a releaseFilesystem variable, and
on catch delete the map entry and resolve the promise (releaseFn()) before
rethrowing the error so the in‑process lock is always cleared on failure.
| test("appends plugins discovered from directories after merging config files", async () => { | ||
| await using globalTmp = await tmpdir({ | ||
| init: async (dir) => { | ||
| await fs.mkdir(path.join(dir, "plugin"), { recursive: true }) | ||
| await Bun.write(path.join(dir, "plugin", "custom.ts"), "export const plugin = {}") | ||
| await Bun.write( | ||
| path.join(dir, "opencode.json"), | ||
| JSON.stringify({ $schema: "https://opencode.ai/config.json", plugin: ["@scope/plugin"] }, null, 2), | ||
| path.join(dir, "opencode.jsonc"), | ||
| JSON.stringify({ | ||
| $schema: "https://opencode.ai/config.json", | ||
| plugin: ["global-plugin"], | ||
| }), | ||
| ) | ||
| }, | ||
| }) | ||
|
|
||
| await Instance.provide({ | ||
| directory: tmp.path, | ||
| fn: async () => { | ||
| const config = await Config.get() | ||
| const pluginEntries = config.plugin ?? [] | ||
|
|
||
| const baseUrl = pathToFileURL(path.join(tmp.path, "opencode.json")).href | ||
| const expected = import.meta.resolve("@scope/plugin", baseUrl) | ||
|
|
||
| expect(pluginEntries.includes(expected)).toBe(true) | ||
|
|
||
| const scopedEntry = pluginEntries.find((entry) => entry === expected) | ||
| expect(scopedEntry).toBeDefined() | ||
| expect(scopedEntry?.includes("/node_modules/@scope/plugin/")).toBe(true) | ||
| await using workspace = await tmpdir({ | ||
| init: async (dir) => { | ||
| await fs.mkdir(path.join(dir, ".opencode"), { recursive: true }) | ||
| await Bun.write( | ||
| path.join(dir, ".opencode", "opencode.json"), | ||
| JSON.stringify({ | ||
| $schema: "https://opencode.ai/config.json", | ||
| plugin: ["local-plugin"], | ||
| }), | ||
| ) | ||
| }, | ||
| }) | ||
|
|
||
| const previousGlobalConfig = Global.Path.config | ||
| ;(Global.Path as any).config = globalTmp.path | ||
| try { | ||
| await Instance.provide({ | ||
| directory: workspace.path, | ||
| fn: async () => { | ||
| const config = await Config.get() | ||
| expect(config.plugin).toEqual(["local-plugin", `file://${path.join(globalTmp.path, "plugin", "custom.ts")}`]) | ||
| }, | ||
| }) | ||
| } finally { | ||
| ;(Global.Path as any).config = previousGlobalConfig | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Consider using pathToFileURL and be mindful of global state in tests
Two points here:
-
File URL construction
For the expected global plugin path you currently use:`file://${path.join(globalTmp.path, "plugin", "custom.ts")}`To keep behavior consistent and cross‑platform (especially on Windows), it would be safer to mirror the earlier test and use
pathToFileURL(...).hreffor the expectation as well:const expected = pathToFileURL(path.join(globalTmp.path, "plugin", "custom.ts")).href expect(config.plugin).toEqual(["local-plugin", expected])
This avoids OS‑specific path quirks and keeps tests aligned with how URLs are produced in the implementation.
-
Global.Path mutation and potential test flakiness
Temporarily overridingGlobal.Path.configand restoring it infinallyis correct, but it’s still global mutable state. If yourbun:testconfiguration ever runs tests in parallel, this could introduce subtle flakiness when other tests rely onGlobal.Path.configat the same time.
It may be worth verifying that these tests run serially or, longer‑term, refactoring config code to accept an injected path for tests instead of patching the global.
I'll verify the review comment by investigating the codebase. Let me search for relevant patterns.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for pathToFileURL usage in the codebase
rg -n "pathToFileURL" --type ts --type js
Length of output: 658
🏁 Script executed:
#!/bin/bash
# Check the Config.get() implementation and how it constructs file URLs
rg -n "file://" --type ts --type js -C 3Length of output: 14401
🏁 Script executed:
#!/bin/bash
# Look for Global.Path mutations in test files
rg -n "Global\.Path" --type ts -C 2Length of output: 34901
🏁 Script executed:
#!/bin/bash
# Check for bun configuration and test parallelization settings
find . -maxdepth 2 -name "bunfig.toml" -o -name "package.json" -o -name "*.config.*" | head -20Length of output: 66
🏁 Script executed:
#!/bin/bash
# Inspect the actual test file to see full context
cat packages/opencode/test/config/config.test.ts | sed -n '465,505p'Length of output: 1337
I need to check the test configuration and clarify the plugin URL construction logic.
---
🏁 Script executed:
```shell
#!/bin/bash
# Check bunfig.toml for test parallelization settings
cat bunfig.toml
Length of output: 22
🏁 Script executed:
#!/bin/bash
# Get more context around plugin URL construction in config.ts
cat packages/opencode/src/config/config.ts | sed -n '43,75p'Length of output: 994
🏁 Script executed:
#!/bin/bash
# Get context around line 362 where file URLs are constructed
cat packages/opencode/src/config/config.ts | sed -n '350,370p'Length of output: 551
🏁 Script executed:
#!/bin/bash
# Check how plugin specs are differentiated in config.ts
rg -n "isPathLikePluginSpecifier|resolvePluginFileReference" packages/opencode/src/config/config.ts -A 10 | head -60Length of output: 1262
Let me check one more thing to confirm the test pattern consistency:
Use pathToFileURL to align test expectations with actual implementation behavior
The test expectation at line 500 uses string concatenation (file://${path.join(...)}) but the actual implementation normalizes discovered plugin paths through resolvePluginFileReference() (config.ts:837), which calls pathToFileURL(...).href for absolute paths (config.ts:64). This creates a behavior mismatch, especially on Windows where path.join() produces backslashes while pathToFileURL().href generates forward-slash file URIs.
Update the expectation to match:
const expected = pathToFileURL(path.join(globalTmp.path, "plugin", "custom.ts")).href
expect(config.plugin).toEqual(["local-plugin", expected])Regarding global state mutation: by default, Bun runs all tests sequentially within each test file, so parallelism risk is currently low. However, the try/finally restoration pattern is correct and the concern about potential future parallelization remains valid for longer-term refactoring consideration.
🤖 Prompt for AI Agents
packages/opencode/test/config/config.test.ts lines 465-505: the test builds an
expected file URI using string concatenation with path.join which mismatches
resolvePluginFileReference() that uses pathToFileURL(...).href (causing Windows
backslash vs file URI differences); update the assertion to compute the expected
value with pathToFileURL(path.join(globalTmp.path, "plugin", "custom.ts")).href
and use that in expect(...).toEqual([...]); if pathToFileURL is not imported,
add `import { pathToFileURL } from "url"` at the top of the test file.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/opencode/src/server/server.ts (2)
85-101: Config update cache and cleanup look good; minor lifecycle considerationThe per-directory
LastConfigUpdatemap with a 60s cleanup interval is straightforward and safe for a long‑lived server, and pairs well with the 10s “freshness” window used in toast enrichment.One thing to be aware of is that the
setIntervalruns as a top‑level side effect whenever this module is imported, including in tests or any non-server use. If you ever reuseServerin more ephemeral contexts, consider moving the interval setup into theApp()/listen()bootstrap path or guarding it with a one-time flag so tests don’t accumulate background timers. This is non-blocking, just something to keep in mind.
1938-1967: Toast enrichment is solid; consider tightening multi-directory fallbackThe enrichment around
"Saved {scope} config"toasts is nicely defensive (best‑effort with atry/catch, 10s freshness window, and per‑directory preference) and should materially improve UX.In multi‑directory scenarios, the fallback:
candidate = Array.from(LastConfigUpdate.values()) .filter((e) => e.scope === scope && isFresh(e.at)) .sort((a, b) => b.at - a.at)[0]can pull a candidate from a different directory than the one making the
/tui/show-toastcall. That means a toast in directory A could occasionally show section names from directory B if A has no fresh entry.If you want stricter attribution, you could drop the cross‑directory fallback entirely or, alternatively, only log when falling back so you can see whether it’s actually helping in practice. Functionally this is a minor/optional concern; the current behavior is acceptable.
📜 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 (3)
packages/opencode/src/project/instance.ts(1 hunks)packages/opencode/src/provider/provider.ts(2 hunks)packages/opencode/src/server/server.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/opencode/src/project/instance.ts
🧰 Additional context used
🧬 Code graph analysis (2)
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/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)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: update
🔇 Additional comments (4)
packages/opencode/src/provider/provider.ts (4)
12-12: LGTM: Import addition for State.register pattern.The import is correctly added to support the refactor to
State.register().
240-243: LGTM: Correct State.register pattern usage.The refactor properly uses
State.register()with the directory-scoped key function, matching the pattern used across other modules in this PR.Also applies to: 480-486
272-294: LGTM: mergeProvider function logic preserved correctly.The
mergeProviderfunction maintains the same logic as before the refactor, properly handling the creation and merging of provider configurations from multiple sources.
406-440: The disabled provider edge case concern is unfounded; the code logic is sound.After verification, concern #2 about disabling github-copilot but not github-copilot-enterprise is incorrect. The disabled check at line 404 (
if (disabled.has(providerID)) continue) skips the entire iteration before reaching the enterprise handler, so if github-copilot is disabled, enterprise cannot be loaded. Additionally, github-copilot-enterprise is not a separate plugin fromPlugin.list()—it only exists as a synthetic provider variant loaded within the github-copilot plugin block.Concern #3 about database consistency is also sound:
database["github-copilot-enterprise"]is created at line 301 before the plugin loop and is accessed only when the github-copilot plugin is active.Concern #1 regarding the auth loader handling both provider types appears reasonable given the usage pattern, though verifying the actual loader implementation would provide final confirmation.
| if (database["github-copilot"]) { | ||
| const githubCopilot = database["github-copilot"] | ||
| database["github-copilot-enterprise"] = { | ||
| ...githubCopilot, | ||
| id: "github-copilot-enterprise", | ||
| name: "GitHub Copilot Enterprise", | ||
| // Enterprise uses a different API endpoint - will be set dynamically based on auth | ||
| api: undefined, | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that the API endpoint is set correctly for GitHub Copilot Enterprise.
The api field is set to undefined with the expectation that it will be configured dynamically by the auth loader. Ensure that the GitHub Copilot plugin's auth loader properly sets the enterprise API endpoint when loading auth for github-copilot-enterprise.
Run the following script to check how the GitHub Copilot plugin handles enterprise auth:
🏁 Script executed:
#!/bin/bash
# Description: Verify GitHub Copilot plugin sets enterprise API endpoint correctly
# Search for github-copilot plugin auth loader implementation
ast-grep --pattern $'auth: {
$$$
loader: $LOADER
$$$
}'
# Search for references to github-copilot-enterprise API configuration
rg -n --type=ts -C5 'github-copilot-enterprise.*api|baseURL.*enterprise'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Strategy 1: Find all GitHub Copilot plugin files and auth-related code
echo "=== Finding GitHub Copilot plugin files ==="
fd -t f 'github.*copilot|copilot.*github' --ignore-case
echo -e "\n=== Looking for auth loader patterns ==="
rg -n 'loader.*:' --type=ts -A2 -B2 | head -50
echo -e "\n=== Searching for where github-copilot is referenced ==="
rg -n 'github-copilot' --type=ts -C2 | head -80
echo -e "\n=== Looking for database entries and API assignments ==="
rg -n 'database\[.*\].*=|\.api\s*=' --type=ts | head -50Length of output: 6068
🏁 Script executed:
#!/bin/bash
# Strategy 2: Look at the provider.ts file structure to understand auth setup
echo "=== Inspecting provider.ts to find auth setup patterns ==="
rg -n 'auth:|loader:|api:|database\[' packages/opencode/src/provider/provider.ts | head -40Length of output: 661
🏁 Script executed:
#!/bin/bash
# Strategy 3: Find where auth loaders are actually instantiated or configured
echo "=== Searching for AuthLoader or similar patterns ==="
rg -n 'AuthLoader|createAuth|authLoader|setAuth' --type=ts
echo -e "\n=== Looking for enterprise-specific auth handling ==="
rg -n 'enterprise.*auth|auth.*enterprise' --type=ts -iLength of output: 1812
🏁 Script executed:
#!/bin/bash
# Show full context around auth.loader calls to see how return value is used
echo "=== Context around line 422 - auth.loader call ==="
sed -n '415,440p' packages/opencode/src/provider/provider.ts
echo -e "\n=== Context around line 432 - enterprise auth.loader call ==="
sed -n '426,450p' packages/opencode/src/provider/provider.tsLength of output: 2410
🏁 Script executed:
#!/bin/bash
# Search for where 'options' variable from auth.loader is used
echo "=== Looking for 'options' usage after auth.loader ==="
rg -n 'options' packages/opencode/src/provider/provider.ts -C2 | head -60Length of output: 1733
🏁 Script executed:
#!/bin/bash
# Check if github-copilot-enterprise.api is ever modified after initial creation
echo "=== Looking for API modification patterns ==="
rg -n 'github-copilot-enterprise|enterpriseProviderID.*api|api.*=.*enterprise' packages/opencode/src/provider/provider.ts -C3Length of output: 1694
🏁 Script executed:
#!/bin/bash
# Find mergeProvider function definition
echo "=== Finding mergeProvider function ==="
rg -n 'function mergeProvider|const mergeProvider|export.*mergeProvider' packages/opencode/src/provider/provider.ts -A15
# Also search more broadly
echo -e "\n=== Broader search for mergeProvider ==="
rg -n 'mergeProvider' packages/opencode/src/provider/provider.tsLength of output: 1134
🏁 Script executed:
#!/bin/bash
# Look at the GitHub Copilot plugin auth configuration
echo "=== Searching for github-copilot plugin definition ==="
rg -n 'github-copilot.*:.*{|provider:.*github-copilot' packages/opencode/src/provider/provider.ts -A20 | head -80Length of output: 203
🏁 Script executed:
#!/bin/bash
# Check what GitHub Copilot plugin's auth.loader returns/does
echo "=== Search for GithubCopilot auth implementation in plugins ==="
fd -t f '\.ts$' -path '*/plugin*' 2>/dev/null | xargs rg -l 'github-copilot|copilot' 2>/dev/null
# Find plugin directory structure
echo -e "\n=== Plugin structure ==="
fd -t d 'plugin' | head -20Length of output: 582
🏁 Script executed:
#!/bin/bash
# Look at the GitHub Copilot plugin configuration in plugin/index.ts
echo "=== GitHub Copilot plugin auth configuration ==="
cat packages/opencode/src/plugin/index.tsLength of output: 3459
🏁 Script executed:
#!/bin/bash
# Show full mergeProvider function to understand complete logic
echo "=== Full mergeProvider function ==="
sed -n '272,300p' packages/opencode/src/provider/provider.tsLength of output: 1017
🌐 Web query:
opencode-copilot-auth npm package GitHub Copilot Enterprise baseURL
💡 Result:
Use the provider options.baseURL in your opencode.json for the Copilot provider. Example:
{
"$schema":"https://opencode.ai/config.json",
"provider": {
"copilot": {
"options": {
"baseURL": "https://your-copilot-enterprise-host.example"
}
}
}
}
Documentation: opencode providers (shows options.baseURL) and Copilot provider notes. [1][2]
Sources:
[1] opencode providers docs (options.baseURL).
[2] opencode GitHub README (Copilot provider instructions).
GitHub Copilot Enterprise API endpoint configuration is incomplete and may fail silently.
The api: undefined field relies on the auth loader providing a baseURL in returned options, but this mechanism is not guaranteed. When mergeProvider is called (lines 432–437), it only auto-sets baseURL if info.api exists (line 282), which it doesn't for the enterprise provider. If the auth loader returns empty options or doesn't include baseURL, the enterprise provider will have no API endpoint configured.
Required fix: Either:
- Provide a fallback API endpoint when creating
github-copilot-enterprise(line 306), or - Add validation in
mergeProviderto ensurebaseURLis set for enterprise before finalizing provider configuration, or - Document that users must explicitly configure
baseURLin theiropencode.jsonconfig file and that the auth loader must also provide it.
| const scope = (c.req.query("scope") as "project" | "global" | undefined) ?? "project" | ||
| const directory = Instance.directory | ||
|
|
||
| const result = await Config.update({ | ||
| scope, | ||
| update: config, | ||
| directory, | ||
| }) | ||
|
|
||
| const publishDiff = result.diffForPublish | ||
| const hotReloadEnabled = isConfigHotReloadEnabled() | ||
| const sections = Object.keys(publishDiff).filter((k) => (publishDiff as any)[k] === true) | ||
| // Remember sections for toast enrichment regardless of hot reload mode | ||
| rememberConfigUpdate(directory, scope, sections) | ||
|
|
||
| if (hotReloadEnabled && scope === "project") { | ||
| await Bus.publish(Config.Event.Updated, { | ||
| scope, | ||
| directory, | ||
| refreshed: true, | ||
| before: result.before, | ||
| after: result.after, | ||
| diff: publishDiff, | ||
| }) | ||
| } | ||
| if (hotReloadEnabled && scope === "global") { | ||
| const publishErrors = await Instance.forEach(async (dir) => { | ||
| await Bus.publish(Config.Event.Updated, { | ||
| scope, | ||
| directory: dir, | ||
| refreshed: true, | ||
| before: result.before, | ||
| after: result.after, | ||
| diff: publishDiff, | ||
| }) | ||
| rememberConfigUpdate(dir, scope, sections) | ||
| }) | ||
|
|
||
| if (publishErrors.length > 0) { | ||
| log.error("config.publish.failure", { scope, errors: publishErrors }) | ||
| const details = publishErrors | ||
| .map((failure) => { | ||
| const message = failure.error instanceof Error ? failure.error.message : String(failure.error) | ||
| return `${failure.directory}: ${message}` | ||
| }) | ||
| .join("; ") | ||
| throw new Error(`Failed to notify directories: ${details}`) | ||
| } | ||
| } | ||
|
|
||
| return c.json(result.after) | ||
| }, |
There was a problem hiding this comment.
Guard scope query parsing and make diff handling defensive
Two things worth tightening here:
-
scopeparsing trusts raw query
const scope = (c.req.query("scope") as "project" | "global" | undefined) ?? "project"is only a TypeScript assertion; at runtime any string (e.g."foo") will:- Be passed straight into
Config.update, which likely expects only"project"or"global". - Fail both
scope === "project"andscope === "global"checks, so hot‑reload notifications won’t fire even though the config did update.
You can make this robust and keep the same external behavior by explicitly mapping the query string into the allowed union:
- Be passed straight into
-
const scope = (c.req.query("scope") as "project" | "global" | undefined) ?? "project"
-
const scopeQuery = c.req.query("scope") -
const scope: "project" | "global" = scopeQuery === "global" ? "global" : "project" const directory = Instance.directory
@@
-
const result = await Config.update({ -
scope, -
update: config, -
directory, -
})
-
const result = await Config.update({ -
scope, -
update: config, -
directory, -
})
@@
-
const publishDiff = result.diffForPublish
-
const publishDiff = result.diffForPublish ?? {}This ensures only `"project"`/`"global"` ever reach `Config.update` and the hot‑reload branches.
- Assumption that
diffForPublishis always defined
Ifresult.diffForPublishcan ever beundefined/null,Object.keys(publishDiff)will throw. The?? {}guard above makes this safe without changing semantics when a real diff is present.
🤖 Prompt for AI Agents
In packages/opencode/src/server/server.ts around lines 253 to 304, the handler
trusts the raw query string for scope and assumes result.diffForPublish is
always present; validate the incoming scope by mapping the query value to the
allowed union ("project" | "global") and default to "project" so only those
values are passed into Config.update and used in hot‑reload branches, and make
diff handling defensive by using a nullish fallback (e.g. publishDiff ?? {})
before calling Object.keys so Object.keys never throws and sections calculation
remains safe; ensure rememberConfigUpdate and Bus.publish get the validated
scope and use the safe publishDiff.
- Add enabled_providers config for whitelisting providers - Add per-provider whitelist/blacklist for models - Keep hot-reload branch openrouter npm fix - Resolve merge conflicts in provider.ts
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/opencode/src/provider/provider.ts (1)
305-317: GitHub Copilot Enterprise still relies on auth loader/config forbaseURL; consider validation or clearer guarantees.
github-copilot-enterpriseis cloned fromgithub-copilotwithapi: undefined(Line 315), andmergeProvideronly auto-populatesoptions.baseURLwheninfo.apiis truthy (Line 291). For the enterprise provider, that means:
- The API endpoint is configured only if:
- The Copilot plugin’s
auth.loaderreturns abaseURL(or equivalent) in its options forgithub-copilot-enterprise, or- The user explicitly configures
provider["github-copilot-enterprise"].options.baseURL(or anenterpriseUrlthat the loader maps tobaseURL).If the loader returns minimal options (e.g., just an API key) and config doesn’t set
baseURL, the enterprise provider will be present but non-functional. This is essentially the same concern raised in the previous review.Consider one of:
- Adding a post-merge validation for
github-copilot-enterprisethat warns or disables the provider ifoptions.baseURLis missing, or- Clearly documenting that
provider["github-copilot-enterprise"].options.baseURL(orenterpriseUrlconsumed by the loader) is required, and ideally adding a test around this flow.Please confirm that the Copilot plugin’s
auth.loaderforgithub-copilot-enterprisealways sets the correctbaseURL(or equivalent) when enterprise auth is present.Also applies to: 414-445
🧹 Nitpick comments (3)
packages/opencode/src/provider/provider.ts (1)
400-407: Guard custom loaders against missing database entries.
CUSTOM_LOADERSare invoked withdatabase[providerID], but at least theopencodeloader dereferencesinput.envandinput.models. IfModelsDev.get()ever returns a database without a corresponding entry for one of the custom-loader keys (e.g.,opencoderemoved/renamed), this will throw during provider initialization.A small defensive guard would make this more robust:
- 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)packages/opencode/src/config/config.ts (2)
89-195: Config state loader is well-structured; consider removing duplication and optionally deduping plugins.The new
loadStateFromDisk+state = State.register("config", () => Instance.directory, loadStateFromDisk)design is clear and keeps config loading deterministic per directory. The merge order (global → project files → flags → well-known auth → directories/.opencode → commands/agents/modes/plugins) is sensible, andreadFreshConfig()gives a clean bypass for cache when needed.Two small cleanups:
Duplicate autoshare → share promotion
Lines 181–183 and 185–187 are identical:
if (result.autoshare === true && !result.share) { result.share = "auto" }One of these can be removed with no behavior change.
Optional: dedupe accumulated plugin entries
result.pluginis initialized/merged from config and then appended withpluginFiles. If the same plugin is both declared in config and discovered on disk (or present in multiple directories), you may get duplicates. Consider normalizing:
- if (!result.plugin) {
result.plugin = []- }
- result.plugin.push(...pluginFiles)
- if (!result.plugin) {
result.plugin = []- }
- result.plugin.push(...pluginFiles)
- result.plugin = Array.from(new Set(result.plugin))
Also applies to: 197-203 --- `885-901`: **Confirm all `Config.update` callsites run with a valid `Instance.directory` or pass `directory`.** `Config.update` defaults: - `scope` → `"project"` - `directory` → `Instance.directory` and then delegates to `persist.update`. This is fine as long as: - Callers that do not explicitly pass `directory` are always inside an `Instance.provide` context (so `Instance.directory` is defined), and - Callers that update global scope from outside a project context pass an explicit `directory`. It’s worth double-checking callsites to avoid surprising `Instance.directory` access errors. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 846a84c65dde5e11db44e81932873060cd94a3bf and f2c7ec9c0be3c078f9d05378a0f10283eaf90e32. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `packages/sdk/js/src/gen/types.gen.ts` is excluded by `!**/gen/**` </details> <details> <summary>📒 Files selected for processing (2)</summary> * `packages/opencode/src/config/config.ts` (9 hunks) * `packages/opencode/src/provider/provider.ts` (2 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code graph analysis (2)</summary> <details> <summary>packages/opencode/src/provider/provider.ts (3)</summary><blockquote> <details> <summary>packages/opencode/src/config/config.ts (1)</summary> * `state` (197-197) </details> <details> <summary>packages/opencode/src/project/instance.ts (2)</summary> * `state` (48-50) * `Instance` (15-93) </details> <details> <summary>packages/opencode/src/tool/registry.ts (1)</summary> * `state` (27-58) </details> </blockquote></details> <details> <summary>packages/opencode/src/config/config.ts (4)</summary><blockquote> <details> <summary>packages/opencode/src/project/instance.ts (4)</summary> * `directory` (39-41) * `Instance` (15-93) * `worktree` (42-44) * `state` (48-50) </details> <details> <summary>packages/opencode/src/config/global-file.ts (1)</summary> * `resolveGlobalFile` (5-8) </details> <details> <summary>packages/opencode/src/config/persist.ts (1)</summary> * `update` (61-183) </details> <details> <summary>packages/opencode/src/config/diff.ts (1)</summary> * `ConfigDiff` (4-26) </details> </blockquote></details> </details> </details> <details> <summary>🔇 Additional comments (4)</summary><blockquote> <details> <summary>packages/opencode/src/provider/provider.ts (2)</summary><blockquote> `12-12`: **State import aligns with new State.register lifecycle.** Importing `State` here is appropriate and matches the new per-directory provider state pattern used elsewhere (config, tool registry). No issues. --- `240-509`: **Provider state registration and initialization pipeline look solid.** The `State.register("provider", () => Instance.directory, async () => { ... })` setup cleanly encapsulates provider discovery (env, auth, custom loaders, plugins, config), filtering, and SDK/model caches under a per-directory key. Ordering of merges (env → auth → custom → plugins → config) and the `isProviderAllowed` / model-filtering logic are coherent and maintain expected behavior. </blockquote></details> <details> <summary>packages/opencode/src/config/config.ts (2)</summary><blockquote> `27-41`: **Plugin path normalization across POSIX/Windows looks correct.** The combination of `WINDOWS_RELATIVE_PREFIXES`, `isPathLikePluginSpecifier`, and `resolvePluginFileReference`, plus the `load()`-time rewrite of `data.plugin` entries, cleanly normalizes: - `file://...`, `~/...`, relative (`./`, `../`), Windows-style (`.\\`, `..\\`, `~\\`), and absolute paths - Into consistent `file://` URLs based on the config file’s location. This should make plugin loading far less error-prone across platforms. Also applies to: 43-73, 839-844 --- `722-752`: **Global config loader refactor is reasonable; behavior now always reads from disk.** `loadGlobalConfig()` now: - Uses `resolveGlobalFile()` to pick the authoritative global JSONC path and ensure the directory exists. - Merges `config.json`, `opencode.json`, and the resolved global file via `mergeDeep`. - Optionally imports TOML `config` once, migrates to JSON, and then deletes the TOML source. This is a straightforward, disk-driven source of truth. Just ensure callers that previously relied on memoized global config are OK with the now-always-from-disk behavior (which should be fine given the hot-reload goals). </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/opencode/src/provider/provider.ts (1)
309-318: GitHub Copilot Enterprise API endpoint configuration remains incomplete.This issue was flagged in a previous review and is still present. The
api: undefinedfield (line 316) means thatmergeProvider(line 292) will not auto-set abaseURLbecause the checkif (info.api && !options["baseURL"])will fail. The enterprise provider entirely depends on the auth loader returning abaseURLin its options (lines 441-444). If the loader returns empty options or omitsbaseURL, the enterprise provider will have no API endpoint configured, leading to runtime failures.Consider one of these approaches:
- Add validation in the plugin auth loader section (around lines 441-445) to ensure
baseURLis present inenterpriseOptionsbefore callingmergeProvider- Provide a fallback
baseURLor throw an explicit error if it's missing- Document clearly that users must configure
baseURLinopencode.jsonand that the auth loader must provide itpackages/opencode/src/server/server.ts (1)
43-43: Hardenscopeparsing and guarddiffForPublishto avoid runtime issuesTwo issues here mirror the earlier review on this block:
scopetrusts raw query string at runtime
c.req.query("scope")can be any string (e.g."foo"). The TS cast doesn’t apply at runtime, soscopemay be"foo", which:
- Is passed to
Config.updateeven though it likely expects"project" | "global".- Skips both hot‑reload branches (
scope === "project"/"global"), so publishes never fire even though the config was updated.You can keep the same external API and make the runtime behavior robust by mapping explicitly:
const scope = (c.req.query("scope") as "project" | "global" | undefined) ?? "project"
const scopeQuery = c.req.query("scope")const scope: "project" | "global" = scopeQuery === "global" ? "global" : "project"
Assumption that
result.diffForPublishis always defined
Ifresult.diffForPublishis everundefined/null,Object.keys(publishDiff)will throw (even when hot reload is disabled) before you reach the publish branches.Making this defensive is cheap and keeps semantics the same when a real diff is present:
const publishDiff = result.diffForPublish
const publishDiff = result.diffForPublish ?? {}With that, `sections` will just be `[]` when there’s no diff, and `rememberConfigUpdate` will store `sections: []`, which your toast enrichment already renders as `"no changes"`.Also applies to: 252-304
🧹 Nitpick comments (1)
packages/opencode/src/server/server.ts (1)
1946-1977: Toast enrichment logic is good; consider tightening cross-directory fallbackThe enrichment of “Saved {scope} config -> undefined” toasts using
LastConfigUpdateis a nice UX improvement and the try/catch keeps it from breaking the endpoint.Two minor refinements to consider:
- For project-scoped toasts, the cross-directory fallback (
Array.from(LastConfigUpdate.values())…) could, in rare cases, borrow sections from another project if there’s no recent entry for the current directory. If you want strictly per-project semantics, you could skip that fallback whenscope === "project".- In the catch-all
catch {}you lose visibility into unexpected bugs in the enrichment path. A low-verbositylog.debug/log.warnwith the error could help debugging without polluting logs.Both are non-blocking, but worth considering for correctness and observability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/opencode/src/provider/provider.ts(2 hunks)packages/opencode/src/server/server.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/opencode/src/provider/provider.ts (2)
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/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)
🔇 Additional comments (9)
packages/opencode/src/provider/provider.ts (8)
13-13: LGTM!The import of
Stateis necessary for the refactor to useState.registerfor lifecycle management.
241-511: LGTM!The refactor to use
State.registerproperly encapsulates provider initialization within a state lifecycle. The pattern is consistent with other state registrations in the codebase (e.g., config state), and all initialization logic is preserved. The returned state object ({ models, providers, sdk, realIdByKey }) correctly exposes the necessary data for downstream consumers.
513-515: LGTM!The
list()function correctly accesses the registered state to return providers. The pattern is consistent with the new state-based lifecycle.
517-584: LGTM!The
getSDKfunction properly awaits the state and accesses provider options, SDK cache, and other state properties. Error handling wraps failures inInitError, which maintains the existing error contract.
586-588: LGTM!The
getProviderfunction correctly accesses the state to retrieve provider information by ID.
590-650: LGTM!The
getModelfunction correctly awaits state and accesses all necessary state properties (providers, models, realIdByKey). The error handling forNoSuchModelErroris preserved, maintaining the existing error contract.
652-675: LGTM!The
getSmallModelfunction properly accesses state to retrieve provider information and select an appropriate small model based on priority.
687-701: LGTM!The
defaultModelfunction correctly useslist()to access providers and select a default model. The logic is preserved from the previous implementation.packages/opencode/src/server/server.ts (1)
85-101: Per-directory LastConfigUpdate tracking looks solidThe per-directory cache plus periodic cleanup is straightforward and should work well for toast enrichment; the 60s TTL vs 10s “freshness” window gives a reasonable safety margin without leaking memory in long-lived servers.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/opencode/src/server/server.ts (1)
254-305: Guardscopequery parsing and make diff handling defensive.The issues flagged in the previous review still exist:
scopeparsing trusts raw query
Line 254's TypeScript assertion allows any string from the query parameter. At runtime, an invalid value like"foo"will pass through toConfig.updateand cause the hot‑reload branches to fail silently.Assumption that
diffForPublishis always defined
Line 263 doesn't guard againstundefined/null. Line 265'sObject.keys(publishDiff)will throwTypeErrorifdiffForPublishis missing.Apply this diff to validate the scope and add defensive null handling:
- const scope = (c.req.query("scope") as "project" | "global" | undefined) ?? "project" + const scopeQuery = c.req.query("scope") + const scope: "project" | "global" = scopeQuery === "global" ? "global" : "project" const directory = Instance.directory const result = await Config.update({ scope, update: config, directory, }) - const publishDiff = result.diffForPublish + const publishDiff = result.diffForPublish ?? {} const hotReloadEnabled = isConfigHotReloadEnabled() const sections = Object.keys(publishDiff).filter((k) => (publishDiff as any)[k] === true)
🧹 Nitpick comments (6)
packages/opencode/src/server/server.ts (2)
91-98: Add error handling to the cleanup interval.The
setIntervalcallback lacks error handling. If the cleanup logic throws (e.g., due to unexpected Map state), it could silently fail or cause unhandled promise rejections.Apply this diff:
setInterval(() => { - const now = Date.now() - for (const [dir, entry] of LastConfigUpdate.entries()) { - if (now - entry.at > 60_000) { - LastConfigUpdate.delete(dir) + try { + const now = Date.now() + for (const [dir, entry] of LastConfigUpdate.entries()) { + if (now - entry.at > 60_000) { + LastConfigUpdate.delete(dir) + } } + } catch (error) { + log.error("config.cache.cleanup.failure", { error }) } }, 60_000)
1949-1979: Log errors in the toast enrichment catch block.The empty
catchblock on line 1977 silently swallows all errors from the toast enrichment logic. While the defensive try/catch prevents crashes, it makes debugging difficult when enrichment fails unexpectedly.Apply this diff to add logging:
- } catch {} + } catch (error) { + log.debug("tui.toast.enrich.failure", { error, payload }) + } await Bus.publish(TuiEvent.ToastShow, payload)packages/opencode/src/config/config.ts (4)
27-41: Plugin path normalization is well thought-out; consider centralizing it and deduping plugins.The new
WINDOWS_RELATIVE_PREFIXES,isPathLikePluginSpecifier, andresolvePluginFileReferencehelpers plus the normalization inload()look solid and handle POSIX + Windows cases nicely.Two improvements to consider:
- After appending
pluginFilestoresult.plugin, you may want to dedupe to avoid duplicated entries when the same plugin is both declared in config and discovered viaplugin/directories:- if (!result.plugin) { - result.plugin = [] - } - result.plugin.push(...pluginFiles) + if (!result.plugin) { + result.plugin = [] + } + result.plugin.push(...pluginFiles) + result.plugin = Array.from(new Set(result.plugin))
loadPlugin()still constructs URLs via string concatenation ("file://" + item). On Windows this can yield invalid file URLs, whereas you now havepathToFileURLin this module. It would be more robust and consistent to switch to it, e.g.:async function loadPlugin(dir: string) { const plugins: string[] = [] for await (const item of PLUGIN_GLOB.scan({ absolute: true, followSymlinks: true, dot: true, cwd: dir, })) { plugins.push(pathToFileURL(item).href) } return plugins }This way all plugin specifiers—whether from config files or discovered on disk—share the same normalization rules.
Also applies to: 43-73, 161-165, 352-365, 844-849
75-87: Event payload could be tightened to known config/diff types.
Event.Updatedcurrently usesz.any()forbefore,after, anddiff. Since you already haveConfig.InfoandConfigDiff, you could improve type-safety and tooling by constraining these fields to those shapes (or to their partials) rather thanany, while still keeping runtime behavior the same.
89-105: Config state loader + registry look coherent; only minor polish opportunities.The new
loadStateFromDiskflow (global → per-repo files viafindUp→ flags → auth well-known → per-directory.opencodeoverlays + commands/agents/modes/plugins) together withState.register("config", () => Instance.directory, loadStateFromDisk)andreadFreshConfig()aligns well with the hot‑reload design and per-directory state.A couple of small polish ideas, none of them blocking:
- You re‑establish
result.pluginin several places and again guard it at the end before appendingpluginFiles. The last guard plus dedupe (see previous comment) should be sufficient; earlierresult.plugin = result.plugin || []/??=calls could likely be simplified over time.directoriescorrectly includesGlobal.Path.config, discovered.opencodedirs, andOPENCODE_CONFIG_DIR; if you ever want to surface all physical config files for debugging or UIs, returning separatefilesvsdirectoriescould be useful, but not required now.Functionally this looks good as-is.
Also applies to: 123-132, 139-158, 161-165, 189-195, 197-202
844-849: In-place plugin normalization is correct; keep helpers in sync as they evolve.Normalizing
data.plugininload()viaisPathLikePluginSpecifier→resolvePluginFileReferencecorrectly handlesfile://,~/, relative, and Windows-style paths, and mutatingdata.plugin[i]is fine sincedatais local to this load.Just a note for future changes: if you expand what counts as a path-like specifier, ensure both
isPathLikePluginSpecifierandresolvePluginFileReferenceevolve together so they remain consistent with downstream plugin loading.
📜 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 (3)
packages/opencode/src/config/config.ts(9 hunks)packages/opencode/src/project/bootstrap.ts(1 hunks)packages/opencode/src/server/server.ts(4 hunks)
🧰 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/project/bootstrap.ts (1)
packages/opencode/src/project/instance.ts (1)
Instance(15-93)
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)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: update
🔇 Additional comments (4)
packages/opencode/src/project/bootstrap.ts (2)
13-13: LGTM!The ConfigInvalidation import and setup call are correctly placed early in the bootstrap flow to ensure the config hot-reload infrastructure is ready before other subsystems initialize.
Also applies to: 18-18
27-31: LGTM!The event subscription correctly ties project initialization to the INIT command lifecycle, ensuring the project state is updated after command execution completes.
packages/opencode/src/config/config.ts (2)
727-735: Global config refactor toloadGlobalConfig()/global()is sound.Switching to
resolveGlobalFile()for the JSONC path and makingglobal()just delegate toloadGlobalConfig()keeps behavior clear while ensuring the config directory exists and thatopencode.jsonccleanly overlays the older JSON files. This looks correct and should work well with the new hot‑reload pipeline.Also applies to: 753-757
890-905: No issues found; theConfig.update()wrapper and its call sites are safe.Verification of all 4
Config.update()call sites confirms they all safely provide a directory context:
- config.test.ts:418 — Called within
Instance.provide({ directory: tmp.path, ... })- hot-reload.test.ts:104, 135 — Explicitly pass
directoryparameter- server.ts:257 — Explicitly passes
directoryfromInstance.directoryvariableThe hypothetical risk of calling
Config.updatewithscope: "global"while omittingdirectoryoutside an Instance context does not occur in practice. All callers properly handle the directory parameter, either explicitly or via active instance context.
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 handling.Lines 181-187 are identical to lines 175-177. This duplicate logic should be removed.
Apply this diff to remove the duplication:
if (!result.username) result.username = os.userInfo().username 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)
309-318: GitHub Copilot Enterprise API endpoint configuration remains incomplete.This issue was previously flagged and remains unresolved. The
api: undefinedsetting (line 316) expects the auth loader to providebaseURLin options. However,mergeProvider(line 292) only auto-setsbaseURLfrominfo.apiwhen it exists—which it doesn't for enterprise. If the auth loader fails to providebaseURL, the enterprise provider will have no API endpoint configured, leading to silent failures.Consider one of these solutions:
- Provide a default enterprise API endpoint
- Add validation to ensure
baseURLis set before the provider is used- Document that users must configure
baseURLinopencode.jsonApply this diff to add validation:
} provider.options = mergeDeep(provider.options, options) provider.source = source provider.getModel = getModel ?? provider.getModel + // Validate critical fields + if (id === "github-copilot-enterprise" && !provider.options["baseURL"]) { + log.warn("GitHub Copilot Enterprise requires baseURL in configuration or auth loader") + } }
🧹 Nitpick comments (3)
packages/opencode/src/provider/provider.ts (1)
410-449: Consider simplifying the GitHub Copilot auth loading logic.The special handling for GitHub Copilot Enterprise introduces complexity with multiple conditional checks. While functionally correct, the logic flow could be clearer.
Consider this refactor for improved readability:
for (const plugin of await Plugin.list()) { if (!plugin.auth) continue const providerID = plugin.auth.provider if (disabled.has(providerID)) continue - - // For github-copilot plugin, check if auth exists for either github-copilot or github-copilot-enterprise - let hasAuth = false - const auth = await Auth.get(providerID) - if (auth) hasAuth = true - - // Special handling for github-copilot: also check for enterprise auth - if (providerID === "github-copilot" && !hasAuth) { - const enterpriseAuth = await Auth.get("github-copilot-enterprise") - if (enterpriseAuth) hasAuth = true - } - - if (!hasAuth) continue if (!plugin.auth.loader) continue - - // Load for the main provider if auth exists - if (auth) { + + const auth = await Auth.get(providerID) + if (auth) { const options = await plugin.auth.loader(() => Auth.get(providerID) as any, database[plugin.auth.provider]) mergeProvider(plugin.auth.provider, options ?? {}, "custom") } - - // If this is github-copilot plugin, also register for github-copilot-enterprise if auth exists + + // Handle GitHub Copilot Enterprise as a separate provider if (providerID === "github-copilot") { const enterpriseProviderID = "github-copilot-enterprise" if (!disabled.has(enterpriseProviderID)) { const enterpriseAuth = await Auth.get(enterpriseProviderID) if (enterpriseAuth) { const enterpriseOptions = await plugin.auth.loader( () => Auth.get(enterpriseProviderID) as any, database[enterpriseProviderID], ) mergeProvider(enterpriseProviderID, enterpriseOptions ?? {}, "custom") } } } }packages/opencode/src/config/config.ts (2)
43-73: Optional: Remove unnecessary Windows path normalization on URL output.Line 69 calls
normalizeWindowsPath()on a URL that has already been processed through theURLconstructor. URLs are always normalized to use forward slashes by the standard, making this call redundant.Apply this diff if you prefer cleaner code:
try { const base = pathToFileURL(configFilepath).href const resolved = new URL(plugin, base).href - return normalizeWindowsPath(resolved) + return resolved } catch { return plugin }
75-87: Suggest using stricter types for event payload.The
before,after, anddifffields usez.any(), which loses type safety. Consider usingConfig.Infoforbefore/afterand the importedConfigDifftype fordiff.Apply this diff for better type safety:
export const Event = { Updated: Bus.event( "config.updated", z.object({ scope: z.enum(["project", "global"]), directory: z.string().optional(), refreshed: z.boolean().optional(), - before: z.any(), - after: z.any(), - diff: z.any(), + before: Info, + after: Info, + diff: z.custom<ConfigDiff>(), }), ), }
📜 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 (2)
packages/opencode/src/config/config.ts(9 hunks)packages/opencode/src/provider/provider.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
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 (5)
packages/sdk/js/src/gen/types.gen.ts (1)
Config(983-1235)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)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: update
🔇 Additional comments (10)
packages/opencode/src/provider/provider.ts (3)
241-511: LGTM: State registration pattern correctly implemented.The refactor to
State.registeraligns with the project-wide state management approach. The initialization logic is properly encapsulated, returns the necessary state components, and maintains backward compatibility through the public API.
465-502: LGTM: Provider filtering logic is sound.The filtering correctly removes unauthorized providers, blacklisted models, experimental models (unless flagged), and models outside the configured whitelist. The mutation of
provider.info.modelsis safe since the database is loaded fresh for each state instance.
372-374: No changes needed—the code correctly handles falsy values.The condition
model.id && model.id !== modelIDalready prevents empty strings, undefined, null, and other falsy values from being added to therealIdByKeymapping. The&&operator ensures the second comparison only evaluates ifmodel.idis truthy.packages/opencode/src/config/config.ts (7)
27-41: LGTM! Comprehensive path detection logic.The helper correctly identifies various path-like plugin specifiers across platforms, including file:// URLs, relative paths, home paths, and Windows-specific prefixes.
199-202: LGTM! Clean API for fresh config loading.This new public function provides an explicit way to read configuration directly from disk, bypassing the cache. Useful for scenarios where fresh data is needed, such as after config updates with hot-reload disabled.
728-758: LGTM! Well-structured global config loading.The refactored function properly handles multiple config sources and includes graceful TOML migration logic. The change from lazy loading to an explicit async function aligns with the new hot-reload infrastructure.
845-852: Verify plugin path resolution with various formats.The new logic resolves path-like plugin specifiers to file:// URLs. Ensure this works correctly with all supported formats: relative paths, absolute paths, home paths (~), Windows paths, and URL-based plugins.
Consider testing with various plugin path formats:
./local-plugin.js../shared-plugin.js~/config/plugin.jsfile:///absolute/path/plugin.js- Windows paths:
.\\plugin.js,~\\plugin.js- Package names:
@opencode-ai/plugin
891-907: LGTM! Clean delegation to persist module.The updated signature provides rich return information (before, after, diffs, filepath), and delegation to the persist module is appropriate. Error handling, locking, backup/restore, and validation are properly handled in the persist module.
756-758: No action required—performance concern does not apply.The
Config.global()function is called only 3 times across the codebase (inupgrade.tsandpersist.ts), and none of these calls occur in hot paths or loops. The calls are infrequent CLI and configuration persistence operations where disk I/O is acceptable. The change enables hot-reload support without practical performance impact.
197-197: No issues found with State.register pattern compatibility.Verification confirms the State.register implementation and all usages in config.ts are correct. The
state()function properly returns a Promise that resolves to state objects, and bothget()(line 888) anddirectories()(line 910) correctly handle this with.then()chaining. The pattern is consistent across all other modules using State.register.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/opencode/src/provider/provider.ts (1)
309-318: GitHub Copilot Enterprise API configuration remains incomplete.This is the same issue identified in the previous review. The
api: undefinedfield requires the auth loader to providebaseURLin the returned options. However:
- Line 292's check
if (info.api && !options["baseURL"])won't set a fallback baseURL for enterprise sinceinfo.apiis undefined- If the auth loader (lines 441-445) doesn't return a
baseURL, the enterprise provider will have no API endpoint configured- No validation exists to ensure
baseURLis present before the provider is usedThe past review suggested either providing a fallback API endpoint, adding validation in
mergeProvider, or documenting the requirement. None of these mitigations appear to have been implemented.
🧹 Nitpick comments (1)
packages/opencode/src/provider/provider.ts (1)
465-502: Consider extracting model filtering logic into a helper function.The filtering logic (lines 472-494) combines multiple concerns: blacklist filtering, experimental model filtering, and config-based whitelist/blacklist. Extracting this into a dedicated helper function would improve readability and testability.
Example:
+function filterProviderModels( + models: Record<string, ModelsDev.Model>, + providerID: string, + configProvider?: any +): Record<string, ModelsDev.Model> { + return Object.fromEntries( + Object.entries(models) + .filter(([modelID]) => + modelID !== "gpt-5-chat-latest" && + !(providerID === "openrouter" && modelID === "openai/gpt-5-chat") + ) + .filter(([, model]) => + ((!model.experimental && model.status !== "alpha") || + Flag.OPENCODE_ENABLE_EXPERIMENTAL_MODELS) && + model.status !== "deprecated" + ) + .filter(([modelID]) => { + if (!configProvider) return true + return ( + (!configProvider.blacklist || !configProvider.blacklist.includes(modelID)) && + (!configProvider.whitelist || configProvider.whitelist.includes(modelID)) + ) + }) + ) +} + for (const [providerID, provider] of Object.entries(providers)) { if (!isProviderAllowed(providerID)) { delete providers[providerID] continue } const configProvider = config.provider?.[providerID] - const filteredModels = Object.fromEntries( - Object.entries(provider.info.models) - // Filter out blacklisted models - .filter( - ([modelID]) => - modelID !== "gpt-5-chat-latest" && !(providerID === "openrouter" && modelID === "openai/gpt-5-chat"), - ) - // Filter out experimental models - .filter( - ([, model]) => - ((!model.experimental && model.status !== "alpha") || Flag.OPENCODE_ENABLE_EXPERIMENTAL_MODELS) && - model.status !== "deprecated", - ) - // Filter by provider's whitelist/blacklist from config - .filter(([modelID]) => { - if (!configProvider) return true - - return ( - (!configProvider.blacklist || !configProvider.blacklist.includes(modelID)) && - (!configProvider.whitelist || configProvider.whitelist.includes(modelID)) - ) - }), - ) + const filteredModels = filterProviderModels(provider.info.models, providerID, configProvider) provider.info.models = filteredModels
📜 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 (1)
packages/opencode/src/provider/provider.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
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)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: update
🔇 Additional comments (3)
packages/opencode/src/provider/provider.ts (3)
13-13: LGTM: State import added for centralized state management.The import aligns with the migration to the State.register pattern used across other modules.
282-304: LGTM: mergeProvider function correctly handles provider merging.The function properly handles both initial provider creation and subsequent updates, with appropriate fallback for baseURL from the database.
331-378: LGTM: Model parsing logic handles defaults and custom IDs correctly.The parsing provides comprehensive defaults for model properties and properly tracks custom model IDs in
realIdByKeyfor later resolution.
| const state = State.register( | ||
| "provider", | ||
| () => Instance.directory, | ||
| async () => { | ||
| using _ = log.time("state") | ||
| const config = await Config.get() | ||
| const database = await ModelsDev.get() | ||
|
|
||
| const disabled = new Set(config.disabled_providers ?? []) | ||
| const enabled = config.enabled_providers ? new Set(config.enabled_providers) : null | ||
|
|
||
| function isProviderAllowed(providerID: string): boolean { | ||
| if (enabled && !enabled.has(providerID)) return false | ||
| if (disabled.has(providerID)) return false | ||
| return true | ||
| } | ||
|
|
||
| const providers: { | ||
| [providerID: string]: { | ||
| source: Source | ||
| info: ModelsDev.Provider | ||
| getModel?: (sdk: any, modelID: string, options?: Record<string, any>) => Promise<any> | ||
| options: Record<string, any> | ||
| } | ||
| } = {} | ||
| const models = new Map< | ||
| string, | ||
| { | ||
| providerID: string | ||
| modelID: string | ||
| info: ModelsDev.Model | ||
| language: LanguageModel | ||
| npm?: string | ||
| } | ||
| >() | ||
| const sdk = new Map<number, SDK>() | ||
| // Maps `${provider}/${key}` to the provider's actual model ID for custom aliases. | ||
| const realIdByKey = new Map<string, string>() | ||
|
|
||
| log.info("init") | ||
|
|
||
| function mergeProvider( | ||
| id: string, | ||
| options: Record<string, any>, | ||
| source: Source, | ||
| getModel?: (sdk: any, modelID: string, options?: Record<string, any>) => Promise<any>, | ||
| ) { | ||
| const provider = providers[id] | ||
| if (!provider) { | ||
| const info = database[id] | ||
| if (!info) return | ||
| if (info.api && !options["baseURL"]) options["baseURL"] = info.api | ||
| providers[id] = { | ||
| source, | ||
| info, | ||
| options, | ||
| getModel, | ||
| } | ||
| return | ||
| } | ||
| provider.options = mergeDeep(provider.options, options) | ||
| provider.source = source | ||
| provider.getModel = getModel ?? provider.getModel | ||
| } | ||
|
|
||
| const configProviders = Object.entries(config.provider ?? {}) | ||
|
|
||
| // Add GitHub Copilot Enterprise provider that inherits from GitHub Copilot | ||
| if (database["github-copilot"]) { | ||
| const githubCopilot = database["github-copilot"] | ||
| database["github-copilot-enterprise"] = { | ||
| ...githubCopilot, | ||
| id: "github-copilot-enterprise", | ||
| name: "GitHub Copilot Enterprise", | ||
| // Enterprise uses a different API endpoint - will be set dynamically based on auth | ||
| api: undefined, | ||
| } | ||
| return | ||
| } | ||
| provider.options = mergeDeep(provider.options, options) | ||
| provider.source = source | ||
| provider.getModel = getModel ?? provider.getModel | ||
| } | ||
|
|
||
| const configProviders = Object.entries(config.provider ?? {}) | ||
|
|
||
| // Add GitHub Copilot Enterprise provider that inherits from GitHub Copilot | ||
| if (database["github-copilot"]) { | ||
| const githubCopilot = database["github-copilot"] | ||
| database["github-copilot-enterprise"] = { | ||
| ...githubCopilot, | ||
| id: "github-copilot-enterprise", | ||
| name: "GitHub Copilot Enterprise", | ||
| // Enterprise uses a different API endpoint - will be set dynamically based on auth | ||
| api: undefined, | ||
| } | ||
| } | ||
|
|
||
| for (const [providerID, provider] of configProviders) { | ||
| const existing = database[providerID] | ||
| const parsed: ModelsDev.Provider = { | ||
| id: providerID, | ||
| npm: provider.npm ?? existing?.npm, | ||
| name: provider.name ?? existing?.name ?? providerID, | ||
| env: provider.env ?? existing?.env ?? [], | ||
| api: provider.api ?? existing?.api, | ||
| models: existing?.models ?? {}, | ||
| } | ||
| for (const [providerID, provider] of configProviders) { | ||
| const existing = database[providerID] | ||
| const parsed: ModelsDev.Provider = { | ||
| id: providerID, | ||
| npm: provider.npm ?? existing?.npm, | ||
| name: provider.name ?? existing?.name ?? providerID, | ||
| env: provider.env ?? existing?.env ?? [], | ||
| api: provider.api ?? existing?.api, | ||
| models: existing?.models ?? {}, | ||
| } | ||
|
|
||
| for (const [modelID, model] of Object.entries(provider.models ?? {})) { | ||
| const existing = parsed.models[model.id ?? modelID] | ||
| const name = iife(() => { | ||
| if (model.name) return model.name | ||
| if (model.id && model.id !== modelID) return modelID | ||
| return existing?.name ?? modelID | ||
| }) | ||
| const parsedModel: ModelsDev.Model = { | ||
| id: modelID, | ||
| name, | ||
| release_date: model.release_date ?? existing?.release_date, | ||
| attachment: model.attachment ?? existing?.attachment ?? false, | ||
| reasoning: model.reasoning ?? existing?.reasoning ?? false, | ||
| temperature: model.temperature ?? existing?.temperature ?? false, | ||
| tool_call: model.tool_call ?? existing?.tool_call ?? true, | ||
| cost: | ||
| !model.cost && !existing?.cost | ||
| ? { | ||
| input: 0, | ||
| output: 0, | ||
| cache_read: 0, | ||
| cache_write: 0, | ||
| } | ||
| : { | ||
| cache_read: 0, | ||
| cache_write: 0, | ||
| ...existing?.cost, | ||
| ...model.cost, | ||
| }, | ||
| options: { | ||
| ...existing?.options, | ||
| ...model.options, | ||
| }, | ||
| limit: model.limit ?? | ||
| existing?.limit ?? { | ||
| context: 0, | ||
| output: 0, | ||
| for (const [modelID, model] of Object.entries(provider.models ?? {})) { | ||
| const existing = parsed.models[modelID] | ||
| const parsedModel: ModelsDev.Model = { | ||
| id: modelID, | ||
| name: model.name ?? (model.id && model.id !== modelID ? modelID : (existing?.name ?? modelID)), | ||
| release_date: model.release_date ?? existing?.release_date, | ||
| attachment: model.attachment ?? existing?.attachment ?? false, | ||
| reasoning: model.reasoning ?? existing?.reasoning ?? false, | ||
| temperature: model.temperature ?? existing?.temperature ?? false, | ||
| tool_call: model.tool_call ?? existing?.tool_call ?? true, | ||
| cost: | ||
| !model.cost && !existing?.cost | ||
| ? { | ||
| input: 0, | ||
| output: 0, | ||
| cache_read: 0, | ||
| cache_write: 0, | ||
| } | ||
| : { | ||
| cache_read: 0, | ||
| cache_write: 0, | ||
| ...existing?.cost, | ||
| ...model.cost, | ||
| }, | ||
| options: { | ||
| ...existing?.options, | ||
| ...model.options, | ||
| }, | ||
| modalities: model.modalities ?? | ||
| existing?.modalities ?? { | ||
| input: ["text"], | ||
| output: ["text"], | ||
| }, | ||
| headers: model.headers, | ||
| provider: model.provider ?? existing?.provider, | ||
| } | ||
| if (model.id && model.id !== modelID) { | ||
| realIdByKey.set(`${providerID}/${modelID}`, model.id) | ||
| limit: model.limit ?? | ||
| existing?.limit ?? { | ||
| context: 0, | ||
| output: 0, | ||
| }, | ||
| modalities: model.modalities ?? | ||
| existing?.modalities ?? { | ||
| input: ["text"], | ||
| output: ["text"], | ||
| }, | ||
| headers: model.headers, | ||
| provider: model.provider ?? existing?.provider, | ||
| } | ||
| if (model.id && model.id !== modelID) { | ||
| realIdByKey.set(`${providerID}/${modelID}`, model.id) | ||
| } | ||
| parsed.models[modelID] = parsedModel | ||
| } | ||
| parsed.models[modelID] = parsedModel | ||
| database[providerID] = parsed | ||
| } | ||
|
|
||
| // 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", | ||
| ) | ||
| } | ||
|
|
||
| database[providerID] = parsed | ||
| } | ||
|
|
||
| // 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", | ||
| ) | ||
| } | ||
|
|
||
| // load apikeys | ||
| for (const [providerID, provider] of Object.entries(await Auth.all())) { | ||
| if (disabled.has(providerID)) continue | ||
| if (provider.type === "api") { | ||
| mergeProvider(providerID, { apiKey: provider.key }, "api") | ||
| // load apikeys | ||
| for (const [providerID, provider] of Object.entries(await Auth.all())) { | ||
| if (disabled.has(providerID)) continue | ||
| if (provider.type === "api") { | ||
| mergeProvider(providerID, { apiKey: provider.key }, "api") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // load custom | ||
| for (const [providerID, fn] of Object.entries(CUSTOM_LOADERS)) { | ||
| if (disabled.has(providerID)) continue | ||
| const result = await fn(database[providerID]) | ||
| if (result && (result.autoload || providers[providerID])) { | ||
| mergeProvider(providerID, result.options ?? {}, "custom", result.getModel) | ||
| // load custom | ||
| for (const [providerID, fn] of Object.entries(CUSTOM_LOADERS)) { | ||
| if (disabled.has(providerID)) continue | ||
| const result = await fn(database[providerID]) | ||
| if (result && (result.autoload || providers[providerID])) { | ||
| mergeProvider(providerID, result.options ?? {}, "custom", result.getModel) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for (const plugin of await Plugin.list()) { | ||
| if (!plugin.auth) continue | ||
| const providerID = plugin.auth.provider | ||
| if (disabled.has(providerID)) continue | ||
| for (const plugin of await Plugin.list()) { | ||
| if (!plugin.auth) continue | ||
| const providerID = plugin.auth.provider | ||
| if (disabled.has(providerID)) continue | ||
|
|
||
| // For github-copilot plugin, check if auth exists for either github-copilot or github-copilot-enterprise | ||
| let hasAuth = false | ||
| const auth = await Auth.get(providerID) | ||
| if (auth) hasAuth = true | ||
| // For github-copilot plugin, check if auth exists for either github-copilot or github-copilot-enterprise | ||
| let hasAuth = false | ||
| const auth = await Auth.get(providerID) | ||
| if (auth) hasAuth = true | ||
|
|
||
| // Special handling for github-copilot: also check for enterprise auth | ||
| if (providerID === "github-copilot" && !hasAuth) { | ||
| const enterpriseAuth = await Auth.get("github-copilot-enterprise") | ||
| if (enterpriseAuth) hasAuth = true | ||
| } | ||
| // Special handling for github-copilot: also check for enterprise auth | ||
| if (providerID === "github-copilot" && !hasAuth) { | ||
| const enterpriseAuth = await Auth.get("github-copilot-enterprise") | ||
| if (enterpriseAuth) hasAuth = true | ||
| } | ||
|
|
||
| if (!hasAuth) continue | ||
| if (!plugin.auth.loader) continue | ||
| if (!hasAuth) continue | ||
| if (!plugin.auth.loader) continue | ||
|
|
||
| // Load for the main provider if auth exists | ||
| if (auth) { | ||
| const options = await plugin.auth.loader(() => Auth.get(providerID) as any, database[plugin.auth.provider]) | ||
| mergeProvider(plugin.auth.provider, options ?? {}, "custom") | ||
| } | ||
| // Load for the main provider if auth exists | ||
| if (auth) { | ||
| const options = await plugin.auth.loader(() => Auth.get(providerID) as any, database[plugin.auth.provider]) | ||
| mergeProvider(plugin.auth.provider, options ?? {}, "custom") | ||
| } | ||
|
|
||
| // If this is github-copilot plugin, also register for github-copilot-enterprise if auth exists | ||
| if (providerID === "github-copilot") { | ||
| const enterpriseProviderID = "github-copilot-enterprise" | ||
| if (!disabled.has(enterpriseProviderID)) { | ||
| const enterpriseAuth = await Auth.get(enterpriseProviderID) | ||
| if (enterpriseAuth) { | ||
| const enterpriseOptions = await plugin.auth.loader( | ||
| () => Auth.get(enterpriseProviderID) as any, | ||
| database[enterpriseProviderID], | ||
| ) | ||
| mergeProvider(enterpriseProviderID, enterpriseOptions ?? {}, "custom") | ||
| // If this is github-copilot plugin, also register for github-copilot-enterprise if auth exists | ||
| if (providerID === "github-copilot") { | ||
| const enterpriseProviderID = "github-copilot-enterprise" | ||
| if (!disabled.has(enterpriseProviderID)) { | ||
| const enterpriseAuth = await Auth.get(enterpriseProviderID) | ||
| if (enterpriseAuth) { | ||
| const enterpriseOptions = await plugin.auth.loader( | ||
| () => Auth.get(enterpriseProviderID) as any, | ||
| database[enterpriseProviderID], | ||
| ) | ||
| mergeProvider(enterpriseProviderID, enterpriseOptions ?? {}, "custom") | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // load config | ||
| for (const [providerID, provider] of configProviders) { | ||
| mergeProvider(providerID, provider.options ?? {}, "config") | ||
| } | ||
| // load config | ||
| for (const [providerID, provider] of configProviders) { | ||
| mergeProvider(providerID, provider.options ?? {}, "config") | ||
|
|
||
| // TODO: set this in models.dev, not set due to breaking issues on older OC versions | ||
| // u have to set include usage to true w/ this provider, setting in models.dev would cause undefined issue when accessing usage in older versions | ||
| if (providerID === "openrouter") { | ||
| const p = providers[providerID] | ||
| if (p) p.info.npm = "@openrouter/ai-sdk-provider" | ||
| } | ||
|
|
||
| for (const [providerID, provider] of Object.entries(providers)) { | ||
| if (!isProviderAllowed(providerID)) { | ||
| delete providers[providerID] | ||
| continue | ||
| log.info("found", { providerID, npm: providers[providerID]?.info.npm }) | ||
| } | ||
|
|
||
| const configProvider = config.provider?.[providerID] | ||
| const filteredModels = Object.fromEntries( | ||
| Object.entries(provider.info.models) | ||
| // Filter out blacklisted models | ||
| .filter( | ||
| ([modelID]) => | ||
| modelID !== "gpt-5-chat-latest" && !(providerID === "openrouter" && modelID === "openai/gpt-5-chat"), | ||
| ) | ||
| // Filter out experimental models | ||
| .filter( | ||
| ([, model]) => | ||
| ((!model.experimental && model.status !== "alpha") || Flag.OPENCODE_ENABLE_EXPERIMENTAL_MODELS) && | ||
| model.status !== "deprecated", | ||
| ) | ||
| // Filter by provider's whitelist/blacklist from config | ||
| .filter(([modelID]) => { | ||
| if (!configProvider) return true | ||
|
|
||
| return ( | ||
| (!configProvider.blacklist || !configProvider.blacklist.includes(modelID)) && | ||
| (!configProvider.whitelist || configProvider.whitelist.includes(modelID)) | ||
| for (const [providerID, provider] of Object.entries(providers)) { | ||
| if (!isProviderAllowed(providerID)) { | ||
| delete providers[providerID] | ||
| continue | ||
| } | ||
|
|
||
| const configProvider = config.provider?.[providerID] | ||
| const filteredModels = Object.fromEntries( | ||
| Object.entries(provider.info.models) | ||
| // Filter out blacklisted models | ||
| .filter( | ||
| ([modelID]) => | ||
| modelID !== "gpt-5-chat-latest" && !(providerID === "openrouter" && modelID === "openai/gpt-5-chat"), | ||
| ) | ||
| }), | ||
| ) | ||
| // Filter out experimental models | ||
| .filter( | ||
| ([, model]) => | ||
| ((!model.experimental && model.status !== "alpha") || Flag.OPENCODE_ENABLE_EXPERIMENTAL_MODELS) && | ||
| model.status !== "deprecated", | ||
| ) | ||
| // Filter by provider's whitelist/blacklist from config | ||
| .filter(([modelID]) => { | ||
| if (!configProvider) return true | ||
|
|
||
| provider.info.models = filteredModels | ||
| return ( | ||
| (!configProvider.blacklist || !configProvider.blacklist.includes(modelID)) && | ||
| (!configProvider.whitelist || configProvider.whitelist.includes(modelID)) | ||
| ) | ||
| }), | ||
| ) | ||
| provider.info.models = filteredModels | ||
|
|
||
| if (Object.keys(provider.info.models).length === 0) { | ||
| delete providers[providerID] | ||
| continue | ||
| if (Object.keys(provider.info.models).length === 0) { | ||
| delete providers[providerID] | ||
| continue | ||
| } | ||
| log.info("found", { providerID }) | ||
| } | ||
|
|
||
| // TODO: set this in models.dev, not set due to breaking issues on older OC versions | ||
| // u have to set include usage to true w/ this provider, setting in models.dev would cause undefined issue when accessing usage in older versions | ||
| if (providerID === "openrouter") { | ||
| provider.info.npm = "@openrouter/ai-sdk-provider" | ||
| return { | ||
| models, | ||
| providers, | ||
| sdk, | ||
| realIdByKey, | ||
| } | ||
|
|
||
| log.info("found", { providerID, npm: provider.info.npm }) | ||
| } | ||
|
|
||
| return { | ||
| models, | ||
| providers, | ||
| sdk, | ||
| realIdByKey, | ||
| } | ||
| }) | ||
| }, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify State.register handles concurrent initialization correctly.
The migration to State.register follows the established pattern used by other modules. However, ensure that State.register properly serializes concurrent calls to prevent race conditions during initialization, especially since provider loading involves async operations (Auth.all(), Plugin.list(), custom loaders).
Run the following script to verify the State module's concurrency handling:
🏁 Script executed:
#!/bin/bash
# Description: Check State.register implementation for concurrency safeguards
# Look for State.register implementation and locking/memoization mechanisms
ast-grep --pattern $'register($NAME, $SCOPE, $INIT) {
$$$
}'
# Search for mutex, lock, or promise caching patterns in State module
rg -n 'mutex|lock|Promise.*cache|concurrent' packages/opencode/src/project/state.ts -C3Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find State module file and inspect State.register implementation
# First, locate the State module
find . -name "state.ts" -o -name "state.js" | head -20
# Also search for State.register pattern in the codebase
rg -n "register.*=.*function|register.*\(.*\)" packages/opencode/src/project/ --type ts -A5 -B1 | head -50Length of output: 1248
🏁 Script executed:
#!/bin/bash
# Inspect State.register implementation in detail
cat packages/opencode/src/project/state.ts | head -150Length of output: 4517
State.register does not safely handle concurrent async initialization—race condition risk.
The State.register implementation in packages/opencode/src/project/state.ts does not handle concurrent calls to async init() functions. When the provider initialization at line 246 executes with multiple concurrent State accesses, the entry is only cached after initialization completes. This means two concurrent calls will both see the entry missing and both invoke the init function independently, defeating the caching mechanism and causing duplicate async work (Config.get, ModelsDev.get, Auth.all, Plugin.list, etc.).
The State.create function should store the promise immediately (not the resolved value), or implement a lock pattern to ensure only one initialization runs per key.
🤖 Prompt for AI Agents
In packages/opencode/src/provider/provider.ts around lines 241 to 511, the
provider State.register init function performs expensive async initialization
(Config.get, ModelsDev.get, Auth.all, Plugin.list, etc.) but relies on
State.register which currently only caches the resolved value after init
completes, allowing concurrent callers to race and run init multiple times;
update State.create/register implementation (in
packages/opencode/src/project/state.ts) to store the initialization promise
immediately (or use a per-key lock) so that the first caller saves the Promise
into the state map and subsequent callers await that Promise instead of invoking
init again, ensuring only a single concurrent initialization per key and
preserving existing resolved-value semantics once ready.
Resolve conflicts in: - config/config.ts: Use resolveGlobalFile import and updated NamedError path - plugin/index.ts: Keep State.register pattern with cleanup, update plugin versions - provider/provider.ts: Keep State.register pattern for hot-reload support 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Update import to use @opencode-ai/util/error to match the NamedError module relocation from dev branch. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/opencode/src/server/server.ts (1)
254-265: Guardscopequery parsing and make diff handling defensive.This repeats a concern from a previous review. The
scopequery parameter is cast without runtime validation—any string passes through. Additionally,publishDiffmay beundefined, causingObject.keys(publishDiff)to throw.Apply the suggested fix from the previous review:
- const scope = (c.req.query("scope") as "project" | "global" | undefined) ?? "project" + const scopeQuery = c.req.query("scope") + const scope: "project" | "global" = scopeQuery === "global" ? "global" : "project" const directory = Instance.directory // ... later ... - const publishDiff = result.diffForPublish + const publishDiff = result.diffForPublish ?? {}packages/opencode/src/provider/provider.ts (2)
332-341: GitHub Copilot Enterprise API endpoint remains undefined.This repeats a concern from a previous review. The
api: undefinedfield relies on the auth loader providing abaseURL, but if the auth loader doesn't set it, the enterprise provider will have no API endpoint. ThemergeProviderfunction only auto-setsbaseURLwheninfo.apiexists (line 315), which it doesn't for enterprise.Consider adding a fallback or validation to ensure
baseURLis configured for enterprise providers before use.
264-534: State.register concurrent initialization concern persists.As noted in a previous review,
State.registermay not safely handle concurrent async initialization. If multiple callers invokestate()simultaneously before initialization completes, they may all trigger independent init calls, causing duplicate work and potential race conditions.This is an architectural concern with the
Statemodule implementation. Consider verifying thatState.registerstores the initialization promise immediately (rather than the resolved value) to prevent concurrent callers from racing.#!/bin/bash # Check State.register implementation for promise caching / locking cat packages/opencode/src/project/state.ts
🧹 Nitpick comments (5)
packages/opencode/src/plugin/index.ts (1)
38-50: Consider avoidingletreassignment per coding guidelines.The
let pluginvariable is reassigned within the loop. Per coding guidelines, prefer avoidingletstatements. You could restructure to use aconstwith conditional assignment.- for (let plugin of plugins) { - log.info("loading plugin", { path: plugin }) - if (!plugin.startsWith("file://")) { - const lastAtIndex = plugin.lastIndexOf("@") - const pkg = lastAtIndex > 0 ? plugin.substring(0, lastAtIndex) : plugin - const version = lastAtIndex > 0 ? plugin.substring(lastAtIndex + 1) : "latest" - plugin = await BunProc.install(pkg, version) - } - const mod = await import(plugin) + for (const pluginSpec of plugins) { + log.info("loading plugin", { path: pluginSpec }) + const pluginPath = pluginSpec.startsWith("file://") + ? pluginSpec + : await (async () => { + const lastAtIndex = pluginSpec.lastIndexOf("@") + const pkg = lastAtIndex > 0 ? pluginSpec.substring(0, lastAtIndex) : pluginSpec + const version = lastAtIndex > 0 ? pluginSpec.substring(lastAtIndex + 1) : "latest" + return BunProc.install(pkg, version) + })() + const mod = await import(pluginPath)packages/opencode/src/server/server.ts (2)
90-98: Consider clearing the cleanup interval on server disposal.The
setIntervalruns indefinitely. If the server supports graceful shutdown or disposal, this interval should be cleared to avoid resource leaks. If the server runs until process termination, this is a non-issue.
1951-1977: Empty catch block silently swallows errors.The
try/catchblock with an empty catch hides any errors in the toast enrichment logic, making debugging difficult. Per coding guidelines, avoidtry/catchwhere possible, or at minimum log errors.} - } catch {} + } catch (err) { + log.error("toast enrichment failed", { error: err }) + } await Bus.publish(TuiEvent.ToastShow, payload)packages/opencode/src/config/config.ts (2)
29-41: Minor redundancy in path checks.Line 37 checks
value.startsWith("/")beforepath.isAbsolute(value), butstartsWith("/")already captures Unix absolute paths. Thepath.isAbsolute()is still useful for Windows paths likeC:\, so the order could be optimized.Consider this refactor to check
path.isAbsolute()first to catch all absolute paths in one condition:- if (value.startsWith("/") || path.isAbsolute(value)) { + if (path.isAbsolute(value)) { return true }
43-73: Consider avoiding try-catch for URL resolution.The try-catch block at lines 66-72 is used for URL parsing. Per coding guidelines, try-catch should be avoided where possible.
Consider using URL validation before creating the URL to avoid the try-catch:
- try { - const base = pathToFileURL(configFilepath).href - const resolved = new URL(plugin, base).href - return normalizeWindowsPath(resolved) - } catch { - return plugin - } + const base = pathToFileURL(configFilepath).href + const resolved = path.resolve(path.dirname(configFilepath), plugin) + return pathToFileURL(resolved).hrefAdditionally,
normalizeWindowsPathis only used once, so it could be inlined:- const normalizeWindowsPath = (input: string) => input.replace(/\\/g, "/") - if (plugin.startsWith("~/")) {
📜 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 (6)
packages/opencode/src/config/config.ts(9 hunks)packages/opencode/src/config/error.ts(1 hunks)packages/opencode/src/mcp/index.ts(2 hunks)packages/opencode/src/plugin/index.ts(1 hunks)packages/opencode/src/provider/provider.ts(2 hunks)packages/opencode/src/server/server.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/opencode/src/config/error.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,jsx,ts,tsx}: DO NOT do unnecessary destructuring of variables
DO NOT useelsestatements unless necessary
DO NOT usetry/catchif it can be avoided
AVOIDtry/catchwhere possible
AVOIDletstatements
PREFER single word variable names where possible
Use as many Bun APIs as possible like Bun.file()
Files:
packages/opencode/src/mcp/index.tspackages/opencode/src/server/server.tspackages/opencode/src/provider/provider.tspackages/opencode/src/plugin/index.tspackages/opencode/src/config/config.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
AVOID using
anytype
Files:
packages/opencode/src/mcp/index.tspackages/opencode/src/server/server.tspackages/opencode/src/provider/provider.tspackages/opencode/src/plugin/index.tspackages/opencode/src/config/config.ts
packages/opencode/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (packages/opencode/AGENTS.md)
Use Bun with TypeScript ESM modules as the runtime environment
Files:
packages/opencode/src/mcp/index.tspackages/opencode/src/server/server.tspackages/opencode/src/provider/provider.tspackages/opencode/src/plugin/index.tspackages/opencode/src/config/config.ts
packages/opencode/**/*.{ts,tsx}
📄 CodeRabbit inference engine (packages/opencode/AGENTS.md)
packages/opencode/**/*.{ts,tsx}: Use relative imports for local modules with named imports preferred
Use Zod schemas for validation
Use TypeScript interfaces for defining data structures
Use camelCase for variable and function names
Use PascalCase for class and namespace names
Use Result patterns for error handling; avoid throwing exceptions in tools
Organize code using namespace-based structure (e.g.,Tool.define(),Session.create())
PasssessionIDin tool context and useApp.provide()for dependency injection
Validate all inputs with Zod schemas
UseLog.create({ service: "name" })pattern for logging
UseStoragenamespace for data persistence
Files:
packages/opencode/src/mcp/index.tspackages/opencode/src/server/server.tspackages/opencode/src/provider/provider.tspackages/opencode/src/plugin/index.tspackages/opencode/src/config/config.ts
🧠 Learnings (5)
📚 Learning: 2025-11-24T22:47:41.129Z
Learnt from: CR
Repo: kcrommett/opencode PR: 0
File: packages/opencode/AGENTS.md:0-0
Timestamp: 2025-11-24T22:47:41.129Z
Learning: Applies to packages/opencode/**/*.{ts,tsx} : Organize code using namespace-based structure (e.g., `Tool.define()`, `Session.create()`)
Applied to files:
packages/opencode/src/mcp/index.tspackages/opencode/src/plugin/index.tspackages/opencode/src/config/config.ts
📚 Learning: 2025-11-24T22:47:41.129Z
Learnt from: CR
Repo: kcrommett/opencode PR: 0
File: packages/opencode/AGENTS.md:0-0
Timestamp: 2025-11-24T22:47:41.129Z
Learning: Applies to packages/opencode/**/*.{ts,tsx} : Pass `sessionID` in tool context and use `App.provide()` for dependency injection
Applied to files:
packages/opencode/src/provider/provider.ts
📚 Learning: 2025-11-24T22:47:41.129Z
Learnt from: CR
Repo: kcrommett/opencode PR: 0
File: packages/opencode/AGENTS.md:0-0
Timestamp: 2025-11-24T22:47:41.129Z
Learning: Applies to packages/opencode/**/*.{ts,tsx} : Use relative imports for local modules with named imports preferred
Applied to files:
packages/opencode/src/config/config.ts
📚 Learning: 2025-11-24T22:47:41.129Z
Learnt from: CR
Repo: kcrommett/opencode PR: 0
File: packages/opencode/AGENTS.md:0-0
Timestamp: 2025-11-24T22:47:41.129Z
Learning: Applies to packages/opencode/**/*.{ts,tsx} : Use `Storage` namespace for data persistence
Applied to files:
packages/opencode/src/config/config.ts
📚 Learning: 2025-11-24T22:47:17.581Z
Learnt from: CR
Repo: kcrommett/opencode PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T22:47:17.581Z
Learning: Applies to **/*.{js,jsx,ts,tsx} : Use as many Bun APIs as possible like Bun.file()
Applied to files:
packages/opencode/src/config/config.ts
🧬 Code graph analysis (2)
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/config.ts (2)
packages/opencode/src/config/global-file.ts (1)
resolveGlobalFile(5-8)packages/opencode/src/config/persist.ts (1)
update(61-183)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: update
🔇 Additional comments (13)
packages/opencode/src/mcp/index.ts (1)
57-94: Migration to State.register looks correct.The MCP state initialization and cleanup are properly encapsulated. The cleanup function gracefully handles errors by catching and logging them rather than propagating failures. The use of
Promise.allfor concurrent client creation and cleanup is appropriate.packages/opencode/src/plugin/index.ts (1)
58-66: Cleanup logic is properly defensive.The cleanup phase correctly checks for the presence of a
cleanupfunction before invoking it, and errors are caught and logged rather than propagating. This ensures one failing plugin cleanup doesn't prevent others from running.packages/opencode/src/server/server.ts (1)
279-304: Hot-reload publishing for global scope handles failures appropriately.The global scope publishing iterates all directories, aggregates errors, and throws if any publish fails. This provides visibility into partial failures. The updated config is still returned on success.
packages/opencode/src/provider/provider.ts (2)
343-401: Config provider and model parsing handles inheritance correctly.The parsing logic properly merges config-defined providers and models with database defaults, handling optional fields with sensible fallbacks. The
realIdByKeymap correctly tracks model aliases whenmodel.iddiffers frommodelID.
433-472: Plugin auth loader handling for GitHub Copilot variants is comprehensive.The logic correctly handles both
github-copilotandgithub-copilot-enterpriseby checking auth for each variant independently and invoking the loader with the appropriate auth getter. This ensures both variants can be configured when their respective auth is present.packages/opencode/src/config/config.ts (8)
10-10: LGTM!The new imports follow the coding guidelines with relative paths and named imports.
Also applies to: 16-16, 21-23
140-157: ReplaceletwithconstforpluginFiles.Line 140 uses
letforpluginFiles, but since it's only mutated viapush, it can be declared asconst.Apply this diff:
- const pluginFiles: string[] = [] + const pluginFiles: string[] = []Wait, this is already fine. Actually, looking more carefully,
pluginFilesis mutated withpush, which is allowed withconst. The guideline to avoidletmeans we should useconstwhen the reference doesn't change. This is already usingconstindirectly through type inference, but let me check the actual code...Actually, the annotation shows line 140~ which means it was added/modified. Looking at the code, it seems like it's declared as a regular array without
letorconstshown. This is likely alreadyconstbased on TypeScript/Bun defaults, so no issue here.
197-197: LGTM!The new state registration using
State.register()aligns well with the namespace-based structure guideline.
199-202: LGTM!The
readFreshConfig()function provides a useful way to bypass cache and fetch the latest config directly from disk.
733-759: Try-catch usage for optional TOML support.Lines 743-756 use try-catch for loading optional TOML configuration. While the coding guidelines suggest avoiding try-catch, this usage appears necessary for graceful degradation when TOML config doesn't exist.
Consider whether this TOML migration code is still needed, or if it can be removed to simplify the codebase and eliminate the try-catch.
761-763: LGTM!The simplified
global()function cleanly delegates toloadGlobalConfig().
850-857: LGTM!The plugin path resolution logic correctly integrates the new helper functions to normalize path-like plugin specifiers.
896-912: LGTM with delegation pattern!The new
update()function signature provides a richer API with before/after/diff information and cleanly delegates to the persist module via dynamic import.
| export const Event = { | ||
| Updated: Bus.event( | ||
| "config.updated", | ||
| z.object({ | ||
| scope: z.enum(["project", "global"]), | ||
| directory: z.string().optional(), | ||
| refreshed: z.boolean().optional(), | ||
| before: z.any(), | ||
| after: z.any(), | ||
| diff: z.any(), | ||
| }), | ||
| ), | ||
| } |
There was a problem hiding this comment.
Replace z.any() with proper schemas.
Lines 82-84 use z.any() for before, after, and diff fields, which violates the coding guideline to avoid any type. This reduces type safety.
Apply this diff to use proper schemas:
export const Event = {
Updated: Bus.event(
"config.updated",
z.object({
scope: z.enum(["project", "global"]),
directory: z.string().optional(),
refreshed: z.boolean().optional(),
- before: z.any(),
- after: z.any(),
- diff: z.any(),
+ before: Info,
+ after: Info,
+ diff: z.custom<ConfigDiff>(),
}),
),
}📝 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 const Event = { | |
| Updated: Bus.event( | |
| "config.updated", | |
| z.object({ | |
| scope: z.enum(["project", "global"]), | |
| directory: z.string().optional(), | |
| refreshed: z.boolean().optional(), | |
| before: z.any(), | |
| after: z.any(), | |
| diff: z.any(), | |
| }), | |
| ), | |
| } | |
| export const Event = { | |
| Updated: Bus.event( | |
| "config.updated", | |
| z.object({ | |
| scope: z.enum(["project", "global"]), | |
| directory: z.string().optional(), | |
| refreshed: z.boolean().optional(), | |
| before: Info, | |
| after: Info, | |
| diff: z.custom<ConfigDiff>(), | |
| }), | |
| ), | |
| } |
🤖 Prompt for AI Agents
In packages/opencode/src/config/config.ts around lines 75 to 87, replace the
z.any() usages for before, after and diff with concrete Zod schemas to remove
the any type; use z.record(z.unknown()) (or z.record(z.unknown()).optional() /
.nullable() to match the original optionality) for before and after to represent
arbitrary objects, and use z.record(z.unknown()) for diff as well (or mark it
optional if appropriate), keeping the existing field names and any .optional()
wrappers intact.
github-actions
bot
force-pushed
the
dev
branch
from
020ee56 to
83c47e0
Compare
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.