Skip to content

fix(cli): remember bash permission migration - #12642

Merged
johnnyeric merged 7 commits into
Kilo-Org:mainfrom
noobezlol:agent/fix-bash-permission-remigration
Sep 1, 2026
Merged

johnnyeric merged 7 commits into
Kilo-Org:mainfrom
noobezlol:agent/fix-bash-permission-remigration

Conversation

@noobezlol

Copy link
Copy Markdown
Contributor

Issue

Fixes #12625

Context

The compatibility migration that preserved bash: "allow" for existing users ran on every startup without recording completion. Deleting the generated permission therefore made the next startup treat the config as unmigrated and insert it again.

The same missing state could also misclassify a fresh installation later: once its schema-only config gained an unrelated setting, a future startup could treat it as an older config and add bash: "allow".

Implementation

Record completion of the bash permission migration in the global Kilo config directory. Completion is recorded whether the installation was migrated, already had an explicit permission, or was identified as fresh. Once recorded, later user edits are left untouched.

Marker writes are best-effort so a read-only config directory with an already valid permission cannot prevent startup.

Screenshots / Video

N/A — this changes config migration behavior without changing the UI.

How to Test

  • bun test test/kilocode/config/config.test.ts — 34 passed
  • bun run typecheck in packages/opencode
  • Targeted oxlint on the changed source and test files — 0 warnings, 0 errors
  • OpenCode annotation and Markdown table guards
  • git diff --check

Reviewer test steps

  1. Start with an existing global config that has no permission.bash.
  2. Start Kilo once and confirm the compatibility migration adds bash: "allow".
  3. Delete only the generated bash permission and restart Kilo.
  4. Confirm the permission remains deleted.

Checklist

  • Issue linked above
  • Tests and verification described
  • Screenshots marked N/A
  • Changeset included

@noobezlol
noobezlol marked this pull request as ready for review July 29, 2026 11:07
@@ -344,13 +345,20 @@ export namespace KilocodeConfig {
* behavior now that the new default is `bash: "ask"`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

SUGGESTION: Docstring doesn't mention the new idempotency marker

The function's core fix in this PR is recording completion via .bash-permission-migrated so a later deletion of permission.bash (or an unrelated config edit on a fresh install) isn't re-migrated. Worth a line in the docstring explaining this, since the existing comment only describes the one-time write and not the marker-based skip-on-subsequent-runs behavior that this PR adds.


Reply with @kilocode-bot fix it to have Kilo Code address this issue.

@kilo-code-bot

kilo-code-bot Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (2 files)
  • packages/opencode/src/kilocode/config/config.ts
  • packages/opencode/test/kilocode/config/config.test.ts
Previous Review Summaries (2 snapshots, latest commit abbeb3a)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit abbeb3a)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (2 files)
  • packages/opencode/src/kilocode/config/config.ts
  • packages/opencode/test/kilocode/config/config.test.ts

Previous review (commit c6acd28)

Status: 1 Issue Found | Recommendation: Merge

Overview

Severity Count
CRITICAL 0
WARNING 0
SUGGESTION 1
Issue Details (click to expand)

SUGGESTION

File Line Issue
packages/opencode/src/kilocode/config/config.ts 345 Docstring doesn't mention the new idempotency marker behavior

The migration logic is sound: the marker is checked first and short-circuits on every path (new user, explicit permission already set, schema-only file, legacy TOML, and after the write), all writes are entirely within packages/opencode/src/kilocode/ (no shared-file merge risk), the marker write is correctly best-effort (failures are logged, never thrown), and the new tests exercise real filesystem behavior without mocks, covering the two scenarios described in the issue (deleted permission not restored, fresh install not later misclassified). A changeset is included and reads well for end users.

Files Reviewed (3 files)
  • .changeset/quiet-bash-permissions.md
  • packages/opencode/src/kilocode/config/config.ts - 1 issue
  • packages/opencode/test/kilocode/config/config.test.ts

Reviewed by grok-4.6 · Input: 130.1K · Output: 9.9K · Cached: 554.8K

Review guidance: REVIEW.md from base branch main

// A schema-only file is generated for editor completion. It does not mean
// the user predates the bash permission default.
if (!hasLegacy && configs.every((item) => Object.keys(item.data).every((key) => key === "$schema"))) return
if (!hasLegacy && configs.every((item) => Object.keys(item.data).every((key) => key === "$schema"))) return done()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

After these changes the migration will be marked as done even in cases of unreadable or malformed config. Fixing the config won't retry the migration. Can you address this issue? Potentially distinguish read/parse failures from empty valid configs and add regression tests to cover these scenarios.

…ed config

Distinguish read/parse failures from empty valid configs so a
transient file error or JSONC syntax error does not permanently mark
the migration as done. Fixing the file now correctly retries.
continue
}
const errors: ParseError[] = []
const data = (parseJsonc(text, errors) as Record<string, unknown> | undefined) ?? {}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for addressing the comment, the change looks good, but please pass { allowTrailingComma: true } here to match ConfigParse.jsonc. Otherwise valid configs with trailing commas are treated as malformed and never receive the compatibility migration. Please add a regression test for that case.

Pass { allowTrailingComma: true } to match ConfigParse.jsonc so
valid JSONC with trailing commas is not treated as malformed and
correctly receives the compatibility migration. Add regression
test for trailing-comma config.
@johnnyeric
johnnyeric merged commit 95c731e into Kilo-Org:main Sep 1, 2026
45 of 48 checks passed
@johnnyeric

Copy link
Copy Markdown
Contributor

Thanks @noobezlol, I was able to reproduce the fix. Merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"permission": { "bash": "allow" } reappears after being deleted

2 participants