From 6c0a1062f47d722a633f87292a7a79f02e7a230a Mon Sep 17 00:00:00 2001 From: wusuiling-if <221723439+wusuiling-if@users.noreply.github.com> Date: Tue, 28 Jul 2026 13:33:26 +0800 Subject: [PATCH 1/2] feat: add refactoring advisor skill --- README.md | 5 +- marketplaces/large-codebase.json | 14 +++ skills/index.js | 16 +++ skills/refactoring-advisor/.claude-plugin | 1 + skills/refactoring-advisor/.codex-plugin | 1 + .../refactoring-advisor/.plugin/plugin.json | 20 +++ skills/refactoring-advisor/README.md | 32 +++++ skills/refactoring-advisor/SKILL.md | 118 ++++++++++++++++++ .../references/discovery.md | 107 ++++++++++++++++ .../references/report-template.md | 96 ++++++++++++++ .../references/smells-and-strategies.md | 70 +++++++++++ 11 files changed, 478 insertions(+), 2 deletions(-) create mode 120000 skills/refactoring-advisor/.claude-plugin create mode 120000 skills/refactoring-advisor/.codex-plugin create mode 100644 skills/refactoring-advisor/.plugin/plugin.json create mode 100644 skills/refactoring-advisor/README.md create mode 100644 skills/refactoring-advisor/SKILL.md create mode 100644 skills/refactoring-advisor/references/discovery.md create mode 100644 skills/refactoring-advisor/references/report-template.md create mode 100644 skills/refactoring-advisor/references/smells-and-strategies.md diff --git a/README.md b/README.md index 232b5c4a..0db0e855 100644 --- a/README.md +++ b/README.md @@ -87,19 +87,20 @@ The JS and Python versions are kept in lock-step by `release-please` and guarded ## Extensions Catalog -This repository contains **2 marketplace(s)** with **63 extensions** (53 skills, 10 plugins). +This repository contains **2 marketplace(s)** with **64 extensions** (54 skills, 10 plugins). ### large-codebase OpenHands skills for interacting, improving, and refactoring large codebases -**4 extensions** (2 skills, 2 plugins) +**5 extensions** (3 skills, 2 plugins) | Name | Type | Description | Commands | |------|------|-------------|----------| | add-javadoc | skill | Add comprehensive JavaDoc documentation to Java classes and methods. Use when documenting Java code, adding API docum... | — | | cobol-modernization | plugin | End-to-end COBOL to Java migration workflow. Handles build setup, mainframe dependency removal, and code migration wi... | — | | migration-scoring | plugin | Evaluate code migration quality with coverage, correctness, and style scoring. Generates executive reports with actio... | — | +| refactoring-advisor | skill | Analyze codebases for structural problems and produce concrete, prioritized refactoring plans without implementing ch... | — | | spark-version-upgrade | skill | Upgrade Apache Spark applications between major versions (2.x→3.x, 3.x→4.x). Covers build files, deprecated APIs, con... | — | ### openhands-extensions diff --git a/marketplaces/large-codebase.json b/marketplaces/large-codebase.json index 0d44fdca..ee3dd6fe 100644 --- a/marketplaces/large-codebase.json +++ b/marketplaces/large-codebase.json @@ -48,6 +48,20 @@ "reporting" ] }, + { + "name": "refactoring-advisor", + "source": "./skills/refactoring-advisor", + "description": "Analyze codebases for structural problems and produce concrete, prioritized refactoring plans without implementing changes.", + "category": "code-quality", + "keywords": [ + "refactoring", + "architecture", + "code-smells", + "technical-debt", + "dependency-injection", + "testability" + ] + }, { "name": "spark-version-upgrade", "source": "./skills/spark-version-upgrade", diff --git a/skills/index.js b/skills/index.js index 73b0cf88..904ce72d 100644 --- a/skills/index.js +++ b/skills/index.js @@ -399,6 +399,22 @@ export const SKILLS_CATALOG = [ ], "content": "# QA Changes\n\nValidate pull request changes by actually running the code — not just reading it. The goal is to verify that new behavior works as the PR claims, existing behavior is not broken, and the repository remains healthy after the change.\n\nThe bar is high: test the way a thorough human QA engineer would. If the PR changes a web UI, spin up the server and verify it in a real browser. If it changes a CLI, run the CLI with real inputs. Do not settle for \"the tests pass\" — actually use the software.\n\n## Core Methodology\n\nQA proceeds in four phases. Complete each phase in order. If a phase fails, report the failure and stop.\n\n### Phase 1: Understand the Change\n\nRead the PR diff, title, and description. **Identify the goal of this PR** — this is the single most important thing to understand before proceeding. A PR might fix a bug, add a feature, refactor code, improve performance, update documentation, or something else entirely. Check:\n\n1. **The PR description \"Why\" / \"Summary\" section** — what is the author trying to accomplish?\n2. **Linked issues** — if the PR references an issue, read it. But note: the PR may address the issue differently than expected, or only partially. The PR description is the real specification for what *this PR* intends to deliver.\n3. **The PR title** — often summarizes the intent (e.g., \"fix: X not working when Y\", \"feat: add Z capability\", \"refactor: consolidate duplicated X logic\").\n\nThen classify every changed file:\n\n- **New feature**: User-visible behavior that did not exist before.\n- **Bug fix**: Corrects existing behavior to match intended behavior.\n- **Refactor**: Restructuring that should not change external behavior.\n- **Configuration / CI / docs**: Non-functional changes.\n\nFor each change, identify the *entry point* — the concrete way a user would interact with it (CLI command, API endpoint, UI page, function call). This drives what to exercise in Phase 3.\n\nFinally, form a clear hypothesis: \"This PR should [achieve stated goal] by [approach taken in the diff].\" Phase 3 will test that hypothesis.\n\n### Phase 2: Set Up the Environment\n\nBootstrap the repository so the project builds and runs successfully.\n\n1. **Read the repo's bootstrap instructions.** Check `AGENTS.md`, `README.md`, `Makefile`, `package.json`, `pyproject.toml`, `Cargo.toml`, or equivalent. Always prefer the project's own documented setup commands.\n2. **Install dependencies.** Use the project's dependency manager (`uv sync`, `npm install`, `pip install -r requirements.txt`, `bundle install`, `cargo build`, etc.).\n3. **Build the project** if a build step is required (compile, transpile, bundle).\n4. **Note CI status.** Glance at the PR's CI checks and note whether they pass or fail. Do NOT re-run the test suite yourself — that is CI's job, not yours. Your job starts in Phase 3.\n\nIf setup fails, report the failure with the exact error output and stop.\n\n### Phase 3: Exercise the Changed Behavior\n\nThis is the most important phase. **Actually use the software** the way a real user would to verify the change works as the PR claims. This is what distinguishes QA from CI (which runs tests) and code review (which reads code).\n\n**Do NOT:**\n- Run the test suite (`pytest`, `npm test`, `cargo test`, etc.) — that is CI's job.\n- Analyze code by reading files and commenting on style, structure, or logic — that is code review's job.\n- Run linters, formatters, type checkers, or pre-commit hooks — that is CI's job.\n\n**DO:**\n- Run the actual application, CLI, or server and interact with it as a user would.\n- Make real HTTP requests, run real commands, open real browser pages.\n- Always attempt real execution first. Running `--help`, `--dry-run`, or `--version` is NOT functional verification — it only proves argument parsing works. If real execution fails due to missing credentials, external services, or environment constraints, report what you tried and what could not be verified. Do not substitute `--help` output for evidence the software works.\n- Reproduce bugs and verify fixes end-to-end.\n- Test user-facing behavior that automated tests cannot or do not cover.\n\n**Start by verifying the PR achieves its stated goal.** Use the hypothesis from Phase 1. For example:\n- If the PR claims to \"fix crash when X is empty\", reproduce the crash scenario and confirm it no longer occurs.\n- If the PR claims to \"add support for Y\", actually use Y end-to-end and confirm it works.\n- If the PR claims to \"add a new dashboard page\", navigate to the page and verify it renders and functions correctly.\n- If the PR claims to \"add a new CLI flag\", run the CLI with that flag and verify the output.\n\n\"Tests pass\" is not a QA finding. The question is: does the software actually do what the PR says it does?\n\n**For frontend / UI changes:**\n- Start the development server.\n- Use a real browser (via Playwright, browser automation tools, or the built-in browser) to navigate to the affected pages.\n- Verify the visual change renders correctly. Take screenshots as evidence.\n- Test user interactions (clicks, form submissions, navigation).\n- Try at least one edge case (empty state, long text, missing data).\n\n**For CLI changes:**\n- Run the CLI command with realistic arguments. Capture stdout and stderr.\n- Verify the output matches the PR's claimed behavior.\n- Try at least one edge case (invalid input, missing flags, empty input).\n\n**For API / backend changes:**\n- Start the server.\n- Make actual HTTP requests (`curl`, `httpie`, or a test client) to affected endpoints.\n- Verify response status codes, response bodies, and side effects (database writes, file creation).\n- Test error cases (bad input, missing auth, not found).\n\n**For bug fixes — use a before/after comparison:**\n1. **Reproduce the bug without the fix.** Check out the base branch (or revert the PR's changes) and run a concrete command or code path that triggers the reported failure. Show the exact command and its output.\n2. **Interpret the baseline result.** Explain what the output means — e.g., \"This confirms the bug exists: the resolver cannot find the package because the lockfile's cutoff date is too old.\"\n3. **Apply the PR's changes.** Check out the PR branch, apply the patch, or set the environment variable — whatever the fix entails.\n4. **Re-run the same verification.** Run the same command or exercise the same code path with the fix in place. Show the exact command and its output.\n5. **Interpret the result.** Explain what the new output means — e.g., \"The resolver now finds the package, confirming the fix works.\"\n6. **Check for side effects.** Confirm the fix does not break related functionality.\n\n**For library / SDK changes:**\n- Write a short script that imports and calls the changed functions.\n- Verify the return values and behavior match the PR's claims.\n- Test edge cases the PR author may have missed.\n\n**For refactors:**\n- If the refactor touches a critical or user-facing path, manually exercise that path to confirm behavior is unchanged.\n- For pure internal refactors where CI passes and no user-facing path is affected, Phase 2's CI check is sufficient.\n\n**For configuration / CI / docs:**\n- Validate syntax (YAML lint, JSON parse, markdown render).\n- If it is a build change, confirm the build still succeeds.\n- For doc changes, confirm the documentation renders correctly if a preview is available.\n\n**Always show your work with a before/after narrative.** For every verification, the report must include: (a) the exact command you ran, (b) the actual output you observed, and (c) your interpretation of that output. For bug fixes and behavioral changes, demonstrate BOTH the broken/old state AND the fixed/new state so the reviewer can see the delta. Present this evidence inside collapsible `
` blocks — the core deliverable is the verdict and summary, not raw logs.\n\n### Knowing When to Give Up\n\nSome verification approaches will fail due to environment constraints, missing system dependencies, or tooling limitations. That is expected.\n\n**The rule: if the same general approach fails after three materially different attempts, stop trying that approach.** For example, if three different Playwright configurations all fail to connect to the dev server, do not try a fourth Playwright variation. Switch to a fundamentally different approach (e.g., `curl` + manual HTML inspection instead of browser automation). If two fundamentally different approaches both fail, give up on that specific verification and say so in the report.\n\nWhen giving up on a verification:\n- State clearly what was attempted and why it failed.\n- State what *could not* be verified as a result.\n- Suggest the human add guidance to `AGENTS.md` (or a custom `/qa-changes` skill) that would help future QA runs succeed — for example: which port the dev server runs on, what system packages are required, how to configure browser automation, or what the expected test output looks like.\n\nDo not silently skip verification. An honest \"I could not verify X because Y\" is far more valuable than a false \"everything works.\"\n\n### Phase 4: Report Results\n\nPost a structured report as a PR review using the GitHub API. **Keep the report scannable.** A reviewer should grasp the verdict and key results in under 10 seconds. Put lengthy evidence (logs, code snippets, full command output) inside collapsible `
` blocks so the top-level report stays compact.\n\n#### Report format\n\n```markdown\n## {verdict_emoji} QA Report: {VERDICT}\n\n{One-sentence summary of what was verified and the outcome.}\n\n### Does this PR achieve its stated goal?\n\n{Direct answer: Yes / Partially / No.}\n{2-3 sentences explaining WHY, referencing specific evidence from\nexercising the software. For bug fixes: is the bug actually fixed?\nFor features: does the new capability work end-to-end? For refactors:\nis the restructuring achieved without changing behavior? Be specific\nabout what the goal was and whether the changes deliver on it.}\n\n| Phase | Result |\n|-------|--------|\n| Environment Setup | {emoji} {one-line status} |\n| CI Status | {emoji} {one-line note from CI checks, e.g. \"all green\" or \"2 checks failing\"} |\n| Functional Verification | {emoji} {one-line status} |\n\n
Functional Verification\n\n{Structure each verification as a before/after narrative:\n\n### Test N: {Description}\n\n**Step 1 — Reproduce / establish baseline (without the fix):**\nRan `{exact command}`:\n```\n{actual output}\n```\nThis shows {interpretation — what the output means, e.g. \"the bug\nexists because...\"}.\n\n**Step 2 — Apply the PR's changes:**\n{What was done — e.g. checked out the PR branch, set env var, etc.}\n\n**Step 3 — Re-run with the fix in place:**\nRan `{same or equivalent command}`:\n```\n{actual output}\n```\nThis shows {interpretation — e.g. \"the fix works because the error\nis gone and the expected result appears\"}.\n\nRepeat for each changed behavior. For non-bug-fix changes\n(features, refactors), the baseline step may simply describe the\nprior state rather than reproducing a failure.}\n\n
\n\n
Unable to Verify\n\n{What could not be verified, what was attempted, and suggested\nAGENTS.md guidance. Omit this section entirely if everything\nwas verified.}\n\n
\n\n### Issues Found\n\n{List concrete problems, or \"None.\" if clean.}\n\n- 🔴 **Blocker**: ...\n- 🟠 **Issue**: ...\n- 🟡 **Minor**: ...\n```\n\n#### Formatting rules\n\n- **Verdict line + summary** come first. One emoji, one sentence. No preamble.\n- **Status table** gives the at-a-glance overview. One row per phase, one-line status.\n- **Evidence goes in `
` blocks.** Any code block, log excerpt, or command output longer than ~4 lines belongs inside a collapsible. Reviewers who want proof can expand; others can skip.\n- **Do not repeat information.** The summary, table, and details should each add new information — not restate the same facts in different formats.\n- **Issues Found** is always visible (not collapsible). If there are no issues, write \"None.\"\n- **Omit empty sections.** If there is nothing unable to verify, drop that `
` block entirely.\n\n#### Verdict values\n\n- ✅ **PASS**: Change works as described, no regressions.\n- ⚠️ **PASS WITH ISSUES**: Change mostly works, but issues were found (list them).\n- ❌ **FAIL**: Change does not work as described, or introduces regressions.\n- 🟡 **PARTIAL**: Some behavior verified, some could not be (list what was and was not verified).\n\n## Key Principles\n\n- **Answer the core question first: does this PR achieve its stated goal?** This is the primary deliverable. Explicitly state whether the changes deliver on what the PR description promises — whether that is a bug fix, a new feature, a refactor, or anything else.\n- **Fail fast.** If setup fails, stop and report. Do not spend tokens on later phases with a broken environment.\n- **Run the code, not the tests.** Execute the actual software — start servers, run CLI commands, make HTTP requests, open browsers. Do not run `pytest`, `npm test`, or equivalent test suites. That is CI's job.\n- **Do not analyze code.** Reading files and commenting on style, structure, or logic is code review's job. Your job is to exercise behavior, not read source files.\n- **Set a high bar.** If the change affects a UI, open it in a real browser. If it affects a CLI, run the actual CLI with real inputs. If it affects an API, make real HTTP requests.\n- **Test what the PR claims.** The PR description is the specification. Verify the claim, not hypothetical scenarios.\n- **Leave CI to CI.** Do not re-run tests, linters, formatters, or type checkers. Note CI status, then focus entirely on functional verification that CI cannot do.\n- **Report evidence, not opinions.** Include exact commands, outputs, and error messages — inside collapsible blocks.\n- **Keep it scannable.** The report is for busy reviewers. Verdict and summary up top, evidence collapsed below. Do not repeat information across sections.\n- **Give up gracefully.** If a verification approach does not work after three materially different attempts, switch approaches. If two different approaches fail, give up and report honestly. Suggest `AGENTS.md` improvements.\n- **Respect the project's conventions.** Use the project's own tools and build commands for setup." }, + { + "name": "refactoring-advisor", + "description": "Analyze a codebase to identify structural problems and propose concrete refactoring plans using design patterns, dependency injection, and established refactoring techniques. This skill should be used when the user asks to \"refactor\", find \"code smells\" or \"technical debt\", simplify complex code, break up a \"god class\", reduce coupling or duplication, resolve circular dependencies, introduce dependency injection, improve testability, or create a refactoring plan for a function, class, module, package, or repository. Produce analysis and proposals only, without writing implementation code.", + "triggers": [ + "refactor", + "code smells", + "technical debt", + "god class", + "spaghetti code", + "too coupled", + "hard to test", + "simplify", + "dependency injection" + ], + "content": "# Refactoring Advisor\n\nAct as a senior software architect. Inspect the requested scope, diagnose structural causes, and produce an evidence-backed plan that a human or implementation agent can execute.\n\nDo not modify production code, tests, configuration, or generated files. Do not provide implementation patches or full replacement code. Small pseudocode signatures or dependency diagrams are acceptable only when they clarify a proposed boundary.\n\n## Core Principles\n\n- Ground every finding in repository evidence. Cite file paths, symbols, and line ranges.\n- Treat metric thresholds as investigation signals, not automatic defects.\n- Diagnose causes and consequences before recommending patterns.\n- Prefer the smallest refactoring that creates a meaningful boundary.\n- Preserve externally observable behavior unless the user explicitly requests a behavior change.\n- Evaluate dependency injection for every proposal. Recommend it when it moves volatile I/O, infrastructure, time, randomness, configuration, or external services behind an explicit boundary. State why it is unnecessary when direct dependencies are already stable and cohesive.\n- Avoid adding interfaces, factories, layers, or patterns without a concrete consumer, testability need, or source of variation.\n- Separate confirmed findings from hypotheses that require runtime data or owner input.\n\n## Workflow\n\n### 1. Establish Scope and Constraints\n\n1. Read repository-level instructions such as `AGENTS.md`, `CONTRIBUTING.md`, and relevant package documentation.\n2. Resolve the requested scope. For an unspecified request, inspect the whole repository while prioritizing production code and architectural boundaries.\n3. Identify languages, frameworks, package boundaries, build systems, test layout, generated or vendored directories, and public APIs.\n4. Exclude generated, vendored, dependency, build-output, fixture, and snapshot files unless they reveal an architectural boundary relevant to the request.\n5. Record constraints that affect sequencing: compatibility promises, framework lifecycle, deployment topology, ownership, migrations, and test coverage.\n\nRead `references/discovery.md` for repository mapping commands, language-specific cues, sampling strategy, and fallback methods. Use available static-analysis tools when the repository already provides them; do not install new tools merely to produce the report.\n\n### 2. Discover Structural Signals\n\nInspect all relevant levels before drawing conclusions:\n\n| Level | Signals to investigate |\n| --- | --- |\n| Function | More than roughly 50 lines, more than 4-5 parameters, nesting deeper than 3 levels, many branches, mixed I/O and domain logic, boolean control flags |\n| Class | Too many responsibilities, low cohesion, many collaborators, feature envy, mutable global state, difficult construction, broad public surface |\n| Module/package | Cycles, unstable boundaries, duplicated policies, leaky abstractions, shotgun changes, unrelated exports, infrastructure mixed with domain logic |\n| Cross-cutting | Service locator use, hidden dependencies, inconsistent error handling, repeated validation, scattered configuration, temporal coupling, untestable side effects |\n\nCorroborate each signal with at least one consequence, such as change amplification, fragile tests, duplicated fixes, unclear ownership, runtime risk, or blocked reuse. Do not report long code or a high count alone as a smell.\n\nBuild a compact evidence inventory before prioritizing. Include:\n\n- exact path and symbol;\n- narrow line range or call sites;\n- dependency direction and affected consumers;\n- tests that cover or fail to isolate the area;\n- confidence level: high, medium, or low.\n\n### 3. Diagnose Root Causes\n\nMap each material finding to one or more named smells from `references/smells-and-strategies.md`. Distinguish the visible symptom from the architectural cause.\n\nFor each finding:\n\n1. State the observed evidence.\n2. Name the smell.\n3. Explain the root cause and why the current boundary permits it.\n4. Describe the maintenance, testability, reliability, or delivery impact.\n5. Note uncertainty or missing evidence.\n\nMerge findings that share one root cause. Do not inflate the report with multiple symptoms of the same dependency problem.\n\n### 4. Design Refactoring Proposals\n\nCreate one proposal per independently executable change. Each proposal must include:\n\n1. **Target and evidence** - exact files, symbols, and line ranges.\n2. **Desired boundary** - named functions, classes, modules, interfaces, or packages to extract, move, split, or replace.\n3. **Step sequence** - behavior-preserving steps small enough for incremental review.\n4. **Dependency injection evaluation** - recommend constructor, parameter, factory, or framework-native injection when applicable; otherwise state `Not needed` with a specific reason.\n5. **Pattern choice** - name a design or refactoring pattern only when it solves the diagnosed problem; explain why it fits better than a simpler extraction.\n6. **Behavior and API constraints** - contracts that must remain stable.\n7. **Verification** - existing tests to run and focused characterization, unit, integration, or architecture tests to add.\n8. **Risks and rollback boundary** - likely regressions, migration concerns, and a safe commit boundary.\n9. **Effort and impact** - low, medium, or high, with a short rationale.\n\nPrefer dependency direction from policy toward abstractions, with infrastructure implementing those abstractions. Keep domain logic independent of frameworks where the repository's architecture supports that separation. Do not recommend a dependency injection container when explicit constructor or parameter injection is sufficient.\n\n### 5. Prioritize the Plan\n\nRank proposals by impact, confidence, dependency order, and effort. Put enabling characterization tests or cycle-breaking boundaries before broad extractions. Identify quick wins separately from foundational work, and call out proposals that should not proceed until a hypothesis is verified.\n\nUse `references/report-template.md` exactly for the final report. Include every section even when no material issues are found. In that case, document the inspected scope, evidence, and residual risks rather than inventing findings.\n\n## Quality Gate\n\nBefore returning the report, verify that:\n\n- Discovery covered functions, classes, modules or packages, and cross-cutting concerns.\n- Every finding names a recognized smell and cites concrete evidence.\n- Every proposal names affected paths and symbols, not generic layers such as \"add a service\".\n- Every proposal contains an explicit dependency injection decision.\n- Proposed steps preserve behavior and are independently verifiable.\n- The action plan reflects prerequisites and impact versus effort.\n- No implementation code, patch, or unrequested file modification was produced.\n\n## References\n\n- `references/discovery.md` - Repository scanning, evidence collection, language cues, and large-repository sampling\n- `references/smells-and-strategies.md` - Named smells, root-cause prompts, DI guidance, and fitting strategies\n- `references/report-template.md` - Required output structure for findings, proposals, and prioritized actions" + }, { "name": "release-notes", "description": "Generate formatted changelogs from git history since the last release tag. Use when preparing release notes that categorize changes into breaking changes, features, fixes, and other sections.", diff --git a/skills/refactoring-advisor/.claude-plugin b/skills/refactoring-advisor/.claude-plugin new file mode 120000 index 00000000..665797f0 --- /dev/null +++ b/skills/refactoring-advisor/.claude-plugin @@ -0,0 +1 @@ +.plugin \ No newline at end of file diff --git a/skills/refactoring-advisor/.codex-plugin b/skills/refactoring-advisor/.codex-plugin new file mode 120000 index 00000000..665797f0 --- /dev/null +++ b/skills/refactoring-advisor/.codex-plugin @@ -0,0 +1 @@ +.plugin \ No newline at end of file diff --git a/skills/refactoring-advisor/.plugin/plugin.json b/skills/refactoring-advisor/.plugin/plugin.json new file mode 100644 index 00000000..6056720f --- /dev/null +++ b/skills/refactoring-advisor/.plugin/plugin.json @@ -0,0 +1,20 @@ +{ + "name": "refactoring-advisor", + "version": "1.0.0", + "description": "Analyze codebases for structural problems and produce concrete, prioritized refactoring plans without implementing changes.", + "author": { + "name": "OpenHands", + "email": "contact@all-hands.dev" + }, + "homepage": "https://github.com/OpenHands/extensions", + "repository": "https://github.com/OpenHands/extensions", + "license": "MIT", + "keywords": [ + "refactoring", + "architecture", + "code-smells", + "technical-debt", + "dependency-injection", + "testability" + ] +} diff --git a/skills/refactoring-advisor/README.md b/skills/refactoring-advisor/README.md new file mode 100644 index 00000000..9ab0de7b --- /dev/null +++ b/skills/refactoring-advisor/README.md @@ -0,0 +1,32 @@ +# Refactoring Advisor + +Analyze a codebase for structural problems and produce an evidence-backed refactoring plan without changing code. + +## What It Covers + +- Function-level complexity and mixed responsibilities +- Class cohesion, collaborator count, and construction problems +- Module and package boundaries, including circular dependencies +- Cross-cutting concerns such as hidden dependencies, duplicated policy, configuration, and untestable side effects +- Dependency injection decisions for every proposal +- Prioritization by impact, effort, confidence, and prerequisite order + +The skill supports Java, Python, TypeScript, and C# with language-specific discovery guidance. It treats size and complexity thresholds as investigation signals rather than automatic defects. + +## Usage + +Ask OpenHands to audit a repository or a narrower scope, for example: + +- "Scan this repository for code smells and create a refactoring plan." +- "These modules are too coupled. How should we separate them?" +- "Make this service easier to test with dependency injection." +- "Diagnose the circular dependencies in this package." + +The output contains a scope statement, architecture snapshot, evidence-linked findings, independently executable proposals, an explicit dependency injection evaluation for each proposal, and a prioritized implementation handoff. + +## Skill Resources + +- `SKILL.md` defines the analysis workflow and quality gate. +- `references/discovery.md` describes repository scanning and language-specific cues. +- `references/smells-and-strategies.md` maps named smells to refactoring and DI strategies. +- `references/report-template.md` defines the required report structure. diff --git a/skills/refactoring-advisor/SKILL.md b/skills/refactoring-advisor/SKILL.md new file mode 100644 index 00000000..0f24fa3d --- /dev/null +++ b/skills/refactoring-advisor/SKILL.md @@ -0,0 +1,118 @@ +--- +name: refactoring-advisor +description: Analyze a codebase to identify structural problems and propose concrete refactoring plans using design patterns, dependency injection, and established refactoring techniques. This skill should be used when the user asks to "refactor", find "code smells" or "technical debt", simplify complex code, break up a "god class", reduce coupling or duplication, resolve circular dependencies, introduce dependency injection, improve testability, or create a refactoring plan for a function, class, module, package, or repository. Produce analysis and proposals only, without writing implementation code. +triggers: + - refactor + - code smells + - technical debt + - god class + - spaghetti code + - too coupled + - hard to test + - simplify + - dependency injection +--- + +# Refactoring Advisor + +Act as a senior software architect. Inspect the requested scope, diagnose structural causes, and produce an evidence-backed plan that a human or implementation agent can execute. + +Do not modify production code, tests, configuration, or generated files. Do not provide implementation patches or full replacement code. Small pseudocode signatures or dependency diagrams are acceptable only when they clarify a proposed boundary. + +## Core Principles + +- Ground every finding in repository evidence. Cite file paths, symbols, and line ranges. +- Treat metric thresholds as investigation signals, not automatic defects. +- Diagnose causes and consequences before recommending patterns. +- Prefer the smallest refactoring that creates a meaningful boundary. +- Preserve externally observable behavior unless the user explicitly requests a behavior change. +- Evaluate dependency injection for every proposal. Recommend it when it moves volatile I/O, infrastructure, time, randomness, configuration, or external services behind an explicit boundary. State why it is unnecessary when direct dependencies are already stable and cohesive. +- Avoid adding interfaces, factories, layers, or patterns without a concrete consumer, testability need, or source of variation. +- Separate confirmed findings from hypotheses that require runtime data or owner input. + +## Workflow + +### 1. Establish Scope and Constraints + +1. Read repository-level instructions such as `AGENTS.md`, `CONTRIBUTING.md`, and relevant package documentation. +2. Resolve the requested scope. For an unspecified request, inspect the whole repository while prioritizing production code and architectural boundaries. +3. Identify languages, frameworks, package boundaries, build systems, test layout, generated or vendored directories, and public APIs. +4. Exclude generated, vendored, dependency, build-output, fixture, and snapshot files unless they reveal an architectural boundary relevant to the request. +5. Record constraints that affect sequencing: compatibility promises, framework lifecycle, deployment topology, ownership, migrations, and test coverage. + +Read `references/discovery.md` for repository mapping commands, language-specific cues, sampling strategy, and fallback methods. Use available static-analysis tools when the repository already provides them; do not install new tools merely to produce the report. + +### 2. Discover Structural Signals + +Inspect all relevant levels before drawing conclusions: + +| Level | Signals to investigate | +| --- | --- | +| Function | More than roughly 50 lines, more than 4-5 parameters, nesting deeper than 3 levels, many branches, mixed I/O and domain logic, boolean control flags | +| Class | Too many responsibilities, low cohesion, many collaborators, feature envy, mutable global state, difficult construction, broad public surface | +| Module/package | Cycles, unstable boundaries, duplicated policies, leaky abstractions, shotgun changes, unrelated exports, infrastructure mixed with domain logic | +| Cross-cutting | Service locator use, hidden dependencies, inconsistent error handling, repeated validation, scattered configuration, temporal coupling, untestable side effects | + +Corroborate each signal with at least one consequence, such as change amplification, fragile tests, duplicated fixes, unclear ownership, runtime risk, or blocked reuse. Do not report long code or a high count alone as a smell. + +Build a compact evidence inventory before prioritizing. Include: + +- exact path and symbol; +- narrow line range or call sites; +- dependency direction and affected consumers; +- tests that cover or fail to isolate the area; +- confidence level: high, medium, or low. + +### 3. Diagnose Root Causes + +Map each material finding to one or more named smells from `references/smells-and-strategies.md`. Distinguish the visible symptom from the architectural cause. + +For each finding: + +1. State the observed evidence. +2. Name the smell. +3. Explain the root cause and why the current boundary permits it. +4. Describe the maintenance, testability, reliability, or delivery impact. +5. Note uncertainty or missing evidence. + +Merge findings that share one root cause. Do not inflate the report with multiple symptoms of the same dependency problem. + +### 4. Design Refactoring Proposals + +Create one proposal per independently executable change. Each proposal must include: + +1. **Target and evidence** - exact files, symbols, and line ranges. +2. **Desired boundary** - named functions, classes, modules, interfaces, or packages to extract, move, split, or replace. +3. **Step sequence** - behavior-preserving steps small enough for incremental review. +4. **Dependency injection evaluation** - recommend constructor, parameter, factory, or framework-native injection when applicable; otherwise state `Not needed` with a specific reason. +5. **Pattern choice** - name a design or refactoring pattern only when it solves the diagnosed problem; explain why it fits better than a simpler extraction. +6. **Behavior and API constraints** - contracts that must remain stable. +7. **Verification** - existing tests to run and focused characterization, unit, integration, or architecture tests to add. +8. **Risks and rollback boundary** - likely regressions, migration concerns, and a safe commit boundary. +9. **Effort and impact** - low, medium, or high, with a short rationale. + +Prefer dependency direction from policy toward abstractions, with infrastructure implementing those abstractions. Keep domain logic independent of frameworks where the repository's architecture supports that separation. Do not recommend a dependency injection container when explicit constructor or parameter injection is sufficient. + +### 5. Prioritize the Plan + +Rank proposals by impact, confidence, dependency order, and effort. Put enabling characterization tests or cycle-breaking boundaries before broad extractions. Identify quick wins separately from foundational work, and call out proposals that should not proceed until a hypothesis is verified. + +Use `references/report-template.md` exactly for the final report. Include every section even when no material issues are found. In that case, document the inspected scope, evidence, and residual risks rather than inventing findings. + +## Quality Gate + +Before returning the report, verify that: + +- Discovery covered functions, classes, modules or packages, and cross-cutting concerns. +- Every finding names a recognized smell and cites concrete evidence. +- Every proposal names affected paths and symbols, not generic layers such as "add a service". +- Every proposal contains an explicit dependency injection decision. +- Proposed steps preserve behavior and are independently verifiable. +- The action plan reflects prerequisites and impact versus effort. +- No implementation code, patch, or unrequested file modification was produced. + +## References + +- `references/discovery.md` - Repository scanning, evidence collection, language cues, and large-repository sampling +- `references/smells-and-strategies.md` - Named smells, root-cause prompts, DI guidance, and fitting strategies +- `references/report-template.md` - Required output structure for findings, proposals, and prioritized actions diff --git a/skills/refactoring-advisor/references/discovery.md b/skills/refactoring-advisor/references/discovery.md new file mode 100644 index 00000000..8d2c45ec --- /dev/null +++ b/skills/refactoring-advisor/references/discovery.md @@ -0,0 +1,107 @@ +# Repository Discovery + +Use this reference while mapping a repository and collecting evidence. Adapt commands to the available shell and tools. + +## 1. Read the Repository Contract + +Locate and read applicable instruction and architecture files before evaluating structure: + +```text +AGENTS.md, CONTRIBUTING.md, README.md, docs/architecture/* +package.json, pyproject.toml, pom.xml, build.gradle*, *.sln, *.csproj +``` + +Treat documented boundaries and public compatibility promises as constraints, not unquestioned proof that the implementation is healthy. + +## 2. Map the Codebase + +Prefer fast, read-only inventory commands: + +```sh +git status --short +git ls-files +find . -maxdepth 3 -type d +rg -n "^(class|interface|def|async def|function|export class|public class) " +rg -n "TODO|FIXME|HACK|deprecated" +``` + +Use repository-provided analyzers, dependency graphs, coverage tools, and linters when already configured. Their output is evidence to inspect, not a substitute for reading the implicated code. + +Identify: + +- entry points and composition roots; +- package or project boundaries; +- domain, application, and infrastructure layers if present; +- high fan-in and high fan-out modules; +- cycles and reversed dependency directions; +- test seams and direct construction of external dependencies; +- repeated concepts with different names and shared names with different meanings. + +Ignore `.git`, dependency caches, virtual environments, build outputs, generated clients, migrations generated by tools, minified files, snapshots, and vendored code unless explicitly in scope. + +## 3. Inspect by Granularity + +### Functions and methods + +Use roughly 50 lines, 4-5 parameters, 3 nesting levels, and high branch count as prompts to inspect. Confirm a real issue by looking for multiple reasons to change, mixed abstraction levels, control flags, repeated condition families, or side effects intertwined with decisions. + +### Classes and components + +Inspect constructor dependencies, field groups, method clusters, public surface, lifecycle responsibilities, and reasons to change. A large cohesive parser can be healthier than a small class coordinating unrelated workflows. + +### Modules and packages + +Trace imports in both directions. Look for cycles, broad utility modules, duplicated orchestration, domain code importing concrete infrastructure, and changes that repeatedly cross the same set of files. + +### Cross-cutting behavior + +Trace configuration, logging, validation, authorization, error translation, retries, caching, transactions, time, and randomness. Flag inconsistent or scattered policy only when consolidation would clarify ownership or prevent drift. + +## 4. Language-Specific Cues + +### Java + +- Inspect constructor and field injection, static service access, framework annotations, package cycles, oversized Spring services/controllers, and interfaces with only speculative value. +- Use Maven or Gradle dependency and test tasks already defined by the project. +- Respect framework-managed lifecycles and transaction boundaries in proposals. + +### Python + +- Inspect imports executed at module load, mutable module globals, broad `except` blocks, mixins, monkeypatch-heavy tests, circular imports hidden by local imports, and functions mixing orchestration with data transformation. +- Distinguish flexible duck-typed boundaries from missing contracts. Recommend protocols or abstract base classes only when multiple implementations, substitution, or static typing benefits justify them. + +### TypeScript + +- Inspect barrel-file cycles, type/value import confusion, service singletons, React components mixing effects with domain decisions, large union switches, and modules that combine transport, validation, and business rules. +- Respect framework composition patterns. Prefer explicit parameters or constructors over introducing a container by default. + +### C# + +- Inspect service registration, service locator calls, static mutable services, broad controllers, partial classes hiding split responsibilities, project reference cycles, and inappropriate service lifetimes. +- Preserve disposal, async, transaction, and scoped-service semantics in proposed boundaries. + +## 5. Scale the Audit + +For a large repository: + +1. Inventory all packages and dependency boundaries. +2. Rank hotspots using repository-provided complexity data, change history, test concentration, file size, and dependency centrality. +3. Inspect representative entry points and the highest-risk hotspots. +4. Trace at least one important flow end to end. +5. State what was sampled and what was not inspected. + +Never claim a complete audit when context or tool limits required sampling. + +## 6. Record Evidence + +For each candidate, record: + +```text +Location: path:start-end and symbol +Signal: observable structural fact +Corroboration: consequence, repeated call site, dependency direction, or test friction +Scope: function | class | module/package | cross-cutting +Confidence: high | medium | low +``` + +Discard candidates that lack a plausible consequence or whose proposed cure would cost more than the documented problem. diff --git a/skills/refactoring-advisor/references/report-template.md b/skills/refactoring-advisor/references/report-template.md new file mode 100644 index 00000000..16e4a240 --- /dev/null +++ b/skills/refactoring-advisor/references/report-template.md @@ -0,0 +1,96 @@ +# Required Report Template + +Use this structure exactly. Replace bracketed instructions and omit no top-level section. + +```markdown +# Refactoring Assessment + +## Executive Summary + +[Summarize the architectural condition, highest-value intervention, and important constraints in 3-6 sentences.] + +## Scope and Method + +- Scope requested: [repository, package, module, class, function, or changed files] +- Inspected: [paths, entry points, tests, and dependency/build metadata] +- Excluded: [generated, vendored, out-of-scope, or uninspected areas] +- Method: [commands, existing analyzers, dependency traces, and sampling] +- Constraints: [compatibility, framework lifecycle, ownership, deployment, or unknowns] + +## Architecture Snapshot + +[Describe current boundaries and dependency direction. Include a compact text or Mermaid diagram only when it materially clarifies at least three components.] + +## Findings + +### F1 - [Specific finding title] + +- Severity: Critical | High | Medium | Low +- Confidence: High | Medium | Low +- Scope: Function | Class | Module/Package | Cross-cutting +- Smell: [recognized smell name] +- Evidence: `[path:start-end]` - `[symbol]`; [related call sites, dependencies, or tests] +- Root cause: [why the current boundary produces the symptom] +- Consequence: [maintenance, testability, reliability, performance, or delivery impact] +- Uncertainty: [missing runtime or ownership evidence, or `None`] + +[Repeat only for material findings. If none exist, state that explicitly and summarize residual risk.] + +## Refactoring Proposals + +### P1 - [Outcome-oriented proposal title] + +- Addresses: [F1, F2] +- Target: `[paths and symbols]` +- Desired boundary: [specific functions, classes, modules, interfaces, or packages] +- Refactoring strategy: [named refactoring operations and justified design pattern, if any] +- DI evaluation: [Constructor | Parameter | Factory | Framework-native | Not needed] - [specific rationale] +- Behavior/API constraints: [contracts to preserve] +- Effort: Low | Medium | High - [rationale] +- Impact: Low | Medium | High - [rationale] +- Risks: [regression and migration risks] +- Verification: [existing tests plus focused tests or architecture checks to add] +- Rollback boundary: [independently revertible commit or compatibility seam] + +Steps: + +1. [Small, behavior-preserving step naming concrete symbols.] +2. [Next step.] +3. [Verification or cutover step.] + +[Repeat for each independently executable proposal.] + +## Prioritized Action Plan + +| Order | Proposal | Prerequisites | Impact | Effort | Why now | +| --- | --- | --- | --- | --- | --- | +| 1 | P# | [none or proposal/test] | High | Low | [reason] | + +### Quick Wins + +- [Low-effort, evidence-backed actions.] + +### Foundational Work + +- [Boundary or test work that enables later proposals.] + +### Defer or Validate First + +- [Low-confidence or poor cost/benefit proposals and required evidence.] + +## Implementation Handoff + +- Suggested change sequence: [review-sized stages] +- Characterization coverage needed first: [specific behavior and tests] +- Decisions requiring owner input: [API, data, ownership, or deployment decisions] +- Definition of done: [observable structural and testability outcomes] +``` + +## Evidence Rules + +- Use repository-relative paths and 1-based line ranges. +- Keep ranges narrow enough to locate the relevant code. +- Name symbols and affected callers when possible. +- Label inferences as inferences. +- Do not paste large code blocks or implementation-ready replacements. +- Keep finding IDs and proposal IDs stable so an implementation agent can reference them. diff --git a/skills/refactoring-advisor/references/smells-and-strategies.md b/skills/refactoring-advisor/references/smells-and-strategies.md new file mode 100644 index 00000000..03a742a6 --- /dev/null +++ b/skills/refactoring-advisor/references/smells-and-strategies.md @@ -0,0 +1,70 @@ +# Smells and Refactoring Strategies + +Use this catalog to name a diagnosis consistently. Select only smells supported by repository evidence. + +| Smell | Typical evidence | Likely root cause | Candidate strategies | +| --- | --- | --- | --- | +| Long Method | Multiple phases, mixed abstraction, many branches or locals | Missing workflow steps or domain concepts | Extract Function, Replace Temp with Query, Decompose Conditional | +| Long Parameter List | Repeated parameter groups, boolean flags, unstable call sites | Missing parameter object or hidden collaborator | Introduce Parameter Object, Preserve Whole Object, inject a collaborator | +| God Class / Large Class | Unrelated field and method clusters, many reasons to change | Responsibilities accumulated around a convenient coordinator | Extract Class, Extract Module, Move Method, Facade | +| Feature Envy | Logic reads another object's data more than its own | Behavior lives outside the concept it governs | Move Function, Extract Function, domain service when ownership spans concepts | +| Data Clumps | Same values travel together across APIs | Missing value object or request concept | Extract Class, Introduce Parameter Object | +| Primitive Obsession | Domain rules repeated around strings, numbers, or flags | Missing domain type | Replace Primitive with Object, Value Object | +| Switch Statements / Repeated Conditionals | Same type or state branching repeated in many places | Missing polymorphic behavior or dispatch table | Replace Conditional with Polymorphism, Strategy, State; keep a switch when it is localized and stable | +| Divergent Change | One module changes for unrelated reasons | Responsibilities share a physical boundary without conceptual cohesion | Extract Module or Class, Separate Query from Modifier | +| Shotgun Surgery | One policy change touches many modules | Scattered ownership or leaky abstraction | Move Function, Inline then re-extract around one owner, Facade | +| Duplicated Code | Same rule or transformation implemented in several places | No authoritative owner or premature copying | Extract Function, Pull Up Method, shared domain policy; avoid generic utilities without cohesion | +| Inappropriate Intimacy | Modules reach into internals or share mutable state | Boundary exposes representation instead of behavior | Move Function, Hide Delegate, Encapsulate Variable | +| Message Chains | Callers traverse deep object graphs | Missing operation at the owning boundary | Hide Delegate, Extract Function; avoid wrappers that only mirror the chain | +| Middle Man | A type delegates nearly everything without policy | Layer exists without responsibility | Remove Middle Man, Inline Class | +| Leaky Abstraction | Callers understand transport, storage, or framework details | Contract does not contain volatility | Extract Interface, Adapter, Repository, Anti-Corruption Layer | +| Circular Dependency | Packages import each other directly or indirectly | Responsibilities or shared contracts are placed on the wrong side | Move shared contract, Dependency Inversion, domain events, merge falsely separated modules | +| Service Locator / Hidden Dependency | Global lookup, static singleton, ambient context | Construction and use are conflated | Constructor or parameter injection, composition root | +| Temporal Coupling | Calls must occur in an undocumented order | Invalid intermediate states or split lifecycle ownership | Encapsulate lifecycle, factory, state object, command | +| Parallel Inheritance Hierarchies | Adding one subtype requires a matching subtype elsewhere | Two variation axes are encoded as inheritance | Move Method, Strategy, composition | +| Speculative Generality | Unused abstractions, one-implementation interfaces, configurable paths with no consumer | Future-proofing without evidence | Collapse Hierarchy, Inline Class, remove dead abstraction | +| Mutable Global State | Tests depend on order or process-wide mutation | Ownership and lifecycle are implicit | Inject state holder, immutable configuration, scoped context | +| Mixed I/O and Domain Logic | Business decisions occur inside HTTP, DB, filesystem, or UI code | Missing application boundary | Extract Function or Service, Ports and Adapters, inject gateway | + +## Dependency Injection Decision + +Evaluate DI for every proposal with these questions: + +1. Does the target directly create or locate a volatile dependency such as a database, network client, filesystem, clock, random source, environment, queue, or framework service? +2. Does substituting that dependency materially improve deterministic tests or support multiple implementations? +3. Is there a clear composition root that can own construction and lifecycle? +4. Can a function parameter or constructor argument solve the problem without a container or new interface? +5. Would injection expose a real architectural boundary, or merely move cohesive internal details into the caller? + +Recommend, in order of simplicity: + +1. Parameter injection for a dependency used by one operation. +2. Constructor injection for a stable collaborator used across instance operations. +3. Factory injection when creation timing or per-operation configuration varies. +4. Framework-native registration when the application already uses a DI framework and lifecycle management matters. + +Mark DI as `Not needed` when the proposal concerns a pure computation, a cohesive value object, internal deterministic helpers, or an extraction with no external dependency. Explain the specific reason. + +Avoid: + +- injecting every helper or value object; +- creating interfaces solely to mock a class with no volatility; +- adding a service locator disguised as an injector; +- passing a general container into domain code; +- changing singleton lifetime without accounting for state and concurrency; +- recommending DI as a substitute for choosing the correct responsibility boundary. + +## Pattern Selection Guardrail + +Name a pattern only after identifying the force it resolves: + +- Use **Strategy** for a genuine, selectable algorithm family. +- Use **State** when allowed behavior changes with explicit lifecycle state. +- Use **Adapter** to translate an external or legacy contract. +- Use **Facade** to present a cohesive entry point over a complicated subsystem. +- Use **Repository** to isolate persistence semantics, not as a generic wrapper over every query. +- Use **Decorator** for independently composable behavior around a stable contract. +- Use **Command** when operations need queuing, logging, retries, undo, or independent dispatch. +- Use **Ports and Adapters** when domain policy must remain independent of multiple volatile external systems. + +Prefer Extract Function, Move Function, Extract Class, or Introduce Parameter Object when those operations solve the problem without a larger pattern. From 3db1b47eae81c90a21c0ec501b1402e7cedf4e68 Mon Sep 17 00:00:00 2001 From: wusuiling-if <221723439+wusuiling-if@users.noreply.github.com> Date: Tue, 28 Jul 2026 14:11:22 +0800 Subject: [PATCH 2/2] feat: strengthen refactoring advisor contract --- skills/index.js | 9 ++- skills/refactoring-advisor/SKILL.md | 8 ++- .../references/report-template.md | 2 +- tests/test_refactoring_advisor.py | 62 +++++++++++++++++++ 4 files changed, 75 insertions(+), 6 deletions(-) create mode 100644 tests/test_refactoring_advisor.py diff --git a/skills/index.js b/skills/index.js index 904ce72d..665936bd 100644 --- a/skills/index.js +++ b/skills/index.js @@ -401,7 +401,7 @@ export const SKILLS_CATALOG = [ }, { "name": "refactoring-advisor", - "description": "Analyze a codebase to identify structural problems and propose concrete refactoring plans using design patterns, dependency injection, and established refactoring techniques. This skill should be used when the user asks to \"refactor\", find \"code smells\" or \"technical debt\", simplify complex code, break up a \"god class\", reduce coupling or duplication, resolve circular dependencies, introduce dependency injection, improve testability, or create a refactoring plan for a function, class, module, package, or repository. Produce analysis and proposals only, without writing implementation code.", + "description": "Analyze a codebase to identify structural problems and propose concrete refactoring plans using design patterns, dependency injection, and established refactoring techniques. This skill should be used when the user asks to \"refactor\", find \"code smells\" or \"technical debt\", \"extract method\", \"reduce complexity\", \"clean up this codebase\", simplify complex code, break up a \"god class\", reduce coupling or duplication, resolve circular dependencies, introduce dependency injection, improve testability, or create a refactoring plan for a function, class, module, package, or repository. Produce analysis and proposals only, without writing implementation code.", "triggers": [ "refactor", "code smells", @@ -411,9 +411,12 @@ export const SKILLS_CATALOG = [ "too coupled", "hard to test", "simplify", - "dependency injection" + "dependency injection", + "extract method", + "reduce complexity", + "clean up this codebase" ], - "content": "# Refactoring Advisor\n\nAct as a senior software architect. Inspect the requested scope, diagnose structural causes, and produce an evidence-backed plan that a human or implementation agent can execute.\n\nDo not modify production code, tests, configuration, or generated files. Do not provide implementation patches or full replacement code. Small pseudocode signatures or dependency diagrams are acceptable only when they clarify a proposed boundary.\n\n## Core Principles\n\n- Ground every finding in repository evidence. Cite file paths, symbols, and line ranges.\n- Treat metric thresholds as investigation signals, not automatic defects.\n- Diagnose causes and consequences before recommending patterns.\n- Prefer the smallest refactoring that creates a meaningful boundary.\n- Preserve externally observable behavior unless the user explicitly requests a behavior change.\n- Evaluate dependency injection for every proposal. Recommend it when it moves volatile I/O, infrastructure, time, randomness, configuration, or external services behind an explicit boundary. State why it is unnecessary when direct dependencies are already stable and cohesive.\n- Avoid adding interfaces, factories, layers, or patterns without a concrete consumer, testability need, or source of variation.\n- Separate confirmed findings from hypotheses that require runtime data or owner input.\n\n## Workflow\n\n### 1. Establish Scope and Constraints\n\n1. Read repository-level instructions such as `AGENTS.md`, `CONTRIBUTING.md`, and relevant package documentation.\n2. Resolve the requested scope. For an unspecified request, inspect the whole repository while prioritizing production code and architectural boundaries.\n3. Identify languages, frameworks, package boundaries, build systems, test layout, generated or vendored directories, and public APIs.\n4. Exclude generated, vendored, dependency, build-output, fixture, and snapshot files unless they reveal an architectural boundary relevant to the request.\n5. Record constraints that affect sequencing: compatibility promises, framework lifecycle, deployment topology, ownership, migrations, and test coverage.\n\nRead `references/discovery.md` for repository mapping commands, language-specific cues, sampling strategy, and fallback methods. Use available static-analysis tools when the repository already provides them; do not install new tools merely to produce the report.\n\n### 2. Discover Structural Signals\n\nInspect all relevant levels before drawing conclusions:\n\n| Level | Signals to investigate |\n| --- | --- |\n| Function | More than roughly 50 lines, more than 4-5 parameters, nesting deeper than 3 levels, many branches, mixed I/O and domain logic, boolean control flags |\n| Class | Too many responsibilities, low cohesion, many collaborators, feature envy, mutable global state, difficult construction, broad public surface |\n| Module/package | Cycles, unstable boundaries, duplicated policies, leaky abstractions, shotgun changes, unrelated exports, infrastructure mixed with domain logic |\n| Cross-cutting | Service locator use, hidden dependencies, inconsistent error handling, repeated validation, scattered configuration, temporal coupling, untestable side effects |\n\nCorroborate each signal with at least one consequence, such as change amplification, fragile tests, duplicated fixes, unclear ownership, runtime risk, or blocked reuse. Do not report long code or a high count alone as a smell.\n\nBuild a compact evidence inventory before prioritizing. Include:\n\n- exact path and symbol;\n- narrow line range or call sites;\n- dependency direction and affected consumers;\n- tests that cover or fail to isolate the area;\n- confidence level: high, medium, or low.\n\n### 3. Diagnose Root Causes\n\nMap each material finding to one or more named smells from `references/smells-and-strategies.md`. Distinguish the visible symptom from the architectural cause.\n\nFor each finding:\n\n1. State the observed evidence.\n2. Name the smell.\n3. Explain the root cause and why the current boundary permits it.\n4. Describe the maintenance, testability, reliability, or delivery impact.\n5. Note uncertainty or missing evidence.\n\nMerge findings that share one root cause. Do not inflate the report with multiple symptoms of the same dependency problem.\n\n### 4. Design Refactoring Proposals\n\nCreate one proposal per independently executable change. Each proposal must include:\n\n1. **Target and evidence** - exact files, symbols, and line ranges.\n2. **Desired boundary** - named functions, classes, modules, interfaces, or packages to extract, move, split, or replace.\n3. **Step sequence** - behavior-preserving steps small enough for incremental review.\n4. **Dependency injection evaluation** - recommend constructor, parameter, factory, or framework-native injection when applicable; otherwise state `Not needed` with a specific reason.\n5. **Pattern choice** - name a design or refactoring pattern only when it solves the diagnosed problem; explain why it fits better than a simpler extraction.\n6. **Behavior and API constraints** - contracts that must remain stable.\n7. **Verification** - existing tests to run and focused characterization, unit, integration, or architecture tests to add.\n8. **Risks and rollback boundary** - likely regressions, migration concerns, and a safe commit boundary.\n9. **Effort and impact** - low, medium, or high, with a short rationale.\n\nPrefer dependency direction from policy toward abstractions, with infrastructure implementing those abstractions. Keep domain logic independent of frameworks where the repository's architecture supports that separation. Do not recommend a dependency injection container when explicit constructor or parameter injection is sufficient.\n\n### 5. Prioritize the Plan\n\nRank proposals by impact, confidence, dependency order, and effort. Put enabling characterization tests or cycle-breaking boundaries before broad extractions. Identify quick wins separately from foundational work, and call out proposals that should not proceed until a hypothesis is verified.\n\nUse `references/report-template.md` exactly for the final report. Include every section even when no material issues are found. In that case, document the inspected scope, evidence, and residual risks rather than inventing findings.\n\n## Quality Gate\n\nBefore returning the report, verify that:\n\n- Discovery covered functions, classes, modules or packages, and cross-cutting concerns.\n- Every finding names a recognized smell and cites concrete evidence.\n- Every proposal names affected paths and symbols, not generic layers such as \"add a service\".\n- Every proposal contains an explicit dependency injection decision.\n- Proposed steps preserve behavior and are independently verifiable.\n- The action plan reflects prerequisites and impact versus effort.\n- No implementation code, patch, or unrequested file modification was produced.\n\n## References\n\n- `references/discovery.md` - Repository scanning, evidence collection, language cues, and large-repository sampling\n- `references/smells-and-strategies.md` - Named smells, root-cause prompts, DI guidance, and fitting strategies\n- `references/report-template.md` - Required output structure for findings, proposals, and prioritized actions" + "content": "# Refactoring Advisor\n\nAct as a senior software architect. Inspect the requested scope, diagnose structural causes, and produce an evidence-backed plan that a human or implementation agent can execute.\n\nDo not modify production code, tests, configuration, or generated files. Do not provide implementation patches, executable pseudocode, or full replacement code. Use dependency diagrams and named interface or method signatures only when they clarify a proposed boundary.\n\n## Core Principles\n\n- Ground every finding in repository evidence. Cite file paths, symbols, and line ranges.\n- Treat metric thresholds as investigation signals, not automatic defects.\n- Diagnose causes and consequences before recommending patterns.\n- Prefer the smallest refactoring that creates a meaningful boundary.\n- Preserve externally observable behavior unless the user explicitly requests a behavior change.\n- Evaluate dependency injection for every proposal. Recommend it when it moves volatile I/O, infrastructure, time, randomness, configuration, or external services behind an explicit boundary. State why it is unnecessary when direct dependencies are already stable and cohesive.\n- When multiple designs are otherwise comparable, prefer explicit dependency injection as the default decoupling technique.\n- Avoid adding interfaces, factories, layers, or patterns without a concrete consumer, testability need, or source of variation.\n- Separate confirmed findings from hypotheses that require runtime data or owner input.\n\n## Workflow\n\n### 1. Establish Scope and Constraints\n\n1. Read repository-level instructions such as `AGENTS.md`, `CONTRIBUTING.md`, and relevant package documentation.\n2. Resolve the requested scope. For an unspecified request, inspect the whole repository while prioritizing production code and architectural boundaries.\n3. Identify languages, frameworks, package boundaries, build systems, test layout, generated or vendored directories, and public APIs.\n4. Exclude generated, vendored, dependency, build-output, fixture, and snapshot files unless they reveal an architectural boundary relevant to the request.\n5. Record constraints that affect sequencing: compatibility promises, framework lifecycle, deployment topology, ownership, migrations, and test coverage.\n\nRead `references/discovery.md` for repository mapping commands, language-specific cues, sampling strategy, and fallback methods. Use available static-analysis tools when the repository already provides them; do not install new tools merely to produce the report.\n\n### 2. Discover Structural Signals\n\nInspect all relevant levels before drawing conclusions:\n\n| Level | Signals to investigate |\n| --- | --- |\n| Function | More than roughly 50 lines, more than 4-5 parameters, nesting deeper than 3 levels, many branches, mixed I/O and domain logic, boolean control flags |\n| Class | Too many responsibilities, low cohesion, many collaborators, feature envy, mutable global state, difficult construction, broad public surface |\n| Module/package | Cycles, unstable boundaries, duplicated policies, leaky abstractions, shotgun changes, unrelated exports, infrastructure mixed with domain logic |\n| Cross-cutting | Service locator use, hidden dependencies, inconsistent error handling, repeated validation, scattered configuration, temporal coupling, untestable side effects |\n\nCorroborate each signal with at least one consequence, such as change amplification, fragile tests, duplicated fixes, unclear ownership, runtime risk, or blocked reuse. Do not report long code or a high count alone as a smell.\n\nBuild a compact evidence inventory before prioritizing. Include:\n\n- exact path and symbol;\n- narrow line range or call sites;\n- dependency direction and affected consumers;\n- tests that cover or fail to isolate the area;\n- confidence level: high, medium, or low.\n\n### 3. Diagnose Root Causes\n\nMap each material finding to one or more named smells from `references/smells-and-strategies.md`. Distinguish the visible symptom from the architectural cause.\n\nFor each finding:\n\n1. State the observed evidence.\n2. Name the smell.\n3. Explain the root cause and why the current boundary permits it.\n4. Describe the maintenance, testability, reliability, or delivery impact.\n5. Note uncertainty or missing evidence.\n\nMerge findings that share one root cause. Do not inflate the report with multiple symptoms of the same dependency problem.\n\n### 4. Design Refactoring Proposals\n\nCreate one proposal per independently executable change. Each proposal must include:\n\n1. **Target and evidence** - exact files, symbols, and line ranges.\n2. **Desired boundary** - named functions, classes, modules, interfaces, or packages to extract, move, split, or replace.\n3. **Step sequence** - behavior-preserving steps small enough for incremental review.\n4. **Dependency injection evaluation** - recommend constructor, parameter, factory, or framework-native injection when applicable; otherwise state `Not needed` with a specific reason.\n5. **Pattern choice** - name a design or refactoring pattern only when it solves the diagnosed problem; explain why it fits better than a simpler extraction.\n6. **Behavior and API constraints** - contracts that must remain stable.\n7. **Verification** - existing tests to run and focused characterization, unit, integration, or architecture tests to add.\n8. **Risks and rollback boundary** - likely regressions, migration concerns, and a safe commit boundary.\n9. **Effort and impact** - low, medium, or high, with a short rationale.\n\nPrefer dependency direction from policy toward abstractions, with infrastructure implementing those abstractions. Keep domain logic independent of frameworks where the repository's architecture supports that separation. Do not recommend a dependency injection container when explicit constructor or parameter injection is sufficient.\n\n### 5. Prioritize the Plan\n\nRank proposals by impact, confidence, dependency order, and effort. Put enabling characterization tests or cycle-breaking boundaries before broad extractions. Identify quick wins separately from foundational work, and call out proposals that should not proceed until a hypothesis is verified.\n\nUse `references/report-template.md` exactly for the final report. Include every section even when no material issues are found. In that case, document the inspected scope, evidence, and residual risks rather than inventing findings.\n\n## Quality Gate\n\nBefore returning the report, verify that:\n\n- Discovery covered functions, classes, modules or packages, and cross-cutting concerns.\n- Every finding names a recognized smell and cites concrete evidence.\n- Every proposal names affected paths and symbols, not generic layers such as \"add a service\".\n- Every proposal contains an explicit dependency injection decision.\n- Proposed steps preserve behavior and are independently verifiable.\n- The action plan reflects prerequisites and impact versus effort.\n- No implementation code, patch, or unrequested file modification was produced.\n\n## References\n\n- `references/discovery.md` - Repository scanning, evidence collection, language cues, and large-repository sampling\n- `references/smells-and-strategies.md` - Named smells, root-cause prompts, DI guidance, and fitting strategies\n- `references/report-template.md` - Required output structure for findings, proposals, and prioritized actions" }, { "name": "release-notes", diff --git a/skills/refactoring-advisor/SKILL.md b/skills/refactoring-advisor/SKILL.md index 0f24fa3d..94bc18e8 100644 --- a/skills/refactoring-advisor/SKILL.md +++ b/skills/refactoring-advisor/SKILL.md @@ -1,6 +1,6 @@ --- name: refactoring-advisor -description: Analyze a codebase to identify structural problems and propose concrete refactoring plans using design patterns, dependency injection, and established refactoring techniques. This skill should be used when the user asks to "refactor", find "code smells" or "technical debt", simplify complex code, break up a "god class", reduce coupling or duplication, resolve circular dependencies, introduce dependency injection, improve testability, or create a refactoring plan for a function, class, module, package, or repository. Produce analysis and proposals only, without writing implementation code. +description: Analyze a codebase to identify structural problems and propose concrete refactoring plans using design patterns, dependency injection, and established refactoring techniques. This skill should be used when the user asks to "refactor", find "code smells" or "technical debt", "extract method", "reduce complexity", "clean up this codebase", simplify complex code, break up a "god class", reduce coupling or duplication, resolve circular dependencies, introduce dependency injection, improve testability, or create a refactoring plan for a function, class, module, package, or repository. Produce analysis and proposals only, without writing implementation code. triggers: - refactor - code smells @@ -11,13 +11,16 @@ triggers: - hard to test - simplify - dependency injection + - extract method + - reduce complexity + - clean up this codebase --- # Refactoring Advisor Act as a senior software architect. Inspect the requested scope, diagnose structural causes, and produce an evidence-backed plan that a human or implementation agent can execute. -Do not modify production code, tests, configuration, or generated files. Do not provide implementation patches or full replacement code. Small pseudocode signatures or dependency diagrams are acceptable only when they clarify a proposed boundary. +Do not modify production code, tests, configuration, or generated files. Do not provide implementation patches, executable pseudocode, or full replacement code. Use dependency diagrams and named interface or method signatures only when they clarify a proposed boundary. ## Core Principles @@ -27,6 +30,7 @@ Do not modify production code, tests, configuration, or generated files. Do not - Prefer the smallest refactoring that creates a meaningful boundary. - Preserve externally observable behavior unless the user explicitly requests a behavior change. - Evaluate dependency injection for every proposal. Recommend it when it moves volatile I/O, infrastructure, time, randomness, configuration, or external services behind an explicit boundary. State why it is unnecessary when direct dependencies are already stable and cohesive. +- When multiple designs are otherwise comparable, prefer explicit dependency injection as the default decoupling technique. - Avoid adding interfaces, factories, layers, or patterns without a concrete consumer, testability need, or source of variation. - Separate confirmed findings from hypotheses that require runtime data or owner input. diff --git a/skills/refactoring-advisor/references/report-template.md b/skills/refactoring-advisor/references/report-template.md index 16e4a240..a5591585 100644 --- a/skills/refactoring-advisor/references/report-template.md +++ b/skills/refactoring-advisor/references/report-template.md @@ -92,5 +92,5 @@ Steps: - Keep ranges narrow enough to locate the relevant code. - Name symbols and affected callers when possible. - Label inferences as inferences. -- Do not paste large code blocks or implementation-ready replacements. +- Do not paste code blocks, executable pseudocode, or implementation-ready replacements. - Keep finding IDs and proposal IDs stable so an implementation agent can reference them. diff --git a/tests/test_refactoring_advisor.py b/tests/test_refactoring_advisor.py new file mode 100644 index 00000000..41673440 --- /dev/null +++ b/tests/test_refactoring_advisor.py @@ -0,0 +1,62 @@ +"""Contract tests for the refactoring-advisor skill.""" + +from pathlib import Path + +import yaml + + +ROOT = Path(__file__).resolve().parents[1] +SKILL_DIR = ROOT / "skills" / "refactoring-advisor" +SKILL_PATH = SKILL_DIR / "SKILL.md" +TEMPLATE_PATH = SKILL_DIR / "references" / "report-template.md" + + +def _frontmatter_and_body(path: Path) -> tuple[dict, str]: + text = path.read_text() + _, frontmatter, body = text.split("---", 2) + return yaml.safe_load(frontmatter), body + + +def test_refactoring_advisor_has_required_triggers_and_scope(): + metadata, body = _frontmatter_and_body(SKILL_PATH) + references = "\n".join(path.read_text() for path in (SKILL_DIR / "references").glob("*.md")) + searchable = f"{metadata['description']}\n{body}\n{references}".lower() + + for phrase in ( + "refactor", + "code smells", + "technical debt", + "extract method", + "reduce complexity", + "clean up this codebase", + "dependency injection", + ): + assert phrase in searchable + + for language in ("java", "python", "typescript", "c#"): + assert language in searchable + + assert len(SKILL_PATH.read_text().splitlines()) < 500 + + +def test_refactoring_advisor_enforces_output_only_and_di_evaluation(): + _, body = _frontmatter_and_body(SKILL_PATH) + template = TEMPLATE_PATH.read_text() + searchable = f"{body}\n{template}".lower() + + assert "do not provide implementation patches" in searchable + assert "executable pseudocode" in searchable + assert "every proposal contains an explicit dependency injection decision" in searchable + assert "di evaluation" in searchable + assert "prioritized action plan" in searchable + + +def test_refactoring_advisor_references_exist(): + for relative_path in ( + "references/discovery.md", + "references/smells-and-strategies.md", + "references/report-template.md", + ".plugin/plugin.json", + "README.md", + ): + assert (SKILL_DIR / relative_path).is_file(), relative_path