Repository navigation
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
|
Thanks for the contribution. This PR still needs the requested real-behavior proof, and the branch has been idle since that ask. Close as stalled awaiting real-provider proof. The dated proof request remains unmet after more than 20 days without head or check activity, and no maintainer has engaged; the useful underlying issue remains open. Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. So I’m closing this for now to keep the review queue honest. Please reopen or open a fresh PR with real-behavior proof (a live run, logs, or a reproducible validation transcript) and it will be reviewed again. Review detailsBest possible solution: Retain the open bug report and return with a compact endpoint-scoped fix that preserves explicit overrides and demonstrates actual cap enforcement. Do we have a high-confidence way to reproduce the issue? Yes for the request-field mismatch: current source deterministically selects the undocumented field, and before/after unit results support that path. Actual provider rejection or ignored limits remain unobserved. Is this the best way to solve the issue? Acceptable mitigation, but not established as the best final shape: the host/path restriction is appropriate and preserves overrides; a plugin-metadata solution would require additional contract work, while per-model overrides would not repair the default. Security review: Security review cleared: The diff changes request-field selection and tests without introducing credential, authorization, dependency, or supply-chain changes. AGENTS.md: found and applied where relevant. What I checked:
Likely related people:
Codex review notes: model internal, reasoning medium; reviewed against 1adb39eebaad. |
|
Thanks — the blast-radius point is the right one to raise, and it is sharper than my PR body allowed for. Two answers: one honest limitation, one narrowing of the claim. I cannot supply real-provider proofI have no Alibaba Cloud Model Studio credentials, so I cannot produce a redacted live request/response. I would rather say that plainly than dress up builder output as something it is not. Everything in the PR is resolver- and request-builder level, and I labelled it as such. If a maintainer or the reporter of #127119 can run one call against The endpoint boundary is narrower than "the whole family", and I should have said soThe predicate covers 6 provider IDs (
The Model Studio OpenAI-compatibility reference documents the Two ways forward, your callA — keep the family whole (current PR). Argues that the codebase already treats these six as one family for B — narrow to the documented surface. Gate I lean A, for one reason worth stating explicitly as reasoning rather than measurement: Tell me which you want and I will push it. If you prefer to hold for live proof, leave it as-is and I will keep the branch current. |
9dd72f1 to
a35b791
Compare
…n_tokens Model Studio's OpenAI-compatibility reference documents max_tokens and does not list max_completion_tokens, but resolveOpenAICompletionsCompatDefaults built usesMaxTokens from every other non-OpenAI-shaped family and left this one out, so the cap shipped under a field the vendor does not document. The resolver already recognises the family: isModelStudioLike gates requiresNonEmptyUserOrAssistantMessage, and isMoonshotLike (which includes it) gates supportsDeveloperRole. usesMaxTokens is the one place it was missing. Measured through buildOpenAICompletionsParams, with Moonshot/Together as non-OpenAI controls and OpenAI as the OpenAI-shaped control: qwen max_completion_tokens -> max_tokens dashscope max_completion_tokens -> max_tokens modelstudio max_completion_tokens -> max_tokens moonshotai max_tokens -> max_tokens together max_tokens -> max_tokens openai max_completion_tokens -> max_completion_tokens Four existing tests in openai-completions-params.test.ts used dashscope as an incidental fixture while asserting on the field name. They are about which value wins (model params vs model maxTokens vs runtime maxTokens) and about CJK clamping, not about the spelling, so the assertions move to max_tokens and keep their values. A configured params.max_completion_tokens is still honoured — the value is carried onto whichever field the provider uses, verified for both spellings. Closes openclaw#127119
|
Took option B. Pushed as I offered A or B above and leaned A. The P1 is right and my lean was wrong, for a reason I could have checked at the time: every case in the reporter's own repro in #127119 is on a So
This still closes #127119 in full, and now everything it changes is covered by the citation. The real-behaviour blocker no longer applies to the changed surface — there is nothing left in the diff that rests on my inference rather than on Alibaba's reference. Coding Plan keeps today's behaviour, so no proof is needed for it either. Matching the path segment rather than listing six URLs, so a new Alibaba region does not silently fall out of the documented surface. What I did not do, deliberately. The recommendation also proposes adding a generic endpoint-level plugin metadata capability for the output-token field. I agree that is the better long-term shape — the manifest declaring one Verification, all runnable without provider credentials: The compatible-mode case now covers all four documented endpoints; two new cases pin Coding Plan on One note on the test fixture, since it shows in the diff: the token-plan URLs are driven through provider
The earlier live-verification failure in the review was infrastructure, not the suite: |
…ible-mode surface The first version gated on isModelStudioLike, which resolves through the `modelstudio-native` endpoint class. That class covers six base URLs and they are not one surface: https://dashscope.aliyuncs.com/compatible-mode/v1 documented https://dashscope-intl.aliyuncs.com/compatible-mode/v1 documented https://token-plan.cn-beijing.maas.aliyuncs.com/compatible-mode/v1 documented https://token-plan.ap-southeast-1.maas.aliyuncs.com/compatible-mode/v1 documented https://coding.dashscope.aliyuncs.com/v1 Coding Plan, NOT documented https://coding-intl.dashscope.aliyuncs.com/v1 Coding Plan, NOT documented Alibaba's OpenAI-compatibility reference describes the `/compatible-mode/v1` surface. It says nothing about the two Coding Plan endpoints, so the citation behind this change does not reach them, and I have no Coding Plan key to test with. Switching them anyway was me widening the fix past its evidence. usesMaxTokens now requires Model Studio AND a `/compatible-mode/` path. Coding Plan keeps max_completion_tokens until someone with a key can confirm. This still closes openclaw#127119 in full: every case in the reporter's repro is on a /compatible-mode/v1 URL. Matching the path segment rather than a list of six URLs so a new Alibaba region does not silently drop out of the documented surface. Tests: the compatible-mode case now covers all four documented endpoints, and two new cases pin Coding Plan on max_completion_tokens. Reverting to the wide predicate fails both of those. The token-plan URLs are driven through provider "qwen" because the unit fixture's endpoint-class map only knows dashscope.aliyuncs.com; in production the plugin manifest lists all six under modelstudio-native, and either way the path gate is what decides. Addresses the P1 "restrict the field switch to documented endpoint classes" from the ClawSweeper review, without adding the endpoint-level plugin metadata capability it also suggests — that is plugin API surface and belongs to a maintainer decision, not to this fix.
|
CI on
The Coding Plan
I would rather give you the evidence than a verdict:
So: strong structural argument, no local reproduction. If it is a known flake on |
check-lint failed on `oxfmt --check`: the Coding Plan it.each block I added in f79309c exceeded the line budget, so the formatter wants the description and callback on their own lines. Formatting only — 25/25 still pass.
…substring A Qwen-family provider accepts an arbitrary custom baseUrl, so testing the path alone handed max_tokens to any route serving /compatible-mode/ — including a proxy Alibaba does not own and the compatibility reference does not describe. The gate now reads the host as well, reusing the shapes extensions/qwen/video-generation-provider.ts already matches, so a future Alibaba region stays inside the documented surface while an unrelated route does not enter it. Three of the four new cases fail on the previous commit.
…t holds cn-hongkong.dashscope, dashscope-us and workspace.ap-southeast-1.maas each serve /compatible-mode/v1 and are named elsewhere in this repository, but none of them is in MODELSTUDIO_NATIVE_BASE_URLS. Matching the host reaches all three; a fixed URL list would leave every one on the wrong field.
|
Pushed P1 — the field policy no longer rests on a path substringYou were right, and the shape of it is worse than "an unrecognized proxy route". The gate was isModelStudioLike && input.baseUrl?.includes("/compatible-mode/") === truewhich decides what a route is from how its URL is spelled. A Qwen-family provider takes an arbitrary It now reads the host as well, reusing the shapes Three of the four new negative cases fail on the parent commit. That is the regression you named:
The fourth passes both ways. It guards behaviour that was already correct rather than fixing anything, and I would rather label it than let it count as four. Why the host, and not a Qwen-owned URL listYou asked me to move this to Qwen-owned route handling. I kept it in the transport, and this is the measurement that decided it — I would rather show it than argue. I ran the new predicate over every Alibaba base URL this repository names — Seven of those are compatible-mode surfaces.
A URL list leaves all three on If you would still rather this live in the Qwen plugin, say so and I will move it with the host predicate intact. My argument is about the list, not about where the code sits. Coding Plan is untouched: The live trace — I cannot produce itI do not have an Alibaba Model Studio account, so I cannot attach an after-fix request/response pair. I would rather write that sentence than pass a mock off as a trace. What I can attest to: the field on the outgoing body, measured through What remains unverified: that Model Studio honours So, whichever suits you:
I am not asking you to merge on documentation alone. If a live trace is the bar, this is not ready, and I will not argue that it is. The two red checks
Exit 143 is SIGTERM — the runner was shut down mid- |
shojikumaru
left a comment
shojikumaru
left a comment
There was a problem hiding this comment.
Real-setup verification — tested at head e8c2c3d6fcb0c47647373de33015abbe8e4124e0
Setup: isolated checkout (git fetch origin pull/127135/head), Docker node:26, pnpm install --ignore-scripts, no secrets in env, tests run with --network=none. Linux host.
After (head e8c2c3d6): node scripts/run-vitest.mjs packages/ai/src/transports/openai-completions-params.cache-and-compat.test.ts packages/ai/src/transports/openai-completions-params.test.ts → 45/45 passed (1.92s).
Before (merge-base 75c44b2b): overlaid only the PR's test file(s) onto the merge-base, same command → 12 failed | 33 passed — reproducing the base behavior the PR fixes (failing: "uses CJK-aware input estimates when clamping proxy-like completions output budgets").
Not covered: runtime behavior beyond the unit surface (unit-level proof only).
Verified by Sho (human) + Alpha (AI agent) on a real OpenClaw setup. Happy to re-run against a new head if the branch moves.
|
ClawSweeper recorded closeout evidence for this proposed close.
|
2 participants
Closes #127119
What Problem This Solves
Fixes an issue where users on any Alibaba Cloud Model Studio provider (
qwen,dashscope,modelstudio) would have their output-token cap sent under a field the vendor does not document. Model Studio's OpenAI-compatibility reference listsmax_tokensand does not listmax_completion_tokens, but every request OpenClaw sends to that family carriedmax_completion_tokens.The endpoint is not obliged to honour an undocumented field, so the configured cap can be silently ignored on the vendor side.
Why This Change Was Made
resolveOpenAICompletionsCompatDefaultsbuildsusesMaxTokensfrom each vendor family it knows is not OpenAI-shaped — Chutes, Mistral, Moonshot, Cloudflare AI Gateway, Z.ai, Together — and the Model Studio family was the one omission, so it fell through to themax_completion_tokensdefault.The same function already recognises this family for two other flags:
isModelStudioLikegatesrequiresNonEmptyUserOrAssistantMessage, andisMoonshotLike(which includes it) gatessupportsDeveloperRole. AddingisModelStudioLiketousesMaxTokensreuses the predicate that is already there rather than introducing a second notion of "is this Model Studio".Scope is deliberately that one boolean. No manifest key, no model-catalog field, no new provider concept.
User Impact
Requests to
qwen,dashscopeandmodelstudionow carry the cap asmax_tokens, the field Model Studio documents. A cap configured asparams.max_completion_tokensin a model entry keeps working — the value is carried onto whichever field the provider uses, so no existing configuration needs editing. Nothing changes for OpenAI or any other provider.Evidence
Measured through
buildOpenAICompletionsParams, so this is the field on the actual request body rather than the resolver's opinion. Moonshot and Together are non-OpenAI controls; OpenAI is the OpenAI-shaped control:qwenmax_completion_tokensmax_tokensdashscopemax_completion_tokensmax_tokensmodelstudiomax_completion_tokensmax_tokensmoonshotaimax_tokensmax_tokenstogethermax_tokensmax_tokensopenaimax_completion_tokensmax_completion_tokensTests. Three new cases in
openai-completions-params.cache-and-compat.test.tsfor the three provider ids, plus a guard asserting OpenAI still getsmax_completion_tokensso the change cannot widen.Four existing tests changed, and why.
openai-completions-params.test.tshas four tests that useddashscopeas an incidental fixture while asserting on the field name:uses model params max_completion_tokens ... before model maxTokenskeeps runtime maxTokens ahead of model params max_completion_tokens ...keeps zero runtime maxTokens falling back to model params ...uses CJK-aware input estimates when clamping proxy-like completions output budgetsEach is about which value wins or about CJK clamping, not about the spelling — their neighbours make the same assertions with
xiaomi-token-plan,chutes,stepfun-planandvllm. The assertions keep their values and move tomax_tokens. I checked the value is not lost in the switch: withparams: { max_completion_tokens: 64_000 }the request now carriesmax_tokens: 64000, and the same holds if it is configured asmax_tokens.If any of those four was in fact a deliberate statement that Model Studio wants
max_completion_tokens, then the premise of #127119 is wrong and this PR should be closed — say so and I will. Nothing in the tests' titles, bodies or history suggests it, but they are the one piece of evidence pointing the other way and I would rather flag it than quietly rewrite it.Suite.
packages/aion Node 24.19.0:oxlintandoxfmt --checkclean on all three files.tsc --noEmitonpackages/aireports 41 pre-existing errors, none in the changed files — they are insrc/media/qr-runtime.tsandsrc/system-agent/chat-turn-router.tsand stem from a missing@types/qrcodein my local install.Updated 2026-08-25 (
f79309c1) — narrowed to Alibaba's documented/compatible-mode/v1surface after the P1 endpoint-scope finding. The two Coding Plan endpoints (coding[-intl].dashscope.aliyuncs.com/v1) keepmax_completion_tokens; nothing this PR changes now rests on inference rather than on the vendor reference. Every case in #127119's repro is on a/compatible-mode/v1URL, so the issue is still closed in full. See the comment thread for the endpoint table and test results.