diff --git a/domains/pr-workflow/skills/pr-validate/hooks/pr-evidence-gate.py b/domains/pr-workflow/skills/pr-validate/hooks/pr-evidence-gate.py new file mode 100755 index 00000000..20319fef --- /dev/null +++ b/domains/pr-workflow/skills/pr-validate/hooks/pr-evidence-gate.py @@ -0,0 +1,259 @@ +#!/usr/bin/env python3 +""" +Emit-time evidence gate (PreToolUse:Bash). + +Blocks outward-facing `gh pr|issue edit|create|comment` whose body contains, in +a validation-scoped paragraph, either a VERDICT/measurement claim with no +co-located inspectable ARTIFACT, or a DEFERRAL ("remains pending" / "not yet +verified" / TODO) with no co-located TRACKER. Rationale: an unbacked +"confirmed / verified / proven / observed / ingested / ✅" launders an +unverified assertion as fact under the author's name, and an untracked +"remains pending" decays to never (see +references/evidence-trustworthiness.md for the disciplines this enforces). + +The reference docs are the checklist; THIS is the trigger that runs it. + +Contract: reads PreToolUse JSON on stdin. Exit 0 = allow. Exit 2 = block +(stderr shown to the model). Fails OPEN on anything it cannot parse, so it +never bricks unrelated Bash commands. +""" +import json +import re +import sys + + +def _out_allow(): + sys.exit(0) + + +def _block(msg): + sys.stderr.write(msg) + sys.exit(2) + + +def main(): + try: + payload = json.load(sys.stdin) + except Exception: + _out_allow() + + if payload.get("tool_name") != "Bash": + _out_allow() + + cmd = (payload.get("tool_input") or {}).get("command", "") + # Outward-facing gh write surfaces: PR + issue, edit/create/comment. The + # surface set is wider than `gh pr edit|create` because the same unbacked + # verdict launders identically through a PR comment or an issue body. + if not re.search(r"\bgh\s+(?:pr|issue)\s+(?:edit|create|comment)\b", cmd): + _out_allow() + if "--body" not in cmd: # covers --body and --body-file + _out_allow() + + body = _extract_body(cmd) + if not body: + _out_allow() # can't read it -> don't block; nothing to scan + + violations = _scan(body) + if not violations: + _out_allow() + + lines = [ + "EVIDENCE GATE (PreToolUse) — blocked outward-facing " + "`gh pr|issue edit|create|comment`.", + "", + "A VERDICT claim needs a co-located inspectable ARTIFACT (https:// permalink,", + "actions/runs/, /blob/, or a `file.test.ts` ref in the same block). A", + "runtime OBSERVATION claim ('rendered', 'snapshot shows', 'byte-identical')", + "needs an OBSERVATION artifact — screenshot/recording/log/JSON/permalink; a", + "code /blob/ link witnesses code, not runtime behavior. A DEFERRAL ('remains", + "pending' / 'not yet verified' / TODO) needs a co-located TRACKER (#issue,", + "an issues/pull URL, 'triage', 'tracked in'). An unbacked verdict launders", + "an unverified claim as fact; an untracked deferral decays to never. All", + "are net-negative under your name. If the evidence exists on disk, BIND it:", + "every collected artifact the claim rests on gets referenced or re-hosted.", + "", + ] + for v in violations[:12]: + kind = v.get("kind", "verdict") + need = { + "verdict": "ARTIFACT", + "observation": "OBSERVATION ARTIFACT (screenshot/recording/log/" + "JSON/permalink — a code /blob/ link witnesses code," + " not runtime behavior)", + "deferral": "TRACKER", + }.get(kind, "ARTIFACT") + lines.append(f' • [{kind}] "{v["token"]}" (needs {need}) in: {v["snippet"]}') + lines += [ + "", + "Fix each: attach the artifact/tracker in the SAME block, or downgrade the", + "lane (⚠️ inconclusive / remove the claim). Then re-run.", + ] + _block("\n".join(lines) + "\n") + + +def _extract_body(cmd): + # 1) --body-file + m = re.search(r"--body-file[=\s]+(?:'([^']+)'|\"([^\"]+)\"|(\S+))", cmd) + if m: + path = m.group(1) or m.group(2) or m.group(3) + try: + with open(path, "r", encoding="utf-8") as fh: + return fh.read() + except Exception: + return "" + # 2) --body "$(cat <<'EOF' ... EOF)" heredoc + m = re.search(r"<<-?'?EOF'?\s*\n(.*?)\n\s*EOF", cmd, re.DOTALL) + if m: + return m.group(1) + # 3) --body '...' / --body "..." + m = re.search(r"--body[=\s]+'((?:[^']|'\\'')*)'", cmd, re.DOTALL) + if m: + return m.group(1) + m = re.search(r'--body[=\s]+"(.*?)"', cmd, re.DOTALL) + if m: + return m.group(1) + return "" + + +# Measurement/verdict tokens that assert a result was achieved/observed. +VERDICT = re.compile( + r"(?i)(?:\bcapture[ds]?\s+confirm\w*|\bconfirm(?:s|ed)\b|\bverif(?:y|ies|ied)\b" + r"|\bproven\b|\bobserved\b|\bingested\b|\bdemonstrat(?:e|es|ed)\b" + r"|does not drop\b|✅)" +) +# Inspectable artifact references: a URL/run-id/blob, or a genuine TEST/SPEC +# file reference (optional `:line`). Deliberately NOT arbitrary `*.js`/`*.ts` +# code tokens — a backticked transaction name like `/service-worker.js` is not +# evidence and must not mask a bare claim. +ARTIFACT = re.compile( + r"(?i)(?:https?://\S+|actions/runs/\d+|/blob/|\bjob/\d+" + r"|`?[\w./-]*\.(?:test|spec)\.[tj]sx?(?::\d+)?`?)" +) +# Runtime-OBSERVATION claims: assert something was *seen happening* in a live +# run (a render, a repro, a state snapshot). A code permalink (/blob/) witnesses +# code structure, NOT runtime behavior — so these get their own artifact class +# and are NOT excused by ARTIFACT. Added 2026-07-21 (PR #44610 postmortem: a +# validation comment shipped "rendered the toast byte-identically" + "snapshot +# shows X and Y simultaneously" with a full evidence bundle collected on disk +# and zero artifacts referenced; the old VERDICT vocabulary missed it). +OBSERVATION = re.compile( + r"(?i)(?:\brendered\b|byte-identical(?:ly)?|\bsnapshot\s+shows?\b" + r"|\bscreenshots?\s+show\w*|\breproduc(?:ed|es)\b" + r"|\bstill\s+(?:shown|shows|fails|failing|raises)\b" + r"|\bin\s+a\s+(?:real|live)\s+browser\b|\blive\s+test\s+build\b" + r"|\bin\s+two\s+independent\s+runs\b|\bworks\s+as\s+described\b)" +) +# What witnesses a runtime observation: an image/recording embed or host, a +# log/HAR/JSON dump, a Sentry permalink, or a CI-run artifact. A named capture +# file (e.g. `flag-on-failure-state.json`) counts at draft time — the +# functional-links rule still requires re-hosting before the reader sees it. +OBS_ARTIFACT = re.compile( + r"(?i)(?:!\[|user-images\.githubusercontent|user-attachments" + r"|gist\.github|sentry\.io/\S+|actions/runs/\d+" + r"|\b[\w./-]+\.(?:png|jpe?g|gif|mp4|webm|har|log|json)\b)" +) +# Deferral/disclosure tokens: an honest "not done yet" — +# "disclosure is not discharge". Excused only by a co-located TRACKER — an +# artifact does not discharge a pending item; a tracked follow-up does. Kept +# tight (no bare "to do") and scope-gated so normal prose does not trip it. +DEFERRAL = re.compile( + r"(?i)(?:remains?\s+pending\b|still\s+pending\b|not\s+yet\s+verif\w*" + r"|not\s+yet\s+captur\w*|\bTODO\b" + r"|(?:capture|end-to-end|live|e2e)[^.\n]{0,40}\bpending\b)" +) +TRACKER = re.compile( + r"(?i)(?:#\d+|https?://\S*(?:issues|pull)/\d+|\btriage\b|follow-?up|tracked\s+in)" +) + + +# A paragraph is policed only if it is a validation CLAIM, not design prose: +# - it sits under an evidence/verification/validation heading, OR +# - it carries a status verdict emoji (✅ ❌ ⚠️), OR +# - it is about a capture/falsifier/ingestion. +# This keeps casual "verified"/"confirms" in Reviewer-notes / Description out. +SCOPE_HEADING = re.compile(r"(?i)\b(validation|verification|evidence)\b") +SCOPE_PARA = re.compile( + r"(?i)(?:[✅❌⚠️]|\bcaptur|\bfalsif|\bingest" + # distinctive observation markers — generic "rendered" alone does NOT put + # a paragraph in scope, so refactor prose stays unpoliced + r"|\bsnapshot\b|\bscreenshot|byte-identical|\bworks\s+as\s+described\b" + r"|\bin\s+two\s+independent\s+runs\b|\bin\s+a\s+(?:real|live)\s+browser\b)" +) + + +def _scan(body): + # Strip bot-generated summary block — not our claim. + body = re.sub(r".*?", + "", body, flags=re.DOTALL) + violations = [] + section = "" + for block in re.split(r"(?m)^(?=\s*#{1,6}\s)", body): + hm = re.match(r"\s*#{1,6}\s*(.+)", block) + if hm: + section = hm.group(1) + section_in_scope = bool(SCOPE_HEADING.search(section)) + for para in re.split(r"\n\s*\n", block): + scan_lines = [] + for ln in para.splitlines(): + s = ln.strip() + if re.match(r"-\s*\[[ xX]\]", s): # checklist item + continue + if s.startswith(">"): # blockquote (bot NOTE) + continue + if s.startswith("_Status key"): # legend + continue + if s.startswith("#"): # heading line + continue + scan_lines.append(ln) + chunk = "\n".join(scan_lines) + if not chunk.strip(): + continue + if not (section_in_scope or SCOPE_PARA.search(chunk)): + continue + # A markdown table row is its own claim unit — scan each row so an + # artifact two rows down cannot excuse a bare row. + units = chunk.splitlines() if chunk.lstrip().startswith("|") else [chunk] + for unit in units: + snip = re.sub(r"\s+", " ", unit.strip())[:120] + # VERDICT claim: excused by a co-located inspectable artifact. + if not ARTIFACT.search(unit): + for m in VERDICT.finditer(unit): + if _negated(unit, m.start()): + continue # "not verified" / "unproven" is not a claim + violations.append( + {"token": m.group(0), "snippet": snip, "kind": "verdict"}) + break + # OBSERVATION claim: needs an observation-class artifact + # (screenshot/recording/log/JSON/Sentry or run permalink). + # A /blob/ code permalink does NOT excuse it. + if not OBS_ARTIFACT.search(unit): + for m in OBSERVATION.finditer(unit): + if _negated(unit, m.start()): + continue + violations.append( + {"token": m.group(0), "snippet": snip, + "kind": "observation"}) + break + # DEFERRAL: excused by a co-located tracker, NOT by an artifact — + # a link to the thing doesn't discharge "haven't done it yet". + if not TRACKER.search(unit): + dm = DEFERRAL.search(unit) + if dm: + violations.append( + {"token": dm.group(0), "snippet": snip, "kind": "deferral"}) + return violations + + +def _negated(text, pos): + """A verdict token preceded by a negator is a hedge, not a claim.""" + pre = text[max(0, pos - 16):pos].lower() + if re.search(r"\b(not|never|no|isn't|aren't|cannot|can't|without|un|yet)\s*$", pre): + return True + # 'unverified' / 'unproven' — negator fused onto the token + if pre.endswith("un"): + return True + return False + + +if __name__ == "__main__": + main() diff --git a/domains/pr-workflow/skills/pr-validate/references/claim-extraction.md b/domains/pr-workflow/skills/pr-validate/references/claim-extraction.md new file mode 100644 index 00000000..11ec8839 --- /dev/null +++ b/domains/pr-workflow/skills/pr-validate/references/claim-extraction.md @@ -0,0 +1,61 @@ +# Claim extraction + +Before choosing any lane, turn the PR into a **falsifiable, surface-specific claim**. Every lane is only as good as the claim it tests. A vague claim ("improves perf", "fixes the bug") can't be proven or refuted; a sharp claim names the precondition, action, observable outcome, and what would disprove it. + +## Read these, in order + +1. **PR body** — Description (what/why), `Fixes #N`, Manual testing steps, the Before/After intent. +2. **Linked issue(s)** — the bug report / acceptance criteria; "Steps to reproduce" and "Expected vs actual" are the claim in the reporter's words. +3. **The diff** (`gh pr diff`) — what actually changed: which surfaces, controllers, modules. Anchor the claim to what the code can do, not only what the body promises. +4. **Labels / type** — bug vs feat vs perf vs refactor changes the claim shape (see special cases). + +## Extraction steps + +1. **Asserted change** — what does the PR say it does? (body + issue) +2. **Anchor to the diff** — which surface/module changed? Reconcile intent with the diff. If the body promises X but the diff can't deliver X, **flag the drift** — that's a finding, not a claim. +3. **Phrase as falsifiable** — `Given , when , then .` The outcome must be observable and checkable. Replace vague verbs (improve / fix / handle / support) with the concrete observable. +4. **Pin the surface + reachability** — exact screen / API / metric. Reachable in the default fixture, or does it need state seeding, a feature flag, or a fallback surface? + - **A surface need not be a screen.** A pipeline's job graph, a build artifact, a policy file, a telemetry shape, or a harness's determinism are all legitimate surfaces with their own falsifiers. Do not force a user-visible observable onto a claim that does not have one — routing a CI or build claim through a product effect is the *wrong* bar, not a stricter one. + - **When the changed code is the automation, the PR's own run may not exercise it.** A CI-config diff commonly skips the very path it edits (build reuse, `needs-*` resolution, event-type conditions). Execute the changed workflow where its trigger conditions hold — a test fork, a branch whose name satisfies the condition — with the failure state forced. Name that substitution explicitly; a claim about *this* repo's pipeline is not proven by a run on another. +5. **Classify the type** → routes to lanes via the matching guide: visible UI · non-visible perf · telemetry · persisted-state · build-output · behavior-no-UI. +6. **Decompose mixed claims** — a PR that changes UI *and* shifts a metric is two claims; validate each. + +## Claim Card (output) + +``` +Claim: Given , when , then . +Surface: (reachable? seed / flag / fallback: …) +Type: → lanes <…> +Falsifier: +Baseline: +``` + +One card per claim. For a refactor, the claim is a **negation** (see below). + +## Claim quality bar + +A good claim is **falsifiable** (observable outcome + clear falsifier), **surface-specific** (names the exact screen/API/metric, not "the app"), **diff-anchored** (the changed code can plausibly produce it), **bounded** (one behavior, one precondition), and **measurable** where quantitative (a number + threshold, not "faster"). + +## Anti-patterns → refinements + +| Vague claim | Refined | +|---|---| +| "Improves performance" | "Opening the Activity tab: TBT drops below 200ms (was >600ms)" — name the interaction, metric, threshold | +| "Fixes the bug" | "With privacy mode on, the Perps tab balance is masked" — observable behavior + precondition + surface | +| "Refactor, no behavior change" | Negation claim: "behavior of `` is unchanged" → prove via a regression test staying green / snapshot / identical output, **not** a screenshot | +| "Adds a null check" (restates the diff) | "No crash when `` is null on ``" — the behavior, not the code | +| Body promises X, diff does Y | Not a claim — **flag the drift** to the author | + +## Special cases + +- **Refactor / no-op:** the claim is "nothing observable changed." Falsifier = any behavior/output diff. Prove via a regression test staying green, an empty snapshot diff, identical bundle/output, or a benchmark within noise. A passing screenshot proves nothing here. +- **Bug fix:** the strongest claim form ships its own falsifier — a test that fails on `main` and passes on the branch. Extract the claim straight from the issue's "Expected vs actual." +- **Perf:** always quantify — metric + interaction + threshold + baseline. Without a number it isn't falsifiable. +- **Persisted-state / migration:** claim = "upgrading from `` preserves `` and applies ``." Falsifier = corrupted/lost state. Baseline = a profile from the prior version. +- **Flag-gated:** two claims, one per flag state. + +## Worked examples + +- **Visible:** body "privacy mode doesn't hide the Perps balance"; issue: expected masked, actual visible; diff touches the Perps balance component. → **Claim:** *Given privacy mode on, when I open the Perps tab, the balance is masked.* **Surface:** Perps tab (gated → fallback: Shield entry modal). **Type:** visible. **Falsifier:** balance digits visible under privacy mode. **Baseline:** same flow on base reproduces the bug. +- **Perf:** body "defer Rive wasm at startup"; diff: dynamic `import()` of the Rive runtime. → **Claim:** *On cold start of the home view, the Rive wasm chunk is not requested until the animation surface mounts.* **Surface:** startup network + chunk graph. **Type:** perf. **Falsifier:** the chunk appears in the cold-start waterfall. **Baseline:** base requests it at startup. +- **Migration:** diff adds a state migration. → **Claim:** *Loading a profile from `` applies the migration; `changedKeys` covers only the touched controllers; all other state intact.* **Type:** state. **Falsifier:** an untouched controller mutated, or migrated state malformed. **Baseline:** a prior-version profile. diff --git a/domains/pr-workflow/skills/pr-validate/references/evidence-catalog.md b/domains/pr-workflow/skills/pr-validate/references/evidence-catalog.md new file mode 100644 index 00000000..1957746b --- /dev/null +++ b/domains/pr-workflow/skills/pr-validate/references/evidence-catalog.md @@ -0,0 +1,78 @@ +# Evidence catalog — extension + +The menu of evidence kinds for validating a `metamask-extension` PR, with **what each proves** and **which skill or tool captures it**. This is a matching guide, not a capture cookbook — for capture mechanics it defers to the sibling skills (`visual-testing`, `performance-testing`, `e2e-test`, `e2e-flakiness-patterns`, `component-view-test`, `test-i18n-usage`, `unit-testing`, `ab-testing`) and to AEP. + +Pick the evidence that would **falsify the claim if it were false**. Prefer a lane that yields an artifact a reviewer can independently re-check (a link, image, number, or replayable trace) over prose. Match, then capture — don't run the whole menu. Verify any `yarn` script name against the repo's `package.json`; they drift. + +**Rely on CI for routine coverage.** Lint, build, typecheck, the full test suite, and changelog validation are already run by CI — cite the check result (e.g. "N pass / 0 fail at head") instead of re-collecting it. Spend independent evidence only on the claim's load-bearing falsifier, on specifically important/noteworthy areas (security, money, permissions, the exact changed surface), or where a green result could be vacuous/misattributed. Re-collecting what CI covers is bundle noise. And **don't restate CI results in the published comment** — the reviewer already sees the Checks tab; reference a CI result only to highlight something specific. + +**Publish falsifier-forward.** The published bundle should foreground **what would have falsified the claim and how each falsifier is closed** — the falsifier is the load-bearing content, not a footnote. Lead with the disproof attempt and the evidence that rules it out, not a lane inventory. + +## Matching guide (claim → lanes) + +| The PR claims… | Lead with | Corroborate | +|---|---|---| +| a visible UI behavior | visual (`visual-testing` / AEP `visual_validation`) | recording for motion; a11y | +| a fixed bug (any) | **falsifying test** — fails on `main`, passes on the branch | visual if visible; Sentry if it errored | +| a concurrency / ordering guarantee (retry, cancellation, supersession, debounce, race) | **deterministic interleaving test** — fake timers + concurrent launch force each race; assert the ordering/cancellation outcome | transition telemetry; live forced-race capture | +| preload / no-double-fetch / lazy-load | AEP `perf_validation` | DevTools CDP netlog, chunk membership | +| a render / over-render fix | WDYR + React DevTools | startup traces | +| interaction responsiveness | INP, long-task TBT | DevTools profile | +| startup / load timing | benchmark A/B (paired) | startup phase traces, FCP/LCP | +| smaller/cleaner bundle | bundle-size diff | chunk membership | +| a memory leak fixed / introduced | **retention-path from code** — holder → held set → outlived boundary | heap-over-a-flow + retainer graph; lifecycle test | +| an error/crash fixed | Sentry rate→0 | falsifying test, visual | +| a dependency change is safe | LavaMoat policy + manifest diff | bundle-size | +| persisted-state change | **state migration** — transform (defer to CI) + **double-apply idempotence** on combined state | unrelated-key preservation; vault round-trip | +| tx / confirmation behavior | transaction simulation | e2e trace | +| dapp / provider behavior | provider connectivity (EIP-1193/6963) | e2e trace | +| flag-gated behavior | feature-flag matrix (on/off) | visual per state | +| snap behavior | snaps execution | distributed trace | +| copy / localization | i18n usage | visual | + +## Lanes by family + +**Behavior & flow** +- **Visual before/after** — UI on a real headed build with controlled state/network. Capture via `visual-testing` (`yarn build:test:webpack` → `dist/chrome`; `mm launch` / `describe-screen` / `screenshot`; `mm mock-network` for degraded paths). Or AEP `visual_validation` (autonomous: deterministic seed + agent navigation). +- **E2E trace + video** — a replayable full-flow proof. Playwright: `yarn playwright test ` (trace `on` by default; view `yarn test:e2e:pw:report`). Selenium: `yarn test:e2e:single --browser chrome|firefox [--retries n]` (screenshots auto on failure). See `e2e-test`. +- **Deterministic interleaving test** — for a concurrency/ordering claim (retry, cancellation, supersession, debounce, locks, queues), the correctness *is* the ordering under races. Force each race deterministically: `jest.useFakeTimers()` + `advanceTimersByTimeAsync(DELAY)` to fire the delayed action at a known point, `Promise.all([opA, opB])` to overlap, `advanceTimersByTimeAsync(0)` to step to a precise interleaving point, then assert the ordering/cancellation outcome — including **asymmetric** guarantees (one path canceled, another must complete). **Trust-gate:** the test must actually interleave (time advanced into the pending window, concurrent op injected *during* it) — a sequential run exercises no race and is a vacuous green. +- **Falsifying regression test** — the strongest single proof a fix targets the bug: a new test that **fails on `main`, passes on the branch**. Show both runs. Reach for it on every bug fix. +- **Component / Storybook** — a component across states in isolation: `yarn storybook`, `yarn test-storybook` (visual + a11y via the Storybook a11y addon). See `component-view-test`. +- **Flaky-stability rerun** — run N× to prove non-flakiness (Playwright `--retries`, Selenium `--retries n`). See `e2e-flakiness-patterns`. + +**Performance & render** +- **Startup / custom traces** — which phase moved: `TraceName` spans in `shared/lib/trace.ts`, read via `window.stateHooks.getCustomTraces()` (test/debug). LCP fallback mark `mm-hero-painted`. +- **Web vitals** — `window.stateHooks.getWebVitalsMetrics()` → INP/FCP/LCP/CLS (`ui/helpers/utils/web-vitals.ts`, attribution build). Note: INP fires on all pages; **FCP/LCP/CLS don't fire on popup pages** (sidepanel/E2E only). +- **Long-task / TBT** — main-thread blocking: `window.stateHooks.getLongTaskMetricsWithTBT()` (`ui/helpers/utils/performance-observers.ts`). TBT lives here, not in the web-vitals lane. +- **React render & selector** — WDYR (`ENABLE_WHY_DID_YOU_RENDER`, wired in `app/scripts/development/wdyr.ts`) for unnecessary re-renders; `yarn devtools:react` for the flame graph. Selectors use `reselect`; no built-in call counter — prove via WDYR. +- **Benchmark A/B** — `yarn test:e2e:benchmark` (presets in `shared/constants/benchmarks.ts`). The rolling baseline sits behind a `continue-on-error` step and can silently freeze — verify it's current and prefer a **paired A/B** (build both refs, compare directly). See `performance-testing` / `ab-testing`. + - **Treatment check first** — before trusting a delta, confirm the mechanism under test is active in each arm (split chunk present in head, absent in base; the span emitted; the flag evaluated). An arm without the treatment delivered is a no-op, not a control — a paired A/B built on it reports noise as signal. + - **A null needs its power stated** — "no change" and "underpowered" print the same result. If the run-to-run spread is wider than the effect under test, report *not resolvable at this n* and name the smallest detectable effect. Correcting a known bias (warm-up, arm order) removes that bias only; it does not make the comparison trustworthy on its own. +- **DevTools / CDP** *(manual)* — flame chart, network waterfall, JS coverage, heap snapshots over a flow (memory leaks), CPU throttling (`Emulation.setCPUThrottlingRate`), animation FPS. No repo helper — DevTools or `mm cdp`. +- **Retention-path from code** — the lead lane for a memory-leak claim, and the one that works at review time: name the **holder** (listener, closure, module singleton, accumulating collection, timer), the **held set** (the specific objects pinned — list the closure's captures), and the **outlived boundary** (`destroy()`, stream close, instance replacement). Method: pair every acquire with its release site (`on`↔`removeListener`, push↔drain, assign↔null) — the absence of the pair, cited at the acquire site, is the finding. Distinguish bounded staleness from unbounded growth, and introduced from pre-existing. Corroborate with a lifecycle test (force the boundary, assert release) and heap-over-a-flow with the retainer graph naming the same path. + +**Build output** +- **Bundle-size diff** — measured grow/shrink (bundle-size CI or local build comparison). +- **Chunk membership** — a module moved to the intended lazy chunk (webpack build; source-map membership). +- **LavaMoat policy / supply-chain capability diff** — audit what a dependency change (bump/add) grants. For a bump the policy *will* change, so the bar is **not** "empty diff" — it's **every new grant justified by the dep's function**. Regenerate `lavamoat/webpack/.../policy.json` (`yarn webpack:lavamoat:policy:build`, `:mv2`/`:mv3`) and `git diff` across **all build variants** (a grant can appear in one and not others), then read **grant-by-grant**: new **globals** (`fetch`, `importScripts`, `WebAssembly`) / **builtins** (`fs`, `child_process`) on a dep that shouldn't need them, new **packages** edges to powerful APIs, or an identifier substitution (`pkgC>name` replacing `pkgB>pkgA>name` = possible dep swap). Falsifier: a surprising grant ("what's it using this for?"). Guide: [lavamoat policy-diff](https://lavamoat.github.io/guides/policy-diff/). +- **Manifest permissions diff** — no scope creep: `git diff app/manifest/v{2,3}/_base.json`. +- **Build-variant matrix** — works across types: `yarn build:test:flask` / `:beta` / `:mv2`. + +**Production telemetry** +- **Sentry query links** — before/after error-rate / transaction / latency, scoped to the release. For PRs that *add/change span instrumentation* (volume), use the analytics span-quota skill instead. + - For a **perf-targeting PR** the lead evidence is the measured impact — and CI publishes it: metamaskbot posts a base-vs-merge-base benchmark matrix on every PR, the published A/B. Read the **primary table (vs the baseline commit)**; ignore the *vs previous 5 runs on main* section — its baseline moves, so opposite-signed deltas for one metric across presets are drift. Mechanism evidence (chunk membership, a request absent from the network waterfall) proves the win *possible*, never that it *happened*. +- **Distributed traces** — a span/transaction now appears / is shaped correctly. + +**Extension integrity** +- **State migration** — a persisted-state change doesn't corrupt existing users. The transform itself is CI-covered: each migration ships `app/scripts/migrations/NNN.test.js` asserting `meta.version`, the shape, and that `changedKeys` covers only mutated controllers — **defer to it**. Independent evidence goes to what those per-path fixtures never do: **run the whole `migrate()` twice** on state that **combines** the branches (the migration rarely clears its source, so interrupted/re-entrant re-runs happen in the field) and assert the second application is a no-op — byte-identical output, empty `changedKeys`. Corroborate no-loss by counting migrated entries in vs out and confirming unrelated keys are byte-preserved; the removal/cleanup paths (a migration that strips entries) are where a double-apply most plausibly breaks. Scaffold with `./development/generate-migration.sh NNN`. Exemplar: #42297 (unified assets-controller migration) — idempotent across EVM + non-EVM + slip44-cleanup on combined synthetic state. +- **Vault / keyring round-trip** — lossless encrypt→decrypt (`app/scripts/lib/encryptor-factory.ts`; `test/e2e/tests/vault-corruption/`). +- **Transaction simulation / gas** — balance-changes/gas correct before submit (`app/scripts/lib/transaction/containers/enforced-simulations.ts`; `test/e2e/tests/simulation-details/`). +- **Provider / dapp connectivity** — injection + connect + requests: `yarn dapp` (test-dapp on :8080); EIP-6963 `test/e2e/provider/eip-6963.spec.js`. +- **Feature-flag matrix** — correct in both remote-flag states: flags come from the remote client-config API (not local config); mock the response in e2e to force each state (`test/e2e/tests/remote-feature-flag/`). +- **Snaps / multichain** — snap behavior (e.g. `snap_startTrace`): `test/e2e/snaps/`, flask build. +- **i18n usage** — no hardcoded strings; locales resolve: `yarn verify-locales` (`app/_locales/`). + +**CI, review & process** +- **CI check links** (`gh pr checks`), **coverage delta** (`yarn test:unit:coverage`; `codecov.yml`), automated-reviewer output, and **manual reproduction steps** (populates the PR template's Manual testing steps). + +Run the cheapest lane that yields an independently re-checkable artifact, confirm the claim holds, then escalate. Don't over-instrument a one-line copy fix; don't under-prove a startup-latency or migration claim with a single screenshot. diff --git a/domains/pr-workflow/skills/pr-validate/references/evidence-gate-setup.md b/domains/pr-workflow/skills/pr-validate/references/evidence-gate-setup.md new file mode 100644 index 00000000..a5c77648 --- /dev/null +++ b/domains/pr-workflow/skills/pr-validate/references/evidence-gate-setup.md @@ -0,0 +1,54 @@ +# Evidence gate — setup (optional, Claude Code only) + +`hooks/pr-evidence-gate.py` is an **optional** mechanical enforcement of the disciplines documented in [`evidence-trustworthiness.md`](./evidence-trustworthiness.md). It is a Claude Code `PreToolUse:Bash` hook: before an outward-facing `gh pr|issue edit|create|comment` runs, it scans the `--body`/`--body-file` for a validation-scoped claim that asserts a verdict, a runtime observation, or a deferral **without** a co-located inspectable artifact or tracker, and blocks the write if it finds one. + +The hook is **Claude-Code-specific**. Other operators (Cursor, Codex, plain review) don't get the mechanical gate — for them the same disciplines apply as *documentation*, self-enforced by reading `evidence-trustworthiness.md`. The hook is not required to use the skill; it just moves the checklist from "remember to run it" to "runs automatically at emit time." + +It **fails open**: anything it cannot parse (non-`gh` command, unreadable body, malformed JSON) is allowed through, so it never bricks unrelated Bash commands. It uses the Python 3 standard library only (`json`, `re`, `sys`) — no dependencies to install. + +## Wire it up (Claude Code `settings.json`) + +Add a `PreToolUse` hook with matcher `Bash` that runs the script with `python3`. Put this in your user `~/.claude/settings.json` or a project `.claude/settings.json`: + +```json +{ + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash", + "hooks": [ + { + "type": "command", + "command": "python3 /absolute/path/to/pr-validate/hooks/pr-evidence-gate.py" + } + ] + } + ] + } +} +``` + +Resolve the path to wherever `pr-validate` lives on disk. Note that `tools/install` copies only the `references`/`scripts`/`assets`/`adapters` bundles into `~/.claude/skills/mms-pr-validate/` — the `hooks/` directory is **not** part of the installed bundle. Point the `command` at your checked-out skills repo instead: + +``` +/domains/pr-workflow/skills/pr-validate/hooks/pr-evidence-gate.py +``` + +**When it blocks:** the hook exits `2` and prints the reason (which claim, what artifact/tracker it needs) to stderr. Claude Code surfaces that to the model, which self-corrects — attaches the missing artifact/tracker or downgrades the verdict — and re-posts. No manual intervention needed. + +## Two other setup requirements the skill needs + +These are independent of the hook; the skill needs them whether or not you install the gate. + +1. **`gh pr comment` must be permitted — pick a grant model.** pr-validate posts its evidence bundle as a PR review comment (`gh pr edit` if publishing into your own PR body). Four options, in descending order of standing safety: + + | Model | How | Tradeoff | + |---|---|---| + | **`ask` (recommended)** | `"Bash(gh pr comment:*)"` in `permissions.ask` | Per-post confirmation prompt. Combined with this hook (content gate) and a draft-confirm habit, that's three independent layers. | + | **`allow` + hook** | same pattern in `permissions.allow`, hook wired | Frictionless posting; safety rests entirely on the hook and your draft discipline. Only sensible where the hook is actually installed — not for operators without hook support. | + | **Allowlisted wrapper** | keep raw `gh pr comment` denied; allowlist a small script that takes `--repo`/`--pr`/`--body-file`, checks preconditions (canonical header present), and is the only sanctioned path | Tightest scoping — the raw verb stays blocked; costs a script to maintain. | + | **No grant — manual post** | the model prepares the body file; you run `gh pr comment --repo --body-file ` yourself | Zero standing grant; you are the bottleneck. The universal fallback, and the only option on operators with no permission system. | + + Avoid a bare **deny** on the comment verbs if you use this skill: it hard-blocks the publish step with no prompt, which reads as a mysterious failure mid-run. + +2. **Image re-hosting needs your own public evidence repo.** Screenshots and recordings captured locally must be re-hosted to a public URL before a reviewer can see them (see items 8–9 in `evidence-trustworthiness.md`). This repo is **yours to provide** — set it to a public repo you control, referenced here as ``. There is no shared/default host: parameterize it in your own configuration and push captures there, then reference the resulting raw URLs in the PR comment. Do not hardcode someone else's host. diff --git a/domains/pr-workflow/skills/pr-validate/references/evidence-trustworthiness.md b/domains/pr-workflow/skills/pr-validate/references/evidence-trustworthiness.md new file mode 100644 index 00000000..46f3e6f1 --- /dev/null +++ b/domains/pr-workflow/skills/pr-validate/references/evidence-trustworthiness.md @@ -0,0 +1,39 @@ +# Evidence trustworthiness (anti-reward-hacking) + +A green result is not proof. An agent — or an eager run — can produce evidence that *looks* like it validates the claim but doesn't. Before believing or publishing any lane, run it through this gate. The Claim Card's **Falsifier** is the anchor: trustworthy evidence is evidence that *could* have shown the falsifier and didn't. + +The gate is conjunctive — a fix for one item must re-pass all the others. Items 1–6 govern whether the evidence is real; items 7–16 govern whether a reader can trust and audit it. + +## The gate (per lane, before publish) + +1. **Non-empty & expected media** — the run produced artifacts of the expected kind. A "pass" with zero artifacts (a skipped or errored step) is not a pass. +2. **Shows the claimed surface** — the screenshot/recording is the Claim Card surface in the asserted state — not a loading spinner, an error toast, the wrong screen, or a pre-action frame. Eyeball it. +3. **Exercises the changed code** — the test/flow actually hits the diff. For a test: it **fails on `main`**. For a flow: the changed component/route is on the path. A green test that never imports the changed module proves nothing. +4. **Signal exceeds noise — and a null states its power** — a perf delta must be beyond run-to-run variance (paired A/B, multiple iterations); a 3% move on a noisy metric is not evidence. The same bar applies in reverse: when the spread is wider than the effect being looked for, the finding is **"not resolvable at this sample size"**, never "no change" — an underpowered run and a true null print the same word, and reporting the word alone lets the reader infer the stronger claim. State the smallest effect the design could have detected. + - **Removing a bias is not establishing validity.** Correcting a flaw you found (discarding a warm-up, alternating the starting arm, pinning CPU governor) removes *that* bias and licenses no more than that. It is not a trust gate, because a trust gate names how the evidence could **still** be vacuous — residual risk, not completed work. List what remains uncontrolled (thermal drift, background load, ordering within a round); an unenumerated confound reads as a nonexistent one. + - **When correcting an overclaim, cut the certainty, not the evidence.** A falsifier that actually fired is the strongest thing on the page — downgrade the conclusion around it, don't delete it with the overclaim. +5. **Could have failed** — the assertion has a reachable failure mode. Always-true assertions (`expect(true)`, a screenshot with no assertion, a query with no time bound) can't falsify anything. +6. **Right baseline** — "before" is the actual base ref / prior version / pre-window, not a stale or mismatched comparison. +7. **Independent & honestly labeled** — checksum every capture set. Byte-identical files across supposedly independent runs can't stand as separate observations: explain the identity (deterministic rendering, with provenance that does differ) or re-capture. Labels describe the observation, not the interpretation the claim hopes for. +8. **Findings ship with artifacts** — a findings write-up carries functional links to the re-hosted artifacts at draft time, not descriptions of files that exist only on the capturing machine. Code permalinks and a repro recipe corroborate the observation; they don't substitute for it. +9. **Signal is surfaced** — evidence is judged at the reader's eyes, not the author's disk. Every exhibit leads with what to open, where to look, and what it should show; a reader who didn't run the session should confirm the claim in ~30 seconds. Deltas are presented *as* deltas (annotated side-by-side, diff, cropped to the differing region), and bulk artifacts are excerpted to the discriminating lines with the full file linked as appendix. The converse also holds: never omit a valid, relevant dimension because it duplicates another's signal — exclusion requires invalidity or irrelevance, stated in one line. +10. **Sibling exhibits are format-uniform** — parallel rows, scenarios, or A/B legs carry the same evidence format and quality. The bar is the best sibling: if the presentation improves mid-session, re-normalize the whole document before publishing. An unexplained format gap reads as an evidence gap. +11. **Lanes derive from the claim, not from existing links** — validation rows are generated from the claim and the PR's Manual testing steps, and each row's payload is the captured output of executing that step. A row restating CI status duplicates the Checks tab and is deleted; a validation surface carries no CI references. Borrowed evidence (a sibling PR's capture, a unit test standing in for a named live surface) never upgrades an uncaptured lane to proven. +12. **Identifiers resolve in one click** — a bare trace/event/run id is a digging assignment, not evidence. Hyperlink each id to its resolving surface (a permalink, or a query pre-filtered to exactly those ids over an absolute window) or include the re-hosted captured output showing the discriminating fields. Ids captured locally that never reached the backend have no permalink — the capture is the only admissible form. +13. **Terminal exhibits are reader-native** — a positive verdict rests on a live link into the resolving system or a visual capture. Raw dumps (`.log`/`.json`/`.har`) are appendix-only: a link whose target is a dump moved the digging one hop away, it didn't remove it. +14. **The audit chain is mechanical** — inline data is a verbatim, greppable excerpt of its artifact (full-length identifiers, exact capture lines, fenced), and every repo-hosted artifact link is commit-pinned and line-anchored (`/blob//…#Lx-Ly`). Hand-transcribed digests read as claims in the shape of data; branch-ref links are mutable and not tamper-evident; unanchored file links land the reader at the top of a dump. +15. **Evidence is captured in its environment** — correct data alone cannot show it was captured live from a functioning system; extracted data is indistinguishable from data typed by hand. For a claim observable in a system of record (telemetry dashboards), the exhibit includes an in-environment capture: the resolving UI with the query, scope selectors, absolute time window, and result rows in frame, beside the live permalink. Quoted excerpts and data files sit in the appendix, never as the exhibit. +16. **"Proven" is an evidence predicate, not a run status** — success vocabulary (proven, validated, successful) applies only when the published surface already carries the exhibits this gate requires. A completed run without them is run-complete, evidence-owed. For telemetry-observable claims the default exhibit pair is fixed in advance — in-environment screenshot plus live permalink — and the capture executes before any write-up closes the session. + +## Lane-specific traps + +- **Visual:** spinner/skeleton mistaken for the loaded state; the toggle (privacy/redaction) not actually flipped; a cached screenshot from a prior run; a fallback surface shown without saying so. +- **Perf / benchmark:** a frozen/stale baseline (verify it's current; prefer paired A/B); single sample; warm-vs-cold mismatch; measuring a different interaction than the claim. +- **Test:** a snapshot regenerated to match the bug (`--updateSnapshot` masking a regression); the test mocks out the changed path; it passes on `main` too (so it isn't a regression test). +- **Telemetry:** query window excludes the release; the error regrouped under a different fingerprint; sample-rate makes "0 events" meaningless. +- **Migration:** only the happy path asserted; `changedKeys` not checked against actual mutations; no real prior-version fixture. +- **Coverage:** a line covered ≠ a behavior asserted (executed but never checked). + +## When evidence fails the gate + +Don't publish it. Either re-capture correctly, **downgrade the verdict to inconclusive** and name what's missing, or — if the evidence shows the claim is false — report the refutation (the validation succeeded; the change didn't). Never round a weak pass up to "proven." diff --git a/domains/pr-workflow/skills/pr-validate/references/principles.md b/domains/pr-workflow/skills/pr-validate/references/principles.md new file mode 100644 index 00000000..55cca696 --- /dev/null +++ b/domains/pr-workflow/skills/pr-validate/references/principles.md @@ -0,0 +1,71 @@ +# Principles + +Eighteen rules the rest of this skill implements. When a situation isn't covered by a +specific instruction, decide by these. + +## What you may claim + +- **Falsifiability** — name the observation that would disprove the claim, then go looking + for it. A review that cannot fail is not a review. +- **Diff-anchored** — the claim is what the code *can* do, not what the PR body promises. + Drift between them is a finding, not a claim. +- **Surface-specific, and a surface need not be a screen** — a job graph, a build artifact, + a policy file, or a telemetry shape are all legitimate surfaces with their own falsifiers. + Routing a CI or build claim through a product effect is the wrong bar, not a stricter one. + +## What counts as evidence + +- **Demonstrate, don't claim** — a verdict shows the objective met with an inspectable + artifact. Prose asserting it is a vacuous pass; report ⚠️ inconclusive instead. +- **In situ** — present output on the tool's own surface: the run page, the Discover view, + the trace waterfall, the console. Retyping output into the report launders evidence into + claim. Verbatim text is better than a hand-built table and still proves nothing about + provenance — and a transcription is a place to be selective without noticing you are + being selective. Check what a harness *imports*: a harness that re-implements the code + under test evidences only itself, however it is presented. +- **Reproducibility of assertions** — the bar is not that the reader *can* re-run it (a + working link is the floor) but that they *needn't*: the exhibit is complete enough — the + numbers, the window, the method, the control — that reading it makes the result + near-certain. The re-issuable link/query is a backstop for the skeptic, offered second, + never the headline. + +## Why believe it + +- **No vacuous passes** — green is not proof. Assert non-empty artifacts; ask whether the + assertion *could* have failed, and whether the test exercises the changed code. +- **Check the instrument, not just the result** — measurement design can manufacture a + finding. Verify the treatment is actually delivered in each arm before interpreting a delta. +- **Removing a bias is not establishing validity** — correcting a flaw you found licenses + only that correction. A trust gate names how the evidence could *still* be vacuous; if the + sentence describes work you did rather than risk that remains, it is not a gate. List what + stays uncontrolled — an unenumerated confound reads as a nonexistent one. +- **A null states its power** — when run-to-run spread exceeds the effect under test, report + *not resolvable at this n* and name the smallest detectable effect. An underpowered run and + a true null print the same word; reporting the word alone lets the reader infer the + stronger claim. +- **Premises are claims** — probe the *because* ("unavailable", "access-limited", "can't be + done here") as hard as the verdict. A false premise silently justifies the wrong method, + and a probe that *agrees* with a premise deserves more suspicion than one that contradicts + it — confirm the probe targeted the right thing. +- **Recompute stated counts** against the source before publishing. A number that was true of + an earlier draft's scope is the commonest stale fact. + +## When to stop + +- **Stop at the falsifier** — match the bar to the claim's risk. Evidence past the closed + falsifier is noise. +- **Defer to CI** where CI already covers it, unless that coverage is itself the point. +- **State what was not covered** — steps that could not be automated are recorded as open, + with the reason. A report listing only successes reads the same as one where nothing was + checked. + +## How it is handled + +- **Refutation is a successful validation** — report it, localize it, hand back the repro. + Don't fix, and don't publish a failure to someone else's PR unprompted. +- **Publish surface follows ownership** — the PR body when you authored it; a comment when + validating someone else's. +- **Scrub before publishing, confirm before any public write** — local paths and usernames + leak through failure summaries, and one PR's approval does not carry to the next. +- **Isolate concurrent runs** — colliding debug ports, artifact dirs, or upload paths + cross-contaminate evidence *silently*. That is an integrity failure, not flakiness. diff --git a/domains/pr-workflow/skills/pr-validate/references/worked-examples.md b/domains/pr-workflow/skills/pr-validate/references/worked-examples.md new file mode 100644 index 00000000..3fae12f0 --- /dev/null +++ b/domains/pr-workflow/skills/pr-validate/references/worked-examples.md @@ -0,0 +1,32 @@ +# Worked examples (end-to-end) + +Full runs: claim → lanes → capture → trust-gate → publish. The visual case is in the skill's main instructions; these cover the non-visual claim shapes. + +**Publish surface follows ownership.** Put the bundle in the **PR body** when you authored the PR — the evidence is part of your own claim. Post it as a **comment** when validating someone else's PR, rather than editing another author's body. The examples below say "in the PR body" for brevity; substitute a comment on PRs you don't own. + +## Perf — "defer Rive wasm at startup" +- **Claim:** on cold start of the home view, the Rive wasm chunk isn't requested until the animation surface mounts. **Surface:** startup network + chunk graph. **Falsifier:** the chunk appears in the cold-start waterfall. **Baseline:** base requests it at startup. +- **Lanes:** AEP perf validation (primary) → chunk membership + CDP netlog (corroborate). +- **Capture:** paired build of base vs head (`yarn webpack --test`); CDP netlog over cold start for each; source-map chunk membership of the Rive runtime. +- **Trust gate:** cold-vs-cold (not warm); the chunk truly absent (not deferred by a few ms); the netlog covers the whole startup window. +- **Publish:** before/after request list + a chunk-membership table in the PR body. No screenshot needed. + +## Migration — "add migration NNN" +- **Claim:** loading a profile from `` applies NNN; `changedKeys = {X, Y}`; all other state intact. **Falsifier:** an untouched controller mutated / malformed state. **Baseline:** a prior-version profile. +- **Lanes:** migration test (primary) → vault round-trip (if the vault is touched). +- **Capture:** run `NNN.test.js` (old-state-in → new-state-out); assert `changedKeys`; load a real prior-version profile and confirm boot. +- **Trust gate:** the test asserts more than the happy path; `changedKeys` matches the actual mutations; the fixture is a real prior profile, not synthetic. +- **Publish:** the `changedKeys` assertion + before/after state shape; link the test run. + +## Flag-gated — "Perps banner behind a remote feature flag" +- **Claim (×2):** flag on → banner shows; flag off → banner absent. **Surface:** home/Perps. **Falsifier:** banner state ≠ flag state. **Baseline:** each flag state is its own baseline. +- **Lanes:** flag matrix → visual per state. +- **Capture:** mock the client-config response for each flag state; screenshot each. +- **Trust gate:** the flag is actually toggled (read `remoteFeatureFlags`); two distinct states are shown, not the same frame twice. +- **Publish:** a two-up before/after (flag off / flag on) in the PR body. + +## Refactor / no-op — "extract a hook, no behavior change" +- **Claim (negation):** behavior of `` is unchanged. **Falsifier:** any output/behavior diff. **Baseline:** base behavior. +- **Lanes:** regression suite stays green + snapshot diff empty + bundle size within noise. +- **Trust gate:** snapshots were *not* regenerated to hide a diff; the tests actually cover the surface; bundle delta is within noise, not "small but real". +- **Publish:** "no behavior change — regression suite green, snapshots unchanged, bundle ±0"; link CI. A passing screenshot is not evidence here. diff --git a/domains/pr-workflow/skills/pr-validate/repos/metamask-extension.md b/domains/pr-workflow/skills/pr-validate/repos/metamask-extension.md new file mode 100644 index 00000000..d7ff2d75 --- /dev/null +++ b/domains/pr-workflow/skills/pr-validate/repos/metamask-extension.md @@ -0,0 +1,84 @@ +--- +repo: metamask-extension +parent: pr-validate +metadata: + type: pr-validation +--- + +# PR Validation — Extension + +Prove a PR does what it claims with **objective, reviewer-grade evidence**, then attach that evidence to the PR. This is the orchestration layer above the capture skills: it decides *what* evidence a PR needs and *assembles* it. For capture mechanics it defers to siblings — `visual-testing` (mm CLI screenshots/flows), `performance-testing`, `pr-manual-testing`, `pr-description` — and to the MetaMask Autonomous Engineering Platform (AEP) for autonomous runs. + +Complements `pr-readiness-check` (which checks that tests and guidelines are *present*); this skill proves the *behavior* is correct. + +## When to Use + +- Before marking a PR ready for review, or after a force-push, to produce the before/after a reviewer expects. +- To back a perf, telemetry, or UI claim with an artifact a reviewer can independently re-check. +- To assemble and publish an evidence bundle into the PR's Screenshots/Recordings section. + +Not for code-correctness review. + +The rules this skill decides by — falsifiability, in-situ evidence, trust gates, when to stop — are collected in `references/principles.md`. Read them once; they govern every case the instructions below do not name. + +## The core move: match evidence to the claim + +A PR makes a **falsifiable claim** ("privacy mode now hides the Perps balance"; "hovering the asset row preloads the chart with no double-fetch"; "this cuts startup latency"). Validation = pick the evidence that would **falsify the claim if it were false**, then capture it. Don't run a fixed checklist. + +First, write a **Claim Card** (`Given , when , then ` + surface, falsifier, baseline) from the PR body, the linked issue, and the diff — the linchpin; every lane is only as good as the claim. Full rubric, anti-patterns, and special cases (refactor/no-op, bug-fix, perf, migration, flag-gated): `references/claim-extraction.md`. Then choose lanes: + +| Claim shape | Lead with | Capture via | +|---|---|---| +| Visible UI change (layout, copy, show/hide, theme) | before/after screenshots | `visual-testing` (mm CLI), or AEP `visual_validation` | +| Motion / multi-step flow | screen recording → GIF | `visual-testing` + ffmpeg | +| Non-visible perf behavior (preload, no double-fetch, lazy-load, chunk membership) | falsifiable network/static assertions | DevTools/CDP netlog, AEP `perf_validation` | +| Latency / startup timing | benchmark numbers | `performance-testing`, DevTools profile | +| Telemetry / error-rate / latency in prod | Sentry query link (before/after window) | Sentry MCP / dashboard | +| Bundle / build output | bundle-size diff, chunk membership | build + source-map analysis | +| Behavior with no UI | targeted tests + repro | `pr-manual-testing`, CI checks | + +A PR that mixes claims (a UI fix that also shifts a metric) needs more than one lane, assembled into one bundle. Full lane menu with capture pointers: `references/evidence-catalog.md`. + +## AEP — autonomous validation + +The MetaMask Autonomous Engineering Platform (`MetaMask/metamask-autonomous-engineering-platform`) can validate a PR autonomously. Submit a task for the PR head with `taskClass: visual_validation` (visible behavior) or `perf_validation` (falsifiable network/static/smoke assertions). It checks out the PR, **deterministically seeds** extension state, has an agent **navigate** and capture, and returns an `evidenceBundle` of artifacts (screenshots / proven assertions) that it can publish to the PR body. + +The split that makes the evidence trustworthy: **seeding is deterministic (the platform), navigation is the agent.** That separation is why a screenshot proves the change instead of reward-hacking a loading screen. + +Output modes: `validation_only` (the default for these task classes), plus `pull_request` / `report_only` / `evidence_only`. See the AEP repo's `docs/` for how to run it. + +## Vacuous-pass discipline + +A green result is real only if the evidence bundle is **non-empty** and contains the expected media. An agent chain can "pass" by skipping with zero artifacts. Always confirm artifacts exist — and show the claimed surface — before believing a pass or publishing. Never upgrade a zero-artifact run to "proven". + +## Concurrent runs + +Two validations on one machine share five mutable resources: CDP debug ports, e2e harness service ports, artifact directories, evidence-host upload paths, and commit pins on published links. Collisions cross-contaminate evidence silently — another session's logs attributed to your run is an integrity failure, not flakiness. Derive ports per run, keep one e2e run per worktree, namespace artifact directories and upload paths by run id, and pin published links only after your own final upload lands. + +## Publishing evidence to the PR body + +Put the evidence where a reviewer expects it: the PR template's **Screenshots/Recordings → Before / After** section (see `pr-description`). Author-run validation publishes to the PR body, never as a PR comment. + +- Host images somewhere GitHub renders inline (an asset store the PR can reach) — a `localhost` or local file path will not render. +- Convert recordings to GIF (e.g. `ffmpeg` two-pass palette); webm/mp4 don't render inline in PR bodies. +- Pin repo-hosted artifact links to a commit with line anchors (`/blob//…#Lx-Ly`) — branch refs are mutable and not tamper-evident. +- Scrub local paths, usernames, and internal URLs from any narrative before posting. + +## Reporting + +Lead with the verdict and the claim it tests: + +``` +PR # — +Claim: <the falsifiable behavior under test> +Verdict: ✅ proven / ❌ refuted / ⚠️ inconclusive +Evidence: <lane> — <artifact or link>, ... +``` + +If a lane comes back inconclusive, say so and name what's missing. Verdict vocabulary is earned: report ✅ proven only when the published surface carries the exhibits `references/evidence-trustworthiness.md` requires — a completed run without them is run-complete, evidence-owed. + +## Boundaries + +- Orchestration + methodology; defers capture to `visual-testing` / `performance-testing` / AEP. +- Proves behavior, not code quality (pair with review skills) and not readiness *presence* (`pr-readiness-check`). +- Confirm before writing to a public PR body. diff --git a/domains/pr-workflow/skills/pr-validate/skill.md b/domains/pr-workflow/skills/pr-validate/skill.md new file mode 100644 index 00000000..e6968493 --- /dev/null +++ b/domains/pr-workflow/skills/pr-validate/skill.md @@ -0,0 +1,4 @@ +--- +name: pr-validate +description: Validate a PR with objective evidence that proves its behavioral claim — match evidence to the claim, capture it (AEP autonomous validation, visual-testing, performance-testing, Sentry, DevTools), and publish before/after into the PR body. Use when asked to validate or prove a PR, capture before/after evidence, attach screenshots/recordings/Sentry links to a PR, or confirm a change actually works before review. Trigger phrases include "validate this PR", "prove the fix", "before/after evidence", and "build an evidence bundle". +---