diff --git a/.changeset/strict-ask-plan-guards.md b/.changeset/strict-ask-plan-guards.md new file mode 100644 index 00000000000..d46736109b6 --- /dev/null +++ b/.changeset/strict-ask-plan-guards.md @@ -0,0 +1,5 @@ +--- +"@kilocode/cli": patch +--- + +Prevent Ask and Plan modes from editing files before an explicit implementation step. diff --git a/packages/opencode/src/kilocode/agent/index.ts b/packages/opencode/src/kilocode/agent/index.ts index 9cc4d4aaa49..e74780ad69a 100644 --- a/packages/opencode/src/kilocode/agent/index.ts +++ b/packages/opencode/src/kilocode/agent/index.ts @@ -6,124 +6,17 @@ import { Truncate } from "../../tool" import { Config } from "../../config" import { Instance } from "../../project/instance" import { makeRuntime } from "@/effect/run-service" -import { Global } from "@/global" import { Telemetry } from "@kilocode/kilo-telemetry" import z from "zod" import path from "path" +import { askGuard, bash, planGuard } from "./permissions" +export { bash, readOnlyBash } from "./permissions" import PROMPT_DEBUG from "../../agent/prompt/debug.txt" import PROMPT_ORCHESTRATOR from "../../agent/prompt/orchestrator.txt" import PROMPT_ASK from "../../agent/prompt/ask.txt" import PROMPT_EXPLORE from "../../agent/prompt/explore.txt" -// Safe bash commands that don't need user approval. -// Only commands that cannot execute arbitrary code or subprocesses. -export const bash: Record = { - "*": "ask", - // read-only / informational - "cat *": "allow", - "head *": "allow", - "tail *": "allow", - "less *": "allow", - "ls *": "allow", - "tree *": "allow", - "pwd *": "allow", - "echo *": "allow", - "wc *": "allow", - "which *": "allow", - "type *": "allow", - "file *": "allow", - "diff *": "allow", - "du *": "allow", - "df *": "allow", - "date *": "allow", - "uname *": "allow", - "whoami *": "allow", - "printenv *": "allow", - "man *": "allow", - // text processing - "grep *": "allow", - "rg *": "allow", - "ag *": "allow", - "sort *": "allow", - "uniq *": "allow", - "cut *": "allow", - "tr *": "allow", - "jq *": "allow", - // file operations - "touch *": "allow", - "mkdir *": "allow", - "cp *": "allow", - "mv *": "allow", - // compilers (no script execution) - "tsc *": "allow", - "tsgo *": "allow", - // archive - "tar *": "allow", - "unzip *": "allow", - "gzip *": "allow", - "gunzip *": "allow", -} - -// Read-only bash commands for ask/plan agents. -// Unknown commands are DENIED (not "ask") because these agents must never modify the filesystem. -export const readOnlyBash: Record = { - "*": "deny", - // read-only / informational - "cat *": "allow", - "head *": "allow", - "tail *": "allow", - "less *": "allow", - "ls *": "allow", - "tree *": "allow", - "pwd *": "allow", - "echo *": "allow", - "wc *": "allow", - "which *": "allow", - "type *": "allow", - "file *": "allow", - "diff *": "allow", - "du *": "allow", - "df *": "allow", - "date *": "allow", - "uname *": "allow", - "whoami *": "allow", - "printenv *": "allow", - "man *": "allow", - // text processing (stdout only, no file modification) - "grep *": "allow", - "rg *": "allow", - "ag *": "allow", - "sort *": "allow", - "uniq *": "allow", - "cut *": "allow", - "tr *": "allow", - "jq *": "allow", - // git — allowlist of read-only subcommands, deny everything else - "git *": "deny", - "git log *": "allow", - "git show *": "allow", - "git diff *": "allow", - "git status *": "allow", - "git blame *": "allow", - "git rev-parse *": "allow", - "git rev-list *": "allow", - "git ls-files *": "allow", - "git ls-tree *": "allow", - "git ls-remote *": "allow", - "git shortlog *": "allow", - "git describe *": "allow", - "git cat-file *": "allow", - "git name-rev *": "allow", - "git stash list *": "allow", - "git tag -l *": "allow", - "git branch --list *": "allow", - "git branch -a *": "allow", - "git branch -r *": "allow", - "git remote -v *": "allow", - // gh — require user approval since commands vary widely - "gh *": "ask", -} // Generate per-server MCP wildcard rules that allow MCP tools with user approval. export function getMcpRules(cfg: Config.Info): Record { @@ -227,26 +120,12 @@ export function patchAgents( if (agents.plan) { agents.plan = { ...agents.plan, - description: "Plan mode. Only allows editing plan files; asks before editing anything else.", + description: "Plan mode. Can only edit plan files; all other filesystem mutations are denied.", permission: Permission.merge( defaults, - Permission.fromConfig({ - question: "allow", - suggest: "allow", // kilocode_change - plan_exit: "allow", - bash: readOnlyBash, - ...kilo.mcpRules, - external_directory: { - [path.join(Global.Path.data, "plans", "*")]: "allow", - }, - edit: { - "*": "ask", - [path.join(".kilo", "plans", "*.md")]: "allow", - [path.join(".opencode", "plans", "*.md")]: "allow", - [path.relative(Instance.worktree, path.join(Global.Path.data, path.join("plans", "*.md")))]: "allow", - }, - }), user, + planGuard(kilo.mcpRules), + user.filter((r: Permission.Rule) => r.action === "deny"), ), } } @@ -347,28 +226,7 @@ export function patchAgents( permission: Permission.merge( defaults, user, // user before ask-specific so ask's deny+allowlist wins - Permission.fromConfig({ - "*": "deny", - bash: readOnlyBash, - read: { - "*": "allow", - "*.env": "ask", - "*.env.*": "ask", - "*.env.example": "allow", - }, - grep: "allow", - glob: "allow", - list: "allow", - question: "allow", - webfetch: "allow", - websearch: "allow", - codesearch: "allow", - codebase_search: "allow", - external_directory: { - [Truncate.GLOB]: "allow", - }, - ...kilo.mcpRules, - }), + askGuard(kilo.mcpRules), user.filter((r: Permission.Rule) => r.action === "deny"), // re-apply user denies so explicit MCP blocks win over mcpRules ), mode: "primary", diff --git a/packages/opencode/src/kilocode/agent/permissions.ts b/packages/opencode/src/kilocode/agent/permissions.ts new file mode 100644 index 00000000000..386eeba7add --- /dev/null +++ b/packages/opencode/src/kilocode/agent/permissions.ts @@ -0,0 +1,168 @@ +import path from "path" +import { Global } from "@/global" +import { Instance } from "@/project/instance" +import { Permission } from "@/permission" +import * as Truncate from "@/tool/truncate" + +export const bash: Record = { + "*": "ask", + "cat *": "allow", + "head *": "allow", + "tail *": "allow", + "less *": "allow", + "ls *": "allow", + "tree *": "allow", + "pwd *": "allow", + "echo *": "allow", + "wc *": "allow", + "which *": "allow", + "type *": "allow", + "file *": "allow", + "diff *": "allow", + "du *": "allow", + "df *": "allow", + "date *": "allow", + "uname *": "allow", + "whoami *": "allow", + "printenv *": "allow", + "man *": "allow", + "grep *": "allow", + "rg *": "allow", + "ag *": "allow", + "sort *": "allow", + "uniq *": "allow", + "cut *": "allow", + "tr *": "allow", + "jq *": "allow", + "touch *": "allow", + "mkdir *": "allow", + "cp *": "allow", + "mv *": "allow", + "tsc *": "allow", + "tsgo *": "allow", + "tar *": "allow", + "unzip *": "allow", + "gzip *": "allow", + "gunzip *": "allow", +} + +export const readOnlyBash: Record = { + "*": "deny", + "cat *": "allow", + "head *": "allow", + "tail *": "allow", + "less *": "allow", + "ls *": "allow", + "tree *": "allow", + "pwd *": "allow", + "echo *": "allow", + "wc *": "allow", + "which *": "allow", + "type *": "allow", + "file *": "allow", + "diff *": "allow", + "du *": "allow", + "df *": "allow", + "date *": "allow", + "uname *": "allow", + "whoami *": "allow", + "printenv *": "allow", + "man *": "allow", + "grep *": "allow", + "rg *": "allow", + "ag *": "allow", + "sort *": "allow", + "uniq *": "allow", + "cut *": "allow", + "tr *": "allow", + "jq *": "allow", + "git *": "deny", + "git log *": "allow", + "git show *": "allow", + "git diff *": "allow", + "git status *": "allow", + "git blame *": "allow", + "git rev-parse *": "allow", + "git rev-list *": "allow", + "git ls-files *": "allow", + "git ls-tree *": "allow", + "git ls-remote *": "allow", + "git shortlog *": "allow", + "git describe *": "allow", + "git cat-file *": "allow", + "git name-rev *": "allow", + "git stash list *": "allow", + "git tag -l *": "allow", + "git branch --list *": "allow", + "git branch -a *": "allow", + "git branch -r *": "allow", + "git remote -v *": "allow", + "gh *": "ask", + "*>*": "deny", + "* > *": "deny", + "*>>*": "deny", + "* >> *": "deny", + "*>|*": "deny", + "* >| *": "deny", + "sort -o *": "deny", + "sort * -o *": "deny", +} + +export function askGuard(mcp: Record = {}) { + return Permission.fromConfig({ + "*": "deny", + bash: readOnlyBash, + read: { + "*": "allow", + "*.env": "ask", + "*.env.*": "ask", + "*.env.example": "allow", + }, + grep: "allow", + glob: "allow", + list: "allow", + question: "allow", + webfetch: "allow", + websearch: "allow", + codesearch: "allow", + codebase_search: "allow", + external_directory: { + [Truncate.GLOB]: "allow", + }, + ...mcp, + }) +} + +export function planGuard(mcp: Record = {}) { + return Permission.fromConfig({ + "*": "deny", + question: "allow", + suggest: "allow", + plan_exit: "allow", + bash: readOnlyBash, + read: { + "*": "allow", + "*.env": "ask", + "*.env.*": "ask", + "*.env.example": "allow", + }, + grep: "allow", + glob: "allow", + list: "allow", + webfetch: "allow", + websearch: "allow", + codesearch: "allow", + codebase_search: "allow", + external_directory: { + [Truncate.GLOB]: "allow", + [path.join(Global.Path.data, "plans", "*")]: "allow", + }, + edit: { + "*": "deny", + [path.join(".kilo", "plans", "*.md")]: "allow", + [path.join(".opencode", "plans", "*.md")]: "allow", + [path.relative(Instance.worktree, path.join(Global.Path.data, path.join("plans", "*.md")))]: "allow", + }, + ...mcp, + }) +} diff --git a/packages/opencode/src/kilocode/session/prompt.ts b/packages/opencode/src/kilocode/session/prompt.ts index 18af8e99f0d..8dcb90e08d9 100644 --- a/packages/opencode/src/kilocode/session/prompt.ts +++ b/packages/opencode/src/kilocode/session/prompt.ts @@ -9,6 +9,7 @@ import { Session } from "@/session" import { Flag } from "@/flag/flag" import { PlanFollowup } from "@/kilocode/plan-followup" import { KiloSession } from "@/kilocode/session" +import { Permission } from "@/permission" import { environmentDetails, type EditorContext } from "@/kilocode/editor-context" import { Identifier } from "@/id/id" import { Filesystem } from "@/util" @@ -54,6 +55,15 @@ export namespace KiloSessionPrompt { return PlanFollowup.abort(sessionID) } + export function guardPermissions(input: { + agent: { name: string; permission: Permission.Ruleset } + session: Pick + }) { + const rules = input.session.permission ?? [] + if (!["ask", "plan"].includes(input.agent.name)) return rules + return Permission.merge(rules, input.agent.permission, rules.filter((rule) => rule.action === "deny")) + } + /** * Mutable cache for environment details, keyed by user message ID * so it recomputes when a new user message arrives. diff --git a/packages/opencode/src/session/prompt.ts b/packages/opencode/src/session/prompt.ts index 193b289c55f..443db3e0064 100644 --- a/packages/opencode/src/session/prompt.ts +++ b/packages/opencode/src/session/prompt.ts @@ -400,7 +400,12 @@ NOTE: At any point in time through this workflow you should feel free to ask the ...req, sessionID: input.session.id, tool: { messageID: input.processor.message.id, callID: options.toolCallId }, - ruleset: Permission.merge(input.agent.permission, input.session.permission ?? []), + // kilocode_change start - reapply Ask/Plan mode guards after session permissions + ruleset: Permission.merge( + input.agent.permission, + KiloSessionPrompt.guardPermissions({ agent: input.agent, session: input.session }), + ), + // kilocode_change end }) .pipe(Effect.orDie), }) @@ -620,7 +625,12 @@ NOTE: At any point in time through this workflow you should feel free to ask the .ask({ ...req, sessionID, - ruleset: Permission.merge(taskAgent.permission, session.permission ?? []), + // kilocode_change start - reapply Ask/Plan subagent guards after session permissions + ruleset: Permission.merge( + taskAgent.permission, + KiloSessionPrompt.guardPermissions({ agent: taskAgent, session }), + ), + // kilocode_change end }) .pipe(Effect.orDie), }) @@ -1388,6 +1398,23 @@ NOTE: At any point in time through this workflow you should feel free to ask the const hasToolCalls = lastAssistantMsg?.parts.some((part) => part.type === "tool" && !part.metadata?.providerExecuted) ?? false + // kilocode_change start - plan_exit is a hard stop before another model call + if ( + lastAssistant?.finish && + hasToolCalls && + lastAssistant.parentID === lastUser.id && + lastUser.id < lastAssistant.id && + KiloSessionPrompt.shouldAskPlanFollowup({ messages: msgs, abort: AbortSignal.any([]) }) + ) { + const action = yield* Effect.promise((signal) => + KiloSessionPrompt.askPlanFollowup({ sessionID, messages: msgs, abort: signal }), + ) + if (action === "continue") continue + yield* slog.info("exiting loop") + break + } + // kilocode_change end + if ( lastAssistant?.finish && !["tool-calls"].includes(lastAssistant.finish) && @@ -1560,7 +1587,9 @@ NOTE: At any point in time through this workflow you should feel free to ask the const result = yield* handle.process({ user: lastUser, agent, - permission: session.permission, + // kilocode_change start - keep Ask/Plan tool filtering hardened against session allows + permission: KiloSessionPrompt.guardPermissions({ agent, session }), + // kilocode_change end sessionID, parentSessionID: session.parentID, system, diff --git a/packages/opencode/test/agent/agent.test.ts b/packages/opencode/test/agent/agent.test.ts index 429ae496098..ab4c7516193 100644 --- a/packages/opencode/test/agent/agent.test.ts +++ b/packages/opencode/test/agent/agent.test.ts @@ -119,20 +119,36 @@ test("ask agent denies edit/write/bash even when user config adds a specific edi // kilocode_change end // kilocode_change start -test("plan agent asks before edits except .kilo/plans/* and .opencode/plans/*", async () => { +test("plan agent denies edits except .kilo/plans/* and .opencode/plans/*", async () => { await using tmp = await tmpdir() await Instance.provide({ directory: tmp.path, fn: async () => { const plan = await load(tmp.path, (svc) => svc.get("plan")) expect(plan).toBeDefined() - // Wildcard requires permission - expect(evalPerm(plan, "edit")).toBe("ask") - // kilocode_change start - // .kilo/plans/ is the primary allowed path + expect(evalPerm(plan, "edit")).toBe("deny") + expect(Permission.evaluate("edit", "src/index.ts", plan!.permission).action).toBe("deny") + expect(Permission.evaluate("edit", ".kilo/plans/foo.md", plan!.permission).action).toBe("allow") + expect(Permission.evaluate("edit", ".opencode/plans/foo.md", plan!.permission).action).toBe("allow") + }, + }) +}) + +test("plan agent user config allows cannot re-enable non-plan edits", async () => { + await using tmp = await tmpdir({ + config: { + permission: { + edit: { "src/output.log": "allow" }, + }, + }, + }) + await Instance.provide({ + directory: tmp.path, + fn: async () => { + const plan = await load(tmp.path, (svc) => svc.get("plan")) + expect(plan).toBeDefined() + expect(Permission.evaluate("edit", "src/output.log", plan!.permission).action).toBe("deny") expect(Permission.evaluate("edit", ".kilo/plans/foo.md", plan!.permission).action).toBe("allow") - // kilocode_change end - // .opencode/plans/ is also allowed as backward compat fallback expect(Permission.evaluate("edit", ".opencode/plans/foo.md", plan!.permission).action).toBe("allow") }, }) diff --git a/packages/opencode/test/kilocode/ask-agent-permissions.test.ts b/packages/opencode/test/kilocode/ask-agent-permissions.test.ts index 482235cec5d..241c15301ae 100644 --- a/packages/opencode/test/kilocode/ask-agent-permissions.test.ts +++ b/packages/opencode/test/kilocode/ask-agent-permissions.test.ts @@ -1,65 +1,6 @@ import { test, expect, describe } from "bun:test" import { Permission } from "../../src/permission" - -// Reconstruct the Ask agent's readOnlyBash allowlist (mirrors kilocode/agent/index.ts) -// Uses an allow-list approach for git: deny by default, allow specific read-only subcommands. -const readOnlyBash: Record = { - "*": "deny", - // read-only / informational - "cat *": "allow", - "head *": "allow", - "tail *": "allow", - "less *": "allow", - "ls *": "allow", - "tree *": "allow", - "pwd *": "allow", - "echo *": "allow", - "wc *": "allow", - "which *": "allow", - "type *": "allow", - "file *": "allow", - "diff *": "allow", - "du *": "allow", - "df *": "allow", - "date *": "allow", - "uname *": "allow", - "whoami *": "allow", - "printenv *": "allow", - "man *": "allow", - // text processing (stdout only, no file modification) - "grep *": "allow", - "rg *": "allow", - "ag *": "allow", - "sort *": "allow", - "uniq *": "allow", - "cut *": "allow", - "tr *": "allow", - "jq *": "allow", - // git — allowlist of read-only subcommands, deny everything else - "git *": "deny", - "git log *": "allow", - "git show *": "allow", - "git diff *": "allow", - "git status *": "allow", - "git blame *": "allow", - "git rev-parse *": "allow", - "git rev-list *": "allow", - "git ls-files *": "allow", - "git ls-tree *": "allow", - "git ls-remote *": "allow", - "git shortlog *": "allow", - "git describe *": "allow", - "git cat-file *": "allow", - "git name-rev *": "allow", - "git stash list *": "allow", - "git tag -l *": "allow", - "git branch --list *": "allow", - "git branch -a *": "allow", - "git branch -r *": "allow", - "git remote -v *": "allow", - // gh — require user approval since commands vary widely - "gh *": "ask", -} +import { readOnlyBash } from "../../src/kilocode/agent/permissions" /** Build the Ask agent ruleset without MCP servers */ function askRuleset() { @@ -166,6 +107,23 @@ describe("Ask agent bash permissions", () => { } }) + describe("denied output redirection and writer flags", () => { + const denied = [ + "echo hi > file", + "echo hi >> file", + "cat a > b", + "jq . a.json > b.json", + "sort names.txt -o names.txt", + ] + + for (const cmd of denied) { + test(`"${cmd}" → deny`, () => { + const result = Permission.evaluate("bash", cmd, ruleset) + expect(result.action).toBe("deny") + }) + } + }) + describe("denied git write commands", () => { const denied = [ "git commit -m 'test'", diff --git a/packages/opencode/test/session/prompt-effect.test.ts b/packages/opencode/test/session/prompt-effect.test.ts index 4b3777f32bc..304d4f0baff 100644 --- a/packages/opencode/test/session/prompt-effect.test.ts +++ b/packages/opencode/test/session/prompt-effect.test.ts @@ -105,6 +105,31 @@ function errorTool(parts: MessageV2.Part[]) { return part?.state.status === "error" ? (part as ErrorToolPart) : undefined } +function names(input: Record | undefined) { + const tools = input?.tools + if (!Array.isArray(tools)) return [] + return tools.flatMap((item) => { + if (!item || typeof item !== "object") return [] + const obj = item as Record + const fn = obj.function + if (fn && typeof fn === "object") { + const name = (fn as Record).name + if (typeof name === "string") return [name] + } + if (typeof obj.name === "string") return [obj.name] + return [] + }) +} + +const waitQuestion = Effect.fn("test.waitQuestion")(function* (sessionID: SessionID) { + for (let i = 0; i < 100; i++) { + const list = yield* Effect.promise(() => Question.list()) + const item = list.find((q) => q.sessionID === sessionID) + if (item) return item + yield* Effect.sleep("10 millis") + } +}) + const mcp = Layer.succeed( MCP.Service, MCP.Service.of({ @@ -543,6 +568,117 @@ it.live("loop continues when finish is stop but assistant has tool parts", () => ), ) +unix("ask agent denies bash redirection during prompt loop", () => + provideTmpdirServer( + ({ dir, llm }) => + withSh(() => + Effect.gen(function* () { + const prompt = yield* SessionPrompt.Service + const sessions = yield* Session.Service + const session = yield* sessions.create({ title: "Ask bash guard" }) + const file = path.join(dir, "ask-bug.txt") + + yield* prompt.prompt({ + sessionID: session.id, + agent: "ask", + noReply: true, + parts: [{ type: "text", text: "Explain the repo" }], + }) + yield* llm.tool("bash", { command: "echo bug > ask-bug.txt", description: "write ask bug" }) + yield* llm.text("done") + + yield* prompt.loop({ sessionID: session.id }) + expect(yield* Effect.promise(() => Bun.file(file).exists())).toBe(false) + + const msgs = yield* MessageV2.filterCompactedEffect(session.id) + const tool = msgs + .flatMap((msg) => msg.parts) + .find((part): part is MessageV2.ToolPart => part.type === "tool" && part.tool === "bash") + expect(tool?.state.status).toBe("error") + }), + ), + { git: true, config: providerCfg }, + ), +) + +it.live("ask agent hides edit tools despite session allow-all", () => + provideTmpdirServer( + ({ dir, llm }) => + Effect.gen(function* () { + const prompt = yield* SessionPrompt.Service + const sessions = yield* Session.Service + const session = yield* sessions.create({ + title: "Ask session guard", + permission: [{ permission: "*", pattern: "*", action: "allow" }], + }) + const file = path.join(dir, "ask-write-bug.txt") + + yield* prompt.prompt({ + sessionID: session.id, + agent: "ask", + noReply: true, + parts: [{ type: "text", text: "Explain the repo" }], + }) + yield* llm.tool("write", { filePath: file, content: "bug\n" }) + yield* llm.text("done") + + yield* prompt.loop({ sessionID: session.id }) + const inputs = yield* llm.inputs + expect(names(inputs[0])).not.toContain("write") + expect(names(inputs[0])).not.toContain("edit") + expect(yield* Effect.promise(() => Bun.file(file).exists())).toBe(false) + }), + { git: true, config: providerCfg }, + ), +) + +unix("plan follow-up stops before queued mutations after plan_exit", () => + provideTmpdirServer( + ({ dir, llm }) => + withSh(() => + Effect.gen(function* () { + const prompt = yield* SessionPrompt.Service + const sessions = yield* Session.Service + const session = yield* sessions.create({ title: "Plan hard stop" }) + const plan = Session.plan(session) + const file = path.join(dir, "plan-after-exit.txt") + + yield* prompt.prompt({ + sessionID: session.id, + agent: "plan", + noReply: true, + parts: [{ type: "text", text: "Create a plan only" }], + }) + yield* llm.tool("write", { filePath: plan, content: "# Plan\n\n- Stop after planning.\n" }) + yield* llm.tool("plan_exit", {}) + yield* llm.tool("bash", { command: "echo bug > plan-after-exit.txt", description: "write plan bug" }) + + const fiber = yield* prompt.loop({ sessionID: session.id }).pipe(Effect.forkChild) + const question = yield* waitQuestion(session.id) + if (!question) { + yield* Fiber.interrupt(fiber) + expect(question).toBeDefined() + return + } + + const calls = yield* llm.calls + const pending = yield* llm.pending + const mutated = yield* Effect.promise(() => Bun.file(file).exists()) + const planned = yield* Effect.promise(() => Bun.file(plan).exists()) + yield* Effect.promise(() => Question.reject(question.id)) + const exit = yield* Fiber.await(fiber) + + expect(Exit.isSuccess(exit)).toBe(true) + expect(calls).toBe(2) + expect(pending).toBe(1) + expect(mutated).toBe(false) + expect(planned).toBe(true) + }), + ), + { git: true, config: providerCfg }, + ), +) + it.live("failed subtask preserves metadata on error tool state", () => provideTmpdirServer( Effect.fnUntraced(function* ({ llm }) {