Merged
Conversation
Cherry-picked from origin/lint/preview (637c908), excluding lint-only changes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cherry-picked from origin/lint/preview (ee36954). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cherry-picked from origin/lint/preview (c5f52cd), excluding lint-only formatting changes. - 新建 src/utils/cliLaunch.ts: 集中化 CLI 子进程启动层 - 修复 --daemon-worker=kind 等号格式解析 - 修复 daemon/bg fast path 缺少 setShellIfWindows() - 修复 checkPathExists 用 existsSync 替代 execSync('dir') - 7 个 spawn 站点迁移到 CliLaunchSpec Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ptions The cherry-pick from 637c908 dropped jsx/strict/etc settings when removing tsconfig.base.json. This commit restores them in a single tsconfig.json. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
✅ Files skipped from review due to trivial changes (12)
📝 WalkthroughWalkthroughThis PR introduces comprehensive support for Agent Client Protocol (ACP) integration with Zed and Cursor IDEs, restructures daemon and background session management under unified CLI/REPL commands, implements template-based job system with filesystem persistence, adds autonomy flow/run orchestration for scheduled tasks and proactive ticks, expands pipe-muting/send-override controls, and integrates language preference resolution across the codebase. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as ACP Client<br/>(Zed/Cursor)
participant Agent as AcpAgent
participant QE as QueryEngine
participant Tools as Tool System
Client->>Agent: newSession(params)
Agent->>QE: initialize (getDefaultAppState)
Agent->>QE: setModel (resolved)
Agent-->>Client: NewSessionResponse (modes/models)
Client->>Agent: prompt(content)
Agent->>QE: resetAbortController()
Agent->>QE: submitMessage(input)
QE->>Tools: execute tools
Tools-->>QE: results
QE-->>Agent: async generator
Agent->>Agent: forwardSessionUpdates()
Agent-->>Client: sessionUpdate (chunks/tools/usage)
Agent-->>Client: sessionUpdate (stop_reason)
Client->>Agent: setSessionMode(mode)
Agent->>Agent: sync config via sessionUpdate
Agent-->>Client: sessionUpdate (modeId)
sequenceDiagram
participant CLI as CLI Entry
participant Engine as BgEngine
participant Tmux as tmux/detached
participant Registry as Session Registry
CLI->>CLI: selectEngine()
alt Windows
CLI->>Engine: DetachedEngine
else Unix
Engine->>Tmux: available?
alt Available
CLI->>Engine: TmuxEngine
else Unavailable
CLI->>Engine: DetachedEngine
end
end
CLI->>Engine: start(opts)
Engine->>Tmux: spawn process
Tmux-->>Engine: pid
Engine->>Registry: write session entry
Engine-->>CLI: BgStartResult
CLI->>Engine: attach(session)
Engine->>Tmux: attach/tail
Tmux-->>CLI: interactive/streaming output
sequenceDiagram
participant Task as Heartbeat Task
participant Flow as Managed Flow
participant Queue as Command Queue
participant Run as Autonomy Run
Task->>Flow: startManagedAutonomyFlow()
Flow->>Flow: initialize steps
alt First step has waitFor
Flow->>Flow: set waiting
else
Flow->>Flow: set queued
end
Flow-->>Queue: enqueue step prompt
Queue->>Run: createAutonomyRun()
Run->>Run: persist to disk
Run-->>Queue: command with autonomy.runId
Queue->>Run: markRunning()
Run-->>Run: update status
alt Success
Queue->>Flow: markStepCompleted()
Flow->>Flow: advance to next step
Flow-->>Queue: enqueue next step
else Failure
Queue->>Run: markRunFailed()
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
…ptions The cherry-pick from 637c908 dropped jsx/strict/etc settings when removing tsconfig.base.json. This commit restores them in a single tsconfig.json. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/builtin-tools/src/tools/SendUserFileTool/SendUserFileTool.ts (1)
21-21:⚠️ Potential issue | 🟡 MinorOutput type definition is incomplete.
The
SendUserFileOutputtype only declaressentandfile_path, but the actual return object (lines 112-118) includes additional properties:size,file_uuid, anderror. This type mismatch reduces type safety downstream.🛠️ Proposed fix to align the type with actual return shape
-type SendUserFileOutput = { sent: boolean; file_path: string } +type SendUserFileOutput = { + sent: boolean + file_path: string + size?: number + file_uuid?: string + error?: string +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/builtin-tools/src/tools/SendUserFileTool/SendUserFileTool.ts` at line 21, The declared SendUserFileOutput type is missing fields returned by SendUserFileTool; update the SendUserFileOutput type to include the additional properties returned (e.g., add size: number, file_uuid: string | null, and error: string | null or undefined to match the actual return shape), ensuring the names and nullable/optional semantics match how those values are produced in the SendUserFileTool implementation.packages/builtin-tools/src/tools/PushNotificationTool/PushNotificationTool.ts (1)
27-27:⚠️ Potential issue | 🟡 MinorOutput type definition is incomplete.
The
PushOutputtype only declaressent, but the actual return object at line 131 includes anerrorfield. This should be reflected in the type for proper type safety.🛠️ Proposed fix
-type PushOutput = { sent: boolean } +type PushOutput = { sent: boolean; error?: string }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/builtin-tools/src/tools/PushNotificationTool/PushNotificationTool.ts` at line 27, The PushOutput type is missing the error field that the function actually returns; update the PushOutput type to include an error property (e.g., error?: string or error: string | null) so it matches the returned object shape, then adjust any return sites (the function that currently returns { sent, error }) and callers to satisfy the new type. Ensure the exported type name PushOutput is updated consistently wherever used.
🟡 Minor comments (27)
src/services/api/openai/__tests__/queryModelOpenAI.isolated.ts-1-15 (1)
1-15:⚠️ Potential issue | 🟡 MinorFile naming deviates from convention and prevents test discovery.
The file uses
.isolated.tssuffix instead of the standard.test.tspattern. Since Bun's default test discovery matches**/*.test.{ts,tsx,js,mjs}and**/*.spec.{ts,tsx,js,mjs}, this file is not executed bybun testin CI or local runs. Rename toqueryModelOpenAI.test.tsto align with coding guidelines: "Place unit tests adjacent to source modules insrc/**/__tests__/directory with filename format<module>.test.ts".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/api/openai/__tests__/queryModelOpenAI.isolated.ts` around lines 1 - 15, The test file is not discovered because its filename uses the `.isolated.ts` suffix; rename the test file to `queryModelOpenAI.test.ts` so it matches Bun's discovery patterns (`*.test.ts` / `*.spec.ts`) and will be executed by CI. Ensure any import/exports or references to this test file (if any) are updated to the new filename and keep the test contents unchanged; verify the test lives alongside the module under test in the __tests__ directory so `queryModelOpenAI` tests run.src/services/langfuse/__tests__/langfuse.isolated.ts-233-237 (1)
233-237:⚠️ Potential issue | 🟡 MinorShutdown test name and assertion are mismatched.
The test says it verifies
forceFlush/shutdown, but it only checks promise resolution. That can pass even if the processor methods are never called.[suggested fix]
Patch to assert the expected processor lifecycle calls
describe('shutdownLangfuse', () => { test('calls forceFlush and shutdown on processor', async () => { - const { shutdownLangfuse } = await import('../client.js') + process.env.LANGFUSE_PUBLIC_KEY = 'pk-test' + process.env.LANGFUSE_SECRET_KEY = 'sk-test' + const { initLangfuse, shutdownLangfuse } = await import('../client.js') + initLangfuse() await expect(shutdownLangfuse()).resolves.toBeUndefined() + expect(mockForceFlush).toHaveBeenCalledTimes(1) + expect(mockShutdown).toHaveBeenCalledTimes(1) }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/langfuse/__tests__/langfuse.isolated.ts` around lines 233 - 237, The test "shutdownLangfuse" currently only asserts the promise resolves but doesn't verify processor lifecycle calls; update the test to spy or mock the processor's forceFlush and shutdown methods (e.g., create spies on processor.forceFlush and processor.shutdown or replace the processor with a mock), call shutdownLangfuse(), await its completion, and assert that both forceFlush and shutdown were invoked (use toHaveBeenCalled or toHaveBeenCalledTimes). Ensure you reference the shutdownLangfuse export and the processor instance (the same processor used by the client) when creating the spies so the test fails if those methods are not called.src/services/langfuse/__tests__/langfuse.isolated.ts-217-230 (1)
217-230:⚠️ Potential issue | 🟡 Minor
initLangfusetests currently validate the wrong thing.Line 220 imports
isLangfuseEnabledinside theinitLangfusesuite, so the “returns true when keys are configured” case never callsinitLangfuse. Also, the idempotency case does not assert single initialization side effects.[suggested fix]
Patch to actually validate init + idempotency behavior
test('returns true when keys are configured', async () => { process.env.LANGFUSE_PUBLIC_KEY = 'pk-test' process.env.LANGFUSE_SECRET_KEY = 'sk-test' - const { isLangfuseEnabled } = await import('../client.js') - expect(isLangfuseEnabled()).toBe(true) + const { initLangfuse } = await import('../client.js') + expect(initLangfuse()).toBe(true) }) test('is idempotent — multiple calls do not re-initialize', async () => { + process.env.LANGFUSE_PUBLIC_KEY = 'pk-test' + process.env.LANGFUSE_SECRET_KEY = 'sk-test' const { initLangfuse } = await import('../client.js') - expect(() => { - initLangfuse() - initLangfuse() - }).not.toThrow() + expect(initLangfuse()).toBe(true) + expect(initLangfuse()).toBe(true) + expect(mockSetLangfuseTracerProvider).toHaveBeenCalledTimes(1) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/langfuse/__tests__/langfuse.isolated.ts` around lines 217 - 230, The tests are asserting the wrong behavior: the "returns true when keys are configured" test imports isLangfuseEnabled instead of calling initLangfuse, and the idempotency test doesn't verify single-initialization effects. Update the first test to set process.env.LANGFUSE_PUBLIC_KEY and LANGFUSE_SECRET_KEY, import and call initLangfuse (not just isLangfuseEnabled), then assert the expected enabled state (e.g., isLangfuseEnabled() === true or that init returned a valid client). For the idempotency test, verify that multiple calls to initLangfuse do not recreate the client by either asserting initLangfuse() returns the same object reference on repeated calls or by spying on the underlying creation function (use jest.spyOn on the module/function that creates the Langfuse client) and asserting it was called only once while calling initLangfuse() multiple times.src/services/langfuse/__tests__/langfuse.isolated.ts-335-343 (1)
335-343:⚠️ Potential issue | 🟡 MinorThe null-root no-op test has no assertion.
This case currently passes even if behavior regresses. Add explicit “no calls” assertions to lock in no-op guarantees.
[suggested fix]
Patch to make the test meaningful
test('no-ops when rootSpan is null', async () => { const { recordToolObservation } = await import('../tracing.js') + mockStartObservation.mockClear() + mockRootUpdate.mockClear() + mockRootEnd.mockClear() recordToolObservation(null, { toolName: 'BashTool', toolUseId: 'id1', input: {}, output: 'out', }) + expect(mockStartObservation).not.toHaveBeenCalled() + expect(mockRootUpdate).not.toHaveBeenCalled() + expect(mockRootEnd).not.toHaveBeenCalled() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/langfuse/__tests__/langfuse.isolated.ts` around lines 335 - 343, The test "no-ops when rootSpan is null" currently has no assertions; update it to assert that no tracing/telemetry methods are called when recordToolObservation is invoked with a null rootSpan. Specifically, in the test that calls recordToolObservation(null, {...}), create spies/mocks for the tracing functions or objects used by recordToolObservation (e.g., any tracer.startSpan, span.addEvent, span.setAttribute, or telemetry client methods used in tracing.js) and assert they were not called (e.g., expect(spy).not.toHaveBeenCalled()) to lock in the no-op guarantee for recordToolObservation.packages/remote-control-server/src/routes/v1/sessions.ts-27-27 (1)
27-27:⚠️ Potential issue | 🟡 MinorUse a type guard instead of forcing
errtoError.
catchvalues can be non-Error; this cast violates the TS guideline to use type guards instead of forced casting.🔧 Suggested fix
- } catch (err) { - logError(`[RCS] Failed to create work item: ${(err as Error).message}`); + } catch (err: unknown) { + const message = err instanceof Error ? err.message : String(err); + logError(`[RCS] Failed to create work item: ${message}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/remote-control-server/src/routes/v1/sessions.ts` at line 27, Replace the forced cast of err to Error in the log call with a proper type guard: check if err is an instanceof Error (or if it has a string .message) and then pass the message, otherwise log a safe fallback (e.g., JSON.stringify(err) or String(err)). Update the log line that calls logError(`[RCS] Failed to create work item: ${(err as Error).message}`) to perform that guard so you never cast directly and always log a meaningful value.packages/remote-control-server/src/routes/web/sessions.ts-39-39 (1)
39-39:⚠️ Potential issue | 🟡 MinorReplace forced error cast with type guard.
The
catchblock uses an unsafe cast(err as Error)instead of type narrowing. Useerr: unknownand check withinstanceof Errorbefore accessing.message.🔧 Suggested fix
- } catch (err) { - logError(`[RCS] Failed to create work item: ${(err as Error).message}`); + } catch (err: unknown) { + const message = err instanceof Error ? err.message : String(err); + logError(`[RCS] Failed to create work item: ${message}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/remote-control-server/src/routes/web/sessions.ts` at line 39, The catch block currently force-casts the exception with (err as Error) when calling logError; change the catch parameter to err: unknown and use a type guard (if (err instanceof Error) ...) to access err.message, otherwise log a safe fallback (e.g. String(err) or a default message); update the logError call in the same catch so it uses the narrowed error message, referencing the existing logError call in the sessions.ts catch block to locate where to apply the change.src/cli/handlers/ant.ts-132-136 (1)
132-136:⚠️ Potential issue | 🟡 MinorGuard against partial numeric strings being treated as indexes.
parseInt()parses numeric prefixes, so a value like"12abc"parses to12. If a session ID starts with digits (e.g.,"12abc"), it will be incorrectly treated as an array index instead of a session ID. Add a check to ensure the entire string is numeric before callingparseInt().Also applies to: 170-176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/handlers/ant.ts` around lines 132 - 136, The current logic uses parseInt on logId which treats strings like "12abc" as numeric indexes; change the idx calculation so you only parse when the entire string is numeric (e.g., use a regex like /^\d+$/ on logId or Number.isInteger after Number(value)) and then convert with Number(...) (or parseInt only after the full-number test); update the following block that defines idx and uses logs and logs.find(l => l.sessionId === String(logId)) so sessionIds that start with digits are matched correctly, and apply the same fix to the other identical parsing occurrence that handles logId later in this file.docs/features/acp-zed.md-24-37 (1)
24-37:⚠️ Potential issue | 🟡 MinorAdd explicit languages to fenced code blocks to satisfy markdownlint.
The unlabeled code fences trigger MD040 and can fail docs lint checks.
Proposed fix
-``` +```text ┌──────────────┐ NDJSON/stdio ┌──────────────────┐ │ Zed / IDE │ ◄────────────────► │ CCB ACP Agent │ │ (Client) │ stdin / stdout │ (Agent) │ └──────────────┘ │ │ │ entry.ts │ ← stdio → NDJSON stream │ agent.ts │ ← ACP protocol handler │ bridge.ts │ ← SDKMessage → ACP SessionUpdate │ permissions.ts │ ← 权限桥接 │ utils.ts │ ← 通用工具 │ │ │ QueryEngine │ ← 内部查询引擎 └──────────────────┘-
+text
命令: ccb --acp
参数: ["--acp"]
通信: stdin/stdout NDJSON
协议版本: ACP v1Also applies to: 111-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/features/acp-zed.md` around lines 24 - 37, The unlabeled fenced code blocks (the ASCII diagram block and the command block starting with "命令: ccb --acp") trigger markdownlint MD040; add explicit language labels to both fenced blocks by changing their opening fences to use a language identifier such as ```text so the diagram and the command/parameters NDJSON example are rendered correctly and linted.src/utils/cliLaunch.ts-39-81 (1)
39-81:⚠️ Potential issue | 🟡 MinorThe edge cases for
-dand-d\tpatterns lack documented justification.The function correctly sanitizes leaked application arguments (addressing the real Bun issue
#11673documented at line 29). However, the checks at line 44 for-d(with space) and-d\t(with tab) as single arguments are questionable — the documented formats are-d KEY:VALUE(two args) or-dKEY:VALUE(single arg without space). There is no evidence these space/tab variants actually occur in practice; shell argument parsing would typically split or strip whitespace. These patterns appear defensive but have no test coverage or GitHub issue reference to justify them. If they were added to handle an observed leak pattern, that context should be documented in a code comment.The rest of the function is sound: the loop index mutations at lines 47, 59, and 73 intentionally consume argument pairs, and the non-null assertions are safe due to bounds checks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/cliLaunch.ts` around lines 39 - 81, The checks for '-d ' and '-d\t' in sanitizeExecArgv are undocumented and unclear; either remove those branches or add a short comment explaining why they're retained. Update the sanitizeExecArgv function to include a clarifying comment (referencing Bun issue `#11673` if this was to handle a known leak or describing the observed malformed-arg pattern) immediately above the Bun define flags block that handles '-d', '-d KEY:VALUE', '-dKEY:VALUE', '-d ' and '-d\t' so future readers know these space/tab variants are intentional defensive handling of malformed leaked args; if no evidence they occur, remove the '-d ' and '-d\t' checks and associated test gaps.docs/task/task-013-bg-engine-abstraction.md-5-5 (1)
5-5:⚠️ Potential issue | 🟡 MinorUpdate stale branch reference in task metadata.
Line 5 lists
feat/integrate-5-branches, but this PR is fromfeat/integrate-lint-preview. Keeping this accurate avoids confusion when tracing implementation context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/task/task-013-bg-engine-abstraction.md` at line 5, Update the stale branch identifier in the task metadata by replacing the string `feat/integrate-5-branches` with the correct branch name `feat/integrate-lint-preview` (the metadata entry shown on line with that branch reference); ensure any occurrences in the document are updated so the task accurately reflects this PR's branch.docs/task/task-013-bg-engine-abstraction.md-195-195 (1)
195-195:⚠️ Potential issue | 🟡 MinorUse the repo-standard TypeScript check command in the checklist.
Line 195 says
tsc --noEmit; please switch tobunx tsc --noEmitto match the project’s enforced TS strict verification flow.As per coding guidelines: “Use TypeScript strict mode —
bunx tsc --noEmitmust pass with zero errors”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/task/task-013-bg-engine-abstraction.md` at line 195, Update the checklist entry that currently reads "tsc --noEmit" to use the repo-standard command "bunx tsc --noEmit" so the project’s enforced TS strict verification runs; locate the checklist line containing the literal "tsc --noEmit" and replace it with "bunx tsc --noEmit".docs/task/task-016-backward-compat-tests.md-5-5 (1)
5-5:⚠️ Potential issue | 🟡 MinorUpdate stale branch reference in task metadata.
The document says
feat/integrate-5-branches, but this PR is onfeat/integrate-lint-preview. Keeping this aligned avoids confusion during traceability and backports.Suggested patch
-> 分支: `feat/integrate-5-branches` +> 分支: `feat/integrate-lint-preview`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/task/task-016-backward-compat-tests.md` at line 5, Update the stale branch reference string `feat/integrate-5-branches` to the correct branch `feat/integrate-lint-preview` in the task metadata so the document matches the PR branch; locate the branch literal in the file header or metadata section and replace it with `feat/integrate-lint-preview`.docs/features/daemon-restructure-design.md-58-58 (1)
58-58:⚠️ Potential issue | 🟡 MinorAdd language identifiers to fenced code blocks (MD040).
Line 58, Line 112, Line 126, Line 235, Line 250, Line 261, and Line 293 start fenced blocks without a language tag. Please label them (for example
text/bash/typescript) to satisfy markdown lint.Also applies to: 112-112, 126-126, 235-235, 250-250, 261-261, 293-293
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/features/daemon-restructure-design.md` at line 58, The file contains untagged fenced code blocks (MD040) at several locations; add appropriate language identifiers to each fenced block to satisfy markdown lint. Edit the fenced code blocks referenced (the blocks starting at the seven occurrences noted in the comment) and append a language hint such as ```bash, ```text, or ```typescript depending on the snippet content so each block is labeled. Ensure consistency (use bash for shell commands, typescript for code samples, text for plain output) and update all seven occurrences mentioned in the review.src/utils/__tests__/autonomyPersistence.test.ts-84-113 (1)
84-113:⚠️ Potential issue | 🟡 Minor“Parallel calls” assertion is too weak and can pass for serialized execution.
The current checks only verify start-before-end within each task. They don’t prove overlap across tasks, so a fully sequential run can still pass.
✅ Suggested assertion hardening
// Both should start before either ends (parallel) - expect(order.indexOf('a-start')).toBeLessThan(order.indexOf('a-end')) - expect(order.indexOf('b-start')).toBeLessThan(order.indexOf('b-end')) + const aStart = order.indexOf('a-start') + const aEnd = order.indexOf('a-end') + const bStart = order.indexOf('b-start') + const bEnd = order.indexOf('b-end') + expect(aStart).toBeGreaterThanOrEqual(0) + expect(aEnd).toBeGreaterThanOrEqual(0) + expect(bStart).toBeGreaterThanOrEqual(0) + expect(bEnd).toBeGreaterThanOrEqual(0) + // True overlap: each task starts before the other task ends + expect(aStart).toBeLessThan(bEnd) + expect(bStart).toBeLessThan(aEnd)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/__tests__/autonomyPersistence.test.ts` around lines 84 - 113, The current test for withAutonomyPersistenceLock only asserts each task's start comes before its own end, which allows serialized runs to pass; modify the assertions to prove overlap by checking interleaving between tasks (for example, assert order.indexOf('a-start') < order.indexOf('b-end') AND order.indexOf('b-start') < order.indexOf('a-end')) using the existing symbols withAutonomyPersistenceLock, tempDir, tempDir2, and the order array so the test fails if the tasks ran strictly sequentially.src/commands/job/job.tsx-13-13 (1)
13-13:⚠️ Potential issue | 🟡 MinorMissing React import for type annotation.
The return type
Promise<React.ReactNode>referencesReactbut the module doesn't import it. This may cause TypeScript errors depending on how the global types are configured.🔧 Proposed fix
import type { LocalJSXCommandOnDone, LocalJSXCommandContext } from '../../types/command.js' +import type React from 'react' /**Alternatively, if React types are globally available, consider using the explicit type:
-): Promise<React.ReactNode> { +): Promise<React.ReactElement | null> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/job/job.tsx` at line 13, The function signature uses the type Promise<React.ReactNode> but the module does not import React types; fix by importing the types from React: either add import React from 'react' if you want to keep React.ReactNode, or preferably add import type { ReactNode } from 'react' and change the signature to Promise<ReactNode> so TypeScript stops complaining about the missing React symbol; update the function declaration that currently returns Promise<React.ReactNode> accordingly.src/utils/autonomyPersistence.ts-19-22 (1)
19-22:⚠️ Potential issue | 🟡 MinorThe lock map cleanup compares against the wrong promise.
You store
previous.then(() => current)inpersistenceLocks, but thefinallyblock compares the map entry tocurrent. That condition never becomes true, so entries are never removed.Suggested fix
- persistenceLocks.set( - key, - previous.then(() => current), - ) + const queued = previous.then(() => current) + persistenceLocks.set(key, queued) @@ - if (persistenceLocks.get(key) === current) { + if (persistenceLocks.get(key) === queued) { persistenceLocks.delete(key) }Also applies to: 42-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/autonomyPersistence.ts` around lines 19 - 22, The lock cleanup compares against the wrong promise: when setting persistenceLocks you store previous.then(() => current) but the finally block compares map.get(key) to current so it never matches; fix by capturing the stored promise in a local (e.g., const stored = previous.then(() => current)), set persistenceLocks.set(key, stored), and in the finally block compare persistenceLocks.get(key) === stored before deleting; apply the same change to the other occurrence that uses the same pattern (the second block around the 42-45 area).src/proactive/useProactive.ts-75-87 (1)
75-87:⚠️ Potential issue | 🟡 MinorAdd error handling for async command generation.
The async IIFE silently swallows any errors from
createProactiveAutonomyCommands. If the command generation fails, ticks will continue scheduling but no commands will be queued, with no indication of failure.🛡️ Proposed fix to add error handling
void (async () => { + try { const commands = await createProactiveAutonomyCommands({ basePrompt: `<${TICK_TAG}>${new Date().toLocaleTimeString()}</${TICK_TAG}>`, currentDir: getCwd(), }) for (const command of commands) { // Always queue proactive turns. This avoids races where the prompt // is built asynchronously, a user turn starts meanwhile, and a // direct-submit path would silently drop the autonomy turn after // consuming its heartbeat due-state. optsRef.current.onQueueTick(command) } + } catch (err) { + // Log but don't crash — tick scheduling continues + console.error('[useProactive] Failed to create autonomy commands:', err) + } })()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/proactive/useProactive.ts` around lines 75 - 87, The async IIFE around createProactiveAutonomyCommands can throw and currently swallows errors; wrap the await/create loop in a try/catch, so failures are surfaced — e.g., try { const commands = await createProactiveAutonomyCommands({...}) ... } catch (err) { (optsRef.current?.onProactiveError ? optsRef.current.onProactiveError(err) : console.error("proactive command generation failed", err)); } — reference createProactiveAutonomyCommands, optsRef.current.onQueueTick, TICK_TAG and getCwd in the fix so the catch logs the error and invokes an existing error handler on optsRef if available.docs/task/task-015-job-command-hierarchy.md-157-164 (1)
157-164:⚠️ Potential issue | 🟡 MinorDocumentation example uses CommonJS
require()in an ESM codebase.The code example uses
require('./commands/job/index.js')but the codebase uses ESM with"type": "module". This should use dynamicimport()or top-levelimportfor consistency with the actual implementation.📝 Suggested documentation fix
-const jobCmd = feature('TEMPLATES') - ? require('./commands/job/index.js').default - : null +// At top level or in COMMANDS(): +// Option 1: Conditional spread with lazy import in command definition +// Option 2: Dynamic import pattern +const jobCmd = feature('TEMPLATES') + ? (await import('./commands/job/index.js')).default + : nullOr use the pattern shown in the
job/index.tsexample above which usesload: () => import('./job.js')for lazy loading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/task/task-015-job-command-hierarchy.md` around lines 157 - 164, The documentation example incorrectly uses CommonJS require() to load the job command; update it to use ESM dynamic import or the documented lazy-load pattern: replace the require('./commands/job/index.js') usage in the jobCmd expression with a dynamic import() (or mirror the load: () => import('./job.js') pattern shown in job/index.ts) so that feature('TEMPLATES') resolves jobCmd via ESM import semantics instead of require.docs/test-plans/openclaw-autonomy-baseline.md-11-11 (1)
11-11:⚠️ Potential issue | 🟡 MinorFix typo in repository name.
"Claude-code-bast" should be "Claude-code-best" to match the repository name.
📝 Proposed fix
-Establish a stable baseline around the parts of `Claude-code-bast` that later autonomy work is most likely to touch: +Establish a stable baseline around the parts of `Claude-code-best` that later autonomy work is most likely to touch:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/test-plans/openclaw-autonomy-baseline.md` at line 11, Replace the typo "Claude-code-bast" with the correct repository name "Claude-code-best" in the openclaw autonomy baseline text (the string "Claude-code-bast" should be updated to "Claude-code-best") so the document references the correct repository.src/utils/handlePromptSubmit.ts-29-34 (1)
29-34:⚠️ Potential issue | 🟡 MinorRemove unused import
markAutonomyRunFailed.The import on line 32 is never used in this file. The error path correctly uses
finalizeAutonomyRunFailedinstead. Remove it to keep imports clean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/handlePromptSubmit.ts` around lines 29 - 34, The import list in handlePromptSubmit.ts includes an unused symbol markAutonomyRunFailed; remove markAutonomyRunFailed from the import statement that currently imports finalizeAutonomyRunCompleted, finalizeAutonomyRunFailed, markAutonomyRunFailed, markAutonomyRunRunning so only the actually used symbols remain (e.g., finalizeAutonomyRunCompleted, finalizeAutonomyRunFailed, markAutonomyRunRunning).src/__tests__/context.baseline.test.ts-77-90 (1)
77-90:⚠️ Potential issue | 🟡 MinorConditional assertion weakens test coverage.
The
if ('cacheBreaker' in second)branch means the test passes regardless of whethersetSystemPromptInjectionactually injects thecacheBreakerfield. If the implementation changes and stops injecting it, the test still passes via theelsebranch. Consider making the assertion deterministic:🧪 Proposed fix to strengthen the assertion
test('getSystemContext reflects system prompt injection after cache invalidation', async () => { const first = await getSystemContext() expect(first.gitStatus).toBeUndefined() - expect(first.cacheBreaker).toBeUndefined() setSystemPromptInjection('baseline-cache-break') const second = await getSystemContext() - if ('cacheBreaker' in second) { - expect(second.cacheBreaker).toContain('baseline-cache-break') - } else { - expect(second.gitStatus).toBeUndefined() - } + // Verify that calling getSystemContext after injection returns a fresh result + // (cache was invalidated). The specific shape depends on implementation. + expect(second).not.toBe(first) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/context.baseline.test.ts` around lines 77 - 90, The test currently uses a conditional that lets it pass whether or not setSystemPromptInjection actually adds cacheBreaker; make the assertion deterministic by removing the if/else and asserting that the returned object from getSystemContext (after calling setSystemPromptInjection) contains the cacheBreaker property and that its value contains 'baseline-cache-break' (use getSystemContext and setSystemPromptInjection and assert on cacheBreaker directly), while keeping the initial assertions for gitStatus/cacheBreaker being undefined before injection.src/services/acp/permissions.ts-171-183 (1)
171-183:⚠️ Potential issue | 🟡 Minor
handleExitPlanModelacks error handling unlike standard tool path.The standard tool permission path (lines 90-131) wraps
conn.requestPermission()in try/catch and returns a deny decision on failure.handleExitPlanModedoesn't have this protection—ifrequestPermission()throws, the error propagates. Consider adding consistent error handling:🛡️ Proposed fix to add error handling
async function handleExitPlanMode( ... ): Promise<PermissionAllowDecision | PermissionDenyDecision> { ... + try { const response = await conn.requestPermission({ sessionId, toolCall, options, }) + } catch { + return { + behavior: 'deny', + message: 'Permission request failed', + decisionReason: { type: 'mode', mode: 'plan' }, + } + } if (response.outcome.outcome === 'cancelled') {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/acp/permissions.ts` around lines 171 - 183, The handleExitPlanMode block calls conn.requestPermission(...) but lacks a try/catch like the standard tool path, so if requestPermission throws the error will bubble up; wrap the conn.requestPermission call inside a try/catch (as done in the standard tool permission path) and on error return the same deny decision object (behavior: 'deny', message like 'Tool permission error' or 'Tool use aborted', and decisionReason: { type: 'mode', mode: 'default' }) to keep behavior consistent with the rest of permissions handling.src/daemon/main.ts-247-254 (1)
247-254:⚠️ Potential issue | 🟡 MinorState file write errors could leave inconsistent state.
writeDaemonState()is called without error handling. If the write fails (e.g., disk full, permission denied), the supervisor continues running but other CLI processes can't discover it. Consider wrapping in try/catch with a warning log.🛡️ Proposed fix to handle write errors
// Write daemon state file so other CLI processes can query/stop us + try { writeDaemonState({ pid: process.pid, cwd: dir, startedAt: new Date().toISOString(), workerKinds: workers.map(w => w.kind), lastStatus: 'running', }) + } catch (err) { + console.warn(`[daemon] failed to write state file: ${errorMessage(err)}`) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daemon/main.ts` around lines 247 - 254, The call to writeDaemonState(...) can fail and is unhandled; wrap the writeDaemonState(...) invocation in a try/catch around the block that builds the state (including pid, cwd, startedAt, workerKinds) and on error log a warning that includes the caught error (e.g., via processLogger.warn or console.warn) so the daemon continues running but operators are informed; do not swallow the error silently—include context like "failed to write daemon state" and the error details in the log.src/hooks/useScheduledTasks.ts-115-122 (1)
115-122:⚠️ Potential issue | 🟡 MinorReplace
as stringcast with type guard to narrow union type.
command.valueis typed asstring | Array<ContentBlockParam>, but the code casts it to string without verification. Replace the cast with a type guard check:if (typeof command.value !== 'string') { // handle non-string case or return early } const injected = injectUserMessageToTeammate( teammate.id, command.value, // now safely narrowed to string // ... )This aligns with the guideline to use type guards for union types instead of forced casting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useScheduledTasks.ts` around lines 115 - 122, Replace the unsafe cast of command.value with a type guard: before calling injectUserMessageToTeammate, check that typeof command.value === 'string' (or handle the non-string branch, e.g., return early or map ContentBlockParam to a string), then pass the now-narrowed command.value into injectUserMessageToTeammate(teammate.id, command.value, { autonomyRunId: command.autonomy?.runId, origin: command.origin }, setAppState); ensure any non-string handling is explicit rather than using "as string".docs/features/stub-recovery-design-1-4.md-18-307 (1)
18-307:⚠️ Potential issue | 🟡 MinorReplace local filesystem links with repo-relative references.
The
</e:/Source_code/...>links only work on the author's machine, so they render as broken links in the repo and on GitHub. Please switch these to repo-relative markdown links or plain code paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/features/stub-recovery-design-1-4.md` around lines 18 - 307, The doc contains absolute local links like </e:/Source_code/...> that break on other machines; update all such occurrences in docs/features/stub-recovery-design-1-4.md to repo-relative references or plain code paths (e.g. change </e:/Source_code/Claude-code-bast/src/daemon/main.ts:1> to [src/daemon/main.ts](src/daemon/main.ts) or `src/daemon/main.ts`). Search for the pattern "</e:/Source_code/" and replace each instance referencing files such as src/daemon/main.ts, src/daemon/workerRegistry.ts, src/commands/remoteControlServer/remoteControlServer.tsx, src/entrypoints/cli.tsx, src/utils/concurrentSessions.ts, src/cli/bg.ts, src/utils/taskSummary.ts, src/cli/handlers/templateJobs.ts, src/utils/markdownConfigLoader.ts, src/jobs/classifier.ts, src/main.tsx, src/remote/RemoteSessionManager.ts, src/hooks/useAssistantHistory.ts, src/assistant/sessionHistory.ts, src/assistant/sessionDiscovery.ts, src/assistant/AssistantSessionChooser.ts, and src/commands/assistant/assistant.tsx to repo-relative markdown links or inline code paths.src/commands/assistant/assistant.tsx-78-91 (1)
78-91:⚠️ Potential issue | 🟡 MinorPotential race: error handler can fire after
onInstalledis called.The
child.on('error')handler and thesetTimeoutforonInstalledcan race. If the daemon spawns successfully but crashes within 1500ms,onInstalled(dir)will have already been called beforeonErrorfires, leaving the UI in an inconsistent state.🛡️ Proposed fix using a completion flag
function startDaemon(): void { if (starting) return; setStarting(true); const dir = defaultDir || resolve('.'); + let completed = false; try { const launch = buildCliLaunch(['daemon', 'start', `--dir=${dir}`]); const child = spawnCli(launch, { cwd: dir, stdio: 'ignore', detached: true, }); child.unref(); child.on('error', err => { + if (completed) return; + completed = true; onError(`Failed to start daemon: ${err.message}`); }); // Give the daemon a moment to initialize, then report success. setTimeout(() => { + if (completed) return; + completed = true; onInstalled(dir); }, 1500); } catch (err) { onError(`Failed to start daemon: ${err instanceof Error ? err.message : String(err)}`); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/assistant/assistant.tsx` around lines 78 - 91, The child spawn error handler (child.on('error')) can fire after the delayed setTimeout calls onInstalled(dir), causing a UI race; fix by introducing a completion flag and coordinating the two flows: add a boolean (e.g., "completed" or "finished") that is set when either onError(...) or onInstalled(dir) runs, have the error handler check the flag and no-op if already completed, and have the setTimeout handler check/set the same flag before calling onInstalled; alternatively remove the listener or use child.once and clear the timeout on error — update the child.on('error'), the setTimeout callback, and the catch block to use this shared flag so only the first outcome is emitted.src/utils/autonomyRuns.ts-236-260 (1)
236-260:⚠️ Potential issue | 🟡 MinorRun creation writes to file then updates flow outside the lock.
The
createAutonomyRunfunction writes the run record insidewithAutonomyPersistenceLock(lines 236-240), but then callsqueueManagedAutonomyFlowStepRun(lines 252-259) outside that lock. If the flow update fails, the run record exists but the flow state is inconsistent.🛡️ Consider wrapping both operations in a single transaction
export async function createAutonomyRun(params: { // ... params }): Promise<AutonomyRunRecord> { // ... record creation - await withAutonomyPersistenceLock(rootDir, async () => { - const runs = await listAutonomyRuns(rootDir) - runs.unshift(record) - await writeAutonomyRuns(runs, rootDir) - }) - if ( - record.parentFlowId && - record.flowStepId && - record.parentFlowSyncMode === 'managed' - ) { - // ... flow step queueing - } + await withAutonomyPersistenceLock(rootDir, async () => { + const runs = await listAutonomyRuns(rootDir) + runs.unshift(record) + await writeAutonomyRuns(runs, rootDir) + if ( + record.parentFlowId && + record.flowStepId && + record.parentFlowSyncMode === 'managed' + ) { + // Note: queueManagedAutonomyFlowStepRun also uses the lock internally, + // so this would need refactoring to avoid nested locks or use a shared + // transaction pattern + } + }) return record }Alternatively, document that partial failures here leave the system in a recoverable state where the run exists but the flow needs manual reconciliation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/autonomyRuns.ts` around lines 236 - 260, The run record is persisted inside withAutonomyPersistenceLock but queueManagedAutonomyFlowStepRun is called after the lock, risking inconsistency if the flow update fails; modify createAutonomyRun so the logic that reads the parent flow (getAutonomyFlowById) and calls queueManagedAutonomyFlowStepRun is performed inside the same withAutonomyPersistenceLock block (after writeAutonomyRuns) or implement an explicit rollback/compensating step on failure; reference the existing symbols to change: withAutonomyPersistenceLock, writeAutonomyRuns, getAutonomyFlowById, queueManagedAutonomyFlowStepRun, and the record/runId/parentFlowId/flowStepId usage so both persistence and flow-queueing occur atomically (or clearly document and implement a safe compensating action).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 42aec06e-d123-466c-9989-33dc4ac93702
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (139)
.gitignore02-kairos (1).mdAGENTS.mdREADME.mdbuild.tsdocs/features/acp-zed.mddocs/features/daemon-restructure-design.mddocs/features/stub-recovery-design-1-4.mddocs/task/task-001-daemon-status-stop.mddocs/task/task-002-bg-sessions-ps-logs-kill.mddocs/task/task-003-templates-job-mvp.mddocs/task/task-004-assistant-session-attach.mddocs/task/task-013-bg-engine-abstraction.mddocs/task/task-014-daemon-command-hierarchy.mddocs/task/task-015-job-command-hierarchy.mddocs/task/task-016-backward-compat-tests.mddocs/test-plans/openclaw-autonomy-baseline.mdpackage.jsonpackages/@ant/computer-use-mcp/src/toolCalls.tspackages/builtin-tools/src/tools/PushNotificationTool/PushNotificationTool.tspackages/builtin-tools/src/tools/SendUserFileTool/SendUserFileTool.tspackages/remote-control-server/src/logger.tspackages/remote-control-server/src/routes/v1/session-ingress.tspackages/remote-control-server/src/routes/v1/sessions.tspackages/remote-control-server/src/routes/web/control.tspackages/remote-control-server/src/routes/web/sessions.tspackages/remote-control-server/src/services/disconnect-monitor.tspackages/remote-control-server/src/services/work-dispatch.tspackages/remote-control-server/src/transport/event-bus.tspackages/remote-control-server/src/transport/sse-writer.tspackages/remote-control-server/src/transport/ws-handler.tsscripts/dev.tssrc/QueryEngine.tssrc/__tests__/context.baseline.test.tssrc/assistant/AssistantSessionChooser.tssrc/assistant/AssistantSessionChooser.tsxsrc/assistant/gate.tssrc/assistant/index.tssrc/assistant/sessionDiscovery.tssrc/bridge/bridgeMain.tssrc/cli/bg.tssrc/cli/bg/__tests__/detached.test.tssrc/cli/bg/__tests__/engine.test.tssrc/cli/bg/__tests__/tail.test.tssrc/cli/bg/engine.tssrc/cli/bg/engines/detached.tssrc/cli/bg/engines/index.tssrc/cli/bg/engines/tmux.tssrc/cli/bg/tail.tssrc/cli/handlers/ant.tssrc/cli/handlers/templateJobs.tssrc/cli/print.tssrc/cli/rollback.tssrc/cli/up.tssrc/commands.tssrc/commands/__tests__/autonomy.test.tssrc/commands/__tests__/proactive.baseline.test.tssrc/commands/assistant/assistant.tssrc/commands/assistant/assistant.tsxsrc/commands/assistant/gate.tssrc/commands/autonomy.tssrc/commands/daemon/__tests__/daemon.test.tssrc/commands/daemon/daemon.tsxsrc/commands/daemon/index.tssrc/commands/init.tssrc/commands/job/__tests__/job.test.tssrc/commands/job/index.tssrc/commands/job/job.tsxsrc/commands/lang/index.tssrc/commands/lang/lang.tssrc/commands/remoteControlServer/remoteControlServer.tsxsrc/commands/send/send.tssrc/commands/torch.tssrc/daemon/__tests__/daemonMain.test.tssrc/daemon/__tests__/state.test.tssrc/daemon/main.tssrc/daemon/state.tssrc/entrypoints/cli.tsxsrc/hooks/useAwaySummary.tssrc/hooks/useMasterMonitor.tssrc/hooks/usePipeIpc.tssrc/hooks/usePipeMuteSync.tssrc/hooks/usePipePermissionForward.tssrc/hooks/usePipeRelay.tssrc/hooks/useScheduledTasks.tssrc/jobs/__tests__/classifier.test.tssrc/jobs/__tests__/state.test.tssrc/jobs/__tests__/templates.test.tssrc/jobs/classifier.tssrc/jobs/state.tssrc/jobs/templates.tssrc/main.tsxsrc/proactive/__tests__/state.baseline.test.tssrc/proactive/useProactive.tssrc/screens/REPL.tsxsrc/services/acp/__tests__/agent.test.tssrc/services/acp/__tests__/bridge.test.tssrc/services/acp/__tests__/permissions.test.tssrc/services/acp/agent.tssrc/services/acp/bridge.tssrc/services/acp/entry.tssrc/services/acp/permissions.tssrc/services/acp/utils.tssrc/services/analytics/growthbook.tssrc/services/api/openai/__tests__/queryModelOpenAI.isolated.tssrc/services/api/openai/__tests__/streamAdapter.test.tssrc/services/awaySummary.tssrc/services/langfuse/__tests__/langfuse.isolated.tssrc/tasks/InProcessTeammateTask/InProcessTeammateTask.tsxsrc/tasks/InProcessTeammateTask/types.tssrc/types/textInputTypes.tssrc/utils/__tests__/autonomyAuthority.test.tssrc/utils/__tests__/autonomyFlows.test.tssrc/utils/__tests__/autonomyPersistence.test.tssrc/utils/__tests__/autonomyRuns.test.tssrc/utils/__tests__/cronScheduler.baseline.test.tssrc/utils/__tests__/cronTasks.baseline.test.tssrc/utils/__tests__/language.test.tssrc/utils/__tests__/pipeMuteState.test.tssrc/utils/__tests__/taskSummary.test.tssrc/utils/autonomyAuthority.tssrc/utils/autonomyFlows.tssrc/utils/autonomyPersistence.tssrc/utils/autonomyRuns.tssrc/utils/cliLaunch.tssrc/utils/config.tssrc/utils/handlePromptSubmit.tssrc/utils/language.tssrc/utils/pipeMuteState.tssrc/utils/pipePermissionRelay.tssrc/utils/pipeTransport.tssrc/utils/swarm/inProcessRunner.tssrc/utils/swarm/spawnInProcess.tssrc/utils/taskSummary.tssrc/utils/windowsPaths.tstests/integration/cli-arguments.test.tstests/mocks/file-system.tstsconfig.base.jsontsconfig.json
💤 Files with no reviewable changes (3)
- src/assistant/AssistantSessionChooser.ts
- src/hooks/useAwaySummary.ts
- src/commands/assistant/assistant.ts
| const body = await c.req.json(); | ||
| const eventType = body.type || "user"; | ||
| console.log(`[RC-DEBUG] web -> server: POST /web/sessions/${sessionId}/events type=${eventType} content=${JSON.stringify(body).slice(0, 200)}`); | ||
| log(`[RC-DEBUG] web -> server: POST /web/sessions/${sessionId}/events type=${eventType} content=${JSON.stringify(body).slice(0, 200)}`); |
Contributor
There was a problem hiding this comment.
Avoid logging raw user payload content in debug logs.
This line logs request content directly; it can leak sensitive user data into production logs.
🔐 Suggested fix
- log(`[RC-DEBUG] web -> server: POST /web/sessions/${sessionId}/events type=${eventType} content=${JSON.stringify(body).slice(0, 200)}`);
+ const payloadKeys =
+ body && typeof body === "object" ? Object.keys(body as Record<string, unknown>).join(",") : typeof body;
+ log(`[RC-DEBUG] web -> server: POST /web/sessions/${sessionId}/events type=${eventType} payload_keys=${payloadKeys}`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| log(`[RC-DEBUG] web -> server: POST /web/sessions/${sessionId}/events type=${eventType} content=${JSON.stringify(body).slice(0, 200)}`); | |
| const payloadKeys = | |
| body && typeof body === "object" ? Object.keys(body as Record<string, unknown>).join(",") : typeof body; | |
| log(`[RC-DEBUG] web -> server: POST /web/sessions/${sessionId}/events type=${eventType} payload_keys=${payloadKeys}`); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/remote-control-server/src/routes/web/control.ts` at line 48, The
debug log currently prints raw request payload via body (log(`[RC-DEBUG] ...
content=${JSON.stringify(body)...}`) which can leak sensitive data; update the
log in the POST /web/sessions/... handler to stop including raw body
contents—either remove the body portion entirely or replace it with a safe
summary (e.g., eventType, sessionId, payload size, or a redacted/sanitized/hash
of body) and keep the existing truncation behavior if needed; locate the log
call referencing sessionId, eventType, and body and replace it with a
metadata-only or sanitized message.
| try { | ||
| const sdkMsg = toSDKMessage(event); | ||
| console.log(`[RC-DEBUG] [WS] -> bridge (outbound): type=${event.type} len=${sdkMsg.length} msg=${sdkMsg.slice(0, 300)}`); | ||
| log(`[RC-DEBUG] [WS] -> bridge (outbound): type=${event.type} len=${sdkMsg.length} msg=${sdkMsg.slice(0, 300)}`); |
Contributor
There was a problem hiding this comment.
Avoid logging raw message payloads on WS ingress/egress.
Line 135 and Line 220 include serialized message content in logs. That can leak prompts/user content or other sensitive fields into persistent logs.
🔧 Proposed safe logging diff
- log(`[RC-DEBUG] [WS] -> bridge (outbound): type=${event.type} len=${sdkMsg.length} msg=${sdkMsg.slice(0, 300)}`);
+ log(`[RC-DEBUG] [WS] -> bridge (outbound): type=${event.type} len=${sdkMsg.length}`);
@@
- log(`[RC-DEBUG] [WS] <- bridge (inbound): sessionId=${sessionId} type=${eventType}${msg.uuid ? ` uuid=${msg.uuid}` : ""} msg=${JSON.stringify(msg).slice(0, 300)}`);
+ log(
+ `[RC-DEBUG] [WS] <- bridge (inbound): sessionId=${sessionId} type=${eventType}${msg.uuid ? ` uuid=${msg.uuid}` : ""}`,
+ );Also applies to: 220-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/remote-control-server/src/transport/ws-handler.ts` at line 135, The
logs currently include raw serialized payloads (sdkMsg) in the log call that can
leak sensitive content; update the WS logging in ws-handler (the log call that
references event.type and sdkMsg) to stop printing full message bodies—log only
non-sensitive metadata (e.g., event.type, message length), a truncated safe
prefix if absolutely needed, or a deterministic hash/ID of sdkMsg, and remove or
redact user/content fields before logging; ensure both occurrences that
reference sdkMsg (the outbound log and the ingress counterpart) are changed
consistently and keep the same log function/format.
Comment on lines
+27
to
+33
| useKeybindings( | ||
| { | ||
| 'select:next': () => setFocusIndex(i => (i + 1) % sessions.length), | ||
| 'select:previous': () => setFocusIndex(i => (i - 1 + sessions.length) % sessions.length), | ||
| 'select:accept': () => onSelect(sessions[focusIndex]!.id), | ||
| }, | ||
| { context: 'Select' }, |
Contributor
There was a problem hiding this comment.
Handle empty sessions defensively to avoid runtime breakage.
If sessions is empty, Line 29/Line 30 perform modulo by zero and Line 31 dereferences sessions[focusIndex]!.id, which can crash on accept.
🛡️ Suggested defensive fix
export function AssistantSessionChooser({ sessions, onSelect, onCancel }: Props): React.ReactNode {
useRegisterOverlay('assistant-session-chooser');
const [focusIndex, setFocusIndex] = useState(0);
+ const hasSessions = sessions.length > 0;
useKeybindings(
{
- 'select:next': () => setFocusIndex(i => (i + 1) % sessions.length),
- 'select:previous': () => setFocusIndex(i => (i - 1 + sessions.length) % sessions.length),
- 'select:accept': () => onSelect(sessions[focusIndex]!.id),
+ 'select:next': () => {
+ if (!hasSessions) return;
+ setFocusIndex(i => (i + 1) % sessions.length);
+ },
+ 'select:previous': () => {
+ if (!hasSessions) return;
+ setFocusIndex(i => (i - 1 + sessions.length) % sessions.length);
+ },
+ 'select:accept': () => {
+ if (!hasSessions) return;
+ onSelect(sessions[focusIndex]!.id);
+ },
},
{ context: 'Select' },
);Also applies to: 41-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/assistant/AssistantSessionChooser.tsx` around lines 27 - 33, The
keybinding handlers in useKeybindings use sessions.length and
sessions[focusIndex] unsafely; update the handlers in useKeybindings (the
'select:next', 'select:previous', 'select:accept' callbacks) to guard against an
empty sessions array by no-op-ing when sessions.length === 0, e.g., avoid
modulo-by-zero in setFocusIndex and check that sessions[focusIndex] exists
before calling onSelect; apply the same defensive checks to the other keybinding
block that mirrors these handlers (the second useKeybindings at lines 41-49) so
both navigation and accept are safe when sessions is empty.
Comment on lines
+307
to
+312
| const logPath = join( | ||
| getClaudeConfigHomeDir(), | ||
| 'sessions', | ||
| 'logs', | ||
| `${sessionName}.log`, | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if engine implementations create the log directory
rg -n "mkdir|mkdirSync" --type=ts src/cli/bg/engines/Repository: claude-code-best/claude-code
Length of output: 248
🏁 Script executed:
#!/bin/bash
# Find all engine implementations and check their file structure
fd -t f "\.ts$" src/cli/bg/engines/
# Check if other engines also handle directory creation
for file in $(fd -t f "\.ts$" src/cli/bg/engines/); do
echo "=== $file ==="
rg -A 5 "mkdirSync|mkdir" "$file" || echo "No mkdir found"
doneRepository: claude-code-best/claude-code
Length of output: 748
🏁 Script executed:
#!/bin/bash
# Check index.ts to see which engines are exposed
cat -n src/cli/bg/engines/index.ts
# Check tmux.ts to see how it handles logPath
cat -n src/cli/bg/engines/tmux.tsRepository: claude-code-best/claude-code
Length of output: 3502
🏁 Script executed:
#!/bin/bash
# Check how the log directory is handled in the actual session startup
# Look for where CLAUDE_CODE_SESSION_LOG is used
rg "CLAUDE_CODE_SESSION_LOG" --type=ts -A 3 -B 3
# Also check the main bg.ts file to see the full flow
cat -n src/cli/bg.ts | head -50
# Check around the logPath creation in bg.ts
sed -n '300,330p' src/cli/bg.tsRepository: claude-code-best/claude-code
Length of output: 3918
🏁 Script executed:
#!/bin/bash
# Check where CLAUDE_CODE_SESSION_LOG is actually used in the codebase
rg "CLAUDE_CODE_SESSION_LOG|process.env.CLAUDE_CODE_SESSION_LOG" --type=ts -B 2 -A 5
# Also check the concurrentSessions.ts file more thoroughly
sed -n '1,100p' src/utils/concurrentSessions.tsRepository: claude-code-best/claude-code
Length of output: 4993
🏁 Script executed:
#!/bin/bash
# Search for where the logs directory might be created
rg "sessions.*logs|logs.*mkdir|mkdirSync.*logs" --type=ts -B 2 -A 2
# Check if there's any setup code in the main entry point that creates logs directory
rg "logs" --type=ts src/cli/bg.ts -B 2 -A 2
# Look for where the logPath is actually used to write logs
rg "logPath|logFd|appendFile|writeFile" --type=ts src/cli/bg/engines/ -B 2 -A 2Repository: claude-code-best/claude-code
Length of output: 2966
🏁 Script executed:
#!/bin/bash
# Check what happens when a bg session starts - look for any logging setup
rg "CLAUDE_CODE_SESSION_LOG|logPath" --type=ts src/ -B 3 -A 3 | head -100
# Look specifically for where logs might be written in the session startup
fd -t f "\.ts$" src/ -x grep -l "mkdir.*logs\|mkdirSync.*logs" {} \;
# Check if there's any logging/redirection in the main startup code
cat -n src/bootstrap/state.ts | head -80Repository: claude-code-best/claude-code
Length of output: 9111
Create the sessions/logs directory before calling engine.start().
The DetachedEngine creates the log directory, but TmuxEngine does not. To ensure directory creation works consistently across all engines, move the mkdirSync call into bg.ts before passing logPath to engine.start(), or add directory creation to TmuxEngine's start() method.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/bg.ts` around lines 307 - 312, The code constructs logPath in bg.ts
but may pass it to engine.start() before the sessions/logs directory exists
(DetachedEngine creates it but TmuxEngine does not), so ensure the directory is
created up-front: before calling engine.start(logPath) in bg.ts, call mkdirSync
(or equivalent) to create getClaudeConfigHomeDir()/sessions/logs (recursive) so
both DetachedEngine and TmuxEngine receive a valid path; alternatively, add the
same directory creation inside TmuxEngine.start(), but prefer creating it in
bg.ts next to the logPath construction to guarantee consistency.
Comment on lines
+51
to
+55
| watchFile(logPath, { interval: 300 }, () => { | ||
| try { | ||
| const stat = statSync(logPath) | ||
| if (stat.size <= position) return | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current truncation handling and coverage around tailLog.
rg -nP --type=ts -C3 'stat\.size\s*<=\s*position|tailLog\s*\(' src/cli/bg
rg -nP --type=ts -C3 'truncate|rotation|rotated|truncat' src/cli/bg/__tests__Repository: claude-code-best/claude-code
Length of output: 1142
🏁 Script executed:
#!/bin/bash
# Get full tail.ts file to understand complete implementation
wc -l src/cli/bg/tail.ts
cat -n src/cli/bg/tail.tsRepository: claude-code-best/claude-code
Length of output: 2437
🏁 Script executed:
#!/bin/bash
# Check for test files related to tail or bg
fd -e test.ts -e spec.ts -e test.js -e spec.js src/cli/bg/
ls -la src/cli/bg/__tests__/ 2>/dev/null || echo "No __tests__ directory"Repository: claude-code-best/claude-code
Length of output: 742
🏁 Script executed:
cat -n src/cli/bg/__tests__/tail.test.tsRepository: claude-code-best/claude-code
Length of output: 361
Handle log truncation/rotation to avoid missing rotated log content.
The condition if (stat.size <= position) return skips reading when the file hasn't grown, but when a log file rotates (shrinks then grows), you miss the content between the truncation point and the current file size. For example, if the file shrinks from 100 bytes to 10 bytes and grows to 150 bytes, the function reads only bytes 100–150 of the new file, skipping bytes 10–100. Reset position on truncation detection to capture the full rotated log.
Proposed fix
- if (stat.size <= position) return
+ if (stat.size < position) {
+ // rotated/truncated log: restart from beginning
+ position = 0
+ }
+ if (stat.size === position) returnAdditionally, test coverage for this scenario is missing — tail.test.ts only validates module exports with no functional tests for truncation or rotation behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| watchFile(logPath, { interval: 300 }, () => { | |
| try { | |
| const stat = statSync(logPath) | |
| if (stat.size <= position) return | |
| watchFile(logPath, { interval: 300 }, () => { | |
| try { | |
| const stat = statSync(logPath) | |
| if (stat.size < position) { | |
| // rotated/truncated log: restart from beginning | |
| position = 0 | |
| } | |
| if (stat.size === position) return |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/bg/tail.ts` around lines 51 - 55, The watcher callback in tail.ts
currently ignores truncation because it returns when stat.size <= position;
change this to detect file rotation/truncation by checking if stat.size <
position and resetting position (e.g., position = 0) before reading so the
rotated/truncated file content between the new start and current size is read;
update the logic in the watchFile callback around statSync(logPath) and the
position variable in the tail function to handle both growth and shrinkage. Also
add unit tests in tail.test.ts that simulate truncation/rotation (write initial
content, truncate to smaller size, append new content) and assert the tail
reader emits the expected post-rotation bytes. Ensure tests exercise the code
path that sets position back to handle stat.size < position.
Comment on lines
+121
to
+141
| /** Business message types that should be dropped when a slave is muted. */ | ||
| const MUTED_DROPPABLE_TYPES = new Set([ | ||
| 'prompt_ack', | ||
| 'stream', | ||
| 'tool_start', | ||
| 'tool_result', | ||
| 'done', | ||
| 'error', | ||
| 'permission_request', | ||
| 'permission_cancel', | ||
| ]) | ||
|
|
||
| /** | ||
| * Centralized mute check used by both attachPipeEntryEmitter and | ||
| * useMasterMonitor's inline handler — keeps the two gates in sync. | ||
| */ | ||
| export function shouldDropMutedMessage(slaveName: string, msgType: string): boolean { | ||
| if (hasSendOverride(slaveName)) return false | ||
| if (!isMasterPipeMuted(slaveName)) return false | ||
| return MUTED_DROPPABLE_TYPES.has(msgType) | ||
| } |
Contributor
There was a problem hiding this comment.
Keep terminal slave events out of the muted drop set.
Including done and error in MUTED_DROPPABLE_TYPES means muting a slave mid-turn suppresses the only events that clear its busy state. The master can stay stuck showing that slave as active forever after a muted completion.
Minimal fix
const MUTED_DROPPABLE_TYPES = new Set([
'prompt_ack',
'stream',
'tool_start',
'tool_result',
- 'done',
- 'error',
'permission_request',
'permission_cancel',
])Also applies to: 184-210, 312-320
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/useMasterMonitor.ts` around lines 121 - 141, The
MUTED_DROPPABLE_TYPES set currently includes terminal event types ('done' and
'error') which causes completion events to be dropped when a slave is muted,
leaving the master thinking the slave is still busy; update the set used by
shouldDropMutedMessage to exclude terminal terminal events by removing 'done'
and 'error' (and any other terminal terminal/cleanup events) from
MUTED_DROPPABLE_TYPES so that shouldDropMutedMessage will allow those events
through even when isMasterPipeMuted(slaveName) is true; ensure the same change
is applied to the other occurrences mentioned (lines ~184-210 and ~312-320) so
all checks and sets remain consistent.
Comment on lines
+250
to
+255
| server.onMessage((msg: PipeMessage, _reply) => { | ||
| if (msg.type === 'relay_mute') { | ||
| pp().setRelayMuted(true) | ||
| } else if (msg.type === 'relay_unmute') { | ||
| pp().setRelayMuted(false) | ||
| } |
Contributor
There was a problem hiding this comment.
Guard relay mute commands to attached-sub context only.
This block currently applies relay_mute/relay_unmute from any sender/role. Add a role/sender check so only the attached master can mutate relay state for a sub session.
Proposed fix
server.onMessage((msg: PipeMessage, _reply) => {
- if (msg.type === 'relay_mute') {
+ const pipeIpc = pt().getPipeIpc(store.getState())
+ if (pipeIpc.role !== 'sub') return
+ if (pipeIpc.attachedBy && msg.from && msg.from !== pipeIpc.attachedBy) return
+
+ if (msg.type === 'relay_mute') {
pp().setRelayMuted(true)
} else if (msg.type === 'relay_unmute') {
pp().setRelayMuted(false)
}
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/usePipeIpc.ts` around lines 250 - 255, The relay mute/unmute
handlers in server.onMessage must only act when the current session is an
attached sub and the message originates from the attached master: before calling
pp().setRelayMuted(true/false) add a guard that verifies the session is an
attached-sub (e.g., pp().isAttachedSub() or equivalent) and that the incoming
PipeMessage identifies the attached master (e.g., msg.sender ===
pp().getAttachedMasterId() or msg.role === 'attached_master'); if the checks
fail, ignore the message. Ensure these checks are applied for both 'relay_mute'
and 'relay_unmute' branches so only the attached master can mutate relay state.
Comment on lines
+18
to
+20
| export function getJobDir(jobId: string): string { | ||
| return join(getJobsDir(), jobId) | ||
| } |
Contributor
There was a problem hiding this comment.
Validate jobId before turning it into a filesystem path.
readJobState() and appendJobReply() take CLI-supplied job IDs, and join(getJobsDir(), jobId) will happily accept ../... or absolute paths. That lets callers read/write outside ~/.claude/jobs.
Suggested guard
+function assertValidJobId(jobId: string): void {
+ if (!/^[A-Za-z0-9_-]+$/.test(jobId)) {
+ throw new Error('Invalid job ID')
+ }
+}
+
export function getJobDir(jobId: string): string {
+ assertValidJobId(jobId)
return join(getJobsDir(), jobId)
}Also applies to: 59-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/jobs/state.ts` around lines 18 - 20, Ensure jobId cannot perform path
traversal or be an absolute path before building filesystem paths: in getJobDir
(and callers readJobState and appendJobReply) validate jobId against a strict
allowlist (e.g. /^[A-Za-z0-9_-]+$/) or reject if path.isAbsolute(jobId) or
jobId.includes('..') or contains path separators, and only then call
join(getJobsDir(), jobId); alternatively normalize the joined path and assert it
startsWith(getJobsDir()) and throw on failure. Update getJobDir, readJobState,
and appendJobReply to perform this validation and return/throw early for invalid
jobId values.
Comment on lines
1804
to
+1807
| feature("KAIROS") && | ||
| assistantModule?.isAssistantMode() && | ||
| assistantModule && | ||
| (assistantModule.isAssistantForced() || | ||
| (options as Record<string, unknown>).assistant === true) && |
Contributor
There was a problem hiding this comment.
This drops settings-driven assistant activation.
This guard now only passes for forced mode or --assistant. The persisted assistant path described above it no longer satisfies the predicate, so settings-enabled assistant sessions will skip the kairosEnabled/team initialization block entirely.
🐛 Suggested fix
if (
feature("KAIROS") &&
assistantModule &&
- (assistantModule.isAssistantForced() ||
+ (assistantModule.isAssistantMode() ||
+ assistantModule.isAssistantForced() ||
(options as Record<string, unknown>).assistant === true) &&
// Spawned teammates share the leader's cwd + settings.json, so
// the flag is true for them too. --agent-id being set📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| feature("KAIROS") && | |
| assistantModule?.isAssistantMode() && | |
| assistantModule && | |
| (assistantModule.isAssistantForced() || | |
| (options as Record<string, unknown>).assistant === true) && | |
| feature("KAIROS") && | |
| assistantModule && | |
| (assistantModule.isAssistantMode() || | |
| assistantModule.isAssistantForced() || | |
| (options as Record<string, unknown>).assistant === true) && |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main.tsx` around lines 1804 - 1807, The current guard using
feature("KAIROS") && assistantModule && (assistantModule.isAssistantForced() ||
(options as Record<string, unknown>).assistant === true) excludes
settings-driven assistant activation; restore the original behavior by adding
the persisted/settings-based check back into this conditional (alongside
assistantModule.isAssistantForced() and options.assistant) so that sessions
enabled via persisted settings still enter the kairosEnabled / team
initialization block; locate the predicate around feature("KAIROS") and
assistantModule and update it to include the persisted/settings flag (the same
flag used earlier to detect persisted assistant activation) rather than only
forced/CLI-driven activation.
Comment on lines
+4843
to
+4869
| const autonomyRunId = queuedCommand.autonomy?.runId; | ||
| if (autonomyRunId) { | ||
| void markAutonomyRunRunning(autonomyRunId); | ||
| } | ||
|
|
||
| void onQuery([userMessage], newAbortController, true, [], mainLoopModel) | ||
| .then(() => { | ||
| if (autonomyRunId) { | ||
| void finalizeAutonomyRunCompleted({ | ||
| runId: autonomyRunId, | ||
| currentDir: getCwd(), | ||
| priority: 'later', | ||
| }).then(nextCommands => { | ||
| for (const command of nextCommands) { | ||
| enqueue(command); | ||
| } | ||
| }); | ||
| } | ||
| }) | ||
| .catch((error: unknown) => { | ||
| if (autonomyRunId) { | ||
| void finalizeAutonomyRunFailed({ | ||
| runId: autonomyRunId, | ||
| error: String(error), | ||
| }); | ||
| } | ||
| logError(toError(error)); |
Contributor
There was a problem hiding this comment.
Chain autonomy persistence into the same error path.
markAutonomyRunRunning() and finalizeAutonomyRunCompleted() are both launched outside the awaited onQuery() chain. If either persistence call rejects, the outer .catch() never sees it, so you can end up with an unhandled rejection and a run stuck in running.
Suggested fix
const autonomyRunId = queuedCommand.autonomy?.runId;
- if (autonomyRunId) {
- void markAutonomyRunRunning(autonomyRunId);
- }
-
- void onQuery([userMessage], newAbortController, true, [], mainLoopModel)
- .then(() => {
- if (autonomyRunId) {
- void finalizeAutonomyRunCompleted({
- runId: autonomyRunId,
- currentDir: getCwd(),
- priority: 'later',
- }).then(nextCommands => {
- for (const command of nextCommands) {
- enqueue(command);
- }
- });
- }
- })
- .catch((error: unknown) => {
- if (autonomyRunId) {
- void finalizeAutonomyRunFailed({
- runId: autonomyRunId,
- error: String(error),
- });
- }
- logError(toError(error));
- });
+ void (async () => {
+ try {
+ if (autonomyRunId) {
+ await markAutonomyRunRunning(autonomyRunId);
+ }
+
+ await onQuery([userMessage], newAbortController, true, [], mainLoopModel);
+
+ if (autonomyRunId) {
+ const nextCommands = await finalizeAutonomyRunCompleted({
+ runId: autonomyRunId,
+ currentDir: getCwd(),
+ priority: 'later',
+ });
+ for (const command of nextCommands) {
+ enqueue(command);
+ }
+ }
+ } catch (error: unknown) {
+ if (autonomyRunId) {
+ await finalizeAutonomyRunFailed({
+ runId: autonomyRunId,
+ error: String(error),
+ }).catch(() => {});
+ }
+ logError(toError(error));
+ }
+ })();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/screens/REPL.tsx` around lines 4843 - 4869, The persistence calls for
autonomy runs are currently detached from the onQuery promise chain
(markAutonomyRunRunning is fire-and-forgot and finalizeAutonomyRunCompleted is
called inside a .then() without returning its promise), which can cause
unhandled rejections and stuck runs; fix by including both persistence promises
in the onQuery chain: call markAutonomyRunRunning and capture its promise (e.g.,
Promise.resolve(markAutonomyRunRunning(autonomyRunId))) and then return
onQuery(...).then(() => finalizeAutonomyRunCompleted({...}).then(nextCommands =>
{ for (const command of nextCommands) enqueue(command); })), ensuring the outer
promise returned from this block is the combined promise so any rejection from
markAutonomyRunRunning, onQuery, or
finalizeAutonomyRunCompleted/finalizeAutonomyRunFailed will propagate to the
existing .catch() that calls finalizeAutonomyRunFailed and logError; keep using
finalizeAutonomyRunFailed and enqueue as before.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements