feat(task): task-local runtime thinking effort state with per-request override (DTE series 2/5) - #1523
Conversation
…nvelope (DTE series 2/5)
… override (DTE series 2/5) - Task: setRuntimeThinkingEffort/getRuntimeThinkingEffort with in-memory apiConfiguration merge/restore; per-request metadata at all four createMessage sites; profile-switch re-capture in updateApiConfiguration - Transient state only: never persisted to settings or history - Tests: 8 focused vitest cases (state machine, profile switch, metadata fragment, non-persistence) Part of #35 (DTE-v2 ship plan, unit 3/5).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📜 Recent review details🧰 Additional context used📓 Path-based instructions (5)Check persistence and lifecycle invariants: awaited atomic writes, rollback or explicit partial-failure behavior, cross-window state consistency, stale listeners/watchers, cancellation, idempotency, and safe restart/resume without lost or d...⚙️ CodeRabbit configuration file Files:
Require regression coverage at the lowest valid harness with behavior-focused assertions, including relevant negative, error, false/unset, and boundary cases.⚙️ CodeRabbit configuration file Files:
Check strict typing and exhaustive behavior across normal, boundary, error, cancellation, retry, and compatibility paths.⚙️ CodeRabbit configuration file Files:
Verify extension/webview contracts, cancellation and error propagation, VS Code lifecycle correctness, and behavior under retries and partial failure.⚙️ CodeRabbit configuration file Files:
Act as an adversarial second-opinion reviewer.⚙️ CodeRabbit configuration file Files:
🔇 Additional comments (2)
📝 SummarySummary by CodeRabbit
WalkthroughThe PR adds the ChangesDynamic thinking effort
Stryker Vitest discovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Task
participant resolveEffectiveReasoningEffort
participant APIHandler
User->>Task: setRuntimeThinkingEffort(effort)
Task->>resolveEffectiveReasoningEffort: pass override, settings effort, and model default
resolveEffectiveReasoningEffort-->>Task: return effective reasoning effort
Task->>APIHandler: send request metadata with reasoningEffort
APIHandler-->>Task: stream API response
Merge Risk: ⚪ Minimal · up to Task-local thinking-effort overrides now reset during disposal, preventing disposed tasks from retaining stale effort state. No current merge-blocking risk remains. Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 4 warnings)
✅ Passed checks (2 passed)
Full details: Linked Issues checkExplanation The directly linked issue [ Full details: Out of Scope Changes checkExplanation The changes include experiment schemas, reasoning-resolution APIs, Stryker configuration, webview settings, visual fixtures, and localization updates. These changes are unrelated to linked issue [ Full details: Regression EvidenceExplanation The new Task request-metadata behavior lacks focused boundary coverage. Resolution Add focused Task-level tests at the request boundaries. Set a runtime override and assert Full details: Trust And Persistence InvariantsExplanation The new Resolution Make API-handler replacement lifecycle-safe. Add an optional disposal contract to
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review statusThanks for contributing. This comment tracks the review sequence and the next action. Current step: Required CI passed. Waiting for automated review of the latest commit. If automated review does not start, a maintainer must restart it. Review-state labels are managed by this workflow; do not edit them manually. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…2-2-per-request-effort
…nce in the updateSettings payload
…alse/unset persistence cases)
c5b48aa
≤400-line redo of #1338 — DTE series 2/5, unit 3/5
Task-local runtime thinking-effort state on
Task: the in-memory overridechannel, its per-request delivery at all four
createMessagesites, and theprofile-switch re-capture in
updateApiConfiguration. Transient state only — nothing is persisted; persistence is the next unit (U4).Stack
main@0dbd5846f(cross-fork PR — headeasonLiangWorldedtech:feat/dte-v2-3-task-runtime-effort; stack branches live in the fork — no push access to create them here)feat/dte-v2-2-per-request-effort@069c34b9a(U2, upstream PR feat(api): per-request thinking effort override and adaptive effort envelope (DTE series 2/5) #1522, new tip — carries U1's post-main-sync heade89ccedddand the advanced upstreammaintip0dbd5846fmerged in)f97f8d999merged U2's tip18f488fa5(U1 final CR fixes53f22dcin); second syncc5b48aa7emerged U2's new tipefbd336e5(U1's final head39762bf81— the last feat(settings): dynamic thinking effort experimental toggle (DTE-1) #1521 CR fix — in); third (main) synce8c66cfad(this head) merges U2's post-main-sync tip069c34b9a(upstreammainadvanced0d937c050→0dbd5846f: v3.82.0 release prep Release v3.82.0 #1533, GPT-6 Astra [Feat] Add verified GPT-6 Astra support across providers #1506, DeepSeek V4 Flash Vision [Feat] Add DeepSeek V4 Flash Vision Exp support #1488, thethrowIfAbortedhelper +completePromptoptions regression tests feat(api): add throwIfAborted helper and completePrompt options regression tests #1288, and the asyncTask.dispose()test-teardown fix [Fix] Unit tests report teardown errors after Task cleanup #1527). One conflict insrc/core/task/Task.tsresolved: main's asyncdispose(): Promise<void>restructure (memoizeddisposalPromise+disposeOnce()) kept, with U3's 4-line DTE JSDoc above the newdispose(); the test'safterEachnow awaitstask.dispose()per the repo idiom. Standalone budget 398 at this head (amended to 403 by the CR re-review fix in commit1edd728c4, then to 418 by the follow-up CR fix in commit1087fcbd4, see below)feat/dte-trial-allunion27a2e97df(tagdte-legacy/union), U3 sliceGitHub's displayed diff vs
mainis cumulative over the unmerged lower units(U1 #1521, U2 #1522); the standalone range below is the review target — the displayed number shrinks as they merge. Merge this PR only after its stack
base PR has merged.
Budget (plan §2: a+d ≤400 soft target; ≤1000 hard)
2 files changed, 417 insertions(+), 1 deletion(-)= 418 — soft target 400 exceeded by 18, CR-driven (see the amendments below); hard cap 1000 ✓src/core/task/Task.tssrc/core/task/__tests__/Task.runtime-thinking-effort.test.ts(new)Budget deviation note (plan §2.6). The plan estimated U3 at ~355
(Task +132/− + tests ~20); those numbers were stale. Measured against the
union, the U3 slice is Task +105/− + a 311-line test file = 417 > 400. Per
§2.6 (no budget bypass), the dispose boundary group is split into U4:
the task-end override reset (6 Task lines + 3 DTE JSDoc lines) and its
12-line
describe("dispose")test block. This matches the plan's own U4 scopeline ("persistence + boundary cases"). U3 keeps the generic 4-line
dispose()JSDoc; U4 expands it with the DTE sentence alongside the reset code.
CR re-review amendment (2026-09-05, review
5120440807). The main-synchead re-review requested the task-end override reset to live in
disposeOnce()of this unit (so a retained disposed task never serves a staleoverride, and the
dispose()JSDoc's "resets transient task-local state"claim is accurate), plus the test
afterEachteardown to callawait task.dispose()unconditionally (the!task.abortguard had skippeddisposal of aborted tasks). Both land here in commit
1edd728c4(+5/−1 ⇒standalone 398 → 403 — a 3-line overshoot of the soft target, within the 1000
hard cap). Under the split above, U4 loses the reset code it had carried; its
12-line dispose-boundary test block stays with U4 (it now exercises this
unit's reset on the stacked tree).
CR follow-up fix (2026-09-06, review thread comment
3942121231). There-review of head
1edd728c4found that the unconditional field clears indisposeOnce()left the override behind inapiConfiguration.reasoningEffortand the built
apihandler — both are populated bysetRuntimeThinkingEffort, so a retained disposed task could still expose theoverride through those copies. Commit
1087fcbd4replaces the threeassignments with the standard clearing call
this.setRuntimeThinkingEffort(undefined)—which restores
apiConfiguration/apifrompreOverrideReasoningEffort(read before it is cleared) and then clears the runtime fields — and adds a
14-line
describe("dispose")killing-test block asserting the clear and therestore (Task.ts net +1; standalone 403 → 418 a+d — CR-driven, ≤1000 hard cap).
Provenance / fidelity
Task.ts: 3-waygit merge-file— base39bdfb188(=6ea45b36a^),ours = U2 head, theirs =
90b47b053(the last U3 commit, before the U4persistence work). Zero conflicts. A whole-file extract was impossible:
the union's
Task.tscarries U14-orchestrator and U4/U5 content(215+/241− vs U1 head), and per-commit
git apply --3wayof the U3 patchesfails on upstream base drift.
90b47b053version (311 lines) minus the disposedescribe (12 lines + separator), plus the 3 mutation-killing assertion
lines below = 301 lines; the header comment is trimmed to the U3 scope
("the task-end reset in dispose()" clause moves with U4).
taskMetadata.ts/history.tspersistence changes,the
describe("history persistence round-trip")anddescribe("abortTask final save")blocks, and theHistoryItemimport(unused in the U3 slice).
src/eslint-suppressions.json: untouched. The union's +21 suppression-countdeltas vs the stack base are all in files owned by other units
(
gemini-format.spec.ts5→6,ask-queued-message-drain.spec.ts18→32,newTaskTool.spec.ts26→31, newextension.ts1) — none U3-owned.Out of scope (next units)
taskMetadatamerge propagation, and the task-end override reset split outabove (dispose boundary + its test).
output_config.effortadaptive envelope.Mutation-diff fix (killing assertions, plan L42 — same PR)
The first CI
mutation-diffrun (headd0b1a3dcd) reported 2 SurvivedConditionalExpressionmutants — both on the two ternaries this unitintroduces:
setRuntimeThinkingEffortsource capture:effort === undefined ? undefined : sourcesource: a label passed on a clearing call leaks intosourcesetRuntimeThinkingEffort(undefined, "stale-source")); the existingsource: undefinedassertion then kills the variantgetRuntimeThinkingEffortMetadata:effort !== undefined ? { reasoningEffort } : {}{ reasoningEffort: <maybe undefined> }not.toHaveProperty("reasoningEffort")while unset —toEqual({})cannot kill it (toEqual ignores keys whose value isundefined); asserted pre-set and post-clearThe complementary variants (L1683-true, L1721-false) were already killed by
the existing
toBe("test-source")andtoEqual({ reasoningEffort: "high" })assertions.
Local dev-stage gate (skill §5.1) on the latest CR-fix head
1087fcbd4:node scripts/stryker-diff.mjs ci --base 069c34b9a --head 1087fcbd4— extension 36 changed lines, 26 valid mutants, 26 Killed, 0 Survived / 0 NoCoverage, exit 0 (previous CR-fix head1edd728c4: 38 lines, 25/25 Killed).Verification (local, head
1087fcbd4)pnpm --dir src exec eslint --prune-suppressions --max-warnings=0 core/task/Task.ts core/task/__tests__/Task.runtime-thinking-effort.test.ts— exit 0 (re-run on1087fcbd4; no suppression-count change)pnpm check-types— 11/11 projectspnpm --dir src exec vitest run core/task/__tests__/Task.runtime-thinking-effort.test.ts— 9/9 (re-run on1087fcbd4, 4.16 s; includes the new dispose killing test)git diff --shortstat 069c34b9a HEAD— 417+/1− = 418 (CR fixes +19/−4 vs1edd728c4; soft overshoot documented above, ≤1000 hard)1087fcbd4— exit 0, 26/26 Killed (see fix above)