Repository navigation
Fix incorrect inspect property usage in scope detection - #301472
Dmitriy Vasyura (dmitrivMS) merged 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes incorrect use of IInspectValue<T> (inspect metadata objects) where scalar *Value properties should be used for configuration scope detection, preventing misclassification of configuration targets.
Changes:
- Use
settings.userRemoteValue(scalar) instead ofsettings.userRemote(inspect object) when auto-selecting theme configuration target. - Use
configInspectValue.workspaceFolderValue(scalar) instead ofconfigInspectValue.workspaceFolder(inspect object) when determining the source target for auto-approve rules.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/vs/workbench/services/themes/common/themeConfiguration.ts | Fixes auto-target selection to correctly detect USER_REMOTE based on the scalar remote value. |
| src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/commandLineAnalyzer/autoApprove/commandLineAutoApprover.ts | Fixes workspace-folder scope detection for auto-approve rule source targeting by using the scalar workspace-folder value. |
You can also share your feedback on Copilot code review. Take the survey.
|
@microsoft-github-policy-service agree |
In findAutoConfigurationTarget() (themeConfiguration.ts), change
settings.userRemote to settings.userRemoteValue. The former is an
IInspectValue<T> object that can be defined when only language
overrides exist, causing the function to incorrectly return
USER_REMOTE when no actual remote value is set.
In commandLineAutoApprover.ts, change configInspectValue.workspaceFolder
to configInspectValue.workspaceFolderValue. The former is an
IInspectValue<T> object whose keys are {value, override, overrides},
so checkTarget() never finds tool-pattern keys on it, making
workspace-folder-scoped auto-approve rules undetectable.
Both properties were introduced in bb18007.
d9f5505 to
d416230
Compare
Raymond Zhao (rzhao271)
assigned
Sandeep Somavarapu (sandy081) and unassigned
Raymond Zhao (rzhao271)
|
Sandeep Somavarapu (@sandy081) I have posted detailed reproduction steps and type-level analysis on the linked issue in response to your question. Quick summary of the two fixes in this PR:
A regression test is included that verifies workspace-folder-scoped rules get |
Sandeep Somavarapu (sandy081)
assigned
Daniel Imms (Tyriar) and
Megan Rogge (meganrogge) and unassigned
Sandeep Somavarapu (sandy081)
Megan Rogge (meganrogge)
left a comment
Megan Rogge (meganrogge)
left a comment
There was a problem hiding this comment.
Thanks! LGTM!
|
Hi Dmitriy Vasyura (@dmitrivMS) Megan Rogge (@meganrogge) thank you both for the approvals! The CI is failing because of an issue in the regression test I added. The test config uses The fix is a one-character change: I'm pushing this now - apologies that it'll reset the approval status. The diff should be trivially reviewable (single character removal in test data). |
The test config 'git*' gets escaped to regex /^git\*\b/ which only matches the literal string 'git*', not 'git status'. Changed to 'git' which produces /^git\b/ and correctly matches 'git status' via word boundary.
Head branch was pushed to by a user without write access
0674d4c
|
Please approve this Megan Rogge (@meganrogge) it was supposed to be approved and merged before. Thank you! |
|
What is preventing this from getting merged? |
|
John Murray (@gjsjohnmurray) Strange... it was stuck waiting for approvals while it should have had 2 already. |
|
Hmm... now it's stuck on the checks. Shehab Sherif (@ShehabSherif0) if you merge from main and retrigger, perhaps it will help to clear those... |
|
Ok, doing Close/Reopen to retrigger checks. |
44a5af2
into
microsoft:main
|
John Murray (@gjsjohnmurray) Merged! |
Fixes #301471
Bug
Two
IConfigurationValueproperty accesses introduced in bb18007 use theIInspectValue<T>object property instead of the scalar value property, causing incorrect scope detection.1.
themeConfiguration.ts-findAutoConfigurationTarget()settings.userRemoteis anIInspectValue<T>object ({value?, override?, overrides?}), not the scalarTvalue. It can be defined when only language-specific overrides exist in the remote config, even with no actual remote value set. This causes the function to incorrectly returnUSER_REMOTE, making theme settings target the wrong configuration scope.Fix:
settings.userRemote->settings.userRemoteValue2.
commandLineAutoApprover.ts- source target detectionconfigInspectValue.workspaceFolderis anIInspectValue<T>object whose keys are{value, override, overrides}. ThecheckTargetfunction looks for tool-pattern keys (e.g.,"git*") viahasOwnProperty, which never matches onIInspectValue. Workspace-folder-scoped auto-approve rules are never detected and fall through to a lower-priority scope.Fix:
configInspectValue.workspaceFolder->configInspectValue.workspaceFolderValueRoot Cause
Both uses are from the same commit (bb18007). The
IConfigurationValue<T>interface exposes both scalar values (userRemoteValue: T) and inspect objects (userRemote: IInspectValue<T>). The similar naming makes it easy to use the wrong one.Testing
Added a regression test that stubs
configurationService.inspectto return aworkspaceFolderValuecontaining a rule key and asserts the matched rule reportssourceTargetasConfigurationTarget.WORKSPACE_FOLDER.