feat(providers): per-model carve-out for reasoning_effort gating

[ausard]

Closes #819

Summary

Replaces the unconditional reasoning_effort write in buildDeepSeekRequest with a per-model capability dispatch backed by a new modelSupportsReasoningEffort(modelId) helper in src/providers/model-match.ts.

Changes

• src/providers/model-match.ts: Adds exported ReasoningEffortMode type ('none' | 'binary' | 'levels') and modelSupportsReasoningEffort(modelId) helper. Currently returns 'levels' for any id containing deepseek (case-insensitive) and 'none' otherwise. The 'binary' branch is wired into buildDeepSeekRequest so future binary-only thinking models can be carved out with one string match here.
• src/providers/deepseek.ts: DeepSeekOptions now accepts optional modelId. The reasoning_effort block dispatches on the helper:
  • 'levels' -> request.reasoning_effort = options.reasoningEffort (existing behaviour).
  • 'binary' -> request.enable_thinking = true (no reasoning_effort).
  • 'none' -> field is dropped.
  • When modelId is omitted, defaults to 'levels' to keep existing DeepSeek behaviour bit-for-bit identical.
• Tests: New src/providers/__tests__/deepseek.test.ts covers all three branches (the 'binary' branch via a vi.mock of modelSupportsReasoningEffort) plus the modelId-omitted default. New src/providers/__tests__/model-match.test.ts covers DeepSeek match, case-insensitivity (DeepSeek-R1 -> 'levels'), and the unknown-id default.

Why

buildDeepSeekRequest previously forwarded reasoningEffort blindly. The moment routing-config / multi-model work (#813) lets users name arbitrary reasoning-style deployments, that blanket write would either be silently ignored or rejected by the provider. opencode learned this with GLM-4 -> 5.2 (sst/opencode#32446, 2026-06-20) where a id.includes('glm') gate silently dropped High/Max variants. Pre-empting it here in one helper + tests is cheaper than three-file refactor after a regression.

Verification

npm run lint          # 0 errors
npm run typecheck     # green
npm run format:check  # green
npm run test:coverage # 192 files, 2704 tests passing
npm run build         # green

No @ts-ignore, eslint-disable, or new as any introduced.

[alexi-bot]

[github-actions]

Coverage Report

Metric	Coverage
Lines	61.06%
Statements	61.09%
Functions	56.91%
Branches	55.81%

Coverage Details

• Lines: 7372/12073
• Statements: 7677/12565
• Functions: 1161/2040
• Branches: 4115/7373

[ausard]

Security Scan — PR #821

Role: Security vertical (.github/prompts/role-security.md). Trigger: pull_request. Base: origin/master.

Summary

Diff is tightly scoped to the DeepSeek provider and a new modelSupportsReasoningEffort helper in src/providers/model-match.ts, plus two new test files under src/providers/__tests__/. Total: 4 files, +132 / -2.

• No changes to package.json / package-lock.json — no new dependencies, no transitive surface change, no lockfile drift.
• No changes under src/permission/**, src/mcp/**, src/tool/**, or .github/**.
• No secret-like tokens in the diff; no long base64 blobs; no password= / clientsecret / BEGIN PRIVATE KEY / ghp_ / sk- / xoxb- matches.
• Provider SDK boundary respected — all changes live in src/providers/, consistent with Constitution principle I (SAP AI Core-First).

Overall: no blocking findings introduced by this PR. The only items below are pre-existing repo-level risks worth tracking separately.

Critical (block)

None.

High (warn)

These are pre-existing npm audit --omit=dev findings on master. They are not introduced by this PR and must not block merge of #821, but should be tracked in a dedicated dependency-hardening issue.

npm audit --omit=dev summary: 0 critical, 8 high, 4 moderate, 0 low, 0 info across 12 advisories.

Production-tree advisories with fixAvailable: true (transitive, can be lifted by a clean lockfile refresh):

• axios — multiple high CVEs (prototype pollution, Proxy-Authorization leak across redirects, NO_PROXY bypass on IPv4-mapped IPv6, ReDoS, header injection).
• @hono/node-server — authorization bypass for protected static paths via encoded slashes / repeated slashes.
• @xmldom/xmldom — XML injection in DocumentType / PI / comment serialization, DoS via uncontrolled recursion.
• form-data — CRLF injection via unescaped multipart field names.
• path-to-regexp — ReDoS via sequential optional groups and multiple wildcards.
• fast-uri — path traversal and host confusion via percent-encoded segments.
• express-rate-limit — per-client bypass on dual-stack via IPv4-mapped IPv6.

Direct dependencies with findings:

• gray-matter (moderate, fixAvailable: true).
• xlsx (high, fixAvailable: false) — Prototype Pollution + ReDoS in SheetJS. This one has no fix in the npm registry (upstream publishes to its own CDN). It is a standing risk: confirm it is only used on trusted spreadsheet inputs, or scope its usage behind a feature flag.

Info

• package-lock.json is unchanged in this PR (verified with git diff --stat origin/master...HEAD -- package.json package-lock.json). Good — no surprise transitive surface change.
• marked resolves to 15.0.12 and marked-terminal to 7.3.0 in the lockfile. The known repo-specific block on marked@>=16 is not triggered here.
• The added modelSupportsReasoningEffort helper in src/providers/model-match.ts:172 does a case-insensitive includes() match. It is non-secret-bearing and side-effect free; safe to import from tests.
• The mock in src/providers/__tests__/deepseek.test.ts:9 mocks ../model-match.js and re-imports actual via vi.importActual before the SUT import — correct ordering per the project's ESM + vitest hoisting rules.
• All new imports correctly include the .js suffix, preserving ESM runtime correctness.
• New enable_thinking: true branch in src/providers/deepseek.ts:147 only fires when mode === 'binary', which the current modelSupportsReasoningEffort implementation never returns (the binary branch is reserved for future model carve-outs). No live model is affected; default behaviour is preserved bit-for-bit when modelId is omitted.
• No provider-SDK call introduced outside src/providers/. Principle I upheld.

Recommendations

1. Merge feat(providers): per-model carve-out for reasoning_effort gating #821 as-is from a security standpoint. No new risk surface, no secrets, no permission changes.
2. Open (or extend) a tracking issue labelled security for the npm audit findings above. Group the transitive fixAvailable: true advisories into a single lockfile-refresh PR; treat xlsx separately as a usage-audit task.
3. When the first real binary-mode model is wired into modelSupportsReasoningEffort, add a router-side test that asserts enable_thinking: true is only sent for that model id (defensive — enable_thinking semantics differ between providers, and a wrong dispatch could leak a thinking trace to a model that returns it verbatim in content).

---

Scan basis: git diff origin/master...HEAD (180-line patch), npm audit --omit=dev --json, lockfile inspection for marked / marked-terminal. Secret patterns checked: AICORE_SERVICE_KEY, clientsecret, BEGIN PRIVATE KEY, xoxb-, ghp_, sk-, password=, and any base64 run >=200 chars. No matches.

[ausard]

Architecture Review -- PR #821

feat(providers): per-model carve-out for reasoning_effort gating
Head: auto/819-provider-per-model-carve-out-for-reasoning-effort- -> master
Files changed: 4 (+132 / -2), all under src/providers/

Posted by the Architecture vertical of the Alexi T-shape factory.
Scope: package boundaries, dependency direction, module decomposition.
Not a feature/code-quality review; see quality and engineering
verticals for those.

Summary

This PR is structurally contained and architecturally sound. It touches
only src/providers/, keeps the provider abstraction intact, and does
not introduce new top-level modules, new dependencies, or upward
imports. No ADR is required.

The change adds a new pure helper modelSupportsReasoningEffort in the
existing src/providers/model-match.ts and consumes it from
src/providers/deepseek.ts. Both files belong to the provider layer, so
the new edge stays inside src/providers/ and does not touch
src/core/, src/cli/, or src/agent/.

Layering scan (forbidden upward imports, ESM .js discipline, core->SDK
leakage) is clean for this diff. The pre-existing layering findings
catalogued in docs/adr/REVIEW-2026-06-15.md (F1..F12) are unchanged by
this PR.

Findings

F-821.1 info -- Provider abstraction preserved

src/providers/deepseek.ts:6 adds import { modelSupportsReasoningEffort } from './model-match.js'. Both files live under src/providers/;
the new dispatch logic is internal to the provider layer and does not
leak into src/core/ or src/cli/. Consumers of buildDeepSeekRequest
remain untouched -- modelId is an optional field with a documented
default ("levels") that preserves existing behaviour bit-for-bit.

This is exactly the shape the constitution III provider-abstraction rule
wants: per-model SDK quirks live behind the provider surface, and the
rest of the codebase keeps talking to the generic interface.

F-821.2 info -- No new top-level package

ls -1 src/ | sort against the module map in role-architecture.md
shows no additions. The PR only modifies/creates files in the existing
src/providers/ directory. No ADR is required.

F-821.3 info -- ESM .js import discipline

The new import (./model-match.js) and the test-side imports
(../deepseek.js, ../model-match.js) all carry the .js suffix
required by module: NodeNext. Repo-wide scan of relative imports
without .js returned only test-fixture string literals in
src/context/__tests__/ranking.test.ts -- not real imports, and
unrelated to this PR.

F-821.4 info -- Tests stay inside the package they cover

Both new tests live under src/providers/__tests__/, consistent with
the project policy that tests may live in either tests/** or
src/**. The mock pattern (vi.mock('../model-match.js', async () => { const actual = await vi.importActual ... })) hoists correctly and
preserves the real implementation for non-mocked exports. This matches
the convention documented in AGENTS.md.

F-821.5 warn -- Carve-out helper has no live caller wiring yet

modelSupportsReasoningEffort is consumed only by
buildDeepSeekRequest, and buildDeepSeekRequest accepts modelId as
an optional parameter. A repo-wide search shows no current caller of
buildDeepSeekRequest is passing modelId -- meaning today the new
branch is unreachable in production code, and the dispatch is exercised
only by the new unit tests.

This is intentional ("preserves existing DeepSeek behaviour bit-for-bit
when modelId is omitted") and the PR description frames it as a
carve-out, but it does create a small risk: if the upstream caller is
later modified to pass modelId without auditing this dispatch, the
silent default flips from "send reasoning_effort always" to "only
send when the model matches deepseek". For any non-DeepSeek model id
threaded through buildDeepSeekRequest, the field will be dropped.

This is not blocking -- buildDeepSeekRequest is the DeepSeek
provider builder, so passing a non-DeepSeek modelId is itself
suspicious. But it deserves a short comment at the call site once
wiring lands. Architecturally I would also accept a stricter signature
that requires modelId and asserts isDeepSeek(modelId) at the entry,
to make the assumption explicit. Hand-off to role-engineering.

F-821.6 info -- 'binary' branch is dead code today

The ReasoningEffortMode = 'none' | 'binary' | 'levels' type and the
else if (mode === 'binary') branch in deepseek.ts are pre-emptive:
modelSupportsReasoningEffort never returns 'binary' for any real
model id. The comment on line 112-113 of model-match.ts calls this
out explicitly ("Pre-emptive carve-out for future binary-only thinking
models"). That is fine from a layering standpoint -- the type lives in
the same file as its producer, the binary branch is fully unit-tested
via a mocked match function, and there is no cross-package leakage.

Flagging only so that whoever adds the first concrete 'binary' model
substring updates both the matcher and the doc comment at the same time.

F-821.7 info -- No routing-config.json schema change

grep confirms the PR does not touch routing-config.json,
routing-config.example.json, or the routing schema in
src/config/routingConfig.ts. The carve-out is purely a provider-side
detail and does not need to surface in the router config. Good.

Recommendations

1. Non-blocking, follow-up issue (engineering): when wiring this
   dispatch into the actual call path, decide whether
   buildDeepSeekRequest should require modelId (strict) or keep it
   optional with the current 'levels' default. The current shape is
   defensible either way; document the choice at the call site.
2. When the first non-DeepSeek 'binary' model lands, update both
   the substring list in modelSupportsReasoningEffort and the JSDoc
   above it, and add a unit test entry under
   src/providers/__tests__/model-match.test.ts for that substring.
   No ADR needed -- this is a localised provider-quirk table, not a
   structural decision.
3. No action required for layering. Cross-package import scan,
   provider-SDK leakage scan, and ESM .js discipline are all clean
   for this diff.

ADR needed?

No.

• No new top-level src/<package>/ directory.
• No change to the provider interface used outside src/providers/.
• No change to routing-config.json or its schema.
• No change to the layering rules in role-architecture.md or
  docs/ARCHITECTURE.md.

The PR is approvable from the Architecture vertical's standpoint.
Pre-existing structural findings F1..F12 from
docs/adr/REVIEW-2026-06-15.md remain open and are unrelated to this
change.

[ausard]

Review - role-quality

• typecheck
• lint (0 errors, warnings unchanged from master)
• format
• tests (11/11 new tests passing; coverage delta: +100% on deepseek.ts itself; overall lines 61.15%, well above the 40% CI floor)
• build (implicit via tsc)

Scope

PR adds a per-model carve-out for the reasoning_effort field in buildDeepSeekRequest:

• New modelSupportsReasoningEffort(modelId) -> 'none' | 'binary' | 'levels' in src/providers/model-match.ts:107.
• buildDeepSeekRequest (src/providers/deepseek.ts:49) dispatches on that mode:
  • levels -> existing reasoning_effort: 'low'|'medium'|'high'.
  • binary -> enable_thinking: true (drops the level).
  • none -> field is not forwarded.
  • When modelId is omitted -> defaults to levels (preserves existing behaviour bit-for-bit).

Findings

Correctness:

• The bit-for-bit fallback when modelId is omitted is the right call given the function name and the absence of production callers passing modelId today (verified via grep — only tests call buildDeepSeekRequest). No silent behaviour change for existing consumers.
• Case-insensitive includes('deepseek') mirrors the rest of model-match.ts (isDeepSeek, getModelFamily) — consistent.

Security: no new untrusted-input surface, no eval, no fs, no network.

Tests:

• Mock placement in deepseek.test.ts is correct (vi.mock above the import).
• All five reasoning_effort branches are covered (levels-default, levels-explicit, binary, none, unset), plus the pre-existing max_tokens paths are regression-tested.
• modelSupportsReasoningEffort unit test covers deepseek match, case-insensitivity (including sap-ai-core/ prefix), unknown id, and empty string.
• One gap: the 'binary' mode has no production model id mapped to it yet — the branch in modelSupportsReasoningEffort is currently unreachable in real callers. That is documented inline ("Pre-emptive carve-out... Add concrete model substrings here when wired into the router."). Acceptable as a forward-looking surface since the PR title is explicitly the carve-out, but worth tracking in a follow-up issue when the first binary-thinking model is wired in.

Style / typing:

• Pre-existing request.max_completion_tokens = 'max' as any; is untouched — not in this PR's scope.
• enable_thinking is not declared on ProviderRequest but the index signature [key: string]: unknown accepts it. Fine.
• No @ts-ignore, no eslint-disable, no console. ESM .js import extension is present.

Performance: O(1) string .includes on the model id; no hot-path impact.

Coverage delta:

• src/providers/deepseek.ts: 100% lines / 100% branches.
• src/providers/model-match.ts: the new function is fully covered; the file's overall low % is pre-existing and not regressed by this PR.

Changes made

none

Verdict

Approved