refactor(development-pr-workflow): convert backtest-change from command to skill - #553
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📚 Documentation Check ✅Verdict: Passed The critical requirement (plugin version bump) is satisfied: development-pr-workflow's plugin.json was bumped 2.2.0 → 2.3.0 (correct minor bump for a new skill), and the root CLAUDE.md version table was kept in sync. Plugin CLAUDE.md and README.md were both updated to document the new backtest-change skill, the reworked thin-wrapper command, and the new file-structure entry. Only a minor, non-blocking ordering inconsistency was found. Documentation Review — PR #553Checks performed:
Minor, non-blocking observation: In Verdict: PASS — the blocking requirement (version bump) is met, and both CLAUDE.md files plus the README were meaningfully updated to reflect the new skill and the command's changed role. Suggestions (1)
🤖 Generated by Claude Documentation Validator | Mode: |
|
● Reviewed · against Note Approved — one warning on the wrapper command's skill reference. Converts AssessmentThe old command body carries over intact — all six workflow steps, the verdict taxonomy, and the output format are present in the new SKILL.md, plus the two documented additions (population separation, Suggestions
Tip Teach the reviewer. React 👍 on findings that helped, 👎 on false positives. Reply to push back or add context — we aggregate this weekly to tune the bot. Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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. 1 inline comment(s) are attached below.
…e/SKILL.md Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Superseded by new review after PR update
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.
…ugin to 2.3.0 Addresses review on #553: - register ./skills/backtest-change in plugin.json (skills are not auto-discovered in this repo; CLAUDE.md requires the manifest entry) - bump plugin 2.2.0 -> 2.3.0 (minor: new skill) - add model: opus to match sibling skills
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.
…able, sync root version table Addresses review round 2 on #553: - README skills table was missing the new skill (plugin inventory drift) - root CLAUDE.md version table still said 2.2.0 Also picks up pre-existing prettier debt in the README Commands table, which was already non-compliant under the repo's pinned prettier 2.8.8.
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.
|
|
||
| Validate a **data-driven change against real historical data before opening the PR** — and be willing to abandon or redirect the approach when the data says it won't work. This is the gate that stops a plausible-but-ineffective change from shipping. | ||
| Load and follow the **`backtest-change` skill** | ||
| (`packages/plugins/development-pr-workflow/skills/backtest-change/SKILL.md`), |
There was a problem hiding this comment.
packages/plugins/... doesn't exist in their checkout, so the Read misses and the wrapper falls back to name-based loading. Sibling commands use the plugin-relative form (start-linear-task.md:108, linear-task-and-pr-from-changes.md:141).
| (`packages/plugins/development-pr-workflow/skills/backtest-change/SKILL.md`), | |
| (`@../skills/backtest-change/SKILL.md`), |
There was a problem hiding this comment.
Note
✅ Approved — see full review in the sticky comment ↑
3 participants
What
Converts
backtest-changefrom a command into a skill, leaving the command as a thin wrapper that delegates to it.Why
backtest-changehas gate semantics — its description literally reads "Before opening a PR for a data-driven change… Refuses to ship when the data disproves the premise." Its entire value is firing at a moment the user will not think to invoke it.But commands only run when explicitly typed. A gate implemented as a command can never fire.
Observed live: during INC-357 a request came in to "add a monitor on container.memory.rss by host at ~700 MB warn / ~1.2 GB alert" — a textbook match for this command's description, right down to "monitor threshold". The command never entered the picture. The backtest happened only because a memory file independently said to always backtest tunes.
That backtest then rejected the proposed number: a third host had held 1.34–1.37 GB flat for ~21 consecutive hours with no incident, so a 1.2 GB critical would have paged for most of a day. Shipped 1.6 GB instead (Uniswap/backend#11074). Exactly the outcome this gate exists to produce — reached without it.
Audit context
Reviewed all 7 commands across the toolkit. This is the only miscategorisation:
backtest-changereview-prreview-codealready handles auto-triggerupdate-claude-md,claude-init-plusupdate-claude-docshandles auto-triggerlinear-task-and-pr-from-changes,start-linear-task,work-through-pr-commentsThe toolkit already uses a deliberate dual pattern — skill for the unprompted case, command for the parameterized run — which is what makes
backtest-changeconspicuous as the only gate with no skill half. This PR gives it one.Changes
skills/backtest-change/SKILL.md— the full workflow, with a description carrying upstream triggers so it fires when a number is proposed, not only when a PR is imminent: "add a monitor at 700MB", "set the threshold to N", "warn at X / critical at Y", "change the sampling rate", "tighten this alert".commands/backtest-change.mdas a thin wrapper preservingargument-hintand$ARGUMENTSparsing, so/backtest-changestill works for deliberate runs.CLAUDE.md— skill list, command list, directory tree.Two additions to the workflow, both learned from the INC-357 run:
REVISEDverdict — the goal is sound but the proposed number isn't. The existing taxonomy only had EFFECTIVE / PARTIAL / INEFFECTIVE, which didn't cover the most common real outcome.Plus a framing line stated explicitly: a user-supplied number is a hypothesis, not a specification.
Blast radius
Additive.
/backtest-changekeeps working with the same arguments and same behaviour. No other command, skill, or agent is touched. Docs/markdown only — no code, no hooks, no CI.Correction (review round 1)
The first revision of this body claimed "skills are auto-discovered from
skills/— noplugin.jsonchange needed", and ticked that as validated. That was wrong, and the review caught it as blocking.Skills in this repo are not auto-discovered —
CLAUDE.mdis explicit that askillsarray entry is required, and.claude/rules/plugin-docs.mdmakes "plugin.json skills array matches actual skill directories" a required verification step. Without it this PR would have shipped the file but not the behaviour it is justified by, which is the one failure mode the PR is supposed to prevent.I generalised from a different marketplace whose manifest omits the
skillsarray, and did not check this repo's. Both blocking items are fixed in030fb90; the version bump was also missed and is included.Validation
quick_validate.pypasses on the new skill./skills/backtest-changeinplugin.jsonand bumped the plugin2.2.0→2.3.0(minor, per CLAUDE.md: new skill)skillsmanifest exactly matches theskills/directory listing — no manifest-only or dir-only entriesmodel: opusadded, matching 4 of 5 sibling skillsargument-hint+allowed-tools, so/backtest-changeis unchanged from a caller's viewNot in this PR
The audit also flagged that
update-claude-mdexists as a command here and as a skill in the Uniswap backend repo. A rename would be the correct fix but it touches 22 files, includingscripts/lefthook/update-claude-docs.sh, which shells out toclaude -p "/update-claude-md"from a git hook. That is a separate change with a real breakage path and does not belong bundled with this one. Details in the PR discussion.Left as a draft for review.