fix(development-pr-workflow): Opus 5 migration fixes - #569
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📚 Documentation Check ✅Verdict: Passed The mandatory plugin version bump (2.3.0 → 2.4.0) was made correctly in both plugin.json and the root CLAUDE.md table in the same change. All cross-plugin agent references introduced by the fix (e.g. development-codebase-tools:agent-orchestrator-agent, uniswap-integrations:migration-assistant-agent, development-planning:plan-reviewer-agent) were verified to exist. No stale references to renamed/removed items (old Linear tool name, graphite-stack-updater, pr-issue-resolver, numeric reviewability scores) remain anywhere in the plugin. PR #569 is a documentation/consistency fix-up pass on the
One caveat unrelated to the PR content itself: the local working tree has an uncommitted modification reverting ✨ No Documentation Updates NeededAll documentation appears to be up to date with the code changes. 🤖 Generated by Claude Documentation Validator | Mode: |
|
● Reviewed · against Latest: ✅ approved (2) ← prior: 🚨 changes requested (3) Note Approved — two nits in Corrects broken agent and MCP tool references across the development-pr-workflow plugin (bare agent names → Suggestions
Iteration history · 2 reviews2026-08-05 23:31 UTC · ✅ approved · 2 findings ·
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a911de755
ℹ️ 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".
|
|
||
| - `stack-splitter-agent` - Requires Graphite for PR stacking | ||
| - `graphite-stack-updater` - Requires Graphite for stack management | ||
| - `update-graphite-stack` (skill) - Requires Graphite for stack management |
There was a problem hiding this comment.
This commit changes files under packages/plugins/development-pr-workflow/, but packages/plugins/development-pr-workflow/.claude-plugin/plugin.json still has 2.3.0 both in this commit and its parent. The docs-check workflow defaults fail_on_missing_version to true and explicitly fails when a plugin is modified without a version bump (.github/workflows/_claude-docs-check.yml:76-80,503-540), so this PR will be blocked and the marketplace will not get a distinct plugin release; please bump development-pr-workflow to 2.3.1 and update the root version table in the same commit.
Useful? React with 👍 / 👎.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| Report every genuine finding. Assign each a severity rather than dropping it — do not omit | ||
| a finding because it is minor, because you are unsure it is real, or because the list is | ||
| getting long. Record uncertainty on the finding itself ("possible", "worth confirming"). | ||
| The `--focus` and `--severity` flags filter what is *presented*; they never narrow what is |
There was a problem hiding this comment.
--severity is defined nowhere: this file declares only --depth, --suggest-fixes, --check-coverage, --focus, and --baseline (lines 11-16), and review-code/SKILL.md:141 carries the same dangling reference against its own input list. A user passing --severity high gets it silently ignored; this is the same broken-reference class the PR removes. Either define the flag or drop it from both files.
| description: Take local changes, create a Linear task, create a branch (optionally in a worktree), commit, and publish a PR | ||
| argument-hint: [--team <id>] [--trunk <branch>] [--create-worktree] [--use-graphite true/false] | ||
| allowed-tools: Bash(*), Read(*), Write(*), AskUserQuestion(*), mcp__graphite__run_gt_cmd(*), mcp__github__create_pull_request(*), mcp__linear__create_issue(*), mcp__linear__get_user(*), mcp__linear__list_teams(*), mcp__linear__list_projects(*), mcp__linear__list_issue_labels(*) | ||
| allowed-tools: Bash(*), Read(*), Write(*), AskUserQuestion(*), mcp__graphite__run_gt_cmd(*), mcp__github__create_pull_request(*), mcp__linear__save_issue(*), mcp__linear__get_user(*), mcp__linear__list_teams(*), mcp__linear__list_projects(*), mcp__linear__list_issue_labels(*) |
There was a problem hiding this comment.
mcp__linear__save_issue has no corroboration anywhere in the repo; all four occurrences are introduced here, and every sibling Linear tool on this line uses the hosted server's canonical names, where the create/update pair is create_issue/update_issue. If the name is wrong, /start-linear-task and /linear-task-and-pr-from-changes fail at task creation and allowed-tools stops permitting the tool that exists. Verify against the live mcp.linear.app tool list before merge.
There was a problem hiding this comment.
Leaving this as save_issue. I checked it against the live server rather than the repo, and the name is correct.
Which server is in play: packages/plugins/uniswap-integrations/.mcp.json declares Linear as {"type": "http", "url": "https://mcp.linear.app/mcp"} — Linear's official hosted server.
Querying that server's actual tool list, save_issue is there, described verbatim as:
Create or update a Linear issue. If
idis provided, updates the existing issue; otherwise creates a new one. When creating,titleandteamare required.
There is no create_issue and no update_issue on that server. The only create_* issue-adjacent tools are create_issue_label and create_initiative_label. The server uses a consistent save_* family throughout: save_issue, save_comment, save_project, save_document, save_initiative, save_milestone.
So the premise is inverted: reverting to create_issue would break both Linear commands at task creation, which is the exact failure this finding was trying to prevent.
The "no corroboration anywhere in the repo" observation is accurate but not decisive here — the repo isn't the authority on a hosted server's tool surface, and these four occurrences are the first uses of the create/update tool in this plugin. Worth noting there IS stale naming elsewhere: packages/plugins/uniswap-integrations/skills/refine-linear-task/SKILL.md:3 still grants mcp__linear__linear_search_issues, linear_update_issue, and linear_add_comment, none of which exist on the hosted server either. That looks like a real bug, but it's a different plugin and out of scope for this PR.
The other three findings were correct and are addressed in e845c70.
|
|
||
| The pipeline below is the **maximum** staffing for a broad, cross-cutting diff — not a | ||
| roster to complete. Skip any phase the diff does not raise, and drop individual agents | ||
| within a phase for the same reason: a docs-only change needs neither the security nor the |
There was a problem hiding this comment.
🔵 info · security — "docs-only" is the wrong example for skipping the security pass: in this plugin, markdown frontmatter carries the allowed-tools capability grants (this file grants Task(subagent_type:*); start-linear-task.md:4 grants Bash(*)). Skipping on "no code, config, or capability grants changed" keeps the fan-out reduction without the misclassification — same wording applies at skills/review-code/SKILL.md:48.
There was a problem hiding this comment.
Caution
…tion ceilings in review paths Opus 5 migration, deltas 2/3/4: - review-code: agent quota table -> ceilings with a when-not-to-delegate rule; all 7 dispatched agents were missing both the `-agent` suffix and the `plugin:` qualifier they need (all live in sibling plugins) - review-pr: same plugin qualification for all 12 dispatched agents; the 4-phase comprehensive pipeline now reads as maximum staffing, not a roster - review-pr output schema: risk.breakdown 0-10 and testQuality.score 0-100 become qualitative bands; coverage and cyclomatic complexity become optional and require a cited tool run - both: added coverage-first findings instructions and output length guidance - CLAUDE.md: `graphite-stack-updater` does not exist on disk; the false "all skills have user-invocable: true" claim removed - README: `/review-pr 123` takes a branch or commit range, not a PR number Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…t, fix retired tool names Opus 5 migration, deltas 1/2/3/4: - stack-splitter: removed "Estimated total review time: 2-3 hours" and "faster review velocity expected" from the canonical example (both reproduce every run and neither is measurable); trailing 8-item self-review checklist reframed as upfront constraints on a valid split; `await Bash(...)` replaced with the real bash + MCP call; output length budget added - resolve-pr-issues: inline-comment-group dispatch now caps at 3 concurrent agents, matching its CI-failure and review-body siblings - review-executor: `/agent review-executor-agent` is not a slash command; replaced with the real Task dispatch form - work-through-pr-comments: get_pull_request / _comments / _reviews were consolidated into mcp__github__pull_request_read - comment-resolver, review-executor: report length caps plus coverage instructions, since both reports land in a caller's context Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…add -A, bump to 2.4.0 Opus 5 migration, remaining broken references plus the mandatory version bump: - start-linear-task, linear-task-and-pr-from-changes: `mcp__linear__create_issue` does not exist. Verified against the live Linear MCP tool list: the real name is `save_issue`. `get_user`, `get_issue`, `list_teams`, `list_projects` and `list_issue_labels` alongside it are all real and left alone - linear-task-and-pr-from-changes: `git add -A` replaced with explicit per-file staging, with the reason inline so it does not get reverted - start-linear-task: the `--skip-graphite` example flag is not in the command's own argument table; corrected to `--use-graphite false` - update-graphite-stack: two references to a `pr-issue-resolver` skill that does not exist (the skill is `resolve-pr-issues`); removed the optional refactorer-agent "verify changes maintain quality" follow-up pass - pr-guide: concurrency cap on the inline-comment path, matching SKILL.md - both Linear commands: decorative CRITICAL/ALL-CAPS emphasis softened, wording otherwise unchanged - plugin.json 2.3.0 -> 2.4.0 and the matching root CLAUDE.md row. Both read 2.3.0 before this change, so they were already in agreement Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… skip heuristic, de-numeralize reviewability - Remove references to a `--severity` flag that review-pr.md never defines in its Inputs section. An undefined flag is silently ignored when passed, so the guidance described a control that does nothing. Both review-pr.md and review-code/SKILL.md now phrase presentation-time filtering without naming a flag. - Replace the "docs-only change needs no security pass" heuristic with "no code, config, or capability grants changed". In this repo markdown frontmatter carries `allowed-tools` grants, so a docs-only diff can still change what a component is permitted to do. - Keep the five reviewability tiers in stack-splitter.md and their observable anchors, but drop the 1-10 numerals and the averaged aggregate. Averaging ordinal tiers to one decimal manufactures precision the judgement does not have. Updated every downstream consumer: the per-PR output field, the example outputs in both stack-splitter.md and split-graphite-stack/SKILL.md, the stack-stats aggregate, and the "no PR scores below 4" constraint (now "no PR lands in the Difficult tier"). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
8324cd8 to
e845c70
Compare
| 1. PR #3 mixed CRUD + rate limiting | ||
| 2. PR #4 mixed new UI + major refactor | ||
| 3. Average reviewability score: 6.5/10 | ||
| 3. Two PRs landed at Challenging, the rest at Acceptable |
There was a problem hiding this comment.
| 3. Two PRs landed at Challenging, the rest at Acceptable | |
| 3. Two PRs mixed concerns, dragging the stack down to Acceptable at the bottom |
| - [ ] No PR is trivially small (< 50 lines) unless it's purely foundational | ||
| - Dependencies must form a valid DAG — a cycle means the split boundary is wrong | ||
| - Each PR has one clear primary purpose | ||
| - No PR lands in the Difficult tier; 2-6 PRs total is the usual range |
There was a problem hiding this comment.
🔵 info · correctness — the reframed constraint list drops "PR sizes are relatively balanced," which isn't among the removals the PR description names. The example's stats section still touts "Average PR size: ~350 lines (reduced from ~540)" (line 557) as a benefit, with nothing left in the constraints to drive it.
There was a problem hiding this comment.
Note
✅ Approved — see full review in the sticky comment ↑
1 participant
Applies Opus 5 migration fixes to
packages/plugins/development-pr-workflow(39 audit findings). Surgical edits only — no wholesale rewrites, noeffortLevelormodel:frontmatter changes.Deltas addressed
Delta 1 — Opus 5 self-verifies; drop mandated verification ceremony
agents/stack-splitter.md: the trailing 8-item self-review checklist over just-written output is reframed as upfront constraints on a valid split. The substance (DAG validity, one purpose per PR, tests with implementations) is kept; the post-hoc verification pass is not.skills/update-graphite-stack: removed the optional "invoke refactorer-agent to verify changes maintain quality" second-agent pass.agents/review-executor.md's "run the tests before claiming a fix works" — that constrains claims against reality, not ceremony.Delta 2 — floors become ceilings, plus when-NOT-to-delegate
skills/review-code: the Review Depth table read| Standard | 4 |/| Comprehensive | 8+ |under anAgentscolumn — a quota. Now "up to 4" / "up to 8" with an explicit note that these are ceilings, that only dimensions the diff actually raises should be staffed, and that analysis finishing in a handful of direct tool calls should not get an agent.commands/review-pr.md: the 4-phase / ~11-agent comprehensive pipeline now says up front that it is the maximum staffing for a broad cross-cutting diff, not a roster to complete. Same note added to the standard 4-agent sequence.skills/resolve-pr-issues: the inline-comment-group dispatch path had no concurrency cap while its CI-failure and review-body siblings capped at 3. Added a matching cap of 3, with the reason (more concurrent writers raises the cross-agent file-conflict rate Phase 5 then has to untangle). Mirrored intopr-guide.md's dispatch table.resolve-pr-issues' existing 3-concurrent CI-agent cap and its "orchestrator handles these directly — no subagent needed" routing, andresolve-all-prs'max(default 5).Delta 3 — coverage-first, filter downstream
commands/review-pr.mdandskills/review-code: findings sections had no coverage instruction. Both now say to report every genuine finding, mark severity rather than dropping, and record uncertainty on the finding instead of discarding it — with the explicit statement that--focus/--severityfilter what is presented, never what is looked for.agents/comment-resolver.mdandagents/review-executor.md: both now require reporting on every comment received, including unresolved ones, since an omitted comment reads to the caller as handled.CRITICAL:/ ALL-CAPS emphasis softened in the two Linear commands. The underlying sequencing instructions are unchanged.--focus/--severity/--scopeflags (user-selected, defaulting toall) and "skip nitpicks" where scoped to style tiers, andresolve-pr-issues'blocking: false(changes priority, drops nothing).Delta 4 — explicit length instructions
review-pr,review-code,stack-splitter,comment-resolver, andreview-executor. Highest priority on the last two, whose reports land directly in a caller's context.stack-splitter's ~260-line worked example now carries a note that it shows structure, not target length, and the Output Format section carries a concrete budget (~10-15 lines per PR, under ~150 total).Broken references
Every agent dispatch name was checked against frontmatter
name:across all plugins.skills/review-codedispatched 7 agents (style-enforcer,refactorer,code-explainer,security-analyzer,performance-analyzer,test-writer,context-loader) with neither the-agentsuffix their frontmatter declares nor theplugin:qualifier they need. All 7 corrected — 6 todevelopment-codebase-tools:*-agent,test-writertodevelopment-productivity:test-writer-agent.commands/review-pr.mddispatched 12 agents with the correct-agentsuffix but no plugin qualifier. All qualified, includingplan-reviewer-agent→development-planning:andmigration-assistant-agent→uniswap-integrations:. Theallowed-toolsTask(subagent_type:agent-orchestrator-agent)entry was qualified too.commands/work-through-pr-comments.md:allowed-toolslistedmcp__github__get_pull_request,_comments, and_reviews— all three consolidated intopull_request_read. Replaced, and Step 2 now names the three methods.commands/start-linear-task.mdandcommands/linear-task-and-pr-from-changes.md:mcp__linear__create_issuedoes not exist. Verified against the live Linear MCP tool list — the real name issave_issue. The neighbouringget_user,get_issue,list_teams,list_projects,list_issue_labelsare all real and were left alone.shared/linear-task-config.mdwas checked and uses only real names.skills/update-graphite-stack: two references to apr-issue-resolverskill that does not exist; the skill isresolve-pr-issues.agents/review-executor.md:/agent review-executor-agentis not a slash command; replaced with the realTask(subagent_type: ...)form.agents/stack-splitter.md:await Bash(...)presented as executable; replaced with the actual bash command plus a sentence naming themcp__nx-mcp__nx_project_detailscall.CLAUDE.md:30listed agraphite-stack-updatercomponent that does not exist on disk (the real component is theupdate-graphite-stackskill).CLAUDE.md:105claimed every skill hasuser-invocable: true; grep returns nothing. Replaced with what is actually true, plus the dispatch-name rule.README.md:67showed/review-pr 123; the command takes a branch or commit range.Invented numbers
agents/stack-splitter.md: removedEstimated total review time: 2-3 hours (vs 3-4 hours...)andFaster review velocity expected. Both sat inside the canonical example output, so they reproduced every run, and the model cannot measure review time.commands/review-pr.mdoutput schema:risk.breakdown0-10 numeric → qualitativelow | medium | highbands, all optional.testQuality.score0-100 → qualitativestrong | adequate | thin | absent.testingReview.coverage→ optional, with a requiredsourcefield naming the command or report the numbers came from. A real coverage run is a legitimate measured input; an estimated percentage is not.performanceReview.complexity→ optional, with a requiredtoolfield, and a comment stating that an eyeballed cyclomatic number is a fabrication and the field should be omitted instead.benchmarknarrowed from "Expected improvement" to "only if an actual benchmark was run".Other repo-rule fixes
commands/linear-task-and-pr-from-changes.md:308usedgit add -A. Replaced with explicit per-file staging plus agit status --shortcheck, and an inline note on why (blanket staging sweeps in untracked secrets and unrelated work).commands/start-linear-task.md:406documented a--skip-graphiteflag absent from that command's own argument table. Corrected to--use-graphite false, which the table does define.Version bump
packages/plugins/development-pr-workflow/.claude-plugin/plugin.json2.3.0 → 2.4.0, with the matching row in the rootCLAUDE.md. Both files read 2.3.0 before this change, so they were already in agreement — no drift found.MINOR is the right bump: dispatch names and fan-out caps change behavior, and nothing was removed or renamed.
Findings in the brief that do not exist here
agents/pr-creator.mdwith ~60 lines of fabricated MCP tool names (mcp__git_status,mcp__github_create_pr,mcp__graphite_stack_info). That file lives inpackages/plugins/development-planning/agents/, not this plugin. A grep for those three names across this plugin returns nothing. Not fixed here — out of scope.Decisions (not resolved, need a human call)
agents/stack-splitter.mdreviewability-score bands (1-10). Left as-is. In favour of keeping: the bands are a defined rubric, not a model estimate — each tier is anchored to observable inputs (lines changed, number of purposes, test presence, dependency clarity), and lines changed is directly measurable from the diff. In favour of removing: the resulting 1-10 number is presented with false precision ("Reviewability Score: 8/10", "Average reviewability score: 7.8/10"), the boundaries between the anchors are judgment calls, and averaging ordinal scores across PRs to one decimal place is not meaningful. A middle option is keeping the five named tiers (Excellent / Good / Acceptable / Challenging / Difficult) and dropping the numerals.skills/split-graphite-stack: "Be small enough to review in 15-30 minutes." Left as-is. It reads as a design target for the human author rather than a number the model reports, unlike the stack-splitter estimate that was removed. Flagging it because it is the same shape of claim.Test plan
Agent dispatch names enumerated from frontmatter (the source used for every correction):
Plugin structure validation:
Markdown lint (repo config):
bunx nx format:write --uncommittedran clean (no diagnostics beyond node deprecation warnings).Residual fabricated references, post-fix — the single hit is the new prose warning against the pattern: