Skip to content

fix(development-pr-workflow): backtest-change also triggers for NEW monitors/gates - #555

Open
dylanschmittle-uniswap wants to merge 1 commit into
nextfrom
dylanschmittle/backtest-change-new-monitor-trigger
Open

fix(development-pr-workflow): backtest-change also triggers for NEW monitors/gates#555
dylanschmittle-uniswap wants to merge 1 commit into
nextfrom
dylanschmittle/backtest-change-new-monitor-trigger

Conversation

@dylanschmittle-uniswap

Copy link
Copy Markdown
Contributor

Description

  • The /backtest-change command's description only advertised itself for changes to existing data-driven config ("Before opening a PR for a data-driven change…"), so authoring a brand-new monitor with a backtest-derived threshold never triggered it — even though the methodology is identical. Concrete miss: the INF-335 EGW-Go blue/green gate PR (Uniswap/backend#11307, 2026-07-30) ran the exact backtest-change workflow (21d live-history replay, threshold headroom statement) manually because the command's trigger language didn't cover new-monitor authoring.
  • Adds new-monitor/gate/alert trigger language plus a mention of joint rate-AND-floor replay for gate composites (raw per-group maxes are inflated by floor-masked drain-tails).
  • Blast radius: one frontmatter description string; command body untouched.

Testing

  • Frontmatter parses (single-line description string, no structural change)

…onitors/gates

The command only advertised itself for CHANGES to existing data-driven
config, so authoring a brand-new gate monitor with a backtest-derived
threshold (the INF-335 EGW-Go blue/green gate) didn't trigger it even
though the methodology is identical. Add new-monitor/gate trigger
language and mention joint rate-AND-floor replay for gate composites.
@vercel

vercel Bot commented Jul 30, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
ai-toolkit-slack-oauth-backend Ready Ready Preview Jul 30, 2026 7:35pm

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📚 Documentation Check ❌

Verdict: Failed

A file under packages/plugins/development-pr-workflow/ was modified (backtest-change.md frontmatter description) but the plugin's .claude-plugin/plugin.json version was not bumped, which is a blocking requirement per CLAUDE.md.


Summary\n\nThe PR updates the description field in the frontmatter of packages/plugins/development-pr-workflow/commands/backtest-change.md to broaden the skill's scope (now also covers NEW monitors/gates whose thresholds must be derived from historical data, not just existing data-driven changes).\n\n### Findings\n\n- Plugin version bump (BLOCKING, missing): This file lives under packages/plugins/development-pr-workflow/. Per CLAUDE.md's "Mandatory Version Bumping" section, any change to files in packages/plugins/<plugin-name>/ requires bumping that plugin's version in .claude-plugin/plugin.json. The current version is 2.2.0 and the diff does not touch plugin.json. Since this is a wording/documentation clarification to an existing command (not a new skill/command/agent, not a breaking change), a patch bump to 2.2.1 is appropriate.\n- CLAUDE.md: No plugin-level CLAUDE.md content describes this specific command's frontmatter in a way that would go stale — no update needed there.\n- README.md: No README changes needed; this is a scope clarification within an existing command's description, not a new user-facing feature requiring README documentation.\n- Changelog: Informational only — no changelog entry required for a description wording change.\n\nBecause fail_on_missing_version is true and the required version bump is absent, the verdict is FAIL.

Missing Updates

Type File Severity Reason
🏷️ version_bump packages/plugins/development-pr-workflow/.claude-plugin/plugin.json ❌ error Required version bump not included in this PR's diff despite modifying a file under this plugin's directory.

Suggestions (1)

💡 Inline suggestions have been posted as review comments. Click "Commit suggestion" to apply each fix directly.

  • packages/plugins/development-pr-workflow/.claude-plugin/plugin.json: backtest-change.md's frontmatter description was changed within packages/plugins/development-pr-workflow/. This is a wording/scope clarification to an existing command (not a new component, not breaking), so it warrants a patch bump from 2.2.0 to 2.2.1 per the plugin versioning rules in CLAUDE.md. Also update the version table entry for development-pr-workflow in the root CLAUDE.md to keep it in sync.

🤖 Generated by Claude Documentation Validator | Mode: suggest

@dylanschmittle-uniswap
dylanschmittle-uniswap marked this pull request as ready for review July 30, 2026 20:38
@dylanschmittle-uniswap
dylanschmittle-uniswap requested a review from a team as a code owner July 30, 2026 20:38
@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

🤖 Claude Code Review

Review complete

Review: backtest-change.md description update

This PR expands the description frontmatter of the backtest-change command to cover a NEW monitor/gate/alert whose threshold must be derived from historical data, aligning the description with the behavior added in the prior commit (96ed23b). The wording change is accurate and consistent with the command body (which already covers threshold/monitor evaluation against a historical series).

No bugs, security, or data-loss concerns — this is a documentation-only string change.

Blocking on repo convention (not a code bug)

Missing plugin version bump. This repo's CLAUDE.md has a mandatory rule:

CRITICAL: After making any changes to files in packages/plugins/, Claude Code MUST bump the plugin version. ... Include the version bump in the same commit as the plugin changes.

A user-facing description change qualifies as a documentation update → patch bump. This PR does not touch plugin.json:

  • packages/plugins/development-pr-workflow/.claude-plugin/plugin.json is still 2.2.0 → should be 2.2.1.
  • The version table in the root CLAUDE.md (development-pr-workflow | 2.2.0) should be updated to match.

I can't place this inline because plugin.json isn't part of the diff, so flagging it here.

Minor (non-blocking)

  • Description length. At ~600 characters with a nested parenthetical (... (or the proposed threshold against history, e.g. a joint rate-AND-floor replay for gate composites) ...), the string is long for a value that surfaces in the slash-command picker. Consider trimming the gate-composite example — it's already covered in the command body — to keep the picker entry scannable. Purely optional.
  • Stale plugin CLAUDE.md blurb. The plugin's CLAUDE.md still describes backtest-change without the NEW-monitor/gate scope (... monitor threshold, alert cadence, query, sampling rate, perf tweak ...). This drift predates the PR, but since you're already touching this command's description it'd be a good moment to sync that blurb too.

Verdict rationale

No runtime bug, so not REQUEST_CHANGES on correctness grounds — but the missing mandatory version bump is a concrete, non-optional repo requirement that should be addressed before merge. Using COMMENT.


💡 Want a fresh review? Add a comment containing @request-claude-review to trigger a new review at any time.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📋 Review verdict: COMMENT

👆 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant