feat: add review-work and ai-slop-remover as built-in skills, add remove-ai-slops command
Embed user-level skills into the plugin's built-in system so they ship with the product rather than requiring per-user configuration. - review-work: 5-agent parallel post-implementation review orchestrator - ai-slop-remover: per-file AI-generated code smell detector and remover - /remove-ai-slops: command that orchestrates parallel ai-slop-remover runs
This commit is contained in:
@@ -8,6 +8,7 @@ export const BuiltinCommandNameSchema = z.enum([
|
|||||||
"refactor",
|
"refactor",
|
||||||
"start-work",
|
"start-work",
|
||||||
"stop-continuation",
|
"stop-continuation",
|
||||||
|
"remove-ai-slops",
|
||||||
])
|
])
|
||||||
|
|
||||||
export type BuiltinCommandName = z.infer<typeof BuiltinCommandNameSchema>
|
export type BuiltinCommandName = z.infer<typeof BuiltinCommandNameSchema>
|
||||||
|
|||||||
@@ -1,6 +1,7 @@
|
|||||||
import { describe, test, expect } from "bun:test"
|
import { describe, test, expect } from "bun:test"
|
||||||
import { loadBuiltinCommands } from "./commands"
|
import { loadBuiltinCommands } from "./commands"
|
||||||
import { HANDOFF_TEMPLATE } from "./templates/handoff"
|
import { HANDOFF_TEMPLATE } from "./templates/handoff"
|
||||||
|
import { REMOVE_AI_SLOPS_TEMPLATE } from "./templates/remove-ai-slops"
|
||||||
import type { BuiltinCommandName } from "./types"
|
import type { BuiltinCommandName } from "./types"
|
||||||
|
|
||||||
describe("loadBuiltinCommands", () => {
|
describe("loadBuiltinCommands", () => {
|
||||||
@@ -60,6 +61,77 @@ describe("loadBuiltinCommands", () => {
|
|||||||
})
|
})
|
||||||
})
|
})
|
||||||
|
|
||||||
|
describe("loadBuiltinCommands — remove-ai-slops", () => {
|
||||||
|
test("should include remove-ai-slops command in loaded commands", () => {
|
||||||
|
//#given
|
||||||
|
const disabledCommands: BuiltinCommandName[] = []
|
||||||
|
|
||||||
|
//#when
|
||||||
|
const commands = loadBuiltinCommands(disabledCommands)
|
||||||
|
|
||||||
|
//#then
|
||||||
|
expect(commands["remove-ai-slops"]).toBeDefined()
|
||||||
|
expect(commands["remove-ai-slops"].name).toBe("remove-ai-slops")
|
||||||
|
})
|
||||||
|
|
||||||
|
test("should exclude remove-ai-slops when disabled", () => {
|
||||||
|
//#given
|
||||||
|
const disabledCommands: BuiltinCommandName[] = ["remove-ai-slops"]
|
||||||
|
|
||||||
|
//#when
|
||||||
|
const commands = loadBuiltinCommands(disabledCommands)
|
||||||
|
|
||||||
|
//#then
|
||||||
|
expect(commands["remove-ai-slops"]).toBeUndefined()
|
||||||
|
})
|
||||||
|
|
||||||
|
test("should include remove-ai-slops template content in command template", () => {
|
||||||
|
//#given - no disabled commands
|
||||||
|
|
||||||
|
//#when
|
||||||
|
const commands = loadBuiltinCommands()
|
||||||
|
|
||||||
|
//#then
|
||||||
|
expect(commands["remove-ai-slops"].template).toContain(REMOVE_AI_SLOPS_TEMPLATE)
|
||||||
|
})
|
||||||
|
|
||||||
|
test("should have correct description for remove-ai-slops", () => {
|
||||||
|
//#given - no disabled commands
|
||||||
|
|
||||||
|
//#when
|
||||||
|
const commands = loadBuiltinCommands()
|
||||||
|
|
||||||
|
//#then
|
||||||
|
expect(commands["remove-ai-slops"].description).toContain("AI-generated code smells")
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
|
describe("REMOVE_AI_SLOPS_TEMPLATE", () => {
|
||||||
|
test("should include phase structure", () => {
|
||||||
|
//#given - the template string
|
||||||
|
|
||||||
|
//#when / #then
|
||||||
|
expect(REMOVE_AI_SLOPS_TEMPLATE).toContain("Identify Changed Files")
|
||||||
|
expect(REMOVE_AI_SLOPS_TEMPLATE).toContain("Parallel AI Slop Removal")
|
||||||
|
expect(REMOVE_AI_SLOPS_TEMPLATE).toContain("Critical Review")
|
||||||
|
})
|
||||||
|
|
||||||
|
test("should reference ai-slop-remover skill", () => {
|
||||||
|
//#given - the template string
|
||||||
|
|
||||||
|
//#when / #then
|
||||||
|
expect(REMOVE_AI_SLOPS_TEMPLATE).toContain("ai-slop-remover")
|
||||||
|
})
|
||||||
|
|
||||||
|
test("should include safety verification checklist", () => {
|
||||||
|
//#given - the template string
|
||||||
|
|
||||||
|
//#when / #then
|
||||||
|
expect(REMOVE_AI_SLOPS_TEMPLATE).toContain("Safety Verification")
|
||||||
|
expect(REMOVE_AI_SLOPS_TEMPLATE).toContain("Behavior Preservation")
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
describe("HANDOFF_TEMPLATE", () => {
|
describe("HANDOFF_TEMPLATE", () => {
|
||||||
test("should include session reading instruction", () => {
|
test("should include session reading instruction", () => {
|
||||||
//#given - the template string
|
//#given - the template string
|
||||||
|
|||||||
@@ -6,6 +6,7 @@ import { STOP_CONTINUATION_TEMPLATE } from "./templates/stop-continuation"
|
|||||||
import { REFACTOR_TEMPLATE } from "./templates/refactor"
|
import { REFACTOR_TEMPLATE } from "./templates/refactor"
|
||||||
import { START_WORK_TEMPLATE } from "./templates/start-work"
|
import { START_WORK_TEMPLATE } from "./templates/start-work"
|
||||||
import { HANDOFF_TEMPLATE } from "./templates/handoff"
|
import { HANDOFF_TEMPLATE } from "./templates/handoff"
|
||||||
|
import { REMOVE_AI_SLOPS_TEMPLATE } from "./templates/remove-ai-slops"
|
||||||
|
|
||||||
const BUILTIN_COMMAND_DEFINITIONS: Record<BuiltinCommandName, Omit<CommandDefinition, "name">> = {
|
const BUILTIN_COMMAND_DEFINITIONS: Record<BuiltinCommandName, Omit<CommandDefinition, "name">> = {
|
||||||
"init-deep": {
|
"init-deep": {
|
||||||
@@ -77,6 +78,16 @@ $ARGUMENTS
|
|||||||
template: `<command-instruction>
|
template: `<command-instruction>
|
||||||
${STOP_CONTINUATION_TEMPLATE}
|
${STOP_CONTINUATION_TEMPLATE}
|
||||||
</command-instruction>`,
|
</command-instruction>`,
|
||||||
|
},
|
||||||
|
"remove-ai-slops": {
|
||||||
|
description: "(builtin) Remove AI-generated code smells from branch changes and critically review the results",
|
||||||
|
template: `<command-instruction>
|
||||||
|
${REMOVE_AI_SLOPS_TEMPLATE}
|
||||||
|
</command-instruction>
|
||||||
|
|
||||||
|
<user-request>
|
||||||
|
$ARGUMENTS
|
||||||
|
</user-request>`,
|
||||||
},
|
},
|
||||||
handoff: {
|
handoff: {
|
||||||
description: "(builtin) Create a detailed context summary for continuing work in a new session",
|
description: "(builtin) Create a detailed context summary for continuing work in a new session",
|
||||||
|
|||||||
@@ -0,0 +1,89 @@
|
|||||||
|
export const REMOVE_AI_SLOPS_TEMPLATE = `# Remove AI Slops Command
|
||||||
|
|
||||||
|
## What this command does
|
||||||
|
Analyzes all files changed in the current branch (compared to parent commit), removes AI-generated code smells in parallel, then critically reviews the changes to ensure safety and behavior preservation. Fixes any issues found during review.
|
||||||
|
|
||||||
|
## Step 0: Task Planning
|
||||||
|
|
||||||
|
Use TodoWrite to create the task list:
|
||||||
|
1. Get changed files from branch
|
||||||
|
2. Run ai-slop-remover on each file in parallel
|
||||||
|
3. Critically review all changes
|
||||||
|
4. Fix any issues found
|
||||||
|
|
||||||
|
## Role Definition
|
||||||
|
You are a senior code quality engineer specialized in identifying and removing AI-generated code patterns while preserving original functionality. You have deep expertise in code review, refactoring safety, and behavioral preservation.
|
||||||
|
|
||||||
|
## Process
|
||||||
|
|
||||||
|
### Phase 1: Identify Changed Files
|
||||||
|
Execute the following command to get all changed files in the current branch:
|
||||||
|
\\\`\\\`\\\`bash
|
||||||
|
git diff $(git merge-base main HEAD)..HEAD --name-only
|
||||||
|
\\\`\\\`\\\`
|
||||||
|
|
||||||
|
### Phase 2: Parallel AI Slop Removal
|
||||||
|
For each changed file, spawn an agent in parallel using the Task tool with the ai-slop-remover skill:
|
||||||
|
|
||||||
|
\\\`\\\`\\\`
|
||||||
|
task(category="quick", load_skills=["ai-slop-remover"], run_in_background=true, description="Remove AI slops from {filename}", prompt="Remove AI slops from: {file_path}")
|
||||||
|
\\\`\\\`\\\`
|
||||||
|
|
||||||
|
**CRITICAL**: Launch ALL agents in a SINGLE message with multiple Task tool calls for maximum parallelism.
|
||||||
|
|
||||||
|
### Phase 3: Critical Review
|
||||||
|
After all ai-slop-remover agents complete, perform a critical review with the following checklist:
|
||||||
|
|
||||||
|
**Safety Verification**:
|
||||||
|
- [ ] No functional logic was accidentally removed
|
||||||
|
- [ ] All error handling is preserved
|
||||||
|
- [ ] Type hints remain correct and complete
|
||||||
|
- [ ] Import statements are still valid
|
||||||
|
- [ ] No breaking changes to public APIs
|
||||||
|
|
||||||
|
**Behavior Preservation**:
|
||||||
|
- [ ] Return values unchanged
|
||||||
|
- [ ] Side effects unchanged
|
||||||
|
- [ ] Exception behavior unchanged
|
||||||
|
- [ ] Edge case handling preserved
|
||||||
|
|
||||||
|
**Code Quality**:
|
||||||
|
- [ ] Removed changes are genuinely AI slop (not intentional patterns)
|
||||||
|
- [ ] Remaining code follows project conventions
|
||||||
|
- [ ] No orphaned code or dead references
|
||||||
|
|
||||||
|
### Phase 4: Fix Issues
|
||||||
|
If any issues are found during critical review:
|
||||||
|
1. Identify the specific problem
|
||||||
|
2. Explain why it's a problem
|
||||||
|
3. Use git checkout to revert the changes from ai-slop-remover
|
||||||
|
4. If remaining ai-slops are found after reverting, remove them by editing the file yourself - with parallel tool calls, per-file
|
||||||
|
5. Verify the fix doesn't introduce new issues
|
||||||
|
|
||||||
|
## Output Format
|
||||||
|
|
||||||
|
### Summary Report
|
||||||
|
\\\`\\\`\\\`
|
||||||
|
## AI Slop Removal Summary
|
||||||
|
|
||||||
|
### Files Processed
|
||||||
|
- file1.py: X changes
|
||||||
|
- file2.py: Y changes
|
||||||
|
|
||||||
|
### Critical Review Results
|
||||||
|
- Safety: PASS/FAIL
|
||||||
|
- Behavior: PASS/FAIL
|
||||||
|
- Quality: PASS/FAIL
|
||||||
|
|
||||||
|
### Issues Found & Fixed
|
||||||
|
1. [Issue description] -> [Fix applied]
|
||||||
|
|
||||||
|
### Final Status
|
||||||
|
[CLEAN / ISSUES FIXED / REQUIRES ATTENTION]
|
||||||
|
\\\`\\\`\\\`
|
||||||
|
|
||||||
|
## Quality Assurance
|
||||||
|
- NEVER remove code that serves a functional purpose
|
||||||
|
- ALWAYS verify changes compile/parse correctly
|
||||||
|
- ALWAYS preserve test coverage
|
||||||
|
- If uncertain about a change, err on the side of keeping the original code`
|
||||||
@@ -1,6 +1,6 @@
|
|||||||
import type { CommandDefinition } from "../claude-code-command-loader"
|
import type { CommandDefinition } from "../claude-code-command-loader"
|
||||||
|
|
||||||
export type BuiltinCommandName = "init-deep" | "ralph-loop" | "cancel-ralph" | "ulw-loop" | "refactor" | "start-work" | "stop-continuation" | "handoff"
|
export type BuiltinCommandName = "init-deep" | "ralph-loop" | "cancel-ralph" | "ulw-loop" | "refactor" | "start-work" | "stop-continuation" | "handoff" | "remove-ai-slops"
|
||||||
|
|
||||||
export interface BuiltinCommandConfig {
|
export interface BuiltinCommandConfig {
|
||||||
disabled_commands?: BuiltinCommandName[]
|
disabled_commands?: BuiltinCommandName[]
|
||||||
|
|||||||
@@ -61,7 +61,7 @@ describe("createBuiltinSkills", () => {
|
|||||||
expect(agentBrowserSkill!.template).toContain("agent-browser snapshot")
|
expect(agentBrowserSkill!.template).toContain("agent-browser snapshot")
|
||||||
})
|
})
|
||||||
|
|
||||||
test("always includes frontend-ui-ux and git-master skills", () => {
|
test("always includes frontend-ui-ux, git-master, review-work, and ai-slop-remover skills", () => {
|
||||||
// given - both provider options
|
// given - both provider options
|
||||||
|
|
||||||
// when
|
// when
|
||||||
@@ -72,10 +72,12 @@ describe("createBuiltinSkills", () => {
|
|||||||
for (const skills of [defaultSkills, agentBrowserSkills]) {
|
for (const skills of [defaultSkills, agentBrowserSkills]) {
|
||||||
expect(skills.find((s) => s.name === "frontend-ui-ux")).toBeDefined()
|
expect(skills.find((s) => s.name === "frontend-ui-ux")).toBeDefined()
|
||||||
expect(skills.find((s) => s.name === "git-master")).toBeDefined()
|
expect(skills.find((s) => s.name === "git-master")).toBeDefined()
|
||||||
|
expect(skills.find((s) => s.name === "review-work")).toBeDefined()
|
||||||
|
expect(skills.find((s) => s.name === "ai-slop-remover")).toBeDefined()
|
||||||
}
|
}
|
||||||
})
|
})
|
||||||
|
|
||||||
test("returns exactly 4 skills regardless of provider", () => {
|
test("returns exactly 6 skills regardless of provider", () => {
|
||||||
// given
|
// given
|
||||||
|
|
||||||
// when
|
// when
|
||||||
@@ -83,8 +85,8 @@ describe("createBuiltinSkills", () => {
|
|||||||
const agentBrowserSkills = createBuiltinSkills({ browserProvider: "agent-browser" })
|
const agentBrowserSkills = createBuiltinSkills({ browserProvider: "agent-browser" })
|
||||||
|
|
||||||
// then
|
// then
|
||||||
expect(defaultSkills).toHaveLength(4)
|
expect(defaultSkills).toHaveLength(6)
|
||||||
expect(agentBrowserSkills).toHaveLength(4)
|
expect(agentBrowserSkills).toHaveLength(6)
|
||||||
})
|
})
|
||||||
|
|
||||||
test("should exclude playwright when it is in disabledSkills", () => {
|
test("should exclude playwright when it is in disabledSkills", () => {
|
||||||
@@ -99,7 +101,9 @@ describe("createBuiltinSkills", () => {
|
|||||||
expect(skills.map((s) => s.name)).toContain("frontend-ui-ux")
|
expect(skills.map((s) => s.name)).toContain("frontend-ui-ux")
|
||||||
expect(skills.map((s) => s.name)).toContain("git-master")
|
expect(skills.map((s) => s.name)).toContain("git-master")
|
||||||
expect(skills.map((s) => s.name)).toContain("dev-browser")
|
expect(skills.map((s) => s.name)).toContain("dev-browser")
|
||||||
expect(skills.length).toBe(3)
|
expect(skills.map((s) => s.name)).toContain("review-work")
|
||||||
|
expect(skills.map((s) => s.name)).toContain("ai-slop-remover")
|
||||||
|
expect(skills.length).toBe(5)
|
||||||
})
|
})
|
||||||
|
|
||||||
test("should exclude multiple skills when they are in disabledSkills", () => {
|
test("should exclude multiple skills when they are in disabledSkills", () => {
|
||||||
@@ -114,13 +118,15 @@ describe("createBuiltinSkills", () => {
|
|||||||
expect(skills.map((s) => s.name)).not.toContain("git-master")
|
expect(skills.map((s) => s.name)).not.toContain("git-master")
|
||||||
expect(skills.map((s) => s.name)).toContain("frontend-ui-ux")
|
expect(skills.map((s) => s.name)).toContain("frontend-ui-ux")
|
||||||
expect(skills.map((s) => s.name)).toContain("dev-browser")
|
expect(skills.map((s) => s.name)).toContain("dev-browser")
|
||||||
expect(skills.length).toBe(2)
|
expect(skills.map((s) => s.name)).toContain("review-work")
|
||||||
|
expect(skills.map((s) => s.name)).toContain("ai-slop-remover")
|
||||||
|
expect(skills.length).toBe(4)
|
||||||
})
|
})
|
||||||
|
|
||||||
test("should return an empty array when all skills are disabled", () => {
|
test("should return an empty array when all skills are disabled", () => {
|
||||||
// #given
|
// #given
|
||||||
const options = {
|
const options = {
|
||||||
disabledSkills: new Set(["playwright", "frontend-ui-ux", "git-master", "dev-browser"]),
|
disabledSkills: new Set(["playwright", "frontend-ui-ux", "git-master", "dev-browser", "review-work", "ai-slop-remover"]),
|
||||||
}
|
}
|
||||||
|
|
||||||
// #when
|
// #when
|
||||||
@@ -138,7 +144,39 @@ describe("createBuiltinSkills", () => {
|
|||||||
const skills = createBuiltinSkills(options)
|
const skills = createBuiltinSkills(options)
|
||||||
|
|
||||||
// #then
|
// #then
|
||||||
expect(skills.length).toBe(4)
|
expect(skills.length).toBe(6)
|
||||||
|
})
|
||||||
|
|
||||||
|
test("review-work skill has correct structure", () => {
|
||||||
|
// #given - default options
|
||||||
|
|
||||||
|
// #when
|
||||||
|
const skills = createBuiltinSkills()
|
||||||
|
const reviewWork = skills.find((s) => s.name === "review-work")
|
||||||
|
|
||||||
|
// #then
|
||||||
|
expect(reviewWork).toBeDefined()
|
||||||
|
expect(reviewWork!.description).toContain("review")
|
||||||
|
expect(reviewWork!.template).toContain("5-Agent Parallel Review Orchestrator")
|
||||||
|
expect(reviewWork!.template).toContain("Goal & Constraint Verification")
|
||||||
|
expect(reviewWork!.template).toContain("QA")
|
||||||
|
expect(reviewWork!.template).toContain("Code Quality")
|
||||||
|
expect(reviewWork!.template).toContain("Security")
|
||||||
|
expect(reviewWork!.template).toContain("Context Mining")
|
||||||
|
})
|
||||||
|
|
||||||
|
test("ai-slop-remover skill has correct structure", () => {
|
||||||
|
// #given - default options
|
||||||
|
|
||||||
|
// #when
|
||||||
|
const skills = createBuiltinSkills()
|
||||||
|
const aiSlopRemover = skills.find((s) => s.name === "ai-slop-remover")
|
||||||
|
|
||||||
|
// #then
|
||||||
|
expect(aiSlopRemover).toBeDefined()
|
||||||
|
expect(aiSlopRemover!.description).toContain("AI-generated code smells")
|
||||||
|
expect(aiSlopRemover!.template).toContain("DETECTION CRITERIA")
|
||||||
|
expect(aiSlopRemover!.template).toContain("SAFETY RULES")
|
||||||
})
|
})
|
||||||
|
|
||||||
test("returns playwright-cli skill when browserProvider is 'playwright-cli'", () => {
|
test("returns playwright-cli skill when browserProvider is 'playwright-cli'", () => {
|
||||||
|
|||||||
@@ -8,6 +8,8 @@ import {
|
|||||||
frontendUiUxSkill,
|
frontendUiUxSkill,
|
||||||
gitMasterSkill,
|
gitMasterSkill,
|
||||||
devBrowserSkill,
|
devBrowserSkill,
|
||||||
|
reviewWorkSkill,
|
||||||
|
aiSlopRemoverSkill,
|
||||||
} from "./skills/index"
|
} from "./skills/index"
|
||||||
|
|
||||||
export interface CreateBuiltinSkillsOptions {
|
export interface CreateBuiltinSkillsOptions {
|
||||||
@@ -27,7 +29,7 @@ export function createBuiltinSkills(options: CreateBuiltinSkillsOptions = {}): B
|
|||||||
browserSkill = playwrightSkill
|
browserSkill = playwrightSkill
|
||||||
}
|
}
|
||||||
|
|
||||||
const skills = [browserSkill, frontendUiUxSkill, gitMasterSkill, devBrowserSkill]
|
const skills = [browserSkill, frontendUiUxSkill, gitMasterSkill, devBrowserSkill, reviewWorkSkill, aiSlopRemoverSkill]
|
||||||
|
|
||||||
if (!disabledSkills) {
|
if (!disabledSkills) {
|
||||||
return skills
|
return skills
|
||||||
|
|||||||
@@ -0,0 +1,145 @@
|
|||||||
|
import type { BuiltinSkill } from "../types"
|
||||||
|
|
||||||
|
export const aiSlopRemoverSkill: BuiltinSkill = {
|
||||||
|
name: "ai-slop-remover",
|
||||||
|
description:
|
||||||
|
"Removes AI-generated code smells from a SINGLE file while preserving functionality. For multiple files, call in PARALLEL per file.",
|
||||||
|
template: `You are an expert code refactorer specializing in removing AI-generated "slop" patterns while STRICTLY preserving functionality.
|
||||||
|
|
||||||
|
**INPUT**: Exactly ONE file path. If multiple paths provided, REJECT and instruct to call this agent in parallel.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## DETECTION CRITERIA (Specific)
|
||||||
|
|
||||||
|
### 1. Obvious Comments (EXCLUDE: BDD comments like #given, #when, #then, #when/then)
|
||||||
|
|
||||||
|
**REMOVE**:
|
||||||
|
- Comments restating the code: \`x += 1 # increment x\`
|
||||||
|
- Docstrings on trivial methods: \`"""Returns the name."""\` for \`def get_name(): return self.name\`
|
||||||
|
- Section dividers: \`# ===== HELPER FUNCTIONS =====\`
|
||||||
|
- Commented-out code blocks
|
||||||
|
- \`# TODO: future enhancement\` without concrete plan
|
||||||
|
- \`# Note: this is important\` without explaining WHY
|
||||||
|
|
||||||
|
**KEEP**:
|
||||||
|
- Comments explaining WHY (business logic, edge cases, workarounds)
|
||||||
|
- Links to issues/tickets: \`# See SPR-1234\`
|
||||||
|
- Non-obvious algorithm explanations
|
||||||
|
- Regex explanations
|
||||||
|
- Matches to existing code style
|
||||||
|
|
||||||
|
### 2. Over-Defensive Code
|
||||||
|
|
||||||
|
**REMOVE**:
|
||||||
|
- Null checks for values that CANNOT be None (e.g., Django request in view)
|
||||||
|
- \`if x is not None and x.attr is not None:\` when x is guaranteed
|
||||||
|
- Try-except around code that can't raise (e.g., dict literal access)
|
||||||
|
- \`isinstance()\` checks for statically typed parameters
|
||||||
|
- Default values for required parameters: \`def foo(x: str = "")\` when empty string is invalid
|
||||||
|
- Backward-compat shims: \`_old_name = new_name # deprecated\`
|
||||||
|
- \`# removed\` or \`# deleted\` comments for removed code
|
||||||
|
- Re-exports of unused items
|
||||||
|
- Verbose, duplicated, or redundant code / test cases
|
||||||
|
|
||||||
|
**KEEP**:
|
||||||
|
- Validation at system boundaries (user input, external API responses)
|
||||||
|
- Error handling for I/O operations
|
||||||
|
- Null checks for nullable DB fields
|
||||||
|
- assertions in test code to matching type expectations
|
||||||
|
|
||||||
|
### 3. Spaghetti Nesting (2+ levels deep)
|
||||||
|
|
||||||
|
**REFACTOR**:
|
||||||
|
- Nested if-else chains -> early returns / guard clauses
|
||||||
|
- \`if x: if y: if z:\` -> \`if not x: return\` / \`if not y: return\`
|
||||||
|
- Nested loops with conditionals -> extract to helper OR use comprehensions
|
||||||
|
- Complex ternary \`a if b else (c if d else e)\` -> explicit if-else
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## PROCESS
|
||||||
|
|
||||||
|
### Step 1: Read & Analyze
|
||||||
|
Read the file. Identify ALL slop instances with line numbers.
|
||||||
|
|
||||||
|
### Step 2: Deep Consideration (CRITICAL)
|
||||||
|
For EACH identified issue, think:
|
||||||
|
- **Functionality Impact**: Will removing this change behavior? If ANY doubt, SKIP.
|
||||||
|
- **Test Coverage**: Are there tests that might break? If uncertain, SKIP.
|
||||||
|
- **Context Dependency**: Is this "slop" actually necessary for this specific codebase? (e.g., defensive code for known flaky external API)
|
||||||
|
- **Readability Trade-off**: Will removal make code LESS readable? If yes, SKIP.
|
||||||
|
|
||||||
|
**RULE**: When in doubt, DO NOT CHANGE. False negatives are better than breaking code.
|
||||||
|
|
||||||
|
### Step 3: Execute Changes
|
||||||
|
Make changes using Edit tool. One logical change at a time.
|
||||||
|
|
||||||
|
### Step 4: Detailed Report
|
||||||
|
|
||||||
|
**OUTPUT FORMAT**:
|
||||||
|
|
||||||
|
\`\`\`
|
||||||
|
## AI Slop Removed: {filename}
|
||||||
|
|
||||||
|
### Analysis Summary
|
||||||
|
- Total issues found: N
|
||||||
|
- Issues fixed: M
|
||||||
|
- Issues skipped (safety): K
|
||||||
|
|
||||||
|
### Changes Made
|
||||||
|
|
||||||
|
#### Change 1: [Category] Line X-Y
|
||||||
|
**Before**: [original code snippet]
|
||||||
|
**After**: [modified code snippet]
|
||||||
|
**Why this is slop**: [Explain why this pattern is problematic]
|
||||||
|
**Why safe to remove**: [Explain why functionality is preserved]
|
||||||
|
**Impact**: None - purely cosmetic improvement
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Skipped Issues (Preserved for Safety)
|
||||||
|
|
||||||
|
#### Skipped 1: Line X
|
||||||
|
**Reason**: [Why you chose not to change this]
|
||||||
|
|
||||||
|
### Summary
|
||||||
|
- Removed N obvious comments
|
||||||
|
- Simplified M defensive patterns
|
||||||
|
- Flattened K nested structures
|
||||||
|
- Preserved L patterns that looked like slop but serve purpose
|
||||||
|
\`\`\`
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## SAFETY RULES
|
||||||
|
|
||||||
|
1. **NEVER remove error handling for I/O, network, or file operations**
|
||||||
|
2. **NEVER simplify validation for user input or external data**
|
||||||
|
3. **NEVER change public API signatures**
|
||||||
|
4. **NEVER remove type hints (even redundant-looking ones)**
|
||||||
|
5. **If a pattern appears in multiple places, it might be intentional - ASK before bulk removal**
|
||||||
|
6. **Preserve all BDD test comments (#given, #when, #then)**
|
||||||
|
|
||||||
|
When finished, your report should be detailed enough that a reviewer can understand EXACTLY what changed and feel confident the changes are safe.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## WHEN NO SLOP FOUND
|
||||||
|
|
||||||
|
If the file is clean, report:
|
||||||
|
|
||||||
|
\`\`\`
|
||||||
|
## AI Slop Analysis: {filename}
|
||||||
|
|
||||||
|
### Result: No AI Slop Detected
|
||||||
|
|
||||||
|
This file is clean. Here's why:
|
||||||
|
|
||||||
|
**Comments**: N comments found, all explain WHY not WHAT
|
||||||
|
**Defensive Code**: Null checks present are appropriate (e.g., checks external API response)
|
||||||
|
**Code Structure**: Maximum nesting depth acceptable, early returns used appropriately
|
||||||
|
|
||||||
|
**Conclusion**: This code appears to be human-written or well-reviewed AI code. No changes needed.
|
||||||
|
\`\`\``,
|
||||||
|
}
|
||||||
@@ -3,3 +3,5 @@ export { playwrightCliSkill } from "./playwright-cli"
|
|||||||
export { frontendUiUxSkill } from "./frontend-ui-ux"
|
export { frontendUiUxSkill } from "./frontend-ui-ux"
|
||||||
export { gitMasterSkill } from "./git-master"
|
export { gitMasterSkill } from "./git-master"
|
||||||
export { devBrowserSkill } from "./dev-browser"
|
export { devBrowserSkill } from "./dev-browser"
|
||||||
|
export { reviewWorkSkill } from "./review-work"
|
||||||
|
export { aiSlopRemoverSkill } from "./ai-slop-remover"
|
||||||
|
|||||||
@@ -0,0 +1,536 @@
|
|||||||
|
import type { BuiltinSkill } from "../types"
|
||||||
|
|
||||||
|
export const reviewWorkSkill: BuiltinSkill = {
|
||||||
|
name: "review-work",
|
||||||
|
description:
|
||||||
|
"Post-implementation review orchestrator. Launches 5 parallel background sub-agents: Oracle (goal/constraint verification), Oracle (code quality), Oracle (security), unspecified-high (hands-on QA execution), unspecified-high (context mining from GitHub/git/Slack/Notion). All must pass for review to pass. MUST USE after completing any significant implementation work. Triggers: 'review work', 'review my work', 'review changes', 'QA my work', 'verify implementation', 'check my work', 'validate changes', 'post-implementation review'.",
|
||||||
|
template: `# Review Work — 5-Agent Parallel Review Orchestrator
|
||||||
|
|
||||||
|
Launch 5 specialized sub-agents in parallel to review completed implementation work from every angle. All 5 must pass for the review to pass. If even ONE fails, the review fails.
|
||||||
|
|
||||||
|
The 5 agents cover complementary concerns — together they form a comprehensive review that no single reviewer could match:
|
||||||
|
|
||||||
|
| # | Agent | Type | Role | Focus Level |
|
||||||
|
|---|-------|------|------|-------------|
|
||||||
|
| 1 | Goal Verifier | Oracle | Did we build what was asked? | MAIN |
|
||||||
|
| 2 | QA Executor | unspecified-high | Does it actually work? | MAIN |
|
||||||
|
| 3 | Code Reviewer | Oracle | Is the code well-written? | MAIN |
|
||||||
|
| 4 | Security Auditor | Oracle | Is it secure? | SUB |
|
||||||
|
| 5 | Context Miner | unspecified-high | Did we miss any context? | MAIN |
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Phase 0: Gather Review Context
|
||||||
|
|
||||||
|
Before launching agents, collect these inputs. Extract from conversation history first — the user's original request, constraints discussed, and decisions made are usually already in the thread. Only ask if truly missing.
|
||||||
|
|
||||||
|
<required_inputs>
|
||||||
|
|
||||||
|
- **GOAL**: The original objective. What was the user trying to achieve? Pull from the initial request in this conversation.
|
||||||
|
- **CONSTRAINTS**: Rules, requirements, or limitations. Tech stack restrictions, performance targets, API contracts, design patterns to follow, backward compatibility needs.
|
||||||
|
- **BACKGROUND**: Why this work was needed. Business context, user stories, related systems, prior decisions that informed the approach.
|
||||||
|
- **CHANGED_FILES**: Auto-collect via \`git diff --name-only HEAD~1\` or against the appropriate base (branch point, specific commit).
|
||||||
|
- **DIFF**: Auto-collect via \`git diff HEAD~1\` or against the appropriate base.
|
||||||
|
- **FILE_CONTENTS**: Read the full content of each changed file (not just the diff). Oracle agents cannot read files — they need full context in the prompt.
|
||||||
|
- **RUN_COMMAND**: How to start/run the application. Check \`package.json\` scripts, \`Makefile\`, \`docker-compose.yml\`, or ask the user.
|
||||||
|
|
||||||
|
</required_inputs>
|
||||||
|
|
||||||
|
|
||||||
|
**NEVER CHECKOUT A PR BRANCH IN THE MAIN WORKTREE. ALWAYS CREATE A NEW GIT WORKTREE (\`git worktree add\`) AND WORK THERE. THIS PREVENTS CONTAMINATING THE USER'S WORKING DIRECTORY WITH UNRELATED BRANCH STATE.**
|
||||||
|
|
||||||
|
**Auto-collection sequence:**
|
||||||
|
|
||||||
|
\`\`\`bash
|
||||||
|
# 1. Get changed files
|
||||||
|
git diff --name-only HEAD~1 # or: git diff --name-only main...HEAD
|
||||||
|
|
||||||
|
# 2. Get diff
|
||||||
|
git diff HEAD~1 # or: git diff main...HEAD
|
||||||
|
|
||||||
|
# 3. Detect run command
|
||||||
|
# Check package.json -> "scripts.dev" or "scripts.start"
|
||||||
|
# Check Makefile -> default target
|
||||||
|
# Check docker-compose.yml -> services
|
||||||
|
\`\`\`
|
||||||
|
|
||||||
|
For GOAL, CONSTRAINTS, BACKGROUND — review the full conversation history. The user's original message almost always contains the goal. Constraints often emerge during discussion. If anything critical is ambiguous, ask ONE focused question — not a checklist.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Phase 1: Launch 5 Agents
|
||||||
|
|
||||||
|
Launch ALL 5 in a single turn. Every agent uses \`run_in_background=true\`. No sequential launches. No waiting between them.
|
||||||
|
|
||||||
|
**Oracle agents receive everything in the prompt** (they cannot read files or run commands). Include DIFF + FILE_CONTENTS + all context directly in the prompt text.
|
||||||
|
|
||||||
|
**unspecified-high agents are autonomous** — they can read files, run commands, and use tools. Give them goals and pointers, not raw content dumps.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Agent 1: Goal & Constraint Verification (Oracle) — MAIN
|
||||||
|
|
||||||
|
This agent answers: "Did we build exactly what was asked, within the rules we were given?"
|
||||||
|
|
||||||
|
\`\`\`
|
||||||
|
task(
|
||||||
|
subagent_type="oracle",
|
||||||
|
run_in_background=true,
|
||||||
|
load_skills=[],
|
||||||
|
description="Verify implementation against original goal and constraints",
|
||||||
|
prompt="""
|
||||||
|
<review_type>GOAL & CONSTRAINT VERIFICATION</review_type>
|
||||||
|
|
||||||
|
<original_goal>
|
||||||
|
{GOAL — paste the user's original request and any clarifications}
|
||||||
|
</original_goal>
|
||||||
|
|
||||||
|
<constraints>
|
||||||
|
{CONSTRAINTS — every rule, requirement, or limitation discussed}
|
||||||
|
</constraints>
|
||||||
|
|
||||||
|
<background>
|
||||||
|
{BACKGROUND — why this work was needed, broader context}
|
||||||
|
</background>
|
||||||
|
|
||||||
|
<changed_files>
|
||||||
|
{CHANGED_FILES — list of modified file paths}
|
||||||
|
</changed_files>
|
||||||
|
|
||||||
|
<file_contents>
|
||||||
|
{FILE_CONTENTS — full content of every changed file, clearly delimited per file}
|
||||||
|
</file_contents>
|
||||||
|
|
||||||
|
<diff>
|
||||||
|
{DIFF — the actual git diff}
|
||||||
|
</diff>
|
||||||
|
|
||||||
|
Review whether this implementation correctly and completely achieves the stated goal within the given constraints. Be obsessively thorough — the point of this review is to catch what the implementer missed.
|
||||||
|
|
||||||
|
REVIEW CHECKLIST:
|
||||||
|
|
||||||
|
1. **Goal Completeness**: Break the goal into every sub-requirement (explicit AND implied). For each, mark ACHIEVED / MISSED / PARTIAL. Missing even one implied requirement that a reasonable engineer would have addressed = PARTIAL at minimum.
|
||||||
|
|
||||||
|
2. **Constraint Compliance**: List every constraint. For each, verify compliance with specific code evidence. A constraint violated = automatic FAIL.
|
||||||
|
|
||||||
|
3. **Requirement Gaps**: Requirements the user clearly wanted but didn't spell out. Things implied by the goal or background that a thoughtful engineer would have included.
|
||||||
|
|
||||||
|
4. **Over-Engineering**: Anything added that wasn't requested — unnecessary abstractions, extra features, premature optimizations, speculative generality. Flag these as scope creep.
|
||||||
|
|
||||||
|
5. **Edge Cases**: Given the goal, what inputs or scenarios would break this? Trace through at least 5 edge cases mentally.
|
||||||
|
|
||||||
|
6. **Behavioral Correctness**: Walk through the code logic for 3+ representative scenarios. Does the code actually produce the expected behavior in each case?
|
||||||
|
|
||||||
|
OUTPUT FORMAT:
|
||||||
|
<verdict>PASS or FAIL</verdict>
|
||||||
|
<confidence>HIGH / MEDIUM / LOW</confidence>
|
||||||
|
<summary>1-3 sentence overall assessment</summary>
|
||||||
|
<goal_breakdown>
|
||||||
|
For each sub-requirement:
|
||||||
|
- [ACHIEVED/MISSED/PARTIAL] Requirement description
|
||||||
|
- Evidence: specific code reference or gap
|
||||||
|
</goal_breakdown>
|
||||||
|
<constraint_compliance>
|
||||||
|
For each constraint:
|
||||||
|
- [ACHIEVED/MISSED] Constraint description — evidence
|
||||||
|
</constraint_compliance>
|
||||||
|
<findings>
|
||||||
|
- [PASS/FAIL/WARN] Category: Description
|
||||||
|
- File: path (line range if applicable)
|
||||||
|
- Evidence: specific code or logic reference
|
||||||
|
</findings>
|
||||||
|
<blocking_issues>Issues that MUST be fixed. Empty if PASS.</blocking_issues>
|
||||||
|
""")
|
||||||
|
\`\`\`
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Agent 2: QA via App Execution (unspecified-high) — MAIN
|
||||||
|
|
||||||
|
This agent answers: "Does it actually work when you run it?"
|
||||||
|
|
||||||
|
The QA agent follows a structured process: brainstorm scenarios exhaustively first, then self-review and augment, then create a task list, then execute systematically.
|
||||||
|
|
||||||
|
\`\`\`
|
||||||
|
task(
|
||||||
|
category="unspecified-high",
|
||||||
|
run_in_background=true,
|
||||||
|
load_skills=["playwright", "dev-browser"],
|
||||||
|
description="QA by actually running and using the application",
|
||||||
|
prompt="""
|
||||||
|
<review_type>QA — HANDS-ON APP EXECUTION</review_type>
|
||||||
|
|
||||||
|
<original_goal>
|
||||||
|
{GOAL}
|
||||||
|
</original_goal>
|
||||||
|
|
||||||
|
<constraints>
|
||||||
|
{CONSTRAINTS}
|
||||||
|
</constraints>
|
||||||
|
|
||||||
|
<changed_files>
|
||||||
|
{CHANGED_FILES}
|
||||||
|
</changed_files>
|
||||||
|
|
||||||
|
<run_command>
|
||||||
|
{RUN_COMMAND — how to start the application, or "unknown" if not determined}
|
||||||
|
</run_command>
|
||||||
|
|
||||||
|
You are a QA engineer. Your job is to RUN the application and verify it works through hands-on testing. You do not review code — you test behavior.
|
||||||
|
|
||||||
|
MANDATORY PROCESS (follow in order):
|
||||||
|
|
||||||
|
### Step 1: Scenario Brainstorm
|
||||||
|
|
||||||
|
Before touching the app, write down EVERY test scenario you can think of. Be exhaustive. Think about:
|
||||||
|
|
||||||
|
- **Happy paths**: The primary use cases this implementation enables. What's the main thing the user wanted to do?
|
||||||
|
- **Boundary conditions**: Empty inputs, maximum-length inputs, zero values, negative numbers, special characters, unicode, very large datasets.
|
||||||
|
- **Error paths**: Invalid inputs, network failures, missing files, permission denied, timeout conditions.
|
||||||
|
- **Regression scenarios**: Existing features that touch the same code paths. Things that worked before and must still work.
|
||||||
|
- **State transitions**: What happens when you do things out of order? Rapid repeated actions? Concurrent usage?
|
||||||
|
- **UX scenarios** (if applicable): Layout on different sizes, keyboard navigation, screen reader compatibility, loading states, error messages.
|
||||||
|
- **Integration points**: Does this feature interact with external services, databases, or other modules? Test those boundaries.
|
||||||
|
|
||||||
|
Write each scenario as a one-liner with expected behavior. Aim for 15-30 scenarios minimum.
|
||||||
|
|
||||||
|
### Step 2: Scenario Augmentation
|
||||||
|
|
||||||
|
Review your scenario list with fresh eyes. For each scenario, ask:
|
||||||
|
- "What could go wrong here that I haven't considered?"
|
||||||
|
- "What would a malicious or careless user do?"
|
||||||
|
- "What environmental conditions could affect this?" (disk full, slow network, expired tokens)
|
||||||
|
|
||||||
|
Add at least 5 more scenarios from this reflection. Group scenarios by priority: P0 (must pass), P1 (should pass), P2 (nice to pass).
|
||||||
|
|
||||||
|
### Step 3: Create Task List
|
||||||
|
|
||||||
|
Convert your augmented scenario list into a structured task list (use TaskCreate/TaskUpdate or your todo system). Each task = one test scenario with:
|
||||||
|
- Test name
|
||||||
|
- Steps to execute
|
||||||
|
- Expected result
|
||||||
|
- Priority (P0/P1/P2)
|
||||||
|
|
||||||
|
### Step 4: Execute Systematically
|
||||||
|
|
||||||
|
Work through the task list in priority order (P0 first). For each test:
|
||||||
|
|
||||||
|
1. Execute the test steps
|
||||||
|
2. Record actual result
|
||||||
|
3. Compare with expected result
|
||||||
|
4. Mark PASS or FAIL
|
||||||
|
5. If FAIL: capture evidence (screenshot, terminal output, error message)
|
||||||
|
6. Mark the task complete
|
||||||
|
|
||||||
|
**Execution guidance by app type:**
|
||||||
|
- **Web app**: Use playwright/dev-browser to navigate, click, fill forms, verify visual output.
|
||||||
|
- **CLI tool**: Run commands with various arguments, pipe inputs, check exit codes and output.
|
||||||
|
- **Library/SDK**: Write and execute a test script that imports and exercises the public API.
|
||||||
|
- **Backend API**: Use curl/httpie to hit endpoints with various payloads, verify response codes and bodies.
|
||||||
|
- **Mobile/Desktop**: If not directly runnable, write integration tests and execute them.
|
||||||
|
|
||||||
|
If the app cannot be started (build failure), that's an immediate FAIL — no need to continue.
|
||||||
|
|
||||||
|
### Step 5: Compile Results
|
||||||
|
|
||||||
|
OUTPUT FORMAT:
|
||||||
|
<verdict>PASS or FAIL</verdict>
|
||||||
|
<confidence>HIGH / MEDIUM / LOW</confidence>
|
||||||
|
<summary>1-3 sentence overall assessment</summary>
|
||||||
|
<scenario_coverage>
|
||||||
|
Total scenarios: N
|
||||||
|
P0: X tested, Y passed
|
||||||
|
P1: X tested, Y passed
|
||||||
|
P2: X tested, Y passed
|
||||||
|
</scenario_coverage>
|
||||||
|
<test_results>
|
||||||
|
For each test:
|
||||||
|
- [PASS/FAIL] Test name (Priority)
|
||||||
|
- Steps: What you did
|
||||||
|
- Expected: What should happen
|
||||||
|
- Actual: What actually happened
|
||||||
|
- Evidence: Screenshot path or terminal output snippet (if FAIL)
|
||||||
|
</test_results>
|
||||||
|
<blocking_issues>P0 or P1 failures only. Empty if PASS.</blocking_issues>
|
||||||
|
""")
|
||||||
|
\`\`\`
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Agent 3: Code Quality Review (Oracle) — MAIN
|
||||||
|
|
||||||
|
This agent answers: "Is the code well-written, maintainable, and consistent with the codebase?"
|
||||||
|
|
||||||
|
\`\`\`
|
||||||
|
task(
|
||||||
|
subagent_type="oracle",
|
||||||
|
run_in_background=true,
|
||||||
|
load_skills=[],
|
||||||
|
description="Review overall code quality, patterns, and architecture",
|
||||||
|
prompt="""
|
||||||
|
<review_type>CODE QUALITY REVIEW</review_type>
|
||||||
|
|
||||||
|
<changed_files>
|
||||||
|
{CHANGED_FILES}
|
||||||
|
</changed_files>
|
||||||
|
|
||||||
|
<file_contents>
|
||||||
|
{FILE_CONTENTS — full content of changed files AND neighboring files that show existing patterns}
|
||||||
|
</file_contents>
|
||||||
|
|
||||||
|
<diff>
|
||||||
|
{DIFF}
|
||||||
|
</diff>
|
||||||
|
|
||||||
|
<background>
|
||||||
|
{BACKGROUND}
|
||||||
|
</background>
|
||||||
|
|
||||||
|
You are a senior staff engineer conducting a code review. Your standard: "Would I approve this PR without comments?"
|
||||||
|
|
||||||
|
REVIEW DIMENSIONS (examine each):
|
||||||
|
|
||||||
|
1. **Correctness**: Logic errors, off-by-one, null/undefined handling, race conditions, resource leaks, unhandled promise rejections.
|
||||||
|
|
||||||
|
2. **Pattern Consistency**: Does new code follow the codebase's established patterns? Compare with the neighboring files provided. Introducing a new pattern where one already exists = finding.
|
||||||
|
|
||||||
|
3. **Naming & Readability**: Clear variable/function/type names? Self-documenting code? Would another engineer understand this without explanation?
|
||||||
|
|
||||||
|
4. **Error Handling**: Errors properly caught, logged, and propagated? No empty catch blocks? No swallowed errors? User-facing errors are helpful?
|
||||||
|
|
||||||
|
5. **Type Safety**: Any \`as any\`, \`@ts-ignore\`, \`@ts-expect-error\`? Proper generic usage? Correct type narrowing? (If TypeScript/typed language)
|
||||||
|
|
||||||
|
6. **Performance**: N+1 queries? Unnecessary re-renders? Blocking I/O on hot paths? Memory leaks? Unbounded growth?
|
||||||
|
|
||||||
|
7. **Abstraction Level**: Right level of abstraction? No copy-paste duplication? But also no premature over-abstraction?
|
||||||
|
|
||||||
|
8. **Testing**: New behaviors covered by tests? Tests are meaningful, not just coverage padding? Test names describe scenarios?
|
||||||
|
|
||||||
|
9. **API Design**: Public interfaces clean and consistent with existing APIs? Breaking changes flagged?
|
||||||
|
|
||||||
|
10. **Tech Debt**: Does this introduce new tech debt? Or create coupling that will be painful to change?
|
||||||
|
|
||||||
|
Categorize each finding by severity:
|
||||||
|
- **CRITICAL**: Will cause bugs, data loss, or crashes in production
|
||||||
|
- **MAJOR**: Significant quality issue that should be fixed before merge
|
||||||
|
- **MINOR**: Improvement worth making but not blocking
|
||||||
|
- **NITPICK**: Style preference, optional
|
||||||
|
|
||||||
|
OUTPUT FORMAT:
|
||||||
|
<verdict>PASS or FAIL</verdict>
|
||||||
|
<confidence>HIGH / MEDIUM / LOW</confidence>
|
||||||
|
<summary>1-3 sentence overall assessment</summary>
|
||||||
|
<findings>
|
||||||
|
- [CRITICAL/MAJOR/MINOR/NITPICK] Category: Description
|
||||||
|
- File: path (line range)
|
||||||
|
- Current: what the code does now
|
||||||
|
- Suggestion: how to improve
|
||||||
|
</findings>
|
||||||
|
<blocking_issues>CRITICAL and MAJOR items only. Empty if PASS.</blocking_issues>
|
||||||
|
""")
|
||||||
|
\`\`\`
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Agent 4: Security Review (Oracle) — SUB
|
||||||
|
|
||||||
|
This agent answers: "Are there security vulnerabilities in these changes?"
|
||||||
|
|
||||||
|
This is supplementary — it focuses exclusively on security. It does NOT comment on code style, architecture, or functionality unless those directly create a security risk.
|
||||||
|
|
||||||
|
\`\`\`
|
||||||
|
task(
|
||||||
|
subagent_type="oracle",
|
||||||
|
run_in_background=true,
|
||||||
|
load_skills=[],
|
||||||
|
description="Security-focused review of implementation changes",
|
||||||
|
prompt="""
|
||||||
|
<review_type>SECURITY REVIEW (supplementary)</review_type>
|
||||||
|
|
||||||
|
<changed_files>
|
||||||
|
{CHANGED_FILES}
|
||||||
|
</changed_files>
|
||||||
|
|
||||||
|
<file_contents>
|
||||||
|
{FILE_CONTENTS — full content of changed files}
|
||||||
|
</file_contents>
|
||||||
|
|
||||||
|
<diff>
|
||||||
|
{DIFF}
|
||||||
|
</diff>
|
||||||
|
|
||||||
|
You are a security engineer. Review this diff exclusively for security vulnerabilities and anti-patterns. Ignore code style, naming, architecture — unless it directly creates a security risk.
|
||||||
|
|
||||||
|
SECURITY CHECKLIST:
|
||||||
|
|
||||||
|
1. **Input Validation**: User inputs sanitized? SQL injection, XSS, command injection, SSRF vectors?
|
||||||
|
2. **Auth & AuthZ**: Authentication checks where needed? Authorization verified for each action? Privilege escalation paths?
|
||||||
|
3. **Secrets & Credentials**: Hardcoded secrets, API keys, tokens in code or config? Secrets in logs?
|
||||||
|
4. **Data Exposure**: Sensitive data in logs? PII in error messages? Over-exposed API responses?
|
||||||
|
5. **Dependencies**: New dependencies added? Known CVEs? Suspicious or unnecessary packages?
|
||||||
|
6. **Cryptography**: Proper algorithms? No custom crypto? Secure random? Proper key management?
|
||||||
|
7. **File & Path**: Path traversal? Unsafe file operations? Symlink following?
|
||||||
|
8. **Network**: CORS configured correctly? Rate limiting? TLS enforced? Certificate validation?
|
||||||
|
9. **Error Leakage**: Stack traces exposed to users? Internal details in error responses?
|
||||||
|
10. **Supply Chain**: Lockfile updated consistently? Dependency pinning?
|
||||||
|
|
||||||
|
OUTPUT FORMAT:
|
||||||
|
<verdict>PASS or FAIL</verdict>
|
||||||
|
<severity>CRITICAL / HIGH / MEDIUM / LOW / NONE</severity>
|
||||||
|
<summary>1-3 sentence overall assessment</summary>
|
||||||
|
<findings>
|
||||||
|
- [CRITICAL/HIGH/MEDIUM/LOW] Category: Description
|
||||||
|
- File: path (line range)
|
||||||
|
- Risk: What could an attacker do?
|
||||||
|
- Remediation: Specific fix
|
||||||
|
</findings>
|
||||||
|
<blocking_issues>CRITICAL and HIGH items only. Empty if PASS.</blocking_issues>
|
||||||
|
""")
|
||||||
|
\`\`\`
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Agent 5: Context Mining (unspecified-high) — MAIN
|
||||||
|
|
||||||
|
This agent answers: "Did we miss any context that should have informed this implementation?"
|
||||||
|
|
||||||
|
\`\`\`
|
||||||
|
task(
|
||||||
|
category="unspecified-high",
|
||||||
|
run_in_background=true,
|
||||||
|
load_skills=["git-master"],
|
||||||
|
description="Mine all accessible contexts for missed requirements or background knowledge",
|
||||||
|
prompt="""
|
||||||
|
<review_type>CONTEXT MINING — MISSED REQUIREMENTS & BACKGROUND</review_type>
|
||||||
|
|
||||||
|
<original_goal>
|
||||||
|
{GOAL}
|
||||||
|
</original_goal>
|
||||||
|
|
||||||
|
<constraints>
|
||||||
|
{CONSTRAINTS}
|
||||||
|
</constraints>
|
||||||
|
|
||||||
|
<changed_files>
|
||||||
|
{CHANGED_FILES}
|
||||||
|
</changed_files>
|
||||||
|
|
||||||
|
<background>
|
||||||
|
{BACKGROUND}
|
||||||
|
</background>
|
||||||
|
|
||||||
|
You are an investigator. Your mission: search every accessible information source to find context that should have informed this implementation but might have been missed. The question: "Is there something we should have known but didn't?"
|
||||||
|
|
||||||
|
SOURCES TO SEARCH (use every available tool):
|
||||||
|
|
||||||
|
1. **Git History** (ALWAYS search):
|
||||||
|
- \`git log --oneline -20 -- {each changed file}\` — recent changes and their reasons
|
||||||
|
- \`git blame {critical sections}\` — who wrote what and when
|
||||||
|
- \`git log --all --grep="{keywords from goal}"\` — related commits
|
||||||
|
- Look for reverted commits, TODO/FIXME/HACK comments in history
|
||||||
|
|
||||||
|
2. **GitHub** (if \`gh\` CLI available):
|
||||||
|
- \`gh issue list --search "{keywords}"\` — related open/closed issues
|
||||||
|
- \`gh pr list --search "{keywords}" --state all\` — related PRs and their review comments
|
||||||
|
- Check if any issue is specifically linked to this work
|
||||||
|
- Look at review comments on past PRs touching these files
|
||||||
|
|
||||||
|
3. **Communication Channels** (if MCP tools available):
|
||||||
|
- Slack: search for messages mentioning the feature, file names, or related keywords
|
||||||
|
- Notion: search for design docs, RFCs, ADRs related to this feature
|
||||||
|
- Discord: relevant discussions
|
||||||
|
|
||||||
|
4. **Codebase Cross-References** (ALWAYS search):
|
||||||
|
- Files that import or reference the changed modules
|
||||||
|
- Tests that might need updating due to behavior changes
|
||||||
|
- Documentation (README, docs/, comments) that references changed behavior
|
||||||
|
- Config files that might need corresponding updates
|
||||||
|
- Related features in the same domain
|
||||||
|
|
||||||
|
WHAT TO LOOK FOR:
|
||||||
|
|
||||||
|
- Requirements mentioned in issues/PRs that the implementation misses
|
||||||
|
- Past decisions explaining WHY code was written a certain way — and whether new changes respect those reasons
|
||||||
|
- Related systems or features affected by these changes
|
||||||
|
- Warnings from previous developers (PR review comments, inline TODOs, commit messages)
|
||||||
|
- Migration or deprecation notes that affect the changed code
|
||||||
|
- Design decisions documented outside the codebase (Notion, Slack, ADRs)
|
||||||
|
|
||||||
|
OUTPUT FORMAT:
|
||||||
|
<verdict>PASS or FAIL</verdict>
|
||||||
|
<confidence>HIGH / MEDIUM / LOW</confidence>
|
||||||
|
<summary>1-3 sentence overall assessment</summary>
|
||||||
|
<sources_searched>
|
||||||
|
- [SEARCHED/SKIPPED] Source name — what was searched (or why it wasn't accessible)
|
||||||
|
</sources_searched>
|
||||||
|
<discovered_context>
|
||||||
|
For each discovery:
|
||||||
|
- Source: Where found (git commit abc123, GitHub issue #42, Slack message, etc.)
|
||||||
|
- Finding: What was found
|
||||||
|
- Relevance: How it relates to the current work
|
||||||
|
- Impact: [BLOCKING / IMPORTANT / FYI]
|
||||||
|
</discovered_context>
|
||||||
|
<missed_requirements>Requirements the implementation should address but doesn't. Empty if none.</missed_requirements>
|
||||||
|
<blocking_issues>BLOCKING items only. Empty if PASS.</blocking_issues>
|
||||||
|
""")
|
||||||
|
\`\`\`
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Phase 2: Wait & Collect
|
||||||
|
|
||||||
|
After launching all 5 agents in one turn, **end your response**. Wait for system notifications as each agent completes.
|
||||||
|
|
||||||
|
As each completes, collect via \`background_output(task_id="...")\`. Store each verdict:
|
||||||
|
|
||||||
|
| Agent | Verdict | Notes |
|
||||||
|
|-------|---------|-------|
|
||||||
|
| 1. Goal Verification | pending | — |
|
||||||
|
| 2. QA Execution | pending | — |
|
||||||
|
| 3. Code Quality | pending | — |
|
||||||
|
| 4. Security | pending | — |
|
||||||
|
| 5. Context Mining | pending | — |
|
||||||
|
|
||||||
|
Do NOT deliver the final report until ALL 5 have completed.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Phase 3: Deliver Verdict
|
||||||
|
|
||||||
|
<verdict_logic>
|
||||||
|
|
||||||
|
ALL 5 agents returned PASS → **REVIEW PASSED**
|
||||||
|
ANY agent returned FAIL → **REVIEW FAILED — criteria not met**
|
||||||
|
|
||||||
|
</verdict_logic>
|
||||||
|
|
||||||
|
Compile the final report in this format:
|
||||||
|
|
||||||
|
\`\`\`markdown
|
||||||
|
# Review Work — Final Report
|
||||||
|
|
||||||
|
## Overall Verdict: PASSED / FAILED
|
||||||
|
|
||||||
|
| # | Review Area | Agent Type | Verdict | Confidence |
|
||||||
|
|---|------------|------------|---------|------------|
|
||||||
|
| 1 | Goal & Constraint Verification | Oracle | PASS/FAIL | HIGH/MED/LOW |
|
||||||
|
| 2 | QA Execution | unspecified-high | PASS/FAIL | HIGH/MED/LOW |
|
||||||
|
| 3 | Code Quality | Oracle | PASS/FAIL | HIGH/MED/LOW |
|
||||||
|
| 4 | Security (supplementary) | Oracle | PASS/FAIL | Severity |
|
||||||
|
| 5 | Context Mining | unspecified-high | PASS/FAIL | HIGH/MED/LOW |
|
||||||
|
|
||||||
|
## Blocking Issues
|
||||||
|
[Aggregated from all agents — deduplicated, prioritized]
|
||||||
|
|
||||||
|
## Key Findings
|
||||||
|
[Top 5-10 most important findings across all agents, grouped by theme]
|
||||||
|
|
||||||
|
## Recommendations
|
||||||
|
[If FAILED: exactly what to fix, in priority order]
|
||||||
|
[If PASSED: non-blocking suggestions worth considering]
|
||||||
|
\`\`\`
|
||||||
|
|
||||||
|
If FAILED — be specific. The user should know exactly what to fix and in what order. No vague "consider improving X" — state the problem, the file, and the fix.
|
||||||
|
|
||||||
|
If PASSED — keep it short. Highlight any non-blocking suggestions, but don't turn a passing review into a lecture.`,
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user