mirror of
https://github.com/claude-code-best/claude-code.git
synced 2026-06-22 16:25:51 +00:00
fixup: address CodeRabbit second-round review on PR #386
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<boolean> 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 `<scheduled-task-result status="failed">` 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.
This commit is contained in:
@@ -25,6 +25,8 @@ import type { EffortValue } from './effort.js'
|
|||||||
import type { FileHistoryState } from './fileHistory.js'
|
import type { FileHistoryState } from './fileHistory.js'
|
||||||
import { fileHistoryEnabled, fileHistoryMakeSnapshot } from './fileHistory.js'
|
import { fileHistoryEnabled, fileHistoryMakeSnapshot } from './fileHistory.js'
|
||||||
import { gracefulShutdownSync } from './gracefulShutdown.js'
|
import { gracefulShutdownSync } from './gracefulShutdown.js'
|
||||||
|
import { toError } from './errors.js'
|
||||||
|
import { logError } from './log.js'
|
||||||
import { enqueue } from './messageQueueManager.js'
|
import { enqueue } from './messageQueueManager.js'
|
||||||
import { resolveSkillModelOverride } from './model/model.js'
|
import { resolveSkillModelOverride } from './model/model.js'
|
||||||
import {
|
import {
|
||||||
@@ -464,6 +466,10 @@ async function executeUserInput(params: ExecuteUserInputParams): Promise<void> {
|
|||||||
commands = queuedAutonomyClaim.attachmentCommands
|
commands = queuedAutonomyClaim.attachmentCommands
|
||||||
const claimedAutonomyCommands = queuedAutonomyClaim.claimedCommands
|
const claimedAutonomyCommands = queuedAutonomyClaim.claimedCommands
|
||||||
if (commands.length === 0) {
|
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
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -487,6 +493,7 @@ async function executeUserInput(params: ExecuteUserInputParams): Promise<void> {
|
|||||||
// context — isolated from the parent's continuation. A process-global
|
// context — isolated from the parent's continuation. A process-global
|
||||||
// mutable slot would be clobbered at the detached closure's first
|
// mutable slot would be clobbered at the detached closure's first
|
||||||
// await by this function's synchronous return path. See state.ts.
|
// await by this function's synchronous return path. See state.ts.
|
||||||
|
let turnError: unknown
|
||||||
try {
|
try {
|
||||||
await runWithWorkload(turnWorkload, async () => {
|
await runWithWorkload(turnWorkload, async () => {
|
||||||
for (let i = 0; i < commands.length; i++) {
|
for (let i = 0; i < commands.length; i++) {
|
||||||
@@ -618,37 +625,52 @@ async function executeUserInput(params: ExecuteUserInputParams): Promise<void> {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
}) // end runWithWorkload — ALS context naturally scoped, no finally needed
|
}) // end runWithWorkload — ALS context naturally scoped, no finally needed
|
||||||
if (claimedAutonomyCommands.length) {
|
} catch (error) {
|
||||||
const finalizableCommands = claimedAutonomyCommands.filter(command => {
|
turnError = error
|
||||||
const runId = command.autonomy?.runId
|
}
|
||||||
return !runId || !deferredAutonomyRunIds.has(runId)
|
|
||||||
})
|
// Finalize claimed autonomy commands as `completed` only if the turn
|
||||||
const nextCommands = await finalizeAutonomyCommandsForTurn({
|
// body itself succeeded. Run the finalize call in its own try/catch so a
|
||||||
commands: finalizableCommands,
|
// failure there does not double-finalize the same commands as `failed`
|
||||||
outcome: { type: 'completed' },
|
// (which previously cancelled follow-up queue state after a successful
|
||||||
currentDir: getCwd(),
|
// turn).
|
||||||
priority: 'later',
|
if (claimedAutonomyCommands.length) {
|
||||||
workload: turnWorkload,
|
const finalizableCommands = claimedAutonomyCommands.filter(command => {
|
||||||
})
|
const runId = command.autonomy?.runId
|
||||||
for (const nextCommand of nextCommands) {
|
return !runId || !deferredAutonomyRunIds.has(runId)
|
||||||
enqueue(nextCommand)
|
})
|
||||||
|
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 => {
|
if (turnError) {
|
||||||
const runId = command.autonomy?.runId
|
throw turnError
|
||||||
return !runId || !deferredAutonomyRunIds.has(runId)
|
|
||||||
})
|
|
||||||
await finalizeAutonomyCommandsForTurn({
|
|
||||||
commands: finalizableCommands,
|
|
||||||
outcome: { type: 'failed', error },
|
|
||||||
currentDir: getCwd(),
|
|
||||||
priority: 'later',
|
|
||||||
workload: turnWorkload,
|
|
||||||
})
|
|
||||||
}
|
|
||||||
throw error
|
|
||||||
}
|
}
|
||||||
} finally {
|
} finally {
|
||||||
// Safety net: release the guard reservation if processUserInput threw
|
// Safety net: release the guard reservation if processUserInput threw
|
||||||
|
|||||||
@@ -266,7 +266,14 @@ async function executeForkedSlashCommand(
|
|||||||
// this ordering, both land at `priority: 'later'` and the next autonomy
|
// this ordering, both land at `priority: 'later'` and the next autonomy
|
||||||
// step can run before the main thread sees this worker's output.
|
// step can run before the main thread sees this worker's output.
|
||||||
enqueueResult(`<scheduled-task-result command="/${commandName}">\n${resultText}\n</scheduled-task-result>`);
|
enqueueResult(`<scheduled-task-result command="/${commandName}">\n${resultText}\n</scheduled-task-result>`);
|
||||||
await finalizeDeferredAutonomyRunCompleted();
|
// The slash command itself succeeded; an error from the finalize call
|
||||||
|
// must not surface as a contradictory <scheduled-task-result status="failed">
|
||||||
|
// via the outer catch below. Log it locally and stop.
|
||||||
|
try {
|
||||||
|
await finalizeDeferredAutonomyRunCompleted();
|
||||||
|
} catch (finalizeError) {
|
||||||
|
logError(finalizeError);
|
||||||
|
}
|
||||||
})().catch(async err => {
|
})().catch(async err => {
|
||||||
logError(err);
|
logError(err);
|
||||||
enqueueResult(
|
enqueueResult(
|
||||||
|
|||||||
@@ -10,8 +10,20 @@
|
|||||||
* surface stays consistent when `auth.ts` exports change.
|
* surface stays consistent when `auth.ts` exports change.
|
||||||
*/
|
*/
|
||||||
export const authMock = () => ({
|
export const authMock = () => ({
|
||||||
checkAndRefreshOAuthTokenIfNeeded: async () => {},
|
// Mirrors the production contract: src/utils/auth.ts returns
|
||||||
getClaudeAIOAuthTokens: () => ({ accessToken: 'token' }),
|
// Promise<boolean> ("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,
|
isClaudeAISubscriber: () => true,
|
||||||
isProSubscriber: () => false,
|
isProSubscriber: () => false,
|
||||||
isMaxSubscriber: () => false,
|
isMaxSubscriber: () => false,
|
||||||
|
|||||||
Reference in New Issue
Block a user