Codex review: needs maintainer review before merge. Reviewed August 3, 2026, 2:10 PM ET / 18:10 UTC.

ClawSweeper review

What this changes

The PR updates openclaw models scan to prefer valid OpenRouter top_provider context and completion limits, with valid catalog-wide fields as independent fallbacks.

Merge readiness

⚠️ Ready for maintainer review - 1 item remains

This PR remains necessary: current main still reads only catalog-wide OpenRouter limits, while the branch correctly normalizes valid top_provider limits first and retains independent catalog fallbacks. The exact-head patch matches the current OpenRouter provider-catalog owner pattern, protects probe construction from invalid numeric metadata, and has focused regression coverage; no blocking correctness or security finding remains.

Priority: P2
Reviewed head: d3c9ac59bbb95539d76b1dd06b58b130fa9bd74f

Review scores

Verification

How this fits together

The model scan fetches OpenRouter’s public model catalog and converts each entry into scan metadata for CLI output and optional capability probes. Normalized context and completion limits flow from the catalog parser into the displayed scan result and the probe model configuration.

flowchart LR
  A[OpenRouter model catalog] --> B[Model scan fetch]
  B --> C[Limit normalization]
  C --> D[Scan model records]
  D --> E[CLI scan output]
  D --> F[Capability probes]

Before merge

• Complete next step (P2) - No repair job is needed: the active assigned PR already contains the focused fix and has no actionable review finding.

The precedence flip is right and it converges on the in-repo sibling, but asFiniteNumber lets 0 through and ?? doesn't fall through on 0 — so a junk top_provider value now outranks a good top-level one.

// packages/normalization-core/src/number-coercion.ts:2-4
export function asFiniteNumber(value: unknown): number | undefined {
  return typeof value === "number" && Number.isFinite(value) ? value : undefined;
}

For { context_length: 32_768, top_provider: { context_length: 0 } } that yields contextLength: 0, and the value isn't only reported — model-scan.ts:507-508 builds the probe model from it:

contextWindow: entry.contextLength ?? baseModel.contextWindow,
maxTokens: entry.maxCompletionTokens ?? baseModel.maxTokens,

0 ?? baseModel.contextWindow is 0, so the probe runs with a zero window instead of the base model's. Negatives and fractions pass the same way.

I want to be careful about how much this proves, because the loose validation isn't new — main already accepted 0 on the top-level field. What's new is the precedence: before this PR the good top-level value was the only one read, so a bad top_provider couldn't shadow it. That's the part I'd call a new failure mode rather than a pre-existing one.

And I can't show you a captured OpenRouter payload containing 0 — the shape I've seen is null, which your code handles correctly (asFiniteNumber(null) → undefined → falls through). So this is a reading of the input contract, not a measurement of it. What makes me think it's worth guarding anyway is that the repo's other OpenRouter reader already treats these exact inputs as expected junk: extensions/kilocode/provider-models.ts:121-123 uses asPositiveSafeInteger, and extensions/kilocode/provider-models.test.ts:240-270 fixtures top_provider: { max_completion_tokens: 0 } and context_length: -1 and asserts fallback to defaults. asPositiveSafeInteger (number-coercion.ts:99-101) exists for this; swapping it in here would close the case and finish the convergence you started — src/agents/embedded-agent-runner/openrouter-model-capabilities.ts:165-170 is the sibling you're already matching on precedence.

On tests: the added fixture covers top_provider smaller than top-level, which is the common case and the right one to pin first. Three neighbours are unpinned, and one has a behavior consequence — 0/null inside top_provider, top_provider larger than top-level, and the mixed case where context_length comes from top_provider while max_completion_tokens falls through to the top level. That last one is worth a fixture on its own, since silently mixing two sources for a single model's limits is hard to spot in the scan output.

None of this is a blocker; reading top_provider first is a clear improvement over the top-level field alone. Whether 0 is worth guarding is your call — you've looked at more of these payloads than I have.

@mushuiyu886 This morning's ClawSweeper pass turned the asFiniteNumber point into the P1 blocking finding and calls the repair mechanical. It is — so rather than restate it, here it is applied and measured on cd15b84b merged with current main (5fc976571e5), paste-ready.

Source change. asPositiveSafeInteger is exported from the same module you already import from, so this adds nothing:

 import {
-  asFiniteNumber,
   asDateTimestampMs,
+  asPositiveSafeInteger,
   resolveTimerTimeoutMs,
 } from "@openclaw/normalization-core/number-coercion";

           const contextLength =
-            asFiniteNumber(topProvider?.context_length) ??
-            asFiniteNumber(obj.context_length) ??
+            asPositiveSafeInteger(topProvider?.context_length) ??
+            asPositiveSafeInteger(obj.context_length) ??
             null;

           const maxCompletionTokens =
-            asFiniteNumber(topProvider?.max_completion_tokens) ??
-            asFiniteNumber(obj.max_completion_tokens) ??
-            asFiniteNumber(obj.max_output_tokens) ??
+            asPositiveSafeInteger(topProvider?.max_completion_tokens) ??
+            asPositiveSafeInteger(obj.max_completion_tokens) ??
+            asPositiveSafeInteger(obj.max_output_tokens) ??
             null;

Measured, same tree, same fixtures. Top-level context_length: 32768 / max_completion_tokens: 4096 in every row; only top_provider varies:

The two control rows are the ones that matter for scope: a well-formed top_provider still wins, and null still falls through, so the guard rejects malformed values without touching the precedence flip this PR is actually about.

Fixtures, including the mixed-field case the review asks for. Both fail on your current head — expected +0 to be 32768 and expected +0 to be 4096 — and pass with the change above, so they're load-bearing rather than descriptive:

  it("falls back when top-provider limits are malformed", async () => {
    for (const topProvider of [
      { context_length: 0, max_completion_tokens: 0 },
      { context_length: -1, max_completion_tokens: -1 },
      { context_length: 8192.5, max_completion_tokens: 8192.5 },
      { context_length: null, max_completion_tokens: null },
    ]) {
      const fetchImpl = createFetchFixture({
        data: [
          {
            id: "acme/provider-limited:free",
            name: "Provider Limited",
            context_length: 32_768,
            max_completion_tokens: 4096,
            top_provider: topProvider,
            supported_parameters: [],
            pricing: { prompt: "0", completion: "0" },
          },
        ],
      });

      const [result] = await scanOpenRouterModels({ fetchImpl, probe: false });

      expect(result?.contextLength).toBe(32_768);
      expect(result?.maxCompletionTokens).toBe(4096);
    }
  });

  it("mixes provider context length with a top-level completion cap", async () => {
    const fetchImpl = createFetchFixture({
      data: [
        {
          id: "acme/provider-limited:free",
          name: "Provider Limited",
          context_length: 32_768,
          max_completion_tokens: 4096,
          top_provider: { context_length: 16_384, max_completion_tokens: 0 },
          supported_parameters: [],
          pricing: { prompt: "0", completion: "0" },
        },
      ],
    });

    const [result] = await scanOpenRouterModels({ fetchImpl, probe: false });

    expect(result?.contextLength).toBe(16_384);
    expect(result?.maxCompletionTokens).toBe(4096);
  });

Validation with everything applied:

node scripts/run-vitest.mjs run src/agents/model-scan.test.ts src/commands/models/scan.test.ts
Tests  13 passed (13)   # model-scan.test.ts, 11 before the two added cases
Tests  10 passed (10)   # commands/models/scan.test.ts, unchanged

./node_modules/.bin/oxfmt --check src/agents/model-scan.ts   All matched files use the correct format.
./node_modules/.bin/oxlint src/agents/model-scan.ts          clean

Take it or adapt it — I'm not attached to the fixture wording, and the null row is worth keeping whichever way you write them, since it's the one that proves the guard didn't just start rejecting everything.

@clawsweeper re-review

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

• State: Complete
• Detail: The durable review result and its route handoff completed.
• Run: https://github.com/openclaw/clawsweeper/actions/runs/30732217159
• Updated: 2026-08-02T04:21:21.412Z

@mushuiyu886 Heads-up: this branch started conflicting a few hours ago, and the review comment above predates it — it says GitHub reports the PR mergeable, but the API now returns mergeable: false, mergeable_state: "dirty". Your work is not obsolete; the cause is mechanical and the resolution is small, so here it is measured.

What moved. 33ea3e16e984 — refactor: consolidate core micro-helpers (#117825), merged today at 06:10 UTC — extracted withTimeout out of src/agents/model-scan.ts into src/node-host/with-timeout.js. That removed one nesting level, so the whole return entries.map(...) chain shifted two spaces left. It is the only commit to touch this file since your July 20 base.

Three-way merge against main at 7068bc2ffe5:

$ git merge-tree --write-tree --name-only origin/main <pr-head>
src/agents/model-scan.ts
CONFLICT (content): Merge conflict in src/agents/model-scan.ts

The conflicting hunk is your two limit blocks against the re-indented ones. No logic disagreement — both sides changed the same lines for unrelated reasons.

The fix is still needed. Current main has no top_provider read at all:

const contextLength =
  typeof obj.context_length === "number" && Number.isFinite(obj.context_length)
    ? obj.context_length
    : null;

So the top-level-only behaviour your PR corrects is exactly what main still does.

Resolution. asPositiveSafeInteger is still exported from packages/normalization-core/src/number-coercion.ts:99, and model-scan.ts still imports from that module, so this is an addition to the existing list:

 import {
   asDateTimestampMs,
+  asPositiveSafeInteger,
   resolveTimerTimeoutMs,
 } from "@openclaw/normalization-core/number-coercion";

Then, at main's new indentation, replace the two limit blocks with:

            const topProvider =
              obj.top_provider && typeof obj.top_provider === "object"
                ? (obj.top_provider as Record<string, unknown>)
                : undefined;

            const contextLength =
              asPositiveSafeInteger(topProvider?.context_length) ??
              asPositiveSafeInteger(obj.context_length) ??
              null;

            const maxCompletionTokens =
              asPositiveSafeInteger(topProvider?.max_completion_tokens) ??
              asPositiveSafeInteger(obj.max_completion_tokens) ??
              asPositiveSafeInteger(obj.max_output_tokens) ??
              null;

I applied exactly that on top of current main and checked it: npx oxlint src/agents/model-scan.ts exits 0, and npx oxfmt --check src/agents/model-scan.ts reports "All matched files use the correct format" — so the indentation above is the formatter's own, not my guess at it.

What I did not check: I did not run the test file through the merged tree, so treat the source resolution as verified and the suite as still owing a run once you push. Your existing top_provider cases should be unaffected — the refactor touched nesting, not behaviour — but that is worth confirming rather than assuming.

Resolved the current-main conflict in aefc09cf3eb8 while retaining main's new withTimeout(work, timeoutMs, label) contract and the provider-limit normalization.

Post-merge verification:

• Node 24.15.0 focused run: 25 tests passed across src/agents/model-scan.test.ts and src/commands/models/scan.test.ts.
• Targeted formatting, lint, and diff checks passed.
• Latest ClawSweeper review is on aefc09cf3eb8 and reports no actionable findings.

Exact-head CI passed every product/check lane except build-artifacts. Its failure is in the unrelated TUI PTY test src/tui/tui-pty-local.e2e.test.ts: the first prompt did not reach the model. The PR diff only changes the two model-scan files, and the same build-artifacts job passed on current-main run https://github.com/openclaw/openclaw/actions/runs/30746443912/job/91493362209.

I attempted to rerun the failed jobs, but GitHub rejected the contributor token because reruns require repository admin rights. A maintainer rerun of https://github.com/openclaw/openclaw/actions/runs/30747370605 is the remaining CI action; no product-code change is indicated by this failure.

@clawsweeper re-review

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

• State: Complete
• Detail: The durable review result and its route handoff completed.
• Run: https://github.com/openclaw/clawsweeper/actions/runs/30840200113
• Updated: 2026-08-03T18:12:34.969Z

Maintainer review complete for exact head d3c9ac59bbb95539d76b1dd06b58b130fa9bd74f.

• ClawSweeper's latest review has no findings, no security concern, and sufficient public live-output proof.
• The sole before-merge item was normal maintainer review; that is complete.
• Focused exact-head tests pass 56/56, direct scoped formatting/lint pass, and required openclaw/ci-gate is green.
• Fresh current-main integration at 05dc73bb35fd6ef1dc56ec82f0e913476d438412 has zero touched-path overlap and a conflict-free merge tree.
• The fix remains production-net-neutral and does not change config, SDK, persistence, protocol, or feature surfaces.

Proceeding through the repository's native prepare and merge verification flow.

Merged via squash.

• Prepared head SHA: d3c9ac59bbb95539d76b1dd06b58b130fa9bd74f
• Landed commit: 8fa1b82e42a35447482f2722df75128916d0c059