fix: make models refresh publish safely#211
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 7 minutes and 8 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughImplements version-tracked catalog publishing for model providers with atomic file operations, validation schemas, and state versioning. Introduces provider state invalidation to reload cached instance state when model catalog versions change, and adds comprehensive test coverage for refresh behaviors. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Provider
participant ModelsDev
participant FileSystem
Client->>Provider: list() / getModel() / etc.
Provider->>Provider: currentState()
Provider->>ModelsDev: version()
ModelsDev-->>Provider: current catalogVersion
alt Version mismatch detected
Provider->>ModelsDev: getWithVersion()
ModelsDev->>FileSystem: read catalog
ModelsDev->>ModelsDev: validate schemas
ModelsDev->>ModelsDev: invoke fromModelsDevProvider
ModelsDev-->>Provider: catalog + new version
Provider->>Provider: reload InstanceState
end
Provider-->>Client: providers from currentState
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/provider/models.ts`:
- Around line 292-305: The getWithVersion function currently has an unbounded
while(true) retry that could loop forever if version keeps changing; add a retry
limit (e.g., MAX_RETRIES constant) inside getWithVersion, increment a counter
each loop, and if the counter exceeds the limit, call Data.reset() (as currently
done on mismatch) and throw an explicit error or return a clear failure result
so callers can handle it; keep the existing behavior (compare before/after
version, call withPawWorkProviders) for successful attempts and ensure the
counter is reset or local to the function so retries are bounded.
- Around line 113-179: PublishModel and PublishProvider largely duplicate Model
and Provider with only small optionality differences; extract shared field
definitions into a common base and build strict and publish variants from it to
avoid drift. Create a BaseModelFields object (or BaseModel zod partial) for
repeated keys and use Model = z.object({ ...BaseModelFields, release_date:
z.string(), /* required fields */ }) and PublishModel = z.object({
...BaseModelFields, release_date: z.string().optional(), /* optional overrides
*/ }).passthrough(); do the same for Provider/PublishProvider (reuse
BaseProviderFields) and replace duplicated inline definitions in
PublishModel/PublishProvider with spreads of the shared bases and only change
optionality where needed.
In `@packages/opencode/src/provider/provider.ts`:
- Around line 1416-1421: The current Provider.currentState implementation races:
two callers can both see existing.modelsVersion !== ModelsDev.version() and both
call InstanceState.invalidate(state) then rebuild, wasting CPU; update
Provider.currentState to serialize invalidate-and-reload (e.g., replace the
manual check+invalidate with an Effect.cached wrapper around the reload logic or
add a Ref<boolean> guard) so only the first caller performs
InstanceState.invalidate(state) and subsequent callers await the cached reload
via InstanceState.get(state); touch the function named Provider.currentState and
calls to InstanceState.get / InstanceState.invalidate and reference
ModelsDev.version when implementing the cache or guard.
In `@packages/opencode/test/provider/models-refresh.test.ts`:
- Around line 20-27: The tests leave the module-private catalogVersion
incremented across runs; add a test-only reset and call it in the afterEach to
avoid cumulative versioning. Update the module that defines
catalogVersion/version() (the same module providing version()) to export a
resetCatalogVersion() or resetForTests() function that sets catalogVersion back
to its initial value, then call that new reset function from the test afterEach
alongside ModelsDev.Data.reset() so each test starts with a clean
catalogVersion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9a844503-b6c1-4830-ab08-76275996ec1e
📒 Files selected for processing (3)
packages/opencode/src/provider/models.tspackages/opencode/src/provider/provider.tspackages/opencode/test/provider/models-refresh.test.ts
📜 Review details
⏰ 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). (9)
- GitHub Check: unit-windows-opencode-server-tools
- GitHub Check: unit-windows-app
- GitHub Check: typecheck
- GitHub Check: unit-desktop
- GitHub Check: unit-app
- GitHub Check: unit-opencode
- GitHub Check: smoke-macos-arm64
- GitHub Check: analyze-js-ts
- GitHub Check: e2e-artifacts
🧰 Additional context used
📓 Path-based instructions (2)
packages/opencode/**/*.ts
📄 CodeRabbit inference engine (packages/opencode/AGENTS.md)
packages/opencode/**/*.ts: UseEffect.gen(function* () { ... })for Effect composition
UseEffect.fn("Domain.method")for named/traced effects andEffect.fnUntracedfor internal helpers; these accept pipeable operators as extra arguments to avoid unnecessary outer.pipe()wrappers
UseEffect.callbackfor callback-based APIs
PreferDateTime.nowAsDateovernew Date(yield* Clock.currentTimeMillis)when you need aDatein Effect code
UseSchema.Classfor multi-field data in Effect schemas
Use branded schemas (Schema.brand) for single-value types in Effect
UseSchema.TaggedErrorClassfor typed errors in Effect schemas
UseSchema.Defectinstead ofunknownfor defect-like causes in Effect code
InEffect.gen/Effect.fn, preferyield* new MyError(...)overyield* Effect.fail(new MyError(...))for direct early-failure branches
UsemakeRuntimefromsrc/effect/run-service.tsfor all services; it returns{ runPromise, runFork, runCallback }backed by a sharedmemoMapthat deduplicates layers
UseInstanceStatefromsrc/effect/instance-state.tsfor per-directory or per-project state that needs per-instance cleanup; do work directly in theInstanceState.makeclosure whereScopedCachehandles run-once semantics
UseEffect.addFinalizerorEffect.acquireReleaseinside theInstanceState.makeclosure for cleanup (subscriptions, process teardown, etc.)
UseEffect.forkScopedinside theInstanceState.makeclosure for background stream consumers — the fiber is interrupted when the instance is disposed
PreferFileSystem.FileSysteminstead of rawfs/promisesfor effectful file I/O in Effect services
PreferChildProcessSpawner.ChildProcessSpawnerwithChildProcess.make(...)instead of custom process wrappers in Effect services
PreferHttpClient.HttpClientinstead of rawfetchin Effect services
PreferPath.Path,Config,Clock, andDateTimeservices when those concerns are already inside Effect code
For backgroun...
Files:
packages/opencode/src/provider/provider.tspackages/opencode/src/provider/models.tspackages/opencode/test/provider/models-refresh.test.ts
packages/opencode/test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (packages/opencode/test/AGENTS.md)
packages/opencode/test/**/*.test.{ts,tsx}: Use thetmpdirfunction fromfixture/fixture.tsto create temporary directories for tests with automatic cleanup. Useawait usingsyntax to ensure automatic cleanup when the variable goes out of scope.
When using thetmpdirfunction with git repository support, pass thegit: trueoption to initialize a git repo with a root commit.
Use theconfigoption intmpdirto write anopencode.jsonconfig file during test setup by passing a partial Config.Info object.
Use theinitoption intmpdirto define custom setup functions that can return extra data accessible viatmp.extra, and use thedisposeoption for custom cleanup logic.
UsetestEffect(...)fromtest/lib/effect.tsfor tests that exercise Effect services or Effect-based workflows.
Useit.effect(...)when the test should run withTestClockandTestConsole. Useit.live(...)when the test depends on real time, filesystem mtimes, child processes, git, locks, or other live OS behavior.
Prefer Effect-aware helpers fromfixture/fixture.tsover building manual runtimes in tests: usetmpdirScoped()for scoped temp directories,provideInstance(dir)(effect)for low-level binding without directory creation,provideTmpdirInstance(...)for single temp instance binding, orprovideTmpdirServer(...)for tests that also need the test LLM server.
Defineconst it = testEffect(...)near the top of the test file and keep the test body insideEffect.gen(function* () { ... }). Yield services directly withyield* MyService.Serviceoryield* MyTool.
Avoid customManagedRuntime,attach(...), or ad hocrun(...)wrappers in Effect tests whentestEffect(...)already provides the runtime.
When a test needs instance-local state, preferprovideTmpdirInstance(...)orprovideInstance(...)over manualInstance.provide(...)inside Promise-style tests.
Files:
packages/opencode/test/provider/models-refresh.test.ts
🧠 Learnings (16)
📚 Learning: 2026-04-22T09:32:54.556Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 126
File: packages/opencode/test/provider/provider.test.ts:64-85
Timestamp: 2026-04-22T09:32:54.556Z
Learning: In `packages/opencode/test/provider/provider.test.ts`, the file intentionally uses AppRuntime-based async helpers (`run`, `list`, `getProvider`, etc.) rather than `testEffect(...)` for all tests. Converting individual tests to `testEffect` while leaving the rest on the async pattern would create internal inconsistency. A full harness migration of this file is the right approach if the pattern needs to change, but that should be a separate PR.
Applied to files:
packages/opencode/src/provider/provider.ts
📚 Learning: 2026-04-20T14:36:21.288Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:21.288Z
Learning: Applies to packages/opencode/**/*.ts : Use `InstanceState` from `src/effect/instance-state.ts` for per-directory or per-project state that needs per-instance cleanup; do work directly in the `InstanceState.make` closure where `ScopedCache` handles run-once semantics
Applied to files:
packages/opencode/src/provider/provider.ts
📚 Learning: 2026-04-21T16:00:44.910Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 102
File: packages/opencode/src/config/model-id.ts:9-11
Timestamp: 2026-04-21T16:00:44.910Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/config/model-id.ts`), `ConfigModelID` uses a regex that requires a non-empty provider segment and a non-empty model segment separated by `/`, but intentionally allows additional slashes within the model segment. This is because `Provider.parseModel` supports model IDs like `synthetic/hf:zai-org/GLM-4.7`. Do not flag the permissive slash handling as a bug.
Applied to files:
packages/opencode/src/provider/provider.tspackages/opencode/src/provider/models.ts
📚 Learning: 2026-04-20T14:36:21.288Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:21.288Z
Learning: Applies to packages/opencode/**/*.ts : Use `Effect.gen(function* () { ... })` for Effect composition
Applied to files:
packages/opencode/src/provider/provider.ts
📚 Learning: 2026-04-20T14:36:31.032Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.032Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Define `const it = testEffect(...)` near the top of the test file and keep the test body inside `Effect.gen(function* () { ... })`. Yield services directly with `yield* MyService.Service` or `yield* MyTool`.
Applied to files:
packages/opencode/src/provider/provider.tspackages/opencode/test/provider/models-refresh.test.ts
📚 Learning: 2026-04-23T08:51:04.230Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 186
File: packages/opencode/test/plugin/workspace-adaptor.test.ts:139-144
Timestamp: 2026-04-23T08:51:04.230Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/util/filesystem.ts`), the `Filesystem` utility does NOT expose a `remove` or `unlink` helper. The established repository pattern for auth.json teardown in tests (e.g. `provider.test.ts`, `amazon-bedrock.test.ts`, `workspace-adaptor.test.ts`) is to combine `Filesystem.write` with `node:fs/promises unlink`. Do not flag this mixed usage as inconsistent — it is the correct and intentional pattern.
Applied to files:
packages/opencode/src/provider/models.ts
📚 Learning: 2026-04-22T09:32:52.806Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 126
File: packages/opencode/test/provider/provider.test.ts:64-85
Timestamp: 2026-04-22T09:32:52.806Z
Learning: For tests under `packages/opencode/test/provider/` (e.g., `*.test.ts`), keep the test harness pattern consistent across the whole file: if the file intentionally uses AppRuntime-based async helpers (`run`, `list`, `getProvider`, etc.) instead of `testEffect(...)`, do not partially convert individual tests to `testEffect` while leaving others on the async helper pattern. If the harness pattern must change, migrate the entire file as a cohesive harness migration (ideally as a separate PR) to avoid internal inconsistency.
Applied to files:
packages/opencode/test/provider/models-refresh.test.ts
📚 Learning: 2026-04-20T14:36:31.032Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.032Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Prefer Effect-aware helpers from `fixture/fixture.ts` over building manual runtimes in tests: use `tmpdirScoped()` for scoped temp directories, `provideInstance(dir)(effect)` for low-level binding without directory creation, `provideTmpdirInstance(...)` for single temp instance binding, or `provideTmpdirServer(...)` for tests that also need the test LLM server.
Applied to files:
packages/opencode/test/provider/models-refresh.test.ts
📚 Learning: 2026-04-20T14:36:31.032Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.032Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : When a test needs instance-local state, prefer `provideTmpdirInstance(...)` or `provideInstance(...)` over manual `Instance.provide(...)` inside Promise-style tests.
Applied to files:
packages/opencode/test/provider/models-refresh.test.ts
📚 Learning: 2026-04-20T14:36:31.032Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.032Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Use the `config` option in `tmpdir` to write an `opencode.json` config file during test setup by passing a partial Config.Info object.
Applied to files:
packages/opencode/test/provider/models-refresh.test.ts
📚 Learning: 2026-04-20T14:36:31.032Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.032Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Use the `tmpdir` function from `fixture/fixture.ts` to create temporary directories for tests with automatic cleanup. Use `await using` syntax to ensure automatic cleanup when the variable goes out of scope.
Applied to files:
packages/opencode/test/provider/models-refresh.test.ts
📚 Learning: 2026-04-20T14:36:31.032Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.032Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Use the `init` option in `tmpdir` to define custom setup functions that can return extra data accessible via `tmp.extra`, and use the `dispose` option for custom cleanup logic.
Applied to files:
packages/opencode/test/provider/models-refresh.test.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/packages/app/src/testing/**/*.ts : Test-only hooks must be inert unless explicitly enabled and should not add normal-runtime listeners, reactive subscriptions, or per-update allocations
Applied to files:
packages/opencode/test/provider/models-refresh.test.ts
📚 Learning: 2026-04-23T08:51:00.819Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 186
File: packages/opencode/test/plugin/workspace-adaptor.test.ts:139-144
Timestamp: 2026-04-23T08:51:00.819Z
Learning: For pawwork tests under packages/opencode/test/**, auth.json teardown may intentionally combine `Filesystem.write` (from `packages/opencode/src/util/filesystem.ts`) with `node:fs/promises` `unlink` for cleanup. Do not flag this as inconsistent style; it is the established/intentional pattern because `Filesystem` does not provide a `remove`/`unlink` helper.
Applied to files:
packages/opencode/test/provider/models-refresh.test.ts
📚 Learning: 2026-04-20T14:36:31.032Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.032Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Use `testEffect(...)` from `test/lib/effect.ts` for tests that exercise Effect services or Effect-based workflows.
Applied to files:
packages/opencode/test/provider/models-refresh.test.ts
📚 Learning: 2026-04-20T17:03:40.214Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 73
File: packages/opencode/src/cli/cmd/tui/context/sync.tsx:486-489
Timestamp: 2026-04-20T17:03:40.214Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/cli/cmd/tui/context/sync.tsx`), `sync.ready` returning `true` when `process.env.OPENCODE_FAST_BOOT` is set is intentional. The plugin-facing data properties `state.config` (initialized to `{}`) and `state.provider` (initialized to `[]`) expose safe-empty defaults, so they are safe to access before bootstrap completes. Do not flag these as needing null-guards or conditional patterns to match `vcs` — the difference is intentional because `vcs` starts as `undefined` while the others have initialized defaults. Changing this would alter the plugin API contract without a concrete failing case.
Applied to files:
packages/opencode/test/provider/models-refresh.test.ts
🔇 Additional comments (12)
packages/opencode/src/provider/provider.ts (3)
1609-1615: LGTM!Good defensive check. After a catalog refresh that removes a provider, this now throws a structured
ModelNotFoundErrorinstead of crashing on undefined access inresolveSDK.
1177-1184: LGTM!The access pattern correctly reflects the new
getWithVersion()return shape withprovidersandversionfields.
963-970: LGTM!The
modelsVersionfield enables tracking when the cached state was built, allowingcurrentState()to detect when a refresh has occurred.packages/opencode/src/provider/models.ts (4)
227-236: LGTM!Correct atomic write pattern: temp file with unique suffix → write → rename. The cleanup in catch with
force: truehandles the case where the temp file wasn't created. This prevents partial writes from corrupting the cache.
307-330: Version increment logic is correct but subtle.The multiple
catalogVersion++calls serve different purposes:
- Lines 309-310: Override path changed externally, force provider rebuild
- Lines 314-315: Fresh cache detected before lock, signal potential external change
- Lines 319-320: Fresh cache inside lock (another process updated), signal change
- Line 324:
publishCandidateincrements after successful publishThis is correct—each increment signals "providers may need to rebuild." Consider adding a brief comment explaining the invariant that any path triggering
Data.reset()should also incrementcatalogVersion.
219-225: Dynamic import avoids circular dependency at module load time.The dynamic
import("./provider")is intentional to break the circular reference (provider.ts statically imports models.ts). The validation ensures catalogs can be transformed to runtime providers before committing to disk.
269-283: LGTM!Good separation of concerns: validation failure (line 280) logs and returns
{}(no valid data to serve), while write failure (line 276) logs but still returns the valid catalog. This ensures a valid fetched catalog is usable even if cache persistence fails.packages/opencode/test/provider/models-refresh.test.ts (5)
77-86: LGTM!
withTestInstancecorrectly usestmpdirandInstance.provideto establish isolated test context, consistent with the established pattern in provider tests (per learnings).
180-183: Type assertion to mutateFilesystem.writeis acceptable for test mocking.The cast
(Filesystem as { write: typeof Filesystem.write }).writeworks around the readonly property. This is a standard pattern for test mocks. The cleanup inafterEachrestores the original.
261-274: LGTM!Excellent test coverage for the provider-removal edge case. This validates that
getLanguagethrowsModelNotFoundErrorwhen the provider is removed from the catalog during refresh, rather than crashing on undefined access.
276-298: LGTM!Good integration test verifying that the overlay pattern (merging
ModelsDev.get()withProvider.list()) correctly reflects refreshed catalog state.
91-101: LGTM!Good baseline test establishing the happy path: valid candidate → cache updated → version incremented.
There was a problem hiding this comment.
Code Review
This pull request implements a versioning system for the models catalog to ensure the runtime state remains synchronized with the underlying data. Key changes include the introduction of Zod schemas for catalog validation, atomic file operations for safe updates, and a mechanism in the provider layer to invalidate stale state based on catalog versioning. Review feedback suggests removing a redundant reset call in the versioned getter and enhancing error reporting in getLanguage by adding provider suggestions when a lookup fails.
d11aea5 to
af6ea3c
Compare
Summary
Make
models.devrefresh safer and visible to already-running provider reads. Refresh now validates candidate catalogs before publishing, writes cache updates through a temporary file plus rename, and tracks a catalog version so provider state rebuilds after successful refreshes or cache source resets.Why
PawWork could fetch a newer
models.devcatalog but keep serving stale provider state in the same running process. A failed or malformed refresh could also risk replacing a usable local cache. This PR makes refresh failure non-destructive while allowing newly published models to appear without requiring a restart.Related Issue
No linked issue. This came from local investigation into stale
models.devrefresh behavior.How To Verify
Manual smoke, using temporary
XDG_*directories so the real PawWork cache is not touched:cd packages/opencode XDG_CACHE_HOME=/tmp/pawwork-models-smoke/cache XDG_DATA_HOME=/tmp/pawwork-models-smoke/data XDG_CONFIG_HOME=/tmp/pawwork-models-smoke/config XDG_STATE_HOME=/tmp/pawwork-models-smoke/state PAWWORK_RUNTIME_NAMESPACE=pawwork-models-refresh-smoke MOONSHOT_API_KEY=dummy bun src/index.ts models moonshotai-cn --refreshObserved
moonshotai-cn/kimi-k2.6after refresh.Screenshots or Recordings
Not applicable. No visible UI changes.
Checklist
dev, and my PR title and commit messages use Conventional Commits in EnglishSummary by CodeRabbit
New Features
Bug Fixes
Tests