-
Notifications
You must be signed in to change notification settings - Fork 3k
feat(tool): add opt-in zvec-grep search tool #6096
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?
Changes from all commits
d536522
bdffe01
1bf3df1
e644308
4323740
3996d0f
1b578fe
19c308b
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 |
|---|---|---|
|
|
@@ -2110,6 +2110,16 @@ export async function loadCliConfig( | |
| disabledSlashCommands.length > 0 ? disabledSlashCommands : undefined, | ||
| disabledSkillNamesProvider: | ||
| bareMode || safeMode ? undefined : disabledSkillNamesProvider, | ||
| zvecGrepEnabled: | ||
| bareMode || safeMode ? false : settings.tools?.zvecGrep?.enabled === true, | ||
| onDisableZvecGrepForWorkspace: async () => { | ||
| const currentSettings = loadSettings(cwd); | ||
|
Comment on lines
+2115
to
+2116
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] Running from $HOME, 'Disable for this workspace' writes to the USER settings file: the workspace path Suggested fix: Offer/persist the workspace opt-out only when a distinct effective workspace scope exists (workspaceSettingsActive true and workspace path != user path); otherwise omit the 'Disable for this workspace' choice. — qwen3.8-max via Qwen Code /review (v0.21.9) |
||
| currentSettings.setValue( | ||
| SettingScope.Workspace, | ||
|
Comment on lines
+2117
to
+2118
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. [Suggestion] In an UNTRUSTED folder the workspace opt-out write succeeds but is never read back: mergeSettings drops the entire workspace scope while untrusted (safeWorkspace = {}), while zvecGrepEnabled itself has no trust gate (user-scope enabled:true survives). Distinct from the refused-write finding — here the write succeeds and is silently ignored. — Failure scenario: With folder trust enabled and the folder untrusted, the user picks 'Do not install or index here. Always use regular search in this workspace' — nothing is persisted that takes effect, and the prompt returns every session: the choice's promise breaks exactly in the folder type where a user is most likely to pick it. Suggested fix: Gate the DISABLE_WORKSPACE_CHOICE on the workspace scope actually being honored (trusted + active), or surface a notice that the opt-out cannot be persisted while the folder is untrusted. — qwen3.8-max via Qwen Code /review (v0.21.9) |
||
| 'tools.zvecGrep.enabled', | ||
| false, | ||
| ); | ||
| }, | ||
| terminalImageRenderSupportProvider: interactive | ||
| ? async () => { | ||
| const { getTerminalImageRenderSupport } = await import( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2521,6 +2521,27 @@ const SETTINGS_SCHEMA = { | |
| }, | ||
| }, | ||
| }, | ||
| zvecGrep: { | ||
| type: 'object', | ||
| label: 'Zvec Grep', | ||
| category: 'Tools', | ||
| requiresRestart: true, | ||
| default: {}, | ||
| description: 'Settings for the zvec-grep built-in search tool.', | ||
| showInDialog: false, | ||
| properties: { | ||
| enabled: { | ||
| type: 'boolean', | ||
| label: 'Enable Zvec Grep', | ||
| category: 'Tools', | ||
| requiresRestart: true, | ||
| default: false, | ||
| description: | ||
| 'When enabled, registers the zvec_grep built-in tool. Disabled by default.', | ||
|
Comment on lines
+2539
to
+2540
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. [Suggestion] The opt-in setting still does not disclose the material side effects the latest maintainer review required at the consent boundary; the identical minimal text ships in packages/vscode-ide-companion/schemas/settings.schema.json. The interactive setup prompt discloses them, but it fires only interactively, after the setting is enabled — and for headless users (see the headless finding) the setting is the ONLY consent point. — Failure scenario: A user enables tools.zvecGrep believing it only 'registers the tool'; first semantic use may then run a global npm install (@zvec/zvec-grep@0.1.5 — ~214 packages, ~172 MB in an isolated prefix) and start a detached indexer that sends workspace fragments to the Qwen/DashScope embedding service with possible API cost. Suggested fix: Expand the description (both schemas) to name the global install, background full-workspace indexing, default remote embedding data flow and cost, the relevant credential env vars, and the ZVEC_GREP_EMBEDDING local-model alternative. — qwen3.8-max via Qwen Code /review (v0.21.9) |
||
| showInDialog: false, | ||
| }, | ||
| }, | ||
| }, | ||
| shell: { | ||
| type: 'object', | ||
| label: 'Shell', | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -173,6 +173,22 @@ describe('<AskUserQuestionDialog />', () => { | |
| expect(lastFrame()).toContain('Type something...'); | ||
| }); | ||
|
|
||
| it('hides custom input when the question only supports fixed choices', () => { | ||
| const details = createConfirmationDetails({ | ||
| questions: [createSingleQuestion({ allowCustomInput: false })], | ||
| }); | ||
| const onConfirm = vi.fn(); | ||
|
|
||
| const { lastFrame } = renderWithProviders( | ||
| <AskUserQuestionDialog | ||
| confirmationDetails={details} | ||
| onConfirm={onConfirm} | ||
| />, | ||
| ); | ||
|
Comment on lines
+182
to
+187
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] The new fixed-choice test renders Suggested fix: Add — qwen3.8-max via Qwen Code /review (v0.21.9) |
||
|
|
||
| expect(lastFrame()).not.toContain('Type something...'); | ||
|
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. [Suggestion] The allowCustomInput:false behavior spans five component gates (totalOptions, isCustomInputSelected, isCustomInputAnswer, the multi-select Enter early-return, and the Box display), but the single new test asserts only the rendered frame; sibling tests in the same file already drive stdin keys. The only production consumer of false is the zvec-grep setup prompt, operated via keyboard. — Failure scenario: Reverting totalOptions to options.length + 1 changes no rendered pixels (the slot is display:none), so the test stays green while Down/number-key navigation can land on the hidden slot past the last fixed choice — and in multi-select, Enter there falls through to premature submission. Suggested fix: Drive keys in the test: Down on the last option asserts no hidden entry is reached; Enter on a selected option asserts onConfirm fires with that option. — qwen3.8-max via Qwen Code /review (v0.21.9) |
||
| }); | ||
|
|
||
| it('renders help text for single select', () => { | ||
| const details = createConfirmationDetails(); | ||
| const onConfirm = vi.fn(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -88,13 +88,17 @@ export const AskUserQuestionDialog: React.FC<AskUserQuestionDialogProps> = ({ | |
| ? null | ||
| : confirmationDetails.questions[currentQuestionIndex]; | ||
| const isMultiSelect = currentQuestion?.multiSelect ?? false; | ||
| const allowCustomInput = currentQuestion?.allowCustomInput !== false; | ||
| // Options + custom input ("Other") | ||
| const totalOptions = currentQuestion ? currentQuestion.options.length + 1 : 2; | ||
| const totalOptions = currentQuestion | ||
|
Comment on lines
+91
to
+93
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. [Suggestion] The new allowCustomInput:false contract is honored only by the CLI TUI dialog. Desktop (AskUserQuestionRequest.tsx renders an unconditional 'Other' textarea; answerForQuestion returns custom || selected[0]) and web-shell (AskUserQuestion.tsx: answers[i] || customInputs[i] || '') ignore the field — grep-verified zero references in either package. — Failure scenario: On desktop/web-shell the zvec setup question still shows the free-text box; a typed answer ('yes, install it') fails the fixed-label match in zvec's onConfirm → falls through to useNativeGrep = true — semantic search silently declined for the session despite affirmative consent. Suggested fix: Propagate allowCustomInput through the desktop/web-shell permission-request types and hide the custom-input control when false; and/or treat an unrecognized answer as an explicit 'unknown choice' notice instead of the silent fall-through. — qwen3.8-max via Qwen Code /review (v0.21.9) |
||
| ? currentQuestion.options.length + (allowCustomInput ? 1 : 0) | ||
| : 2; | ||
|
|
||
| // Check if the custom input option is selected | ||
| const isCustomInputSelected = | ||
| !isSubmitTab && | ||
| currentQuestion && | ||
| allowCustomInput && | ||
| selectedIndex === currentQuestion.options.length; | ||
|
|
||
| const getCustomInputValue = (idx: number) => | ||
|
|
@@ -103,6 +107,7 @@ export const AskUserQuestionDialog: React.FC<AskUserQuestionDialogProps> = ({ | |
| const isCustomInputAnswer = | ||
| !isSubmitTab && | ||
| currentQuestion && | ||
| allowCustomInput && | ||
| !isMultiSelect && | ||
| selectedOptions[currentQuestionIndex] !== undefined && | ||
| !currentQuestion.options.some( | ||
|
|
@@ -331,7 +336,10 @@ export const AskUserQuestionDialog: React.FC<AskUserQuestionDialogProps> = ({ | |
| // Handle multi-select: Enter advances to next question / submits | ||
| if (isMultiSelect && currentQuestion) { | ||
| // Custom input is handled by TextInput's onSubmit | ||
| if (selectedIndex === currentQuestion.options.length) { | ||
| if ( | ||
| allowCustomInput && | ||
| selectedIndex === currentQuestion.options.length | ||
| ) { | ||
| return; | ||
| } | ||
| handleMultiSelectSubmit(); | ||
|
|
@@ -523,7 +531,10 @@ export const AskUserQuestionDialog: React.FC<AskUserQuestionDialogProps> = ({ | |
| })} | ||
|
|
||
| {/* Type something option/input */} | ||
| <Box flexDirection="column"> | ||
| <Box | ||
| flexDirection="column" | ||
| display={allowCustomInput ? 'flex' : 'none'} | ||
| > | ||
| {isCustomInputSelected ? ( | ||
| // Inline TextInput replaces the option text | ||
| <Box> | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -531,6 +531,7 @@ const FS_PATH_TOOL_NAMES: ReadonlySet<string> = new Set<string>([ | |||||||||||||||||||||||||||||||||||||||||||||||
| ToolNames.EDIT, | ||||||||||||||||||||||||||||||||||||||||||||||||
| ToolNames.WRITE_FILE, | ||||||||||||||||||||||||||||||||||||||||||||||||
| ToolNames.GREP, | ||||||||||||||||||||||||||||||||||||||||||||||||
| ToolNames.ZVEC_GREP, | ||||||||||||||||||||||||||||||||||||||||||||||||
| ToolNames.GLOB, | ||||||||||||||||||||||||||||||||||||||||||||||||
| ToolNames.LS, | ||||||||||||||||||||||||||||||||||||||||||||||||
| ToolNames.LSP, | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -714,6 +715,27 @@ export function extractToolFilePaths( | |||||||||||||||||||||||||||||||||||||||||||||||
| return out; | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| case ToolNames.ZVEC_GREP: { | ||||||||||||||||||||||||||||||||||||||||||||||||
|
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. [Suggestion] PATTERN (3 occurrences): zvec_grep was wired into some tool-classification tables but misses three sibling registrations its same-shape siblings have — (1) PermissionManager.CORE_TOOLS, so registerLazy's isToolEnabled skips the allowlist branch and operators pinning the surface with --core-tools/tools.core still get zvec_grep registered (the opt-in precedent web_search IS in CORE_TOOLS; zvec fits neither documented exemption; image_gen is also absent, so deliberate omission is possible); (2) PLAN_REQUIRED_TEAMMATE_PRE_APPROVAL_TOOLS, so a plan-required teammate awaiting approval is blocked from zvec_grep while identical-shape grep_search is allowed (fails closed, but defeats the gate's read-only-investigation purpose); (3) microcompaction's COMPACTABLE_TOOLS, so zvec_grep match listings persist in context forever while equivalent grep_search/glob outputs are evicted in long sessions. — Failure scenario: An operator's explicit tool allowlist still registers a tool that spawns background indexers and makes network embedding calls; plan-mode teammates lose one read-only investigation tool; long semantic-heavy sessions compact earlier than grep-equivalent ones. Suggested fix: Add zvec_grep to CORE_TOOLS (or document the exemption), to PLAN_REQUIRED_TEAMMATE_PRE_APPROVAL_TOOLS (read-only; install path has its own gate), and to COMPACTABLE_TOOLS (same output contract as GREP). — qwen3.8-max via Qwen Code /review (v0.21.9) |
||||||||||||||||||||||||||||||||||||||||||||||||
| const pathField = obj['path']; | ||||||||||||||||||||||||||||||||||||||||||||||||
| const pathsField = obj['paths']; | ||||||||||||||||||||||||||||||||||||||||||||||||
| const globField = obj['glob']; | ||||||||||||||||||||||||||||||||||||||||||||||||
| push(pathField); | ||||||||||||||||||||||||||||||||||||||||||||||||
| if (Array.isArray(pathsField)) { | ||||||||||||||||||||||||||||||||||||||||||||||||
| for (const item of pathsField) { | ||||||||||||||||||||||||||||||||||||||||||||||||
| push(item); | ||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+723
to
+725
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. [Suggestion] extractToolFilePaths' ZVEC_GREP case joins Suggested fix: Also push joinSearchRootAndGlob(item, globField) for each string in paths, and extend the focused test to the paths-only shape. — qwen3.8-max via Qwen Code /review (v0.21.9) |
||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
| if (typeof globField === 'string' && globField.length > 0) { | ||||||||||||||||||||||||||||||||||||||||||||||||
| push( | ||||||||||||||||||||||||||||||||||||||||||||||||
| joinSearchRootAndGlob( | ||||||||||||||||||||||||||||||||||||||||||||||||
| typeof pathField === 'string' ? pathField : undefined, | ||||||||||||||||||||||||||||||||||||||||||||||||
| globField, | ||||||||||||||||||||||||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+728
to
+735
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. [Suggestion]
Suggested change
— qwen3.7-max via Qwen Code /review |
||||||||||||||||||||||||||||||||||||||||||||||||
| return out; | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| case ToolNames.LS: | ||||||||||||||||||||||||||||||||||||||||||||||||
| push(obj['path']); | ||||||||||||||||||||||||||||||||||||||||||||||||
| return out; | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
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] No test asserts that
zvecGrepEnabledis forced tofalsewhenbareModeorsafeModeis active. — Failure scenario: a future refactor could naively readsettings.tools?.zvecGrep?.enabledwithout the guard, exposing the semantic search tool (which contacts an external embedding API) in restricted modes. The three existing CLI config tests all use normal mode, so the regression would go undetected. Consider adding a test withbareMode: true+tools.zvecGrep.enabled: true.— qwen3.7-max via Qwen Code /review