fix(agents): bootstrap non-main models.json on skip to prevent Unknown model after agents.create#58373
fix(agents): bootstrap non-main models.json on skip to prevent Unknown model after agents.create#58373xiangri678 wants to merge 4 commits into
Conversation
Greptile SummaryThis PR fixes a narrow but real startup failure: when Key changes:
Confidence Score: 5/5Safe to merge; only a P2 hardening suggestion remains (use the existing atomic write helper instead of a direct The fix is logically correct and well-scoped. All guard conditions (identity check, existing-file check, empty-source check) are sound, and the write lock ensures serialization. The sole P2 concern — using src/agents/models-config.ts line 121-122: consider switching to
|
| Filename | Overview |
|---|---|
| src/agents/models-config.ts | Adds inheritModelsJsonFromMainAgent helper and wires it into the skip branch of ensureOpenClawModelsJson. Logic and guard conditions are correct; the only concern is a direct fs.writeFile instead of the existing atomic write helper, leaving a narrow crash-corruption window. |
| src/agents/models-config.skips-writing-models-json-no-env-token.test.ts | New test correctly exercises the skip-branch bootstrap scenario: mocks the planner to return skip, seeds a main agent models.json, calls ensureOpenClawModelsJson against a secondary dir, and asserts both wrote: true and content equality. Coverage is targeted and sufficient for the stated scope. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/agents/models-config.ts
Line: 121-122
Comment:
**Non-atomic write inconsistent with existing helper**
`inheritModelsJsonFromMainAgent` writes the bootstrapped file with `fs.writeFile` directly, while every other write path in this module goes through `writeModelsFileAtomic` (write-to-temp + `rename`). If the process is killed between `mkdir` and `writeFile` completing, the target file could be left partially written. On the *next* startup, `fs.access` at line 103 will succeed (the corrupt file exists), causing `inheritModelsJsonFromMainAgent` to return `false` early — so the bootstrap won't re-run and the corrupt file persists, which is exactly the "Unknown model" failure mode this PR is fixing.
`writeModelsFileAtomic` already exists in this file and the directory is created just before, so using it here is straightforward:
```suggestion
await fs.mkdir(params.agentDir, { recursive: true, mode: 0o700 });
await writeModelsFileAtomic(params.targetPath, sourceRaw);
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(agents): bootstrap non-main models.j..." | Re-trigger Greptile
Re-review progress:
- State: Complete
- Detail: The targeted re-review finished, the durable review comment was updated, and the synced verdict was routed.
- Run: https://github.com/openclaw/clawsweeper/actions/runs/25601336946
- Updated: 2026-05-09T12:45:35.193Z
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| await fs.mkdir(params.agentDir, { recursive: true, mode: 0o700 }); | ||
| await fs.writeFile(params.targetPath, sourceRaw, { mode: 0o600 }); |
There was a problem hiding this comment.
inheritModelsJsonFromMainAgent writes the bootstrapped file with fs.writeFile directly, while every other write path in this module goes through writeModelsFileAtomic (write-to-temp + rename). If the process is killed between mkdir and writeFile completing, the target file could be left partially written. On the next startup, fs.access at line 103 will succeed (the corrupt file exists), causing inheritModelsJsonFromMainAgent to return false early — so the bootstrap won't re-run and the corrupt file persists, which is exactly the "Unknown model" failure mode this PR is fixing.
writeModelsFileAtomic already exists in this file and the directory is created just before, so using it here is straightforward:
| await fs.mkdir(params.agentDir, { recursive: true, mode: 0o700 }); | |
| await fs.writeFile(params.targetPath, sourceRaw, { mode: 0o600 }); | |
| await fs.mkdir(params.agentDir, { recursive: true, mode: 0o700 }); | |
| await writeModelsFileAtomic(params.targetPath, sourceRaw); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/models-config.ts
Line: 121-122
Comment:
**Non-atomic write inconsistent with existing helper**
`inheritModelsJsonFromMainAgent` writes the bootstrapped file with `fs.writeFile` directly, while every other write path in this module goes through `writeModelsFileAtomic` (write-to-temp + `rename`). If the process is killed between `mkdir` and `writeFile` completing, the target file could be left partially written. On the *next* startup, `fs.access` at line 103 will succeed (the corrupt file exists), causing `inheritModelsJsonFromMainAgent` to return `false` early — so the bootstrap won't re-run and the corrupt file persists, which is exactly the "Unknown model" failure mode this PR is fixing.
`writeModelsFileAtomic` already exists in this file and the directory is created just before, so using it here is straightforward:
```suggestion
await fs.mkdir(params.agentDir, { recursive: true, mode: 0o700 });
await writeModelsFileAtomic(params.targetPath, sourceRaw);
```
How can I resolve this? If you propose a fix, please make it concise.![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.
Reviewed commit: 1b0d95b761
ℹ️ 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".
| if (plan.action === "skip") { | ||
| return { fingerprint, result: { agentDir, wrote: false } }; | ||
| const inherited = await inheritModelsJsonFromMainAgent({ agentDir, targetPath }); | ||
| return { fingerprint, result: { agentDir, wrote: inherited } }; |
There was a problem hiding this comment.
This skip branch now depends on inheritModelsJsonFromMainAgent, but the cached fingerprint it returns was computed before this check and only tracks the target agent files. If a secondary agent calls this once before main models.json exists, inherited is cached as false; later calls in the same process can keep hitting the cache path and never retry inheritance even after main models.json is created, so the intended bootstrap fix can remain inactive until restart.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| await fs.mkdir(params.agentDir, { recursive: true, mode: 0o700 }); | ||
| await fs.writeFile(params.targetPath, sourceRaw, { mode: 0o600 }); |
There was a problem hiding this comment.
The inheritance path writes directly with fs.writeFile, unlike the normal generation path which uses writeModelsFileAtomic. In the plan.action === "skip" + missing-target case, an interrupted write (process crash, I/O error) can leave a partially written models.json, which then breaks JSON parsing/model discovery for that agent; using the existing atomic writer here would avoid introducing this corruption path.
Useful? React with 👍 / 👎.
43e378b to
3332a97
Compare
|
Thanks for the reviews. I’ve addressed the feedback and rebased this branch onto the latest For the current failing checks: they appear to be outside the scope of this PR’s changes (this PR only touches |
3332a97 to
493bdbb
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 493bdbbedb
ℹ️ 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".
| try { | ||
| await fs.access(params.targetPath); | ||
| // Keep existing per-agent models.json untouched. | ||
| return false; | ||
| } catch { |
There was a problem hiding this comment.
The non-main bootstrap path has a TOCTOU race: it checks fs.access(targetPath) and later writes via atomic rename, but there is no cross-process lock, so another process can create models.json in between and this rename will overwrite it. In multi-process startup (for example, two commands initializing the same agent), this can clobber a freshly generated or user-customized per-agent catalog despite the intent to keep existing files untouched.
Useful? React with 👍 / 👎.
0a3fbb4 to
7b43e3c
Compare
|
This pull request has been automatically marked as stale due to inactivity. |
|
Codex review: needs changes before merge. Reviewed July 3, 2026, 9:02 PM ET / July 4, 2026, 01:02 UTC. Summary PR surface: Source +168, Tests +350, Other +7. Total +525 across 5 files. Reproducibility: yes. Source inspection on current main shows the empty-provider skip branch returns without creating root Review metrics: 2 noteworthy metrics.
Stored data model Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Copy recommended automerge instructionNext step before merge
Security Review findings
Review detailsBest possible solution: Remove the stale allowlist entries, refresh the branch against current main, and land a maintainer-approved missing-file fix with regression coverage for sanitized inheritance and stored-auth request resolution. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection on current main shows the empty-provider skip branch returns without creating root Is this the best way to solve the issue? Unclear. The persisted sanitized bootstrap is a focused mitigation for the missing-file case, but maintainers still need to confirm that this provider/auth compatibility boundary is preferable to read-through inheritance or an early actionable failure. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against f59310a555d7. Label changesLabel justifications:
Evidence reviewedPR surface: Source +168, Tests +350, Other +7. Total +525 across 5 files. View PR surface stats
Acceptance criteria:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
7b43e3c to
b191131
Compare
openclaw-barnacle
Bot
added
the
triage: needs-real-behavior-proof
250e3b0 to
fc46b1b
Compare
openclaw-barnacle
Bot
added
proof: supplied
fc46b1b to
bcdbda2
Compare
clawsweeper
Bot
added
proof: sufficient
clawsweeper
Bot
added
proof: sufficient
openclaw-barnacle
Bot
removed
the
proof: sufficient
clawsweeper
Bot
added
proof: sufficient
|
What's different
How I checked it node scripts/run-vitest.mjs src/agents/models-config.skips-writing-models-json-no-env-token.test.ts src/agents/sessions/model-registry.test.ts node scripts/run-vitest.mjs src/agents/models-config.skips-writing-models-json-no-env-token.test.ts src/agents/sessions/model-registry.test.ts src/agents/model-auth.test.ts -t "injects Authorization Bearer header when authHeader is true" pnpm deadcode:unused-files pnpm exec oxfmt --check scripts/deadcode-unused-files.allowlist.mjs src/agents/models-config.ts src/agents/models-config.skips-writing-models-json-no-env-token.test.ts src/agents/sessions/model-registry.ts src/agents/sessions/model-registry.test.ts pnpm exec oxlint scripts/deadcode-unused-files.allowlist.mjs src/agents/models-config.ts src/agents/models-config.skips-writing-models-json-no-env-token.test.ts src/agents/sessions/model-registry.ts src/agents/sessions/model-registry.test.ts One note on the deadcode allowlist I'm keeping the The 15 failed jobs did not reach the test shards and look like runner/setup infrastructure failures. Could you please rerun them or confirm the CI baseline? |
clawsweeper
Bot
added
proof: sufficient
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: fix(agents): bootstrap non-main models.json on skip to prevent Unknown model after agents.create This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: fix(agents): bootstrap non-main models.json on skip to prevent Unknown model after agents.create This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
1 participant
Summary
agents.createcreates a valid non-main agent (config/workspace), but does not createmodels.jsonby design. Under a narrow startup path, this can later fail withUnknown model.action: "skip",ensureOpenClawModelsJsonnow performs a one-time bootstrap from main agentmodels.jsononly if target is non-main and targetmodels.jsonis missing.models.json, no change toagents.createbehavior itself.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause / Regression History (if applicable)
planOpenClawModelsJsonreturnsskipwhen providers resolve empty;ensureOpenClawModelsJsonpreviously returned early (wrote: false) without creating/bootstrapping missing non-mainmodels.json.models.jsonin theskipbranch.git blame, prior PR, issue, or refactor if known): auth store already supports main→non-main inheritance/fallback, but model registry bootstrap was asymmetric.Regression Test Plan (if applicable)
src/agents/models-config.skips-writing-models-json-no-env-token.test.tsmodels.json+plan.action === "skip"+ existing non-empty mainmodels.json=> target file is created from main.skipbranch and validates one-time bootstrap behavior.User-visible / Behavior Changes
agents.create, a newly created non-main agent that enters the empty-providerskippath no longer proceeds with a missing model catalog andUnknown modelhard failure.Diagram (if applicable)
Security Impact (required)
NoNoNoNoNoYes, explain risk + mitigation: N/ARepro + Verification
Environment
agents.createv2026.3.28andmain(HEAD as of2026-03-31)Steps
agents.createto create a new non-main agent.models.jsonis initially absent (current expected behavior of create path).plan.action === "skip").Expected
Actual
models.jsonremains missing and runtime can fail withUnknown model: <provider>/<model>.Evidence
agents.createsuccess path does not createmodels.json(by design).skipbranch bootstrap path.pnpm test -- src/agents/models-config.skips-writing-models-json-no-env-token.test.ts -t "inherits models.json from main agent for new secondary agents when plan skips"pnpm buildpnpm checkReal behavior proof
agents.createcan have a valid agent directory but nomodels.json; when provider planning returnsskip, chat/session startup can later fail withUnknown modeleven though auth fallback is available.agents.create, confirmed the target agentmodels.jsonwas absent before startup, started the non-main chat/session path that entersensureOpenClawModelsJson, and checked the target agent directory after startup.models.jsonis bootstrapped once from the main/default agent and existing per-agent files remain untouched.Human Verification (required)
Review Conversations
Compatibility / Migration
YesNoNoRisks and Mitigations
models.json.