Skip to content

feat: add the metamask-extension base skill set - #136

Open
andrepimenta wants to merge 1 commit into
feat/base-skills-mobilefrom
feat/base-skills-extension
Open

feat: add the metamask-extension base skill set#136
andrepimenta wants to merge 1 commit into
feat/base-skills-mobilefrom
feat/base-skills-extension

Conversation

@andrepimenta

Copy link
Copy Markdown
Member

⚠️ Merge after #135

Stacked on feat/base-skills-mobile. The base flag this PR sets is introduced there — merge #135 first, then this retargets to main automatically.

What

Marks the 10 skills every metamask-extension engineer needs as base, so a fresh clone lands exactly those.

Four are already shared with Mobile and land in #135coding-guidelines, ui-development, controller-integration, pr-guidelines. This PR adds the six Extension-only ones:

Skill Why base
extension-testing Testing entrypoint — unit, integration (stub), Selenium E2E
controller-guidelines BaseController patterns; complements controller-integration
perf-rendering Performance is cross-cutting — same basis as Mobile's performance
perf-hooks-effects ditto
perf-react-compiler ditto
perf-state-management ditto

Always-on cost: ~670 tokens per session.

On the four perf-* skills

They cover the same cross-cutting concern that makes Mobile's performance base; Extension's guidance is simply split across four skills rather than consolidated into one.

Two known caveats, included deliberately rather than treated as reasons for inclusion:

  1. Descriptions are 34–39 characters and will not self-trigger. "Rendering performance optimization" doesn't match how anyone phrases the problem — people say "this screen is janky when I scroll". CI warns on each until rewritten, so the TODO cannot rot. The content behind them (17–25KB each) stays reachable by name in the meantime (/mms-perf-rendering).
  2. Four listing slots for one concern, and ~20KB loaded per invocation. Mobile's pattern — one entrypoint plus on-demand reference files — is cheaper and less ambiguous. Worth considering after the descriptions land.

Scope

Proposed from the catalogue rather than from working in metamask-extension, so it needs owner review. Two things I could not judge from outside the repo:

  • whether extension-testing is genuinely the right single testing entrypoint
  • whether controller-guidelines and controller-integration overlap enough that only one should be base

Known Extension gaps (not addressed here)

Gap Notes
Almost no PR automation 9 of 10 pr-workflow skills are Mobile-only, including create-pr. Extension gets PR standards but no PR automation. Overlay work on existing skills — likely the cheapest high-value win.
No component-scaffold equivalent Design-system-first intent has no enforcement at generation time
No component-view test layer The "steer agents toward component view tests" goal is Mobile-only today
Platform APIs 1 of ~11 areas covered
3 deprecated testing skills e2e-testing, unit-testing, e2e-flakiness-patterns still install and compete with extension-testing
6 non-base skills with thin descriptions incl. ab-testing (26 chars), test-i18n-usage (33)

Verified

Extension default path (--repo metamask-extension --domain none) installs exactly 10 skills. 65/65 tests, 0 lint errors.

🤖 Generated with Claude Code

Mark the ten skills every metamask-extension engineer needs as `base`, so a
fresh clone lands exactly those. Four are already shared with Mobile
(coding-guidelines, ui-development, controller-integration, pr-guidelines);
this adds the six Extension-only ones.

Depends on the `base` flag introduced for Mobile — merge that first.

Extension base set: coding-guidelines, ui-development,
controller-integration, pr-guidelines, extension-testing,
controller-guidelines, perf-rendering, perf-hooks-effects,
perf-react-compiler, perf-state-management.

The four `perf-*` skills cover the same cross-cutting concern that makes
Mobile's `performance` base; Extension's guidance is split across four
skills rather than consolidated into one. Their descriptions are 34-39
characters and will not self-trigger yet — CI warns on each until they are
rewritten. The content behind them (17-25KB each) is reachable by name in
the meantime.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@NicolasMassart NicolasMassart left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One concern before treating the proposed Extension set as the default base.

---
name: perf-rendering
description: Rendering performance optimization
base: true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suggestion: fix the perf trigger descriptions before making these base

The PR already calls out that the four perf-* descriptions are too thin to self-trigger.

That seems at odds with #135's rationale for the base-description guard: a base skill permanently consumes listing context, so installing one that rarely triggers gives us the cost without much benefit.

Could we either improve these descriptions in this PR before marking them base, or leave the perf skills opt-in until that cleanup is done?

@Qbandev

Qbandev commented Aug 28, 2026

Copy link
Copy Markdown

Verified against a clone of feat/base-skills-extension: repo scoping is airtight (simulating the repos/${REPO}.md gate gives extension 10, mobile 10, core 0 — the body's claim is exact), and 65/65 tests / 0 lint errors reproduce. Three things before marking these base:

The thin-description problem covers 5 of the 6 skills, not 4.
@NicolasMassart's comment targets the four perf-* skills, but controller-guidelines has the identical defect — description is "BaseController development patterns", 35 chars — while being presented in the inclusion table without a caveat. Only extension-testing (389 chars) clears the 120-char BASE_DESCRIPTION_MIN floor that #135 introduces. Full audit on this branch: perf-rendering 34, controller-guidelines 35, perf-hooks-effects 36, perf-react-compiler 36, perf-state-management 39.

"CI warns on each until rewritten, so the TODO cannot rot" doesn't hold.
lint-skill-entry.mjs sets process.exitCode = errorCount > 0 ? 1 : 0 — warnings never fail the job. The audit on this branch reports 0 errors and 95 warnings; the five base-description warnings would sit invisibly inside a green check. If the intent is a real ratchet, the base-description rule needs to be an error for base: true skills.

The "~670 tokens/session" figure appears to be characters.
Summing name + description across the six skills gives 679 characters ≈ 170 tokens. #135's "~1,130 tokens" is a genuine token estimate (4,221 chars ÷ ~4), so the two headline numbers are on different scales.

Suggestion: mark extension-testing base now; hold the other five until the descriptions are rewritten to 120+ chars (which also resolves whether four separate perf slots vs. Mobile's single-entrypoint shape is the right structure). Separately: the five stubs omit maturity: entirely, so #135's new base + experimental guard never evaluates them — adding explicit maturity: stable makes the stability claim deliberate.

@MajorLift

MajorLift commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

This list overlaps almost entirely with the list of skill pointers we have in extension's AGENTS.md file:

### Coding Guidelines

- **Controller Patterns:** [.cursor/rules/mms-controller-guidelines/RULE.md](./.cursor/rules/mms-controller-guidelines/RULE.md)
- **Unit Testing:** [.cursor/rules/mms-extension-testing/RULE.md](./.cursor/rules/mms-extension-testing/RULE.md)`references/unit.md`
- **E2E Testing:** [./test/e2e/AGENTS.md](./test/e2e/AGENTS.md) and [.cursor/rules/mms-extension-testing/RULE.md](./.cursor/rules/mms-extension-testing/RULE.md)`references/e2e.md`
- **E2E CI Decision Tree:** [.github/guidelines/E2E_DECISION_TREE.md](./.github/guidelines/E2E_DECISION_TREE.md)
- **E2E Deprecated Patterns:** [./test/e2e/AGENTS.md](./test/e2e/AGENTS.md)
- **CI Workflows:** [.github/AGENTS.md](./.github/AGENTS.md)
- **Front-End Performance:**
  - [Rendering Performance](.cursor/rules/mms-perf-rendering/RULE.md) - Start here (keys, memoization, virtualization)
  - [Hooks & Effects](.cursor/rules/mms-perf-hooks-effects/RULE.md) - useEffect best practices
  - [React Compiler & Anti-Patterns](.cursor/rules/mms-perf-react-compiler/RULE.md) - React Compiler considerations
  - [State Management](.cursor/rules/mms-perf-state-management/RULE.md) - Redux optimization
- **Pull Requests:** [.cursor/rules/mms-pr-guidelines/RULE.md](./.cursor/rules/mms-pr-guidelines/RULE.md)
- **General Coding:** [.cursor/rules/mms-coding-guidelines/RULE.md](./.cursor/rules/mms-coding-guidelines/RULE.md)
- **Official Guidelines:** [.github/guidelines/CODING_GUIDELINES.md](./.github/guidelines/CODING_GUIDELINES.md)

-- https://github.com/MetaMask/metamask-extension/blob/main/AGENTS.md#coding-guidelines.

Setting aside the fact that the file badly needs to be cleaned up, I think it's a good indication that this PR covers the category of skills that need to be "accessible by default."

However, there's another category of critical skills that need to be "discoverable by default", a) because they otherwise would not be human-invoked or agent-discovered due to lack of awareness, or b) because they actively correct agent (or human) predispositions that are erroneous. From experience reviewing extension PRs, these skills have contributed useful or even mission-critical interventions, and would be valuable to include as base skills:

Skill Why it needs to be discoverable
pr-workflow/silent-failure
swallowed errors, fallbacks that substitute a default, caches that fail open
not invoked or discovered — The failure emits nothing, so there is no observation to start from, and engineers are unaware that there's even a problem that a skill should target.
analytics/sentry-quota
span instrumentation whose fan-out is quota-risky
not invoked or discovered — The only feedback is the bill, after release. Engineers are not aware this needs to be checked (sometimes they're even unaware they're activating new instrumentation) so the skill is not invoked when it's most needed -- discovery is the only option. This awareness gap has resulted in multiple incidents over the past couple months.
security/privacy-egress-diligence
changes to what user data leaves the device — Sentry state masks, new MetaMetrics/Segment properties
not invoked or discovered — Data is sent irrevocably, but no test fails, no user reports it, and the mask list is not something a reviewer thinks to check, so engineers are not aware there's a problem. Intervention via skill discovery at authoring time is the critical intervention point.
platform/extension-lifecycle-decoupling
lifecycle events assumed to cause application-level side effects
corrects a predisposition — Preempts confidently wrong MV3 claims that have been prevalent enough to be disruptive to platform team, e.g. frequent reports that all manner of bugs are attributable to service worker frequently dying mid-session (it doesn't die due to a keepalive).
typescript/tsc-blindspots
type defects tsc is structurally unable to report
both — Engineers are not aware that there's a problem as long as tsc returns green status, even though tsc can ratify incorrect and unsafe types because it only checks branch-internal consistency, not correspondence to authoritative type sources.
typescript/derive-types
hand-written ad-hoc types that restate an authoritative source
corrects a predisposition — writing ad-hoc inline types even when they can and should be derived from authoritative sources is a strong agent predisposition that needs to be preempted
performance/effect-antipattern-scan
effects synchronising state derivable during render, dependency arrays that lie
both — Engineers writing defective code are not aware there's a problem, and therefore cannot invoke this skill. Defects have no symptom at authoring time, and fail silently at runtime, only being detectable through active profiling. Only static code analysis via gate or skill discovery preempts the error and closes the gap.
performance/selector-antipattern-scan
selectors returning a fresh identity per call, cascading renders
both — Same as above.

Together they add 6,964 characters of always-loaded description, against 1,528 for the eleven AGENTS.md already points at, but I think the two are worth separate budgets. It's more efficient and reliable to specify the "needs to be accessible" category discovery conditions in AGENTS.md, while the "needs to be discoverable" category MUST have its descriptions always-loaded in context.

The additional skills can be added in a follow-up once they've been merged, but I think they're critical to include.

Comment on lines 2 to +4
name: controller-guidelines
description: BaseController development patterns
base: true

@MajorLift MajorLift Aug 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we're only choosing one controller-related skill for extension it should probably be controller-integration(https://github.com/MetaMask/skills/blob/main/domains/coding/skills/controller-integration/repos/metamask-extension.md), since writing controllers is not a client-side concern, while integrating them comes up very frequently.

We can probably fit in both if wider understanding of controller internals seems valuable to have by default (or better yet have controller-integration reference sections of controller-guidelines so relevant information can be pulled by the agent as needed), but either way controller-integration would need to be included.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants