From 217bc9794b79368d6255750d56fd45a35bacbc61 Mon Sep 17 00:00:00 2001 From: marius-kilocode Date: Tue, 14 Jul 2026 08:19:20 +0200 Subject: [PATCH] fix(cli): close file permission review gaps --- .../opencode/src/kilocode/session/prompt.ts | 48 ++-- .../opencode/src/kilocode/tool/read-object.ts | 104 +++---- .../instance/httpapi/handlers/session.ts | 45 ++- packages/opencode/src/session/prompt.ts | 139 +++++----- packages/opencode/src/tool/read.ts | 90 +++--- .../session-prompt-permission-refresh.test.ts | 257 ++++++++++++++++-- packages/opencode/test/session/prompt.test.ts | 10 +- 7 files changed, 449 insertions(+), 244 deletions(-) diff --git a/packages/opencode/src/kilocode/session/prompt.ts b/packages/opencode/src/kilocode/session/prompt.ts index 360a14031c..e7a27acd97 100644 --- a/packages/opencode/src/kilocode/session/prompt.ts +++ b/packages/opencode/src/kilocode/session/prompt.ts @@ -2,7 +2,7 @@ import path from "path" import fs from "fs/promises" import { StringDecoder } from "string_decoder" -import { Cause, Effect, Exit, Fiber, Latch, Scope } from "effect" +import { Cause, Effect, Exit, Fiber, Scope } from "effect" import { SessionID, PartID } from "@/session/schema" import { MessageV2 } from "@/session/message-v2" import { Session } from "@/session/session" @@ -16,10 +16,9 @@ import { KiloSession } from "@/kilocode/session" import { KiloSessionMessageOrder } from "@/kilocode/session/message-order" import { Permission } from "@/permission" import { Question } from "@/question" -import { environmentDetails, type EditorContext } from "@/kilocode/editor-context" +import { environmentDetails } from "@/kilocode/editor-context" import { Identifier } from "@/id/id" import { Filesystem } from "@/util/filesystem" -import { InstanceState } from "@/effect/instance-state" import NATIVE_PLAN_PROMPT from "@/kilocode/session/native-plan-prompt.txt" import { MemoryPaths } from "@kilocode/kilo-memory/effect/paths" import { MemoryMarker } from "@/kilocode/memory/marker" @@ -29,34 +28,33 @@ import CODE_SWITCH from "@/session/prompt/code-switch.txt" export namespace KiloSessionPrompt { const modes = ["ask", "plan", "architect"] - type Intake = { cancelled: boolean; fiber?: Fiber.Fiber } + type Intake = { cancelled: boolean; fiber?: Fiber.Fiber } const intakes = new Map>() - export const startAsyncPrompt = Effect.fn("KiloSessionPrompt.startAsyncPrompt")(function* (input: { - sessionID: SessionID - scope: Scope.Scope - work: Effect.Effect - }) { - const ready = yield* Latch.make() - const entry: Intake = { cancelled: false } - const work = ready.whenOpen(input.work).pipe( - Effect.ensuring( - Effect.sync(() => { - const entries = intakes.get(input.sessionID) - entries?.delete(entry) - if (entries?.size === 0) intakes.delete(input.sessionID) + export function intake(sessionID: SessionID, work: Effect.Effect) { + return Effect.scoped( + Effect.uninterruptibleMask((restore) => + Effect.gen(function* () { + const scope = yield* Scope.Scope + const entry: Intake = { cancelled: false } + const cleanup = Effect.sync(() => { + const entries = intakes.get(sessionID) + entries?.delete(entry) + if (entries?.size === 0) intakes.delete(sessionID) + }) + const entries = intakes.get(sessionID) ?? new Set() + entries.add(entry) + intakes.set(sessionID, entries) + const fiber = yield* work.pipe(Effect.ensuring(cleanup), Effect.forkIn(scope, { startImmediately: true })) + entry.fiber = fiber + if (entry.cancelled) yield* Fiber.interrupt(fiber) + return yield* restore(Fiber.join(fiber)) }), ), ) - const entries = intakes.get(input.sessionID) ?? new Set() - entries.add(entry) - intakes.set(input.sessionID, entries) - const fiber = yield* work.pipe(Effect.forkIn(input.scope, { startImmediately: true })) - entry.fiber = fiber - yield* (entry.cancelled ? Fiber.interrupt(fiber) : ready.open).pipe(Effect.uninterruptible) - }, Effect.uninterruptible) + } - export const abortAsyncPrompts = Effect.fn("KiloSessionPrompt.abortAsyncPrompts")(function* (sessionID: SessionID) { + export const abortIntakes = Effect.fn("KiloSessionPrompt.abortIntakes")(function* (sessionID: SessionID) { const entries = [...(intakes.get(sessionID) ?? [])] yield* Effect.forEach( entries, diff --git a/packages/opencode/src/kilocode/tool/read-object.ts b/packages/opencode/src/kilocode/tool/read-object.ts index ca0b2ebf70..e16bfa6f05 100644 --- a/packages/opencode/src/kilocode/tool/read-object.ts +++ b/packages/opencode/src/kilocode/tool/read-object.ts @@ -1,5 +1,5 @@ -import { open, readdir, realpath, stat, type FileHandle } from "node:fs/promises" -import { type BigIntStats } from "node:fs" +import { constants, type BigIntStats } from "node:fs" +import { open, realpath, stat, type FileHandle } from "node:fs/promises" import { Readable } from "node:stream" import { Effect } from "effect" import { FSUtil } from "@opencode-ai/core/fs-util" @@ -7,22 +7,47 @@ import { FSUtil } from "@opencode-ai/core/fs-util" export namespace KiloReadObject { export class ChangedError extends Error {} - export type File = { + export type FileInfo = { requested: string target: string - handle: FileHandle stat: BigIntStats + } + + export type File = FileInfo & { + handle: FileHandle read: (limit?: number, signal?: AbortSignal) => Promise sample: (limit: number, signal?: AbortSignal) => Promise stream: (signal?: AbortSignal) => Readable } - export type Directory = { - target: string - items: string[] + const failure = (err: unknown) => (err instanceof Error ? err : new Error(String(err))) + const same = (left: BigIntStats, right: BigIntStats) => left.dev === right.dev && left.ino === right.ino + const normalize = (input: string) => (process.platform === "win32" ? FSUtil.normalizePath(input) : input) + + export function namedPipe(input: string) { + return ( + /^\\\\[.?]\\pipe\\/i.test(input) || + /^\\\\[^\\]+\\pipe\\/i.test(input) || + /^\\\\\?\\GLOBALROOT\\Device\\NamedPipe\\/i.test(input) + ) } - const failure = (err: unknown) => (err instanceof Error ? err : new Error(String(err))) + async function inspect(requested: string) { + if (process.platform === "win32" && namedPipe(requested)) { + throw new ChangedError(`Named pipes cannot be read: ${requested}`) + } + const opened = await stat(requested, { bigint: true }) + const resolved = await realpath(requested) + const seen = await stat(resolved, { bigint: true }) + if (!same(opened, seen)) throw new ChangedError(`Path changed while inspecting: ${requested}`) + return { requested, target: normalize(resolved), stat: opened } + } + + export const file = Effect.fn("KiloReadObject.file")(function* (requested: string) { + const info = yield* Effect.tryPromise({ try: () => inspect(requested), catch: failure }) + if (!info.stat.isFile()) return yield* Effect.fail(new ChangedError(`Not a regular file: ${requested}`)) + return info satisfies FileInfo + }) async function bytes(handle: FileHandle, limit?: number, signal?: AbortSignal) { const chunks: Buffer[] = [] @@ -53,9 +78,13 @@ export namespace KiloReadObject { } } - export function use(requested: string, fn: (file: File) => Effect.Effect) { + export function use(info: FileInfo, fn: (file: File) => Effect.Effect) { + const flags = + process.platform === "win32" + ? constants.O_RDONLY + : constants.O_RDONLY | constants.O_NONBLOCK | constants.O_NOFOLLOW const acquire = Effect.tryPromise({ - try: () => open(requested, "r"), + try: () => open(info.target, flags), catch: failure, }) return Effect.acquireUseRelease( @@ -66,59 +95,34 @@ export namespace KiloReadObject { try: () => handle.stat({ bigint: true }), catch: failure, }) - const probe = process.platform === "linux" ? `/proc/self/fd/${handle.fd}` : requested - const resolved = yield* Effect.tryPromise({ - try: () => realpath(probe), - catch: failure, - }) + if (!opened.isFile() || !same(info.stat, opened)) { + return yield* Effect.fail(new ChangedError(`File changed after authorization: ${info.requested}`)) + } + const probe = process.platform === "linux" ? `/proc/self/fd/${handle.fd}` : info.target + const resolved = yield* Effect.tryPromise({ try: () => realpath(probe), catch: failure }) const seen = yield* Effect.tryPromise({ try: () => stat(resolved, { bigint: true }), catch: failure, }) - if (opened.dev !== seen.dev || opened.ino !== seen.ino) { - return yield* Effect.fail(new ChangedError(`File changed while opening: ${requested}`)) + if (!same(opened, seen) || normalize(resolved) !== info.target) { + return yield* Effect.fail(new ChangedError(`File changed after authorization: ${info.requested}`)) } - const target = process.platform === "win32" ? FSUtil.normalizePath(resolved) : resolved return yield* fn({ - requested, - target, + ...info, handle, - stat: opened, read: (limit, signal) => bytes(handle, limit, signal), sample: (limit, signal) => bytes(handle, limit, signal), stream: (signal) => Readable.from(chunks(handle, signal)), }) }), - (handle) => Effect.promise(() => handle.close()).pipe(Effect.catch(() => Effect.void)), + (handle) => + Effect.tryPromise({ + try: async () => { + await handle.close() + }, + catch: failure, + }).pipe(Effect.catch(() => Effect.void)), ) } - export const directory = Effect.fn("KiloReadObject.directory")(function* (requested: string) { - return yield* Effect.tryPromise({ - try: async () => { - const opened = await stat(requested, { bigint: true }) - if (!opened.isDirectory()) throw new ChangedError(`Not a directory: ${requested}`) - const resolved = await realpath(requested) - const target = process.platform === "win32" ? FSUtil.normalizePath(resolved) : resolved - const seen = await stat(resolved, { bigint: true }) - if (opened.dev !== seen.dev || opened.ino !== seen.ino) { - throw new ChangedError(`Directory changed while opening: ${requested}`) - } - const entries = await readdir(resolved, { withFileTypes: true }) - const after = await stat(resolved, { bigint: true }) - const current = await realpath(requested) - const canonical = process.platform === "win32" ? FSUtil.normalizePath(current) : current - if (opened.dev !== after.dev || opened.ino !== after.ino || canonical !== target) { - throw new ChangedError(`Directory changed while reading: ${requested}`) - } - return { - target, - items: entries - .map((entry) => (entry.isDirectory() ? `${entry.name}/` : entry.name)) - .sort((a, b) => a.localeCompare(b)), - } satisfies Directory - }, - catch: failure, - }) - }) } diff --git a/packages/opencode/src/server/routes/instance/httpapi/handlers/session.ts b/packages/opencode/src/server/routes/instance/httpapi/handlers/session.ts index 16988ce8ae..c977cfcdbf 100644 --- a/packages/opencode/src/server/routes/instance/httpapi/handlers/session.ts +++ b/packages/opencode/src/server/routes/instance/httpapi/handlers/session.ts @@ -1,6 +1,5 @@ import { Image } from "@/image/image" // kilocode_change - classify user image validation defects import { KiloSessionHttpApi } from "@/kilocode/server/httpapi/session-fork" // kilocode_change -import { KiloSessionPrompt } from "@/kilocode/session/prompt" // kilocode_change import { BlockedError as AgentRequirementError } from "@/kilocode/agent-requirements" // kilocode_change import { PermissionV1 } from "@opencode-ai/core/v1/permission" import { Agent } from "@/agent/agent" @@ -314,32 +313,26 @@ export const sessionHandlers = HttpApiBuilder.group(InstanceHttpApi, "session", payload: typeof PromptPayload.Type }) { yield* requireSession(ctx.params.sessionID) - // kilocode_change start - keep async attachment permission waits cancellable - yield* KiloSessionPrompt.startAsyncPrompt({ - sessionID: ctx.params.sessionID, - scope, - work: promptSvc - .prompt({ ...ctx.payload, sessionID: ctx.params.sessionID } as unknown as SessionPrompt.PromptInput) - .pipe( - Effect.asVoid, - Effect.catchCause((cause) => { - if (Cause.hasInterruptsOnly(cause)) return Effect.void - return Effect.gen(function* () { - yield* Effect.logError("prompt_async failed").pipe( - Effect.annotateLogs({ sessionID: ctx.params.sessionID, cause }), - ) - const error = Cause.squash(cause) - yield* events.publish(Session.Event.Error, { - sessionID: ctx.params.sessionID, - error: AgentRequirementError.isInstance(error) - ? error.toObject() - : new NamedError.Unknown({ message: Cause.pretty(cause) }).toObject(), - }) + yield* promptSvc + .prompt({ ...ctx.payload, sessionID: ctx.params.sessionID } as unknown as SessionPrompt.PromptInput) + .pipe( + Effect.catchCause((cause) => { + if (Cause.hasInterruptsOnly(cause)) return Effect.void // kilocode_change - Stop is not an error + return Effect.gen(function* () { + yield* Effect.logError("prompt_async failed").pipe( + Effect.annotateLogs({ sessionID: ctx.params.sessionID, cause }), + ) + const error = Cause.squash(cause) + yield* events.publish(Session.Event.Error, { + sessionID: ctx.params.sessionID, + error: AgentRequirementError.isInstance(error) + ? error.toObject() + : new NamedError.Unknown({ message: Cause.pretty(cause) }).toObject(), }) - }), - ), - }) - // kilocode_change end + }) + }), + Effect.forkIn(scope, { startImmediately: true }), + ) return HttpApiSchema.NoContent.make() }) diff --git a/packages/opencode/src/session/prompt.ts b/packages/opencode/src/session/prompt.ts index 512a22adce..c90e3cd9d4 100644 --- a/packages/opencode/src/session/prompt.ts +++ b/packages/opencode/src/session/prompt.ts @@ -180,7 +180,7 @@ export const layer = Layer.effect( yield* elog.info("cancel", { sessionID }) yield* KiloSessionPromptQueue.cancel(sessionID) // kilocode_change - drop queued follow-up loops on abort KiloSessionPrompt.abortPlanFollowup(sessionID) // kilocode_change - abort pending plan-followup handover work - yield* KiloSessionPrompt.abortAsyncPrompts(sessionID) // kilocode_change - interrupt attachment permission waits + yield* KiloSessionPrompt.abortIntakes(sessionID) // kilocode_change - interrupt attachment permission waits yield* state.cancel(sessionID) }) @@ -996,7 +996,7 @@ export const layer = Layer.effect( abort: controller.signal, agent: ag.name, messageID: info.id, - extra: { ...extra, referenceRoot: reference?.root, includeInstructions: false }, + extra: { ...extra, referenceRoot: reference?.root, includeInstructions: false, denyDirectory: true }, messages: [], metadata: () => Effect.void, ask, @@ -1136,63 +1136,63 @@ export const layer = Layer.effect( ] } - // kilocode_change start - authorize and consume direct attachments through one open object - const access = yield* KiloReadObject.use(filepath, (bound) => - Effect.gen(function* () { - if (!bound.stat.isFile()) return yield* Effect.fail(new Error(`Cannot read non-file: ${filepath}`)) - const instance = yield* InstanceState.context - const context = ctx() - const explicit = reference ? yield* KiloReference.path(fsys, reference.root, bound.target) : false - const referenced = - explicit || - ((yield* references.contains(filepath)) && - (yield* KiloReference.contains({ fs: fsys, references, target: bound.target }))) - yield* assertExternalDirectoryEffect(context, bound.target, { bypass: referenced, kind: "file" }) - yield* context.ask({ - permission: "read", - patterns: [ - ...new Set([filepath, bound.target].map((item) => path.relative(instance.worktree, item))), - ], - always: ["*"], - metadata: {}, - }) + // kilocode_change start - authorize metadata, then reopen and verify before consuming bytes + const access = yield* Effect.gen(function* () { + const file = yield* KiloReadObject.file(filepath) + const instance = yield* InstanceState.context + const context = ctx() + const explicit = reference ? yield* KiloReference.path(fsys, reference.root, file.target) : false + const referenced = + explicit || + ((yield* references.contains(filepath)) && + (yield* KiloReference.contains({ fs: fsys, references, target: file.target }))) + yield* assertExternalDirectoryEffect(context, file.target, { bypass: referenced, kind: "file" }) + yield* context.ask({ + permission: "read", + patterns: [...new Set([filepath, file.target].map((item) => path.relative(instance.worktree, item)))], + always: ["*"], + metadata: {}, + }) - const limit = mime.startsWith("image/") - ? ((yield* config.get()).attachment?.image?.max_base64_bytes ?? Image.MAX_BASE64_BYTES) - : undefined - const raw = limit === undefined ? undefined : Math.floor(limit / 4) * 3 + 1 - const bytes = yield* Effect.tryPromise({ - try: (signal) => bound.read(raw, AbortSignal.any([context.abort, signal])), - catch: (err) => (err instanceof Error ? err : new Error(String(err))), - }) - if (limit !== undefined) { - const encoded = Math.ceil(bytes.byteLength / 3) * 4 - if (encoded > limit) { - return yield* Effect.fail( - new Image.SizeError({ - bytes: encoded, - max: limit, - width: 0, - height: 0, - max_width: 0, - max_height: 0, - }), - ) + return yield* KiloReadObject.use(file, (bound) => + Effect.gen(function* () { + const limit = mime.startsWith("image/") + ? ((yield* config.get()).attachment?.image?.max_base64_bytes ?? Image.MAX_BASE64_BYTES) + : undefined + const raw = limit === undefined ? undefined : Math.floor(limit / 4) * 3 + 1 + const bytes = yield* Effect.tryPromise({ + try: (signal) => bound.read(raw, AbortSignal.any([context.abort, signal])), + catch: (err) => (err instanceof Error ? err : new Error(String(err))), + }) + if (limit !== undefined) { + const encoded = Math.ceil(bytes.byteLength / 3) * 4 + if (encoded > limit) { + return yield* Effect.fail( + new Image.SizeError({ + bytes: encoded, + max: limit, + width: 0, + height: 0, + max_width: 0, + max_height: 0, + }), + ) + } } - } - const file: MessageV2.FilePart = { - id: part.id ? PartID.make(part.id) : PartID.ascending(), - messageID: info.id, - sessionID: input.sessionID, - type: "file", - url: `data:${mime};base64,${bytes.toString("base64")}`, - mime, - filename: part.filename!, - source: part.source, - } - return mime.startsWith("image/") ? yield* image.normalize(file) : file - }), - ).pipe(Effect.exit) + const file: MessageV2.FilePart = { + id: part.id ? PartID.make(part.id) : PartID.ascending(), + messageID: info.id, + sessionID: input.sessionID, + type: "file", + url: `data:${mime};base64,${bytes.toString("base64")}`, + mime, + filename: part.filename!, + source: part.source, + } + return mime.startsWith("image/") ? yield* image.normalize(file) : file + }), + ) + }).pipe(Effect.exit) if (Exit.isFailure(access)) { const error = Cause.squash(access.cause) if ( @@ -1222,9 +1222,7 @@ export const layer = Layer.effect( } // kilocode_change end return [ - ...(referenceContext - ? [{ ...referenceContext, messageID: info.id, sessionID: input.sessionID }] - : []), + ...(referenceContext ? [{ ...referenceContext, messageID: info.id, sessionID: input.sessionID }] : []), { messageID: info.id, sessionID: input.sessionID, @@ -1423,7 +1421,7 @@ export const layer = Layer.effect( yield* KiloSessionPrompt.recoverDanglingAssistant({ sessionID: input.sessionID, status, sessions }) yield* KiloSessionPrompt.recoverProviderFinishError({ sessionID: input.sessionID, status, sessions }) // kilocode_change end - const message = yield* createUserMessage(input) + const message = yield* KiloSessionPrompt.intake(input.sessionID, createUserMessage(input)) // kilocode_change yield* sessions.touch(input.sessionID) const permissions: PermissionV1.Rule[] = [] @@ -1995,14 +1993,17 @@ export const layer = Layer.effect( }) yield* getModel(model.providerID, model.modelID, input.sessionID) const text = `/${input.command}${input.arguments ? ` ${input.arguments}` : ""}` - const user = yield* createUserMessage({ - sessionID: input.sessionID, - messageID: input.messageID, - model, - agent: agent.name, - variant: input.variant, - parts: [{ type: "text", text }, ...(input.parts ?? [])], - }) + const user = yield* KiloSessionPrompt.intake( + input.sessionID, + createUserMessage({ + sessionID: input.sessionID, + messageID: input.messageID, + model, + agent: agent.name, + variant: input.variant, + parts: [{ type: "text", text }, ...(input.parts ?? [])], + }), + ) yield* sessions.touch(input.sessionID) const ctx = yield* InstanceState.context const completed = Date.now() diff --git a/packages/opencode/src/tool/read.ts b/packages/opencode/src/tool/read.ts index f52aa7d931..d4813556c1 100644 --- a/packages/opencode/src/tool/read.ts +++ b/packages/opencode/src/tool/read.ts @@ -84,26 +84,18 @@ export const ReadTool = Tool.define< const reference = yield* Reference.Service const scope = yield* Scope.Scope - // kilocode_change start - canonicalize missing-file parents before suggestion disclosure - const miss = Effect.fn("ReadTool.miss")(function* (filepath: string, ctx: Tool.Context) { + // kilocode_change start - authorize missing paths without enumerating sibling names + const miss = Effect.fn("ReadTool.miss")(function* (filepath: string, worktree: string, ctx: Tool.Context) { const dir = path.dirname(filepath) - const base = path.basename(filepath) - const parent = yield* KiloReadObject.directory(dir).pipe(Effect.option) + const parent = yield* fs.realPath(dir).pipe(Effect.option) if (parent._tag === "None") return yield* Effect.fail(new Error(`File not found: ${filepath}`)) - yield* assertExternalDirectoryEffect(ctx, parent.value.target, { bypass: false, kind: "directory" }) - const items = parent.value.items - .filter( - (item) => item.toLowerCase().includes(base.toLowerCase()) || base.toLowerCase().includes(item.toLowerCase()), - ) - .map((item) => path.join(parent.value.target, item)) - .slice(0, 3) - - if (items.length > 0) { - return yield* Effect.fail( - new Error(`File not found: ${filepath}\n\nDid you mean one of these?\n${items.join("\n")}`), - ) - } - + yield* assertExternalDirectoryEffect(ctx, parent.value, { bypass: false, kind: "directory" }) + yield* ctx.ask({ + permission: "read", + patterns: [...new Set([filepath, parent.value].map((item) => path.relative(worktree, item)))], + always: ["*"], + metadata: {}, + }) return yield* Effect.fail(new Error(`File not found: ${filepath}`)) }) // kilocode_change end @@ -113,6 +105,22 @@ export const ReadTool = Tool.define< yield* lsp.touchFile(filepath).pipe(Effect.ignoreCause, Effect.forkIn(scope)) }) + const list = Effect.fn("ReadTool.list")(function* (filepath: string) { + const items = yield* fs.readDirectoryEntries(filepath) + return yield* Effect.forEach( + items, + Effect.fnUntraced(function* (item) { + if (item.type === "directory") return item.name + "/" + if (item.type !== "symlink") return item.name + + const target = yield* fs.stat(path.join(filepath, item.name)).pipe(Effect.catch(() => Effect.void)) + if (target?.type === "Directory") return item.name + "/" + return item.name + }), + { concurrency: "unbounded" }, + ).pipe(Effect.map((items: string[]) => items.sort((a, b) => a.localeCompare(b)))) + }) + // kilocode_change start - extracted formats and text consume the authorized open object const lines = Effect.fn("ReadTool.lines")( (file: KiloReadObject.File, opts: { limit: number; offset: number }, abort: AbortSignal) => @@ -210,14 +218,14 @@ export const ReadTool = Tool.define< ), ) if (!info) { - return yield* miss(requested, ctx) + return yield* miss(requested, instance.worktree, ctx) } // kilocode_change end // kilocode_change start - directory mentions expose only a bound listing, never child file bodies if (info.type === "Directory") { - const directory = yield* KiloReadObject.directory(requested) - const target = directory.target + const resolved = yield* fs.realPath(requested) + const target = process.platform === "win32" ? FSUtil.normalizePath(resolved) : resolved const explicit = typeof ctx.extra?.["referenceRoot"] === "string" && (yield* KiloReference.path(fs, ctx.extra["referenceRoot"], target)) @@ -232,7 +240,10 @@ export const ReadTool = Tool.define< always: ["*"], metadata: {}, }) - const items = directory.items + if (ctx.extra?.["denyDirectory"] === true) { + return yield* Effect.fail(new Error(`Directory attachments cannot be expanded: ${requested}`)) + } + const items = yield* list(target) const limit = Math.max(1, params.limit ?? DEFAULT_READ_LIMIT) // kilocode_change - prevent zero-limit loops const offset = params.offset || 1 const start = offset - 1 @@ -266,25 +277,24 @@ export const ReadTool = Tool.define< }, } } - // kilocode_change start - hold one object open across authorization and every content read - return yield* KiloReadObject.use(requested, (bound) => + // kilocode_change start - authorize metadata, then bind every content read to the same reopened object + const file = yield* KiloReadObject.file(requested) + const explicit = + typeof ctx.extra?.["referenceRoot"] === "string" && + (yield* KiloReference.path(fs, ctx.extra["referenceRoot"], file.target)) + const referenced = + explicit || + ((yield* reference.contains(requested)) && + (yield* KiloReference.contains({ fs, references: reference, target: file.target }))) + yield* assertExternalDirectoryEffect(ctx, file.target, { bypass: referenced, kind: "file" }) + yield* ctx.ask({ + permission: "read", + patterns: [...new Set([requested, file.target].map((item) => path.relative(instance.worktree, item)))], + always: ["*"], + metadata: {}, + }) + return yield* KiloReadObject.use(file, (bound) => Effect.gen(function* () { - if (!bound.stat.isFile()) return yield* Effect.fail(new Error(`Cannot read non-file: ${requested}`)) - const explicit = - typeof ctx.extra?.["referenceRoot"] === "string" && - (yield* KiloReference.path(fs, ctx.extra["referenceRoot"], bound.target)) - const referenced = - explicit || - ((yield* reference.contains(requested)) && - (yield* KiloReference.contains({ fs, references: reference, target: bound.target }))) - yield* assertExternalDirectoryEffect(ctx, bound.target, { bypass: referenced, kind: "file" }) - yield* ctx.ask({ - permission: "read", - patterns: [...new Set([requested, bound.target].map((item) => path.relative(instance.worktree, item)))], - always: ["*"], - metadata: {}, - }) - const loaded = ctx.extra?.["includeInstructions"] === false ? [] diff --git a/packages/opencode/test/kilocode/session-prompt-permission-refresh.test.ts b/packages/opencode/test/kilocode/session-prompt-permission-refresh.test.ts index 39d22d2079..e25f121b13 100644 --- a/packages/opencode/test/kilocode/session-prompt-permission-refresh.test.ts +++ b/packages/opencode/test/kilocode/session-prompt-permission-refresh.test.ts @@ -1,6 +1,6 @@ import { NodeFileSystem } from "@effect/platform-node" import { expect } from "bun:test" -import { Effect, Exit, Fiber, Layer, Scope } from "effect" +import { Cause, Effect, Exit, Fiber, Layer } from "effect" import { FetchHttpClient } from "effect/unstable/http" import { rename, rm, symlink } from "fs/promises" import os from "os" @@ -51,6 +51,7 @@ import { ToolRegistry } from "../../src/tool/registry" import { Truncate } from "../../src/tool/truncate" import { KiloHeadless } from "../../src/kilocode/permission/headless" import { KiloSessionPrompt } from "../../src/kilocode/session/prompt" +import { KiloReadObject } from "../../src/kilocode/tool/read-object" import { MemoryService } from "@kilocode/kilo-memory/effect/service" import { provideTmpdirServer } from "../fixture/fixture" import { awaitWithTimeout, pollWithTimeout, testEffect } from "../lib/effect" @@ -209,6 +210,14 @@ function makeHttp() { const it = testEffect(makeHttp()) const symlinkIt = process.platform === "win32" ? it.live.skip : it.live +it.live("recognizes Windows named-pipe paths before filesystem inspection", () => + Effect.sync(() => { + expect(KiloReadObject.namedPipe("\\\\.\\pipe\\secret")).toBe(true) + expect(KiloReadObject.namedPipe("\\\\server\\pipe\\secret")).toBe(true) + expect(KiloReadObject.namedPipe("C:\\project\\secret.txt")).toBe(false) + }), +) + const cfg = { provider: { test: { @@ -290,7 +299,7 @@ it.live( ) it.live( - "asks before adding @file content to the prompt", + "fails closed when an @file path changes while permission is pending", () => provideTmpdirServer( Effect.fnUntraced(function* ({ dir }) { @@ -320,6 +329,7 @@ it.live( return requests.find((request) => request.sessionID === session.id && request.permission === "read") }), "file mention read permission was never requested", + "15 seconds", ) expect(pending.patterns).toEqual(["ask.txt"]) @@ -332,8 +342,9 @@ it.live( .filter((part) => part.type === "text") .map((part) => part.text) .join("\n") - expect(text).toContain(sentinel) + expect(text).not.toContain(sentinel) expect(text).not.toContain(denied) + expect(text).toContain("changed after authorization") } }), { @@ -347,6 +358,57 @@ it.live( 30_000, ) +it.live( + "adds @file content after read permission approval", + () => + provideTmpdirServer( + Effect.fnUntraced(function* ({ dir }) { + const sentinel = "KILO_12133_APPROVED_SENTINEL" + const file = path.join(dir, "approved.txt") + yield* Effect.promise(() => Bun.write(file, sentinel)) + + const prompt = yield* SessionPrompt.Service + const sessions = yield* Session.Service + const permission = yield* Permission.Service + const session = yield* sessions.create({}) + const fiber = yield* prompt + .prompt({ + sessionID: session.id, + noReply: true, + parts: yield* prompt.resolvePromptParts("Read @approved.txt"), + }) + .pipe(Effect.forkScoped) + const pending = yield* pollWithTimeout( + Effect.gen(function* () { + const requests = yield* permission.list() + return requests.find((request) => request.sessionID === session.id && request.permission === "read") + }), + "approved file read permission was never requested", + "15 seconds", + ) + + yield* permission.reply({ requestID: pending.id, reply: "once" }) + const exit = yield* Fiber.await(fiber) + expect(Exit.isSuccess(exit)).toBe(true) + if (Exit.isSuccess(exit)) { + const text = exit.value.parts + .filter((part) => part.type === "text") + .map((part) => part.text) + .join("\n") + expect(text).toContain(sentinel) + } + }), + { + git: true, + config: (url) => ({ + ...providerCfg(url), + permission: { read: { "*": "allow", "approved.txt": "ask" } }, + }), + }, + ), + 30_000, +) + it.live( "stops a prompt while an attachment read permission is pending", () => @@ -359,24 +421,20 @@ it.live( const prompt = yield* SessionPrompt.Service const sessions = yield* Session.Service const permission = yield* Permission.Service - const scope = yield* Scope.Scope const session = yield* sessions.create({}) - yield* KiloSessionPrompt.startAsyncPrompt({ - sessionID: session.id, - scope, - work: prompt - .prompt({ - sessionID: session.id, - parts: yield* prompt.resolvePromptParts("Read @abort.txt"), - }) - .pipe(Effect.asVoid), - }) + const fiber = yield* prompt + .prompt({ + sessionID: session.id, + parts: yield* prompt.resolvePromptParts("Read @abort.txt"), + }) + .pipe(Effect.forkScoped) yield* pollWithTimeout( Effect.gen(function* () { const requests = yield* permission.list() return requests.find((request) => request.sessionID === session.id && request.permission === "read") }), "attachment read permission was never requested", + "15 seconds", ) yield* prompt.cancel(session.id) @@ -386,11 +444,14 @@ it.live( return requests.some((request) => request.sessionID === session.id) ? undefined : true }), "attachment read permission remained after cancellation", + "15 seconds", ) const messages = yield* sessions.messages({ sessionID: session.id }) expect( messages.flatMap((message) => message.parts).some((part) => "text" in part && part.text.includes(sentinel)), ).toBe(false) + const exit = yield* Fiber.await(fiber) + expect(Exit.isFailure(exit) && Cause.hasInterruptsOnly(exit.cause)).toBe(true) expect(yield* llm.calls).toBe(0) }), { @@ -402,7 +463,61 @@ it.live( ) it.live( - "reads a direct attachment from the object authorized before path replacement", + "stops a legacy command while an attachment read permission is pending", + () => + provideTmpdirServer( + Effect.fnUntraced(function* ({ dir }) { + const sentinel = "KILO_12133_COMMAND_ABORT_SENTINEL" + const file = path.join(dir, "command.txt") + yield* Effect.promise(() => Bun.write(file, sentinel)) + + const prompt = yield* SessionPrompt.Service + const sessions = yield* Session.Service + const permission = yield* Permission.Service + const session = yield* sessions.create({}) + const fiber = yield* prompt + .command({ + sessionID: session.id, + command: "local-review", + arguments: "", + parts: [ + { + type: "file", + mime: "text/plain", + filename: "command.txt", + url: pathToFileURL(file).href, + }, + ], + }) + .pipe(Effect.forkScoped) + yield* pollWithTimeout( + Effect.gen(function* () { + const requests = yield* permission.list() + return requests.find((request) => request.sessionID === session.id && request.permission === "read") + }), + "legacy command attachment permission was never requested", + "15 seconds", + ) + + yield* prompt.cancel(session.id) + const exit = yield* Fiber.await(fiber) + expect(Exit.isFailure(exit) && Cause.hasInterruptsOnly(exit.cause)).toBe(true) + expect((yield* permission.list()).some((request) => request.sessionID === session.id)).toBe(false) + const messages = yield* sessions.messages({ sessionID: session.id }) + expect( + messages.flatMap((message) => message.parts).some((part) => "text" in part && part.text.includes(sentinel)), + ).toBe(false) + }), + { + git: true, + config: (url) => ({ ...providerCfg(url), permission: { read: "ask" } }), + }, + ), + 30_000, +) + +it.live( + "fails closed when a direct attachment path changes while permission is pending", () => provideTmpdirServer( Effect.fnUntraced(function* ({ dir }) { @@ -437,6 +552,7 @@ it.live( return requests.find((request) => request.sessionID === session.id && request.permission === "read") }), "binary attachment read permission was never requested", + "15 seconds", ) yield* Effect.promise(() => rename(replacement, file)) @@ -444,13 +560,14 @@ it.live( const exit = yield* Fiber.await(fiber) expect(Exit.isSuccess(exit)).toBe(true) if (Exit.isSuccess(exit)) { - const attachment = exit.value.parts.find((part) => part.type === "file") - expect(attachment?.type).toBe("file") - if (attachment?.type === "file") { - const content = Buffer.from(attachment.url.split(",")[1] ?? "", "base64").toString() - expect(content).toBe(allowed) - expect(content).not.toContain(denied) - } + const text = exit.value.parts + .filter((part) => part.type === "text") + .map((part) => part.text) + .join("\n") + expect(text).not.toContain(allowed) + expect(text).not.toContain(denied) + expect(text).toContain("changed after authorization") + expect(exit.value.parts.some((part) => part.type === "file")).toBe(false) } }), { @@ -732,7 +849,7 @@ symlinkIt( ) symlinkIt( - "uses the authorized directory snapshot after the path is replaced", + "does not expand a directory attachment after permission approval", () => provideTmpdirServer( Effect.fnUntraced(function* ({ dir }) { @@ -767,6 +884,7 @@ symlinkIt( return requests.find((request) => request.sessionID === session.id && request.permission === "read") }), "directory read permission was never requested", + "15 seconds", ) yield* Effect.promise(async () => { @@ -781,8 +899,9 @@ symlinkIt( .filter((part) => part.type === "text") .map((part) => part.text) .join("\n") - expect(text).toContain("allowed-name.txt") + expect(text).not.toContain("allowed-name.txt") expect(text).not.toContain("secret-name.txt") + expect(text).toContain("Directory attachments cannot be expanded") } }), { @@ -793,6 +912,92 @@ symlinkIt( 30_000, ) +it.live( + "checks read permission without enumerating missing-file suggestions", + () => + provideTmpdirServer( + Effect.fnUntraced(function* ({ dir }) { + const folder = path.join(dir, "private") + const fs = yield* FSUtil.Service + yield* fs.ensureDir(folder) + yield* Effect.promise(() => Bun.write(path.join(folder, "missing-secret-name.txt"), "secret")) + + const prompt = yield* SessionPrompt.Service + const sessions = yield* Session.Service + const session = yield* sessions.create({}) + const missing = path.join(folder, "missing-secret") + const message = yield* prompt.prompt({ + sessionID: session.id, + noReply: true, + parts: [ + { type: "text", text: "Read @private/missing-secret" }, + { + type: "file", + mime: "text/plain", + filename: "private/missing-secret", + url: pathToFileURL(missing).href, + }, + ], + }) + const text = message.parts + .filter((part) => part.type === "text") + .map((part) => part.text) + .join("\n") + + expect(text).not.toContain("missing-secret-name.txt") + expect(text).toContain("prevents you from using this specific tool call") + }), + { + git: true, + config: (url) => ({ + ...providerCfg(url), + permission: { read: { "*": "allow", "private/*": "deny" } }, + }), + }, + ), + 30_000, +) + +symlinkIt( + "rejects a denied FIFO attachment without waiting for a writer", + () => + provideTmpdirServer( + Effect.fnUntraced(function* ({ dir }) { + const fifo = path.join(dir, "secret.pipe") + const child = Bun.spawn(["mkfifo", fifo], { stdout: "ignore", stderr: "pipe", windowsHide: true }) + expect(yield* Effect.promise(() => child.exited)).toBe(0) + + const prompt = yield* SessionPrompt.Service + const sessions = yield* Session.Service + const session = yield* sessions.create({}) + const message = yield* prompt.prompt({ + sessionID: session.id, + noReply: true, + parts: [ + { type: "text", text: "Read @secret.pipe" }, + { + type: "file", + mime: "text/plain", + filename: "secret.pipe", + url: pathToFileURL(fifo).href, + }, + ], + }) + const text = message.parts + .filter((part) => part.type === "text") + .map((part) => part.text) + .join("\n") + + expect(text).toContain("Not a regular file") + }), + { + git: true, + config: (url) => ({ ...providerCfg(url), permission: { read: "deny" } }), + }, + ), + 30_000, +) + it.live("active tool calls use permissions changed after model streaming starts", () => provideTmpdirServer( Effect.fnUntraced(function* ({ dir, llm }) { @@ -884,8 +1089,8 @@ it.live("headless run: subagent permission asks fail instead of waiting forever" expect(err).toBeInstanceOf(Permission.DeniedError) expect(yield* permission.list()).toEqual([]) - expect(yield* KiloHeadless.denies(child.id)).toBe(true) - expect(yield* KiloHeadless.denies(root.id)).toBe(false) + expect(yield* KiloHeadless.denies(child.id)).toBe(true) + expect(yield* KiloHeadless.denies(root.id)).toBe(false) KiloHeadless.clear(root.id) }), diff --git a/packages/opencode/test/session/prompt.test.ts b/packages/opencode/test/session/prompt.test.ts index 7ba79ea29b..b0e825b710 100644 --- a/packages/opencode/test/session/prompt.test.ts +++ b/packages/opencode/test/session/prompt.test.ts @@ -2405,14 +2405,8 @@ noLLMServer.instance( const text = stored.parts.find((part): part is SessionV1.TextPart => part.type === "text" && !part.synthetic) expect(text?.text).toBe("Use @docs for context") - expect(synthetic.some((part) => part.text.includes(JSON.stringify({ filePath: docs })))).toBe(true) - expect(files).toHaveLength(1) - expect(files[0]).toMatchObject({ - filename: "docs", - mime: "application/x-directory", - source: { type: "file", path: "docs", text: { value: "@docs", start: 4, end: 9 } }, - }) - expect(fileURLToPath(files[0].url)).toBe(docs) + expect(synthetic.some((part) => part.text.includes("Directory attachments cannot be expanded"))).toBe(true) // kilocode_change + expect(files).toHaveLength(0) // kilocode_change yield* sessions.remove(session.id) }),