-
Notifications
You must be signed in to change notification settings - Fork 37k
Add workspace and session allow options in terminal tool #284284
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull request overview
This PR adds workspace-scoped and session-scoped auto-approval options for terminal commands in the chat agent tools feature. Users can now choose to auto-approve commands for the current session only, for the workspace, or globally (user settings).
Key changes:
- Adds session-scoped auto-approve rules stored in-memory (cleared when chat session ends)
- Adds workspace-scoped auto-approve rules persisted in workspace settings
- Updates UI to provide three action buttons: "Allow in this Session", "Allow in this Workspace", and "Always Allow"
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
commandLineAutoApprover.ts |
Adds session rule support with _getSessionRules() method, updates isCommandAutoApproved() and isCommandLineAutoApproved() to accept optional chatSessionId parameter, and defines AutoApproveRuleSource enum |
commandLineAutoApproveAnalyzer.ts |
Updates rule link formatting to display scope indicators (session/workspace/user/default/remote) and handle non-actionable session rules |
runInTerminalHelpers.ts |
Generates three separate action buttons for each scope (session/workspace/user) for both command and exact command line rules, adds separator between action groups |
terminalChatService.ts |
Implements in-memory storage for session auto-approve rules with cleanup on session disposal via addSessionAutoApproveRule() and getSessionAutoApproveRules() methods |
terminal.ts |
Adds interface methods for session auto-approve rule management to ITerminalChatService |
chatTerminalToolConfirmationSubPart.ts |
Handles rule creation by scope, groups rules by session/workspace/user, updates configuration targets appropriately, and provides scope-specific feedback messages |
| export const enum AutoApproveRuleSource { | ||
| Default = 'default', | ||
| User = 'user', | ||
| Workspace = 'workspace', | ||
| Session = 'session' | ||
| } | ||
|
|
Copilot
AI
Dec 18, 2025
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.
The AutoApproveRuleSource enum is defined but appears to be unused anywhere in the codebase. The sourceTarget property in IAutoApproveRule uses ConfigurationTarget | 'session' directly instead of this enum. Consider either using this enum in the IAutoApproveRule interface or removing it if it's not needed.
| export const enum AutoApproveRuleSource { | |
| Default = 'default', | |
| User = 'user', | |
| Workspace = 'workspace', | |
| Session = 'session' | |
| } |
| if (e.rule!.sourceTarget === 'session') { | ||
| return localize('autoApproveRule.sessionIndicator', '{0} (session)', `\`${e.rule!.sourceText}\``); |
Copilot
AI
Dec 18, 2025
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.
The non-null assertion operator (!) is used on e.rule, but the type signature shows that rule is optional (rule?: IAutoApproveRule). While the current callers of formatRuleLinks check for rule existence before calling, this function should be defensive and add a guard clause at the beginning to handle the case where e.rule is undefined. Consider adding a check like: if (!e.rule) { return ''; } at the start of the map callback.
See below for a potential fix:
if (!e.rule) {
return '';
}
// Session rules cannot be actioned currently so no link
if (e.rule.sourceTarget === 'session') {
return localize('autoApproveRule.sessionIndicator', '{0} (session)', `\`${e.rule.sourceText}\``);
}
const settingsUri = createCommandUri(TerminalChatCommandId.OpenTerminalSettingsLink, e.rule.sourceTarget);
const tooltip = localize('ruleTooltip', 'View rule in settings');
let label = e.rule.sourceText;
switch (e.rule.sourceTarget) {
| const settingsUri = createCommandUri(TerminalChatCommandId.OpenTerminalSettingsLink, e.rule!.sourceTarget); | ||
| return `[\`${e.rule!.sourceText}\`](${settingsUri.toString()} "${localize('ruleTooltip', 'View rule in settings')}")`; | ||
| const tooltip = localize('ruleTooltip', 'View rule in settings'); | ||
| let label = e.rule!.sourceText; |
Copilot
AI
Dec 18, 2025
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.
The non-null assertion operator (!) is used on multiple accesses to e.rule throughout this function (lines 198 and 200), but rule is optional according to the type signature. Add a guard clause at the beginning of the map callback to handle undefined rules safely.
See below for a potential fix:
if (!e.rule) {
return e.reason;
}
// Session rules cannot be actioned currently so no link
if (e.rule.sourceTarget === 'session') {
return localize('autoApproveRule.sessionIndicator', '{0} (session)', `\`${e.rule.sourceText}\``);
}
const settingsUri = createCommandUri(TerminalChatCommandId.OpenTerminalSettingsLink, e.rule.sourceTarget);
const tooltip = localize('ruleTooltip', 'View rule in settings');
let label = e.rule.sourceText;
switch (e.rule.sourceTarget) {
| subCommandLabel = `\`${subCommandsToSuggest[0]} \u2026\``; | ||
| } else { | ||
| const commandSeparated = subCommandsToSuggest.join(', '); | ||
| subCommandLabel = localize('autoApprove.baseCommand', 'Always Allow Commands: {0}', commandSeparated); | ||
| subCommandLabel = `Commands ${subCommandsToSuggest.map(e => `\`${e} \u2026\``).join(', ')}`; | ||
| } | ||
|
|
||
| actions.push({ | ||
| label: subCommandLabel, | ||
| label: `Allow ${subCommandLabel} in this Session`, |
Copilot
AI
Dec 18, 2025
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.
The label formatting is inconsistent between single and multiple commands. For a single command, the label is "Allow command ... in this Session", but for multiple commands it becomes "Allow Commands cmd1 ..., cmd2 ... in this Session". The word "Commands" appearing in the middle creates an awkward construction. Consider restructuring the label to either: (1) always use a consistent pattern like "Allow Command(s) ... in this Session", or (2) move the plural indicator outside the variable part, such as "Allow the following commands in this Session: cmd1 ..., cmd2 ...".
| } satisfies TerminalNewAutoApproveButtonData | ||
| }); | ||
| actions.push({ | ||
| label: `Allow ${subCommandLabel} in this Workspace`, |
Copilot
AI
Dec 18, 2025
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.
The same label inconsistency issue exists here as well. The pattern "Allow Commands ... in this Workspace" for multiple commands vs "Allow command ... in this Workspace" for a single command creates an awkward reading experience.
| } satisfies TerminalNewAutoApproveButtonData | ||
| }); | ||
| actions.push({ | ||
| label: `Always Allow ${subCommandLabel}`, |
Copilot
AI
Dec 18, 2025
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.
The same label inconsistency issue exists here. The pattern "Always Allow Commands ..." for multiple commands vs "Always Allow command ..." for a single command should be made consistent.
Fixes #270529