From 452a7e6a15e6e5520043f76410a79e06e314fce2 Mon Sep 17 00:00:00 2001 From: unraid Date: Wed, 29 Apr 2026 15:17:50 +0800 Subject: [PATCH] fixup: address CodeRabbit review on PR #386 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Twelve actionable items (7 Major + 5 Minor) from the CodeRabbit review on claude-code-best/claude-code#386: - docs/internals/autonomy-jira.md: typo "due input close" → "due to input close". - src/utils/autonomyRuns.ts: - selectPersistedAutonomyRuns no longer evicts active (queued/running) runs when the combined list exceeds AUTONOMY_RUNS_MAX. Active runs are kept in full and the inactive history is capped to the remaining budget so persisted ownership for live work survives. - isValidOwnerProcessId now allows pid <= 4_194_304 so a live run owned by the maximum Linux PID is not treated as stale. - src/utils/autonomyAuthority.ts: maskCodeFencedLines tracks the active fence length and only closes the fence when a same-character run of equal-or- greater length appears with no trailing content, so a nested ```yaml inside an outer ```` block no longer leaks fake `tasks:` entries into the parser. - src/cli/print.ts: late-shutdown branches in the cron and scheduled-task paths now call cancelQueuedAutonomyCommands({ commands: [command] }) instead of markAutonomyRunCancelled(...). Updating run state alone left the queue-side record orphaned for resume/recovery. - src/utils/processUserInput/processSlashCommand.tsx: scheduled-task-result notification is enqueued before finalizeAutonomyRunCompleted (which queues follow-up autonomy commands) so both at priority: 'later' land in order and the next autonomy step can not run before the worker's output is observed. - src/screens/REPL.tsx + src/utils/handlePromptSubmit.ts: - onQuery now returns Promise: false from the concurrent-guard skip path, true otherwise. Other call sites use `void onQuery(...)` and are unaffected. handlePromptSubmit's onQuery prop type matches. - The autonomy-prompt callsite captures the executed flag, finalizes claim.claimedCommands as { type: 'completed' } only when onQuery actually ran, and runs the completed-finalize in its own try/catch so a failure there does not propagate into the outer catch and trigger a second finalize as { type: 'failed' } for the same commands. - Removed the unsafe `command.value as string` cast; createUserMessage already accepts `string | ContentBlockParam[]`. - createUserMessage mock in src/__tests__/handlePromptSubmit.test.ts now matches the new Promise shape. - packages/builtin-tools/src/tools/RemoteTriggerTool/__tests__/ RemoteTriggerTool.test.ts: - Inline auth mock replaced with the shared tests/mocks/auth (added). - The full mock of src/constants/oauth.js is replaced by a narrow side-effect-only mock that overrides the env-reading helpers (getOauthConfig, fileSuffixForOauthConfig, MCP_CLIENT_METADATA_URL) and delegates pure data exports to the real module. - tests/integration/dependency-overrides.test.ts: - mermaid does not export `./package.json` in its exports map, so require.resolve('mermaid/package.json') throws ERR_PACKAGE_PATH_NOT_EXPORTED in runtimes that honor exports semantics. The test now resolves the package entry and walks up to the package root via a small findPackageJson helper. - readFileSync from node:fs is replaced with `await Bun.file(...).text()` to match the project's Bun-API requirement. Validation: - bun run typecheck (clean). - bun test → 3996 pass / 0 fail across 305 test files. Targets PRs: - amDosion/claude-code-bast#8 (fork-internal review) - claude-code-best/claude-code#386 (upstream review, same head branch) --- docs/internals/autonomy-jira.md | 2 +- .../__tests__/RemoteTriggerTool.test.ts | 31 +++++---- src/__tests__/handlePromptSubmit.test.ts | 4 +- src/cli/print.ts | 15 +---- src/screens/REPL.tsx | 45 +++++++++---- src/utils/autonomyAuthority.ts | 15 ++++- src/utils/autonomyRuns.ts | 31 ++++----- src/utils/handlePromptSubmit.ts | 2 +- .../processUserInput/processSlashCommand.tsx | 7 +- .../integration/dependency-overrides.test.ts | 64 ++++++++++++++----- tests/mocks/auth.ts | 19 ++++++ 11 files changed, 151 insertions(+), 84 deletions(-) create mode 100644 tests/mocks/auth.ts diff --git a/docs/internals/autonomy-jira.md b/docs/internals/autonomy-jira.md index be297ed50..5593fdcf9 100644 --- a/docs/internals/autonomy-jira.md +++ b/docs/internals/autonomy-jira.md @@ -260,7 +260,7 @@ Acceptance criteria: - Proactive hook cancellation checks run both before commit and after command creation. - Headless proactive and cron paths cancel any already-created command that is - dropped due input close. + dropped due to input close. - REPL scheduled-task cleanup cancels already-created commands when unmounted. - A regression test verifies a proactive command created but dropped before enqueue is marked cancelled. diff --git a/packages/builtin-tools/src/tools/RemoteTriggerTool/__tests__/RemoteTriggerTool.test.ts b/packages/builtin-tools/src/tools/RemoteTriggerTool/__tests__/RemoteTriggerTool.test.ts index 62e1e47d4..b3640822f 100644 --- a/packages/builtin-tools/src/tools/RemoteTriggerTool/__tests__/RemoteTriggerTool.test.ts +++ b/packages/builtin-tools/src/tools/RemoteTriggerTool/__tests__/RemoteTriggerTool.test.ts @@ -1,4 +1,5 @@ import { afterEach, beforeEach, describe, expect, mock, test } from 'bun:test' +import { authMock } from '../../../../../../tests/mocks/auth' let requestStatus = 200 const auditRecords: Record[] = [] @@ -12,11 +13,7 @@ mock.module('axios', () => ({ }, })) -mock.module('src/utils/auth.js', () => ({ - checkAndRefreshOAuthTokenIfNeeded: async () => {}, - getClaudeAIOAuthTokens: () => ({ accessToken: 'token' }), - isClaudeAISubscriber: () => true, -})) +mock.module('src/utils/auth.js', authMock) mock.module('src/services/oauth/client.js', () => ({ getOrganizationUUID: async () => 'org', @@ -30,17 +27,19 @@ mock.module('src/services/policyLimits/index.js', () => ({ isPolicyAllowed: () => true, })) -mock.module('src/constants/oauth.js', () => ({ - ALL_OAUTH_SCOPES: ['user:profile', 'user:inference'], - CLAUDE_AI_INFERENCE_SCOPE: 'user:inference', - CLAUDE_AI_OAUTH_SCOPES: ['user:profile', 'user:inference'], - CLAUDE_AI_PROFILE_SCOPE: 'user:profile', - CONSOLE_OAUTH_SCOPES: ['org:create_api_key', 'user:profile'], - MCP_CLIENT_METADATA_URL: 'https://example.test/oauth/metadata', - OAUTH_BETA_HEADER: 'oauth-test', - fileSuffixForOauthConfig: () => '', - getOauthConfig: () => ({ BASE_API_URL: 'https://example.test' }), -})) +// Narrow mock for the side-effectful entries in `src/constants/oauth.js`. +// Pure data exports (ALL_OAUTH_SCOPES, CLAUDE_AI_*_SCOPE, etc.) come from +// the real module and are not mocked, per the test policy that constants +// modules without side effects should not be replaced wholesale. +mock.module('src/constants/oauth.js', () => { + const actual = require('../../../../../../src/constants/oauth.js') + return { + ...actual, + fileSuffixForOauthConfig: () => '', + getOauthConfig: () => ({ BASE_API_URL: 'https://example.test' }), + MCP_CLIENT_METADATA_URL: 'https://example.test/oauth/metadata', + } +}) mock.module('src/utils/remoteTriggerAudit.js', () => ({ appendRemoteTriggerAuditRecord: async ( diff --git a/src/__tests__/handlePromptSubmit.test.ts b/src/__tests__/handlePromptSubmit.test.ts index 42db05af0..1c0cca36f 100644 --- a/src/__tests__/handlePromptSubmit.test.ts +++ b/src/__tests__/handlePromptSubmit.test.ts @@ -38,9 +38,9 @@ function createBaseParams() { commands: [], setUserInputOnProcessing: mock((_prompt?: string) => {}), setAbortController: mock((_abortController: AbortController | null) => {}), - onQuery: mock(async () => undefined) as unknown as ( + onQuery: mock(async () => true) as unknown as ( ...args: unknown[] - ) => Promise, + ) => Promise, setAppState: mock((_updater: unknown) => {}), } } diff --git a/src/cli/print.ts b/src/cli/print.ts index c494a060e..0166cec94 100644 --- a/src/cli/print.ts +++ b/src/cli/print.ts @@ -2839,10 +2839,7 @@ function runHeadlessStreaming( workload: WORKLOAD_CRON, }) if (inputClosed) { - await markAutonomyRunCancelled( - command.autonomy!.runId, - command.autonomy!.rootDir, - ) + await cancelQueuedAutonomyCommands({ commands: [command] }) return } enqueue({ @@ -2875,10 +2872,7 @@ function runHeadlessStreaming( }) if (!command) return if (inputClosed) { - await markAutonomyRunCancelled( - command.autonomy!.runId, - command.autonomy!.rootDir, - ) + await cancelQueuedAutonomyCommands({ commands: [command] }) return } await markAutonomyRunFailed( @@ -2899,10 +2893,7 @@ function runHeadlessStreaming( }) if (!command) return if (inputClosed) { - await markAutonomyRunCancelled( - command.autonomy!.runId, - command.autonomy!.rootDir, - ) + await cancelQueuedAutonomyCommands({ commands: [command] }) return } enqueue({ diff --git a/src/screens/REPL.tsx b/src/screens/REPL.tsx index 682398490..fe26e38cf 100644 --- a/src/screens/REPL.tsx +++ b/src/screens/REPL.tsx @@ -3474,7 +3474,7 @@ export function REPL({ onBeforeQueryCallback?: (input: string, newMessages: MessageType[]) => Promise, input?: string, effort?: EffortValue, - ): Promise => { + ): Promise => { // If this is a teammate, mark them as active when starting a turn if (isAgentSwarmsEnabled()) { const teamName = getTeamName(); @@ -3505,7 +3505,7 @@ export function REPL({ logEvent('tengu_concurrent_onquery_enqueued', {}); } }); - return; + return false; } try { @@ -3538,7 +3538,7 @@ export function REPL({ if (onBeforeQueryCallback && input) { const shouldProceed = await onBeforeQueryCallback(input, latestMessages); if (!shouldProceed) { - return; + return true; } } @@ -3687,6 +3687,7 @@ export function REPL({ } } } + return true; }, [onQueryImpl, setAppState, resetLoadingState, queryGuard, mrOnBeforeQuery, mrOnTurnComplete], ); @@ -4851,13 +4852,37 @@ export function REPL({ // Create a user message with the formatted content (includes XML wrapper) const userMessage = createUserMessage({ - content: command.value as string, + content: command.value, isMeta: command.isMeta ? true : undefined, origin: command.origin, }); + let executed = false; + try { + executed = (await onQuery([userMessage], newAbortController, true, [], mainLoopModel)) !== false; + } catch (error: unknown) { + try { + await finalizeAutonomyCommandsForTurn({ + commands: claim.claimedCommands, + outcome: { type: 'failed', error }, + currentDir: getCwd(), + priority: 'later', + }); + } catch (finalizeError: unknown) { + logError(toError(finalizeError)); + } + logError(toError(error)); + return; + } + + // Only finalize as completed when onQuery actually executed the turn + // (it returns false from the concurrent-guard path without running). + // Keep this finalize in its own try/catch so a failure here does not + // trigger a second finalize as `failed` for the same commands. + if (!executed) { + return; + } try { - await onQuery([userMessage], newAbortController, true, [], mainLoopModel); const nextCommands = await finalizeAutonomyCommandsForTurn({ commands: claim.claimedCommands, outcome: { type: 'completed' }, @@ -4867,14 +4892,8 @@ export function REPL({ for (const nextCommand of nextCommands) { enqueue(nextCommand); } - } catch (error: unknown) { - await finalizeAutonomyCommandsForTurn({ - commands: claim.claimedCommands, - outcome: { type: 'failed', error }, - currentDir: getCwd(), - priority: 'later', - }); - logError(toError(error)); + } catch (finalizeError: unknown) { + logError(toError(finalizeError)); } })().catch((error: unknown) => { logError(toError(error)); diff --git a/src/utils/autonomyAuthority.ts b/src/utils/autonomyAuthority.ts index 43d9fee88..cd5326f60 100644 --- a/src/utils/autonomyAuthority.ts +++ b/src/utils/autonomyAuthority.ts @@ -143,15 +143,24 @@ function mergeAgentsAuthority(files: AutonomyAuthorityFile[]): string | null { function maskCodeFencedLines(lines: string[]): string[] { const masked = lines.slice() let activeFenceChar: '`' | '~' | null = null + let activeFenceLen = 0 for (let i = 0; i < masked.length; i++) { const trimmed = masked[i]!.trim() - const fenceMatch = trimmed.match(/^(```+|~~~+)/) + const fenceMatch = trimmed.match(/^([`~])\1{2,}/) if (fenceMatch) { - const fenceChar = fenceMatch[1]![0] as '`' | '~' + const fenceChar = fenceMatch[1]! as '`' | '~' + const fenceLen = fenceMatch[0]!.length + const trailing = trimmed.slice(fenceLen) if (activeFenceChar === null) { activeFenceChar = fenceChar - } else if (activeFenceChar === fenceChar) { + activeFenceLen = fenceLen + } else if ( + activeFenceChar === fenceChar && + fenceLen >= activeFenceLen && + trailing.trim() === '' + ) { activeFenceChar = null + activeFenceLen = 0 } masked[i] = '' continue diff --git a/src/utils/autonomyRuns.ts b/src/utils/autonomyRuns.ts index 70adee92c..89970eaaf 100644 --- a/src/utils/autonomyRuns.ts +++ b/src/utils/autonomyRuns.ts @@ -130,20 +130,18 @@ function isAutonomyRunActive(run: AutonomyRunRecord): boolean { function selectPersistedAutonomyRuns( runs: AutonomyRunRecord[], ): AutonomyRunRecord[] { - const retained = runs - .slice() - .map(cloneRunRecord) - .sort((left, right) => { - const leftActive = isAutonomyRunActive(left) - const rightActive = isAutonomyRunActive(right) - if (leftActive !== rightActive) { - return leftActive ? -1 : 1 - } - return right.createdAt - left.createdAt - }) - .slice(0, AUTONOMY_RUNS_MAX) + const cloned = runs.slice().map(cloneRunRecord) + const active = cloned + .filter(isAutonomyRunActive) + .sort((left, right) => right.createdAt - left.createdAt) + const history = cloned + .filter(run => !isAutonomyRunActive(run)) + .sort((left, right) => right.createdAt - left.createdAt) + .slice(0, Math.max(0, AUTONOMY_RUNS_MAX - active.length)) - return retained.sort((left, right) => right.createdAt - left.createdAt) + return [...active, ...history].sort( + (left, right) => right.createdAt - left.createdAt, + ) } function normalizePersistedRunRecord( @@ -260,7 +258,7 @@ function isValidOwnerProcessId(pid: number | undefined): pid is number { typeof pid === 'number' && Number.isInteger(pid) && pid > 0 && - pid < 4_194_304 + pid <= 4_194_304 ) } @@ -407,10 +405,7 @@ async function persistAutonomyRunRecord( continue } if (isStaleActiveAutonomyRun(run)) { - const recovered = recoverStaleActiveAutonomyRun( - run, - record.createdAt, - ) + const recovered = recoverStaleActiveAutonomyRun(run, record.createdAt) runs[i] = recovered recoveredStaleRuns.push(recovered) staleRecoveriesApplied = true diff --git a/src/utils/handlePromptSubmit.ts b/src/utils/handlePromptSubmit.ts index 13125755d..7c2b35e22 100644 --- a/src/utils/handlePromptSubmit.ts +++ b/src/utils/handlePromptSubmit.ts @@ -74,7 +74,7 @@ type BaseExecutionParams = { onBeforeQuery?: (input: string, newMessages: Message[]) => Promise, input?: string, effort?: EffortValue, - ) => Promise + ) => Promise setAppState: (updater: (prev: AppState) => AppState) => void onBeforeQuery?: (input: string, newMessages: Message[]) => Promise canUseTool?: CanUseToolFn diff --git a/src/utils/processUserInput/processSlashCommand.tsx b/src/utils/processUserInput/processSlashCommand.tsx index 5c52a5dd5..5e6059702 100644 --- a/src/utils/processUserInput/processSlashCommand.tsx +++ b/src/utils/processUserInput/processSlashCommand.tsx @@ -260,8 +260,13 @@ async function executeForkedSlashCommand( } const resultText = extractResultText(agentMessages, 'Command completed'); logForDebugging(`Background forked command /${commandName} completed (agent ${agentId})`); - await finalizeDeferredAutonomyRunCompleted(); + // Enqueue the worker's result before finalizing the autonomy run so the + // notification is observed before any follow-up + // autonomy commands the finalizer enqueues at the same priority. Without + // 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(); })().catch(async err => { logError(err); enqueueResult( diff --git a/tests/integration/dependency-overrides.test.ts b/tests/integration/dependency-overrides.test.ts index 337558158..65a667952 100644 --- a/tests/integration/dependency-overrides.test.ts +++ b/tests/integration/dependency-overrides.test.ts @@ -1,14 +1,43 @@ import { describe, expect, test } from 'bun:test' -import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs' import { createRequire } from 'node:module' import { tmpdir } from 'node:os' -import { join, resolve } from 'node:path' +import { dirname, join, resolve } from 'node:path' import { pathToFileURL } from 'node:url' const repoRoot = resolve(import.meta.dir, '..', '..') const uuidV4Pattern = /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/ +async function findPackageJson( + startPath: string, + expectedName: string, +): Promise { + let current = dirname(startPath) + for (let depth = 0; depth < 10; depth++) { + const candidate = join(current, 'package.json') + const file = Bun.file(candidate) + if (await file.exists()) { + try { + const parsed = JSON.parse(await file.text()) as { name?: unknown } + if (parsed.name === expectedName) { + return candidate + } + } catch { + // ignore parse errors and keep walking up + } + } + const parent = dirname(current) + if (parent === current) { + break + } + current = parent + } + throw new Error( + `package.json with name "${expectedName}" not found above ${startPath}`, + ) +} + describe('dependency security overrides', () => { test('mcpb can load patched inquirer prompts from its package context', async () => { const mcpbRequire = createRequire(import.meta.resolve('@anthropic-ai/mcpb')) @@ -28,10 +57,7 @@ describe('dependency security overrides', () => { ) const gaxios = vertexRequire('gaxios') as { request(options: { - adapter(options: { - headers: Headers - url: string - }): Promise<{ + adapter(options: { headers: Headers; url: string }): Promise<{ config: unknown data: string headers: Record @@ -39,7 +65,7 @@ describe('dependency security overrides', () => { status: number statusText: string }> - multipart: Array<{ body: string; headers: Record }> + multipart: Array<{ body: string; headers: Record }> url: string }): Promise<{ status: number }> } @@ -47,8 +73,10 @@ describe('dependency security overrides', () => { const response = await gaxios.request({ url: 'https://example.com/upload', - multipart: [{ body: 'payload', headers: { 'Content-Type': 'text/plain' } }], - adapter: async (options) => { + multipart: [ + { body: 'payload', headers: { 'Content-Type': 'text/plain' } }, + ], + adapter: async options => { contentType = options.headers.get('content-type') ?? undefined return { config: options, @@ -62,14 +90,14 @@ describe('dependency security overrides', () => { }) expect(response.status).toBe(200) - expect(contentType).toMatch( - /^multipart\/related; boundary=[0-9a-f-]{36}$/, - ) + expect(contentType).toMatch(/^multipart\/related; boundary=[0-9a-f-]{36}$/) expect(contentType?.split('boundary=')[1]).toMatch(uuidV4Pattern) }) test('azure identity msal guid generation works through its package context', () => { - const identityRequire = createRequire(import.meta.resolve('@azure/identity')) + const identityRequire = createRequire( + import.meta.resolve('@azure/identity'), + ) const msal = identityRequire('@azure/msal-node') as { CryptoProvider: new () => { createNewGuid(): string } } @@ -91,11 +119,13 @@ describe('dependency security overrides', () => { pathToFileURL(streamdownRequire.resolve('uuid')).href )) as { v4(): string } const mermaidPath = streamdownRequire.resolve('mermaid') - const mermaidPackagePath = streamdownRequire.resolve( - 'mermaid/package.json', - ) + // mermaid does not export ./package.json in its exports map, so resolving + // 'mermaid/package.json' throws ERR_PACKAGE_PATH_NOT_EXPORTED in runtimes + // that honor exports semantics. Walk up from the resolved entry until a + // package.json with name === 'mermaid' is found. + const mermaidPackagePath = await findPackageJson(mermaidPath, 'mermaid') const mermaidPackage = JSON.parse( - readFileSync(mermaidPackagePath, 'utf8'), + await Bun.file(mermaidPackagePath).text(), ) as { name?: unknown exports?: { '.'?: { import?: unknown } } diff --git a/tests/mocks/auth.ts b/tests/mocks/auth.ts new file mode 100644 index 000000000..805cc1fbd --- /dev/null +++ b/tests/mocks/auth.ts @@ -0,0 +1,19 @@ +/** + * Shared mock for `src/utils/auth.js`. Use it via: + * + * import { authMock } from '../../tests/mocks/auth' + * mock.module('src/utils/auth.js', authMock) + * + * Tests that need different return values can override the helper used by + * the suite (e.g. by extending this object and re-registering with mock.module). + * Always extend here rather than inlining a different shape per test, so the + * surface stays consistent when `auth.ts` exports change. + */ +export const authMock = () => ({ + checkAndRefreshOAuthTokenIfNeeded: async () => {}, + getClaudeAIOAuthTokens: () => ({ accessToken: 'token' }), + isClaudeAISubscriber: () => true, + isProSubscriber: () => false, + isMaxSubscriber: () => false, + isTeamSubscriber: () => false, +})