[DO-NOT-MERGE] trial branch: all skill PRs merged, for install and testing - #108
Draft
MajorLift wants to merge 154 commits into
Draft
[DO-NOT-MERGE] trial branch: all skill PRs merged, for install and testing#108MajorLift wants to merge 154 commits into
MajorLift wants to merge 154 commits into
Conversation
sentry-mcp-queries: document data-fidelity loss on older releases for longer-range (30D+) queries (sample-rate drift, extrapolation hiding thin samples, retention downsampling) and percentile (p75+) sample-size/quality filtering; add table rows + pitfalls for stored-span-count and superseded-patch releases. performance-attribution: new skill for attributing release-over-release p75/p95 movements to code changes via black-box diff analysis, with an extension repo file (Trace Explorer queries, key transactions, highest-sample-patch version selection, 90d-vs-30d empirics, hot-path files, core-package changelog analysis, worked v13.11->v13.15 catalogue). analytics-instrumentation: cross-link volume-estimation caveats to the new fidelity guidance. CHANGELOG updated.
…n-out, latent trace? param, trace-arg PR-review scan, per-name budget
…cidental instrumentation)
…-types, decompose-large-files
…ary-identification is step one of any migration
…d exception (bivariant callback `any`)
…und unit conversion, swap in the stronger derive-types example, slim CHANGELOG - migration-context-cost: line count is a factor not a non-factor; fan-in is a reading cost (not a change/review surface), fan-out is the change surface; drop the wrong "upstream types land first" and off-topic barrel bullet - decompose-large-files: the point is converting to TS in small self-contained units, not extraction - derive-types: replace the NetworkState restatement with the reinvented-messenger + hand-copied-return example (derive via `ReturnType<Action['handler']>`) - CHANGELOG: list the domain, not each skill
…action is optional
Two-phase retention review for JavaScript/TypeScript. Phase 1 is a static read of a diff: enumerate the retention primitives the change introduces — listeners, timers, pending-request registries, subscriptions, module singletons, growing collections — and pair every acquire with its release site. A primitive with a teardown is safe; one without is the finding. Phase 2 escalates to DevTools/CDP heap snapshots only for a primitive the read cannot pair. Leading with the read rather than the instrument settles most leak claims without ever taking a snapshot.
…oat-policy-diligence Two composing skills for "is this dependency change safe to take". `supply-chain-audit` is the breadth pass: Socket findings, `yarn npm audit` advisories, lockfile and manifest diffs, and the fronts no upstream scanner sees because they are things the repo does to its dependencies afterwards — yarn patches that modify dependency source at install, `resolutions` that force or stub versions, `npmAuditIgnoreAdvisories` suppression lists, CI actions riding mutable tags, and yarn plugins that execute at install. `lavamoat-policy-diligence` is the depth pass it delegates capability containment to. Because a LavaMoat policy is generated from a real run, every grant has a call site by construction — so "each addition is justified" is a tautology, not a finding. It instead reads each grant's use at the installed version to find its gate, and sorts into removable / removable-at-a-cost / load-bearing. Neither renders an accept/reject verdict; disposition belongs to the people who own the dependency. Adds a CODEOWNERS entry for the new domain, defaulted to the platform teams.
Replaces #56, whose skill.md shipped frontmatter with no body at all — the installer's bodyAfterFrontmatter() returned empty, so an agent loading it got a description and six references it had no instruction to read. pr-validate: for a PR's specific falsifiable claim, name the observation that would prove the claim false, gather it, and publish it into the PR body. Drives the AEP harness (visual_validation, perf_validation) as the primary engine, backed by a catalog of complementary lanes, a trustworthiness gate that rejects vacuous passes, and a publishing flow with an audience-reachability rule for re-hosted artifacts. falsifying-test: the strongest single proof that a fix targets the reported bug — a test that fails on the base commit and passes on the branch, with both runs shown. Its own falsifier is a base-commit failure for the wrong reason (import error, missing fixture, unrelated red), which looks identical in an exit code and proves nothing. pr-validate calls it as the engine behind lane B3. hooks/pr-evidence-gate.py enforces the trustworthiness gate at emit time, blocking an outward-facing write whose body carries an unbacked verdict, an untracked deferral, a CI restatement, a bare or truncated identifier, a mutable ref, a dump-as-resolver, a link-only or data-only exhibit, or a step waiver. It polices `gh api` body writes as well as the porcelain, since a PATCH to a comment is the same publish with a different spelling.
CHANGELOG.md tracks consumer-facing changes to the `@metamask/skills` package, per CONTRIBUTING's "CLI / tooling changes" section. No merged skill-only PR adds an entry (#80, #78, #70, #62, #61 all touch zero changelog lines). Carrying one here bought nothing and was the sole source of this branch's conflict with `main`, since every skill PR edits the same `[Unreleased]` block. Restoring the file to its merge-base state makes the branch conflict-free without a merge commit.
CHANGELOG.md tracks consumer-facing changes to the `@metamask/skills` package, per CONTRIBUTING's "CLI / tooling changes" section. No merged skill-only PR adds an entry (#80, #78, #70, #62, #61 all touch zero changelog lines). It was also the sole source of this branch's conflict with `main`, since every skill PR edits the same `[Unreleased]` block.
CHANGELOG.md tracks consumer-facing changes to the `@metamask/skills` package, per CONTRIBUTING's "CLI / tooling changes" section. No merged skill-only PR adds an entry (#80, #78, #70, #62, #61 all touch zero changelog lines). It was also the sole source of this branch's conflict with `main`, since every skill PR edits the same `[Unreleased]` block.
The selector and effect anti-pattern definitions existed in two places: these knowledge files, and the `performance` skill's own mm-* references already on main. Same patterns, same worked examples, two homes that would drift. knowledge/selector-anti-patterns.md and knowledge/effect-anti-patterns.md are now the canonical, platform-agnostic taxonomy — the union of both sides. The selector file absorbs mutation-in-result and over-broad-input from mm-selector-memoization; the effect file absorbs the dependency-side patterns from mm-hook-dependency-arrays and the lifecycle-side patterns (derived state, effect chains, uncancelled async). mm-selector-memoization.md keeps everything only it can say — the codebase's own selector creators, the verified instance table with file:line, the fix recipes, the scoped greps, the don't-over-correct caveats — and maps each generic pattern onto this codebase instead of redefining it. mm-hook-dependency-arrays.md keeps its richer JSON.stringify treatment and gains a scope note. Citations are by NAME, not by relative link. `install` copies domain knowledge/ and a skill's references/ as siblings under the installed skill directory, so `../../../knowledge/x.md` resolves in the repo and breaks once installed, and `../knowledge/x.md` does the reverse. Section anchors are dropped for the same reason — they broke the moment the taxonomy was renumbered. Also drops the CHANGELOG entry: that file tracks the @metamask/skills CLI package, no merged skill-only PR adds one, and it was this branch's sole conflict with main.
Folds in the react-render-proof skill (was #82) and the mobile reference-library additions (was #49). All three were the same effort seen from different ends — moving performance work earlier in the loop — and they share a substrate, so reviewing them apart meant reviewing the substrate three times. The loop this domain now covers: - catch it at review — effect/selector anti-pattern review skills, driven by the knowledge taxonomy - prove it moved — react-render-proof, with a delivery gate so an arm whose treatment never reached the bundle cannot report as a null - measure it honestly — data-analysis, benchmark hygiene, web-vitals framing - know the codebase — the mm-* reference library and its audit playbook Also neutralizes five references to private planning tickets, which do not belong on a public repository — they named internal epic and audit-ticket numbers. The surrounding guidance is unchanged; only the identifiers are gone.
These name MetaMask-org planning epics and audit tickets. The audience for this repo is the MetaMask org, for whom those identifiers are load-bearing context — they are where the guidance came from and where the follow-up lives. The scrub line is personal references, not org-internal ones.
Over-scrubbed. The audience is the MetaMask org, and the ticket is the evidence for the claim the item makes — that the LaunchDarkly provisioning blocker covers only the prod-flag half of that lane. Without it the example is an assertion. The scrub line is personal references, not org-internal ones.
The skill covers runtime retention behaviour, not code authoring, and `coding` reads as language- and style-level guidance. Registers `/domains/stability/` in CODEOWNERS alongside the other platform-owned domains.
…main Was a separate PR against `domains/testing`. It belongs here: its subject is whether a hand-written type agrees with its authoritative source, which is the question `derive-types` answers from the authoring side, and it shares this domain's premise that a green `tsc` is not evidence the types are correct. Directory name and frontmatter `name` already agree; only the domain moved.
…' into trial/combined
…nto trial/combined
MajorLift
changed the title
…finds it A precise annotation fed `any` at every call site is reportable by neither `tsc` nor `no-explicit-any`, and an ambient `declare module` in the path re-mints the `any` as a confident `string`. Both arms of the probe verified against `metamask-extension`. Also fixes `avoid-any`'s frontmatter, which did not parse as YAML.
Renamed on the security-domain branch; installs as `mms-lavamoat-policy`.
Renamed on the security-domain branch; installs as `mms-lavamoat-policy`.
…he breadth step Installs as `mms-lavamoat-policy`. A trim pass had deleted the step that asks how wide each grant is versus what the call actually uses, leaving only the reachability question. That step is what produces findings on grants that are reached and not stale — a bare `node:url` standing in for `fileURLToPath` and `pathToFileURL` clears every other check. Also corrects the stated mechanism. Policy is generated by static analysis (`lavamoat-tofu`, `@babel/parser`), not by observing a run, and the difference matters: a statically detected grant says nothing about whether our usage reaches it, which is where removal candidates come from. And scopes the tautology claim to the *existence* of a call site rather than its content — the policy records `"crypto": true` and discards what a reviewer needs, so recovering that is the job rather than something to skip.
…e its marker pair Three additions to the output contract. A fenced `diff` against `lavamoat/webpack/<variant>/policy-override.json` for every narrowing or removal in the table. Prose like "could be narrowed to `node:url.fileURLToPath`" makes the reviewer translate it into JSON; a diff makes it a decision. The override is the file a human edits — the generated `policy.json` is regenerated and would lose the change. Dotted paths already work there: `copy-webpack-plugin>serialize-javascript` is granted `crypto.getRandomValues`, so the precedent is cited rather than the support asserted. A marker pair, `LAVAMOAT_DILIGENCE_START`/`_END`. `evidence/skill.md` already described this contract as having "its own header and marker pair" and no such pair was ever defined, so a re-run appended a second comment. Deliberately not `VALIDATION_RUN_*` — sharing that region would let an evidence re-run silently eat a diligence comment. And a sharper rule on runtime claims. A permalink witnesses a line; it does not witness what the author ran. An `npm pack` result, a grep over a tarball, a byte-comparison across policy files — those read as properties of the package and are properties of an unwitnessed local run. State them as the search or publish the output. Bare integers in prose fall under the same rule.
A diligence comment renders no verdict and deliberately does not use the Validation Run envelope. That exemption meant it was checked by nothing: this gate only knew the Validation Run shape, and `pr-evidence-gate.py` by design does not trip on a body claiming no verdict. So every rule the diligence skills state about their own output — including "runtime claims need a runtime artifact" — had no execution path. It showed. A lavamoat comment shipped with no marker pair, an `npm pack` specifier set no reader could fetch, and two bare integers traceable to nothing. `--diligence` swaps the four envelope checks for that contract's own — its marker pair, its header, permalinks pinned to a tag or SHA rather than a branch head, and a runtime claim check asking for the thing a `/blob/` link cannot witness. 3, 8 and 9 report SKIP with the reason rather than passing silently, since a check that cannot fail should not read as a check that passed. Everything downstream of the envelope is shared, because those defects are shared. Run against the comment that prompted this, it fails 1, 5 and 13 and passes the rest.
This repository is public. Naming a private repository here discloses its existence,
its owner and roughly its contents to every reader — and a prohibition naming it
("do not re-host to X, it is private") discloses exactly as much as a recommendation
would. Four such references were doing that, and the guidance survives without them:
the rule is audience-reachability, which is stated directly rather than by example.
Two memory-file citations offered as "source of truth" pointed into a private repo, so
a reader was told to follow a rule whose justification they cannot open. The reasoning
is inlined; the pointer is gone.
The publish-surface snippet hardcoded a GitHub username, which decided the destination
for whoever ran it. Now derived from `gh api user --jq .login`, and the surrounding
prose is second-person rather than first — a shared skill has no "my PRs".
`/attest` is no longer linked to a personal repository. That leaves it named but not
resolvable, which is honest and is the smaller problem; the workflow depending on a
command nobody else has is tracked separately.
The re-hosting section named a personal S3 bucket, its region, its prefix layout, and stated that anonymous `GetObject` is enabled under `public/*`. That is a live unauthenticated endpoint advertised, with its structure, to every reader of a public repository — a larger disclosure than the repository names removed alongside it, and one that reads as configuration rather than as a secret, which is why it survived two passes over this file. Now `EVIDENCE_BUCKET` and `EVIDENCE_REGION` from the environment. The requirements the bucket must satisfy — anonymous GetObject under `public/*`, listing disabled — are stated, because those are the load-bearing part; the name never was. The G5 lane likewise named a private test fork, which carried both the org and a personal handle. Now "your own test fork".
…setup Removing the hardcoded username left `gh pr view --jq --arg me "$ME"`, which is not a thing gh supports — its built-in filter takes no --arg and the command dies with "accepts at most 1 arg(s)". Piped to real jq instead, and checked against both branches of the logic: a PR authored by someone else resolves to "skip", one authored by the caller to "body". Replacing the named bucket with `EVIDENCE_BUCKET` removed a working default and put nothing in its place, so the section told you to configure a bucket without saying what "conforming" meant. The policy is now stated: anonymous `s3:GetObject` under `public/*`, public-access blocks off for that bucket, `s3:ListBucket` to nobody. With a note that an org-owned bucket beats a personal one, since artifact links outlive their publisher.
A results section reached a public PR whose entire content was hand-typed to look like
terminal output. Three independent things had to hold for that, and all three did.
The gate is model-invoked, so it can be skipped: the publish and the gate ran as two
statements rather than one chain, and the verdict was read after the write.
The hook that fires on the publish call carried a SECOND, narrower copy of the rules —
keyed on verdict tokens — so a comment rendering no verdict satisfied neither copy.
Two rule sets means the weaker one governs whatever falls between them. The hook now
delegates to `attest-gate.sh`: one rule set, invoked by construction rather than by
choice, in the mode the body's markers imply. It fails CLOSED once it has identified a
body it is about to publish — an enforcement point that waves things through when it
cannot find its rules is not one.
And check 5 in `--diligence` had been rewritten as a phrase denylist ("npm pack",
"complete specifier set"), which is precisely the regression its own comment records as
having shipped four times: every property of plaintext is forgeable by whatever emits
the plaintext. It is a medium test again — if the artifact shows a command or a run
result, it owes the reader something fetchable. `/blob/` links are excluded, because a
permalink to a `.json` file satisfied a naive extension test and was the specific reason
the hand-typed section passed.
Comment-update URLs carry the comment id, not the issue's, so check 12 was asking
whether pull #5177261620 was open. Resolved through the API instead.
Four-arm verified: blocks the exact command and body that shipped; ignores `ls`; ignores
a `gh` read with no body write; refuses when the gate is unreachable.
`mms-evidence` named `/attest` as the gate its output passes and shipped no such thing. Anyone installing the skill got phase 0 as a script and a reference to a command only its author had, which is half a publish path for everyone else. Phase 0 is `attest-gate.sh`, already here. Phase 1 is three briefs sent to fresh instances — frame, coverage, and how it reads to a stranger — written out in `references/dispatched-passes.md` so dispatch does not depend on commands that live elsewhere. The check table in `references/phase-0-checks.md` is generated from the gate rather than retyped, so it cannot drift from what runs. Two things the port makes explicit that the original left to discipline. The gate must be the same shell chain as the publish, because running both and reading the verdict after the write is how a blocked artifact reached a public PR. And softening a check to fit the case in hand is called out as an anti-pattern: if the new version could be satisfied by better prose alone, it is no longer the check.
A three-arm build on extension#44727 established nothing. Denying sass-loader every URL and path builtin still compiled the extension, exactly as granting all of them did — so "it still builds" was the expected result either way and carried no information. Most grants are not on the startup path; that is usually why they look removable. So a build or a boot with the grant removed shows only that startup did not need it. The test has to name the scenario that actually executes the read — the error path formatting a span URL, the source-map write, the importer resolving a relative `@use`, the flag that turns the feature on — and run that. Two preconditions before either arm is believed, both cheap and both skipped on that run: confirm the effective policy really changed by merging the override into the base and printing the resource, and confirm a fully-denied arm actually fails. If denying everything passes, the grant is not enforced on that path, and "this suite does not arbitrate this grant" is the finding.
The installer emits `mms-react-render-delta`; the description advertised `/react-render-delta`, which resolves to nothing. Caught by the check #99 adds — this branch predates it and only fails once combined.
The installer emits `mms-debug`; the description advertised `/debug`.
The installer emits `mms-memory-leak`; the description advertised `/memory-leak`.
The installer emits `mms-race-condition-repro`; the description advertised `/race-condition-repro`.
The installer emits `mms-tsc-blindspots`; the description advertised `/tsc-blindspots`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Caution
DO NOT MERGE. This branch is a merge of fourteen open PRs, kept on
MetaMask/skillsso thewhole set can be installed and exercised as one thing. It has no content of its own — every line
in it belongs to one of the PRs below and should be reviewed there. It will be deleted once they
land.
Why this exists
No single PR installs into a working state.
mms-evidencealone routes to engines that live ineight different domains, so checking out its branch gets you an orchestrator naming skills that
are not installed. Reproducing the full set by hand means a fourteen-way merge against two
remotes, which is not a reasonable thing to ask of someone who wants to spend twenty minutes
trying it.
What is merged
39 skills across 13 PRs, plus one CI change.
--no-ffon top ofmain.jongsun/add/pr-validate-falsifying-testevidencejongsun/add/audit-skillsfalsifiers-first,unmeasured-join,instrument-check,scope-of-search,coverage-partition,selection-audit,silent-failure,unintended-breakage,red-on-base,distinguishing-observation,observability-gapjongsun/add/lane-graphslane-graphsjongsun/add/debug-orchestratordebugjongsun/add/memory-leak-hunt-skill-v2memory-leakjongsun/add/race-condition-proofrace-condition-reprojongsun/add/security-domainsupply-chain-audit,lavamoat-policyjongsun/add/privacy-egress-diligenceprivacy-egress-diligenceadd/performance(fork)react-render-delta,selector-antipattern-scan,effect-antipattern-scan,extension-profiling,data-analysis,benchmark-designadd/analytics-tracing-cross-ref(fork)sentry-quota,sentry-mcp-queries,sentry-grafana-correlation,grafana-tempo-queries,performance-attribution,instrumentationadd/typescript-typing-skill(fork)tsc-blindspots,avoid-any,derive-types,decompose-large-files,migration-context-costadd/platform-skills(fork)extension-errors-debugging,extension-lifecycle-decouplingjongsun/add/agent-run-costagent-run-costjongsun/ci/ship-hooks-and-trigger-checkhooks/, adds the installed-command checkOne conflict along the way, in
.github/CODEOWNERS, resolved by keeping both the/domains/security/and/domains/stability/lines.Deliberately not merged: #79 (
mobile-visual-testing), which is someone else's open PR, andthe CI/infra PRs #46, #87, #93, #94, #95, #103, which change tooling rather than add skills. Say
so if you want any of them in.
Install
Add
--dry-runfirst to see the file list without writing. That installs 65 skills formetamask-extension(70 formetamask-mobile, 43 forcore) into.claude/skills/mms-*/,.cursor/rules/and.agents/skills/.--savewrites the selection to.skills.localso laterruns repeat it.
Four things that are easy to get wrong:
--domain.--domain pr-workflowlooks correct and silently dropsred-on-base,memory-leak,react-render-deltaand the rest, whilemms-evidencestillnames them. The skills span fifteen domains:
pr-workflow(12),performance(9),testing(8),
analytics(6),typescript(5),coding(5),web3-tools(4),perps(4),security(3),
stability(2),platform(2),general(2),ui(1),swaps(1),agentic(1).--maturity experimentalis required. Everything exceptlane-graphsisstable, and thedefault filter is
stable— so a default run installs 64 of 65 and does not report which one itskipped.
package.json;override with
--repoif there isn't one.scope: project. They install into<target>/.claude/skills/, not$HOME.--targetdefaults to the current directory, so run it from — or point it at — the repo youwant them in.
To remove them:
--prune-staleon a later sync, or delete themms-*directories.Usage
Two orchestrators, and the rest are engines and lenses they route to. Everything also stands
alone, triggered by name or by description match — you do not have to route by hand.
mms-evidencetakes a claim about a PR and produces something a reader can check withouttrusting the author:
It runs three ways: on a PR whose claim someone else made, in the inner loop against uncommitted
changes before a reviewer sees them, and on a symptom with no claim yet, where the hypothesis to
kill is your own.
mms-debugis the same shape pointed at a symptom rather than a claim.Representative engines:
/mms-red-on-basefor a regression test that must fail on the basecommit;
/mms-silent-failurefor paths that can fail without saying so;/mms-unmeasured-joinfor the inference step between two true facts that nobody instrumented;
/mms-tsc-blindspotsforwhat the type checker does not catch;
/mms-sentry-mcp-queriesand/mms-sentry-grafana-correlationfor reading production signal;/mms-supply-chain-auditand/mms-privacy-egress-diligencefor dependency and egress review.What it is for. The bar is not that a run reaches the right verdict on every concern. It is
that it surfaces concerns, falsifiers and avenues of inquiry that a human then judges. A run that
ends "inconclusive, and here is what would settle it" has done its job.
Known state
Six lint errors, all pre-existing on the source branches rather than introduced by the merge —
mainis clean under the same linter. Five are the same class, from the check #99 adds: adescriptionadvertises/namewhere the installer emitsmms-name. Those branches predate therule and only fail once combined with it.
performance/react-render-delta/react-render-delta, installed asmms-react-render-deltastability/memory-leakstability/race-condition-reprotypescript/tsc-blindspotspr-workflow/debugpr-workflow/evidenceprobes/is not an allowed directory besideskill.mdThey are fixable only on the PRs that own them. Fixing them here would put the fix on a branch
that is never merged and leave the real one broken — which is the failure mode #84 was already
caught by once, when four runner fixes landed on the copy CI executed and never reached the copy
under review.
Also carried here:
domains/pr-workflow/skills/evidence/scripts/duplicatesa standalone runners repository (
v0.1.0). Thatrepository is the source; the copies are pending removal and exist so the branch can run
standalone while it is reviewed.
evidence-runners/tools/check-duplicates.shcompares them.