fix: permission config deny rules not working with wildcard ask (#15664)#15736
fix: permission config deny rules not working with wildcard ask (#15664)#15736zoe531 wants to merge 1 commit into
Conversation
…alyco#15664) Fixed by changing merge order in config.ts. Previously mergeDeep(perms, result.permission) placed tool deny rules BEFORE user config rules, causing findLast in disabled() to return the catch-all ask rule instead of tool-specific deny rules. Changed to mergeDeep(result.permission ?? {}, perms) so user config comes first, and tool deny rules come last for correct findLast behavior.
|
The following comment was made by an LLM, it may be inaccurate: Based on my search results, I found one potentially related PR: Related PR:
This appears related as it also addresses permission config issues. However, the current PR (#15736) is specifically about deny rules being overridden by wildcard ask rules due to merge order in config.ts, while #14070 concerns root-level permission config not being respected - these are distinct issues even though both involve permission configuration. No exact duplicate PRs were found that address the same wildcard ask/deny rules merge order problem. |
|
Friendly bump! This fix is ready. Please review. Thank you! |
zoe531
left a comment
There was a problem hiding this comment.
This fix is ready. Please review.
|
Closing this pull request because it has had no updates for more than 60 days. If you plan to continue working on it, feel free to reopen or open a new PR. |
Issue for this PR
Closes #15664
Type of change
What does this PR do?
Fixes issue where tools config deny rules are silently overridden by wildcard ask rule in permission config.
The fix changes the merge order in config.ts. Previously mergeDeep(perms, result.permission) placed tool deny rules BEFORE user config rules. Changed to mergeDeep(result.permission ?? {}, perms) so user config comes first.
How did you verify your code works?
Analyzed the code and confirmed the merge order issue.
Local tests pass:
test/permission/next.test.ts: 62 pass
Screenshots / recordings
N/A - This is a backend fix, no UI changes.
Checklist