Conversation
📝 WalkthroughWalkthroughRenames and canonicalizes agent tools (read, write, edit, find, grep, ls, exec, process); adds system env/runtime prompt builders and consolidated system-prompt composition; normalizes skill allowedTools with warnings; adds cwd support for command execution; updates offload eligibility and multiple planning/spec docs and tests. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client as Agent Client
participant SkillPresenter as SkillPresenter\n(normalizeSkillAllowedTools)
participant MessageBuilder as MessageBuilder\n(composeAgentSystemPromptSections)
participant SystemEnv as SystemEnvPromptBuilder
participant AgentToolManager as AgentToolManager\n(canonical tools)
participant ToolProcessor as ToolCallProcessor
User->>Client: Start agent run (with skills)
Client->>SkillPresenter: getActiveSkillsAllowedTools()
SkillPresenter->>SkillPresenter: normalizeSkillAllowedTools()
SkillPresenter-->>Client: canonical allowedTools
Client->>MessageBuilder: compose system prompt
MessageBuilder->>SystemEnv: buildSystemEnvPrompt()
SystemEnv->>SystemEnv: detect git, read AGENTS.md, resolve model id
SystemEnv-->>MessageBuilder: env prompt
MessageBuilder->>MessageBuilder: composeAgentSystemPromptSections()
MessageBuilder-->>Client: final system prompt
Client->>AgentToolManager: getToolDefinitions()
AgentToolManager-->>Client: definitions (read, write, edit, find, grep, ls, exec, process)
Note over Client,ToolProcessor: Model emits tool calls
Client->>ToolProcessor: process tool calls
ToolProcessor->>ToolProcessor: pre-checks, permission batch
ToolProcessor->>AgentToolManager: execute tool (canonical name)
ToolProcessor->>ToolProcessor: decide offload (exec, ls, find, grep)
ToolProcessor-->>Client: tool results / offload events
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@docs/specs/agent-tooling-v2/spec.md`:
- Around line 182-197: The <files>...</files> block in the Env Prompt example in
spec.md is empty and ambiguous; update the Env Prompt example (the block
containing "You are powered by the model named <model-name>..." and the <env>
and <files> sections) to include a clear placeholder or descriptive comment
inside the <files> block (e.g., indicating it should contain a file listing or
file contents/paths) so implementers know what to place there and how it will be
used.
In `@docs/specs/agent-tooling-v2/tasks.md`:
- Around line 16-51: The PR checklist in docs/specs/agent-tooling-v2/tasks.md is
outdated: mark completed items as done by changing the T2 and T5 task boxes to
checked; specifically update the T2 items referencing systemEnvPromptBuilder.ts
and buildSystemEnvPrompt(...) to [x], and update T5 items that mention
systemEnvPromptBuilder tests and updated toolCallProcessor/agentToolManager
tests to [x]; leave T1 (allowParallel) and T4 (promptEnhancer convergence)
unchecked so remaining work is tracked accurately.
In `@src/main/presenter/agentPresenter/message/systemEnvPromptBuilder.ts`:
- Line 113: The template literal that builds the instructions string contains a
stray space before the newline sequence: change the fragment using
agentsFilePath (the ``Instructions from: ${agentsFilePath} \n`` template) to
remove the trailing space so the newline is immediate (e.g., ``Instructions
from: ${agentsFilePath}\n``) ensuring no visible trailing space appears in the
generated prompt; update this in the systemEnvPromptBuilder code where that
template string is constructed.
In `@src/main/presenter/skillPresenter/index.ts`:
- Around line 76-84: The current repair logic uses
compareResolved.startsWith(compareBrokenPrefix) which can false-positive on
names like "<homeDir>.deepchat-extension"; update the check to ensure the
matched prefix is followed only by a path separator or end-of-string before
repairing: after computing compareResolved and compareBrokenPrefix, only proceed
if compareResolved.startsWith(compareBrokenPrefix) AND (compareResolved.length
=== compareBrokenPrefix.length OR the character at index
compareBrokenPrefix.length in compareResolved is '/' or '\\'); keep the existing
platform-specific case handling and then compute suffix from the original
resolved (using brokenPrefix.length) and return path.join(homeDir, '.deepchat',
suffix) as before.
In `@test/main/presenter/agentPresenter/systemEnvPromptBuilder.test.ts`:
- Around line 65-66: The test is asserting the prompt contains an exactModelId
string that buildSystemEnvPrompt doesn't emit; either include exactModelId in
the prompt or remove the assertion and relocate the test file to mirror source
layout. Fix by updating buildSystemEnvPrompt (the function) to destructure
exactModelId from resolveModelIdentity and append a human-readable line (e.g.,
"The exact model ID is <exactModelId>") in the prompt composition, or delete the
offending expect line in the test; then move the test file to
test/main/presenter/agentPresenter/message/systemEnvPromptBuilder.test.ts so the
tests mirror
src/main/presenter/agentPresenter/message/systemEnvPromptBuilder.ts.
🧹 Nitpick comments (10)
src/main/presenter/agentPresenter/loop/toolCallProcessor.ts (1)
578-583: Non-English user-facing prompt strings remain.Lines 579, 671, 693–694 contain Chinese text used as user prompts in legacy function-call and error paths. While these are pre-existing and not part of this PR's changes, they violate the coding guideline requiring all logs and comments to be in English. Consider addressing in a follow-up.
src/main/presenter/agentPresenter/message/systemEnvPromptBuilder.ts (3)
62-74: Synchronousfs.existsSyncblocks the main process thread.
isGitRepositoryperforms synchronous filesystem I/O that traverses up the directory tree. Since the caller (buildSystemEnvPrompt) is alreadyasync, this could usefs.promises.accessinstead to avoid blocking the Electron main process.♻️ Proposed async refactor
-function isGitRepository(workdir: string): boolean { - let current = path.resolve(workdir) - while (true) { - if (fs.existsSync(path.join(current, '.git'))) { - return true - } - const parent = path.dirname(current) - if (parent === current) { - return false - } - current = parent - } -} +async function isGitRepository(workdir: string): Promise<boolean> { + let current = path.resolve(workdir) + while (true) { + try { + await fs.promises.access(path.join(current, '.git')) + return true + } catch { + // .git not found at this level + } + const parent = path.dirname(current) + if (parent === current) { + return false + } + current = parent + } +}Then update line 110:
- `Is directory a git repo: ${isGitRepository(workdir) ? 'yes' : 'no'}`, + `Is directory a git repo: ${(await isGitRepository(workdir)) ? 'yes' : 'no'}`,
104-104:exactModelIdis computed but never used.
resolveModelIdentityreturns bothmodelNameandexactModelId, but onlymodelNameis destructured on line 104. IfexactModelIdis intended for future use, consider deferring its computation; otherwise, the concatenation at line 50 is dead code.
14-35: Near-duplicate ofresolveModelDisplayNameinmcpClient.ts.This function is nearly identical to the one in
src/main/presenter/mcpPresenter/mcpClient.ts(lines 824–845 in the relevant snippet). Consider extracting it to a shared utility to avoid drift between the two implementations.src/main/presenter/agentPresenter/utility/promptEnhancer.ts (1)
1-16: Function name and interface are now misleading after gutting the logic.
enhanceSystemPromptWithDateTimeno longer enhances with date/time — it's just a trim-or-passthrough. TheEnhanceOptionsinterface retains unused fields (isAgentMode,agentWorkspacePath,platform). Consider renaming or simplifying in a follow-up to avoid confusion for future maintainers.src/main/presenter/agentPresenter/acp/agentBashHandler.ts (2)
26-26: Newcwdfield inExecuteCommandArgsSchema— no length or pattern constraint.The
cwdfield isz.string().optional()with no maximum length or character restriction. WhileresolveWorkingDirectoryvalidates against the allowlist, a very long or malformed string could still reachpath.resolvebefore validation. Consider adding a reasonable.max()bound for defense in depth.♻️ Suggested constraint
- cwd: z.string().optional(), + cwd: z.string().max(1024).optional(),
190-197: Path validation algorithms diverge between AgentBashHandler and AgentFileSystemHandler.
AgentBashHandler.isPathAllowedusespath.relative()with platform-aware normalization (case-insensitive on Windows), whileAgentFileSystemHandler.isPathAlloweduses string prefix matching without normalization. This causes real divergence on Windows: paths with different casing would pass validation in AgentBashHandler but fail in AgentFileSystemHandler. Consider extracting a shared utility for consistent path allowlist validation across both handlers.src/main/presenter/toolPresenter/index.ts (1)
150-166: Consider extracting the duplicated JSON argument parsing logic.The
callToolandpreCheckToolPermissionmethods share identical JSON parse → jsonrepair fallback → empty args logic. Extracting a private helper likeparseToolArguments(argsString: string): Record<string, unknown>would reduce duplication.♻️ Suggested helper extraction
+ private parseToolArguments(argsString: string | undefined): Record<string, unknown> { + const raw = argsString || '' + if (raw.trim().length === 0) return {} + try { + return JSON.parse(raw) as Record<string, unknown> + } catch (error) { + console.warn('[ToolPresenter] Failed to parse tool arguments, trying jsonrepair:', error) + try { + return JSON.parse(jsonrepair(raw)) as Record<string, unknown> + } catch (error) { + console.warn('[ToolPresenter] Failed to repair tool arguments, using empty args.', error) + return {} + } + } + }Also applies to: 206-226
docs/specs/agent-tooling-v2/tasks.md (1)
1-1: Consider writing task descriptions in English for broader contributor access.The task document is entirely in Chinese. While this doesn't violate the coding guidelines (which target code files), English documentation would be more accessible to international contributors. As per coding guidelines, the project enforces English for all logs and comments in code files — extending that convention to spec documents would improve consistency.
src/main/presenter/agentPresenter/acp/agentToolManager.ts (1)
742-761: Redundant nullish coalescing ondepth.
lsArgs.depth ?? 1is unnecessary since the zod schema at Line 99 defines.default(1), guaranteeingdepthis always a number after parsing. This won't cause a bug but is misleading — it suggestsdepthmight be undefined.Suggested simplification
- if ((lsArgs.depth ?? 1) > 1) { + if (lsArgs.depth > 1) {And update the type accordingly:
const lsArgs = parsedArgs as { path: string - depth?: number + depth: number }
src/main/presenter/agentPresenter/message/systemEnvPromptBuilder.ts
Outdated
Show resolved
Hide resolved
test/main/presenter/agentPresenter/message/systemEnvPromptBuilder.test.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/presenter/agentPresenter/message/systemEnvPromptBuilder.ts`:
- Around line 76-82: readAgentsInstructions currently embeds raw error details
into the system prompt when AGENTS.md can't be read, leaking internal info;
change readAgentsInstructions to return a neutral, non-sensitive fallback string
(e.g. "[SystemEnvPromptBuilder] AGENTS.md not available") instead of
interpolating the error, and move the error logging to a private logger call (or
processLogger) inside the catch so the actual error is recorded internally but
not returned to the LLM; keep the function signature and ensure the fallback is
used wherever readAgentsInstructions is consumed.
🧹 Nitpick comments (2)
src/main/presenter/agentPresenter/message/systemEnvPromptBuilder.ts (1)
14-35: DuplicatedresolveModelDisplayNamelogic — consider extracting a shared helper.This function is nearly identical to
resolveModelDisplayNameinsrc/main/presenter/mcpPresenter/mcpClient.ts(lines 824–845). Consider extracting it into a shared utility to avoid drift between the two copies.test/main/presenter/agentPresenter/message/systemEnvPromptBuilder.test.ts (1)
85-88: Test asserts on internal error message format — fragile coupling.Line 88 asserts on the
[SystemEnvPromptBuilder]prefix of the error fallback string. If the error message wording changes (e.g., per the suggestion to use a neutral fallback), this assertion breaks. Consider asserting on a stable, user-facing fallback string instead.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/architecture/tool-system.md (1)
553-566:⚠️ Potential issue | 🟡 MinorStale tool names in the documentation table.
This PR canonicalizes tool names (e.g.,
read_file→read,write_file→write,list_directory→ls,glob_search→find,grep_search→grep, etc.), but this table and several other sections (e.g., line 201, line 646) still reference the old names. The doc should be updated to stay consistent with the new canonical names.
🤖 Fix all issues with AI agents
In `@src/main/presenter/agentPresenter/message/systemEnvPromptBuilder.ts`:
- Around line 111-118: The promptLines array has an inconsistency: the string
"The exact model ID is ${exactModelId}" (in promptLines) is missing a trailing
period; update that element inside promptLines so it ends with a period to match
the other sentences (i.e., change the promptLines entry referencing exactModelId
within the same block where modelName and workdir are used).
🧹 Nitpick comments (2)
src/main/presenter/agentPresenter/message/systemEnvPromptBuilder.ts (2)
15-36: DuplicatedresolveModelDisplayNamelogic withmcpClient.ts.This function is nearly identical to
McpClient.resolveModelDisplayName(seesrc/main/presenter/mcpPresenter/mcpClient.tslines 824–845). Consider extracting a shared helper under a common utility module to avoid drift between the two copies.
63-75: Synchronous directory traversal on the main process thread.
isGitRepositoryusesfs.existsSyncin a loop that walks up to the filesystem root. While directory depth is bounded, this blocks the Electron main process during each call. Consider usingfs.promises.access(orfs.promises.stat) in an async version to keep the main thread responsive.
1 participant
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests