feat(cli): add local control plane CLI - #2088
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe pull request implements Local Control Plane V1 and a bundled ChangesLocal Control Plane and Bundled CLI
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (16)
test/main/cli/runService.test.ts (1)
198-207: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the specific rejection for an oversized prompt.
rejects.toBeDefined()passes for any rejection, including a harness failure such as a missing route. The prompt limit is an input-bounding control, so pin the error code as the neighbouring tests do.♻️ Proposed change
await expect( invokeRoute(service, sessionsRunDetachedRoute.name, { prompt: 'x'.repeat(RUN_PROMPT_MAX_CHARACTERS + 1) }) - ).rejects.toBeDefined() + ).rejects.toMatchObject({ code: 'invalid_request' }) expect(lifecycle.createDetachedSession).not.toHaveBeenCalled()Adjust the expected code if the route reports a different one for input validation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/main/cli/runService.test.ts` around lines 198 - 207, Update the oversized-prompt test around invokeRoute to assert the specific input-validation error code, matching the neighboring tests and the code reported by sessionsRunDetachedRoute, instead of only checking that a rejection exists. Keep the assertion that createDetachedSession is not called.test/main/routes/routeRegistry.test.ts (1)
27-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the accept path for
requireRendererCaller.The tests only assert rejection for non-renderer callers. A regression that rejects a valid renderer caller would break every renderer-only route and stay undetected here.
♻️ Proposed addition
it.each<RouteContext>([Add before the
it.eachblock:it('returns the renderer caller at renderer boundaries', () => { const context = createRendererRouteContext(42, 7) expect(requireRendererCaller(context)).toEqual(context.caller) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/main/routes/routeRegistry.test.ts` around lines 27 - 55, Add a positive-path test alongside the existing renderer-boundary rejection cases, using createRendererRouteContext to construct a valid renderer context and asserting requireRendererCaller returns context.caller unchanged.test/main/contracts/localControl.test.ts (1)
30-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit the multi-violation negative cases into one case per constraint.
The descriptor object at Lines 32-41 violates five independent rules at once:
protocolVersionpinning, NUL rejection in the endpoint path, thepidlower bound, the token length, and the unknown-key rejection. A bare.toThrow()passes if only one rule survives. The token-length and NUL-path rules can then regress silently. The request object at Lines 60-66 has the same problem foridandmethod.♻️ Proposed restructuring
- it('rejects malformed descriptors and duplicate scopes', () => { - expect(() => - LocalControlDescriptorSchema.parse({ - protocolVersion: 2, - surfaceVersion: LOCAL_CONTROL_SURFACE_VERSION, - appVersion: '1.2.3', - endpoint: { kind: 'unix', path: '/tmp/deepchat.sock\0hidden' }, - pid: 0, - token: 'secret', - startedAt: 1_000, - ignored: true - }) - ).toThrow() + const validDescriptor = { + protocolVersion: LOCAL_CONTROL_PROTOCOL_VERSION, + surfaceVersion: LOCAL_CONTROL_SURFACE_VERSION, + appVersion: '1.2.3', + endpoint: { kind: 'unix', path: '/tmp/deepchat.sock' }, + pid: 42, + token: 'a'.repeat(43), + startedAt: 1_000 + } + + it.each([ + ['unsupported protocol version', { protocolVersion: 2 }], + ['NUL in the endpoint path', { endpoint: { kind: 'unix', path: '/tmp/deepchat.sock\0hidden' } }], + ['non-positive pid', { pid: 0 }], + ['short token', { token: 'secret' }], + ['unknown key', { ignored: true }] + ])('rejects a descriptor with %s', (_label, override) => { + expect(LocalControlDescriptorSchema.safeParse({ ...validDescriptor, ...override }).success).toBe( + false + ) + }) + + it('rejects duplicate scopes', () => { expect(() => LocalControlScopesSchema.parse(['models:invoke', 'models:invoke'])).toThrow( 'Duplicate local-control scope' ) })Apply the same one-violation-per-case split to the
LocalControlRpcRequestSchemanegative case foridandmethod.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/main/contracts/localControl.test.ts` around lines 30 - 68, Split the combined negative tests into separate cases, each violating exactly one constraint. In the descriptor tests around LocalControlDescriptorSchema.parse, add independent cases for protocolVersion, endpoint NUL rejection, pid lower bound, token length, and unknown-key rejection; in the LocalControlRpcRequestSchema tests, separate invalid id and invalid method cases while keeping all other fields valid.test/main/tool/agentTools/agentBashHandler.test.ts (1)
144-152: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the
as neverspies from private methods.
AgentBashHandleralready exposesAgentCommandEnvironmentPortto tests, andAgentCommandEnvironmentPort.createEnvironmentis spied on without a cast. Type the remaining test dependencies the same way instead of relying onvi.spyOn(handler as never, '...').🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/main/tool/agentTools/agentBashHandler.test.ts` around lines 144 - 152, Update the test spies for AgentBashHandler’s private methods, including prepareCommand and runShellProcess, to avoid as never casts. Use the existing AgentCommandEnvironmentPort-based typing pattern so these dependencies remain type-safe and consistent with the createEnvironment spy.Source: Coding guidelines
test/main/cli/args.test.ts (1)
122-126: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
DEFAULT_COMPUTE_TIMEOUT_MSinstead of the literal.This file already imports
DEFAULT_COMPUTE_TIMEOUT_MSand uses it at line 184 and line 242. Lines 124, 406, and 647 assert the same value as the literal1_800_000. Use the imported constant at those three sites so a change to the default updates every expectation together.♻️ Proposed change (apply at lines 124, 406, and 647)
operation: 'stream', readStdin: true, - timeoutMs: 1_800_000, + timeoutMs: DEFAULT_COMPUTE_TIMEOUT_MS, params: {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/main/cli/args.test.ts` around lines 122 - 126, Replace the literal 1_800_000 with the imported DEFAULT_COMPUTE_TIMEOUT_MS in the expectations at the stream operation block and the corresponding sites around lines 406 and 647, preserving the existing assertions.test/main/cli/launcherService.test.ts (1)
225-254: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPOSIX-only filesystem assertions run without a platform guard. Both suites assert real host filesystem semantics — directory and file symbolic links, and POSIX permission bits — while the PR packages and ships the CLI for Windows. On Windows,
fs.promises.symlinktypically fails withEPERMfor unprivileged accounts, andmode & 0o111is always0. Each case then fails for a host reason rather than a product reason.
test/main/cli/launcherService.test.ts#L225-L254: gate this case and the related permission-bit andchmodcases (lines 94, 270, 306, 321-332) on a non-win32host.test/main/cli/mediaOutput.test.ts#L89-L105: skip this case onwin32, or pass the'junction'type for the directory link at line 93.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/main/cli/launcherService.test.ts` around lines 225 - 254, Gate the POSIX-specific assertions in launcherService.test and mediaOutput.test so they do not run on win32 hosts: update the launcherService test block around the fails-closed scenario, and the related permission-bit/chmod cases in the same file, to only execute on non-win32, and either skip the mediaOutput link assertion on win32 or switch its directory symlink setup to use the junction form. Keep the existing product assertions and test flow unchanged for non-Windows platforms.src/main/cli/surface.ts (1)
256-267: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider stripping control characters on the reviewable branch.
When
includeReviewableValuesis true,descriptionandiconpass through unchanged.agentMcpAddInputAllowedrejects directional controls, so endpoint spoofing in the approval dialog is already blocked. It does not reject C0/C1 control characters, which the non-reviewable branch removes throughsanitizePublicText. The result is display noise in the approval dialog rather than a bypass.A control-character strip that preserves full length keeps the value fully reviewable and removes the difference between the two branches.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/cli/surface.ts` around lines 256 - 267, The reviewable branch should also remove C0/C1 control characters while preserving the full remaining value for display. Update the includeReviewableValues handling in the description and icon projection logic, reusing the existing sanitization behavior or helper without truncating reviewable text, while leaving directional-control rejection and non-reviewable truncation behavior unchanged.src/shared/contracts/routes/cli.routes.ts (1)
19-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the shared local-control schemas instead of redefining them. Both sites redeclare a contract that
src/shared/contracts/localControl.tsalready exports, so the principal and scope contracts now have more than one definition. Import the existing export at each site.
src/shared/contracts/routes/cli.routes.ts#L19-L20: replacez.array(LocalControlScopeSchema).min(1)withLocalControlScopesSchema.min(1), which already bounds the array and rejects duplicates, and add a duplicate check tocallers.src/shared/contracts/events/approvals.events.ts#L13-L13: replace the inlinez.enum(['human', 'agent'])withLocalControlPrincipalSchema, imported from the../localControlmodule this file already imports from.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/contracts/routes/cli.routes.ts` around lines 19 - 20, Reuse the shared local-control schemas at both sites: in src/shared/contracts/routes/cli.routes.ts lines 19-20, use LocalControlScopesSchema.min(1) for scopes and add duplicate rejection to callers while using the shared principal schema; in src/shared/contracts/events/approvals.events.ts line 13, replace the inline principal enum with LocalControlPrincipalSchema from the existing ../localControl import.src/cli/format.ts (2)
26-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an exhaustive default branch to
formatHumanResult.The switch has no
defaultbranch, and the function declares astringreturn type. The code compiles today only because the cases cover every member ofCliRpcContract. If a new contract is added to the union and this switch is not updated, the function returnsundefinedat runtime and the CLI printsundefined. Aneverguard converts that into a compile-time error.♻️ Proposed exhaustiveness guard
].join('\n') } + default: { + const unhandled: never = contract + throw new Error( + `Unsupported CLI contract: ${(unhandled as { name: string }).name}` + ) + } } }Also applies to: 279-280
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/format.ts` at line 26, Update the switch in formatHumanResult to include an exhaustive default branch that passes the unmatched contract value to a never guard, ensuring newly added CliRpcContract members produce a compile-time error instead of allowing an undefined return. Preserve the existing formatting cases and string return behavior.
199-208: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
formatMcpRuntimein themcp.listPublicbranch.Line 204 repeats the runtime formatting logic that
formatMcpRuntimealready implements, with a different label (runtime-unknowninstead ofunknown). Reuse the helper to keep one runtime label per state.♻️ Proposed refactor
- `${server.name} ${server.type} ${server.enabled ? 'enabled' : 'disabled'} ${server.running === null ? 'runtime-unknown' : server.running ? 'running' : 'stopped'} ${server.managedBy}${server.metadataTruncated ? ' metadata-truncated' : ''} ${server.description}` + `${server.name} ${server.type} ${server.enabled ? 'enabled' : 'disabled'} ${formatMcpRuntime(server.running)} ${server.managedBy}${server.metadataTruncated ? ' metadata-truncated' : ''} ${server.description}`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/format.ts` around lines 199 - 208, Update the mcp.listPublic branch to call the existing formatMcpRuntime helper for each server’s running state instead of duplicating the ternary formatting. Preserve the surrounding server fields and truncation message while using the helper’s canonical runtime labels.src/main/cli/artifactRoutes.ts (1)
36-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument why
artifacts.readreturns only metadata.The
artifacts.readhandler body is identical to theartifacts.describehandler body. Both callartifactSpool.describe. The byte payload arrives over the separateGETartifact path thatsrc/main/cli/server.tsdispatches tohandleArtifactDownload, andsrc/cli/format.tsline 75 reports the saved size from that download. The design is correct, but the two identical bodies invite a future reader to treat one of them as a copy-paste defect.Add a short comment that states
artifacts.readis the metadata pre-flight for the streamed download.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/cli/artifactRoutes.ts` around lines 36 - 45, Add a concise comment immediately above the artifacts.read handler in the route definitions explaining that it returns metadata as the pre-flight for the separately streamed artifact download. Leave the existing parsing, caller validation, and artifactSpool.describe behavior unchanged.src/cli/index.ts (1)
14-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
ignoreBrokenPipeto match its behavior.The function does not ignore the error. It terminates the process: exit code 0 for
EPIPE, exit code 8 for every other stream error. A name such asexitOnStreamErrordescribes the actual behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/index.ts` around lines 14 - 19, Rename ignoreBrokenPipe in the CLI stream error handler to reflect that it exits the process rather than ignoring errors, and update any call sites in the same module to use the new name. Keep the existing NodeJS.WriteStream error handling behavior in place, including process.exit(0) for EPIPE and process.exit(8) for all other stream errors, and prefer a name like exitOnStreamError that matches the symbol’s actual effect.src/main/approval/routes.ts (1)
4-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImport
RendererRouteCallerinstead of deriving it withReturnType.
src/routes/routeRegistryexportsRendererRouteCalleras a named type, andsrc/main/app/routes.tsline 26 imports it directly. Use the same named type here so one concept has one name.♻️ Proposed refactor
-import { createRouteMap, requireRendererCaller } from '`@/routes/routeRegistry`' +import { + createRouteMap, + requireRendererCaller, + type DeepchatRouteMap, + type RendererRouteCaller +} from '`@/routes/routeRegistry`' export type ApprovalRoutesDependencies = Readonly<{ resolve( input: { requestId: string; decision: 'approved' | 'denied' }, - caller: ReturnType<typeof requireRendererCaller> + caller: RendererRouteCaller ): boolean }> -export function createApprovalRoutes(dependencies: ApprovalRoutesDependencies) { +export function createApprovalRoutes(dependencies: ApprovalRoutesDependencies): DeepchatRouteMap {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/approval/routes.ts` around lines 4 - 9, Update ApprovalRoutesDependencies.resolve to use the named RendererRouteCaller type exported by routeRegistry instead of ReturnType<typeof requireRendererCaller>, and add the corresponding type import while preserving the existing resolve signature.src/main/cli/audioTranscriptionService.ts (1)
179-189: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winParse transcription results with the shared route output schema.
Both
audio.transcribeArtifactandaudio.transcribeUploadreturn throughtranscribeFile, but the result is parsed throughaudioTranscribeUploadRoute.output. UseAudioTranscriptionOutputSchemadirectly to keep the artifact response coupled only to the shared output schema.♻️ Proposed refactor
import { AUDIO_TRANSCRIPTION_MAX_INPUT_BYTES, AUDIO_TRANSCRIPTION_MAX_TEXT_CHARACTERS, AudioInputMimeTypeSchema, + AudioTranscriptionOutputSchema, audioTranscribeArtifactRoute, audioTranscribeUploadRoute,- return audioTranscribeUploadRoute.output.parse({ + return AudioTranscriptionOutputSchema.parse({🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/cli/audioTranscriptionService.ts` around lines 179 - 189, Update the return parsing in transcribeFile to use the shared AudioTranscriptionOutputSchema directly instead of audioTranscribeUploadRoute.output, while preserving the existing transcription result fields and truncation behavior for both artifact and upload callers.src/main/cli/ocrService.ts (1)
275-275: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate extraction output against the shared schema, not the upload route.
toImageOutputandtoDocumentOutputserve both the upload path and the artifact path, but both parse throughocrExtractUploadRoute.output. The behavior is identical today because both routes reuseOcrExtractionOutputSchema. ReferenceOcrExtractionOutputSchemadirectly so the artifact path stays correct if the two route outputs diverge later.♻️ Proposed refactor
-import { - OCR_EXTRACTION_MAX_INPUT_BYTES, - OcrInputMimeTypeSchema, +import { + OCR_EXTRACTION_MAX_INPUT_BYTES, + OcrExtractionOutputSchema, + OcrInputMimeTypeSchema,- return ocrExtractUploadRoute.output.parse({ + return OcrExtractionOutputSchema.parse({ kind: 'image',- return ocrExtractUploadRoute.output.parse({ + return OcrExtractionOutputSchema.parse({ kind: 'document',Also applies to: 297-297
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/cli/ocrService.ts` at line 275, Update the output parsing in toImageOutput and toDocumentOutput to validate directly with the shared OcrExtractionOutputSchema instead of ocrExtractUploadRoute.output. Keep the existing parsed output behavior unchanged for both upload and artifact paths.src/main/app/composition.ts (1)
1075-1079: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
loggerinstead ofconsole.warnfor the CLI Skill activity failure.Every other CLI dependency in this composition receives
logger. This callback writes toconsole.warn, so the failure does not reach the application log sink.♻️ Proposed change
recordSettingsActivity: (input) => { void settingsDatabase.recordSettingsActivity(input).catch((error) => { - console.warn('[SettingsActivity] Failed to record CLI Skill activity:', error) + logger.warn('[SettingsActivity] Failed to record CLI Skill activity:', error) }) },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/app/composition.ts` around lines 1075 - 1079, Update the recordSettingsActivity callback to use the existing logger for recording CLI Skill activity failures instead of console.warn, preserving the current error message and asynchronous catch behavior so the failure reaches the application log sink.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/args.ts`:
- Around line 458-466: Update the --timeout handling in the argument parser to
reject the next token when it is flag-shaped, matching the guard used by
readOptionValue, and report “Missing value for --timeout” without consuming that
flag. Preserve normal timeout parsing for non-flag values.
In `@src/cli/artifacts.ts`:
- Around line 141-143: Update the MIME-type comparison in the artifact
validation block to parse contentType and compare only its media type against
input.metadata.mimeType, preserving the existing normalization and
protocolFailure behavior. Reuse the project’s established Content-Type parser if
available, and ensure parameter-bearing values such as charset do not trigger a
mismatch.
In `@src/main/cli/artifactSpool.ts`:
- Around line 550-571: Hoist the stored-artifact usage calculation out of the
per-chunk write loop, using a single snapshot before the loop in the surrounding
write method. Update reserveBytes to validate against that snapshot plus the
reservation delta accumulated during the current write, rather than rebuilding
artifacts and rescanning quotas for every chunk. Keep reserveBytes responsible
for recording reservations and preserve all existing owner, request, connection,
and total-limit checks.
In `@src/main/cli/computeService.ts`:
- Around line 275-296: Update the queued emission handling around
executeWithRateLimit so repeated onQueued callbacks chain each emitEvent call
onto the previous queuedEmission promise instead of overwriting it. Preserve
queuedEmissionError propagation and ensure the final await does not complete
until all queued rate-limit events have finished writing.
In `@src/main/cli/launcherService.ts`:
- Around line 305-310: Update the Windows executable path logic in the launcher
service’s platform branch to use a program-owned directory under localAppData,
such as a MyApp/bin path, instead of Microsoft/WindowsApps; ensure installation
and any PATH update use this same directory while preserving the deepchat.cmd
filename.
In `@src/main/cli/mutationGuard.ts`:
- Around line 84-111: Update the scopeKey construction in the mutation approval
flow around approvalRequestId and approvals.create to use only
cli:${input.connectionId}, removing executionId from the scope. Preserve
executionId as the bindingKey so each mutation remains uniquely identified, and
leave resolve’s use of the recorded scopeKey unchanged.
In `@src/main/cli/ocrService.ts`:
- Around line 96-124: Wrap the ocrClearCacheRoute switch case body in braces so
its local declarations, especially const status, are scoped to that clause and
noSwitchDeclarations is satisfied. Keep the existing permission, cache-clearing,
error handling, status validation, and return behavior unchanged.
In `@src/main/cli/runService.ts`:
- Around line 127-154: The projectMessagePage budget loop must always include
the newest projected message, even when its serialized size exceeds
RUN_SNAPSHOT_MESSAGE_BUDGET_BYTES. Admit the newest message before enforcing the
budget, then apply the existing size cap to older messages so pagination retains
the correct cursor and never returns an empty messages array for a non-empty
page.
In `@src/shared/contracts/routes/runs.routes.ts`:
- Around line 11-14: Update PublicRunMessageSchema.text in the shared runs route
contracts to enforce RUN_MESSAGE_MAX_TEXT_BYTES based on UTF-8 byte length,
while preserving its existing validation behavior. Add a regression test
covering multibyte text whose character count is within the limit but whose
UTF-8 byte length exceeds it, ensuring runs.get rejects or excludes oversized
public messages.
In `@src/shared/utils/filesystem.ts`:
- Around line 12-15: Update isHardlinkUnavailableError in filesystem.ts to guard
the unknown error before reading .code, so null or undefined inputs return false
instead of throwing a TypeError. Keep the existing HARDLINK_UNAVAILABLE_CODES
check intact, and use a nullish-safe narrowing path around the current
NodeJS.ErrnoException code access so callers like skillService can still fall
back normally.
In `@test/main/cli/packagedSmoke.test.ts`:
- Around line 43-56: Set an explicit per-test timeout on the packaged CLI smoke
test declared by the `it` call, using a value large enough to cover the full
`buildCli` operation and all sequential 5-second subprocess waits. Keep the test
body and existing global timeout behavior unchanged.
In `@test/main/provider/providerRuntime.test.ts`:
- Around line 605-625: Update the generateSpeechStandalone test at
test/main/provider/providerRuntime.test.ts:605-625 so the streamed
image_data.data and expected audio.data use bare base64 rather than a data: URI.
Update the computeService mock at test/main/cli/computeService.test.ts:142-146
to use the same bare-base64 shape; no other behavior needs changing.
In `@test/main/scripts/buildCli.test.ts`:
- Line 62: Guard the executable-bit assertion in the build CLI test around the
existing stat check so it runs only when process.platform is not win32; preserve
the current 0o111 validation on supported POSIX platforms.
- Around line 47-51: Set an explicit 15,000 ms timeout on the standalone CLI
build test in the it block beginning “builds a standalone Node entry and
explicit bundled-runtime launchers.” Keep the test body and global Vitest
timeout unchanged.
---
Nitpick comments:
In `@src/cli/format.ts`:
- Line 26: Update the switch in formatHumanResult to include an exhaustive
default branch that passes the unmatched contract value to a never guard,
ensuring newly added CliRpcContract members produce a compile-time error instead
of allowing an undefined return. Preserve the existing formatting cases and
string return behavior.
- Around line 199-208: Update the mcp.listPublic branch to call the existing
formatMcpRuntime helper for each server’s running state instead of duplicating
the ternary formatting. Preserve the surrounding server fields and truncation
message while using the helper’s canonical runtime labels.
In `@src/cli/index.ts`:
- Around line 14-19: Rename ignoreBrokenPipe in the CLI stream error handler to
reflect that it exits the process rather than ignoring errors, and update any
call sites in the same module to use the new name. Keep the existing
NodeJS.WriteStream error handling behavior in place, including process.exit(0)
for EPIPE and process.exit(8) for all other stream errors, and prefer a name
like exitOnStreamError that matches the symbol’s actual effect.
In `@src/main/app/composition.ts`:
- Around line 1075-1079: Update the recordSettingsActivity callback to use the
existing logger for recording CLI Skill activity failures instead of
console.warn, preserving the current error message and asynchronous catch
behavior so the failure reaches the application log sink.
In `@src/main/approval/routes.ts`:
- Around line 4-9: Update ApprovalRoutesDependencies.resolve to use the named
RendererRouteCaller type exported by routeRegistry instead of ReturnType<typeof
requireRendererCaller>, and add the corresponding type import while preserving
the existing resolve signature.
In `@src/main/cli/artifactRoutes.ts`:
- Around line 36-45: Add a concise comment immediately above the artifacts.read
handler in the route definitions explaining that it returns metadata as the
pre-flight for the separately streamed artifact download. Leave the existing
parsing, caller validation, and artifactSpool.describe behavior unchanged.
In `@src/main/cli/audioTranscriptionService.ts`:
- Around line 179-189: Update the return parsing in transcribeFile to use the
shared AudioTranscriptionOutputSchema directly instead of
audioTranscribeUploadRoute.output, while preserving the existing transcription
result fields and truncation behavior for both artifact and upload callers.
In `@src/main/cli/ocrService.ts`:
- Line 275: Update the output parsing in toImageOutput and toDocumentOutput to
validate directly with the shared OcrExtractionOutputSchema instead of
ocrExtractUploadRoute.output. Keep the existing parsed output behavior unchanged
for both upload and artifact paths.
In `@src/main/cli/surface.ts`:
- Around line 256-267: The reviewable branch should also remove C0/C1 control
characters while preserving the full remaining value for display. Update the
includeReviewableValues handling in the description and icon projection logic,
reusing the existing sanitization behavior or helper without truncating
reviewable text, while leaving directional-control rejection and non-reviewable
truncation behavior unchanged.
In `@src/shared/contracts/routes/cli.routes.ts`:
- Around line 19-20: Reuse the shared local-control schemas at both sites: in
src/shared/contracts/routes/cli.routes.ts lines 19-20, use
LocalControlScopesSchema.min(1) for scopes and add duplicate rejection to
callers while using the shared principal schema; in
src/shared/contracts/events/approvals.events.ts line 13, replace the inline
principal enum with LocalControlPrincipalSchema from the existing
../localControl import.
In `@test/main/cli/args.test.ts`:
- Around line 122-126: Replace the literal 1_800_000 with the imported
DEFAULT_COMPUTE_TIMEOUT_MS in the expectations at the stream operation block and
the corresponding sites around lines 406 and 647, preserving the existing
assertions.
In `@test/main/cli/launcherService.test.ts`:
- Around line 225-254: Gate the POSIX-specific assertions in
launcherService.test and mediaOutput.test so they do not run on win32 hosts:
update the launcherService test block around the fails-closed scenario, and the
related permission-bit/chmod cases in the same file, to only execute on
non-win32, and either skip the mediaOutput link assertion on win32 or switch its
directory symlink setup to use the junction form. Keep the existing product
assertions and test flow unchanged for non-Windows platforms.
In `@test/main/cli/runService.test.ts`:
- Around line 198-207: Update the oversized-prompt test around invokeRoute to
assert the specific input-validation error code, matching the neighboring tests
and the code reported by sessionsRunDetachedRoute, instead of only checking that
a rejection exists. Keep the assertion that createDetachedSession is not called.
In `@test/main/contracts/localControl.test.ts`:
- Around line 30-68: Split the combined negative tests into separate cases, each
violating exactly one constraint. In the descriptor tests around
LocalControlDescriptorSchema.parse, add independent cases for protocolVersion,
endpoint NUL rejection, pid lower bound, token length, and unknown-key
rejection; in the LocalControlRpcRequestSchema tests, separate invalid id and
invalid method cases while keeping all other fields valid.
In `@test/main/routes/routeRegistry.test.ts`:
- Around line 27-55: Add a positive-path test alongside the existing
renderer-boundary rejection cases, using createRendererRouteContext to construct
a valid renderer context and asserting requireRendererCaller returns
context.caller unchanged.
In `@test/main/tool/agentTools/agentBashHandler.test.ts`:
- Around line 144-152: Update the test spies for AgentBashHandler’s private
methods, including prepareCommand and runShellProcess, to avoid as never casts.
Use the existing AgentCommandEnvironmentPort-based typing pattern so these
dependencies remain type-safe and consistent with the createEnvironment spy.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 709b8cef-3f87-447f-ae0b-013ee161b0db
📒 Files selected for processing (157)
docs/README.mddocs/architecture/local-control-plane/plan.mddocs/architecture/local-control-plane/spec.mddocs/architecture/local-control-plane/tasks.mddocs/guides/cli.mdelectron-builder.ymlpackage.jsonresources/acp-registry/registry.jsonresources/skills/deepchat-cli/SKILL.mdscripts/build-cli.mjssrc/cli/args.tssrc/cli/artifacts.tssrc/cli/discovery.tssrc/cli/errors.tssrc/cli/format.tssrc/cli/globals.d.tssrc/cli/index.tssrc/cli/run.tssrc/cli/stdin.tssrc/cli/transport.tssrc/main/app/applicationDataReset.tssrc/main/app/composition.tssrc/main/app/routes.tssrc/main/app/settingsRoutes.tssrc/main/approval/approvalBroker.tssrc/main/approval/index.tssrc/main/approval/routes.tssrc/main/cli/agentCommandAccess.tssrc/main/cli/agentTokenAuthority.tssrc/main/cli/artifactRoutes.tssrc/main/cli/artifactSpool.tssrc/main/cli/audioTranscriptionService.tssrc/main/cli/auditLog.tssrc/main/cli/body.tssrc/main/cli/computeService.tssrc/main/cli/descriptor.tssrc/main/cli/errors.tssrc/main/cli/index.tssrc/main/cli/launcherService.tssrc/main/cli/mcpAdminRoutes.tssrc/main/cli/mediaOutput.tssrc/main/cli/mutationGuard.tssrc/main/cli/ocrService.tssrc/main/cli/policy.tssrc/main/cli/providerModelAdminRoutes.tssrc/main/cli/publicText.tssrc/main/cli/routes.tssrc/main/cli/runService.tssrc/main/cli/server.tssrc/main/cli/skillService.tssrc/main/cli/surface.tssrc/main/desktop/routes.tssrc/main/desktop/sessionBinding.tssrc/main/events/sessionEventRouter.tssrc/main/events/typedEventHub.tssrc/main/mcp/routes.tssrc/main/notifications/electronWindowNotificationTargets.tssrc/main/notifications/routes.tssrc/main/ocr/ocrRuntimeService.tssrc/main/ocr/routes.tssrc/main/provider/index.tssrc/main/provider/routes.tssrc/main/routes/index.tssrc/main/routes/routeRegistry.tssrc/main/session/data/tables/deepchatSessionMetadata.tssrc/main/session/routes.tssrc/main/session/sessionService.tssrc/main/skill/archive.tssrc/main/skill/archiveDownload.tssrc/main/skill/index.tssrc/main/tool/agentTools/agentBashHandler.tssrc/main/tool/agentTools/agentToolManager.tssrc/main/tool/index.tssrc/main/tool/permission/commandPermissionService.tssrc/main/tool/permission/index.tssrc/main/tool/permission/toolPermissionBroker.tssrc/renderer/api/ApprovalClient.tssrc/renderer/src/apps/chat-main/ChatMainApp.vuesrc/renderer/src/components/cli/CliApprovalDialog.vuesrc/renderer/src/stores/cliApproval.tssrc/shared/contracts/cliCommands.tssrc/shared/contracts/common.tssrc/shared/contracts/contract.tssrc/shared/contracts/events.tssrc/shared/contracts/events/approvals.events.tssrc/shared/contracts/events/runs.events.tssrc/shared/contracts/json.tssrc/shared/contracts/localControl.tssrc/shared/contracts/routes.tssrc/shared/contracts/routes/approvals.routes.tssrc/shared/contracts/routes/artifacts.routes.tssrc/shared/contracts/routes/audio.routes.tssrc/shared/contracts/routes/cli.routes.tssrc/shared/contracts/routes/mcp.routes.tssrc/shared/contracts/routes/media.routes.tssrc/shared/contracts/routes/models.routes.tssrc/shared/contracts/routes/ocr.routes.tssrc/shared/contracts/routes/providers.routes.tssrc/shared/contracts/routes/runs.routes.tssrc/shared/contracts/routes/settings.routes.tssrc/shared/contracts/routes/skills.routes.tssrc/shared/types/agent-interface.d.tssrc/shared/types/provider.tssrc/shared/types/skill.tssrc/shared/utils/filesystem.tstest/main/app/applicationDataReset.test.tstest/main/app/routes.test.tstest/main/approval/approvalBroker.test.tstest/main/approval/routes.test.tstest/main/cli/agentCommandAccess.test.tstest/main/cli/agentTokenAuthority.test.tstest/main/cli/args.test.tstest/main/cli/artifactSpool.test.tstest/main/cli/artifacts.test.tstest/main/cli/auditLog.test.tstest/main/cli/body.test.tstest/main/cli/client.test.tstest/main/cli/computeService.test.tstest/main/cli/descriptor.test.tstest/main/cli/discovery.test.tstest/main/cli/errors.test.tstest/main/cli/inputCapabilityServices.test.tstest/main/cli/launcherService.test.tstest/main/cli/mcpAdminRoutes.test.tstest/main/cli/mediaOutput.test.tstest/main/cli/mutationGuard.test.tstest/main/cli/packagedSmoke.test.tstest/main/cli/policy.test.tstest/main/cli/providerModelAdminRoutes.test.tstest/main/cli/runService.test.tstest/main/cli/server.test.tstest/main/cli/skillService.test.tstest/main/cli/stdin.test.tstest/main/cli/surface.test.tstest/main/cli/transport.test.tstest/main/contracts/localControl.test.tstest/main/desktop/sessionBinding.test.tstest/main/events/typedEventHub.test.tstest/main/notifications/routes.test.tstest/main/ocr/routes.test.tstest/main/orchestration/orchestrationRoutes.test.tstest/main/provider/providerRuntime.test.tstest/main/provider/routes.test.tstest/main/routes/dispatcher.test.tstest/main/routes/routeRegistry.test.tstest/main/scripts/buildCli.test.tstest/main/session/data/tables/deepchatSessionMetadata.test.tstest/main/session/sessionService.test.tstest/main/skill/archive.test.tstest/main/skill/archiveDownload.test.tstest/main/skill/skillService.test.tstest/main/skill/skillServiceAgentScopes.test.tstest/main/tool/agentTools/agentBashHandler.test.tstest/main/tool/permission/commandPermissionService.test.tstest/main/tool/toolPermissionBroker.test.tstest/renderer/stores/cliApproval.test.tstsconfig.node.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/main/shared/filesystem.test.ts`:
- Around line 5-14: Update the isHardlinkUnavailableError tests to include a
plain object with a recognized string code, such as EPERM, and assert it returns
true. Keep the existing primitive, Error-instance, and unrecognized-code cases
unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b18694b4-63f2-42e3-8de0-1f5f5a88a345
📒 Files selected for processing (15)
src/cli/args.tssrc/main/cli/artifactSpool.tssrc/main/cli/computeService.tssrc/main/cli/mutationGuard.tssrc/shared/contracts/routes/runs.routes.tssrc/shared/utils/filesystem.tstest/main/cli/args.test.tstest/main/cli/artifactSpool.test.tstest/main/cli/computeService.test.tstest/main/cli/mutationGuard.test.tstest/main/cli/packagedSmoke.test.tstest/main/cli/runService.test.tstest/main/scripts/buildCli.test.tstest/main/shared/filesystem.test.tstest/setup.renderer.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/shared/utils/filesystem.ts
- test/main/cli/mutationGuard.test.ts
- src/main/cli/mutationGuard.ts
- src/shared/contracts/routes/runs.routes.ts
- src/cli/args.ts
zerob13
reviewed
Aug 6, 2026
zerob13
left a comment
Collaborator
There was a problem hiding this comment.
Review: feat(cli) add local control plane CLI
Overall: one of the most security-conscious local control plane implementations I have reviewed. The architecture decisions are sound — canonical contract reuse (no second schema catalog), the ApprovalBroker extraction with a unique, non-replayable CLI approval path, the deny-by-default CLI_SURFACE with an explicit caller model, bounded body handling with approval-before-body via Expect: 100-continue, and agent token scopes derived from the command catalog. I did not find an exploitable vulnerability in the reviewed paths (caller + scope + agentPolicy gating, TOCTOU defenses, shell-control detection). Issues below are ordered by priority.
P1 (maintainability): command metadata is spread across 5 places with no compile-time consistency
Adding a command requires touching:
src/shared/contracts/cliCommands.ts(definition)src/cli/args.ts—VALUE_DOMAIN_OPTIONS(L65) +COMMAND_DOMAIN_OPTIONS(L187)- the validation chain (L632-739)
- the params-building chain (L756-959)
- the hardcoded help text in
formatCliHelp(L989)
Any missed spot is a silent drift (e.g. help text diverging from real constraints). Suggest moving per-command option definitions (name/type/constraints/params mapping/help) into CliCommandDefinition so validation and help derive from one source.
P2 (availability): audit failure fails closed and takes down ALL CLI requests
CliRequestPolicy.authorize awaits audit('allowed') for every request including read-only ones (policy.ts:227), and CliAuditLog.record rejects when the file is unwritable/closed/disk-full (auditLog.ts:42). Once the audit file is corrupt, every CLI capability (including cli.status) returns 500. Fail-closed is defensible for a security audit trail, but the blast radius includes read-only diagnostics; consider best-effort audit (log + continue) or at least not making read-only requests depend on audit success.
P3 (quota semantics): agent byte quota is consumed on failure and never refunded
ArtifactSpool consumes per-chunk during write (artifactSpool.ts:354-361), and consumeRecordBytes pins usedBytes to maxBytes on overflow (agentTokenAuthority.ts:278-280). A failed/cancelled 200MB generation permanently burns the token quota until expiry. Consider settling after success or refunding on failure.
P4 (consistency): providerModelAdminRoutes mutations lack error normalization
addProviderAtomic/updateProviderAtomic/setModelConfig are called bare (providerModelAdminRoutes.ts:141/176/252). A transient store failure surfaces as a raw 500 internal_error, non-retriable, while mcpAdminRoutes wraps the same class of failure as 503 retriable. Please normalize consistently.
P5 (semantics): agent run ownership model is split
requireOwnedRun lets an agent access sessions where conversationId === runId (runService.ts:426-429), but subscribeToRun additionally requires metadata.source === 'cli_run' (runService.ts:338), so an agent always gets 404 from events.subscribe after passing runs.get/runs.cancel. The behavior matches the spec (an agent must not watch its own run), but the denial should happen in requireOwnedRun instead of a second failure point in subscribe.
P6 (launcher): two issues that can permanently lock users in a conflict state
launcherService.ts:416comparespath.resolve(marker.commandPath) !== commandPathcase-sensitively on win32, whileisCommandDirectoryOnPath(L728) lowercases — inconsistent casing across starts flags an owned launcher asconflict/ownership-marker-invalid.launcherService.ts:774classifies a >1MB shell profile asblockState: 'modified'and refuses install ("managed block has been modified") — a large but untouched.zshrcmakes installation permanently fail with a misleading error. Give it a distinct reason (e.g.profile-too-large).
P7 (small)
surface.ts:947—artifactsReadRoutelimitmaxBodyBytes: 1magic number.mcpAdminRoutes.ts:240/254— non-null assertions (updates.command!,updates.baseUrl!) after if-guards.run.ts:290-299— stdin byte limits hardcoded by contract name, duplicated from contract constants.
Consistency / dedup (medium)
- Factory-function style (
mcpAdminRoutes/providerModelAdminRoutes/artifactRoutes) vs class +createRoutes()(skillService/computeService/ocrService/audioTranscriptionService/runService) within the same PR — pick one. requireCliCaller/requireHumanCliCallerduplicated 5x; error-normalization helpers 4x;removeIfPresent3x;writeAll2x; the launcherService atomic-file-ops cluster (~7 methods) shares patterns withartifactSpool/auditLog. A small sharedcliGuards/cliFileOpsmodule would help.
Over-engineering candidates (low)
TypedEventHub: 10 tunable parameters, most only exercised by tests — acceptable, but consider trimming.ArtifactSpooldouble-track quota accounting (6 maps, ~200 lines) is correct but very hard to review; a single-trackQuotaTrackerwould help.agentTokenAuthority.createUniqueTokenId(L262) constrains the injected tokenId generator with a 16-128 regex, breaking tests that inject short ids.
Tests (non-blocking)
+11.7k lines for +31k implementation lines is a reasonable ratio and quality is high (black-box, contract-bound). No file should be deleted wholesale. Suggested trims:
surface.test.tsL354-589: golden table fully inlines implementation-derived data (will break on any implementation change) and overlaps the policy list at L81-99 — use a snapshot or drop the duplicate list (~-200 lines).computeService.test.tsL264-379: two process-ordered timing tests — merge into one behavioral test (~-80 lines).cliApproval.test.ts:30 lines of-60 lines).vi.doMockboilerplate per test — extract a helper (launcherService.test.ts: five isomorphic tests →it.each(~-60 lines).args.test.ts: help assertions scattered across 5itblocks (~-25 lines).errors.test.tsL13-24: asserts a constant against a hardcoded copy of itself — drop (~-12 lines).
Confirmed good (no action)
ApprovalBrokerextraction preserves tool semantics; CLI approvals are unique, non-deduplicated, non-replayable;approvals.resolveis renderer-only.- Upload flow: rejection happens before reading the body (
100-continuegated behind approval); chunked bodies cannot bypass limits (body.ts). - Agent file boundary: no
--file/--out/artifact bytes for agents, double-enforced client-side (run.ts:360-379) and server-side. hasShellControlSyntax+ signature hashing incommandPermissionServicecovers redirection,$(and backticks inside quotes.- Docs (spec/plan/guide/SKILL.md) match the implementation, including honest disclosure of the pre-existing shell redirection gap.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/main/cli/launcherService.test.ts`:
- Around line 397-410: Extend the “matches an owned Windows marker path without
case sensitivity” test after the removeOwnedLauncher assertion to verify the
resolved Windows launcher path no longer exists. Use the fixture’s resolved
launcher-path symbol rather than the uppercased marker.commandPath, preserving
the existing ownership and removal assertions.
- Around line 338-347: Update the oversized-profile test around
fixture.service.ensureInstalled() to capture the original profile contents
before the rejection, then read the file afterward and assert the contents are
identical. Retain the existing size assertion and command-path absence check.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 65b50722-20b2-4165-afa0-de9cffb92810
📒 Files selected for processing (5)
src/main/cli/launcherService.tssrc/main/cli/providerModelAdminRoutes.tstest/main/cli/launcherService.test.tstest/main/cli/providerModelAdminRoutes.test.tstest/main/shared/filesystem.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- test/main/shared/filesystem.test.ts
- test/main/cli/providerModelAdminRoutes.test.ts
- src/main/cli/launcherService.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/cli/publicText.ts`:
- Around line 25-31: Update stripC0AndC1Controls to enforce the same bounded
scan limit while iterating input, limiting both character scanning and output
allocation. Ensure callers such as mcpConfigProjection cannot process unbounded
config.description or config.icon values when includeReviewableValues is
enabled, while preserving control-character filtering for the scanned portion.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d2eb419-bb15-484e-ab82-dbb2812023a3
📒 Files selected for processing (18)
src/cli/format.tssrc/cli/index.tssrc/main/app/composition.tssrc/main/approval/routes.tssrc/main/cli/artifactRoutes.tssrc/main/cli/audioTranscriptionService.tssrc/main/cli/ocrService.tssrc/main/cli/publicText.tssrc/main/cli/surface.tssrc/shared/contracts/events/approvals.events.tssrc/shared/contracts/routes/cli.routes.tstest/main/cli/args.test.tstest/main/cli/launcherService.test.tstest/main/cli/mediaOutput.test.tstest/main/cli/runService.test.tstest/main/cli/surface.test.tstest/main/contracts/localControl.test.tstest/main/routes/routeRegistry.test.ts
🚧 Files skipped from review as they are similar to previous changes (16)
- test/main/contracts/localControl.test.ts
- test/main/routes/routeRegistry.test.ts
- src/main/approval/routes.ts
- src/main/cli/artifactRoutes.ts
- test/main/cli/surface.test.ts
- test/main/cli/mediaOutput.test.ts
- test/main/cli/launcherService.test.ts
- src/cli/index.ts
- src/shared/contracts/events/approvals.events.ts
- src/main/cli/ocrService.ts
- src/shared/contracts/routes/cli.routes.ts
- src/main/cli/audioTranscriptionService.ts
- src/cli/format.ts
- test/main/cli/args.test.ts
- src/main/cli/surface.ts
- src/main/app/composition.ts
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
src/main/tool/permission/toolPermissionBroker.ts (1)
159-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the
onRequestfailure before you deny the request.The bare
catchdiscards the error. When the renderer delivery fails, the tool call is denied with no diagnostic record. Capture the error and log it.♻️ Proposed change
try { onRequest(this.toPermissionRequest(pending)) - } catch { + } catch (error) { + console.warn('[ToolPermission] Failed to deliver approval request', error) this.approvals.resolve({🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/tool/permission/toolPermissionBroker.ts` around lines 159 - 167, Update the catch block around onRequest in the permission request flow to capture the thrown error and log it before resolving the pending approval as denied. Preserve the existing requestId, scopeKey, and denied decision behavior.src/main/events/sessionEventRouter.ts (1)
67-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the loop variable to avoid shadowing.
Line 68 names the callback parameter
ownership. The outer array at line 45 has the same name and a different type. Rename the parameter to keep the two values distinct.♻️ Proposed rename
- for (const runId of new Set(cliRunOwnership.map((ownership) => ownership.runId))) { + for (const runId of new Set(cliRunOwnership.map((entry) => entry.runId))) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/events/sessionEventRouter.ts` around lines 67 - 71, Rename the map callback parameter in the run-ID collection within the RUN_STREAM_EVENTS branch to avoid shadowing the outer ownership value, and update its runId reference accordingly; leave the loop and publishing behavior unchanged.test/main/cli/discovery.test.ts (1)
26-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTemp-directory cleanup is not idempotent in two test files. Both
afterEachhooks callrm(directory, { recursive: true })withoutforce: true. If a test already removed a directory,rmthrowsENOENTand the hook fails, which reports as an unrelated suite failure. Addforce: trueat both sites.
test/main/cli/discovery.test.ts#L26-L30: change thermcall in theafterEachhook torm(directory, { recursive: true, force: true }).test/main/cli/inputCapabilityServices.test.ts#L81-L86: change thermcall in theafterEachhook torm(directory, { recursive: true, force: true }).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/main/cli/discovery.test.ts` around lines 26 - 30, Make both afterEach cleanup hooks idempotent by adding force: true to the rm options in test/main/cli/discovery.test.ts lines 26-30 and test/main/cli/inputCapabilityServices.test.ts lines 81-86. Update the rm calls used by each hook while preserving recursive cleanup behavior.test/main/cli/agentTokenAuthority.test.ts (1)
53-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueToken id generation depends on
shift()call order.
createTokenIdderives the id fromgeneratedTokens.length, whichcreateTokenmutates. The ids stay unique only whileissue()callscreateTokenbeforecreateTokenId. The other tests in this file use an independent counter, which does not depend on that order. Use the same counter here.♻️ Proposed change
const generatedTokens = [token('a'), token('b'), token('c')] + let tokenId = 0 const authority = new AgentCliTokenAuthority({ createToken: () => generatedTokens.shift()!, - createTokenId: () => `token-id-${generatedTokens.length}`.padEnd(16, '0') + createTokenId: () => `token-id-${String((tokenId += 1)).padStart(8, '0')}` })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/main/cli/agentTokenAuthority.test.ts` around lines 53 - 57, Update the test fixture for AgentCliTokenAuthority so createTokenId uses an independent counter rather than generatedTokens.length, matching the counter-based setup used by the other tests in the file. Keep generatedTokens responsible only for returned token values and ensure each generated ID remains unique regardless of whether createToken or createTokenId is called first.src/shared/contracts/routes/ocr.routes.ts (1)
205-216: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueNo cross-check between
artifactTerminationandresourceLimit.This file validates two other cross-field invariants: runtime readiness at Line 146 and cache-hit consistency at Line 229. The document output has a third invariant that is not enforced.
artifactTermination: 'resource_limited'should imply thatresourceLimitis present, and the reverse should also hold. Adding the check keeps the contract self-consistent.♻️ Proposed change
.strict() -}).strict() +}) + .strict() + .superRefine((output, context) => { + if ((output.artifactTermination === 'resource_limited') !== (output.resourceLimit !== undefined)) { + context.addIssue({ + code: 'custom', + message: 'OCR resource limit detail does not match its termination reason', + path: ['resourceLimit'] + }) + } + })Note that
z.discriminatedUnionat Line 227 requires plain object members. If the added refinement breaks the discriminated union, apply the same check inside the existingsuperRefineat Line 228 instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/contracts/routes/ocr.routes.ts` around lines 205 - 216, Update the document output validation around the schema containing artifactTermination and resourceLimit to enforce both directions of the invariant: artifactTermination must be resource_limited exactly when resourceLimit is present. Prefer adding the check in the existing superRefine near the discriminated union so its plain-object member requirement remains valid, while preserving the runtime-readiness and cache-hit consistency checks.src/shared/contracts/routes/providers.routes.ts (1)
189-193: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPublic connection-test route reuses a non-strict internal schema.
providersTestPublicConnectionRoutereusesprovidersTestConnectionRoute.inputfrom Line 179. That schema is not.strict()and itsproviderIdusesEntityIdSchemawithout.max(128). Every other public route added in this file uses.strict()and a boundedproviderId. Unknown keys from a CLI caller are therefore stripped here instead of rejected.Define a bounded strict input for the public route.
♻️ Proposed change
export const providersTestPublicConnectionRoute = defineRouteContract({ name: 'providers.testPublicConnection', - input: providersTestConnectionRoute.input, + input: z + .object({ + providerId: EntityIdSchema.max(128), + modelId: z.string().min(1).max(256).optional() + }) + .strict(), output: providersTestConnectionRoute.output })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/contracts/routes/providers.routes.ts` around lines 189 - 193, Update providersTestPublicConnectionRoute to define its own strict input schema instead of reusing providersTestConnectionRoute.input; ensure providerId uses the bounded EntityIdSchema with a maximum length of 128, while preserving the existing route output schema.src/main/cli/skillService.ts (1)
79-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
retainUploadFileinjectslinkbut notcopyFile.The function accepts
linkFilefor testing, but the fallback path calls the importedcopyFiledirectly. A test cannot exercise the fallback branch through the parameter. Consider injecting both, or neither.♻️ Proposed change
export async function retainUploadFile( uploadPath: string, - linkFile: typeof link = link + linkFile: typeof link = link, + copyFileFn: typeof copyFile = copyFile ): Promise<Readonly<{ path: string }>> { const retainedPath = path.join(path.dirname(uploadPath), `body-${randomUUID()}.tmp`) try { await linkFile(uploadPath, retainedPath) } catch (error) { if (!isHardlinkUnavailableError(error)) throw error try { - await copyFile(uploadPath, retainedPath, constants.COPYFILE_EXCL) + await copyFileFn(uploadPath, retainedPath, constants.COPYFILE_EXCL)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/cli/skillService.ts` around lines 79 - 96, Update retainUploadFile so the fallback copy operation uses an injected dependency consistently with linkFile: either add a copyFile parameter with the imported function as its default and call that parameter, or remove linkFile injection and use both imported functions directly. Preserve the existing hardlink fallback and cleanup behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/shared/contracts/routes/providers.routes.ts`:
- Around line 29-43: Update PublicProviderBaseUrlSchema’s superRefine callback
to handle URL construction failures from invalid inputs by catching new
URL(value) errors and adding a Zod validation issue instead of allowing a
TypeError to escape. Preserve support for localhost HTTP endpoints and keep the
existing protocol, credential, query, and fragment validations unchanged.
---
Nitpick comments:
In `@src/main/cli/skillService.ts`:
- Around line 79-96: Update retainUploadFile so the fallback copy operation uses
an injected dependency consistently with linkFile: either add a copyFile
parameter with the imported function as its default and call that parameter, or
remove linkFile injection and use both imported functions directly. Preserve the
existing hardlink fallback and cleanup behavior.
In `@src/main/events/sessionEventRouter.ts`:
- Around line 67-71: Rename the map callback parameter in the run-ID collection
within the RUN_STREAM_EVENTS branch to avoid shadowing the outer ownership
value, and update its runId reference accordingly; leave the loop and publishing
behavior unchanged.
In `@src/main/tool/permission/toolPermissionBroker.ts`:
- Around line 159-167: Update the catch block around onRequest in the permission
request flow to capture the thrown error and log it before resolving the pending
approval as denied. Preserve the existing requestId, scopeKey, and denied
decision behavior.
In `@src/shared/contracts/routes/ocr.routes.ts`:
- Around line 205-216: Update the document output validation around the schema
containing artifactTermination and resourceLimit to enforce both directions of
the invariant: artifactTermination must be resource_limited exactly when
resourceLimit is present. Prefer adding the check in the existing superRefine
near the discriminated union so its plain-object member requirement remains
valid, while preserving the runtime-readiness and cache-hit consistency checks.
In `@src/shared/contracts/routes/providers.routes.ts`:
- Around line 189-193: Update providersTestPublicConnectionRoute to define its
own strict input schema instead of reusing providersTestConnectionRoute.input;
ensure providerId uses the bounded EntityIdSchema with a maximum length of 128,
while preserving the existing route output schema.
In `@test/main/cli/agentTokenAuthority.test.ts`:
- Around line 53-57: Update the test fixture for AgentCliTokenAuthority so
createTokenId uses an independent counter rather than generatedTokens.length,
matching the counter-based setup used by the other tests in the file. Keep
generatedTokens responsible only for returned token values and ensure each
generated ID remains unique regardless of whether createToken or createTokenId
is called first.
In `@test/main/cli/discovery.test.ts`:
- Around line 26-30: Make both afterEach cleanup hooks idempotent by adding
force: true to the rm options in test/main/cli/discovery.test.ts lines 26-30 and
test/main/cli/inputCapabilityServices.test.ts lines 81-86. Update the rm calls
used by each hook while preserving recursive cleanup behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9435e4b4-cef5-4a9d-803a-043be68fd992
📒 Files selected for processing (161)
docs/README.mddocs/architecture/local-control-plane/plan.mddocs/architecture/local-control-plane/spec.mddocs/architecture/local-control-plane/tasks.mddocs/guides/cli.mdelectron-builder.ymlpackage.jsonresources/acp-registry/registry.jsonresources/model-db/providers.jsonresources/skills/deepchat-cli/SKILL.mdscripts/build-cli.mjssrc/cli/args.tssrc/cli/artifacts.tssrc/cli/discovery.tssrc/cli/errors.tssrc/cli/format.tssrc/cli/globals.d.tssrc/cli/index.tssrc/cli/run.tssrc/cli/stdin.tssrc/cli/transport.tssrc/main/app/applicationDataReset.tssrc/main/app/composition.tssrc/main/app/routes.tssrc/main/app/settingsRoutes.tssrc/main/approval/approvalBroker.tssrc/main/approval/index.tssrc/main/approval/routes.tssrc/main/cli/agentCommandAccess.tssrc/main/cli/agentTokenAuthority.tssrc/main/cli/artifactRoutes.tssrc/main/cli/artifactSpool.tssrc/main/cli/audioTranscriptionService.tssrc/main/cli/auditLog.tssrc/main/cli/body.tssrc/main/cli/computeService.tssrc/main/cli/descriptor.tssrc/main/cli/errors.tssrc/main/cli/index.tssrc/main/cli/launcherService.tssrc/main/cli/mcpAdminRoutes.tssrc/main/cli/mediaOutput.tssrc/main/cli/mutationGuard.tssrc/main/cli/ocrService.tssrc/main/cli/policy.tssrc/main/cli/providerModelAdminRoutes.tssrc/main/cli/publicText.tssrc/main/cli/routes.tssrc/main/cli/runService.tssrc/main/cli/server.tssrc/main/cli/skillService.tssrc/main/cli/surface.tssrc/main/desktop/routes.tssrc/main/desktop/sessionBinding.tssrc/main/events/sessionEventRouter.tssrc/main/events/typedEventHub.tssrc/main/mcp/routes.tssrc/main/notifications/electronWindowNotificationTargets.tssrc/main/notifications/routes.tssrc/main/ocr/ocrRuntimeService.tssrc/main/ocr/routes.tssrc/main/provider/index.tssrc/main/provider/routes.tssrc/main/routes/index.tssrc/main/routes/routeRegistry.tssrc/main/session/data/tables/deepchatSessionMetadata.tssrc/main/session/routes.tssrc/main/session/sessionService.tssrc/main/skill/archive.tssrc/main/skill/archiveDownload.tssrc/main/skill/index.tssrc/main/tool/agentTools/agentBashHandler.tssrc/main/tool/agentTools/agentToolManager.tssrc/main/tool/index.tssrc/main/tool/permission/commandPermissionService.tssrc/main/tool/permission/index.tssrc/main/tool/permission/toolPermissionBroker.tssrc/renderer/api/ApprovalClient.tssrc/renderer/src/apps/chat-main/ChatMainApp.vuesrc/renderer/src/components/cli/CliApprovalDialog.vuesrc/renderer/src/stores/cliApproval.tssrc/shared/contracts/cliCommands.tssrc/shared/contracts/common.tssrc/shared/contracts/contract.tssrc/shared/contracts/events.tssrc/shared/contracts/events/approvals.events.tssrc/shared/contracts/events/runs.events.tssrc/shared/contracts/json.tssrc/shared/contracts/localControl.tssrc/shared/contracts/routes.tssrc/shared/contracts/routes/approvals.routes.tssrc/shared/contracts/routes/artifacts.routes.tssrc/shared/contracts/routes/audio.routes.tssrc/shared/contracts/routes/cli.routes.tssrc/shared/contracts/routes/mcp.routes.tssrc/shared/contracts/routes/media.routes.tssrc/shared/contracts/routes/models.routes.tssrc/shared/contracts/routes/ocr.routes.tssrc/shared/contracts/routes/providers.routes.tssrc/shared/contracts/routes/runs.routes.tssrc/shared/contracts/routes/settings.routes.tssrc/shared/contracts/routes/skills.routes.tssrc/shared/types/agent-interface.d.tssrc/shared/types/provider.tssrc/shared/types/skill.tssrc/shared/utils/filesystem.tstest/main/app/applicationDataReset.test.tstest/main/app/routes.test.tstest/main/approval/approvalBroker.test.tstest/main/approval/routes.test.tstest/main/cli/agentCommandAccess.test.tstest/main/cli/agentTokenAuthority.test.tstest/main/cli/args.test.tstest/main/cli/artifactSpool.test.tstest/main/cli/artifacts.test.tstest/main/cli/auditLog.test.tstest/main/cli/body.test.tstest/main/cli/client.test.tstest/main/cli/computeService.test.tstest/main/cli/descriptor.test.tstest/main/cli/discovery.test.tstest/main/cli/errors.test.tstest/main/cli/inputCapabilityServices.test.tstest/main/cli/launcherService.test.tstest/main/cli/mcpAdminRoutes.test.tstest/main/cli/mediaOutput.test.tstest/main/cli/mutationGuard.test.tstest/main/cli/packagedSmoke.test.tstest/main/cli/policy.test.tstest/main/cli/providerModelAdminRoutes.test.tstest/main/cli/publicText.test.tstest/main/cli/runService.test.tstest/main/cli/server.test.tstest/main/cli/skillService.test.tstest/main/cli/stdin.test.tstest/main/cli/surface.test.tstest/main/cli/transport.test.tstest/main/contracts/localControl.test.tstest/main/desktop/sessionBinding.test.tstest/main/events/typedEventHub.test.tstest/main/notifications/routes.test.tstest/main/ocr/routes.test.tstest/main/orchestration/orchestrationRoutes.test.tstest/main/provider/providerRuntime.test.tstest/main/provider/routes.test.tstest/main/routes/dispatcher.test.tstest/main/routes/routeRegistry.test.tstest/main/scripts/buildCli.test.tstest/main/session/data/tables/deepchatSessionMetadata.test.tstest/main/session/sessionService.test.tstest/main/shared/filesystem.test.tstest/main/skill/archive.test.tstest/main/skill/archiveDownload.test.tstest/main/skill/skillService.test.tstest/main/skill/skillServiceAgentScopes.test.tstest/main/tool/agentTools/agentBashHandler.test.tstest/main/tool/permission/commandPermissionService.test.tstest/main/tool/toolPermissionBroker.test.tstest/renderer/stores/cliApproval.test.tstest/setup.renderer.tstsconfig.node.json
🚧 Files skipped from review as they are similar to previous changes (139)
- package.json
- src/shared/types/agent-interface.d.ts
- src/main/skill/archiveDownload.ts
- test/setup.renderer.ts
- test/main/tool/toolPermissionBroker.test.ts
- src/renderer/api/ApprovalClient.ts
- src/shared/contracts/routes/approvals.routes.ts
- src/main/session/data/tables/deepchatSessionMetadata.ts
- test/main/orchestration/orchestrationRoutes.test.ts
- test/main/cli/errors.test.ts
- tsconfig.node.json
- src/main/cli/index.ts
- src/shared/contracts/json.ts
- test/main/shared/filesystem.test.ts
- src/shared/utils/filesystem.ts
- test/main/skill/skillService.test.ts
- src/main/desktop/sessionBinding.ts
- src/shared/types/skill.ts
- src/main/session/routes.ts
- src/main/app/settingsRoutes.ts
- src/shared/contracts/events.ts
- src/renderer/src/components/cli/CliApprovalDialog.vue
- src/shared/contracts/common.ts
- src/main/notifications/routes.ts
- src/main/tool/index.ts
- test/main/contracts/localControl.test.ts
- test/main/approval/routes.test.ts
- test/main/provider/providerRuntime.test.ts
- src/cli/discovery.ts
- src/main/app/applicationDataReset.ts
- src/cli/errors.ts
- test/main/ocr/routes.test.ts
- test/main/cli/policy.test.ts
- resources/acp-registry/registry.json
- src/main/tool/permission/index.ts
- test/main/app/routes.test.ts
- src/main/cli/agentTokenAuthority.ts
- test/main/notifications/routes.test.ts
- src/shared/contracts/routes/artifacts.routes.ts
- src/renderer/src/apps/chat-main/ChatMainApp.vue
- test/main/routes/routeRegistry.test.ts
- test/main/cli/stdin.test.ts
- src/shared/contracts/routes/audio.routes.ts
- test/main/cli/mediaOutput.test.ts
- test/main/desktop/sessionBinding.test.ts
- test/main/cli/artifacts.test.ts
- test/main/cli/body.test.ts
- src/shared/contracts/cliCommands.ts
- src/main/tool/agentTools/agentToolManager.ts
- src/main/provider/index.ts
- electron-builder.yml
- src/shared/contracts/contract.ts
- src/shared/contracts/routes/media.routes.ts
- docs/README.md
- test/main/cli/agentCommandAccess.test.ts
- test/main/cli/runService.test.ts
- src/main/routes/index.ts
- src/main/ocr/ocrRuntimeService.ts
- src/shared/contracts/routes/settings.routes.ts
- src/main/session/sessionService.ts
- src/main/cli/mutationGuard.ts
- src/main/provider/routes.ts
- test/main/cli/descriptor.test.ts
- src/main/notifications/electronWindowNotificationTargets.ts
- test/main/tool/agentTools/agentBashHandler.test.ts
- test/main/session/sessionService.test.ts
- test/main/app/applicationDataReset.test.ts
- src/main/cli/surface.ts
- src/main/cli/providerModelAdminRoutes.ts
- src/main/approval/index.ts
- scripts/build-cli.mjs
- src/cli/format.ts
- src/shared/contracts/events/runs.events.ts
- src/shared/contracts/routes/runs.routes.ts
- src/main/cli/ocrService.ts
- src/main/cli/auditLog.ts
- test/main/cli/transport.test.ts
- test/main/cli/client.test.ts
- src/main/app/routes.ts
- src/main/cli/body.ts
- src/shared/contracts/events/approvals.events.ts
- src/main/ocr/routes.ts
- src/shared/contracts/routes.ts
- src/main/cli/policy.ts
- src/main/cli/errors.ts
- docs/architecture/local-control-plane/tasks.md
- test/main/cli/mcpAdminRoutes.test.ts
- src/cli/stdin.ts
- src/main/cli/runService.ts
- test/main/approval/approvalBroker.test.ts
- test/main/cli/mutationGuard.test.ts
- src/cli/artifacts.ts
- src/main/events/typedEventHub.ts
- src/cli/args.ts
- src/main/tool/permission/commandPermissionService.ts
- src/main/mcp/routes.ts
- src/shared/contracts/routes/models.routes.ts
- test/main/tool/permission/commandPermissionService.test.ts
- test/main/routes/dispatcher.test.ts
- test/main/cli/providerModelAdminRoutes.test.ts
- src/main/cli/mediaOutput.ts
- test/main/cli/surface.test.ts
- test/main/provider/routes.test.ts
- src/main/cli/computeService.ts
- src/main/cli/routes.ts
- test/main/session/data/tables/deepchatSessionMetadata.test.ts
- src/shared/contracts/localControl.ts
- src/main/routes/routeRegistry.ts
- docs/guides/cli.md
- src/cli/globals.d.ts
- src/main/desktop/routes.ts
- src/cli/index.ts
- src/cli/transport.ts
- src/shared/contracts/routes/skills.routes.ts
- src/main/cli/mcpAdminRoutes.ts
- test/main/cli/server.test.ts
- test/main/skill/archiveDownload.test.ts
- src/shared/contracts/routes/cli.routes.ts
- src/main/cli/launcherService.ts
- test/main/events/typedEventHub.test.ts
- test/main/cli/artifactSpool.test.ts
- src/main/cli/audioTranscriptionService.ts
- resources/skills/deepchat-cli/SKILL.md
- src/main/approval/routes.ts
- test/renderer/stores/cliApproval.test.ts
- test/main/cli/auditLog.test.ts
- test/main/cli/args.test.ts
- src/cli/run.ts
- src/main/cli/artifactSpool.ts
- test/main/cli/skillService.test.ts
- src/shared/contracts/routes/mcp.routes.ts
- test/main/cli/launcherService.test.ts
- src/main/cli/artifactRoutes.ts
- test/main/skill/skillServiceAgentScopes.test.ts
- src/main/skill/index.ts
- src/main/approval/approvalBroker.ts
- src/main/app/composition.ts
- docs/architecture/local-control-plane/spec.md
- test/main/cli/computeService.test.ts
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
2 participants
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
deepchatCLI backed by the DeepChat main process over HTTP on UDS/named pipes.CLI UX
Approval UI
Before:
After:
The CLI can only wait for an opaque request ID. Approval resolution remains exclusive to the renderer IPC boundary.
Summary by CodeRabbit
New Features
Documentation
Tests