From 723344530be155e5d949565a26b6061a7db42f4f Mon Sep 17 00:00:00 2001 From: Alex Alecu Date: Wed, 15 Apr 2026 15:10:42 +0300 Subject: [PATCH] refactor(cli): move test to kilocode dir --- .kilo/plans/1776243586968-stellar-forest.md | 198 ++++++++++++++++++ .../permission-allow-everything.test.ts | 12 +- 2 files changed, 204 insertions(+), 6 deletions(-) create mode 100644 .kilo/plans/1776243586968-stellar-forest.md rename packages/opencode/test/{ => kilocode}/server/permission-allow-everything.test.ts (92%) diff --git a/.kilo/plans/1776243586968-stellar-forest.md b/.kilo/plans/1776243586968-stellar-forest.md new file mode 100644 index 00000000000..2e72f99fc09 --- /dev/null +++ b/.kilo/plans/1776243586968-stellar-forest.md @@ -0,0 +1,198 @@ +# Fix: Disable Auto-Approve Mode + Permission Prompts + +## Problem Summary + +Two issues after merging upstream OpenCode: + +1. **"Disable auto-approve mode" throws an error** — the TUI command palette shows "Failed to disable auto-approve mode" toast +2. **Previously auto-approved commands now prompt** — users with `"*": "allow"` in global config are getting asked for permissions on tools that used to be auto-allowed + +## Root Cause Analysis + +### Issue 1: `stripNulls` Leaves Empty Objects + +When disabling auto-approve, the route handler at `src/kilocode/permission/routes.ts:55` calls: + +```ts +await Config.updateGlobal({ permission: { "*": { "*": null } } }, { dispose: false }) +``` + +The `null` is a delete sentinel — it's supposed to remove the `"*": { "*": "allow" }` rule from the config. The flow goes through `KilocodeConfig.mergeConfig` → `stripNulls`: + +**JSON path** (`config.ts:1830-1832`): `mergeConfig` promotes the existing scalar `"*": "allow"` to `"*": { "*": "allow" }`, deep-merges with `{ "*": { "*": null } }`, then `stripNulls` removes the null — but leaves an **empty object** `{ "*": {} }` instead of removing the key entirely. + +**JSONC path** (`config.ts:1835-1837`): `patchJsonc` recursively processes the patch. For the innermost key `["permission", "*", "*"]` with value `null`, it correctly calls `modify(input, path, undefined)` to delete. But this leaves `"*": {}` as an empty object in the JSONC text. + +After either path, the config file on disk has `"permission": { "*": {} }`. + +This empty object `{ "*": {} }`: + +- Passes Zod validation (empty `z.record()` is valid) +- Produces zero rules when converted via `Permission.fromConfig` (no entries to iterate) +- Is NOT the same as the key being absent — it's a "present but empty" object + +The error itself most likely comes from `parseConfig(updated, file)` at line 1836 failing when the JSONC content is malformed after `patchJsonc` manipulations, OR from `InstanceState.invalidate(state)` at line 1843 when the config state becomes inconsistent during the non-dispose invalidation path. **A test for the global disable path needs to be written to pinpoint the exact throw.** + +### Issue 2: Permission Prompts After Config Corruption + +After the disable operation writes `{ "*": {} }` to the config, the user's wildcard allow rule is gone. When re-evaluating permissions: + +1. `Config.get()` loads the config with `permission: { "*": {} }` +2. `Permission.fromConfig({ "*": {} })` produces `[]` (empty ruleset — no entries in the empty object) +3. The agent defaults (`agent.ts:97-115`) set `"*": "allow"` as base defaults +4. The user override at `agent.ts:122` (`Permission.fromConfig(cfg.permission ?? {})`) produces an empty array +5. Since `evaluate` uses `findLast` on the merged rulesets, the user's empty override doesn't add any rules, and the base defaults apply +6. But tools like `bash` were changed to default to `"ask"` in newer upstream (addressed by `migrateBashPermission`), and any tool permission not covered by the base defaults would fall through to `"ask"` + +The `{ "*": {} }` in the config is semantically different from `{ "*": "allow" }` or the key being absent. It adds an empty PermissionObject for the wildcard, contributing nothing but potentially interfering with how the config is displayed and managed. + +## Fix Plan + +### Fix 1: `stripNulls` — Remove Empty Objects After Stripping + +**File**: `packages/opencode/src/kilocode/config/config.ts:296-307` + +Change `stripNulls` to also remove keys whose value becomes an empty object after recursion: + +```ts +export function stripNulls(obj: Record): Record { + const result: Record = {} + for (const [key, value] of Object.entries(obj)) { + if (value === null) continue + if (isRecord(value)) { + const stripped = stripNulls(value) + if (Object.keys(stripped).length > 0) result[key] = stripped + // empty object after stripping → omit key entirely + } else { + result[key] = value + } + } + return result +} +``` + +This ensures `{ "*": { "*": null } }` → `{}` (the `"*"` key is removed entirely when its children are all null). + +### Fix 2: `patchJsonc` — Clean Up Empty Objects After Deletion + +**File**: `packages/opencode/src/config/config.ts:1274-1278` + +After the recursive `patchJsonc` processes all entries of an object-valued patch, check if the resulting JSONC node at the current `path` has become empty. If so, delete it: + +```ts +// After recursive reduce for object patches +let result = Object.entries(patch).reduce((result, [key, value]) => { + if (value === undefined) return result + return patchJsonc(result, value, [...path, key]) +}, input) + +// Clean up empty objects after deletion +if (path.length > 0) { + const tree = parseTree(result) + const node = tree && findNodeAtLocation(tree, path) + if (node && node.type === "object" && node.children?.length === 0) { + const edits = modify(result, path, undefined, { + formattingOptions: { insertSpaces: true, tabSize: 2 }, + }) + result = applyEdits(result, edits) + } +} +return result +``` + +### Fix 3: Route Handler — Alternative Simpler Approach + +Instead of fixes 1+2 (which are general improvements), we can also fix the route handler directly to use a simpler deletion strategy: + +**File**: `packages/opencode/src/kilocode/permission/routes.ts:55` + +Instead of: + +```ts +await Config.updateGlobal({ permission: { "*": { "*": null } } }, { dispose: false }) +``` + +Rewrite the entire `"*"` permission key to `null` (top-level delete): + +```ts +await Config.updateGlobal({ permission: { "*": null } }, { dispose: false }) +``` + +This deletes the entire `"*"` key from permission, which `stripNulls` already handles correctly for top-level nulls. The result is that `permission["*"]` is completely removed from the config file. + +**This is the simplest and most correct fix.** The current approach tries to delete the inner `"*"` key of a nested object, which leaves the outer `"*"` key as an empty shell. Deleting the outer key directly achieves the intended result. + +### Fix 4: Add Test for Global Disable Path + +**File**: `packages/opencode/test/server/permission-allow-everything.test.ts` + +Add a new test case that exercises the global (no sessionID) disable path: + +```ts +test("disables global allow-all and removes wildcard from config", async () => { + await using tmp = await tmpdir({ git: true }) + await Instance.provide({ + directory: tmp.path, + fn: async () => { + const app = Server.Default().app + + // Enable global auto-approve + await app.request("/permission/allow-everything", { + method: "POST", + headers: { "Content-Type": "application/json", "x-kilo-directory": tmp.path }, + body: JSON.stringify({ enable: true }), + }) + + // Disable global auto-approve + const response = await app.request("/permission/allow-everything", { + method: "POST", + headers: { "Content-Type": "application/json", "x-kilo-directory": tmp.path }, + body: JSON.stringify({ enable: false }), + }) + + expect(response.status).toBe(200) + expect(await response.json()).toBe(true) + + // Verify permission is asked after disabling + const session = await Session.create({}) + const pending = Permission.ask({ + id: PermissionID.make("permission_global_disable"), + sessionID: session.id, + permission: "bash", + patterns: ["ls"], + metadata: {}, + always: [], + ruleset: [], + }) + + await Permission.reply({ + requestID: PermissionID.make("permission_global_disable"), + reply: "reject", + }) + + await expect(pending).rejects.toBeInstanceOf(Permission.RejectedError) + }, + }) +}) +``` + +## Recommended Approach + +1. **Apply Fix 3** (route handler uses `{ "*": null }` instead of `{ "*": { "*": null } }`) — simplest, most targeted +2. **Apply Fix 1** (`stripNulls` removes empty objects) — general improvement that prevents similar issues +3. **Apply Fix 4** (add test) — prevents regression +4. Fix 2 is optional but good for completeness in the JSONC path + +## Files to Modify + +| File | Change | +| ------------------------------------------------------------------- | ------------------------------------------------------- | +| `packages/opencode/src/kilocode/permission/routes.ts:55` | Use `{ "*": null }` instead of `{ "*": { "*": null } }` | +| `packages/opencode/src/kilocode/config/config.ts:296-307` | `stripNulls` removes empty objects after recursion | +| `packages/opencode/test/server/permission-allow-everything.test.ts` | Add global disable test | + +## Notes + +- The permission prompts issue (Issue 2) is a **symptom** of Issue 1. Once the config file correctly removes `"*"` instead of leaving `{ "*": {} }`, subsequent config loads will have the correct permission rules. +- The `isAllowEverything` check in `app.tsx:46-52` already handles both forms correctly — it returns `false` for `{ "*": {} }` since `wildcard["*"]` is `undefined`. So the toggle UI works correctly; the problem is in the config write path. +- All changes are in kilocode-specific files, so no `kilocode_change` markers are needed (except in `config.ts:1274` if Fix 2 is applied, which is a shared file). diff --git a/packages/opencode/test/server/permission-allow-everything.test.ts b/packages/opencode/test/kilocode/server/permission-allow-everything.test.ts similarity index 92% rename from packages/opencode/test/server/permission-allow-everything.test.ts rename to packages/opencode/test/kilocode/server/permission-allow-everything.test.ts index d913d56b9c4..f79cf10c0fc 100644 --- a/packages/opencode/test/server/permission-allow-everything.test.ts +++ b/packages/opencode/test/kilocode/server/permission-allow-everything.test.ts @@ -1,11 +1,11 @@ // kilocode_change - new file import { describe, expect, test } from "bun:test" -import { Permission } from "../../src/permission" -import { PermissionID } from "../../src/permission/schema" -import { Instance } from "../../src/project/instance" -import { Server } from "../../src/server/server" -import { Session } from "../../src/session" -import { tmpdir } from "../fixture/fixture" +import { Permission } from "../../../src/permission" +import { PermissionID } from "../../../src/permission/schema" +import { Instance } from "../../../src/project/instance" +import { Server } from "../../../src/server/server" +import { Session } from "../../../src/session" +import { tmpdir } from "../../fixture/fixture" describe("permission.allowEverything endpoint", () => { test("disables global allow-all and removes wildcard from config", async () => {