Skip to content

fix(models): filter models list by configured providers in replace mode [AI-assisted]#94914

Closed
aniruddhaadak80 wants to merge 2 commits into
openclaw:mainfrom
aniruddhaadak80:fix/models-mode-replace-filter-list
Closed

fix(models): filter models list by configured providers in replace mode [AI-assisted]#94914
aniruddhaadak80 wants to merge 2 commits into
openclaw:mainfrom
aniruddhaadak80:fix/models-mode-replace-filter-list

Conversation

@aniruddhaadak80

@aniruddhaadak80 aniruddhaadak80 commented Jun 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Closes #94705

What Problem This Solves

The output of openclaw models list was not filtered when models.mode is set to "replace" with a restricted list of providers, leading to a default list leak where unauthorized provider models were shown.

Why This Change Was Made

To correctly enforce the models.mode "replace" configuration. The fix applies provider allowlisting to the shared model-list row filtering when in replace mode, but explicitly bypasses this restriction when the user explicitly requests full catalog browsing (e.g., using --all or a specific provider filter).

User Impact

Users relying on models.mode "replace" will correctly only see models from their configured models.providers. Explicit browsing (--all) remains unaffected.

Evidence

Unit tests added and passed on Windows.

@openclaw-barnacle openclaw-barnacle Bot added commands Command implementations size: S triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels Jun 19, 2026
@clawsweeper

clawsweeper Bot commented Jun 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 5, 2026, 4:44 AM ET / 08:44 UTC.

Summary
The PR adds replace-mode provider allowlisting to model-list row filtering, passes an explicit browse flag for --all and provider-filtered lists, and adds row-builder regression tests.

PR surface: Source +27, Tests +116. Total +143 across 3 files.

Reproducibility: yes. at source level: current main's default models list path appends authenticated catalog rows without checking models.mode, and the linked issue supplies concrete leaked-provider output. I did not run a live CLI reproduction in this read-only review.

Review metrics: 2 noteworthy metrics.

  • Existing config-mode behavior: 1 mode tightened. The PR changes how models.mode: replace constrains default model-list output for existing configurations.
  • Type-check failures: 2 fixture entries invalid. Both new test fixtures use provider config objects that fail the current test type lane.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #94705
Summary: This PR is a candidate fix for the canonical replace-mode openclaw models list leakage issue and overlaps with other open candidate fixes.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦪 silver shellfish
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Make the models.providers.xai fixtures type-valid and rerun the test type lane.
  • [P1] Add redacted terminal output, copied live output, logs, or a terminal screenshot from a real after-patch openclaw models list run.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body reports unit tests on Windows but does not include real after-patch openclaw models list output from a configured setup. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] Exact-head check-test-types currently fails on the new test fixtures, so the branch cannot merge as-is.
  • [P1] Contributor proof is still only unit-test output; external PR policy needs redacted real after-patch openclaw models list output, logs, or a terminal screenshot.
  • [P1] Default models list output for existing replace-mode setups becomes narrower after upgrade, which matches the documented contract but remains compatibility-sensitive.

Maintainer options:

  1. Fix Type Fixtures And Re-Prove (recommended)
    Repair the test config fixtures, add real after-patch CLI proof, and land if maintainers accept the documented replace-mode output tightening.
  2. Choose Another Candidate
    Maintainers can pause this branch and select a same-root candidate with stronger proof or cleaner coverage if it becomes the safer canonical fix.

Next step before merge

  • [P1] Contributor action is needed for real behavior proof, and the remaining code blocker is a small test-fixture type repair.

Security
Cleared: The diff changes TypeScript CLI row filtering and tests only; it adds no dependency, workflow, secret-handling, package, install, or release surface.

Review findings

  • [P2] Use type-valid provider fixtures — src/commands/models/list.rows.test.ts:232
Review details

Best possible solution:

Land one canonical fix that filters the default replace-mode model list, preserves explicit browse views, has type-clean regression coverage, and includes real CLI proof.

Do we have a high-confidence way to reproduce the issue?

Yes, at source level: current main's default models list path appends authenticated catalog rows without checking models.mode, and the linked issue supplies concrete leaked-provider output. I did not run a live CLI reproduction in this read-only review.

Is this the best way to solve the issue?

Mostly yes, but not mergeable yet. The browse bypass addresses the prior over-filtering concern, but the new tests do not typecheck and the PR still lacks real after-patch CLI proof.

Full review comments:

  • [P2] Use type-valid provider fixtures — src/commands/models/list.rows.test.ts:232
    The added replace-mode tests set models.providers.xai to {}, but ModelProviderConfig requires baseUrl and models, so exact-head check-test-types fails with TS2739 at both inserted fixtures. This was equally visible in the previously reviewed head for the first fixture, so this is a late finding; use a complete minimal provider fixture or a typed helper that satisfies the real config shape.
    Confidence: 0.94
    Late finding: first raised on code an earlier review cycle already covered.

Overall correctness: patch is incorrect
Overall confidence: 0.91

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 53bd4dde6c00.

Label changes

Label changes:

  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body reports unit tests on Windows but does not include real after-patch openclaw models list output from a configured setup. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • remove status: 🔁 re-review loop: Current PR status label is status: 📣 needs proof.

Label justifications:

  • P2: This targets a real but limited CLI/model catalog correctness bug; runtime allowlisting still blocks disallowed model use.
  • merge-risk: 🚨 compatibility: Merging changes default models list visibility for existing replace-mode configurations, even though that aligns with the documented contract.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body reports unit tests on Windows but does not include real after-patch openclaw models list output from a configured setup. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +27, Tests +116. Total +143 across 3 files.

View PR surface stats
Area Files Added Removed Net
Source 2 27 0 +27
Tests 1 116 0 +116
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 3 143 0 +143

What I checked:

  • Repository policy read: Root AGENTS.md was read fully; its whole-surface PR review and config compatibility guidance applies because this changes model-list behavior for an existing config mode. (AGENTS.md:12, 53bd4dde6c00)
  • Current main default-list leak path: Plain models list does not enable source-plan cascade and calls appendConfiguredModelRowSources, whose default path appends authenticated catalog rows after configured rows. (src/commands/models/list.list-command.ts:118, 53bd4dde6c00)
  • Current main authenticated catalog behavior: Current appendAuthenticatedCatalogRows loads the model catalog and gates rows by provider auth only, with no replace-mode configured-provider guard. (src/commands/models/list.rows.ts:386, 53bd4dde6c00)
  • Documented replace-mode contract: Config help says models.mode: "replace" uses only configured providers, while concepts docs reserve the full built-in catalog for explicit browse views like openclaw models list --all. (src/config/schema.help.ts:978, 53bd4dde6c00)
  • Prior browse finding addressed: At the current PR head, buildRowContext sets browse: enableSourcePlanCascade, and isProviderAllowed returns true for browse contexts, so explicit --all and provider-filtered paths are no longer blocked by replace mode. (src/commands/models/list.list-command.ts:178, fe51174b8055)
  • Current PR test type failure: Exact-head check-test-types fails with TS2739 because both new test fixtures use models.providers.xai: {} where ModelProviderConfig requires baseUrl and models. (src/commands/models/list.rows.test.ts:232, fe51174b8055)

Likely related people:

  • steipete: GitHub commit metadata ties this account to adding authenticated catalog rows, restoring provider catalog listing, and documenting model-list source behavior. (role: model-list architecture contributor; confidence: high; commits: d5c094f1691a, 56c4f9761c75, 9d7f83b17504; files: src/commands/models/list.rows.ts, src/commands/models/list.row-sources.ts, docs/cli/models.md)
  • shakkernerd: GitHub commit metadata shows source-plan and provider-filter model-list work that defines the explicit browse behavior this PR now preserves. (role: source-plan and provider-filter contributor; confidence: high; commits: 25dda844b710, 4e4f9204d75c, b418c08a2297; files: src/commands/models/list.source-plan.ts, src/commands/models/list.row-sources.ts, src/commands/models/list.list-command.ts)
  • samson910022: Recent merged work bounded default model browsing while preserving explicit full-catalog browsing, which is the compatibility boundary involved here. (role: adjacent model browse contributor; confidence: medium; commits: 44f45d8729f5; files: src/commands/models/list.rows.ts, src/commands/models/list.rows.test.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (1 earlier review cycle)
  • reviewed 2026-06-22T03:13:23.898Z sha e90e0b5 :: needs real behavior proof before merge. :: [P1] Preserve explicit full-catalog browsing

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jun 19, 2026
This was referenced Jun 19, 2026
Closed
Open
Closed
@aniruddhaadak80
aniruddhaadak80 force-pushed the fix/models-mode-replace-filter-list branch from e90e0b5 to f829437 Compare June 28, 2026 12:41
@aniruddhaadak80

aniruddhaadak80 commented Jun 28, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review - Rebased onto main, added browse bypass context, resolved default list leak, and verified all tests pass on Windows.

@clawsweeper

clawsweeper Bot commented Jun 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@openclaw-barnacle openclaw-barnacle Bot added the triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. label Jun 28, 2026
@clawsweeper clawsweeper Bot added status: 🔁 re-review loop A fresh ClawSweeper review was explicitly requested after the latest review. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 1, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. label Jul 5, 2026
@clawsweeper clawsweeper Bot added status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed status: 🔁 re-review loop A fresh ClawSweeper review was explicitly requested after the latest review. labels Jul 5, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jul 9, 2026
Draft
@steipete

steipete commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Thanks for the detailed tests. Closing this duplicate in favor of #94735 for #94705. The selected branch gates the exact authenticated-catalog append that leaks providers, preserves explicit browse views, and has live CLI proof. This branch broadens filtering across configured/default row sources, still has invalid test fixtures in the type lane, and lacks equivalent live proof.

@steipete steipete added duplicate This issue or pull request already exists close:duplicate Closed as duplicate dedupe:child Duplicate issue/PR child in dedupe cluster labels Jul 13, 2026
@steipete steipete closed this Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

close:duplicate Closed as duplicate commands Command implementations dedupe:child Duplicate issue/PR child in dedupe cluster duplicate This issue or pull request already exists merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

models.mode: "replace" does not filter openclaw models list output

2 participants

@aniruddhaadak80 @steipete