fix(models): separate Codex harness from model choices#71193
Conversation
🔒 Aisle Security AnalysisWe found 1 potential security issue(s) in this PR:
1. 🟠 Prototype pollution via legacy Codex models allowlist key migration
Description
This migrator runs as part of the Vulnerable code: const next: Record<string, unknown> = {};
...
next[rawKey] = mergeModelEntry(entry, next[rawKey]);
...
next[migratedKey] = mergeModelEntry(entry, next[migratedKey]);RecommendationPrevent dangerous keys from being used as object properties during migration. Options:
const DANGEROUS_RECORD_KEYS = new Set(["__proto__", "prototype", "constructor"]);
const next: Record<string, unknown> = Object.create(null);
for (const [rawKey, entry] of Object.entries(rawModels)) {
if (DANGEROUS_RECORD_KEYS.has(rawKey)) continue; // or record a warning
const migratedKey = migrateLegacyCodexModelRef(rawKey);
const key = migratedKey ?? rawKey;
if (DANGEROUS_RECORD_KEYS.has(key)) continue;
next[key] = mergeModelEntry(entry, next[key]);
}
Also consider adding shared helpers (similar to other migration code that defines Analyzed PR: #71193 at commit Last updated on: 2026-04-24T18:31:08Z |
openclaw-barnacle
Bot
added
docs
PR SummaryMedium Risk Overview Doctor compatibility migration now rewrites legacy primary Docs and changelog are updated to clarify the split between Reviewed by Cursor Bugbot for commit f2f5185. Bugbot is set up for automated code reviews on this repo. Configure here. |
Greptile SummaryThis PR hides the virtual Confidence Score: 5/5Safe to merge; all remaining findings are P2 style/observability concerns with no correctness impact. The migration logic is correct (merge precedence, fallback preservation, harness injection all verified against tests). The two P2 findings (unnecessary list reference replacement and a missing change-log entry for the harness mutation) are non-blocking and do not affect runtime behavior. legacy-config-core-normalizers.ts — shared Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/commands/doctor/shared/legacy-config-core-normalizers.ts
Line: 357-372
Comment:
**Stale `nextAgents.list` replacement when only defaults changed**
`changed` is shared between the `defaults` block and the `list` block. If `defaults` changes but no list entry changes, `nextAgents.list = nextList` is still executed because `changed` is already `true`. `nextList` is a new array but contains the exact same object references as the original list — functionally correct, but any downstream caller that relies on reference equality to detect list mutations will see a false change. A separate `listChanged` flag would isolate the two sections cleanly.
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/commands/doctor/shared/legacy-config-core-normalizers.ts
Line: 323-329
Comment:
**Silent `embeddedHarness` mutation omitted from change log**
When `ensureCodexEmbeddedHarness` returns `changed: true`, `changed = true` is set on the container but no message is pushed to `changes`. Users running the doctor command will see "Moved … legacy codex/* primary refs to openai/* with Codex harness." but will not see an explicit entry for the `embeddedHarness.runtime` field being overwritten. If the field previously held a custom harness id (e.g. `"auto"` or a third-party harness), the silent mutation could be surprising. Adding a `changes.push(...)` call here, analogous to the model and models entries, would make the log complete.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix: separate Codex harness from model c..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (Array.isArray(rawAgents.list)) { | ||
| const nextList = rawAgents.list.map((entry, index) => { | ||
| if (!isRecord(entry)) { | ||
| return entry; | ||
| } | ||
| const agentId = normalizeOptionalString(entry.id) ?? String(index); | ||
| const agent = normalizeLegacyCodexAgentContainer(entry, `agents.list.${agentId}`, changes); | ||
| if (!agent.changed) { | ||
| return entry; | ||
| } | ||
| changed = true; | ||
| return agent.value; | ||
| }); | ||
| if (changed) { | ||
| nextAgents.list = nextList; | ||
| } |
There was a problem hiding this comment.
nextAgents.list replacement when only defaults changed
changed is shared between the defaults block and the list block. If defaults changes but no list entry changes, nextAgents.list = nextList is still executed because changed is already true. nextList is a new array but contains the exact same object references as the original list — functionally correct, but any downstream caller that relies on reference equality to detect list mutations will see a false change. A separate listChanged flag would isolate the two sections cleanly.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/commands/doctor/shared/legacy-config-core-normalizers.ts
Line: 357-372
Comment:
**Stale `nextAgents.list` replacement when only defaults changed**
`changed` is shared between the `defaults` block and the `list` block. If `defaults` changes but no list entry changes, `nextAgents.list = nextList` is still executed because `changed` is already `true`. `nextList` is a new array but contains the exact same object references as the original list — functionally correct, but any downstream caller that relies on reference equality to detect list mutations will see a false change. A separate `listChanged` flag would isolate the two sections cleanly.
How can I resolve this? If you propose a fix, please make it concise.| if (model.codexPrimarySelected) { | ||
| const harness = ensureCodexEmbeddedHarness(raw.embeddedHarness); | ||
| if (harness.changed) { | ||
| next.embeddedHarness = harness.value; | ||
| changed = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
embeddedHarness mutation omitted from change log
When ensureCodexEmbeddedHarness returns changed: true, changed = true is set on the container but no message is pushed to changes. Users running the doctor command will see "Moved … legacy codex/* primary refs to openai/* with Codex harness." but will not see an explicit entry for the embeddedHarness.runtime field being overwritten. If the field previously held a custom harness id (e.g. "auto" or a third-party harness), the silent mutation could be surprising. Adding a changes.push(...) call here, analogous to the model and models entries, would make the log complete.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/commands/doctor/shared/legacy-config-core-normalizers.ts
Line: 323-329
Comment:
**Silent `embeddedHarness` mutation omitted from change log**
When `ensureCodexEmbeddedHarness` returns `changed: true`, `changed = true` is set on the container but no message is pushed to `changes`. Users running the doctor command will see "Moved … legacy codex/* primary refs to openai/* with Codex harness." but will not see an explicit entry for the `embeddedHarness.runtime` field being overwritten. If the field previously held a custom harness id (e.g. `"auto"` or a third-party harness), the silent mutation could be surprising. Adding a `changes.push(...)` call here, analogous to the model and models entries, would make the log complete.
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: 9bffce2868
ℹ️ 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".
| changes.push(`Moved ${path}.model legacy codex/* primary refs to openai/* with Codex harness.`); | ||
| } | ||
|
|
||
| const models = normalizeLegacyCodexAllowlistModels(raw.models, model.codexPrimarySelected); |
There was a problem hiding this comment.
This rewrites raw.models keys to openai/* whenever the current container has a codex primary, which is problematic for agents.defaults: other agents can still intentionally keep fallback-only codex/* refs (their primary is non-codex, so those refs are preserved), but per-model overrides are read by exact key at runtime (for example in resolveExtraParams and fast-mode resolution). After this migration, those fallback codex/* runs no longer match their agents.defaults.models entry, so alias/params behavior silently changes even though the fallback ref itself was preserved.
Useful? React with 👍 / 👎.
9bffce2 to
f2f5185
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f2f5185. Configure here.
| const initialKeys = allowedKeySet | ||
| ? initialSeeds.filter((key) => allowedKeySet.has(key)) | ||
| : initialSeeds; | ||
| : initialSeeds.filter(isModelPickerVisibleModelRef); |
There was a problem hiding this comment.
Allowlist initial values not filtered by visibility
Low Severity
When allowedKeySet is present, initialKeys is filtered only by allowlist membership but not by isModelPickerVisibleModelRef. Meanwhile, the allowedCatalog options and supplementalKeys both apply the visibility filter. This creates an inconsistency where initialValues passed to the multiselect may include codex/* keys that have no matching option, since those entries were removed from the options list by the new visibility filtering.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit f2f5185. Configure here.
|
Landed via squash merge onto main.
Thanks @vincentkoc for the original direction in #71176. |
* fix: separate Codex harness from model choices * docs: note Codex harness model choice fix
* fix: separate Codex harness from model choices * docs: note Codex harness model choice fix
* fix: separate Codex harness from model choices * docs: note Codex harness model choice fix
* fix: separate Codex harness from model choices * docs: note Codex harness model choice fix
* fix: separate Codex harness from model choices * docs: note Codex harness model choice fix
* fix: separate Codex harness from model choices * docs: note Codex harness model choice fix
* fix: separate Codex harness from model choices * docs: note Codex harness model choice fix
1 participant
Summary
codexharness provider from normal model picker and/modelsprovider surfaces.codex/*agent model refs to canonicalopenai/*plus explicitembeddedHarness.runtime = "codex".codex/*refs instead of pinning the whole agent/defaults container to the Codex harness.Replaces #71176 with a maintainer-owned rewrite because that branch is not maintainer-editable and the original migration was unsafe for fallback-only
codex/*refs.Tests
pnpm test src/commands/model-picker.test.ts src/auto-reply/reply/commands-models.test.ts src/commands/doctor-legacy-config.migrations.test.tsOPENCLAW_VITEST_MAX_WORKERS=1 pnpm test src/agents/model-fallback.test.ts src/agents/tools/image-tool.test.ts src/agents/pi-embedded-runner/run.overflow-compaction.loop.test.ts src/agents/pi-embedded-runner/run.attempt-param-forwarding.test.ts src/agents/pi-embedded-runner/usage-reporting.test.tspnpm lint:corepnpm check:test-typespnpm check:docsgit diff --checkpnpm check:changedpassed type/lint/guards but the broad agents test shard timed out under concurrent local load; the failed files passed when rerun serially with one worker.