-
Notifications
You must be signed in to change notification settings - Fork 3k
feat(core): add simplify bundled skill #3570
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
Changes from all commits
4b3d602
990cece
dcd568b
17c5f7c
5243573
30230e1
195eb09
4cb90a7
c9307c6
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 | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -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 <focus>`. | ||||||||||||||||||
|
pomelo-nwu marked this conversation as resolved.
|
||||||||||||||||||
| 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. | ||||||||||||||||||
|
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] Step 1 includes staged changes in the auto-cleanup scope: "If there are staged changes, review against HEAD so both staged and unstaged tracked changes are included." A user who carefully curated their staging via
Suggested change
— deepseek-v4-pro via Qwen Code /review |
||||||||||||||||||
|
|
||||||||||||||||||
| 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 | ||||||||||||||||||
|
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] Step 2 launches 3 parallel sub-agents but provides no error handling for agent failures (timeout, crash, tool rejection). If an agent silently fails, the LLM may hang, skip findings, or hallucinate results with no diagnostic output. The
Suggested change
— deepseek-v4-pro via Qwen Code /review |
||||||||||||||||||
|
|
||||||||||||||||||
| 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. | ||||||||||||||||||
|
|
||||||||||||||||||
|
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] Sub-agents launched in Step 2 inherit full tool permissions ( The
Suggested change
— deepseek-v4-pro via Qwen Code /review |
||||||||||||||||||
| 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: | ||||||||||||||||||
|
pomelo-nwu marked this conversation as resolved.
|
||||||||||||||||||
|
|
||||||||||||||||||
| - 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 | ||||||||||||||||||
|
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] Step 4 directly modifies files via
Suggested change
— deepseek-v4-pro via Qwen Code /review |
||||||||||||||||||
|
|
||||||||||||||||||
| 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 | ||||||||||||||||||
|
|
||||||||||||||||||
|
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] "remove dead or redundant code" is listed as an example of a safe automatic fix, but LLMs cannot reliably identify dead code without full call-graph visibility. Code that has no callers within the diff scope may still be active elsewhere in the repository. This could lead to silent deletion of live code paths.
Suggested change
— deepseek-v4-pro via Qwen Code /review |
||||||||||||||||||
| Skip items that are uncertain, risky, or too invasive. Do not spend time debating rejected findings; simply move on. | ||||||||||||||||||
|
|
||||||||||||||||||
| ## Step 5: Verify the cleanup | ||||||||||||||||||
|
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] Step 5 instructs the agent to run tests/typecheck/lint after cleanup but does not specify what to do when verification fails — revert the changes, fix and retry, or just report to the user. This leaves the agent's behavior unpredictable on failure.
Suggested change
— deepseek-v4-pro via Qwen Code /review |
||||||||||||||||||
|
|
||||||||||||||||||
| 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. | ||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -667,43 +667,76 @@ Skill 3 content`); | |||||||||||||||||||||||||||||||||||
| isSymbolicLink: () => false, | ||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| const simplifyDirEntry = { | ||||||||||||||||||||||||||||||||||||
| name: 'simplify', | ||||||||||||||||||||||||||||||||||||
| isDirectory: () => true, | ||||||||||||||||||||||||||||||||||||
| isFile: () => false, | ||||||||||||||||||||||||||||||||||||
| isSymbolicLink: () => false, | ||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| const emptyDir = [] as unknown as Awaited<ReturnType<typeof fs.readdir>>; | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| function mockReaddirForLevels(levels: Set<string>) { | ||||||||||||||||||||||||||||||||||||
| 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<ReturnType<typeof fs.readdir>>); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||||||||||||
| (levels.has('bundled') && isBundled) || | ||||||||||||||||||||||||||||||||||||
| (levels.has('project') && isProject) || | ||||||||||||||||||||||||||||||||||||
| (levels.has('user') && isUser) | ||||||||||||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||||||||||||
| return Promise.resolve([reviewDirEntry] as unknown as Awaited< | ||||||||||||||||||||||||||||||||||||
| ReturnType<typeof fs.readdir> | ||||||||||||||||||||||||||||||||||||
| >); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| return Promise.resolve(emptyDir); | ||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| function setupReviewSkillMocks() { | ||||||||||||||||||||||||||||||||||||
| vi.mocked(fs.access).mockResolvedValue(undefined); | ||||||||||||||||||||||||||||||||||||
| vi.mocked(fs.readFile).mockResolvedValue(`--- | ||||||||||||||||||||||||||||||||||||
| vi.mocked(fs.readFile).mockImplementation(async (filePath) => { | ||||||||||||||||||||||||||||||||||||
|
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 Also: this mock pairs with the Recommend: a table-driven registry, e.g.
Suggested change
with a single — claude-opus-4-7 via Claude Code /qreview |
||||||||||||||||||||||||||||||||||||
| 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) => { | ||||||||||||||||||||||||||||||||||||
|
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] Recommend either delegate to
Suggested change
— claude-opus-4-7 via Claude Code /qreview |
||||||||||||||||||||||||||||||||||||
| 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 }); | ||||||||||||||||||||||||||||||||||||
|
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 cross-level priority tests ("should prioritize project-level over bundled" / "user-level over bundled") verify review deduplication but do not assert that If a future change causes all bundled skills to be dropped whenever a project-level skill exists (rather than per-name deduplication), these tests would not catch the regression.
Suggested change
— deepseek-v4-pro via Qwen Code /review |
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| 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 () => { | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
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] The new simplify loading test is missing the
listSkillscall assertion that existing similar tests include.Add after
expect(commands[0].kind).toBe(CommandKind.SKILL);:— deepseek-v4-pro via Qwen Code /review