-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(core): make permissions.allow restrict the tool schemas sent to the model #9829
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
cd8509f
6af0884
a554edb
b8ba258
477c36f
105e5f0
4d30e18
c68f473
4f1ee39
a9031c1
e9afe40
1494aad
de9a8e3
c965468
df5fada
b0adfa7
23189f2
55f1fb5
0869a84
f25d7ce
be96336
519be05
56b1db6
ced9a08
5437a89
a4ee647
640ec59
63970a3
6c2694b
c9b670e
3ca966c
7283ed1
5e63280
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2198,6 +2198,16 @@ export async function loadCliConfig( | |
| allow: mergedAllow.length > 0 ? mergedAllow : undefined, | ||
| ask: mergedAsk.length > 0 ? mergedAsk : undefined, | ||
| deny: mergedDeny.length > 0 ? mergedDeny : undefined, | ||
| // Only `settings.permissions.allow` (never `--allowed-tools` nor the | ||
| // legacy `tools.allowed` key, which stay pure auto-approval grants) | ||
| // activates the registry-level allowlist that hides unlisted built-in | ||
| // tools from the model request (#9827). | ||
| registryAllowList: | ||
| bareMode || safeMode | ||
| ? undefined | ||
| : settings.permissions?.allow?.length | ||
| ? settings.permissions.allow | ||
| : undefined, | ||
|
Comment on lines
+2205
to
+2210
Collaborator
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. [Critical] R2-1: A single "Always allow" confirmation choice persists into 中文说明一次"始终允许"确认选择会持久化到 — qwen3.8-max via Qwen Code /review (v0.22.0)
Collaborator
Author
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. Confirmed real at HEAD — the full chain reproduces (persist I'm escalating rather than fixing because the only correct fixes are a data-semantics decision that shouldn't be made unilaterally inside this PR. Interactively-persisted rules and hand-authored rules are indistinguishable once both live in
My lean is (1) since it matches the grant-vs-allowlist split this PR already establishes, but it changes the on-disk settings format and the public
Collaborator
Author
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. Already escalated in the round-2 thread — see reply 3840449009: the chain is confirmed real at HEAD (persisting an "Always allow" rule writes settings.permissions.allow, which activates the allowlist on next restart and collapses the toolset to that one family). The remediation requires a maintainer settings data-format decision — persist interactive grants under a separate non-activating key, or gate allowlist activation behind explicit hand-authored intent — so it is not being fixed in this review round. Leaving this thread open to track that decision.
Collaborator
Author
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. Still maintainer-gated this round — no code change on this surface (the round-6 R6-1 fix in c9b670e only touches the tool_search exemption). Note: round 6's R1-8 finding reduces to the same root question — what activates the |
||
| autoMode: | ||
| bareMode || safeMode ? undefined : settings.permissions?.autoMode, | ||
| }, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.