Skip to content

fix(core): apply global permission rules after repository rules - #15356

Open
iscekic wants to merge 10 commits into
mainfrom
drain/harden-agent-permission-precedence
Open

iscekic wants to merge 10 commits into
mainfrom
drain/harden-agent-permission-precedence

Conversation

@iscekic

@iscekic iscekic commented Oct 21, 2026 •

Copy link
Copy Markdown
Collaborator

Evidence

Evidence: demonstrated — the change was exercised on the work VM. The code agent ruleset was resolved through real config discovery with a global document and a repository document that both name the same bash pattern, and then used to gate a real tool call: the pattern-specific command is refused by the permission check and does not execute, while a command covered by the global entry runs and returns its output. debug config prints the ordered document layers and the merged map. Unit tests cover the same resolution for V1 and V2, including the markdown source guard.

What changed

packages/core/src/config/plugin/agent.ts applies agent permission rules from configuration documents in reverse document priority: repository-scoped documents first, the user-global document last. Permission resolution keeps the last matching rule, so a global rule defines the result when a global and a repository document both configure the same action and resource.

  • Non-permission settings (default_agent, model, variant, request, system, description, mode, hidden, color, steps, disabled) keep their nearest-wins merge, in a separate pass over the same documents.
  • Inside one document, the top-level permissions array is applied before that document's agents[<id>].permissions, and statement order inside each array is unchanged.
  • Repository rules cover requests the global configuration does not mention, and follow the built-in agent defaults because the built-in agent plugin runs first.
  • The permission pass skips ids that are absent after the settings pass, so a disabled agent stays removed.

packages/opencode/src/config/config.ts applies the same ordering to the V1 permission configuration that CLI agent resolution reads:

  • Global-scoped and project-scoped permission entries are collected as separate documents, ordered project first and global last, and exposed as the derived permission_layers field alongside the merged map.
  • packages/opencode/src/permission/index.ts gains fromScopes, which flattens those documents in order, and packages/opencode/src/agent/agent.ts builds the configured ruleset with it. A key that carries several patterns therefore resolves each pattern by document priority instead of by the position the key first appeared at in the merged map.
  • The merged map is still rebuilt in the same order, so permission_origins, the config overlay, and config reads agree with the ruleset. The derived field is stripped from persisted and editable config.
  • The markdown source guard reads permission rules for each config directory, so the ordered layers are applied before that scan. A global markdown_source entry decides a root that a repository document also names.
  • The legacy .kilocodeignore migration joins the project side, so a project permission entry still applies after the migrated ignore rules.
  • Project entries that no global file mentions keep applying, and every other config key keeps project-overrides-global.

Policy

The ordering matches what packages/core/src/config.ts already does for experimental.policies. packages/core/src/permission.ts (V2) and packages/opencode/src/permission/index.ts (V1) both resolve with the last matching rule, so the ruleset is built in document order rather than from a merged map whose key order no longer carries precedence.

Verification

  • packages/core: bun run typecheck clean; bun test ./test/config/agent.test.ts → 8 pass, including a global allow with a pattern-specific deny and a repository document that names the same pattern as allow (resolved effect: deny), and a command the global entry allows (resolved effect: allow).
  • packages/opencode: bun run typecheck clean; bun run script/test-runner.ts permission-origins config-overlay → 2 files, 2 passed. The permission tests cover a global multi-pattern bash entry with a repository entry for the same key and pattern (global effect wins), a repository entry for a key the global document does not mention (still applies), and a global markdown_source entry with a repository entry for the same root (the project agent document is not loaded from that root).
  • End-to-end through the CLI on the work VM with an isolated config tree, data, state, and cache directories, and a temporary project: a global document sets bash to allow with a pattern-specific deny, the repository document names that pattern as allow, and debug agent code --tool bash refuses the pattern-specific command with the permission error without executing it, while a command the global entry allows runs and returns 1.4.2. debug config prints permission_layers as [project, global] and the merged bash entry with both patterns.
  • Root bun run lint: 0 errors. script/check-opencode-annotations.ts --worktree reports all shared upstream changes annotated. check:duplication, check-workflows, and check-md-table-padding pass.

Production prerequisites

None.

Proxy answers

The owner's proxy gave these answers in the owner's place.

  1. Question: Precedence model: Which precedence model should agent permission rules use across config documents?
    Answer: Reverse document order
    Confidence: 0.85 - Precedent: least work with no quality drop; matching the policy loader's user-global-over-repository ordering under last-match resolution avoids a second evaluation mode.
  2. Question: Per-agent rules: Should cross-document precedence also cover agents[id].permissions (agent-level rules), or only top-level permissions?
    Answer: Both, same reversal
    Confidence: 0.85 - Precedent: fix the class on every affected surface, and the brief explicitly covers agents defined in config.
  3. Question: Docs: Add a short neutral precedence note to packages/kilo-docs/pages/customize/agent-permissions.md?
    Answer: Yes, neutral note
    Confidence: 0.65 - Precedent: update docs that would otherwise be incomplete, worded neutrally.
  4. Question: V1 scope: the V2 fix covers the config-agent path, while CLI agent resolution reads the V1 permission map that packages/opencode/src/config/config.ts merges. Should this PR also reorder the V1 path, or keep the V2 fix and disclose the V1 behavior?
    Answer: Extend this PR to V1 too
    Confidence: 0.80 - Precedent: fix the root cause and the class on every affected surface, and the brief asks for any separate CLI path with the same ordering to be made consistent.

@iscekic iscekic added drain Work item from the findings drain janitor:trust Owner-trusted drain work labels Oct 21, 2026
@github-actions github-actions Bot added area:cli CLI, TUI and core engine area:docs Documentation labels Oct 21, 2026
Comment thread packages/kilo-docs/pages/customize/agent-permissions.md Outdated
Comment thread packages/core/src/config/plugin/agent.ts Outdated
@kilo-code-bot

kilo-code-bot Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

The V1 ruleset now resolves from ordered permission documents (permission_layers) instead of the merged map, so a project entry can no longer outrank a global entry through key position for a shared multi-pattern key. Permission.fromScopes flattens project documents before global documents, the layers are applied before the markdown source guard scans each config directory, and the derived field is stripped from persisted/editable config. The new tests cover the shared-key multi-pattern case for both V1 and V2 plus the markdown source guard, and no other tool-enforcement path still builds a ruleset from the merged permission map.

Files Reviewed (6 files)
  • packages/opencode/src/config/config.ts
  • packages/opencode/src/permission/index.ts
  • packages/opencode/src/agent/agent.ts
  • packages/opencode/src/kilocode/tui/config.ts
  • packages/opencode/test/kilocode/permission/permission-origins.test.ts
  • packages/core/test/config/agent.test.ts
Previous Review Summaries (8 snapshots, latest commit fe20f2b)

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

Previous review (commit fe20f2b)

Status: No Issues Found | Recommendation: Merge

The incremental change adds a temp global config directory and redirects Global.Path.config in the overlay refresh test, which resolves the previous finding that GET /agent was reading the developer's real global config. The afterEach hook restores Global.Path.config, so the redirect does not leak into other tests.

Files Reviewed (1 file)
  • packages/opencode/test/kilocode/server/config-overlay.test.ts

Previous review (commit 0c80210)

Status: 1 Issue Found | Recommendation: Address before merge

The incremental change only removes the unused global permission.edit fixture from the overlay refresh test, which resolves the previous inert-fixture note. However, it also removes the only call that redirected Global.Path.config, so that test now reads the real user global config.

Overview

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

SUGGESTION

File Line Issue
packages/opencode/test/kilocode/server/config-overlay.test.ts 985 Removing setGlobal also stops redirecting Global.Path.config, so GET /agent now reads the developer's real global config and the before/permission source assertions become environment-dependent. Keep the redirect with a temp dir (as lines 914/938/1162 do) without the fixture.
Files Reviewed (1 file)
  • packages/opencode/test/kilocode/server/config-overlay.test.ts - 1 suggestion

Fix these issues in Kilo Cloud

Previous review (commit f830abd)

Status: 1 Issue Found | Recommendation: Address before merge

The incremental change only retargets the overlay refresh test to a project-only permission key, which is the correct adjustment for the new global-wins precedence. One test-quality nit remains: the global fixture is now inert and the baseline assertion passes on the built-in default.

Overview

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

SUGGESTION

File Line Issue
packages/opencode/test/kilocode/server/config-overlay.test.ts 990 Global permission.edit fixture no longer affects the before assertion (baseline websearch: allow comes from "*": "allow"); use a key whose default is not allow to keep the before/after assertion meaningful.
Files Reviewed (1 file)
  • packages/opencode/test/kilocode/server/config-overlay.test.ts - 1 suggestion

Previous review (commit c70240e)

Status: No Issues Found | Recommendation: Merge

The previous warning about legacy .kilocodeignore rules being applied after project config is resolved: the legacy permission contribution now joins the project bucket, so project kilo.json entries still apply after migrated ignore rules, and the new test covers the .kilocodeignore + project read: { "*": "deny" } case.

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

Previous review (commit 2b3cebf)

Status: 1 Issue Found | Recommendation: Address before merge

Overview

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

WARNING

File Line Issue
packages/opencode/src/config/config.ts 655 The globalPermissions bucket is seeded from result.permission, which at that point already includes merged legacy config (.kilocodeignore) derived read/edit rules with a wildcard "*": "allow". Those project-scoped rules are therefore applied after project config by the new rebuild, so a project kilo.json read/edit deny for "*" is silently replaced by allow, and permission_origins reports local for a value that came from the legacy layer.
Files Reviewed (4 files)
  • packages/opencode/src/config/config.ts - 1 issue
  • packages/opencode/test/kilocode/permission/permission-origins.test.ts - no issues
  • packages/kilo-docs/pages/customize/agent-permissions.md - no issues
  • packages/kilo-docs/pages/getting-started/settings/index.md - no issues

Fix these issues in Kilo Cloud

Previous review (commit 2864313)

Status: No Issues Found | Recommendation: Merge

The incremental commit 28643133 resolves both prior suggestions: the top-level permission loop is now guarded by permissions.length > 0 (a pure no-op skip, since pushing an empty ruleset changed nothing), and the precedence note now names agent Markdown files in a project config directory. The doc wording is consistent with the implementation: a Markdown agent document's own rules are appended before the reversed pass reaches the global document, so global rules still evaluate last. Test markers now wrap the whole reordered expectation block instead of a single argument line. Ordering behavior is unchanged and the added end-to-end test still exercises the intended last-match resolution.

Files Reviewed (3 files)
  • packages/core/src/config/plugin/agent.ts - no issues
  • packages/core/test/config/agent.test.ts - no issues
  • packages/kilo-docs/pages/customize/agent-permissions.md - no issues

Previous review (commit acdbc11)

Status: 2 Issues Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 0
WARNING 0
SUGGESTION 2

The incremental commit (acdbc11c) only adds kilocode_change markers around the permission-ordering test expectations; no behavior changed. The ordering change itself remains sound: configs is built lowest-to-highest priority per Config.entries(), so documents.toReversed() appends repository rules before global rules and the findLast evaluation in PermissionV2.evaluate lets global rules win. The settings pass keeps nearest-wins, disabled agents stay removed, and the new test covers the intended end-to-end result. The two prior suggestions below are re-verified against current HEAD and still apply.

Issue Details (click to expand)

SUGGESTION

File Line Issue
packages/kilo-docs/pages/customize/agent-permissions.md 80 Note should clarify the change also applies to agent Markdown files: a global top-level rule is appended after an agent's own permission rules, so a global bash: allow overrides an agent-level deny in .kilo/agent/*.md.
packages/core/src/config/plugin/agent.ts 110 The per-agent loop runs draft.update for every agent for every document, including Markdown agent documents that never define top-level permissions; skip when permissions is empty.
Files Reviewed (4 files)
  • .changeset/agent-permission-config-precedence.md - no issues
  • packages/core/src/config/plugin/agent.ts - 1 suggestion
  • packages/core/test/config/agent.test.ts - no issues (marker-only increment)
  • packages/kilo-docs/pages/customize/agent-permissions.md - 1 suggestion

Fix these issues in Kilo Cloud

Previous review (commit b974f7b)

Status: 2 Issues Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 0
WARNING 0
SUGGESTION 2

The ordering change itself looks correct: configs is built lowest-to-highest priority (Config.entries() contract), so documents.toReversed() appends repository rules before global rules and the findLast evaluation in PermissionV2.evaluate makes global rules win as described. The settings pass keeps nearest-wins, disabled agents stay removed, and the new test exercises the intended end-to-end result.

Issue Details (click to expand)

SUGGESTION

File Line Issue
packages/kilo-docs/pages/customize/agent-permissions.md 80 Note should clarify the change also applies to agent Markdown files: a global top-level rule is appended after an agent's own permission rules, so a global bash: allow overrides an agent-level deny in .kilo/agent/*.md.
packages/core/src/config/plugin/agent.ts 110 The per-agent loop runs draft.update for every agent for every document, including Markdown agent documents that never define top-level permissions; skip when permissions is empty.
Files Reviewed (4 files)
  • .changeset/agent-permission-config-precedence.md - no issues
  • packages/core/src/config/plugin/agent.ts - 1 suggestion
  • packages/core/test/config/agent.test.ts - no issues
  • packages/kilo-docs/pages/customize/agent-permissions.md - 1 suggestion

Fix these issues in Kilo Cloud


Reviewed by deepseek-flash-latest · Input: 145.2K · Output: 40.8K · Cached: 4.9M

Review guidance: REVIEW.md from base branch main

Comment thread packages/opencode/src/config/config.ts Outdated
Comment thread packages/opencode/test/kilocode/server/config-overlay.test.ts
Comment thread packages/opencode/test/kilocode/server/config-overlay.test.ts
@iscekic iscekic added the merge:human A human reviews and merges this pull request. label Oct 22, 2026
@iscekic
iscekic enabled auto-merge (squash) October 22, 2026 00:03
@iscekic
iscekic disabled auto-merge October 22, 2026 15:05
@iscekic

iscekic commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

Changes requested: the V1 precedence fix can still be bypassed through key order.

  • packages/opencode/src/config/config.ts rebuilds result.permission = mergeDeep(merge(local), merge(global)). remeda's mergeDeep keeps the project object's key order, and fromConfig/evaluate in permission/index.ts use the last matching rule.
  • Example: global bash: {"*": "allow", "rm *": "deny"} plus project bash: {"rm *": "allow"} merges to {"rm *": "deny", "*": "allow"}, so rm -rf x is allowed. Global {"*": "allow", "bash": "deny"} plus project {"bash": "allow"} gives bash = allow. This was checked with the real remeda merge and a copy of the evaluate logic, not by running the CLI.
  • Build the V1 ruleset by appending fromConfig(local) rules and then fromConfig(global) rules, the same way V2 does. Do not merge them into one object.
  • ExternalMarkdown.scopes reads result.permission before the rebuild. Apply the rebuilt order before that read, so that a global markdown_source deny holds.
  • Add tests with multi-pattern configs where the project names the same key or pattern as the global config, for both V1 and V2.

Build the v1 ruleset from config documents in ascending precedence instead
of flattening the merged permission map, and apply the same order before the
markdown source guard reads permission rules. Multi-pattern entries in a
shared key now resolve with the global rule last.
@iscekic iscekic removed the merge:human A human reviews and merges this pull request. label Oct 24, 2026
@iscekic

iscekic commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

bot: Addressed in 183d107.

  • Agent permission rules are built from config documents in ascending precedence. packages/opencode/src/config/config.ts keeps global-scoped and project-scoped permission entries as separate documents, ordered project first and global last, and exposes them as the derived permission_layers field. packages/opencode/src/permission/index.ts adds fromScopes, which flattens those documents in order, and packages/opencode/src/agent/agent.ts builds the configured ruleset with it, so every pattern under a shared key follows document priority. The derived field is stripped from persisted and editable config.
  • The ordered layers are applied before the markdown source guard scans each config directory, so a global markdown_source entry decides a root that a repository document also names.
  • Tests: packages/opencode/test/kilocode/permission/permission-origins.test.ts covers documents that both configure bash, a key only the repository document configures, and the markdown source guard for a shared root. packages/core/test/config/agent.test.ts covers the same shape for the V2 documents.
  • Verified on the work VM with an isolated config tree and a temporary project: with a global document and a repository document both configuring bash, the resolved ruleset keeps the global rule last, Permission.evaluate returns the global effects for both patterns, and a command the global entry allows runs through debug agent code --tool bash. debug config prints permission_layers in document order. bun run typecheck is clean for both packages, root bun run lint reports 0 errors, and the annotation, duplication, workflow, and table guards pass.

@iscekic iscekic added merge:blocked:kilobot-not-ready Review bot has not recommended merge at this head merge:human A human reviews and merges this pull request. and removed merge:blocked:kilobot-not-ready Review bot has not recommended merge at this head labels Oct 24, 2026

This branch has not been deployed

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

Labels

area:cli CLI, TUI and core engine area:docs Documentation drain Work item from the findings drain janitor:trust Owner-trusted drain work merge:human A human reviews and merges this pull request.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant