Skip to content
Merged
148 changes: 135 additions & 13 deletions packages/cli/src/ui/hooks/atCommandProcessor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import * as fs from 'node:fs/promises';
import * as path from 'node:path';
import type { Part, PartListUnion } from '@google/genai';
import type { Config } from '@qwen-code/qwen-code-core';
import type { Config, Extension } from '@qwen-code/qwen-code-core';
import {
getErrorMessage,
isNodeError,
Expand All @@ -19,6 +19,7 @@ import {
emptyMcpResourceText,
formatMcpResourceContents,
summarizeMcpResource,
stripTerminalControlSequences,
} from '@qwen-code/qwen-code-core';
import type {
HistoryItemToolGroup,
Expand All @@ -27,6 +28,11 @@ import type {
} from '../types.js';
import { ToolCallStatus } from '../types.js';
import { matchMcpServerPrefix } from './mcpResourceRef.js';
import {
parseExtensionRef,
matchExtensionByRef,
buildExtensionContextText,
} from './extension-mention-ref.js';

export interface ResolveAtCommandParams {
query: string;
Expand Down Expand Up @@ -205,6 +211,13 @@ export async function resolveAtCommandQuery({
uri: string;
}> = [];

// Extension references (`@ext:<name>`) collected during the loop.
const activeExtensions = config.getActiveExtensions?.() ?? [];

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point — the autocomplete side has the guard, but the resolution path doesn't. However, adding it to the resolution path would mean @ext:name silently disappears in untrusted folders after autocomplete showed it. The current behavior is consistent: if autocomplete doesn't show extensions in untrusted folders, the user can't construct the reference. If they manually type it, the extension context is still from the user's own installed extensions, not the workspace.

const extensionMentions: Array<{
originalAtPath: string;
extension: Extension;
}> = [];

for (const atPathPart of atPathCommandParts) {
const originalAtPath = atPathPart.content; // e.g., "@file.txt" or "@"

Expand All @@ -217,6 +230,28 @@ export async function resolveAtCommandQuery({

const pathName = originalAtPath.substring(1);

// Extension reference (`@ext:<name>`): detected BEFORE MCP/filesystem
// resolution. Only matches when the path starts with `ext:` and the name
// corresponds to an active extension.
const extRef = parseExtensionRef(pathName);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By design — ext: is an unambiguous prefix. If a user types @ext:foo and foo isn't an extension, it's a typo, not an MCP server reference. MCP servers don't use the ext: prefix.

if (extRef) {
const extension = matchExtensionByRef(extRef.name, activeExtensions);
if (extension) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] No deduplication of extension mentions.

If the user types @ext:foo @ext:foo, the same extension is matched twice, its context files are read twice, and the context text is injected twice into the prompt — wasting both I/O and context window budget.

Suggested change
if (extension) {
const extension = matchExtensionByRef(extRef.name, activeExtensions);
if (extension) {
if (!extensionMentions.some((m) => m.extension.name === extension.name)) {
extensionMentions.push({ originalAtPath, extension });
}
atPathToResolvedSpecMap.set(originalAtPath, pathName);
continue;
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — added dedup check: extensionMentions.some(m => m.extension.name === extension.name) before pushing.

if (
!extensionMentions.some((m) => m.extension.name === extension.name)
) {
extensionMentions.push({ originalAtPath, extension });
}
atPathToResolvedSpecMap.set(originalAtPath, pathName);
continue;
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — unmatched ext: refs now continue with a debug message listing available extensions, instead of falling through to filesystem resolution.

onDebugMessage(
`Extension "${extRef.name}" not found among active extensions. ` +
`Available: ${activeExtensions.map((e) => e.name).join(', ') || '(none)'}`,
);
continue;
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already fixed — unmatched @ext: refs now continue with a debug message listing available extensions (commit b45f061).


// MCP resource reference (`@server:uri`): detected BEFORE filesystem
// resolution so a resource URI containing ':' / '//' isn't mistaken for
// a path. Only matches when `server` is a configured MCP server; all
Expand Down Expand Up @@ -444,8 +479,12 @@ export async function resolveAtCommandQuery({

// Fallback for lone "@" or completely invalid @-commands resulting in empty
// initialQueryText — only when there is nothing to read at all (no valid
// file paths AND no resource references).
if (pathSpecsToRead.length === 0 && mcpResourceRefs.length === 0) {
// file paths, resource references, or extension mentions).
if (
pathSpecsToRead.length === 0 &&
mcpResourceRefs.length === 0 &&
extensionMentions.length === 0
) {
onDebugMessage('No valid file paths found in @ commands to read.');
if (initialQueryText === '@' && query.trim() === '@') {
// If the only thing was a lone @, pass original query (which might have spaces)
Expand All @@ -461,8 +500,83 @@ export async function resolveAtCommandQuery({
};
}

// Build extension context parts and display cards for @-mentioned extensions.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Missing integration tests for security boundary code

113 lines of new integration logic — path traversal guard (isSubpath), 200KB aggregate budget, per-file cap, dedup, extension resolution priority — have zero test coverage. The R1 review requested tests; 20 unit tests were added for extension-mention-ref.ts (pure helpers), but the security-critical code here remains untested.

The existing atCommandProcessor.test.ts has 44 tests with established mocking patterns for Config, fs.readFile, etc. At minimum, add tests for:

  1. @ext:name resolution produces extension display + context part
  2. Path traversal rejection for context files outside extension.path
  3. Budget truncation when a context file exceeds 50KB
  4. Dedup when the same extension is mentioned twice (@ext:browser @ext:browser)
  5. @ext:nonexistent falls through to file/MCP resolution

— qwen3.7-max via Qwen Code /review

// Processed BEFORE file reads so that extension labels/displays are available
// in the file-read error path (mirroring how resourceDisplays/resourceLabels
// are already built before the file read).
// Aggregate cap across all extensions to prevent unbounded context injection.
const EXTENSION_CONTEXT_BUDGET = 200_000; // 200KB total

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The budget is an approximate guard against context blowup, not a precise byte counter. String.length is consistent with how the rest of the codebase handles text size limits.

const PER_FILE_CAP = 50_000; // 50KB per context file
let extensionContextBudgetRemaining = EXTENSION_CONTEXT_BUDGET;

const extensionParts: Part[] = [];
const extensionDisplays: IndividualToolCallDisplay[] = [];
const extensionLabels: string[] = [];
for (let i = 0; i < extensionMentions.length; i++) {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The outer loop needs to be sequential because the aggregate budget (extensionContextBudgetRemaining) is shared across extensions. Parallelizing would require pre-allocating budget per extension or a post-hoc trim, adding complexity for a rare edge case (multiple @-mentioned extensions).

const { extension } = extensionMentions[i];
const displayName = stripTerminalControlSequences(
extension.displayName || extension.name,
);
const callId = `client-extension-${userMessageTimestamp}-${i}`;

let contextText = buildExtensionContextText(extension);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point, but the metadata block is ~200-500 bytes which is negligible against the 200KB budget. Not worth the complexity of deducting it.


// Read extension context files in parallel, with path traversal and
// budget checks.
if (extension.contextFiles && extension.contextFiles.length > 0) {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The metadata block (buildExtensionContextText) is always emitted regardless of budget — it's the context files that are budget-gated. The metadata (~200-500 bytes) is negligible. Adding an outer check would skip the metadata block too, which would be wrong.

const fileReads = await Promise.allSettled(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Promise.allSettled reads all context files before budget check

All context files are read into memory simultaneously via Promise.allSettled. When the 200KB budget is already exhausted by a prior extension, subsequent extensions' files are still read and discarded — wasted IO and memory.

Add an early budget guard before the parallel read phase:

Suggested change
const fileReads = await Promise.allSettled(
if (extension.contextFiles && extension.contextFiles.length > 0
&& extensionContextBudgetRemaining > 0) {

— qwen3.7-max via Qwen Code /review

extension.contextFiles.map(async (contextFilePath) => {
const resolved = path.resolve(contextFilePath);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] Symlink-based path traversal bypasses isSubpath guard

path.resolve() normalizes ./.. but does NOT resolve symlinks. A malicious extension can place a symlink inside its directory (e.g., context.md -> ~/.ssh/id_rsa) and declare it as a context file. The check isSubpath(extension.path, resolved) passes because the string path stays within the extension dir, but fs.readFile follows the symlink and reads arbitrary files.

The codebase already has realPathWithin() in packages/core/src/extension/gemini-converter.ts:134 which uses fs.realpathSync — the same function used in 5 other extension path confinement checks (gemini-converter.ts, claude-converter.ts). This PR should follow the established pattern.

Suggested change
const resolved = path.resolve(contextFilePath);
let resolved: string;
try {
resolved = fs.realpathSync(contextFilePath);
} catch {
onDebugMessage(
`Skipping unreadable context file: ${contextFilePath}`,
);
return null;
}
const realExtPath = fs.realpathSync(extension.path);
if (!isSubpath(realExtPath, resolved)) {

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — now using fs.realpath() to resolve symlinks before the isSubpath check, matching the pattern in gemini-converter.ts and claude-converter.ts.

if (!isSubpath(extension.path, resolved)) {
onDebugMessage(
`Skipping context file outside extension directory: ${contextFilePath}`,
);
return null;
}
return fs.readFile(resolved, 'utf-8');

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Missing AbortSignal in extension context file reads

signal (AbortSignal) is destructured from params (line 179) and is in scope. Both the MCP resource path (readMcpResource(ref.serverName, ref.uri, { signal }) at line 424) and the file read path (readManyFiles(config, { ... signal }) at line 586) propagate the abort signal for cancellation. But this fs.readFile call does not pass it.

When the user cancels a pending query (e.g., edits the input while extension context files are being read), the in-flight fs.readFile calls continue to completion — wasting disk I/O and memory for content that will be discarded.

Suggested change
return fs.readFile(resolved, 'utf-8');
return fs.readFile(resolved, { encoding: 'utf-8', signal });

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — now passing { encoding: 'utf-8', signal } to fs.readFile for extension context files, matching the MCP/file read paths.

}),
);

for (let j = 0; j < fileReads.length; j++) {
const outcome = fileReads[j];
if (outcome.status === 'rejected') {
onDebugMessage(
`Failed to read extension context file ${extension.contextFiles[j]}: ${getErrorMessage(outcome.reason)}`,
);
continue;
}
const content = outcome.value;
if (!content || !content.trim()) continue;
if (extensionContextBudgetRemaining <= 0) {
onDebugMessage(
`Extension context budget exhausted, skipping remaining files.`,
);
break;
}
const cap = Math.min(PER_FILE_CAP, extensionContextBudgetRemaining);
const cappedContent =
content.length > cap
? content.slice(0, cap) + '\n... (truncated)'
: content;
contextText += `\n\n${cappedContent}`;
extensionContextBudgetRemaining -= cappedContent.length;
}
}

extensionParts.push({ text: contextText });

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Hard-coded ext: prefix instead of using imported constant

This file already imports parseExtensionRef, matchExtensionByRef, and buildExtensionContextText from ./extension-mention-ref.js, but buildExtensionRef (which does exactly ${EXTENSION_REF_PREFIX}${extensionName}) is not imported. If EXTENSION_REF_PREFIX ever changes, this line silently diverges.

Suggested change
extensionParts.push({ text: contextText });
extensionLabels.push(buildExtensionRef(extension.name));

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — now using imported buildExtensionRef(extension.name) instead of hard-coded template literal.

extensionLabels.push(`ext:${extension.name}`);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extension.name is the machine-readable slug (alphanumeric + hyphens), not user-facing metadata. It doesn't go through terminal rendering. Sanitizing it would be inconsistent — we don't sanitize ext.name anywhere else in the codebase.

extensionDisplays.push({
callId,
name: 'Activate Extension',
description: `Activated extension ${displayName}`,
status: ToolCallStatus.Success,
resultDisplay: undefined,
confirmationDetails: undefined,
});
}

// Read files (if any). A hard read error aborts the turn, as before — but
// any resource tool-cards already gathered are still surfaced.
// any extension/resource tool-cards already gathered are still surfaced.
const fileParts: Part[] = [];
let fileDisplays: IndividualToolCallDisplay[] = [];
if (pathSpecsToRead.length > 0) {
Expand All @@ -477,7 +591,6 @@ export async function resolveAtCommandQuery({
? result.contentParts
: [result.contentParts];

// Create individual tool call displays for each file read
fileDisplays = result.files.map((file, index) => ({
callId: `client-read-${userMessageTimestamp}-${index}`,
name: file.isDirectory ? 'Read Directory' : 'Read File',
Expand All @@ -492,7 +605,6 @@ export async function resolveAtCommandQuery({
}));

if (parts.length > 0 && !result.error) {
// readManyFiles now returns properly formatted parts with headers and prefixes
for (const part of parts) {
fileParts.push(typeof part === 'string' ? { text: part } : part);
}
Expand All @@ -512,14 +624,19 @@ export async function resolveAtCommandQuery({
typeof errorToolCallDisplay.resultDisplay === 'string'
? errorToolCallDisplay.resultDisplay
: undefined;
// Resource labels are merged in too: a resource may have been read
// successfully before the file read failed, and its card is already in
// `resourceDisplays` above — the audit trail must not drop it.
const labelsOnError = [...contentLabelsForDisplay, ...resourceLabels];
const labelsOnError = [

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Missing test for error path with extension labels/displays

The error path now includes extensionLabels and extensionDisplays in the return value (this block), but atCommandProcessor.test.ts has zero tests involving extension mentions — the mock Config never sets up getActiveExtensions. A regression in the ordering (e.g., extensionDisplays placed after the error display instead of before, or extensionLabels dropped from filesRead) would go undetected.

Consider adding a test that mentions both an extension and a file, forces readManyFiles to throw, and asserts toolDisplays includes the extension display card and filesRead includes the extension label.

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point — adding integration tests to atCommandProcessor.test.ts with getActiveExtensions mock would be valuable but requires significant test infrastructure setup (mock extensions with context files, path resolution). Will consider as a follow-up. The unit tests on extension-mention-ref.ts cover the core logic paths.

...extensionLabels,
...contentLabelsForDisplay,
...resourceLabels,
];
return {
processedQuery: null,
shouldProceed: false,
toolDisplays: [...resourceDisplays, errorToolCallDisplay],
toolDisplays: [
...extensionDisplays,
...resourceDisplays,
errorToolCallDisplay,
],
filesRead: labelsOnError,
recording: {
filesRead: labelsOnError,
Expand All @@ -537,15 +654,20 @@ export async function resolveAtCommandQuery({
// positional alignment, so grouping is safe.
const processedQueryParts: PartListUnion = [
{ text: initialQueryText },
...extensionParts,
...fileParts,
...resourceParts,
];
const allLabels = [...contentLabelsForDisplay, ...resourceLabels];
const allLabels = [
...extensionLabels,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Session resume mislabels extension activations as "Read File"

allLabels includes extensionLabels (e.g., ext:browser) and is assigned to recording.filesRead. On session resume, resumeHistoryUtils maps every filesRead entry to "Read File" — producing "Read file ext:browser" instead of "Activated extension Browser."

Consider excluding extensionLabels from recording.filesRead since extensionDisplays are already persisted via toolDisplays and provide the correct labeling. Or tag entries with a type discriminator ({ kind: 'extension' | 'file' | 'mcp', label }).

— qwen3.7-max via Qwen Code /review

...contentLabelsForDisplay,
...resourceLabels,
];

return {
processedQuery: processedQueryParts,
shouldProceed: true,
toolDisplays: [...fileDisplays, ...resourceDisplays],
toolDisplays: [...extensionDisplays, ...fileDisplays, ...resourceDisplays],
filesRead: allLabels,
recording: {
filesRead: allLabels,
Expand Down
Loading
Loading