From 189766c5afceff164cdb0080d98d3bd312d5c78a Mon Sep 17 00:00:00 2001 From: unraid Date: Wed, 29 Apr 2026 15:49:54 +0800 Subject: [PATCH] fixup: address CodeRabbit second-round review on PR #386 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four inline + one outside-diff actionable comment from the second CodeRabbit review on claude-code-best/claude-code#386: - tests/mocks/auth.ts: align mock return contracts with src/utils/auth.ts. checkAndRefreshOAuthTokenIfNeeded resolves to a Promise and getClaudeAIOAuthTokens returns the full token shape (refreshToken, expiresAt, scopes, subscriptionType, rateLimitTier) so tests that branch on these values can not silently drift away from production. - src/utils/handlePromptSubmit.ts (461-468): clear the freshly-published abortController before the early return when every claimed autonomy command was skipped as non-consumable, so this turn's stale controller does not leak into the next turn. - src/utils/handlePromptSubmit.ts (621-649): separate execution failure from finalizer failure. The turn body now writes to a `turnError` slot; a single pass after the inner try decides whether to finalize claimed commands as `completed` or `failed`, with each finalize call wrapped in its own try/catch so a failure inside finalize does not flip a successful turn into `failed` and double-finalize the same commands. The outer catch only rethrows the original turn error. - src/utils/processUserInput/processSlashCommand.tsx (228-276): wrap the post-success `finalizeDeferredAutonomyRunCompleted()` call in its own try/catch so a finalize failure no longer falls into the worker-failure catch path and emits a contradictory `` for a slash command that actually succeeded. Outside scope (not changed) — the CodeRabbit suggestion to add a `.ts` extension to the shared `tests/mocks/auth` import contradicts the project's existing convention: every other test imports the shared mocks without the extension (e.g. `tests/mocks/log`, `tests/mocks/debug`, `tests/mocks/file-system`), and the project's tsconfig does not enable `allowImportingTsExtensions`, so adding the extension fails typecheck. The import is kept extension-less to match the rest of the suite. Validation: - bun run typecheck (clean). - bun test → 3996 pass / 0 fail across 305 test files. --- src/utils/handlePromptSubmit.ts | 80 ++++++++++++------- .../processUserInput/processSlashCommand.tsx | 9 ++- tests/mocks/auth.ts | 16 +++- 3 files changed, 73 insertions(+), 32 deletions(-) diff --git a/src/utils/handlePromptSubmit.ts b/src/utils/handlePromptSubmit.ts index 7c2b35e22..e8c387167 100644 --- a/src/utils/handlePromptSubmit.ts +++ b/src/utils/handlePromptSubmit.ts @@ -25,6 +25,8 @@ import type { EffortValue } from './effort.js' import type { FileHistoryState } from './fileHistory.js' import { fileHistoryEnabled, fileHistoryMakeSnapshot } from './fileHistory.js' import { gracefulShutdownSync } from './gracefulShutdown.js' +import { toError } from './errors.js' +import { logError } from './log.js' import { enqueue } from './messageQueueManager.js' import { resolveSkillModelOverride } from './model/model.js' import { @@ -464,6 +466,10 @@ async function executeUserInput(params: ExecuteUserInputParams): Promise { commands = queuedAutonomyClaim.attachmentCommands const claimedAutonomyCommands = queuedAutonomyClaim.claimedCommands if (commands.length === 0) { + // Clear the abort controller published a few lines above so this turn's + // stale controller does not leak into the next turn when every claimed + // autonomy command was skipped as non-consumable. + setAbortController(null) return } @@ -487,6 +493,7 @@ async function executeUserInput(params: ExecuteUserInputParams): Promise { // context — isolated from the parent's continuation. A process-global // mutable slot would be clobbered at the detached closure's first // await by this function's synchronous return path. See state.ts. + let turnError: unknown try { await runWithWorkload(turnWorkload, async () => { for (let i = 0; i < commands.length; i++) { @@ -618,37 +625,52 @@ async function executeUserInput(params: ExecuteUserInputParams): Promise { } } }) // end runWithWorkload — ALS context naturally scoped, no finally needed - if (claimedAutonomyCommands.length) { - const finalizableCommands = claimedAutonomyCommands.filter(command => { - const runId = command.autonomy?.runId - return !runId || !deferredAutonomyRunIds.has(runId) - }) - const nextCommands = await finalizeAutonomyCommandsForTurn({ - commands: finalizableCommands, - outcome: { type: 'completed' }, - currentDir: getCwd(), - priority: 'later', - workload: turnWorkload, - }) - for (const nextCommand of nextCommands) { - enqueue(nextCommand) + } catch (error) { + turnError = error + } + + // Finalize claimed autonomy commands as `completed` only if the turn + // body itself succeeded. Run the finalize call in its own try/catch so a + // failure there does not double-finalize the same commands as `failed` + // (which previously cancelled follow-up queue state after a successful + // turn). + if (claimedAutonomyCommands.length) { + const finalizableCommands = claimedAutonomyCommands.filter(command => { + const runId = command.autonomy?.runId + return !runId || !deferredAutonomyRunIds.has(runId) + }) + if (turnError) { + try { + await finalizeAutonomyCommandsForTurn({ + commands: finalizableCommands, + outcome: { type: 'failed', error: turnError }, + currentDir: getCwd(), + priority: 'later', + workload: turnWorkload, + }) + } catch (finalizeError) { + logError(toError(finalizeError)) + } + } else { + try { + const nextCommands = await finalizeAutonomyCommandsForTurn({ + commands: finalizableCommands, + outcome: { type: 'completed' }, + currentDir: getCwd(), + priority: 'later', + workload: turnWorkload, + }) + for (const nextCommand of nextCommands) { + enqueue(nextCommand) + } + } catch (finalizeError) { + logError(toError(finalizeError)) } } - } catch (error) { - if (claimedAutonomyCommands.length) { - const finalizableCommands = claimedAutonomyCommands.filter(command => { - const runId = command.autonomy?.runId - return !runId || !deferredAutonomyRunIds.has(runId) - }) - await finalizeAutonomyCommandsForTurn({ - commands: finalizableCommands, - outcome: { type: 'failed', error }, - currentDir: getCwd(), - priority: 'later', - workload: turnWorkload, - }) - } - throw error + } + + if (turnError) { + throw turnError } } finally { // Safety net: release the guard reservation if processUserInput threw diff --git a/src/utils/processUserInput/processSlashCommand.tsx b/src/utils/processUserInput/processSlashCommand.tsx index 5e6059702..da6763f7e 100644 --- a/src/utils/processUserInput/processSlashCommand.tsx +++ b/src/utils/processUserInput/processSlashCommand.tsx @@ -266,7 +266,14 @@ async function executeForkedSlashCommand( // this ordering, both land at `priority: 'later'` and the next autonomy // step can run before the main thread sees this worker's output. enqueueResult(`\n${resultText}\n`); - await finalizeDeferredAutonomyRunCompleted(); + // The slash command itself succeeded; an error from the finalize call + // must not surface as a contradictory + // via the outer catch below. Log it locally and stop. + try { + await finalizeDeferredAutonomyRunCompleted(); + } catch (finalizeError) { + logError(finalizeError); + } })().catch(async err => { logError(err); enqueueResult( diff --git a/tests/mocks/auth.ts b/tests/mocks/auth.ts index 805cc1fbd..7c0da17a7 100644 --- a/tests/mocks/auth.ts +++ b/tests/mocks/auth.ts @@ -10,8 +10,20 @@ * surface stays consistent when `auth.ts` exports change. */ export const authMock = () => ({ - checkAndRefreshOAuthTokenIfNeeded: async () => {}, - getClaudeAIOAuthTokens: () => ({ accessToken: 'token' }), + // Mirrors the production contract: src/utils/auth.ts returns + // Promise ("did the access token change") and a token object that + // carries scopes, subscriptionType, expiresAt, etc. Tests that branch on + // these values must see the full shape so they can not silently drift away + // from production. + checkAndRefreshOAuthTokenIfNeeded: async () => false, + getClaudeAIOAuthTokens: () => ({ + accessToken: 'token', + refreshToken: null, + expiresAt: null, + scopes: ['user:inference'], + subscriptionType: null, + rateLimitTier: null, + }), isClaudeAISubscriber: () => true, isProSubscriber: () => false, isMaxSubscriber: () => false,