[DO-NOT-MERGE] trial branch: all skill PRs merged, for install and testing - #108
Draft
MajorLift wants to merge 173 commits into
Draft
[DO-NOT-MERGE] trial branch: all skill PRs merged, for install and testing#108MajorLift wants to merge 173 commits into
MajorLift wants to merge 173 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.
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 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.
`benchmark-design` and `browser-extension-profiling` both cover E2E benchmarking with statistical rigor, and they sat in different domains here while the rest of the measurement work — `data-analysis`, `react-render-proof` — lives in the performance PR. Splitting one subject across two PRs made both harder to review. Moves `benchmark-design`, its `benchmark-statistical-hygiene` knowledge, and `browser-extension-profiling`. Neither skill referenced the other by path, so nothing needed rewriting. What remains is the extension-runtime work this PR is named for.
Generic scripting guidance — paginate an API, handle rate limits, retry transient errors — with no MetaMask specificity and no relationship to this PR's extension-runtime subject. `coding/` otherwise holds MetaMask internals. Nothing referenced it and it referenced nothing, so removal is self-contained. The content stays in this branch's history if it is wanted later.
The keepalive row was the one entry here resting on a live implementation detail rather than on an absent handler or persistent storage, and it was cited as `background.js:750-758` — a line range that drifts. If the interval grows past the idle timeout or the keepalive is removed, the conclusion inverts from "eviction is prevented" to "eviction happens routinely", and a skill still asserting the first is worse than no skill. Replaces the line range with a symbol grep (`saveTimestamp`, `SAVE_TIMESTAMP_INTERVAL_MS`), names the two conditions the conclusion depends on — sub-idle-timeout interval, and an extension API call rather than a bare timer — and records what was verified, against which commit.
`app/scripts/constants/sentry-state.ts` decides what user data leaves the machine. It carries 116 fields currently set to `true` — meaning the real value is copied and sent — and it is edited inside ordinary feature PRs (onboarding, swaps, rewards, the analytics controller) with no CODEOWNERS entry, so no privacy reviewer is automatically tagged. Same shape as `lavamoat-policy-diligence`: the diff is mechanical, the judgement is what each grant means. `git diff` finds every newly-`true` field exactly, so the deliverable is not "is it listed" but what the field holds at runtime — a mask path cannot distinguish `selectedTab` from `selectedAddress`. Sorts findings into safe / needs-narrowing / must-not-egress with the evidence and a proposed mask for each, and renders no accept verdict: that call belongs to privacy and legal, and a confident reviewer "this is fine" is precisely what lets an unreviewed field through. Also covers the sibling pipes a PR widens at the same time — new MetaMetrics or Segment properties, and error strings that interpolate runtime values, both of which egress regardless of the mask.
Scripted automation announces its cost in wall-clock time; agentic automation does not. A fan-out of forty subagents and a single call are the same shape in a diff, and the difference surfaces later on a bill attributed to nothing in particular. The token-spend counterpart to `sentry-quota`, and deliberately the same posture: operate on code and PRs before the spend exists, and produce figures rather than adjectives. Names the amplifier triad — fan-out × trigger frequency × no-kill-switch — where one alone is usually fine and all three together is the shape that produces a surprise. Requires the arithmetic be shown, and the worst case stated separately from the expected case, since the budget conversation is about the PR that touches 400 files rather than the normal one. Renders no ship verdict: whether a cost is worth paying belongs to whoever owns the budget. Raised as an open question during ADR-0058 review (MetaMask/decisions#173), where agent token consumption had no estimate.
…rency The falsifier for a concurrency claim is a test that never interleaved: a sequential run exercises no race and produces a green indistinguishable from a real pass. The skill therefore treats showing the interleaving occurred as the proof obligation, not the assertion passing. Lands in `stability/` beside `memory-leak-hunt`, the other defect-class engine `pr-validate` delegates to.
`pr-validate` is handed a claim and looks for the observation that would falsify it. Debugging starts from a symptom and has to generate the hypothesis first, which is where the expensive failure lives: the theory is yours, nobody else is positioned to challenge it, and confirmation is cheap. Routes the symptom to the engine that owns its defect class rather than reimplementing any investigation, so both orchestrators share one set of engines. Carries `pr-validate`'s trust gates, which bind harder here — a weak instrument in review yields a claim someone challenges, in debugging it yields a theory nobody checks.
The telemetry analogue of MUTABLE_REF. statsPeriod=7d / period=24h / from=now- resolve against the reader's clock, so a link published beside typed counts stops producing them without anything having been edited — auditable-looking and silently wrong, which is worse than no link. Sits beside mutable-ref in _scan, with a NEEDS entry naming the fix (absolute start/end, plus a link whose view performs any cited grouping). Controls: fires on statsPeriod=7d and Grafana from=now-, silent on start=/end= and on prose mentioning "a 7d period".
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.
`benchmark-design` and `browser-extension-profiling` are the capture half of the measurement work already here: `data-analysis` turns raw numbers into a defensible before/after, and `react-render-proof` proves a specific change moved work. Both arrived from the platform PR, which shipped them alongside unrelated extension-runtime skills. `benchmark-design` stays in `testing` — that domain already owns benchmark methodology (`performance-testing`) — and brings its `benchmark-statistical-hygiene` knowledge with it. The PR spans two domains because the subject does, not because it is a grab bag.
`metamask-extension` moved its default branch to `main`; `develop` still exists but its last commit is 2026-01-15, so six links in the extension overlays resolved to code roughly six months stale. They loaded, which is why nothing caught it — a frozen branch is worse than a dead one here, since the reader gets plausible but outdated source. All five cited paths verified present on `main` (HTTP 200): `ui/`, `ui/hooks/`, `ui/selectors/`, `shared/lib/selectors/selector-creators.ts`, and `app/scripts/metamask-controller.js`.
`browser-extension-profiling` drops `browser-`, which distinguishes nothing: an extension is a browser extension, and the `extension-` half is what separates it from the mobile work this domain also covers. `anti-pattern` loses its hyphen in identifiers, matching what `main` already ships in `review-antipatterns.md` and `mm-redux-antipatterns.md`. Both skills and both knowledge files move together, since a skill and its knowledge sharing a stem is what makes the by-name citation convention resolvable. Prose inside the two renamed skills is normalised with them so each file agrees with its own name; hyphenated prose elsewhere is left alone as pre-existing and outside this change.
`scan` says what they do. Both walk a diff looking for a known set of shapes and report what they find; `review` implied a judgement they do not make and overlapped with the correctness review these deliberately are not. The suffix still carries its original job of keeping each skill distinct from the knowledge file it cites — `selector-antipatterns.md` and `effect-antipatterns.md` — which the by-name citation resolver needs, since it matches on filename. Installed as `mms-selector-antipattern-scan` and `mms-effect-antipattern-scan`.
…g it `C4` is an address into `evidence-catalog.md`. A reader who has not opened the catalog cannot resolve it, and the frontmatter `description` cannot link out to one. Both sites now name the category and link the catalog by URL — a relative path would not survive installation, which flattens skills to `mms-<name>/`. Also updates two sibling names that no longer resolve: `pr-validate` is now `evidence`, and `memory-leak-hunt` is now `memory-leak`.
`-proof` as a noun suffix reads as "immune to", so the old name parsed as "immune to React renders". `-delta` names what the skill actually produces, and matches the skill's own insistence that its output is a measured quantity rather than a boolean.
Found by running the skill against real merged PRs: the §3 detection matched
only named collection constructors, so a result function returning an object
literal directly went undetected.
`(metamask) => ({ userRegion: ..., ... })` builds a new object on every
recompute and matches none of `new Set`, `new Map`, `Object.values`, `?? {}`,
or `?? []`. Adds `=> ({` and `=> [` as alternates, with the reason recorded
beside the table so the next person does not narrow it again.
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.
Ports the mentions check, the reply-receipt gate and the two newest
rule classes (ad-hoc artifact host, context leak) into the packaged
hook, so the repo copy is a superset of the installed one rather than
a fork of it. The two had drifted in both directions: the installed
copy carried these checks, this one carried the repo-relative path
resolution and the code-span exemptions. Neither was replaceable by
the other, which is the state where a fix lands in one copy and the
other keeps shipping without it.
`_issue_is_foreign` is ported with its return-code check added rather
than as-written. `gh api` writes its error envelope to stdout, so
reading stdout alone turned every failed lookup into `{"message":"Not
Found"}` — truthy, never equal to the caller login, and therefore
"somebody else authored this", which waves the enrichment rule
through. The docstring already promised the opposite. The existing
enrichment arm of gate-controls.sh caught it on the first run, because
its fixture repo does not resolve.
The mentions check gets both control arms it was missing: a body
naming a handle must block, and an @v6 action pin must pass. The
negative arm is what separates this rule from one that rejects every
@ in a body — defeat the action-pin exemption and it goes red.
…ng-test' into HEAD
…r-check' into HEAD
MajorLift
force-pushed
the
jongsun/trial/combined
branch
from
August 13, 2026 00:00
4cc9aa5 to
3845664
Compare
MajorLift
added a commit
that referenced
this pull request
Sep 1, 2026
The symptom table and the description both named `react-render-proof`. No skill of that name exists on `main` or in any open pull request; the render engine is `react-render-delta`, added by #43 and carried by #84 and #108, and installed locally as `mms-react-render-delta`. The other six engines this skill routes to all resolve to skills in open pull requests, so this was the only wrong name rather than one of three. Checked against the repository rather than against an install: none of the seven is on `main` yet, which is a merge-ordering fact and not a defect here. Nothing validates this today. #103, which would have checked cross-skill references, was closed as superseded by #87, and #87 is not merged — so this name would have shipped unflagged.
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. A merge of fourteen open PRs, so the set installs and runs as one thing. No content of its own — every line belongs to a PR below and should be reviewed there. Deleted once they land.
Overview
39 skills across 13 PRs plus one CI change,
--no-ffontomain. Four sit on a fork. No single PR installs into a working state anyway:mms-evidencealone routes to engines in eight domains.evidencefalsifiers-first,unmeasured-join,instrument-check,scope-of-search,coverage-partition,selection-audit,silent-failure,unintended-breakage,red-on-base,distinguishing-observation,observability-gaplane-graphsdebugmemory-leakrace-condition-reprosupply-chain-audit,lavamoat-policyprivacy-egress-diligencereact-render-delta,selector-antipattern-scan,effect-antipattern-scan,extension-profiling,data-analysis,benchmark-designsentry-quota,sentry-mcp-queries,sentry-grafana-correlation,grafana-tempo-queries,performance-attribution,instrumentationtsc-blindspots,avoid-any,derive-types,decompose-large-files,migration-context-costextension-errors-debugging,extension-lifecycle-decouplingagent-run-costhooks/, adds the command checkOne
.github/CODEOWNERSconflict, resolved by keeping both lines. Deliberately not merged: the CI/infra PRs #46, #87, #93 (pins installed content to a release tag), #94, #95 and #103, and #79 (mobile-visual-testing), which is someone else's.Showcase
Installs 68 skills for
metamask-extensioninto<target>/.claude/skills/mms-*/.--dry-runfirst;--prune-staleremoves them.--maturity experimentalis required: the defaultstableinstalls 36 of 68, naming none of the 32 it drops. Do not pass--domain: it silently drops enginesmms-evidencestill names.mms-evidenceproduces something a reader can check without trusting the author:Known state — six lint errors, all pre-existing on the source branches;
mainis clean under the same linter. Five are the check #99 adds; the sixth isevidence'sprobes/. Fixable only on their own PRs.