Fix OpenClaw model picker sync#206
Conversation
📝 WalkthroughWalkthroughThe PR adds automatic postinstall setup, broadens OpenClaw discovery, synchronizes model caches and configuration ordering, patches installed OpenClaw bundles, removes advertised alias entries, updates Gemini and top-model ordering coverage, and narrows plugin cleanup. ChangesOpenClaw model catalog and synchronization
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant npm
participant postinstall.mjs
participant dist/cli.js
participant cmdSetup
participant OpenClaw
participant ConfigFiles
npm->>postinstall.mjs: run postinstall
postinstall.mjs->>dist/cli.js: execute setup
dist/cli.js->>cmdSetup: invoke cmdSetup
cmdSetup->>OpenClaw: discover installation
cmdSetup->>ConfigFiles: synchronize models and cache
cmdSetup->>OpenClaw: patch installed bundles
OpenClaw-->>cmdSetup: return setup status
cmdSetup-->>postinstall.mjs: exit status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/cli.ts (2)
631-659: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winModel-cache write isn't atomic, and the agent path is hardcoded to "main".
injectModelsConfigin src/index.ts explicitly uses a tmp-file + rename pattern to avoid corruptingopenclaw.jsonon crash; this cache write uses a plainwriteFileSync, so a crash mid-write could corrupt~/.openclaw/agents/main/agent/models.json. Separately, the path assumes a single agent named"main"— worth confirming this generalizes across OpenClaw installs/agent configurations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli.ts` around lines 631 - 659, Update the model-cache sync in the CLI flow to write models.json atomically using the existing temporary-file-and-rename approach from injectModelsConfig, preserving the current JSON content and logging behavior. Replace the hardcoded "main" agent segment with the appropriate configured or discovered OpenClaw agent path so installs using another agent are handled.
678-698: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffSilent no-op if OpenClaw's bundle source drifts from the exact expected strings.
The
.includes()gate can match while the follow-up exact.replace()target string doesn't, in which case no patch is applied and no warning is logged — this build will look "successful" while quietly failing to apply the model-picker-ordering fix on any OpenClaw release with minor formatting/variable-name changes.Can you confirm with OpenClaw upstream whether the
buildModelsProviderDatainternals (visibleCatalog/byProvider/normalizeProviderId) are stable across2026.5.xpatch releases, or if there's a more stable extension point (e.g. a plugin hook) instead of patching bundled dist output?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli.ts` around lines 678 - 698, Make the buildModelsProviderData patch fail loudly when the upstream bundle does not match: track whether each required replacement in the source patching block actually changed the source, and emit a warning or error and avoid reporting “model picker ordering” as applied when any target is missing. Prefer a stable upstream extension point or plugin hook if available; otherwise anchor matching to verified OpenClaw 2026.5.x internals and document or validate compatibility rather than silently accepting drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli.ts`:
- Around line 556-586: In src/cli.ts lines 556-586, add bounded timeout and
killSignal options to both execSync calls used for OpenClaw path detection. In
scripts/postinstall.mjs lines 19-29, add a timeout to the spawnSync call
wrapping cli.js setup and handle result.signal or result.error in addition to
result.status so terminated or failed children are not treated as successful.
- Around line 660-705: Wrap the OpenClaw bundle-patching block beginning at
realpathSync(openclawPath) in its own try/catch, so failures from path
resolution, reading, or writing patched bundles are handled locally. Log the
patch failure as a warning and continue successful setup without propagating it
to the outer “Config sync failed” handler or exiting with failure; preserve the
existing patch behavior when operations succeed.
---
Nitpick comments:
In `@src/cli.ts`:
- Around line 631-659: Update the model-cache sync in the CLI flow to write
models.json atomically using the existing temporary-file-and-rename approach
from injectModelsConfig, preserving the current JSON content and logging
behavior. Replace the hardcoded "main" agent segment with the appropriate
configured or discovered OpenClaw agent path so installs using another agent are
handled.
- Around line 678-698: Make the buildModelsProviderData patch fail loudly when
the upstream bundle does not match: track whether each required replacement in
the source patching block actually changed the source, and emit a warning or
error and avoid reporting “model picker ordering” as applied when any target is
missing. Prefer a stable upstream extension point or plugin hook if available;
otherwise anchor matching to verified OpenClaw 2026.5.x internals and document
or validate compatibility rather than silently accepting drift.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1f8f2d1f-6d1e-446c-93fb-512d6c67ef61
⛔ Files ignored due to path filters (5)
dist/cli.jsis excluded by!**/dist/**dist/cli.js.mapis excluded by!**/dist/**,!**/*.mapdist/index.d.tsis excluded by!**/dist/**dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (10)
package.jsonscripts/postinstall.mjssrc/cli.tssrc/index.config.test.tssrc/index.tssrc/models.test.tssrc/models.tssrc/top-models.jsonsrc/top-models.test.tstsup.config.ts
| try { | ||
| const pathOpenClaw = execSync("command -v openclaw", { | ||
| encoding: "utf8", | ||
| stdio: ["ignore", "pipe", "ignore"], | ||
| }).trim(); | ||
| if (pathOpenClaw) candidates.push(pathOpenClaw); | ||
| } catch { | ||
| // npm install scripts often run with a stripped PATH. | ||
| } | ||
|
|
||
| if (process.env.npm_config_prefix) { | ||
| candidates.push(join(process.env.npm_config_prefix, "bin", "openclaw")); | ||
| } | ||
|
|
||
| try { | ||
| const npmRoot = execSync("npm root -g", { | ||
| encoding: "utf8", | ||
| stdio: ["ignore", "pipe", "ignore"], | ||
| }).trim(); | ||
| if (npmRoot) candidates.push(join(dirname(dirname(npmRoot)), "bin", "openclaw")); | ||
| } catch { | ||
| // npm may be unavailable in embedded runtimes. | ||
| } | ||
|
|
||
| candidates.push( | ||
| join(homedir(), ".npm-global", "bin", "openclaw"), | ||
| join(homedir(), ".local", "bin", "openclaw"), | ||
| "/usr/local/bin/openclaw", | ||
| ); | ||
|
|
||
| openclawPath = candidates.find((candidate) => existsSync(candidate)) ?? ""; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
No timeout on synchronous child-process calls in the auto-triggered postinstall chain.
scripts/postinstall.mjs runs on every npm install of this package and synchronously spawns cli.js setup, which itself shells out multiple times via execSync. None of these calls set a timeout, so a hang anywhere in this chain (corporate proxy, broken PATH lookup, stalled npm root -g) blocks the consumer's npm install indefinitely.
src/cli.ts#L556-L586: add atimeout(andkillSignal) option to theexecSync("command -v openclaw", ...)andexecSync("npm root -g", ...)calls used for OpenClaw path detection.scripts/postinstall.mjs#L19-L29: add atimeoutoption to thespawnSynccall as the outermost guard so a hang insidecli.js setupcan't blocknpm installindefinitely, and also checkresult.signal/result.error(not justresult.status) so a killed/crashed child process isn't silently treated as success.
📍 Affects 2 files
src/cli.ts#L556-L586(this comment)scripts/postinstall.mjs#L19-L29
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli.ts` around lines 556 - 586, In src/cli.ts lines 556-586, add bounded
timeout and killSignal options to both execSync calls used for OpenClaw path
detection. In scripts/postinstall.mjs lines 19-29, add a timeout to the
spawnSync call wrapping cli.js setup and handle result.signal or result.error in
addition to result.status so terminated or failed children are not treated as
successful.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/reinstall.sh`:
- Line 217: The plugin cleanup filters in scripts/reinstall.sh (line 217) and
scripts/update.sh (line 230) must also exclude the PascalCase identifier
ClawRouter. Update both filter conditions to remove clawrouter, ClawRouter, and
`@blockrun/clawrouter`.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7f76fed4-2129-472a-9d1d-42e3827232ee
📒 Files selected for processing (2)
scripts/reinstall.shscripts/update.sh
This reverts commit c209439.
P0: v0.12.220 shipped to npm unusable. The tsup banner injected __cjs_createRequire, and a bundled dependency emits an import under the same name into the same ESM scope, so every entrypoint threw a load-time SyntaxError -- `clawrouter` would not start and the library would not import. Renamed the banner identifier to __blockrun_createRequire (spotted by @0xCheetah1 in #206). CI published it because the pipeline runs build && typecheck && test and none of those ever load dist/. Added scripts/smoke-dist.mjs as a postbuild step that imports dist/index.js and runs dist/cli.js --version; verified it exits 1 on the broken bundle and 0 on the fix, so this cannot reach npm again. Models, synced from blockrun (source of truth): - anthropic/claude-fable-5 relisted (Anthropic restored the offer 2026-07-06). fable aliases resolve to the real model again instead of opus-4.8, and it is restored as the premium COMPLEX primary -- reverting the forced 2026-06-13 downgrade. The auto profile is untouched. - xai/grok-4.5 added (upstream 2026-07-13). Explicit pins only; generic `grok` stays on 4.3 pending benchmarks, since 4.5 costs ~1.7x more and doubles the whole request above 200K prompt tokens. Verified end-to-end through the built proxy with real x402 payments on Base. /v1/models advertises 205 entries (was 201) with all 118 aliases intact.
VickyXAI
left a comment
VickyXAI
left a comment
There was a problem hiding this comment.
Thanks @0xCheetah1 — and credit first, because it's owed: the tsup banner change buried in this PR was catching a P0. v0.12.220 was on npm with a CLI that wouldn't start at all (SyntaxError: Identifier '__cjs_createRequire' has already been declared), and @blockrun/mcp couldn't boot either through @blockrun/llm. Nothing in build && typecheck && test ever loads dist/, so CI published it happily and none of us noticed. That's shipped as v0.12.221–223, and you're credited in the CHANGELOG.
Digging into it turned up the deeper cause, which is worth sharing since it changes what this PR should keep:
The banner collision was a symptom. We and @blockrun/llm depend on each other, and noExternal: [/.*/] inlines it. v0.12.220's Polymarket port added src/polymarket/fund.ts — our first-ever @blockrun/llm import — so its import { ... } from "@blockrun/clawrouter" resolved through node_modules to the last published build of this package, and esbuild inlined that entire stale bundle beside the one it was building. Two ClawRouters, different versions, separate module state. The old copy carried its own __cjs_createRequire, and esbuild can't rename around a banner it never sees (banners are raw text injected after bundling) — hence the collision. Renaming fixes the crash but leaves the stale copy: v0.12.221 still shipped at 10.2MB with a whole second ClawRouter inside, and every release would have inlined the previous one (~20MB next). The real fix is an esbuild alias pinning that back-import to our own source — 10.2MB → 7.6MB, on main now. So please keep the rename (good defense-in-depth) but drop the expectation that it's the whole story.
Blocking: the models.ts change guts /v1/models
This is the one thing that can't land as-is. Running both list-builders over the current head (86091aaf) vs main:
main (v0.12.223): 205 entries
pr206 (86091aaf) : 83 entries → 120 aliases lost
Everything lost is a friendly alias: sonnet, opus, haiku, gpt5, kimi, flash, glm, codex, dalle, nano-banana, … The cause is in src/models.ts: ALIAS_MODELS is deleted and the catalog filter narrows from !(m.id in MODEL_ALIASES) to !(m.id.includes("/") && m.id in MODEL_ALIASES). OPENCLAW_MODELS is the sole input to buildProxyModelList() (src/proxy.ts), whose docstring is explicit:
This includes alias IDs (e.g.,
flash,kimi) so/model <alias>works reliably. The picker visibility filter lives at the openclaw.json allowlist layer, not here —/v1/modelsstays open for API-level discovery.
resolveModelAlias() still resolves them, but any client that validates against /v1/models first — including OpenClaw's /model — will reject them. (The other 2 of the 122 are claude-fable-5 / grok-4.5, just branch staleness; a rebase picks those up.)
The good news: you don't need this change to get the dedup. Which brings me to the part of this PR that was dead right.
You found a real bug, and the cache sync was the correct fix
The duplicate/stale rows come from a third model-list plane — ~/.openclaw/agents/<agent>/agent/models.json — separate from openclaw.json's picker and allowlist. Nothing ever synced it. Measured on a real machine after clawrouter setup had already repaired openclaw.json to the correct 47, that cache still held 155 entries — 127 retired (gpt-5.2, gpt-4.1, o1, …), duplicate free / moonshot/kimi-k2.5 rows, and none of the current flagships.
Your model-cache sync targets exactly that. It's shipped in v0.12.223 as syncAgentModelCache(), generalized a little: iterates all agent dirs rather than hardcoding main, only repairs an existing blockrun provider (never introduces one), preserves other providers and baseUrl/api/apiKey, compares order positionally, and is gated outside gateway mode. Your positional-comparison insight for the picker/allowlist is in there too. Thank you — that diagnosis is what made it findable.
Two things I'd drop
Patching OpenClaw's dist/*.js. I'm fairly sure I know why you went there — openclaw plugins install ends in Config write rejected: openclaw.json (size-drop:76642->25200). OpenClaw guards against large config shrinks, and pruning 155 stale models → 47 is a large shrink, so it rejects the write and rolls the install back, restoring the stale list. It's a genuine wall and you were right that something is broken. But string-replacing another package's shipped bundle gets reverted by any openclaw update, and writing OpenClaw's own state from under its install transaction is what stranded users on the 2026.5.2 baseHash rollback. clawrouter setup sidesteps it (it writes directly) and is the supported repair path. I think the real fix belongs upstream in OpenClaw — happy to co-write that issue with you.
FWIW I tested the obvious workaround (prune first, then install) and it does not work: OpenClaw compares against a cached baseline, not current disk.
postinstall auto-running setup. It runs on every npm install — CI, Docker, a dev's local npm install — and silently mutates the user's global ~/.openclaw config. If it ever fires inside OpenClaw's own plugin-install transaction it's the baseHash rollback again. The discoverability problem it solves is real (a bare npm install -g does zero integration); I'd rather solve it with a loud hint than an implicit global write.
Also worth knowing, since it bit this exact flow: anything loose under scripts/ that imports child_process makes the plugin uninstallable — OpenClaw's scanner blocks the whole install. I shipped that bug myself in v0.12.222 and fixed it in v0.12.223; dist/ is exempt.
Suggested path
Rebase on main (v0.12.223) and keep: the openclaw detection candidates in cmdSetup, the Gemini Pro aliases, and the positional comparisons. Drop: the models.ts list rewrite, the OpenClaw dist patch, and the postinstall. The tsup and cache-sync pieces are already in.
On top-models.json: the gpt-5.6-sol-before-terra reorder matches blockrun's registry order, but it re-fronts the tier that 500s after ~250s waits (#202) and that v0.12.219 deliberately routed generic aliases away from. Fine either way — just flagging it was intentional.
Really good catch on the P0. Happy to take the rebase in pieces if that's easier.
2 participants
Summary
Verification
Summary by CodeRabbit