Skip to content

CLI: include synthetic Codex gpt-5.4 rows in models --all#38696

Closed
vicjayjay wants to merge 2 commits intoopenclaw:mainfrom
vicjayjay:main
Closed

CLI: include synthetic Codex gpt-5.4 rows in models --all#38696
vicjayjay wants to merge 2 commits intoopenclaw:mainfrom
vicjayjay:main

Conversation

@vicjayjay
Copy link
Copy Markdown

Summary

Describe the problem and fix in 2-5 bullets:

  • Problem: openclaw models list --all only used discovered registry models, so synthetic forward-compat catalog entries like openai-codex/gpt-5.4 were omitted even though source-level fallback support already existed.
  • Why it matters: users comparing docs/config/auth output against models list --all saw inconsistent Codex OAuth model coverage.
  • What changed: merge synthetic catalog entries into models list --all via loadModelCatalog() and resolveModelWithRegistry(), and update/add tests for the openai-codex/gpt-5.4 path.
  • What did NOT change (scope boundary): this PR does not change provider auth flows or model runtime behavior beyond list output and stale test expectations.

Change Type (select all)

  • Bug fix
  • Feature
  • Refactor
  • Docs
  • Security hardening
  • Chore/infra

Scope (select all touched areas)

  • Gateway / orchestration
  • Skills / tool execution
  • Auth / tokens
  • Memory / storage
  • Integrations
  • API / contracts
  • UI / DX
  • CI/CD / infra

Linked Issue/PR

User-visible / Behavior Changes

  • openclaw models list --all now includes synthetic forward-compat entries like openai-codex/gpt-5.4 when the catalog exposes them.
  • openclaw models auth login --provider openai-codex test expectations now match the current gpt-5.4 default.

Security Impact (required)

  • New permissions/capabilities? (Yes/No) No
  • Secrets/tokens handling changed? (Yes/No) No
  • New/changed network calls? (Yes/No) No
  • Command/tool execution surface changed? (Yes/No) No
  • Data access scope changed? (Yes/No) No
  • If any Yes, explain risk + mitigation:

Repro + Verification

Environment

  • OS: Windows 11
  • Runtime/container: Node 22 / pnpm / Vitest
  • Model/provider: openai-codex/gpt-5.4
  • Integration/channel (if any): None
  • Relevant config (redacted): default OpenClaw config with Codex OAuth model refs in tests

Steps

  1. Configure or synthesize openai-codex/gpt-5.4 via the source-level model catalog fallback.
  2. Run openclaw models list --all.
  3. Run the targeted auth + model list tests.

Expected

  • models list --all includes the synthetic openai-codex/gpt-5.4 row.
  • Auth login tests reflect the current gpt-5.4 default.

Actual

  • Before this patch, models list --all could omit openai-codex/gpt-5.4, and auth tests still expected gpt-5.3-codex.

Evidence

Attach at least one:

  • Failing test/log before + passing after
  • Trace/log snippets
  • Screenshot/recording
  • Perf numbers (if relevant)

Human Verification (required)

What you personally verified (not just CI), and how:

  • Verified scenarios: targeted Vitest run for src/commands/models/auth.test.ts and src/commands/models/list.list-command.forward-compat.test.ts.
  • Edge cases checked: configured-mode fallback still marks synthetic Codex rows as available when auth exists; configured-mode error handling still exits cleanly when the registry is unavailable.
  • What you did not verify: full end-to-end OAuth login in an interactive TTY session.

Compatibility / Migration

  • Backward compatible? (Yes/No) Yes
  • Config/env changes? (Yes/No) No
  • Migration needed? (Yes/No) No
  • If yes, exact upgrade steps:

Failure Recovery (if this breaks)

  • How to disable/revert this change quickly: revert commit b2d3ebf0a.
  • Files/config to restore: src/commands/models/list.list-command.ts, src/commands/models/auth.test.ts, src/commands/models/list.list-command.forward-compat.test.ts.
  • Known bad symptoms reviewers should watch for: duplicate rows or missing rows in models list --all for forward-compat catalog entries.

Risks and Mitigations

List only real risks for this PR. Add/remove entries as needed. If none, write None.

  • Risk: synthetic catalog entries could surface duplicate rows if merge keys are inconsistent.
    • Mitigation: rows are deduplicated by modelKey(provider, id) before printing, and the new test covers the synthetic openai-codex/gpt-5.4 path.

@openclaw-barnacle openclaw-barnacle Bot added commands Command implementations size: S labels Mar 7, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Mar 7, 2026

Greptile Summary

This PR merges synthetic forward-compat catalog entries (e.g., openai-codex/gpt-5.4) into models list --all output by calling loadModelCatalog() after the registry load and resolving each catalog entry via resolveModelWithRegistry(), and updates auth test expectations to reflect the current gpt-5.4 default.

Key changes:

  • list.list-command.ts: Adds a catalog-merge block (gated on opts.all && modelRegistry) that deduplicates by modelKey and resolves synthetic entries into full Model objects before they reach the sort/print loop.
  • list.list-command.forward-compat.test.ts: Adds a mock for loadModelCatalog and a new test verifying the synthetic row is present in --all output; also fixes the default config entry from gpt-5.3-codex to gpt-5.4.
  • auth.test.ts: Updates two assertions from gpt-5.3-codex to gpt-5.4 to match current behavior.

Issue found: The toModelRow calls inside the --all loop do not pass allowProviderAvailabilityFallback: true for catalog-merged entries. Because synthetic models won't appear in availableKeys (populated from registry.getAvailable()), they will always render as available: false in --all output even when the user has auth configured for the provider. The configured-mode path already handles this correctly using !discoveredKeys.has(key). The new test does not assert on the available field, leaving this gap undetected.

Confidence Score: 3/5

  • Safe to merge as a display-only change, but synthetic entries in --all will consistently show as unavailable even when the user is authenticated, which is misleading and inconsistent with the configured-mode behavior.
  • The core mechanic (merging catalog entries into the model list) is correct and the deduplication logic is sound. The resolveForwardCompatModel path always returns a model with the requested id, so no key mismatch occurs. However, omitting allowProviderAvailabilityFallback for catalog-merged --all rows means these rows will show available: false even for authenticated users — the opposite of the behavior described in the PR for configured mode. This is a functional inaccuracy in the displayed output and warrants a fix before merge.
  • Pay close attention to src/commands/models/list.list-command.ts lines 98–118 and the corresponding test in list.list-command.forward-compat.test.ts.

Comments Outside Diff (1)

  1. src/commands/models/list.list-command.ts, line 98-118 (link)

    Catalog-merged rows always show available: false in --all mode

    Synthetic catalog entries resolved via resolveModelWithRegistry (forward-compat fallbacks like openai-codex/gpt-5.4) will never appear in availableKeys — those keys come from registry.getAvailable(), which only knows about models the registry itself discovered. As a result, the toModelRow call here never sets allowProviderAvailabilityFallback: true, so when availableKeys is defined the available field is forced to false for every catalog-merged row, even when the user is authenticated for the provider.

    The configured-mode path (the else branch, lines 130–155) already handles this correctly by passing allowProviderAvailabilityFallback: !discoveredKeys.has(modelKey(model.provider, model.id)). Because discoveredKeys is computed before the catalog merge, it still accurately reflects only the originally-registered models, making it the right key to check here as well.

    Adding allowProviderAvailabilityFallback: !discoveredKeys.has(key) to the toModelRow call inside this loop would bring --all mode in line with configured mode for synthetic/forward-compat entries.

    The new test only asserts on key presence and does not exercise the available field, so this regression is not caught. A companion test that mocks a provider auth profile and asserts available: true on the synthetic --all row would cover the gap.

Last reviewed commit: b2d3ebf

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b2d3ebf0a8

ℹ️ 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".

Comment on lines +78 to +81
mergedByKey.set(key, model);
}

models = [...mergedByKey.values()];
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve auth-based availability for synthetic --all rows

Merging catalog-only entries into models here introduces rows that are not present in availableKeys, but the later --all rendering path still calls toModelRow without allowProviderAvailabilityFallback; per toModelRow’s logic this forces available to false whenever getAvailable() lacks that key. In practice, a synthetic forward-compat model like openai-codex/gpt-5.4 can be usable (auth exists and non---all mode marks it available) yet models list --all will incorrectly report it as unavailable.

Useful? React with 👍 / 👎.

@dorukardahan
Copy link
Copy Markdown
Contributor

i opened #40160 as a smaller follow-up on latest main. it keeps discovered --all rows, fills the missing synthetic catalog rows, and covers the availability + empty-catalog cases. i think it supersedes this one: #40160

Upstream refactored --all listing into appendDiscoveredRows/appendCatalogSupplementRows
and expanded forward-compat tests. Accept upstream's structure since the feature is
already covered by the new test suite.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@steipete
Copy link
Copy Markdown
Contributor

steipete commented Apr 4, 2026

Thanks for the patch. I pulled latest main and checked the current tree; this is already covered.

Current main already has explicit coverage for this path in src/commands/models/list.list-command.forward-compat.test.ts:

  • :219-235 marks synthetic openai-codex/gpt-5.4 rows as available when provider auth exists
  • :263-290 covers models list --all including synthetic Codex gpt-5.4 rows when the catalog supports them

Closing as already in main. Thanks for chasing this.

@steipete steipete closed this Apr 4, 2026
@idusma0110-hub
Copy link
Copy Markdown

give me codex 5.4 api key please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

commands Command implementations size: XS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants