mirror of
https://github.com/Kilo-Org/kilocode.git
synced 2026-09-21 05:52:35 +08:00
fix(cli): attribute tool call denials to the rule that actually decided them
DeniedError.ruleset only carried the deny-permission subset, so
PermissionProvenance.classifyDenial had to guess the deciding rule via
findLast(action === "deny"). With two deny rules for different
patterns under the same permission (e.g. bash: { "git push *": deny,
"rm -rf *": deny }), this could attribute a denial to whichever rule
sorted last instead of the one that actually matched the request.
Permission.ask now embeds the exact rule resolve()/evaluate() matched
against the request's pattern directly on the error (ruleset: { rule,
matches }), so classifyDenial reads it instead of re-deriving it.
Some denials carry no rule at all (e.g. the headless-subagent policy
denial), where classify({ rule: undefined }) reports the same
{ source: "default" } shape as the *approval* fallback -- silently
rendering a refusal as an auto-approval in the TUI and kilo export.
classifyDenial now synthesizes an explicit deny rule for the request's
permission/pattern in that case, so rule.action always reflects the
real outcome.
Adds test/kilocode/permission/deny-provenance.test.ts covering both
regressions against the real Permission.Service, and updates the
existing session-tools.test.ts denial fixture to the new ruleset
shape.
This commit is contained in:
@@ -0,0 +1,91 @@
|
||||
import { describe, expect, test } from "bun:test"
|
||||
import { Cause, Effect, Exit, Layer } from "effect"
|
||||
import { Bus } from "../../../src/bus"
|
||||
import { Permission } from "../../../src/permission"
|
||||
import { PermissionProvenance } from "../../../src/kilocode/permission/provenance"
|
||||
import { EventV2Bridge } from "../../../src/event-v2-bridge"
|
||||
import { Database } from "@opencode-ai/core/database/database"
|
||||
import { SessionID } from "../../../src/session/schema"
|
||||
import * as Config from "../../../src/config/config"
|
||||
import * as CrossSpawnSpawner from "@opencode-ai/core/cross-spawn-spawner"
|
||||
import { provideTmpdirInstance } from "../../fixture/fixture"
|
||||
import { testEffect } from "../../lib/effect"
|
||||
|
||||
const env = Layer.mergeAll(
|
||||
Permission.layer.pipe(
|
||||
Layer.provide(EventV2Bridge.defaultLayer),
|
||||
Layer.provide(Config.defaultLayer),
|
||||
Layer.provide(Database.defaultLayer),
|
||||
),
|
||||
Config.defaultLayer,
|
||||
Bus.layer,
|
||||
CrossSpawnSpawner.defaultLayer,
|
||||
)
|
||||
const it = testEffect(env)
|
||||
|
||||
const ask = (input: Parameters<Permission.Interface["ask"]>[0]) =>
|
||||
Effect.gen(function* () {
|
||||
const permission = yield* Permission.Service
|
||||
return yield* permission.ask(input)
|
||||
})
|
||||
|
||||
function withDir(options: { git?: boolean } | undefined, self: (dir: string) => Effect.Effect<any, any, any>) {
|
||||
return provideTmpdirInstance(self, options)
|
||||
}
|
||||
|
||||
describe("Permission.ask denial provenance", () => {
|
||||
it.live(
|
||||
"attributes a denial to the rule that matched the request's pattern, not just the textually-last deny rule for the permission",
|
||||
() =>
|
||||
withDir({ git: true }, () =>
|
||||
Effect.gen(function* () {
|
||||
// Two deny rules under the same permission for different patterns. Matching by
|
||||
// permission alone (e.g. findLast over rules with action "deny") would pick
|
||||
// "rm -rf *" here since it sorts last, even though "git push *" is the one that
|
||||
// actually matched the request.
|
||||
const ruleset = [
|
||||
{ permission: "bash", pattern: "git push *", action: "deny" as const },
|
||||
{ permission: "bash", pattern: "rm -rf *", action: "deny" as const },
|
||||
]
|
||||
const exit = yield* ask({
|
||||
sessionID: SessionID.make("session_test"),
|
||||
permission: "bash",
|
||||
patterns: ["git push origin main"],
|
||||
metadata: {},
|
||||
always: [],
|
||||
ruleset,
|
||||
}).pipe(Effect.exit)
|
||||
|
||||
expect(Exit.isFailure(exit)).toBe(true)
|
||||
const err = Exit.isFailure(exit) ? Cause.squash(exit.cause) : undefined
|
||||
expect(err).toBeInstanceOf(Permission.DeniedError)
|
||||
|
||||
const approval = PermissionProvenance.classifyDenial({
|
||||
ruleset: (err as Permission.DeniedError).ruleset,
|
||||
permission: "bash",
|
||||
patterns: ["git push origin main"],
|
||||
agent: "build",
|
||||
origins: undefined,
|
||||
})
|
||||
expect(approval.rule).toEqual({ permission: "bash", pattern: "git push *", action: "deny" })
|
||||
}),
|
||||
),
|
||||
)
|
||||
|
||||
test("a denial with no rule in the carried ruleset is still reported as denied, not as an ambiguous default approval", () => {
|
||||
// Some denials carry no `rule` at all (e.g. the headless-subagent policy denial in
|
||||
// Permission.ask, which isn't decided by any ruleset match). classify({ rule: undefined })
|
||||
// reports { source: "default" } — the same shape the *approval* fallback produces for "no
|
||||
// rule matched" — so without a synthesized deny rule, a refusal would render (and export)
|
||||
// as an auto-approval.
|
||||
const approval = PermissionProvenance.classifyDenial({
|
||||
ruleset: { matches: [] },
|
||||
permission: "bash",
|
||||
patterns: ["rm -rf /"],
|
||||
agent: "build",
|
||||
origins: undefined,
|
||||
})
|
||||
expect(approval.rule?.action).toBe("deny")
|
||||
expect(approval.rule).toEqual({ permission: "bash", pattern: "rm -rf /", action: "deny" })
|
||||
})
|
||||
})
|
||||
@@ -361,11 +361,11 @@ it.live("records why a denied tool call was refused on the tool part's metadata"
|
||||
Effect.gen(function* () {
|
||||
const dirs = yield* fixture()
|
||||
const metadataCalls: { toolCallID: string; value: Record<string, any> }[] = []
|
||||
const deniedRuleset = [{ permission: "bash", pattern: "*", action: "deny" as const, source: "project" as const }]
|
||||
const deniedRule = { permission: "bash", pattern: "*", action: "deny" as const, source: "project" as const }
|
||||
const overrides = Layer.mergeAll(
|
||||
TestConfig.layer({ get: () => Effect.succeed({ sandbox: { enabled: false } }) }),
|
||||
Layer.mock(Permission.Service)({
|
||||
ask: () => Effect.fail(new Permission.DeniedError({ ruleset: deniedRuleset })),
|
||||
ask: () => Effect.fail(new Permission.DeniedError({ ruleset: { rule: deniedRule, matches: [deniedRule] } })),
|
||||
}),
|
||||
)
|
||||
const tools = yield* resolve(dirs.ctx, metadataCalls).pipe(Effect.provide(overrides))
|
||||
|
||||
Reference in New Issue
Block a user