mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-05-17 12:21:10 +00:00
f5af7fbf95
* feat(memory): add autoSkill background project skill extraction
* fix(test): add missing mock methods for autoSkill (getAutoSkillEnabled, recordCompletedToolCall, consumePendingMemoryTaskPromises)
* fix(test): fix cross-platform path comparison in skillReviewNudge integration test
* fix(autoSkill): address critical review comments
- Fix merged_with_extract silent drop: remove broken merge optimization
in scheduleSkillReview(). When an extract task is pending/running,
skill review is now scheduled independently instead of recording
metadata that no production code ever reads.
- Fix SKILL_MANAGE blocked from skill review agent: prepareTools() now
only enforces the recursion guard (AGENT tool) when the agent has an
explicit tools list. Wildcard/inherit subagents still get the full
EXCLUDED_TOOLS_FOR_SUBAGENTS filter, preventing task subagents from
calling skill_manage. The dedicated skill review agent can now receive
the skill_manage tool it requires.
- Update manager.test.ts: replace merged_with_extract tests with
concurrent-extract independent-scheduling tests.
- Update skill-manage.test.ts: clarify test description to reflect
wildcard-only exclusion semantics.
* fix(autoSkill): reject symlink traversal in skill_manage path guard
assertProjectSkillPath() uses path.resolve() which is purely lexical and
does not dereference symlinks. If any path component inside .qwen/skills/
is a symlink pointing outside the project, fs.writeFile/readFile/rm would
follow the link and mutate files outside the advertised write boundary.
Add assertRealProjectSkillPath() (async) in skill-paths.ts that:
- Resolves the real path of the skills root via fs.realpath()
- Walks up from targetPath to find the nearest existing ancestor
- Resolves that ancestor to its real filesystem path
- Rejects if the real path falls outside the real skills root
skill-manage.ts execute() now calls both the cheap lexical check (fast
fail for obviously wrong paths) and the async real-path check before any
fs.writeFile / fs.rm mutation.
Add three symlink-specific tests in skill-paths.test.ts covering:
- Legitimate path accepted
- Symlinked directory pointing outside skills root rejected
- Skills root itself being a symlink (safe target) accepted
* refactor(autoSkill): remove skill_manage tool, use path-based skill write detection
Address reviewer feedback: instead of keeping skill_manage as the sole
write gate (which still had symlink bypass risk via generic tools), remove
the dedicated tool entirely and replace with a two-layer protection:
1. skillsModifiedInSession (client.ts): detects writes to .qwen/skills/
by inspecting the file_path arg of every completed tool call, replacing
the fragile historyCallsSkillManage() history scan.
2. hasAutoSkillSource + evaluateScopedDecision (skillReviewAgentPlanner.ts):
the review agent's permission sandbox now verifies BOTH that the target
path is inside the skills directory AND that the existing file already
contains 'source: auto-skill' in its frontmatter before allowing edits,
preventing the agent from overwriting user-managed skills.
Changes:
- Delete skill-manage.ts and skill-manage.test.ts
- Remove SKILL_MANAGE from ToolNames, ToolDisplayNames, config registerLazy,
agent-core EXCLUDED_TOOLS comment, and agent.ts comment
- Replace historyCallsSkillManage() with skillsModified: boolean param in
scheduleSkillReview; skip reason renamed skills_modified_in_session
- recordCompletedToolCall(name, filePath?) detects .qwen/skills/ writes;
CLI layers pass file_path arg from tool call request
- Fix buildTaskPrompt frontmatter template to use top-level source: auto-skill
- Update skill-paths.ts error messages to remove skill_manage references
- Update all unit/integration tests accordingly
* fix(autoSkill): deduplicate concurrent skill-review tasks per projectRoot
scheduleSkillReview() was launching a new background task every time the
threshold was reached for the same project, with no guard against multiple
in-flight reviews running concurrently.
Fix: add skillReviewInFlightByProject Map that tracks the taskId of any
running review per projectRoot. A second call while one is in-flight returns
{ status: 'skipped', skippedReason: 'already_running', taskId: <existing> }.
The map entry is cleared in a finally block inside runSkillReview() so the
next session can schedule a fresh review after the current one completes.
Also extend SkillReviewScheduleResult.skippedReason union to include
'already_running', and add a unit test covering the full lifecycle:
first call schedules, second call is skipped with existing taskId, and a
third call after completion schedules a new task.
* fix(autoSkill): address all critical review comments
1. hasAutoSkillSource: narrow catch to ENOENT only (EISDIR/EACCES etc.
return false to deny); tighten frontmatter regex to match opening block only.
2. evaluateScopedDecision: add explicit allow for READ_FILE and LS so they
don't fall to 'default' which the base PermissionManager might widen;
EDIT/WRITE_FILE now call assertRealProjectSkillPath() (async realpath guard)
in addition to the lexical check, closing the symlink traversal hole.
3. isScopedTool / getScopedDenyRule: cover READ_FILE and LS so hasRelevantRules
returns true and findMatchingDenyRule is correctly consulted for them.
4. recordCompletedToolCall (client.ts): broaden tool name set to match
WRITE_TOOL_NAMES in manager.ts (write_file, edit, replace, create_file) and
inspect all three arg keys (file_path, path, target_file). Signature changed
from (name, filePath?) to (name, args?) to carry all args through.
5. client.ts hardcoded literals: replace threshold/maxTurns/timeoutMs with the
named constants AUTO_SKILL_THRESHOLD / DEFAULT_AUTO_SKILL_MAX_TURNS /
DEFAULT_AUTO_SKILL_TIMEOUT_MS imported from manager.ts and
skillReviewAgentPlanner.ts.
6. toolCallCount / skillsModifiedInSession reset: only reset when skill review
is actually scheduled (status === 'scheduled'), not every turn, so the
counter correctly accumulates across turns within a session as per design doc.
7. runSkillReview (manager.ts): rethrow after marking record failed, consistent
with runExtract behavior.
8. skillReviewNudge.integration.test.ts test 5: rewrite to reflect the
in-flight dedup contract (second same-project call returns already_running
with existing taskId; third call after completion gets a new task). Add
vi.mock for runSkillReviewByAgent so the test does not need a full Config.
* fix(autoSkill): address all review comments
- skill-paths: detect dangling symlinks with lstat before treating ENOENT as safe
- skill-paths: fix isProjectSkillPath relative path resolution to use projectRoot
- skillReviewAgentPlanner: restrict READ_FILE/LS to project root only
- skillReviewAgentPlanner: remove SHELL tool from review agent tool list
- skillReviewAgentPlanner: add path import; remove unused shell imports
- skillReviewAgentPlanner: add comment for buildAgentHistory trailing user message
- client: fix runManagedAutoMemoryBackgroundTasks gate widening
- client: fix skillsModifiedInSession deadlock
- client: add .catch() to skill review promise
- client: hoist SKILL_WRITE_TOOL_NAMES to module-level ReadonlySet
- agent-core: use full EXCLUDED_TOOLS_FOR_SUBAGENTS for explicit tool list subagents
- manager: extend notify() signature to accept 'skill-review' taskType
- config: fix JSDoc default value comment (false, not true)
* fix(autoSkill): address second round review comments
- client: reset toolCallCount when scheduleSkillReview returns already_running
and count >= threshold, preventing immediate cascade after in-flight review
- client.test: add autoSkill branch tests (scheduled/already_running/skills_modified)
- client.test: add full recordCompletedToolCall unit tests (skillsModifiedInSession,
toolCallCount increment, skill path detection for write_file/edit/read_file)
- client.test: add scheduleSkillReview mock to mockMemoryManager
- nonInteractiveCli.test: add assertions for recordCompletedToolCall and
consumePendingMemoryTaskPromises in tool-call integration test
|
||
|---|---|---|
| .. | ||
| skill-nudge.md | ||