diff --git a/docs/users/features/commands.md b/docs/users/features/commands.md index 5ba31859c54..33a5ac26b1f 100644 --- a/docs/users/features/commands.md +++ b/docs/users/features/commands.md @@ -85,6 +85,7 @@ These commands invoke bundled skills that provide specialized workflows. | ------------ | ------------------------------------------------------------------- | ------------------------------------------------- | | `/review` | Review code changes with 5 parallel agents + deterministic analysis | `/review`, `/review 123`, `/review 123 --comment` | | `/loop` | Run a prompt on a recurring schedule | `/loop 5m check the build` | +| `/simplify` | Review recent changes and apply safe cleanup edits directly | `/simplify`, `/simplify focus on duplication` | | `/qc-helper` | Answer questions about Qwen Code usage and configuration | `/qc-helper how do I configure MCP?` | See [Code Review](./code-review.md) for full `/review` documentation. diff --git a/packages/cli/src/services/BundledSkillLoader.test.ts b/packages/cli/src/services/BundledSkillLoader.test.ts index a2bd4544c90..aebc297010f 100644 --- a/packages/cli/src/services/BundledSkillLoader.test.ts +++ b/packages/cli/src/services/BundledSkillLoader.test.ts @@ -142,6 +142,26 @@ describe('BundledSkillLoader', () => { expect(commands.map((c) => c.name)).toEqual(['review', 'deploy']); }); + it('should load simplify bundled skill like other slash commands', async () => { + const skills = [ + makeSkill({ + name: 'simplify', + description: 'Simplify recent changes', + filePath: '/bundled/simplify/SKILL.md', + body: 'Simplify body', + }), + ]; + mockSkillManager.listSkills.mockResolvedValue(skills); + + const loader = new BundledSkillLoader(mockConfig); + const commands = await loader.loadCommands(signal); + + expect(commands).toHaveLength(1); + expect(commands[0].name).toBe('simplify'); + expect(commands[0].description).toBe('Simplify recent changes'); + expect(commands[0].kind).toBe(CommandKind.SKILL); + }); + it('should resolve {{model}} template variable in skill body', async () => { const skill = makeSkill({ body: 'Review by {{model}} via Qwen Code', diff --git a/packages/core/src/skills/bundled/simplify/SKILL.md b/packages/core/src/skills/bundled/simplify/SKILL.md new file mode 100644 index 00000000000..cf8b3d4614c --- /dev/null +++ b/packages/core/src/skills/bundled/simplify/SKILL.md @@ -0,0 +1,123 @@ +--- +name: simplify +description: Review recent code changes for reuse, code quality, and efficiency, then directly apply straightforward cleanup improvements. Use when the user wants a post-implementation cleanup pass, pre-PR polish, or asks to simplify/refine recent changes. Invoke with `/simplify` or `/simplify `. +allowedTools: + - agent + - run_shell_command + - grep_search + - read_file + - write_file + - edit + - glob +--- + +# Simplify Recent Changes + +You are running a structured cleanup workflow over recent code changes. Your goal is not just to comment on the code, but to safely improve it. + +## Step 1: Identify the review scope + +Determine which files and changes to review. + +1. First inspect the current git state. +2. If there are staged changes, review against `HEAD` so both staged and unstaged tracked changes are included. +3. Otherwise review the current uncommitted diff. +4. If there is no git diff, fall back to `git ls-files --modified --others --exclude-standard` so the scope respects `.gitignore` (this keeps build output, `node_modules`, and other ignored paths out of the cleanup). +5. If that is still empty, fall back to files edited in this conversation. +6. If you still cannot identify a meaningful scope, stop and tell the user there are no recent changes to simplify. + +Preferred commands: + +- `git diff --name-only` +- `git diff --staged --name-only` +- `git diff HEAD --name-only` +- `git diff` +- `git diff HEAD` +- `git status --short` + +Use `git diff HEAD` whenever staged changes exist. Otherwise use `git diff`. + +## Step 2: Launch three review passes in parallel + +Use the `agent` tool and launch all review passes in a single response so they run concurrently. Each pass must receive the same review scope and diff command. These passes are read-only: each one inspects and reports findings only and must not modify files — all edits happen later in Step 4. + +Keep each review prompt short and focused. Do not paste the full diff into the prompt. Tell each pass to read the diff itself and inspect only files relevant to its findings. + +### Pass 1: Code Reuse Review + +Look for opportunities to reduce duplication and reuse existing code: + +- existing utilities or helpers that should be reused +- duplicated logic introduced in new code +- inline logic that should delegate to an existing abstraction +- ad-hoc helpers for string, path, env, parsing, or type checks when a project utility already exists + +### Pass 2: Code Quality Review + +Look for maintainability issues: + +- copy-paste variants that should be unified +- parameter sprawl or awkward APIs +- redundant state or indirection +- abstraction leaks +- stringly-typed code that should be modeled more clearly +- unnecessary nesting +- unnecessary comments that explain what instead of why +- naming or structure that does not match surrounding code + +### Pass 3: Efficiency Review + +Look for wasteful work and unnecessary overhead: + +- repeated work that can be memoized, cached, or removed +- serial work that can be parallelized safely +- unnecessary scans, allocations, reads, or traversals +- hot-path blocking work +- redundant no-op updates +- overly broad operations when a narrower one would work +- existence-check patterns that introduce TOCTOU style waste or risk + +## Step 3: Aggregate findings + +Wait for all three passes to finish, then merge overlapping findings. + +Prioritize fixes that are: + +- low risk +- local in scope +- clearly aligned with existing project patterns +- easy to validate with tests or targeted commands + +Do not force a cleanup if it would require speculative architectural changes. + +## Step 4: Apply straightforward improvements + +Directly implement safe cleanup improvements. + +Examples of good automatic fixes: + +- replace duplicated logic with an existing helper +- remove redundant code, but only after a repository-wide search confirms it has no remaining callers +- simplify conditionals or control flow +- tighten loops or repeated work +- reduce unnecessary state or wrapper code +- remove low-value comments +- align code with nearby conventions + +Skip items that are uncertain, risky, or too invasive. Do not spend time debating rejected findings; simply move on. + +## Step 5: Verify the cleanup + +After making changes: + +1. Run focused tests for the changed area when they exist. +2. Run the relevant project quality checks you can identify for the touched code. +3. If there are no applicable tests, at least run a targeted build, typecheck, or lint command that covers the edited files. + +Prefer targeted verification over whole-repo commands unless the project only exposes repo-wide checks. + +## Additional focus + +If the user supplied extra instructions after `/simplify`, treat them as additional review focus and prioritize them alongside the default dimensions. + +The raw user invocation appears below when present. Use it to extract any extra focus such as performance, duplication, rendering, API clarity, testability, or naming consistency. diff --git a/packages/core/src/skills/skill-manager.test.ts b/packages/core/src/skills/skill-manager.test.ts index 9dd23ed6232..a044a462638 100644 --- a/packages/core/src/skills/skill-manager.test.ts +++ b/packages/core/src/skills/skill-manager.test.ts @@ -667,21 +667,33 @@ Skill 3 content`); isSymbolicLink: () => false, }; + const simplifyDirEntry = { + name: 'simplify', + isDirectory: () => true, + isFile: () => false, + isSymbolicLink: () => false, + }; + const emptyDir = [] as unknown as Awaited>; function mockReaddirForLevels(levels: Set) { vi.mocked(fs.readdir).mockImplementation((dirPath) => { const pathStr = String(dirPath); - const isBundled = - pathStr.endsWith(bundledDirSegment) && !pathStr.includes('.qwen'); + const isBundled = pathStr.endsWith(bundledDirSegment); const isProject = pathStr.includes(projectDirSegment) && pathStr.startsWith(projectPrefix); const isUser = pathStr.includes(userDirSegment) && pathStr.startsWith(userPrefix); + if (levels.has('bundled') && isBundled) { + return Promise.resolve([ + reviewDirEntry, + simplifyDirEntry, + ] as unknown as Awaited>); + } + if ( - (levels.has('bundled') && isBundled) || (levels.has('project') && isProject) || (levels.has('user') && isUser) ) { @@ -689,21 +701,42 @@ Skill 3 content`); ReturnType >); } + return Promise.resolve(emptyDir); }); } function setupReviewSkillMocks() { vi.mocked(fs.access).mockResolvedValue(undefined); - vi.mocked(fs.readFile).mockResolvedValue(`--- + vi.mocked(fs.readFile).mockImplementation(async (filePath) => { + const pathStr = String(filePath); + if (pathStr.includes(`${path.sep}simplify${path.sep}`)) { + return `--- +name: simplify +description: Simplify recent changes +--- +Simplify content`; + } + + return `--- name: review description: Review code changes --- -Review content`); +Review content`; + }); + + mockParseYaml.mockImplementation((yamlString: string) => { + if (yamlString.includes('name: simplify')) { + return { + name: 'simplify', + description: 'Simplify recent changes', + }; + } - mockParseYaml.mockReturnValue({ - name: 'review', - description: 'Review code changes', + return { + name: 'review', + description: 'Review code changes', + }; }); } @@ -714,8 +747,11 @@ Review content`); const skills = await manager.listSkills({ force: true }); expect(skills.some((s) => s.name === 'review')).toBe(true); + expect(skills.some((s) => s.name === 'simplify')).toBe(true); const reviewSkill = skills.find((s) => s.name === 'review'); + const simplifySkill = skills.find((s) => s.name === 'simplify'); expect(reviewSkill!.level).toBe('bundled'); + expect(simplifySkill!.level).toBe('bundled'); }); it('should prioritize project-level over bundled skills with same name', async () => { @@ -727,6 +763,8 @@ Review content`); const reviewSkills = skills.filter((s) => s.name === 'review'); expect(reviewSkills).toHaveLength(1); expect(reviewSkills[0].level).toBe('project'); + // simplify has no name conflict, so it must still survive alongside the deduped review skill + expect(skills.some((s) => s.name === 'simplify')).toBe(true); }); it('should prioritize user-level over bundled skills with same name', async () => { @@ -738,6 +776,8 @@ Review content`); const reviewSkills = skills.filter((s) => s.name === 'review'); expect(reviewSkills).toHaveLength(1); expect(reviewSkills[0].level).toBe('user'); + // simplify has no name conflict, so it must still survive alongside the deduped review skill + expect(skills.some((s) => s.name === 'simplify')).toBe(true); }); it('should skip all skills in bare mode', async () => {