Repository navigation
Conversation
Bugbot already reviews every non-draft PR update. This checked-in `.cursor/BUGBOT.md` focuses those runs on skill-file security: hidden/overriding instructions (prompt injection), credential access & data exfiltration, unsafe execution & safety-control bypass, and remote instructions fetched at runtime. Scope is changed lines only, with advisory comments that escalate to the full security review when needed. This is the same mechanism already used by metamask-mobile and metamask-extension. A marked merge point in the file takes the security team's tailored OWASP LLM Top 10 prompt when it is delivered. --------- Co-authored-by: behroozreview <behrooz.aghakhanian@gmail.com>
## Summary Acceptance criteria for skill contributions, in both halves — a CI validator for the deterministic properties, and the two questions in `CONTRIBUTING.md` that no check can answer. From the [ADR 0057 review](MetaMask/decisions#162), where acceptance criteria were raised as an open item. ## The validator `yarn lint:skills` checks directory layout, `skill.md` path and name pattern, frontmatter keys and maturity values, `mms-` prefix misuse, repo-overlay names, and description length. `tools/skill-schema.mjs` is the single source of truth, imported by the linter so the documented and enforced schemas cannot drift. **Description budget is 1,536** — a repo budget, not an operator limit. An earlier draft used 1,024, justified as the per-operator minimum citing OpenCode. That does not hold up: `tools/install` emits the description verbatim with no truncation anywhere in the tooling, and six skills over 1,024 — up to 1,344 — install and load in Claude Code today. It also never bound anything, since the 46 skills on `main` have a median description of 45 characters and a maximum of 928. Enforcing it had a real cost. A description is the discovery surface and the only part of a skill carrying its own trigger cues, so trimming one to fit makes the skill less likely to be selected when it is relevant. The budget still exists — always-on context should be bounded — but at a number that reflects a deliberate choice rather than an unverified external claim, and the comment now asks for an operator and version before anyone tightens it. ## What running it against `main` turned up Three errors, two of which were defects in the repo rather than in the check: - **`workflows/` never shipped.** 14 files across two `web3-tools` skills, referenced **22 times** from their bodies, absent from the installer's bundle list — so every installed copy carried 22 dangling links and nothing reported it. Added to `copy_bundle_dirs`; all 22 now resolve, 42 files delivered. This is the third instance of one root cause, after `pr-validate`'s `hooks/` and domain `knowledge/`: a directory that exists in source and is not in the copy list. - **`metadata` was rejected** as unknown frontmatter, though README documents it as preserved through install. Now in the optional set. - **`performance` description was 1,078 chars.** Under the 1,536 budget, so it stays as authored — an earlier trim to 928 was reverted along with the ceiling that motivated it. Validator now reports **0 errors across 46 skills**. ## So the list stops being written twice Two tests read `tools/install` directly: - `BUNDLE_DIRS` must match the directories it copies. - Every key it reads via `frontmatter_value` must be declared in the schema. The comment asking a human to keep Bash and JS in sync is now checked. Knowledge files also get their own declared keys, since they take `domain` rather than the installer-behaviour keys skills use. ## And the half a linter cannot check `CONTRIBUTING.md` gains two questions: **Is it a skill, or does it belong in an enforcement layer?** Skills shape generation; linters, hooks, and fitness functions enforce. A skill that *substitutes* for enforcement is unsafe — an agent can ignore any context it is given. One that *restates* a deterministic check is wasteful. One that *teaches the upstream pattern* so enforcement rarely fires is the right shape, and should name the layer it pairs with. **Does it earn its context budget?** Frontmatter for every installed skill loads at agent startup, so a skill that is never selected still costs its `description` on every run. ## Keeping the number honest Two tests, because the failure mode here was documentation drifting from enforcement in both directions: - The boundary case derives its length from `DESCRIPTION_MAX` rather than hardcoding one, so it keeps testing the boundary when the budget moves. The previous hardcoded 1,100 silently stopped exercising anything the moment the ceiling rose. - A new check fails if `README.md`, `CONTRIBUTING.md`, or `.github/SKILL_TEMPLATE.md` state a figure the schema does not enforce. All three had said 1,536 while the schema said 1,024; reconciling them by editing the docs down was the wrong direction, and this makes that disagreement fail rather than get silently resolved. ## Test plan - [x] `node .github/scripts/lint-skill-entry.mjs` — 0 errors, 46 skills - [x] `test/lint-skill-entry.test.mjs` — 10 pass, including both drift checks - [x] `test/cli.test.mjs` 11 pass, `test/unit.test.mjs` 30 pass - [x] Real install of `web3-tools`: 22/22 `workflows/` references resolve, 42 files delivered - [x] 84 remaining warnings are all missing recommended sections — advisory, non-blocking ## Notes - Supersedes #92, folded here: the check handles what it can, the prose says where to spend review attention instead. - 84 warnings suggest either the recommended sections are wrong for this corpus or most skills predate them. Worth a follow-up decision; not blocking, since warnings do not fail CI.
## Summary - Add `testing/mobile-testing` as the single MetaMask Mobile functional-testing skill: short router + progressive-disclosure references for unit, component-view, integration, Appium E2E, Detox→Appium migration, and test-layer placement. - Deprecate superseded Mobile peer skills (`unit-testing`, `component-view-test`, `integration-test`, `e2e-test`, `test-layer-placement`) as thin redirects; remove the Mobile `e2e-testing` overlay while keeping Extension E2E/unit overlays. - Default new device journeys to Appium; keep Detox docs only as nearly-deprecated migration / remaining-suite support. Update `testing-layers.md`, README, and Mobile coding/PR pointers. Closes / tracks: [MMQA-2142](https://consensyssoftware.atlassian.net/browse/MMQA-2142) ## Test plan - [ ] `metamask-skills describe testing/mobile-testing` - [ ] `metamask-skills sync --repo metamask-mobile --include testing/mobile-testing` installs router + all references (unit/CV/integration/appium/detox/placement) - [ ] Mobile install skips `e2e-testing` (no Mobile overlay) and still installs deprecated stubs that redirect to `mobile-testing` - [ ] Extension install still gets `e2e-testing` + `unit-testing` Extension overlays; `mobile-testing` is skipped (Mobile-only) - [ ] Router/layers policy mark Appium as default E2E and Detox as nearly deprecated - [ ] Spot-check relative links from `skill.md` → layer refs → nested refs Made with [Cursor](https://cursor.com) Co-authored-by: Cursor <cursoragent@cursor.com>
…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.
MajorLift
changed the title
race-condition-proof skill
…xing it The description read "the engine behind evidence's B7 deterministic-interleaving evidence category". "B7" is an address into evidence-catalog.md, not a name: it carries no meaning to a reader who has not opened the catalog, and a frontmatter description cannot link out to one. - Drop the lane id from the description; name the role instead. - Replace the trailing Notes reference with a Related section that links the catalog by URL. A relative path would not survive installation, which flattens skills to mms-<name>/. - Drop the exogram-daemon path, which is a private repo the reader cannot open. - Update the stale pr-validate name to evidence.
As a noun suffix, `-proof` is privative in English — waterproof, bulletproof, tamper-proof all mean "immune to". So `race-condition-proof` parses as "immune to race conditions" rather than "produces a proof about ordering", and that misreading is plausible enough not to self-correct. `-repro` names what the harness produces and carries no such inversion.
MajorLift
changed the title
race-condition-proof skillrace-condition-repro skill
Contributor
Author
Context budgetWhat this PR costs an agent, measured from an install rather than read from the diff. Three tiers, and only the first is unavoidable.
Frontmatter is the only tier paid unconditionally — every agent loads it on every run once the skill is installed, used or not, because it is what the agent reads to decide relevance. The 28 skills across the eleven open skill PRs sit at a median of ~1,716 tokens selected and ~1,860 with references followed. All are within the 1,536-character description budget. Selected is paid only when the agent picks the skill. + refs & knowledge is the ceiling if every bundled reference is then read; it is a worst case, not an expectation. Method
These figures are pinned to the commit above and drift on every push; #96 tracks automating them. |
This was referenced Jul 31, 2026
The installer emits `mms-race-condition-repro`; the description advertised `/race-condition-repro`.
7 tasks done
MajorLift
changed the title
race-condition-repro skillstability domain - race condition repro skill
MajorLift
changed the title
stability domain - race condition repro skillstability domain - /race-condition-repro skill
MajorLift
changed the base branch from
main
to
jongsun/add/memory-leak-hunt-skill-v2
August 31, 2026 14:47
MajorLift
merged commit Aug 31, 2026
cb9777b
into
jongsun/add/memory-leak-hunt-skill-v2
28 checks passed
MajorLift
added a commit
that referenced
this pull request
Aug 31, 2026
The 25 conflicts were all in files this branch picked up when #97 merged in, never in its own `domains/stability/` content. `main` has since carried five more commits over those same files — #109, #115, #54, #120, #132 — against this branch's single snapshot of them, so `main` is authoritative for every one and its version is taken throughout. That includes six deletions: #120 dropped Detox E2E, and the four `references/detox/` files plus `detox-to-appium.md` and `detox/legacy-playbook.md` go with it. Nothing references them. The merged tree now differs from `main` only in the `CODEOWNERS` line this PR adds and its five `domains/stability/` files.
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
3 participants
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.
Summary
race-condition-repro, the engine behindevidence's B7 deterministic-interleaving evidence category and the siblingred-on-basenames for ordering bugs.domains/stability/besidememory-leak, the other defect-class engineevidencedelegates to.Test plan
node .github/scripts/lint-skill-entry.mjs domains/stability/skills/race-condition-repro/skill.md— 0 errorsnamematches directory,maturity: experimental, description within the 1536-char budgetdomains/stability/is also created by feat: Addsstabilitydomain -/memory-leak,/race-condition-reproskills #81; domains are discovered viareaddirSync, not a registry, so both can add it independentlyValidation runs
Trial runs of this PR's skills against merged
metamask-extensionPRs nobody flagged. Every claim was re-verified against the real diff before posting. Clean results are included on purpose — a skill that only ever reports problems cannot be calibrated.race-condition-reprorace-condition-reproEach comment carries a trial-run disclaimer and links back here for feedback.