Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions packages/cli/src/agent/agent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import path from "path"
import { Plugin } from "@/plugin"
import { Skill } from "../skill"
import { Log } from "@/util/log"
import { ToolGuidance } from "@/tool/guidance"

export namespace Agent {
const log = Log.create({ service: "agent" })
Expand Down Expand Up @@ -50,6 +51,14 @@ export namespace Agent {
})
export type Info = z.infer<typeof Info>

const prompts = new WeakMap<Info, { source: string; render: ToolGuidance.Renderer }>()

export function renderPrompt(agent: Info, tools: ReadonlySet<string>) {
const owned = prompts.get(agent)
if (!owned || agent.prompt !== owned.source) return agent.prompt
return owned.render(owned.source, tools)
}

const state = Instance.state(async () => {
const cfg = await Config.get()

Expand Down Expand Up @@ -171,6 +180,8 @@ export namespace Agent {
},
}

prompts.set(result.explore, { source: PROMPT_EXPLORE, render: ToolGuidance.explore })

for (const [key, value] of Object.entries(cfg.agent ?? {})) {
// the plan agent was removed: an old override of it must not become a custom agent with full permissions
if (key === "plan") {
Expand All @@ -192,6 +203,7 @@ export namespace Agent {
}
if (value.model) item.model = Provider.parseModel(value.model)
item.variant = value.variant ?? item.variant
if (value.prompt !== undefined) prompts.delete(item)
item.prompt = value.prompt ?? item.prompt
item.description = value.description ?? item.description
item.temperature = value.temperature ?? item.temperature
Expand Down
7 changes: 5 additions & 2 deletions packages/cli/src/file/time.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,15 @@ export namespace FileTime {
}

const time = get(sessionID, filepath)
if (!time) throw new Error(`You must read file ${filepath} before overwriting it. Use the Read tool first`)
if (!time)
throw new Error(
`You must read file ${filepath} before overwriting it. This session has no recorded read of its current contents.`,
)
const mtime = Filesystem.stat(filepath)?.mtime
// Allow a 50ms tolerance for Windows NTFS timestamp fuzziness / async flushing
if (mtime && mtime.getTime() > time.getTime() + 50) {
throw new Error(
`File ${filepath} has been modified since it was last read.\nLast modification: ${mtime.toISOString()}\nLast read: ${time.toISOString()}\n\nPlease read the file again before modifying it.`,
`File ${filepath} has been modified since it was last read.\nLast modification: ${mtime.toISOString()}\nLast read: ${time.toISOString()}\n\nA current read must be recorded in this session before modifying the file.`,
)
}
}
Expand Down
28 changes: 16 additions & 12 deletions packages/cli/src/session/llm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,14 @@ import {
import { mergeDeep, pipe } from "remeda"
import { ProviderTransform } from "@/provider/transform"
import { Config } from "@/config/config"
import type { Agent } from "@/agent/agent"
import { Agent } from "@/agent/agent"
import type { MessageV2 } from "./message-v2"
import { Plugin } from "@/plugin"
import { SystemPrompt } from "./system"
import { PermissionNext } from "@/permission/next"
import { Auth } from "@/auth"
import { ProviderTermination } from "@/provider/termination"
import { ToolGuidance } from "@/tool/guidance"

export namespace LLM {
const log = Log.create({ service: "llm" })
Expand Down Expand Up @@ -67,11 +68,14 @@ export namespace LLM {
Auth.get(input.model.providerID),
])
const isCodex = provider.id === "openai" && auth?.type === "oauth"
const selected = await resolveTools(input)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Tools snapshot taken before Plugin.trigger mutations.

Suggested change
const selected = await resolveTools(input)
Re-resolve or merge tools after Plugin.trigger (or pass the resolved clone through the trigger and use its output) so plugin-side additions/removals on input.tools still affect the final request; alternatively document input.tools as frozen for that hook.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #252, packages/cli/src/session/llm.ts:71-73):

Problem: Tools snapshot taken before Plugin.trigger mutations
Detail: resolveTools() moved from after the Plugin.trigger params hook (old ~L152) to the top of stream(), and now operates on a clone of input.tools. A plugin that adds or removes entries on input.tools during the LLM-param trigger previously had its changes respected (resolution ran afterwards against the mutated object); now those mutations land on input.tools after the `selected`/`names`/`tools` snapshot was taken, so plugin-added tools are dropped and plugin-removed tools are still offered in the request. If no plugin mutates input.tools at that hook today this is latent, but the reorder silently changes the contract for plugin authors. Repro: given a plugin whose chat.params hook does `input.tools["x"] = tool({...})`, streaming any prompt now omits "x" from the outgoing request where the pre-PR code included it.
Suggested fix: Re-resolve or merge tools after Plugin.trigger (or pass the resolved clone through the trigger and use its output) so plugin-side additions/removals on input.tools still affect the final request; alternatively document input.tools as frozen for that hook.

Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters

resolveTools() moved from after the Plugin.trigger params hook (old ~L152) to the top of stream(), and now operates on a clone of input.tools. A plugin that adds or removes entries on input.tools during the LLM-param trigger previously had its changes respected (resolution ran afterwards against the mutated object); now those mutations land on input.tools after the selected/names/tools snapshot was taken, so plugin-added tools are dropped and plugin-removed tools are still offered in the request. If no plugin mutates input.tools at that hook today this is latent, but the reorder silently changes the contract for plugin authors. Repro: given a plugin whose chat.params hook does input.tools["x"] = tool({...}), streaming any prompt now omits "x" from the outgoing request where the pre-PR code included it.

      Auth.get(input.model.providerID),
    ])
    const isCodex = provider.id === "openai" && auth?.type === "oauth"
    const selected = await resolveTools(input)
    const names = ToolGuidance.capabilities(selected)
    const tools = ToolGuidance.render(selected, names)

const names = ToolGuidance.capabilities(selected)
const tools = ToolGuidance.render(selected, names)

const system = []
system.push(
[
...leadPrompt(input.agent, isCodex),
...leadPrompt(input.agent, isCodex, names),
// any custom prompt passed into this call
...input.system,
// any custom prompt from last user message
Expand Down Expand Up @@ -110,7 +114,7 @@ export namespace LLM {
mergeDeep(variant),
)
if (isCodex) {
options.instructions = SystemPrompt.instructions()
options.instructions = SystemPrompt.instructions(names)
}

const params = await Plugin.trigger(
Expand Down Expand Up @@ -148,8 +152,6 @@ export namespace LLM {

const maxOutputTokens = isCodex ? undefined : ProviderTransform.maxOutputTokens(input.model)

const tools = await resolveTools(input)

// LiteLLM and some Anthropic proxies require the tools parameter to be present
// when message history contains tool calls, even if no tools are being used.
// Add a dummy tool that is never called to satisfy this validation.
Expand Down Expand Up @@ -269,22 +271,24 @@ export namespace LLM {
* The prompt that opens the system message: the agent's own prompt, else the shared base prompt.
* Codex sign-in sessions send no base prompt here, because they send the Codex header as `options.instructions`.
*/
function leadPrompt(agent: Agent.Info, isCodex: boolean) {
if (agent.prompt) return [agent.prompt]
function leadPrompt(agent: Agent.Info, isCodex: boolean, tools: ReadonlySet<string>) {
const prompt = Agent.renderPrompt(agent, tools)
if (prompt) return [prompt]
if (isCodex) return []
return [SystemPrompt.base()]
return [SystemPrompt.base(tools)]
}

async function resolveTools(input: Pick<StreamInput, "tools" | "agent" | "permission" | "user">) {
// same ruleset tool calls are checked against, so a denied tool is never offered to the model
const ruleset = PermissionNext.merge(input.agent.permission, input.permission ?? [])
const disabled = PermissionNext.disabled(Object.keys(input.tools), ruleset)
for (const tool of Object.keys(input.tools)) {
const tools = { ...input.tools }
const disabled = PermissionNext.disabled(Object.keys(tools), ruleset)
for (const tool of Object.keys(tools)) {
if (input.user.tools?.[tool] === false || disabled.has(tool)) {
delete input.tools[tool]
delete tools[tool]
}
}
return input.tools
return tools
}

// Check if messages contain any tool-call content
Expand Down
16 changes: 7 additions & 9 deletions packages/cli/src/session/prompt.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { Plugin } from "../plugin"
import MAX_STEPS from "../session/prompt/max-steps.txt"
import { defer } from "../util/defer"
import { ToolRegistry } from "../tool/registry"
import { ToolGuidance } from "../tool/guidance"
import { MCP } from "../mcp"
import { ReadTool } from "../tool/read"
import { FileTime } from "../file/time"
Expand Down Expand Up @@ -941,7 +942,7 @@ export namespace SessionPrompt {
input.session.id,
)) {
const schema = ProviderTransform.schema(input.model, z.toJSONSchema(item.parameters))
tools[item.id] = tool({
const definition = tool({
id: item.id as any,
description: item.description,
inputSchema: jsonSchema(schema as any),
Expand Down Expand Up @@ -981,6 +982,7 @@ export namespace SessionPrompt {
return output
},
})
tools[item.id] = ToolGuidance.bind(definition, item.builtin, item.renderDescription)
}

for (const [key, item] of Object.entries(await MCP.tools())) {
Expand Down Expand Up @@ -1432,9 +1434,6 @@ export namespace SessionPrompt {
}

if (part.type === "agent") {
// Check if this agent would be denied by task permission
const perm = PermissionNext.evaluate("task", part.name, agent.permission)
const hint = perm.action === "deny" ? " . Invoked by user; guaranteed to exist." : ""
return [
{
...part,
Expand All @@ -1446,12 +1445,11 @@ export namespace SessionPrompt {
sessionID: input.sessionID,
type: "text",
synthetic: true,
// An extra space is added here. Otherwise the 'Use' gets appended
// to user's last word; making a combined word
// Keep this fresh instruction neutral: final request tools are resolved later.
text:
" Use the above message and context to generate a prompt and call the task tool with subagent: " +
part.name +
hint,
` The user requested delegation to subagent: ${part.name}. ` +
"Use the preceding message and context to prepare its task. " +
"Delegate if an appropriate delegation tool is available; otherwise report that delegation is unavailable.",
},
]
}
Expand Down
49 changes: 45 additions & 4 deletions packages/cli/src/session/system.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,56 @@ import { Instance } from "../project/instance"
import PROMPT_DEFAULT from "./prompt/default.txt"
import PROMPT_CODEX from "./prompt/codex_header.txt"
import type { Provider } from "@/provider/provider"
import { ToolGuidance } from "../tool/guidance"

export namespace SystemPrompt {
export function instructions() {
return PROMPT_CODEX.trim()
export function instructions(tools?: ReadonlySet<string>) {
const patch =
"- Try to use apply_patch for single file edits, but it is fine to explore other options to make the edit if it does not work well. Do not use apply_patch for changes that are auto-generated (i.e. generating package.json or running a lint or format command like gofmt) or when scripting is more efficient (such as search and replacing a string across a codebase).\n"
const files = " - Use Read to view files, Edit to modify files, and Write only when needed.\n"
const search = " - Use Glob to find files by name and Grep to search file contents.\n"
const bash = "- Use Bash for terminal operations (git, bun, builds, tests, running scripts).\n"
const specialized = "- Prefer specialized tools over shell for file operations:\n"
const available = (name: string) => tools?.has(name) ?? true
const operations = [
...(available("read") ? ["Read to view files"] : []),
...(available("edit") ? ["Edit to modify files"] : []),
...(available("write") ? ["Write only when needed"] : []),
]
const searches = [
...(available("glob") ? ["Glob to find files by name"] : []),
...(available("grep") ? ["Grep to search file contents"] : []),
]
const prompt = ToolGuidance.replace(PROMPT_CODEX.trim(), patch, available("apply_patch") ? patch : "")
const editing = ToolGuidance.replace(
prompt,
files,
operations.length === 3 ? files : operations.length ? ` - Use ${operations.join("; ")}.\n` : "",
)
const searching = ToolGuidance.replace(
editing,
search,
searches.length === 2 ? search : searches.length ? ` - Use ${searches[0]}.\n` : "",
)
return ToolGuidance.replace(
ToolGuidance.replace(searching, bash, available("bash") ? bash : ""),
specialized,
operations.length || searches.length ? specialized : "",
)
}

/** The base system prompt, shared by every provider and model family. */
export function base() {
return PROMPT_DEFAULT.trim()
export function base(tools?: ReadonlySet<string>) {
const task = "- Use the Task tool when a search is broad and open-ended, or when asked to delegate.\n"
const bash =
"- When you have finished, run the lint and typecheck commands (for example npm run lint, npm run typecheck, ruff) with bash if they were provided to you.\n"
return ToolGuidance.replace(
ToolGuidance.replace(PROMPT_DEFAULT.trim(), task, tools && !tools.has("task") ? "" : task),
bash,
tools && !tools.has("bash")
? "- When you have finished, verify your changes using the available capabilities. Report any provided lint or typecheck commands you could not run.\n"
: bash,
)
}

export async function environment(model: Provider.Model) {
Expand Down
7 changes: 6 additions & 1 deletion packages/cli/src/tool/bash.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import z from "zod"
import { spawn } from "child_process"
import { Tool } from "./tool"
import { ToolGuidance } from "./guidance"
import path from "path"
import DESCRIPTION from "./bash.txt"
import { Log } from "../util/log"
Expand Down Expand Up @@ -57,7 +58,10 @@ const SHELL_INTERPRETERS = new Set(["bash", "sh", "zsh", "dash", "ksh", "fish"])
const TOKEN_TYPES = new Set(["command_name", "word", "string", "raw_string", "concatenation"])

/** Extract token text from a command node, filtering to known token types. */
function getCommandTokens(node: { childCount: number; child(i: number): { type: string; text: string } | null }): string[] {
function getCommandTokens(node: {
childCount: number
child(i: number): { type: string; text: string } | null
}): string[] {
const tokens: string[] = []
for (let i = 0; i < node.childCount; i++) {
const child = node.child(i)
Expand Down Expand Up @@ -94,6 +98,7 @@ export const BashTool = Tool.define("bash", async () => {
log.info("bash tool using shell", { shell })

return {
renderDescription: ToolGuidance.bash,
// Replacer functions insert values literally, without `$` pattern expansion
description: DESCRIPTION.replaceAll("${directory}", () => Instance.directory)
.replaceAll("${maxLines}", () => String(Truncate.MAX_LINES))
Expand Down
2 changes: 2 additions & 0 deletions packages/cli/src/tool/batch.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
import z from "zod"
import { Tool } from "./tool"
import DESCRIPTION from "./batch.txt"
import { ToolGuidance } from "./guidance"

const DISALLOWED = new Set(["batch"])
const FILTERED_FROM_SUGGESTIONS = new Set(["invalid", "patch", ...DISALLOWED])

export const BatchTool = Tool.define("batch", async () => {
return {
description: DESCRIPTION,
renderDescription: ToolGuidance.batch,
parameters: z.object({
tool_calls: z
.array(
Expand Down
2 changes: 2 additions & 0 deletions packages/cli/src/tool/edit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import z from "zod"
import * as path from "path"
import { Tool } from "./tool"
import { ToolGuidance } from "./guidance"
import { createTwoFilesPatch, diffLines } from "diff"
import DESCRIPTION from "./edit.txt"
import { File } from "../file"
Expand All @@ -21,6 +22,7 @@ function normalizeLineEndings(text: string): string {

export const EditTool = Tool.define("edit", {
description: DESCRIPTION,
renderDescription: ToolGuidance.edit,
parameters: z.object({
filePath: z.string().describe("The absolute path to the file to modify"),
oldString: z.string().describe("The text to replace"),
Expand Down
2 changes: 2 additions & 0 deletions packages/cli/src/tool/glob.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import z from "zod"
import path from "path"
import { Tool } from "./tool"
import { ToolGuidance } from "./guidance"
import { Filesystem } from "../util/filesystem"
import DESCRIPTION from "./glob.txt"
import { Ripgrep } from "../file/ripgrep"
Expand All @@ -9,6 +10,7 @@ import { assertExternalDirectory } from "./external-directory"

export const GlobTool = Tool.define("glob", {
description: DESCRIPTION,
renderDescription: ToolGuidance.glob,
parameters: z.object({
pattern: z.string().describe("The glob pattern to match files against"),
path: z
Expand Down
2 changes: 2 additions & 0 deletions packages/cli/src/tool/grep.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import z from "zod"
import { text } from "node:stream/consumers"
import { Tool } from "./tool"
import { ToolGuidance } from "./guidance"
import { Filesystem } from "../util/filesystem"
import { Ripgrep } from "../file/ripgrep"
import { Process } from "../util/process"
Expand All @@ -14,6 +15,7 @@ const MAX_LINE_LENGTH = 2000

export const GrepTool = Tool.define("grep", {
description: DESCRIPTION,
renderDescription: ToolGuidance.grep,
parameters: z.object({
pattern: z.string().describe("The regex pattern to search for in file contents"),
path: z.string().optional().describe("The directory to search in. Defaults to the current working directory."),
Expand Down
Loading
Loading