improve(development-codebase-tools): tune analyze-code skill with concrete execution steps#465
improve(development-codebase-tools): tune analyze-code skill with concrete execution steps#465
Conversation
…crete execution steps - Change model from sonnet to opus (multi-agent orchestration requires stronger synthesis) - Replace vague "Quick Process" + redundant "When to Activate" with concrete numbered execution steps - Add depth inference rules (overview/deep/architectural based on request keywords) - Add error handling guidance for missing files and failed agents
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🤖 Claude Code Review
Re-Review SummaryThis PR improves the Changes Reviewed
Previous Review CommentsAll three prior comments about the contradictory "all four agents in parallel" wording have been addressed. The current text at line 28 clearly reads: "run context-loader-agent first to gather system-level context, then spawn ... in parallel with that context" — making the sequential-then-parallel order unambiguous. No new issues found. 💡 Want a fresh review? Add a comment containing |
📚 Documentation Check ✅Verdict: Passed Plugin version was bumped (2.1.1 → 2.1.2), CLAUDE.md and README descriptions for analyze-code remain accurate after the skill content restructuring. PR #465 Documentation CheckChanges analyzed:
Version bump: Present and appropriate (patch bump for skill content improvement). CLAUDE.md ( README.md: Lists analyze-code with the same description — still accurate; no update needed. Pre-existing issue (not introduced by this PR): README skills table is missing the Suggestions (1)
🤖 Generated by Claude Documentation Validator | Mode: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8af69af1fa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| - `overview`: spawn **code-explainer-agent** only; pass the file path and its contents | ||
| - `deep`: spawn **code-explainer-agent** + **security-analyzer-agent** + **performance-analyzer-agent** in parallel | ||
| - `architectural`: spawn all four agents in parallel, loading system-level context via **context-loader-agent** first |
There was a problem hiding this comment.
Run context loader before parallel analyzers
The architectural instruction is internally contradictory because it says to "spawn all four agents in parallel" while also requiring context to be loaded "first." In practice this leaves execution order undefined, so architecture runs can start the explainer/security/performance agents before context-loader-agent finishes, producing incomplete or inconsistent system-level analysis. This should be made strictly sequential (context-loader first, then parallel analyzers) or fully parallel without the "first" requirement.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the latest commit (18079db). Step 4 now explicitly sequences context-loader first: "run context-loader-agent first to gather system-level context, then spawn the remaining three agents in parallel with that context." The contradictory "all four in parallel" phrasing has been removed.
There was a problem hiding this comment.
Confirmed fixed. Line 28 now clearly states: "run context-loader-agent first to gather system-level context, then spawn code-explainer-agent + security-analyzer-agent + performance-analyzer-agent in parallel with that context." The contradictory phrasing is gone.
There was a problem hiding this comment.
📋 Review verdict: REQUEST_CHANGES
👆 The main review comment above is the source of truth for this PR review. It is automatically updated on each review cycle, so always refer to it for the most current feedback.
This formal review submission is for the verdict only. 2 inline comment(s) are attached below.
| description: Comprehensive code explanation and analysis. Use when user says "explain this file to me", "what does this code do", "analyze the security of this module", "review the performance of this function", or "help me understand this architecture". | ||
| allowed-tools: Read, Grep, Glob, Bash(git show:*), Bash(git ls-files:*), Task(subagent_type:code-explainer-agent), Task(subagent_type:security-analyzer-agent), Task(subagent_type:performance-analyzer-agent), Task(subagent_type:context-loader-agent) | ||
| model: sonnet | ||
| model: opus |
There was a problem hiding this comment.
Per repository guidelines (CLAUDE.md > Mandatory Version Bumping), any change to packages/plugins/ must include a version bump in .claude-plugin/plugin.json. This PR changes execution behavior and the model — bump development-codebase-tools from 2.1.1 to 2.2.0 (minor, since it's a behavioral enhancement).
There was a problem hiding this comment.
Fixed in the latest commit (18079db). plugin.json has been bumped from 2.1.1 to 2.1.2 per the mandatory version bumping policy.
There was a problem hiding this comment.
Version has been bumped to 2.1.2. Note: per the repo's versioning guidelines, changing the model and rewriting execution behavior would typically warrant a minor bump (2.2.0) rather than a patch, since these are backward-compatible behavioral enhancements, not bug fixes. Not blocking — leaving for the author to decide.
|
|
||
| - `overview`: spawn **code-explainer-agent** only; pass the file path and its contents | ||
| - `deep`: spawn **code-explainer-agent** + **security-analyzer-agent** + **performance-analyzer-agent** in parallel | ||
| - `architectural`: spawn all four agents in parallel, loading system-level context via **context-loader-agent** first |
There was a problem hiding this comment.
"spawn all four agents in parallel" and "loading system-level context via context-loader-agent first" are contradictory. If context-loader must run first, the remaining three run in parallel after it completes — not all four in parallel.
| - `architectural`: spawn all four agents in parallel, loading system-level context via **context-loader-agent** first | |
| - `architectural`: spawn **context-loader-agent** first for system-level context, then spawn the remaining three agents (**code-explainer-agent** + **security-analyzer-agent** + **performance-analyzer-agent**) in parallel |
There was a problem hiding this comment.
Fixed in the latest commit (18079db). The wording now reads: "run context-loader-agent first to gather system-level context, then spawn code-explainer-agent + security-analyzer-agent + performance-analyzer-agent in parallel with that context" — making the sequential-then-parallel execution order unambiguous.
There was a problem hiding this comment.
Confirmed fixed. The wording now unambiguously sequences context-loader first, then the remaining three agents in parallel.
…chitectural agent order - Bump plugin.json from 2.1.1 to 2.1.2 per mandatory version bump policy - Clarify step 4 architectural dispatch: context-loader runs first, then the remaining three agents run in parallel — resolves the contradictory wording
Superseded by new review after PR update
There was a problem hiding this comment.
📋 Review verdict: APPROVE
👆 The main review comment above is the source of truth for this PR review. It is automatically updated on each review cycle, so always refer to it for the most current feedback.
This formal review submission is for the verdict only.
Summary
sonnet→opus— multi-agent orchestration requires stronger synthesis than sonnet providesoverview/deep/architecturalso Claude picks the right agent set without guessingWhat was wrong
The original "Quick Process" listed category names ("Structure Analysis", "Dependency Mapping") that told Claude what to analyze but not how to start or which agents to dispatch. The "When to Activate" section was a verbatim repeat of the description. A fresh Claude instance with this skill had no concrete execution path.
Test plan
AI-Generated Description
Summary
sonnet→opus— multi-agent orchestration requires stronger synthesis than Sonnet providesoverview/deep/architecturalso Claude picks the right agent set without guessingWhat was wrong
The original "Quick Process" listed category names that told Claude what to analyze but not how to start or which agents to dispatch. The "When to Activate" section was a verbatim repeat of the description frontmatter. A fresh Claude instance with this skill had no concrete execution path — it would guess at agent orchestration instead of following a deterministic procedure.
Changes
packages/plugins/development-codebase-tools/skills/analyze-code/SKILL.mdsonnet→opus; replace vague category list with 6 concrete execution steps; add depth inference rules and agent dispatch matrix; add error handling for missing targets and failed agentspackages/plugins/development-codebase-tools/.claude-plugin/plugin.jsonTest plan