Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
460 changes: 460 additions & 0 deletions docs/design/skill-nudge/skill-nudge.md

Large diffs are not rendered by default.

10 changes: 8 additions & 2 deletions packages/cli/src/config/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
NativeLspService,
isBareMode,
isToolEnabled,
type ConfigParameters,
} from '@qwen-code/qwen-code-core';
import { extensionsCommand } from '../commands/extensions.js';
import { hooksCommand } from '../commands/hooks.js';
Expand Down Expand Up @@ -1106,7 +1107,7 @@ export async function loadCliConfig(

const modelProvidersConfig = settings.modelProviders;

const config = new Config({
const configParams: ConfigParameters = {
sessionId,
sessionData,
embeddingModel: DEFAULT_QWEN_EMBEDDING_MODEL,
Expand Down Expand Up @@ -1231,6 +1232,9 @@ export async function loadCliConfig(
? false
: (settings.memory?.enableManagedAutoMemory ?? true),
enableManagedAutoDream: settings.memory?.enableManagedAutoDream ?? false,
enableAutoSkill: bareMode
? false
: (settings.memory?.enableAutoSkill ?? false),
fastModel: settings.fastModel || undefined,
// Use separated hooks if provided, otherwise fall back to merged hooks
userHooks: bareMode
Expand Down Expand Up @@ -1268,7 +1272,9 @@ export async function loadCliConfig(
: undefined,
}
: undefined,
});
};

const config = new Config(configParams);

if (lspEnabled) {
try {
Expand Down
10 changes: 10 additions & 0 deletions packages/cli/src/config/settingsSchema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1123,6 +1123,16 @@ const SETTINGS_SCHEMA = {
'Enable automatic consolidation (dream) of collected memories.',
showInDialog: false,
},
enableAutoSkill: {
type: 'boolean',
label: 'Enable Auto Skill',
category: 'Memory',
requiresRestart: false,
default: false,
description:
'Enable background review for reusable project skills after tool-heavy sessions.',
showInDialog: false,
},
},
},

Expand Down
3 changes: 3 additions & 0 deletions packages/cli/src/nonInteractiveCli.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,8 @@ describe('runNonInteractive', () => {

mockGeminiClient = {

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] TS2353: consumePendingMemoryTaskPromises 在 mock 对象类型中不存在。GeminiClient 新增了此方法(第 762 行),但 mock 类型未包含它,导致 tsc --noEmit 编译失败。

Suggested change
mockGeminiClient = {
consumePendingMemoryTaskPromises: vi.fn().mockReturnValue([]),
recordCompletedToolCall: vi.fn(),

— DeepSeek/deepseek-v4-pro via Qwen Code /review

sendMessageStream: vi.fn(),
consumePendingMemoryTaskPromises: vi.fn().mockReturnValue([]),
recordCompletedToolCall: vi.fn(),
getChatRecordingService: vi.fn(() => ({
initialize: vi.fn(),
recordMessage: vi.fn(),
Expand Down Expand Up @@ -154,6 +156,7 @@ describe('runNonInteractive', () => {
getCronScheduler: vi.fn().mockReturnValue(null),
setModelInvocableCommandsProvider: vi.fn(),
setModelInvocableCommandsExecutor: vi.fn(),
getAutoSkillEnabled: vi.fn().mockReturnValue(false),
getDisabledSlashCommands: vi.fn().mockReturnValue([]),
getBackgroundTaskRegistry: vi.fn().mockReturnValue({
setNotificationCallback: vi.fn(),
Expand Down
13 changes: 13 additions & 0 deletions packages/cli/src/nonInteractiveCli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -451,6 +451,9 @@ export async function runNonInteractive(
}

adapter.emitToolResult(finalRequestInfo, toolResponse);
config
.getGeminiClient()
.recordCompletedToolCall(finalRequestInfo.name);

if (toolResponse.responseParts) {
toolResponseParts.push(...toolResponse.responseParts);
Expand Down Expand Up @@ -599,6 +602,9 @@ export async function runNonInteractive(
}

adapter.emitToolResult(requestInfo, toolResponse);
config
.getGeminiClient()
.recordCompletedToolCall(requestInfo.name);

if (toolResponse.responseParts) {
itemToolResponseParts.push(...toolResponse.responseParts);
Expand Down Expand Up @@ -713,6 +719,13 @@ export async function runNonInteractive(
await new Promise((r) => setTimeout(r, 100));
}

const memoryTaskPromises = config
.getGeminiClient()
.consumePendingMemoryTaskPromises();
if (memoryTaskPromises.length > 0) {
await Promise.allSettled(memoryTaskPromises);
}

const metrics = uiTelemetryService.getMetrics();
const usage = computeUsageFromMetrics(metrics);
// Get stats for JSON format output
Expand Down
1 change: 1 addition & 0 deletions packages/cli/src/ui/hooks/useGeminiStream.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ const MockedGeminiClientClass = vi.hoisted(() =>
this.sendMessageStream = mockSendMessageStream;
this.addHistory = vi.fn();
this.consumePendingMemoryTaskPromises = vi.fn().mockReturnValue([]);
this.recordCompletedToolCall = vi.fn();
this.getChatRecordingService = vi.fn().mockReturnValue({
recordThought: vi.fn(),
initialize: vi.fn(),
Expand Down
4 changes: 4 additions & 0 deletions packages/cli/src/ui/hooks/useGeminiStream.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1705,6 +1705,10 @@ export const useGeminiStream = (
(t) => !t.request.isClientInitiated,
);

for (const toolCall of geminiTools) {
geminiClient?.recordCompletedToolCall(toolCall.request.name);
}

if (geminiTools.length === 0) {
return;
}
Expand Down
8 changes: 7 additions & 1 deletion packages/core/src/agents/runtime/agent-core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ import { type ContextState, templateString } from './agent-headless.js';
*/
export const EXCLUDED_TOOLS_FOR_SUBAGENTS: ReadonlySet<string> = new Set([
ToolNames.AGENT,
ToolNames.SKILL_MANAGE,
ToolNames.CRON_CREATE,
ToolNames.CRON_LIST,
ToolNames.CRON_DELETE,
Expand Down Expand Up @@ -334,7 +335,12 @@ export class AgentCore {
),
);
}
toolsList.push(...onlyInlineDecls);
// Also filter inline FunctionDeclaration[] passed directly in toolConfig.
toolsList.push(

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] 内联 FunctionDeclaration[](onlyInlineDecls)仅按 recursionGuardOnly(只含 AGENT)过滤,而显式字符串工具列表按完整的 excludedFromSubagents(含 CRON_CREATE、TASK_STOP、SEND_MESSAGE 等)过滤。如果通过内联方式传入控制面工具,将绕过子 agent 排除过滤器。建议对 onlyInlineDecls 使用相同的 excludedFromSubagents 集合,保持一致性。

— DeepSeek/deepseek-v4-pro via Qwen Code /review

...onlyInlineDecls.filter(
(d) => !(d.name && excludedFromSubagents.has(d.name)),
),
);
} else {
// Inherit all available tools by default when not specified.
toolsList.push(
Expand Down
12 changes: 12 additions & 0 deletions packages/core/src/config/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,8 @@ export interface ConfigParameters {
enableManagedAutoMemory?: boolean;
/** Enable managed auto-dream consolidation separately from extraction. Defaults to true. */
enableManagedAutoDream?: boolean;
/** Enable automatic project skill review after tool-heavy sessions. Defaults to true. */

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] JSDoc says Defaults to true. but the Config constructor (this.enableAutoSkill = params.enableAutoSkill ?? false), settingsSchema.ts (default: false), and loadCliConfig all default to false. The PR description also says「默认关闭」. The JSDoc is misleading.

Suggested change
/** Enable automatic project skill review after tool-heavy sessions. Defaults to true. */
/** Enable automatic project skill review after tool-heavy sessions. Defaults to false. */
enableAutoSkill?: boolean;

— deepseek-v4-pro via Qwen Code /review

enableAutoSkill?: boolean;
/**
* Lightweight model for background tasks (memory extraction, dream, /btw side questions).
* When set and valid for the current auth type, forked agents use this model instead of
Expand Down Expand Up @@ -690,6 +692,7 @@ export class Config {
private readonly defaultFileEncoding: FileEncodingType | undefined;
private readonly enableManagedAutoMemory: boolean;
private readonly enableManagedAutoDream: boolean;
private readonly enableAutoSkill: boolean;
private fastModel?: string;
private readonly disableAllHooks: boolean;
/** User-level hooks (always loaded regardless of trust) */
Expand Down Expand Up @@ -886,6 +889,7 @@ export class Config {
});
this.enableManagedAutoMemory = params.enableManagedAutoMemory ?? true;
this.enableManagedAutoDream = params.enableManagedAutoDream ?? false;
this.enableAutoSkill = params.enableAutoSkill ?? false;
this.fastModel = params.fastModel || undefined;
this.disableAllHooks = params.disableAllHooks ?? false;
// Store user and project hooks separately for proper source attribution
Expand Down Expand Up @@ -2128,6 +2132,10 @@ export class Config {
return this.enableManagedAutoDream && !this.getBareMode();
}

getAutoSkillEnabled(): boolean {
return this.enableAutoSkill && !this.getBareMode();
}

/**
* Return the MemoryManager instance created for this Config.
* Use this to share background-task state (registry, drainer) with memory
Expand Down Expand Up @@ -2630,6 +2638,10 @@ export class Config {
const { SkillTool } = await import('../tools/skill.js');
return new SkillTool(this);
});
await registerLazy(ToolNames.SKILL_MANAGE, async () => {
const { SkillManageTool } = await import('../tools/skill-manage.js');
return new SkillManageTool(this);
});
await registerLazy(ToolNames.LS, async () => {
const { LSTool } = await import('../tools/ls.js');
return new LSTool(this);
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/core/client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,7 @@ describe('Gemini Client (client.ts)', () => {
getArenaAgentClient: vi.fn().mockReturnValue(null),
getManagedAutoMemoryEnabled: vi.fn().mockReturnValue(true),
getMemoryManager: vi.fn().mockReturnValue(mockMemoryManager),
getAutoSkillEnabled: vi.fn().mockReturnValue(false),
getDisableAllHooks: vi.fn().mockReturnValue(true),
getArenaManager: vi.fn().mockReturnValue(null),
getMessageBus: vi.fn().mockReturnValue(undefined),
Expand Down
46 changes: 41 additions & 5 deletions packages/core/src/core/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ const EMPTY_RELEVANT_AUTO_MEMORY_RESULT: RelevantAutoMemoryPromptResult = {
export class GeminiClient {
private chat?: GeminiChat;
private sessionTurnCount = 0;
private toolCallCount = 0;
private readonly surfacedRelevantAutoMemoryPaths = new Set<string>();

private readonly loopDetector: LoopDetectionService;
Expand Down Expand Up @@ -513,18 +514,45 @@ export class GeminiClient {
private runManagedAutoMemoryBackgroundTasks(
messageType: SendMessageType,
): void {
if (messageType !== SendMessageType.UserQuery) {
return;
}

if (!this.config.getManagedAutoMemoryEnabled()) {
if (
messageType !== SendMessageType.UserQuery &&

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] runManagedAutoMemoryBackgroundTasks gate expanded from UserQuery-only to UserQuery || ToolResult, but the gate change applies to the entire method — not just the autoSkill block. This causes scheduleExtract() and scheduleDream() to fire on every tool result submission, not just on user-initiated turns.

Each call triggers getHistory() (deep clone of entire conversation) and file I/O checks. A session with 20 tool calls now triggers 20 extract+dream attempts instead of 1.

Suggested change
messageType !== SendMessageType.UserQuery &&
private runManagedAutoMemoryBackgroundTasks(
messageType: SendMessageType,
): void {
// autoSkill: can trigger on UserQuery or ToolResult
if (
messageType === SendMessageType.UserQuery ||
messageType === SendMessageType.ToolResult
) {
if (this.config.getAutoSkillEnabled()) {
// ... scheduleSkillReview logic (existing)
}
}
// extract/dream: keep UserQuery-only to preserve existing behavior
if (messageType !== SendMessageType.UserQuery) return;
if (!this.config.getManagedAutoMemoryEnabled()) return;
// ... scheduleExtract, scheduleDream (existing)
}

— deepseek-v4-pro via Qwen Code /review

messageType !== SendMessageType.ToolResult
) {
return;
}

const projectRoot = this.config.getProjectRoot();
const sessionId = this.config.getSessionId();
const history = this.getHistory();
const mgr = this.config.getMemoryManager();
const autoSkillEnabled = this.config.getAutoSkillEnabled();

if (autoSkillEnabled) {
const skillReviewResult = mgr.scheduleSkillReview({
projectRoot,
sessionId,
history,
config: this.config,
toolCallCount: this.toolCallCount,
enabled: autoSkillEnabled,
threshold: 20,

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] threshold: 20, maxTurns: 8, timeoutMs: 120_000 are hardcoded literals at this call site, shadowing the named constants that are exported from manager.ts:187 (AUTO_SKILL_THRESHOLD) and skillReviewAgentPlanner.ts:26-27 (DEFAULT_AUTO_SKILL_MAX_TURNS, DEFAULT_AUTO_SKILL_TIMEOUT_MS). A maintainer who tunes AUTO_SKILL_THRESHOLD and runs the test suite (which uses the constant directly) sees green tests but ships a behavior bug because production reads the literal here.

Also enabled: autoSkillEnabled is dead — the call site is already inside if (autoSkillEnabled) { ... } two lines above, so this field is always true at this point.

Fix: Drop the four explicit fields and let scheduleSkillReview use its built-in defaults; or import the constants and pass them by reference. Either way, do not mix.

— claude-opus-4-7 via Qwen Code /review

maxTurns: 8,
timeoutMs: 120_000,
});
if (skillReviewResult.promise) {
this.pendingMemoryTaskPromises.push(
skillReviewResult.promise.then((record) => {
const touched = record.metadata?.['touchedSkillFiles'];
return Array.isArray(touched) ? touched.length : 0;
}),
);
}
}
this.toolCallCount = 0;

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] toolCallCount = 0 and skillsModifiedInSession = false are reset every turn (at the end of runManagedAutoMemoryBackgroundTasks, which fires per UserQuery/ToolResult). The PR description and design doc (docs/design/skill-nudge/skill-nudge.md) explicitly say the counter is session-scoped and only resets at session start. The implementation makes it turn-scoped.

Two observable consequences:

  1. A single 20-tool-call turn fires immediately (matches the threshold, but feels surprising while the user is still in the session).
  2. A multi-turn session totaling 200 tool calls (none ≥ 20 in a single turn) never fires — contrary to the design intent.

Also, skillsModifiedInSession reset semantics: if the user edits a skill file in turn N (the gate at runManagedAutoMemoryBackgroundTasks skips that turn but then resets the flag), then turn N+1 with 20 unrelated tool calls will fire a review even though the user just hand-modified a skill.

Fix: Either (a) move the reset to a real session-end / /clear hook to match the design, or (b) update the design doc to per-turn semantics and rename the field to skillsModifiedThisTurn. Whichever the team picks, pin it in a test.

— claude-opus-4-7 via Qwen Code /review


if (!this.config.getManagedAutoMemoryEnabled()) {
return;
}

const extractPromise = mgr
.scheduleExtract({
Expand Down Expand Up @@ -581,6 +609,14 @@ export class GeminiClient {
return promises;
}

recordCompletedToolCall(toolName: string): void {
if (toolName === 'skill_manage') {
this.toolCallCount = 0;
return;
}
this.toolCallCount += 1;
}

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] toolCallCount increments for all completed tool calls, including read-only operations (list_directory, read_file, grep_search). A simple "read one file and edit one line" task can produce 5-10 tool calls. In practice, the threshold of 20 is reached much sooner than the design doc's intended "tool-heavy session" semantics suggest.

Consider weighting write operations higher, or counting only non-read-only tools.

— deepseek-v4-pro via Qwen Code /review

async *sendMessageStream(
request: PartListUnion,
signal: AbortSignal,
Expand Down
Loading
Loading