Skip to content

修复服务商带有[1m],运行时不带[1m]导致thinking和effort不生效问题 - #911

Open
zhbdesign wants to merge 1 commit into
NanmiCoder:mainfrom
zhbdesign:patch-9
Open

修复服务商带有[1m],运行时不带[1m]导致thinking和effort不生效问题#911
zhbdesign wants to merge 1 commit into
NanmiCoder:mainfrom
zhbdesign:patch-9

Conversation

@zhbdesign

Copy link
Copy Markdown
Contributor

修复服务商带有[1m],运行时不带[1m]导致thinking和effort不生效问题

Summary

Feature Quality Contract

  • Changed surface:
  • Tests added or updated:
  • Coverage evidence:
  • E2E / live-model evidence:
  • Known risk / rollback:

Verification

  • I ran the relevant local checks, or explained why they do not apply.
  • I added or updated same-area tests for every production behavior change.
  • I ran bun run verify for code changes, including the coverage gate.
  • New or changed executable production lines meet the changed-line coverage threshold, or the blocker/maintainer override is documented.
  • I attached or summarized the quality report path, JUnit/log artifact path, and pass/fail/skip counts.
  • I ran E2E/live smoke for cross-boundary, provider/runtime, desktop chat, agent-loop, native, or release changes, or documented the blocker.

Risk

  • This PR does not touch CLI core paths, or it has maintainer approval for allow-cli-core-change.
  • Production code changes include matching tests, or have maintainer approval for allow-missing-tests.
  • Coverage baseline/threshold changes have maintainer approval for allow-coverage-baseline-change.
  • Quarantined tests still have owners, exit criteria, and unexpired review windows.
  • Provider/runtime changes were covered by mock contract tests, and live smoke was run or explicitly deferred.

@dosubot review this PR for changed-area risk, missing tests, docs impact, desktop startup risk, and CLI core impact.

修复服务商带有[1m],运行时不带[1m]导致thinking和effort不生效问题
@dosubot dosubot Bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Jun 25, 2026
@github-actions

Copy link
Copy Markdown

PR quality triage

Changed areas: area:cli-core

CLI core policy: Blocked by policy until a maintainer applies allow-cli-core-change and approves the PR.

Missing-test policy: Blocked by policy until a maintainer applies allow-missing-tests or matching tests are added.

Coverage baseline policy: No coverage-baseline policy block detected.

CLI core files:

  • src/utils/model/modelSupportOverrides.ts

Coverage policy files:

  • none

Expected checks:

  • change-policy
  • server-checks
  • coverage-checks

Test coverage signals:

  • BLOCKING unless allow-missing-tests is applied: Agent/runtime product files changed without a tools/utils test file in the PR.
  • Agent/model runtime path changed: use mock/request-shape tests in PR and maintainer live-model smoke before release.

Risk notes:

  • No special risk notes from changed paths.

Hard merge gates still come from GitHub Actions, not AI review.

Dosu handoff: Dosu can be used as the AI reviewer for risk explanation, missing-test prompts, and maintainer Q&A. If it does not comment automatically from the PR template, ask:

@dosubot review this PR for changed-area risk, missing tests, docs impact, desktop startup risk, and CLI core impact.

@dosubot

dosubot Bot commented Jun 25, 2026

Copy link
Copy Markdown

Here's my review of PR #911:

Changed-Area Risk

This PR modifies the get3PModelCapabilityOverride function (and its memoization key) to strip [1m]/[2m] context-window suffixes before comparing model names. The change is localized and the logic is straightforward — a regex replace on a string before comparison [1].

Risk level: Low-Medium. The regex /\[(1|2)m\]/gi is simple and only targets the known suffixes. However, since get3PModelCapabilityOverride gates whether thinking and effort capabilities are enabled for third-party models, a regression here could silently disable those features. The memoization cache key change is correct — without it, a cached result under the old (un-stripped) key would cause stale lookups.

Missing Tests

This is a concern. The PR labels include allow-missing-tests, but there are no tests added or updated for the new stripContextSuffix() function or for the updated matching logic. Recommended test cases:

  1. Model name with [1m] suffix matches pinned env var without suffix
  2. Model name without suffix matches pinned env var with [1m] suffix
  3. Model name with [2m] suffix works correctly
  4. Case-insensitive matching still works (e.g., [1M])
  5. Model name with no suffix still matches correctly (no regression)
  6. Memoization cache key produces the same result regardless of suffix presence

Docs Impact

None. This is an internal runtime fix for model capability matching. No user-facing documentation changes are needed.

Desktop Startup Risk

None. The change is in capability override resolution logic, not in startup paths, initialization, or UI rendering.

CLI Core Impact

The PR is labeled allow-cli-core-change. The function get3PModelCapabilityOverride is part of the model capability resolution path which is used by the CLI agent loop. The impact is minimal — it only changes how model names are compared, not the control flow. The fix is additive (stripping suffixes) and falls back to the same behavior when no suffix is present.


Summary: The fix is sound and well-scoped. The main gap is the lack of tests — even a few unit tests for stripContextSuffix and the updated matching logic would significantly reduce regression risk. I'd recommend adding those before merge unless there's a reason to fast-track this.


Leave Feedback Ask Dosu about cc-haha

@NanmiCoder
NanmiCoder self-requested a review as a code owner July 28, 2026 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:cli-core needs-maintainer-approval size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants