agents: bootstrap non-main agent models.json from main when provider plan is empty#58391
agents: bootstrap non-main agent models.json from main when provider plan is empty#58391rogerdigital wants to merge 1 commit into
Conversation
🔒 Aisle Security AnalysisWe found 3 potential security issue(s) in this PR:
1. 🟠 Non-main agents bootstrap models.json by copying main agent provider API keys
Description
This can expose sensitive provider credentials (e.g.,
Vulnerable code (verbatim copy): const mainModelsPath = path.join(mainAgentDir, "models.json");
mainContents = await fs.readFile(mainModelsPath, "utf-8");
...
await writeModelsFileAtomicForModelsJson(targetPath, mainContents);Even though the file mode is set to RecommendationAvoid copying secrets from the main agent to non-main agents. Safer options:
Example: strip const parsed = JSON.parse(mainContents);
if (parsed?.providers && typeof parsed.providers === "object") {
for (const provider of Object.values(parsed.providers)) {
if (provider && typeof provider === "object") {
delete (provider as any).apiKey;
}
}
}
await writeModelsFileAtomicForModelsJson(targetPath, JSON.stringify(parsed, null, 2) + "\n");(Adjust to your actual provider secret fields / secret-ref mechanism.) 2. 🟡 Malformed models.json can block non-main bootstrap (persistent config poisoning / DoS)
Description
Vulnerable flow:
RecommendationTreat malformed JSON as not a valid existing configuration for purposes of the non-main bootstrap decision. Options:
const existingModelsFile = await readExistingModelsFile(targetPath);
const hasValidExisting = existingModelsFile.parsed !== null;
if (agentDir !== mainAgentDir) {
if (hasValidExisting) {
// keep existing
return ...;
}
// bootstrap from main
}
3. 🔵 Filesystem path disclosure via stack trace on models.json read errors
Description
This can disclose sensitive local filesystem paths and other runtime details contained in Node.js error stacks (and potentially error messages), for example:
Vulnerable sink: runtime.error(`Model registry unavailable:\n${formatErrorWithStack(err)}`);If this output can reach untrusted log collectors or be shared in support bundles, it may unintentionally leak hostnames, usernames, directory layouts, etc. RecommendationAvoid printing full stack traces (which often include absolute paths) by default. Prefer a sanitized message, optionally gated behind a debug flag. Example: } catch (err) {
const message = err instanceof Error ? err.message : String(err);
runtime.error(`Model registry unavailable: ${message}`);
// Optionally: if (runtime.debug) runtime.error(err instanceof Error ? err.stack ?? "" : "");
process.exitCode = 1;
return;
}Also consider mapping common filesystem errors (e.g., Analyzed PR: #58391 at commit Last updated on: 2026-04-09T04:39:17Z |
![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: 0b2454e5d6
ℹ️ 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".
Greptile SummaryThis PR adds a bootstrap fallback in Key findings:
Confidence Score: 4/5Needs one content-equality guard before merging; without it the fingerprint cache never stabilizes and every call for a non-main agent unnecessarily rewrites the file. One P1 defect: the bootstrap write is unconditional, breaking the mtime-based fingerprint cache contract. The core intent and guard conditions are correct; the fix is a small early-return analogous to the existing noop path in planOpenClawModelsJson. src/agents/models-config.ts lines 180–186: add an existingModelsFile.raw === mainContents early-return before calling writeModelsFileAtomic.
|
| Filename | Overview |
|---|---|
| src/agents/models-config.ts | Adds bootstrap fallback in the plan.action === "skip" branch, but writes unconditionally — the fingerprint (which includes the target file's mtime) never stabilizes, so every successive call re-writes the file and returns wrote: true even when content is unchanged. |
| src/agents/models-config.non-main-bootstrap.test.ts | New test file covering three bootstrap scenarios; all pass but none exercise idempotency (second-call cache hit), which is the scenario where the cache-instability bug manifests. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/agents/models-config.ts
Line: 180-186
Comment:
**Bootstrap writes unconditionally, breaking cache stability**
The fingerprint is built from `modelsFileMtimeMs` of the non-main agent's `models.json` (see `buildModelsJsonFingerprint`). Because the bootstrap always writes (even when the content hasn't changed), the following loop occurs on every successive call:
1. Call N: `modelsFileMtimeMs = T(N-1)` → fingerprint F(N) ≠ F(N-1) (cache miss)
2. Plan still returns `"skip"` (providers are still empty for the non-main agent)
3. Bootstrap writes main's content again → mtime becomes T(N), fingerprint would be F(N+1)
4. Repeat
In practice this means each call to `ensureOpenClawModelsJson` for a non-main bootstrapped agent (e.g. every run start in `pi-embedded-runner/run.ts` and every compact in `pi-embedded-runner/compact.ts`) rewrites the file unnecessarily and returns `wrote: true` even though the content is identical.
The fix mirrors how `planOpenClawModelsJson` avoids this with its own `noop` path — compare `existingModelsFile.raw` against `mainContents` before writing:
```
const mainContents = await fs.readFile(mainModelsPath, "utf-8");
if (mainContents.trim()) {
if (existingModelsFile.raw === mainContents) {
await ensureModelsFileMode(targetPath);
return { fingerprint, result: { agentDir, wrote: false } };
}
await fs.mkdir(agentDir, { recursive: true, mode: 0o700 });
await writeModelsFileAtomic(targetPath, mainContents);
await ensureModelsFileMode(targetPath);
return { fingerprint, result: { agentDir, wrote: true } };
}
```
This stabilises the fingerprint on the second call (once the file exists with the right content, mtime no longer changes, and the cache hits cleanly from then on).
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/models-config.non-main-bootstrap.test.ts
Line: 26-62
Comment:
**Tests don't exercise second-call cache idempotency**
Each test calls `ensureOpenClawModelsJson` exactly once and then reads the result. This means the fingerprint-instability bug described in the production code comment above passes all three tests undetected.
Consider adding a fourth test that calls `ensureOpenClawModelsJson` twice for the same non-main agent and asserts that the second call returns `{wrote: false}` and does not change the file's mtime:
```typescript
it("does not rewrite on subsequent call when content is unchanged", async () => {
await withTempHome(async (home) => {
const mainAgentDir = path.join(home, "agents", "main", "agent");
const nonMainAgentDir = path.join(home, "agents", "worker", "agent");
process.env.OPENCLAW_AGENT_DIR = mainAgentDir;
process.env.PI_CODING_AGENT_DIR = mainAgentDir;
await fs.mkdir(mainAgentDir, { recursive: true });
await fs.writeFile(
path.join(mainAgentDir, "models.json"),
JSON.stringify({ providers: { openai: {} } }, null, 2),
);
const first = await ensureOpenClawModelsJson({}, nonMainAgentDir);
expect(first.wrote).toBe(true);
resetModelsJsonReadyCacheForTest();
const mtimeAfterFirst = (await fs.stat(path.join(nonMainAgentDir, "models.json"))).mtimeMs;
const second = await ensureOpenClawModelsJson({}, nonMainAgentDir);
expect(second.wrote).toBe(false);
const mtimeAfterSecond = (await fs.stat(path.join(nonMainAgentDir, "models.json"))).mtimeMs;
expect(mtimeAfterSecond).toBe(mtimeAfterFirst);
});
});
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "agents: bootstrap non-main agent models...." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
openclaw-barnacle
Bot
added
scripts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f12a5f21e4
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f12a5f21e4
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83a3f32024
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1530130433
ℹ️ 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".
|
All actionable review feedback on this PR has been addressed and the review threads are resolved. Follow-up fixes landed in:
Current state:
|
962454e to
03664d8
Compare
openclaw-barnacle
Bot
added
size: S
size: M
and removed
scripts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15c00afc25
ℹ️ 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".
| return { cacheable: true, fingerprint, result: { agentDir, wrote: true } }; | ||
| } | ||
| } | ||
| return { cacheable: true, fingerprint, result: { agentDir, wrote: false } }; |
There was a problem hiding this comment.
When main/models.json exists but is empty/whitespace, this branch returns cacheable: true and wrote: false, but the fingerprint only tracks the target agent's files and config (not the main file contents). In a long-lived process, a first call made while main is empty will be cached and subsequent calls for the non-main agent will keep returning the cached skip even after main later gets valid content, so bootstrap never retries and model resolution can stay broken until cache reset/restart.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Handled in 6f04c34. The empty-main bootstrap path now returns cacheable: false, matching the missing-main path (83a3f32), so a long-lived process retries after main gets populated. Added a regression test for the empty-main → populated-main transition.
15c00af to
f4d8135
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4d8135afc
ℹ️ 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".
| return { cacheable: true, fingerprint, result: { agentDir, wrote: true } }; | ||
| } | ||
| } | ||
| return { cacheable: true, fingerprint, result: { agentDir, wrote: false } }; |
There was a problem hiding this comment.
When plan.action === "skip" for a non-main agent and main/models.json exists but is empty, this path returns cacheable: true with wrote: false. Because buildModelsJsonFingerprint only tracks the target agent state (not the main agent file), that cached skip can persist in a long-lived process even after main later writes valid content, so bootstrap is never retried and the non-main agent can remain without a usable models.json.
Useful? React with 👍 / 👎.
f4d8135 to
99c7511
Compare
This comment was marked as spam.
This comment was marked as spam.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99c7511cff
ℹ️ 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".
| return { cacheable: true, fingerprint, result: { agentDir, wrote: true } }; | ||
| } | ||
| } | ||
| return { cacheable: true, fingerprint, result: { agentDir, wrote: false } }; |
There was a problem hiding this comment.
When plan.action === "skip" for a non-main agent and main/models.json exists but contains only whitespace, this path returns a cacheable wrote: false result. Because the fingerprint only tracks the target agent state (not the main agent file), a long-lived process can keep serving this cached skip even after main/models.json is later populated, so bootstrap is never retried and the non-main agent can remain without a usable catalog until process restart or unrelated fingerprint change.
Useful? React with 👍 / 👎.
6f04c34 to
eaf826c
Compare
…plan is empty
When `planOpenClawModelsJson` returns `{ action: "skip" }` for a non-main
agent (provider resolution finds zero providers), the agent's models.json
is never written, leaving the model registry empty and causing "Unknown
model" failures at runtime.
Add a bootstrap fallback: copy the main agent's models.json into the
non-main agent directory when the plan skips and no existing file is
present. Includes idempotency guards, error surfacing, retry-after-main-
appears logic, and preservation of existing/malformed non-main files.
Closes openclaw#58289
eaf826c to
02b6411
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02b6411e2e
ℹ️ 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 (existingModelsFile.readError) { | ||
| throw existingModelsFile.readError; |
There was a problem hiding this comment.
ensureOpenClawModelsJson now throws immediately when readExistingModelsFile reports readError, before planOpenClawModelsJson runs. That makes any non-ENOENT/ENOTDIR read failure (for example an unreadable models.json after a permissions drift) fatal even when the plan would be write and could regenerate the file via atomic replace in a writable parent directory. Before this commit those cases could recover by rewriting models.json; this change regresses that recovery path outside the intended non-main skip-bootstrap handling.
Useful? React with 👍 / 👎.
|
Closing: upstream has been actively refactoring this module (cacheable field removed, MODELS_JSON_STATE introduced, function renames), making it difficult to keep this patch in sync. Will reopen once issue #58289 receives maintainer triage and the models-config churn settles. |
2 participants
Summary
planOpenClawModelsJsonreturns{ action: "skip" }for a non-main agent (provider resolution finds zero providers),models.jsonis never written. The model registry stays empty and any subsequent inference call fails with "Unknown model".plan.action === "skip"branch ofensureOpenClawModelsJson. When the target is a non-main agent directory and has no existingmodels.json, the main agent'smodels.jsonis copied in. Includes idempotency guards, error surfacing, retry-after-main-appears logic, and preservation of existing/malformed non-main files.planOpenClawModelsJsonitself is untouched. The normalnoop/writepaths are unchanged. Main agent behavior is unchanged. No config schema or CLI surface changes.Change Type (select all)
Scope (select all touched areas)
Other: Agent runtime — model registry bootstrap
Linked Issue/PR
Root Cause / Regression History
ensureOpenClawModelsJsontreatedplan.action === "skip"as a terminal no-op for all agents. For non-main agents where provider resolution returns empty, this meantmodels.jsonwas never created, leaving the model registry empty at runtime.models.jsonfrom initial setup), but non-main agents created viaagents.createstart with an empty directory.Regression Test Plan
src/agents/models-config.non-main-bootstrap.test.tsmodels.json+ main agent has one → bootstrap copies itmodels.json→ graceful skip (no crash)models.json→ preserved, not overwrittenmodels.jsonappears after initial miss → retry succeedsmodels.json→ preserved, not overwrittenmodels.json(permission error) → error surfacedUser-visible / Behavior Changes
models.jsonand start successfully.Diagram
Security Impact (required)
models.json(which may contain API keys) into the non-main agent directory. This is safe in the current threat model because: (a) both directories are under~/.openclaw/agents/owned by the same OS user with mode0700; (b) non-main agents are spawned by the same gateway process that owns the main agent; (c) the file is written with mode0600viaensureModelsFileMode. There is no cross-user or cross-tenant boundary being crossed.agentDirOverride(Aisle WA business, groups & office hours #3):agentDirOverrideis only passed from internal callers (runEmbeddedPiAgent), never from external/user input. TheresolveOpenClawAgentDir()fallback resolves from env vars set by the gateway itself. No API surface accepts arbitraryagentDirfrom untrusted sources.bundled-plugin-build-entries.mjs(Aisle fix: add @lid format support and allowFrom wildcard handling #1): Pre-existing issue unrelated to this PR —sourceEntriescomes from repo-ownedpackage.jsonfiles, not user input. Flagged for separate tracking.Repro + Verification
Environment
Steps
~/.openclaw/agents/main/agent/models.json.agents.create(or manually create an agent dir withoutmodels.json).Expected
models.jsonfrom main and session starts successfully.Actual (before fix)
models.jsonis never written, runtime fails withUnknown model: <provider>/<model>.Evidence
Test output:
Human Verification (required)
models.json, ranensureOpenClawModelsJson→ file bootstrapped from main, content matcheswrote: false, mtime unchanged (idempotency confirmed)models.json, ran for non-main → graceful skip, no crashpnpm checkpasses,pnpm test -- src/agents/models-configpassesENOENThandled, falls through to skipmodels.jsoncontains invalid JSON → preserved as-ismodels.jsonunreadable (permission denied) → error surfacedReview Conversations
Follow-up fixes addressing bot feedback:
f12a5f2— preserve existing non-mainmodels.json, stop swallowing bootstrap write failures, add idempotency coverage83a3f32— retry bootstrap after main file appears later, avoid depending on main readability when non-main already has content1530130— preserve malformed non-mainmodels.json962454e— surface unreadable existing non-mainmodels.jsoninstead of treating as missing6f04c34— mark empty bootstrap source as non-cacheable so retry picks up later contentCompatibility / Migration
Risks and Mitigations
~/.openclaw/agents/tree, same OS user, mode0600. No cross-trust-boundary exposure. If future multi-tenant support is added, this path should be revisited.models.jsonis stale or incomplete when copied.models.jsonat all. Once bootstrapped, subsequentensureOpenClawModelsJsoncalls with real provider resolution will overwrite via the normalwritepath.