-
Notifications
You must be signed in to change notification settings - Fork 27.6k
fix: preserve permission ordering by accepting a layered array #23214
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -56,6 +56,13 @@ function mergeConfigConcatArrays(target: Info, source: Info): Info { | |
| if (target.instructions && source.instructions) { | ||
| merged.instructions = Array.from(new Set([...target.instructions, ...source.instructions])) | ||
| } | ||
| // Accumulate permission layers for later merging as rulesets. | ||
| // This preserves the ordering semantics: later rules override earlier rules. | ||
| // Each layer keeps the raw shape the user wrote on disk; consumers should use | ||
| // ConfigPermission.toLayers to normalise. | ||
| if (source.permission) { | ||
| merged.permission = [...ConfigPermission.toLayers(target.permission), ...ConfigPermission.toLayers(source.permission)] | ||
| } | ||
| return merged | ||
| } | ||
|
|
||
|
|
@@ -229,7 +236,12 @@ export const Info = Schema.Struct({ | |
| description: "Additional instruction files or patterns to include", | ||
| }), | ||
| layout: Schema.optional(ConfigLayout.Layout).annotate({ description: "@deprecated Always uses stretch layout." }), | ||
| permission: Schema.optional(ConfigPermission.Info), | ||
| permission: Schema.optional( | ||
| Schema.Union([ConfigPermission.Info, Schema.mutable(Schema.Array(ConfigPermission.Info))]), | ||
| ).annotate({ | ||
| description: | ||
| "Permission configuration. Accepts a single object (per-tool action map) or an array of layered configs; arrays are merged in order so later layers override earlier ones.", | ||
| }), | ||
| tools: Schema.optional(Schema.Record(Schema.String, Schema.Boolean)), | ||
| attachment: Schema.optional(ConfigAttachment.Info).annotate({ | ||
| description: "Attachment processing configuration, including image size limits and resizing behavior", | ||
|
|
@@ -708,11 +720,12 @@ export const layer = Layer.effect( | |
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: normalize the parsed env value with |
||
|
|
||
| if (Flag.OPENCODE_PERMISSION) { | ||
| result.permission = mergeDeep(result.permission ?? {}, JSON.parse(Flag.OPENCODE_PERMISSION)) | ||
| const envPermission = JSON.parse(Flag.OPENCODE_PERMISSION) as ConfigPermission.Info | ||
| result.permission = [...ConfigPermission.toLayers(result.permission), envPermission] | ||
| } | ||
|
|
||
| if (result.tools) { | ||
| const perms: Record<string, ConfigPermission.Action> = {} | ||
| const perms: ConfigPermission.Info = {} | ||
| for (const [tool, enabled] of Object.entries(result.tools)) { | ||
| const action: ConfigPermission.Action = enabled ? "allow" : "deny" | ||
| if (tool === "write" || tool === "edit" || tool === "patch") { | ||
|
|
@@ -721,7 +734,8 @@ export const layer = Layer.effect( | |
| } | ||
| perms[tool] = action | ||
| } | ||
| result.permission = mergeDeep(perms, result.permission ?? {}) | ||
| // Tools permissions come before other permissions (they can be overridden) | ||
| result.permission = [perms, ...ConfigPermission.toLayers(result.permission)] | ||
| } | ||
|
|
||
| if (!result.username) result.username = os.userInfo().username | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: consider using this layered merge in
loadGlobalas well. Right nowconfig.json,opencode.json, andopencode.jsoncare still combined withmergeConfig, so permission blocks split across global config files will still deep-merge into a single object before reaching this helper and can lose the user-written rule ordering this PR is trying to preserve.