Skip to content

Commit 7eb5c55

Browse files
dylanschmittle-uniswapgithub-actions[bot]wkoutre
authored
refactor(development-pr-workflow): convert backtest-change from command to skill (#553)
* refactor(development-pr-workflow): convert backtest-change from command to skill * Update packages/plugins/development-pr-workflow/skills/backtest-change/SKILL.md Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * fix(development-pr-workflow): register backtest-change skill, bump plugin 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 * docs(development-pr-workflow): add backtest-change to README skills table, 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. --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Nick Koutrelakos <wkoutre@users.noreply.github.com>
1 parent 4d5a387 commit 7eb5c55

6 files changed

Lines changed: 167 additions & 51 deletions

File tree

CLAUDE.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -226,7 +226,7 @@ All plugins follow semantic versioning (semver). Key versioning rules:
226226
| claude-setup | 1.0.5 |
227227
| development-codebase-tools | 2.6.2 |
228228
| development-planning | 2.0.7 |
229-
| development-pr-workflow | 2.2.0 |
229+
| development-pr-workflow | 2.3.0 |
230230
| development-productivity | 2.4.1 |
231231
| skill-management | 1.0.2 |
232232
| spec-workflow | 2.0.1 |

packages/plugins/development-pr-workflow/.claude-plugin/plugin.json

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "development-pr-workflow",
3-
"version": "2.2.0",
3+
"version": "2.3.0",
44
"description": "Pull request review, issue resolution, and Graphite stack management",
55
"author": {
66
"name": "Uniswap Labs",
@@ -10,6 +10,7 @@
1010
"keywords": ["pull-request", "pr-review", "graphite", "git"],
1111
"license": "MIT",
1212
"skills": [
13+
"./skills/backtest-change",
1314
"./skills/resolve-all-prs",
1415
"./skills/resolve-pr-issues",
1516
"./skills/review-code",

packages/plugins/development-pr-workflow/CLAUDE.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,13 +35,14 @@ This plugin supports two PR creation workflows:
3535

3636
- **resolve-all-prs**: Batch resolve issues across all your open PRs in parallel with auto-commit/push
3737
- **resolve-pr-issues**: Orchestrated PR issue resolution — triages inline comments, review bodies, and CI failures, then dispatches `comment-resolver-agent` subagents per file group for code changes and posts replies for items that don't need action
38+
- **backtest-change**: Gate a data-driven change (monitor threshold, alert cadence, query, sampling rate, perf tweak) on live historical data — replay old-vs-new over the same window and refuse to ship when the data disproves the premise. **Auto-triggers** whenever someone proposes a measurable change and names a number; the number is treated as a hypothesis, not a specification
3839
- **review-code**: Comprehensive code review for architecture, security, performance, and style
3940
- **split-graphite-stack**: Split monolithic branches into logical PR stacks
4041
- **update-graphite-stack**: Update Graphite PR stacks by resolving comments and syncing
4142

4243
### Commands (./commands/)
4344

44-
- **backtest-change**: Before opening a PR for a data-driven change (monitor threshold, alert cadence, query, sampling rate, perf tweak), pull live historical data, replay old-vs-new over the same window, and gate the PR on whether the data proves the change achieves its goal
45+
- **backtest-change**: Thin wrapper — deliberate, argument-taking entry point for the `backtest-change` skill above. The workflow lives in the skill so it can fire unprompted; a slash command only runs when explicitly typed
4546
- **review-pr**: Comprehensive multi-agent PR review for architecture, security, performance
4647
- **work-through-pr-comments**: Methodically work through PR comments in a conversational workflow
4748
- **linear-task-and-pr-from-changes**: Take local changes, create a Linear task, create a branch (optionally in a worktree), commit, and publish a PR
@@ -114,6 +115,7 @@ development-pr-workflow/
114115
├── .claude-plugin/
115116
│ └── plugin.json
116117
├── skills/
118+
│ ├── backtest-change/
117119
│ ├── resolve-all-prs/
118120
│ ├── resolve-pr-issues/
119121
│ ├── review-code/

packages/plugins/development-pr-workflow/README.md

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ claude /plugin install development-pr-workflow
1818

1919
| Skill | Description |
2020
| ------------------------- | ------------------------------------------------------------------- |
21+
| **backtest-change** | Gate a data-driven change on live historical data before it ships |
2122
| **resolve-all-prs** | Batch resolve issues on all your open PRs in parallel (auto-commit) |
2223
| **resolve-pr-issues** | Address PR review comments and fix CI failures |
2324
| **review-code** | Comprehensive code review using specialized agents |
@@ -26,13 +27,13 @@ claude /plugin install development-pr-workflow
2627

2728
## Commands
2829

29-
| Command | Description |
30-
| ---------------------------------- | -------------------------------------------- |
30+
| Command | Description |
31+
| ---------------------------------- | ------------------------------------------------------- |
3132
| `/backtest-change` | Validate a data-driven change vs live history before PR |
32-
| `/review-pr` | Review a pull request comprehensively |
33-
| `/work-through-pr-comments` | Methodically address PR comments |
34-
| `/start-linear-task` | Start working on a Linear task in a worktree |
35-
| `/linear-task-and-pr-from-changes` | Create Linear task and PR from local changes |
33+
| `/review-pr` | Review a pull request comprehensively |
34+
| `/work-through-pr-comments` | Methodically address PR comments |
35+
| `/start-linear-task` | Start working on a Linear task in a worktree |
36+
| `/linear-task-and-pr-from-changes` | Create Linear task and PR from local changes |
3637

3738
## Agents
3839

Lines changed: 15 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -1,54 +1,27 @@
11
---
2-
description: Before opening a PR for a data-driven change (monitor threshold, alert routing/renotify, metric query, sampling rate, perf tweak), validate it against LIVE historical data — replay old-vs-new and report whether it actually achieves its goal. Refuses to ship (or redirects) when the data disproves the premise.
2+
description: Backtest a data-driven change against LIVE historical data before opening the PR — replay old-vs-new and report whether it actually achieves its goal. Refuses to ship (or redirects) when the data disproves the premise. Deliberate entry point for the `backtest-change` skill.
33
argument-hint: [what you're about to change + the metric/signal it should move]
44
allowed-tools: Bash(*), Read(*), Grep(*), Glob(*), AskUserQuestion(*)
55
---
66

77
# Backtest a change before you PR it
88

9-
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.
9+
Load and follow the **`backtest-change` skill**
10+
(`packages/plugins/development-pr-workflow/skills/backtest-change/SKILL.md`),
11+
which holds the full workflow, verdict taxonomy, and output format.
1012

11-
Use it for any change whose success is measurable: monitor thresholds, alert routing / re-notify cadence, metric/log/trace queries, sampling rates, cache TTLs, rate limits, autoscaling params, or a perf optimization with a latency/throughput target.
13+
Parse `$ARGUMENTS` for the two inputs the skill requires:
1214

13-
## Inputs
15+
1. **The change** you intend to make (and the file(s), if known).
16+
2. **The goal** — which metric/signal should move, in which direction, by how
17+
much. If it isn't stated, ask. A backtest is meaningless without a target.
1418

15-
Parse `$ARGUMENTS` for: the change you intend to make (and the file(s) if known), and the **goal** it should achieve (what metric/signal should move, in which direction, by how much). If the goal isn't stated, ask for it — a backtest is meaningless without a target.
19+
Then run the skill's workflow and produce its backtest report.
1620

17-
## The discipline (why this exists)
21+
## Why this is a thin wrapper
1822

19-
A change that *looks* right is not the same as a change the data supports. The common failure is shipping a fix whose premise is wrong — the real driver was something else, so the metric never moves. Catch that **before** the PR, not in a post-merge validation.
20-
21-
## Workflow
22-
23-
1. **State the hypothesis precisely.** "Changing X will move metric M from ~A to ~B because C." Write it down. If you can't name the metric and the expected direction, stop and clarify.
24-
25-
2. **Find the authoritative data source** and respect sampling:
26-
- **Metrics** (standard Datadog metrics, `trace.*`, CloudWatch) are ~100% — use these to count rates/volumes/percentiles.
27-
- For alert/page/incident questions, pull the alert system's own event history (e.g. incident.io alerts), not a proxy.
28-
29-
3. **Pull a representative window** (typically 7–30 days; long enough to include the conditions the change targets).
30-
31-
4. **Replay old logic vs new logic over that same window.** Compute concrete deltas: old **N** vs new **M** — alerts fired, pages, error rate, p95, cost, rows, whatever the goal metric is. For threshold/monitor changes, evaluate both the old and the new condition against the historical series and count transitions. Identify *which groups/series* change, not just the aggregate.
32-
33-
5. **Classify the result:**
34-
- **EFFECTIVE** — data shows the change achieves the goal. Capture the old-vs-new numbers for the PR body.
35-
- **PARTIAL** — moves the metric but not enough / not for the cases that matter. Note the gap.
36-
- **INEFFECTIVE / PREMISE DISPROVED** — the data shows the real driver is elsewhere, or the change barely moves M. **Stop. Do not open the PR.** Report what the data actually shows and propose the lever that *would* work.
37-
38-
6. **Only if it holds up**, proceed to the change + PR, and put the backtest in the PR body: the hypothesis, the window, old-vs-new numbers, and a link to the live dashboard/query (prefer a link over stale typed numbers).
39-
40-
## Output
41-
42-
A short backtest report:
43-
44-
- **Hypothesis** and goal metric.
45-
- **Window + data source** (and any sampling caveat applied).
46-
- **Old vs new** with hard numbers and which groups changed.
47-
- **Verdict** (EFFECTIVE / PARTIAL / INEFFECTIVE) + recommendation. If INEFFECTIVE, the alternative lever.
48-
49-
## Principles
50-
51-
- Backtest **before** acting; never claim a change works without replaying data.
52-
- Be willing to **reverse** — a disproved premise is a successful backtest, not a failure.
53-
- Prefer **dashboard/query links** over typed numbers that go stale.
54-
- When the change spans owners (e.g. an external-config change + a repo change), say which half the data supports and which is out of scope.
23+
The workflow lives in a skill so that it **auto-triggers**. The highest-value
24+
case is someone proposing a threshold without thinking to invoke a backtest —
25+
"add a monitor at 700 MB warn / 1.2 GB critical" — and a slash command can only
26+
fire when it is explicitly typed. This command remains the deliberate entry point
27+
for when you *do* want a backtest on demand, with arguments.
Lines changed: 139 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,139 @@
1+
---
2+
name: backtest-change
3+
description: >
4+
Validate a data-driven change against LIVE historical data before it ships —
5+
replay old-vs-new over a real window, report whether it achieves its goal, and
6+
refuse to ship when the data disproves the premise. Fires whenever someone
7+
proposes a measurable change and names a number: "add a monitor at 700MB",
8+
"set the threshold to N", "warn at X / critical at Y", "alert when it exceeds
9+
N", "raise the timeout to 5s", "change the sampling rate", "bump the cache
10+
TTL", "tighten this alert", "loosen the threshold", "this should reduce the
11+
noise", "that will fix the p95" — and before opening any PR for a monitor
12+
threshold, alert routing or renotify cadence, metric/log/trace query, sampling
13+
rate, rate limit, autoscaling parameter, or a perf change with a latency or
14+
throughput target. A proposed number is a hypothesis, not a decision: backtest
15+
it and let the data override it. Always report old N vs new M with the window
16+
and data source. The /backtest-change command loads this same skill.
17+
allowed-tools: Bash, Read, Grep, Glob, AskUserQuestion
18+
model: opus
19+
---
20+
21+
# Backtest a change before you ship it
22+
23+
Validate a **data-driven change against real historical data before opening the
24+
PR** — and be willing to abandon or redirect the approach when the data says it
25+
won't work. This is the gate that stops a plausible-but-ineffective change from
26+
shipping.
27+
28+
Use it for any change whose success is measurable: monitor thresholds, alert
29+
routing / re-notify cadence, metric/log/trace queries, sampling rates, cache
30+
TTLs, rate limits, autoscaling params, or a perf optimization with a
31+
latency/throughput target.
32+
33+
## When this fires without being asked
34+
35+
The most valuable case is the one nobody invokes deliberately: **someone hands
36+
you a number.** "Add a monitor at 700 MB warn / 1.2 GB critical." "Set the
37+
timeout to 5s." A named threshold arrives with an implicit claim attached — that
38+
it separates the bad cases from the good ones. That claim is testable, and it is
39+
often wrong, because the person proposing it has seen the incident population and
40+
not the healthy population.
41+
42+
Treat a user-supplied number as a **hypothesis to test**, never as a
43+
specification to implement. Then say plainly what the data did to it.
44+
45+
Worked example. A request arrived for a per-host memory monitor at "700 MB warn /
46+
1.2 GB critical", motivated by two hosts that had OOM-wedged at a 2 GiB limit. A
47+
30-day replay found a third host that had held **1.34–1.37 GB flat for ~21
48+
consecutive hours with no incident** — so the proposed critical would have paged
49+
continuously for most of a day. Shipped 1.6 GB instead. The warning was kept
50+
exactly as proposed, because a *non-paging* tier is allowed to sit inside normal
51+
range when its job is lead time. Both halves of that outcome came from the
52+
backtest, not from the proposal.
53+
54+
## Inputs
55+
56+
You need: the change intended (and the file(s) if known), and the **goal** it
57+
should achieve — which metric/signal should move, in which direction, by how
58+
much. If the goal isn't stated, ask. A backtest is meaningless without a target.
59+
60+
When invoked as `/backtest-change`, parse `$ARGUMENTS` for the same two things.
61+
62+
## The discipline (why this exists)
63+
64+
A change that *looks* right is not the same as a change the data supports. The
65+
common failure is shipping a fix whose premise is wrong — the real driver was
66+
something else, so the metric never moves. Catch that **before** the PR, not in a
67+
post-merge validation.
68+
69+
## Workflow
70+
71+
1. **State the hypothesis precisely.** "Changing X will move metric M from ~A to
72+
~B because C." Write it down. If you can't name the metric and the expected
73+
direction, stop and clarify.
74+
75+
2. **Find the authoritative data source** and respect sampling:
76+
- **Metrics** (standard Datadog metrics, `trace.*`, CloudWatch) are ~100% —
77+
use these to count rates/volumes/percentiles.
78+
- Spans and logs are often heavily sampled on the success path; don't count
79+
volume from them.
80+
- For alert/page/incident questions, pull the alert system's own event history
81+
(e.g. incident.io alerts), not a proxy.
82+
- Beware aggregation defaults that hide the shape you're testing — e.g. a
83+
scalar query that silently averages a `max:` series returns avg-of-max and
84+
will understate peaks. Set the aggregator explicitly.
85+
86+
3. **Pull a representative window** (typically 7–30 days; long enough to include
87+
the conditions the change targets).
88+
89+
4. **Replay old logic vs new logic over that same window.** Compute concrete
90+
deltas: old **N** vs new **M** — alerts fired, pages, error rate, p95, cost,
91+
rows, whatever the goal metric is. For threshold/monitor changes, evaluate
92+
both the old and the new condition against the historical series and count
93+
transitions. Identify *which groups/series* change, not just the aggregate.
94+
For a brand-new monitor, "old" is 0 — say so explicitly rather than omitting it.
95+
96+
5. **Separate the two populations.** The threshold's whole job is to divide
97+
incident from healthy. Report the highest *legitimate* value observed and the
98+
lowest *incident* value. If they overlap, the threshold cannot work at any
99+
setting and the signal itself needs to change — say that instead of picking a
100+
number in the overlap.
101+
102+
6. **Classify the result:**
103+
- **EFFECTIVE** — data shows the change achieves the goal. Capture the
104+
old-vs-new numbers for the PR body.
105+
- **PARTIAL** — moves the metric but not enough / not for the cases that
106+
matter. Note the gap.
107+
- **INEFFECTIVE / PREMISE DISPROVED** — the data shows the real driver is
108+
elsewhere, or the change barely moves M. **Stop. Do not open the PR.**
109+
Report what the data actually shows and propose the lever that *would* work.
110+
- **REVISED** — the goal is sound but the proposed number isn't. Ship the
111+
corrected value and state prominently what you changed and why.
112+
113+
7. **Only if it holds up**, proceed to the change + PR, and put the backtest in
114+
the PR body: the hypothesis, the window, old-vs-new numbers, and a link to the
115+
live dashboard/query (prefer a link over stale typed numbers).
116+
117+
## Output
118+
119+
A short backtest report:
120+
121+
- **Hypothesis** and goal metric.
122+
- **Window + data source** (and any sampling caveat applied).
123+
- **Old vs new** with hard numbers and which groups changed.
124+
- **Population separation** — highest healthy value vs lowest incident value.
125+
- **Verdict** (EFFECTIVE / PARTIAL / INEFFECTIVE / REVISED) + recommendation. If
126+
INEFFECTIVE, the alternative lever.
127+
128+
## Principles
129+
130+
- Backtest **before** acting; never claim a change works without replaying data.
131+
- A user-supplied number is a hypothesis. Testing it is the job, not overriding
132+
the request — but when the data rejects it, say so and ship the corrected value.
133+
- Be willing to **reverse** — a disproved premise is a successful backtest, not a
134+
failure.
135+
- Prefer **dashboard/query links** over typed numbers that go stale.
136+
- Distinguish tiers by consequence: a chatty non-paging warning can be
137+
acceptable; a chatty page destroys trust in the monitor.
138+
- When the change spans owners (e.g. an external-config change + a repo change),
139+
say which half the data supports and which is out of scope.

0 commit comments

Comments
 (0)