diff --git a/CHANGELOG.md b/CHANGELOG.md index 4038b401..f0ea2984 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,13 +10,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Add `platform/notifications-category-sync`: a repo-agnostic workflow for syncing a client with the backend notifications category manifest (fallback snapshot, preference keys, settings rows), with a MetaMask Mobile overlay. +- Add a MetaMask Extension overlay for `feature-flags` (registry sync, E2E flag seeding) and a Core overlay for `controller-guidelines` (failure shapes, public type changes). - Add `performance/profiling-regression-proposal`: an evidence-only skill that proposes follow-up actions after MetaMask Mobile CI has already classified a Hermes CPU-profile regression. - Support explicit-only workflow skills through native invocation controls, preserve repository overlays, and prune managed retired skill names during sync. -- Distribute Perps static review as a shared execution checklist with repository overlays, source-tracked client rules and per-rule evidence outcomes, without a duplicate template catalog. +- Distribute Perps static review as a shared execution checklist with repository overlays, source-tracked client rules and per-rule evidence outcomes, without a duplicate template catalog. Reviews read consumer code from the provided reference checkouts at a cited revision and end in APPROVE or REQUEST_CHANGES; a check the reviewer could not run is listed as not verified instead of downgrading the verdict to COMMENT. - Add `navigation` skill with a repo-agnostic base and a MetaMask Mobile overlay for `Routes` and `NavigationService`. Marked `base: true` so it installs even when its domain is filtered out. - Add `feature-flags` skill with a repo-agnostic base and a MetaMask Mobile overlay for version-gated remote flags. Marked `base: true` so it installs even when its domain is filtered out. ([#147](https://github.com/MetaMask/skills/pull/147)) - Add `analytics` skill (`platform/analytics`, moved from `coding`) with a repo-agnostic base and a MetaMask Mobile overlay for the canonical tracking API. Marked `base: true` so it installs even when its domain is filtered out. ([#140](https://github.com/MetaMask/skills/pull/140)) +### Fixed + +- Keep a source's skill when a later source has the same directory without `skill.md`, and remove unprefixed aliases of managed skills during `--prune-stale`. + ## [0.3.1] ### Fixed diff --git a/domains/coding/skills/controller-guidelines/repos/core.md b/domains/coding/skills/controller-guidelines/repos/core.md new file mode 100644 index 00000000..3ee4b837 --- /dev/null +++ b/domains/coding/skills/controller-guidelines/repos/core.md @@ -0,0 +1,9 @@ +--- +repo: core +parent: controller-guidelines +--- + +# Controller guidelines — core + +- API response handling: test both failure shapes, a thrown error and a non-`ok` result the caller wraps. +- Classify a public type change by `AGENTS.md` ("changes the signature of any public export"), not by neighbouring code. Widening an exported union breaks exhaustive `Record` consumers. diff --git a/domains/perps/skills/perps-review-pr/references/mobile.md b/domains/perps/skills/perps-review-pr/references/mobile.md index 4a91577e..c2292d46 100644 --- a/domains/perps/skills/perps-review-pr/references/mobile.md +++ b/domains/perps/skills/perps-review-pr/references/mobile.md @@ -49,6 +49,7 @@ Use `docs/perps/perps-sentry-reference.md` for Mobile trace names and lifecycle. Mobile UI constants live in `app/components/UI/Perps/constants/perpsConfig.ts`. Use `docs/perps/perps-metametrics-reference.md` for Mobile event definitions; controller constants remain the shared contract. +A change to a pattern documented in `docs/perps/` updates that document in the same PR. diff --git a/domains/perps/skills/perps-review-pr/references/parity.md b/domains/perps/skills/perps-review-pr/references/parity.md index 652cc0d5..440d56b9 100644 --- a/domains/perps/skills/perps-review-pr/references/parity.md +++ b/domains/perps/skills/perps-review-pr/references/parity.md @@ -50,22 +50,7 @@ Extension adds: `usePerpsLiveMarketData`, `usePerpsStreamManager`, `usePerpsView ## Formatting Divergence -See `formatting-rules` knowledge file for full rules. - -| Platform | Formatter | Behavior | -|---|---|---| -| Mobile | `formatPerpsFiat` | Adaptive sig-dig by price range | -| Extension | `formatCurrencyWithMinThreshold` | Generic, no sig-dig | -| Extension | `formatNumber({min:2,max:2})` | Always 2 decimals | -| Extension | `.toFixed(2)` | Hardcoded 2 decimals | - -**Files with hardcoded formatting (extension):** -- `ui/components/app/perps/utils/transactionTransforms.ts` -- `.toFixed(2)` x7 -- `ui/components/app/perps/order-entry/components/auto-close-section/` -- `{min:2, max:2}` -- `ui/components/app/perps/order-entry/components/limit-price-input/` -- `{min:2, max:2}` -- `ui/components/app/perps/edit-margin/edit-margin-modal-content.tsx` -- `.toFixed(2)` -- `ui/components/app/perps/reverse-position/reverse-position-modal.tsx` -- `.toFixed(2)` -- `ui/hooks/perps/usePerpsOrderForm.ts` -- `formatCurrencyWithMinThreshold` x6 +Use `formatPerpsFiat` on both platforms (Extension: `shared/lib/perps-formatters.ts`). Do not add `.toFixed(2)`, `formatNumber({min:2,max:2})` or `formatCurrencyWithMinThreshold` for displayed perps fiat values on Extension; migrate a legacy call site the PR touches. ## TestID Mapping diff --git a/domains/perps/skills/perps-review-pr/references/review-sources.json b/domains/perps/skills/perps-review-pr/references/review-sources.json index 1522849c..cf44cdae 100644 --- a/domains/perps/skills/perps-review-pr/references/review-sources.json +++ b/domains/perps/skills/perps-review-pr/references/review-sources.json @@ -1,13 +1,13 @@ { "repository": "MetaMask/experimental-metamask-recipe-perps", - "revision": "e06bb8d750acbbd90ce1af62063260e85f0a0995", + "revision": "f9f8cb914c4c0fd1f7a592b9b4507b410a5d8651", "files": { - "review/antipatterns.md": "5d16b3b25757a95c39748a2fe699dbc6bf02a77e42a3bc8d8ed66b7eeed7be36", - "review/antipatterns.mobile.md": "541cedb7c79dfe01991472147d5de8b5a38dcbc1a182057e68c2f0fb470b3ab1", + "review/antipatterns.md": "552598c77f9ac2e4dbb5d9c1157c52106ad289fa22d01bf085981fef2f19c3b7", + "review/antipatterns.mobile.md": "2c6611037e8fc135b17f1f79e7b517e36cb789ffc1b433ae9ec7c01bfcf6eb17", "review/antipatterns.extension.md": "6fd041ff96e9d864fe5bc62b3d27f5bd022b221d25c159658d00bcd78501bf57", "review/antipatterns.core.md": "2c7b3990b111b50bf2a6b7a4aa3a12344dc5858ce57fa086a4da1a1798c4f392", - "review/parity.md": "8895a333d526d7819c3bbf526f09f6f216aca41ddd197581e454b0bbd4bc482c", - "review/shared-packages.md": "11268974eef5e69a3ca2c8b68202bc7789133c352855e3e758bd32069ee8255d", + "review/parity.md": "02cdefde81f1d0ea4f26da1b082873bef89e188efc9b258bb01976dced863666", + "review/shared-packages.md": "b83c7d946f85d8b72af193c206c5822cb3c8d180c61c079ae1ebfb706523817a", "owned-paths.json": "e5a3994708c61c158dd2c4adbc321269bf11435d82ff18f900d9372e831e9eb3" } } diff --git a/domains/perps/skills/perps-review-pr/references/shared-packages.md b/domains/perps/skills/perps-review-pr/references/shared-packages.md index 697956e8..809e0b7a 100644 --- a/domains/perps/skills/perps-review-pr/references/shared-packages.md +++ b/domains/perps/skills/perps-review-pr/references/shared-packages.md @@ -5,9 +5,7 @@ Companion to `parity.md`. Tracks what's shared, what should be, and what can't b ## Already Shared via @metamask/perps-controller -**Utils (20)**: `significantFigures`, `orderValidation`, `orderCalculations`, `marketDataTransform`, `sortMarkets`, `marketUtils`, `accountUtils`, `errorUtils`, `hyperLiquidAdapter`, `hyperLiquidOrderBookProcessor`, `hyperLiquidValidation`, `myxAdapter`, `standaloneInfoClient`, `stringParseUtils`, `idUtils`, `rewardsUtils`, `transferData`, `wait` - -**Services (14)**: `AccountService`, `TradingService`, `MarketDataService`, `DepositService`, `EligibilityService`, `HyperLiquidClientService`, `HyperLiquidSubscriptionService`, `HyperLiquidWalletService`, `MYXClientService`, `MYXWalletService`, `RewardsIntegrationService`, `TradingReadinessCache`, `DataLakeService`, `FeatureFlagConfigurationService` +**Utils** (`src/utils/`) and **Services** (`src/services/`) of `@metamask/perps-controller`: check them on core `main` before adding a client-side copy. ## Priority 1 -- Move to Controller (pure TS, no React deps) @@ -35,22 +33,6 @@ Once in controller, extension imports directly instead of reimplementing. | `isHip3Market`/`isCryptoMarket` | `UI/Perps/utils/` | `ui/components/app/perps/utils.ts` | YES | | `groupTransactionsByDate` | `UI/Perps/utils/transactionTransforms.ts` | `ui/components/app/perps/utils/transactionTransforms.ts` | Near-identical | -## Priority 3 -- Formatting Abstraction - -Extract pure formatting logic into controller: - -``` -Controller exports: - PRICE_RANGES_CONFIG (range thresholds + sig dig rules) - calculateDisplayDecimals(value, config) -> { decimals, sigDigs } - roundToDisplayPrecision(value, config) -> number - -Platform layer: - formatPerpsFiat(value, opts) -> calls calculateDisplayDecimals + locale formatter -``` - -Extension currently uses `.toFixed(2)` and `formatNumber({min:2, max:2})` everywhere -- both wrong for low-value and high-precision assets. - ## Can't Share (platform-bound) | File | Reason | diff --git a/domains/perps/skills/perps-review-pr/references/shared.md b/domains/perps/skills/perps-review-pr/references/shared.md index baab5c3f..ac38b91c 100644 --- a/domains/perps/skills/perps-review-pr/references/shared.md +++ b/domains/perps/skills/perps-review-pr/references/shared.md @@ -12,18 +12,27 @@ - [ ] **New dependency not in DI interface** — controller code reaching outside its boundary (e.g., importing a hook, React context, or an app utility). Everything the controller needs must come through `PerpsPlatformDependencies`. - [ ] **Breaking the publisher contract** — changing PerpsController's public API (state shape, method signatures, event names) without considering both consumers. Controller is a publisher — mobile and extension both consume it. - + -## Magic Strings, Magic Numbers & Placeholder Values +## Missing numeric data stays undefined -Constants live in the controller package (`core/packages/perps-controller/src/constants/perpsConfig.ts`, exported by `@metamask/perps-controller`) and in the reviewed client's UI constants module. PRs must use these — not inline literals. +Missing Perps numeric data must remain `undefined` through provider adapters, controller state, selectors, hooks and calculations. Zero means an observed or computed zero from known inputs. It must never stand for missing, loading, failed or invalid data. This applies to prices, balances, PnL, fees, margin, leverage and percentages. -- [ ] **Defaulting to `0` when data is unavailable** — the most common mistake. When price/percentage/data hasn't loaded yet, use the placeholder constants, NOT `0`, `$0`, or `0%`: +- [ ] **Missing data converted to zero**: Reject `value ?? 0`, `value || 0`, default parameters of `0`, and parsing or arithmetic that turns absent or invalid input into zero. Check availability before conversion or calculation; if a required input is missing, the result stays `undefined`. An explicit accumulator seed for known values is valid, but must not conceal an unavailable collection or missing required member. +- [ ] **Unknown treated as confirmed zero**: Preserve a genuine `0` with explicit availability checks. Do not use truthiness to decide whether a value exists. Keep actions that require the missing value disabled until it is available. +- [ ] **Reviewer proposes a fabricated value**: Review fixes follow the same rule. A crash or type error caused by missing data requires explicit missing-state handling, never a numeric fallback. Require separate cases for absent input, invalid input and genuine zero; verify missing inputs cannot enable submission or feed financial calculations as zero. +- [ ] **Placeholder written into numeric state**: Render missing values with display placeholders only at the formatting boundary: - `PERPS_CONSTANTS.FallbackPriceDisplay` (`'$---'`) — price not yet loaded - `PERPS_CONSTANTS.FallbackPercentageDisplay` (`'--%'`) — percentage not yet loaded - `PERPS_CONSTANTS.FallbackDataDisplay` (`'--'`) — generic data not yet loaded - `PERPS_CONSTANTS.ZeroAmountDisplay` (`'$0'`) / `ZeroAmountDetailedDisplay` (`'$0.00'`) — ONLY for actual confirmed zero values (e.g., no volume), never for "loading" or "unavailable" - - Defaulting to `0` hides loading states, makes bugs invisible, and can mislead users into thinking their balance/PnL is actually zero. + + + +## Magic Strings, Magic Numbers & Placeholder Values + +Constants live in the controller package (`core/packages/perps-controller/src/constants/perpsConfig.ts`, exported by `@metamask/perps-controller`) and in the reviewed client's UI constants module. PRs must use these — not inline literals. + - [ ] **Inline timeout/delay values** — hardcoded `5000`, `10000`, `300` instead of `PERPS_CONSTANTS.WebsocketTimeout`, `PERPS_CONSTANTS.ConnectionTimeoutMs`, `PERFORMANCE_CONFIG.ValidationDebounceMs`, etc. Every timing constant has a named export. - [ ] **Hardcoded slippage** — using `0.03` or `300` instead of `ORDER_SLIPPAGE_CONFIG.DefaultMarketSlippageBps`, `DefaultTpslSlippageBps`, `DefaultLimitSlippageBps`. - [ ] **Hardcoded leverage fallback** — using `3` or `50` instead of `PERPS_CONSTANTS.DefaultMaxLeverage` or `MARGIN_ADJUSTMENT_CONFIG.FallbackMaxLeverage`. @@ -41,14 +50,14 @@ Constants live in the controller package (`core/packages/perps-controller/src/co - [ ] **Provider identity lost during transformation**: Preserve provider identity through fill aggregation and apply provider-specific classification at the normalization boundary. Adding a provider must retain existing providers and cover equivalent inputs with different provider semantics. -All provider access must go through `AggregatedPerpsProvider` → `ProviderRouter`. HyperLiquid is primary, MYX is feature-flagged. +All provider access must go through `AggregatedPerpsProvider` → `ProviderRouter`. HyperLiquid is primary; Lighter is feature-flagged (`perpsLighterProviderEnabled`). -- [ ] **Hardcoded provider** — uses HyperLiquid or MYX APIs directly instead of going through `AggregatedPerpsProvider` / `ProviderRouter`. All operations must route through the abstraction. +- [ ] **Hardcoded provider** — uses a provider's API directly instead of going through `AggregatedPerpsProvider` / `ProviderRouter`. All operations must route through the abstraction. - [ ] **Provider-specific branching in UI** — `if (provider === 'hyperliquid')` in components or hooks. Provider differences must be normalized in the aggregation layer, not leaked to the view. - [ ] **Provider-specific error handling** — catches errors from one provider but not others. All providers must have consistent error boundaries via the aggregated layer. - [ ] **Hardcoded market symbols** — string literals `"BTC"` or `"ETH"` instead of market config constants. Breaks when new markets or providers are added. -- [ ] **Hardcoded decimals/precision** — using provider-native decimal formats without normalization. HyperLiquid and MYX use different precision for prices, sizes, and leverage. Must go through `MarketDataFormatters` (DI). -- [ ] **`detailedOrderType` rendered directly in UI** — `detailedOrderType` is provider-native text, not an enum. HyperLiquid returns `Limit`, `Market`, `Stop Limit`, `Stop Market`, `Take Profit Limit`, `Take Profit Market`; MYX (`myxAdapter.mjs`) returns `Take Profit`, `Stop Loss`, `Liquidation` — which are not in that set. Any UI that renders `detailedOrderType` directly is provider-dependent by construction. **Grep for `detailedOrderType` in any PR touching order display** — it should be mapped through a locale string or normalized constant, not rendered raw. +- [ ] **Hardcoded decimals/precision** — using provider-native decimal formats without normalization. Providers use different precision for prices, sizes, and leverage. Must go through `MarketDataFormatters` (DI). +- [ ] **`detailedOrderType` rendered directly in UI** — `detailedOrderType` is provider-native text, not an enum. HyperLiquid returns, e.g., `Limit`, `Market`, `Stop Limit`, `Stop Market`, `Take Profit Limit`, `Take Profit Market`. Any UI that renders `detailedOrderType` directly is provider-dependent by construction. **Grep for `detailedOrderType` in any PR touching order display** — it should be mapped through a locale string or normalized constant, not rendered raw. @@ -103,6 +112,7 @@ A single `PerpsAlwaysOnProvider` at the wallet root owns connect/disconnect; `Pe ## Data Flow & State +- [ ] **Delayed balance tracker loses its initiating context**: Bind the initiating account, provider and network. The first channel delivery may replay persisted preload data; it is not proof of fresh credit. Channel clearing also occurs on reconnect, so it is not an account-change signal. A timeout means credit was not observed, not that funds are available. Test provider and network changes while the account address stays fixed. - [ ] **A changed classification leaves old priority rules**: When a validation becomes advisory, audit message ranking and CTA gating together. A finished-input warning needs commit/blur state that clears on the next edit; interaction alone is insufficient. Test mixed blockers/advice and editing an already committed value. - [ ] **State persists outside the rendered control**: Disabling new presses does not dismiss an open keypad or active gesture. Test the transition while editing, and preserve the intended input when live limits update. - [ ] **React persistence mistaken for WebView synchronization**: Inline and fullscreen charts can remain mounted together. Prove the handoff updates each chart's local range/state, including subsequent stream updates. diff --git a/domains/perps/skills/perps-review-pr/repos/core.md b/domains/perps/skills/perps-review-pr/repos/core.md index af019a76..92651ea8 100644 --- a/domains/perps/skills/perps-review-pr/repos/core.md +++ b/domains/perps/skills/perps-review-pr/repos/core.md @@ -16,6 +16,7 @@ Required for changes to the controller package or its public contract, in additi ## Verdict and handoff -- [ ] Write artifacts/review.md with Summary, Criteria outcomes, Findings, Evidence, Limitations and Recommended Action. Include the frozen head and rule revision. Findings need severity, file:line, impact and the smallest correction. Preserve prior findings and their re-review disposition. Required NOT_CHECKED items prevent APPROVE; use COMMENT for missing evidence in standalone reports and REQUEST_CHANGES for actionable findings. If the host only accepts pass/issues, missing required evidence must block the task instead of fabricating an issue or passing it. Follow the host's required verdict/header fields. Distinguish runtime QA requests from static conclusions. +- [ ] Write artifacts/review.md with Summary, Criteria outcomes, Findings, Evidence, Limitations and Recommended Action. Include the frozen head, the rule revision and each consumer revision read. Findings need severity, file:line, impact and the smallest correction. Preserve prior findings and their re-review disposition. Follow the host's required verdict/header fields. Distinguish runtime QA requests from static conclusions. +- [ ] Decide APPROVE or REQUEST_CHANGES. APPROVE needs an empty BLOCKERS list; non-blocking nits may come with it. REQUEST_CHANGES names each blocker with a concrete ask. COMMENT is only for a draft PR or an explicitly informational request. A row the reviewer could not check (no reference checkout, no runtime) goes under Limitations as "not verified by this review" and does not block APPROVE. Evidence the author owes, such as runtime behaviour a PR claims without the proof its repository guidelines require, is a NIT or BLOCKER with a concrete ask, never a silent COMMENT. A pass/issues host maps APPROVE to pass and REQUEST_CHANGES to issues. For pass, keep issues empty; retain non-blocking nits in review.md and line-comments.json under the host contract. - [ ] Write artifacts/line-comments.json using the host contract, or {"pr_number": , "recommendation": "APPROVE|REQUEST_CHANGES|COMMENT", "summary": "...", "comments": [{"path": "...", "line": 1, "body": "...", "severity": "must_fix|suggestion|nitpick"}]} for a PR task. Only attach changed-line findings; retain other findings in review.md. Write artifacts/learnings.md. For a branch-only review, use an empty comments array without inventing a PR number when the terminal contract requires that file. -- [ ] Reconcile the changed-file/acceptance-criteria inventory with the rule outcomes before choosing a verdict. Every applicable rule needs evidence or an explicit gap; required NOT_CHECKED items prevent approval. In a hosted child checklist, return the report to the caller without completing the parent. For a standalone materialized task, satisfy inputs/worker-terminal-contract.json and its completion command. The caller owns publication, retained sessions and cleanup. +- [ ] Reconcile the changed-file/acceptance-criteria inventory with the rule outcomes before choosing a verdict. Every applicable rule needs evidence or an explicit gap: a gap the author owes is a finding, a reviewer-side gap is listed as not verified. In a hosted child checklist, return the report to the caller without completing the parent. For a standalone materialized task, satisfy inputs/worker-terminal-contract.json and its completion command. The caller owns publication, retained sessions and cleanup. diff --git a/domains/perps/skills/perps-review-pr/repos/metamask-extension.md b/domains/perps/skills/perps-review-pr/repos/metamask-extension.md index 90c9ef53..d04aa70d 100644 --- a/domains/perps/skills/perps-review-pr/repos/metamask-extension.md +++ b/domains/perps/skills/perps-review-pr/repos/metamask-extension.md @@ -21,6 +21,7 @@ Required for Extension changes, in addition to the Perps families above. ## Verdict and handoff -- [ ] Write artifacts/review.md with Summary, Criteria outcomes, Findings, Evidence, Limitations and Recommended Action. Include the frozen head and rule revision. Findings need severity, file:line, impact and the smallest correction. Preserve prior findings and their re-review disposition. Required NOT_CHECKED items prevent APPROVE; use COMMENT for missing evidence in standalone reports and REQUEST_CHANGES for actionable findings. If the host only accepts pass/issues, missing required evidence must block the task instead of fabricating an issue or passing it. Follow the host's required verdict/header fields. Distinguish runtime QA requests from static conclusions. +- [ ] Write artifacts/review.md with Summary, Criteria outcomes, Findings, Evidence, Limitations and Recommended Action. Include the frozen head, the rule revision and each consumer revision read. Findings need severity, file:line, impact and the smallest correction. Preserve prior findings and their re-review disposition. Follow the host's required verdict/header fields. Distinguish runtime QA requests from static conclusions. +- [ ] Decide APPROVE or REQUEST_CHANGES. APPROVE needs an empty BLOCKERS list; non-blocking nits may come with it. REQUEST_CHANGES names each blocker with a concrete ask. COMMENT is only for a draft PR or an explicitly informational request. A row the reviewer could not check (no reference checkout, no runtime) goes under Limitations as "not verified by this review" and does not block APPROVE. Evidence the author owes, such as runtime behaviour a PR claims without the proof its repository guidelines require, is a NIT or BLOCKER with a concrete ask, never a silent COMMENT. A pass/issues host maps APPROVE to pass and REQUEST_CHANGES to issues. For pass, keep issues empty; retain non-blocking nits in review.md and line-comments.json under the host contract. - [ ] Write artifacts/line-comments.json using the host contract, or {"pr_number": , "recommendation": "APPROVE|REQUEST_CHANGES|COMMENT", "summary": "...", "comments": [{"path": "...", "line": 1, "body": "...", "severity": "must_fix|suggestion|nitpick"}]} for a PR task. Only attach changed-line findings; retain other findings in review.md. Write artifacts/learnings.md. For a branch-only review, use an empty comments array without inventing a PR number when the terminal contract requires that file. -- [ ] Reconcile the changed-file/acceptance-criteria inventory with the rule outcomes before choosing a verdict. Every applicable rule needs evidence or an explicit gap; required NOT_CHECKED items prevent approval. In a hosted child checklist, return the report to the caller without completing the parent. For a standalone materialized task, satisfy inputs/worker-terminal-contract.json and its completion command. The caller owns publication, retained sessions and cleanup. +- [ ] Reconcile the changed-file/acceptance-criteria inventory with the rule outcomes before choosing a verdict. Every applicable rule needs evidence or an explicit gap: a gap the author owes is a finding, a reviewer-side gap is listed as not verified. In a hosted child checklist, return the report to the caller without completing the parent. For a standalone materialized task, satisfy inputs/worker-terminal-contract.json and its completion command. The caller owns publication, retained sessions and cleanup. diff --git a/domains/perps/skills/perps-review-pr/repos/metamask-mobile.md b/domains/perps/skills/perps-review-pr/repos/metamask-mobile.md index 0fdf01c9..8a30f7ea 100644 --- a/domains/perps/skills/perps-review-pr/repos/metamask-mobile.md +++ b/domains/perps/skills/perps-review-pr/repos/metamask-mobile.md @@ -16,6 +16,7 @@ Apply these Mobile-specific families in addition to the shared checks. Compare c ## Verdict and handoff -- [ ] Write artifacts/review.md with Summary, Criteria outcomes, Findings, Evidence, Limitations and Recommended Action. Include the frozen head and rule revision. Findings need severity, file:line, impact and the smallest correction. Preserve prior findings and their re-review disposition. Required NOT_CHECKED items prevent APPROVE; use COMMENT for missing evidence in standalone reports and REQUEST_CHANGES for actionable findings. If the host only accepts pass/issues, missing required evidence must block the task instead of fabricating an issue or passing it. Follow the host's required verdict/header fields. Distinguish runtime QA requests from static conclusions. +- [ ] Write artifacts/review.md with Summary, Criteria outcomes, Findings, Evidence, Limitations and Recommended Action. Include the frozen head, the rule revision and each consumer revision read. Findings need severity, file:line, impact and the smallest correction. Preserve prior findings and their re-review disposition. Follow the host's required verdict/header fields. Distinguish runtime QA requests from static conclusions. +- [ ] Decide APPROVE or REQUEST_CHANGES. APPROVE needs an empty BLOCKERS list; non-blocking nits may come with it. REQUEST_CHANGES names each blocker with a concrete ask. COMMENT is only for a draft PR or an explicitly informational request. A row the reviewer could not check (no reference checkout, no runtime) goes under Limitations as "not verified by this review" and does not block APPROVE. Evidence the author owes, such as runtime behaviour a PR claims without the proof its repository guidelines require, is a NIT or BLOCKER with a concrete ask, never a silent COMMENT. A pass/issues host maps APPROVE to pass and REQUEST_CHANGES to issues. For pass, keep issues empty; retain non-blocking nits in review.md and line-comments.json under the host contract. - [ ] Write artifacts/line-comments.json using the host contract, or {"pr_number": , "recommendation": "APPROVE|REQUEST_CHANGES|COMMENT", "summary": "...", "comments": [{"path": "...", "line": 1, "body": "...", "severity": "must_fix|suggestion|nitpick"}]} for a PR task. Only attach changed-line findings; retain other findings in review.md. Write artifacts/learnings.md. For a branch-only review, use an empty comments array without inventing a PR number when the terminal contract requires that file. -- [ ] Reconcile the changed-file/acceptance-criteria inventory with the rule outcomes before choosing a verdict. Every applicable rule needs evidence or an explicit gap; required NOT_CHECKED items prevent approval. In a hosted child checklist, return the report to the caller without completing the parent. For a standalone materialized task, satisfy inputs/worker-terminal-contract.json and its completion command. The caller owns publication, retained sessions and cleanup. +- [ ] Reconcile the changed-file/acceptance-criteria inventory with the rule outcomes before choosing a verdict. Every applicable rule needs evidence or an explicit gap: a gap the author owes is a finding, a reviewer-side gap is listed as not verified. In a hosted child checklist, return the report to the caller without completing the parent. For a standalone materialized task, satisfy inputs/worker-terminal-contract.json and its completion command. The caller owns publication, retained sessions and cleanup. diff --git a/domains/perps/skills/perps-review-pr/scripts/materialize-review.mjs b/domains/perps/skills/perps-review-pr/scripts/materialize-review.mjs index 6175a764..67dc2853 100644 --- a/domains/perps/skills/perps-review-pr/scripts/materialize-review.mjs +++ b/domains/perps/skills/perps-review-pr/scripts/materialize-review.mjs @@ -109,7 +109,7 @@ function baseBody(inline) { 'Check applicability against the frozen diff and its affected callers. Open each applicable family and examine every rule in it; a family checkbox is complete only when its individual outcomes are recorded.', '', ...rows('shared', perps, inline), '## Cross-repository conformity', '', - `- [ ] When screens, hooks, formatters or shared behavior change, compare the affected client counterparts using the parity map${inline ? ' below' : ' in references/parity.md'}. Mobile is the reference implementation; do not copy Extension divergence back into Mobile. Record applicable missing references as NOT_CHECKED.`, + `- [ ] When screens, hooks, formatters or shared behavior change, compare the affected client counterparts using the parity map${inline ? ' below' : ' in references/parity.md'}. Mobile is the reference implementation; do not copy Extension divergence back into Mobile. Read consumers from the task's reference checkouts (\`MM_HARNESS_REF_MOBILE\`, \`MM_HARNESS_REF_EXTENSION\`, \`MM_HARNESS_REF_CORE\`) and cite the revision you read; an unavailable checkout is NOT_CHECKED, not verified by this review.`, `- [ ] When controller state, methods, events, exports or package versions change, inspect Core and both consumers at recorded revisions${inline ? '' : ', using references/shared-packages.md for the shared surface and references/owned-paths.json for the paths this review covers'}. Check public imports, compatibility and migrations. Report evidence gaps; do not claim that clients compile from source inspection.`, '', ...(inline ? [documents['review/parity.md'].replace(/^#/gm, '##').trim(), '', @@ -122,9 +122,10 @@ function baseBody(inline) { const verdict = [ '## Verdict and handoff', '', - '- [ ] Write artifacts/review.md with Summary, Criteria outcomes, Findings, Evidence, Limitations and Recommended Action. Include the frozen head and rule revision. Findings need severity, file:line, impact and the smallest correction. Preserve prior findings and their re-review disposition. Required NOT_CHECKED items prevent APPROVE; use COMMENT for missing evidence in standalone reports and REQUEST_CHANGES for actionable findings. If the host only accepts pass/issues, missing required evidence must block the task instead of fabricating an issue or passing it. Follow the host\'s required verdict/header fields. Distinguish runtime QA requests from static conclusions.', + '- [ ] Write artifacts/review.md with Summary, Criteria outcomes, Findings, Evidence, Limitations and Recommended Action. Include the frozen head, the rule revision and each consumer revision read. Findings need severity, file:line, impact and the smallest correction. Preserve prior findings and their re-review disposition. Follow the host\'s required verdict/header fields. Distinguish runtime QA requests from static conclusions.', + '- [ ] Decide APPROVE or REQUEST_CHANGES. APPROVE needs an empty BLOCKERS list; non-blocking nits may come with it. REQUEST_CHANGES names each blocker with a concrete ask. COMMENT is only for a draft PR or an explicitly informational request. A row the reviewer could not check (no reference checkout, no runtime) goes under Limitations as "not verified by this review" and does not block APPROVE. Evidence the author owes, such as runtime behaviour a PR claims without the proof its repository guidelines require, is a NIT or BLOCKER with a concrete ask, never a silent COMMENT. A pass/issues host maps APPROVE to pass and REQUEST_CHANGES to issues. For pass, keep issues empty; retain non-blocking nits in review.md and line-comments.json under the host contract.', '- [ ] Write artifacts/line-comments.json using the host contract, or {"pr_number": , "recommendation": "APPROVE|REQUEST_CHANGES|COMMENT", "summary": "...", "comments": [{"path": "...", "line": 1, "body": "...", "severity": "must_fix|suggestion|nitpick"}]} for a PR task. Only attach changed-line findings; retain other findings in review.md. Write artifacts/learnings.md. For a branch-only review, use an empty comments array without inventing a PR number when the terminal contract requires that file.', - '- [ ] Reconcile the changed-file/acceptance-criteria inventory with the rule outcomes before choosing a verdict. Every applicable rule needs evidence or an explicit gap; required NOT_CHECKED items prevent approval. In a hosted child checklist, return the report to the caller without completing the parent. For a standalone materialized task, satisfy inputs/worker-terminal-contract.json and its completion command. The caller owns publication, retained sessions and cleanup.', + '- [ ] Reconcile the changed-file/acceptance-criteria inventory with the rule outcomes before choosing a verdict. Every applicable rule needs evidence or an explicit gap: a gap the author owes is a finding, a reviewer-side gap is listed as not verified. In a hosted child checklist, return the report to the caller without completing the parent. For a standalone materialized task, satisfy inputs/worker-terminal-contract.json and its completion command. The caller owns publication, retained sessions and cleanup.', ]; function overlayBody(client, inline) { diff --git a/domains/perps/skills/perps-review-pr/skill.md b/domains/perps/skills/perps-review-pr/skill.md index 4796704a..e754102d 100644 --- a/domains/perps/skills/perps-review-pr/skill.md +++ b/domains/perps/skills/perps-review-pr/skill.md @@ -7,7 +7,7 @@ maturity: stable # Perps static review -Generated from MetaMask/experimental-metamask-recipe-perps @ e06bb8d750acbbd90ce1af62063260e85f0a0995. Do not hand-edit: regenerate with scripts/materialize-review.mjs. references/review-sources.json records every source digest. +Generated from MetaMask/experimental-metamask-recipe-perps @ f9f8cb914c4c0fd1f7a592b9b4507b410a5d8651. Do not hand-edit: regenerate with scripts/materialize-review.mjs. references/review-sources.json records every source digest. Run only on explicit invocation by name or an explicitly selected workflow. Review source and diff only: no harness, no app launch, no product change, no publish, no workspace cleanup. The criteria below are review criteria, not instructions to perform the fixes, releases or migrations they describe. @@ -35,13 +35,14 @@ For maintaining or adapting this skill to another team, see references/maintaini Check applicability against the frozen diff and its affected callers. Open each applicable family and examine every rule in it; a family checkbox is complete only when its individual outcomes are recorded. - [ ] Controller Portability (Core): `PerpsController` lives in `core/packages/perps-controller` and is published as `@metamask/perps-controller`; mobile and extension both consume the package. See references/shared.md#controller-portability-core +- [ ] Missing numeric data stays undefined: Missing Perps numeric data must remain `undefined` through provider adapters, controller state, selectors, hooks and calculations. See references/shared.md#missing-numeric-data-stays-undefined - [ ] Magic Strings, Magic Numbers & Placeholder Values: Constants live in the controller package (`core/packages/perps-controller/src/constants/perpsConfig.ts`, exported by `@metamask/perps-controller`) and in the reviewed client's UI constants module. See references/shared.md#magic-strings-magic-numbers-placeholder-values - [ ] Protocol Abstraction: Execution identity inferred from display fields: Preserve the venue's documented identity at the provider boundary and expose a stable opaque ID shared by REST and WebSocket paths. See references/shared.md#protocol-abstraction - [ ] Pro Mode UI Gating: Pro market UI renders only when the remote flag (`selectPerpsProModeEnabledFlag`) and the controller mode (`PerpsMode.Pro`) are both active; a PR that checks one gate ships a silent no-op that looks… See references/shared.md#pro-mode-ui-gating - [ ] MetaMetrics Events: Every perps event uses one of the eight consolidated events and their typed property constants; no new event names or untyped properties. See references/shared.md#metametrics-events - [ ] Sentry Tracing: Unbounded background trace volume: For unlock, polling, reconnect or fan-out instrumentation, estimate added spans at normal and retry load. See references/shared.md#sentry-tracing - [ ] Connection & WebSocket Architecture: Cleanup has no owner for in-flight setup: Register the owner before asynchronous initialization starts. See references/shared.md#connection-websocket-architecture -- [ ] Data Flow & State: A changed classification leaves old priority rules: When a validation becomes advisory, audit message ranking and CTA gating together. See references/shared.md#data-flow-state +- [ ] Data Flow & State: Delayed balance tracker loses its initiating context: Bind the initiating account, provider and network. See references/shared.md#data-flow-state - [ ] Trade Flow & Order Execution: Signed bounds collapsed into magnitudes: A gain-side and loss-side RoE are different inputs. See references/shared.md#trade-flow-order-execution - [ ] Locale Coverage & Orphaned Keys: Duplicate JSON keys shadow new copy: Verify the containing locale object has one definition and search rendered copy across every test layer. See references/shared.md#locale-coverage-orphaned-keys - [ ] Test Layer Coverage: Degenerate fixtures hide formula errors: Choose values where competing calculations differ, such as a deeper order-book row with size unequal to cumulative total. See references/shared.md#test-layer-coverage @@ -49,5 +50,5 @@ Check applicability against the frozen diff and its affected callers. Open each ## Cross-repository conformity -- [ ] When screens, hooks, formatters or shared behavior change, compare the affected client counterparts using the parity map in references/parity.md. Mobile is the reference implementation; do not copy Extension divergence back into Mobile. Record applicable missing references as NOT_CHECKED. +- [ ] When screens, hooks, formatters or shared behavior change, compare the affected client counterparts using the parity map in references/parity.md. Mobile is the reference implementation; do not copy Extension divergence back into Mobile. Read consumers from the task's reference checkouts (`MM_HARNESS_REF_MOBILE`, `MM_HARNESS_REF_EXTENSION`, `MM_HARNESS_REF_CORE`) and cite the revision you read; an unavailable checkout is NOT_CHECKED, not verified by this review. - [ ] When controller state, methods, events, exports or package versions change, inspect Core and both consumers at recorded revisions, using references/shared-packages.md for the shared surface and references/owned-paths.json for the paths this review covers. Check public imports, compatibility and migrations. Report evidence gaps; do not claim that clients compile from source inspection. diff --git a/domains/platform/skills/feature-flags/repos/metamask-extension.md b/domains/platform/skills/feature-flags/repos/metamask-extension.md new file mode 100644 index 00000000..46600ccb --- /dev/null +++ b/domains/platform/skills/feature-flags/repos/metamask-extension.md @@ -0,0 +1,10 @@ +--- +repo: metamask-extension +parent: feature-flags +--- + +# Feature flags — MetaMask Extension + +- Shared helper: `validatedVersionGatedFeatureFlag` in `shared/lib/remote-feature-flag-utils.ts`. +- `test/e2e/feature-flags/feature-flag-registry.ts` holds production defaults and feeds the E2E `/v1/flags` mock; a scheduled sync PR adds production keys. Grep it on `origin/main` and reuse any synced entry; add a pre-launch key with `inProd: false`. +- E2E with a non-default flag: prefer `manifestFlags.remoteFeatureFlags`. When the consumer reads `RemoteFeatureFlagController` state directly, set the fixture and the `/v1/flags` mock from one constant: the controller refetches whenever the UI opens and overwrites a fixture-only seed. diff --git a/domains/testing/skills/ab-testing/repos/metamask-extension.md b/domains/testing/skills/ab-testing/repos/metamask-extension.md index d6a06723..d667cedb 100644 --- a/domains/testing/skills/ab-testing/repos/metamask-extension.md +++ b/domains/testing/skills/ab-testing/repos/metamask-extension.md @@ -74,9 +74,7 @@ const activeABTests = experiment.isActive - Register every new remote A/B test flag in `test/e2e/feature-flags/feature-flag-registry.ts` with the production default threshold-array JSON value. - - Use test overrides such as `manifestFlags.remoteFeatureFlags` or - `FixtureBuilder.withRemoteFeatureFlags(...)` when a test needs - deterministic assignment. + - Override assignment in tests as the `feature-flags` Extension overlay says. - If the change is copy-only or config-only, you may skip new tests with a brief rationale. 7. Run the A/B compliance checker using the repository's current supported invocation and report the result. diff --git a/domains/typescript/skills/tsc-blindspots/skill.md b/domains/typescript/skills/tsc-blindspots/skill.md index 75395b8a..52f17f6d 100644 --- a/domains/typescript/skills/tsc-blindspots/skill.md +++ b/domains/typescript/skills/tsc-blindspots/skill.md @@ -2,23 +2,18 @@ name: tsc-blindspots description: >- Find the type defects `tsc` is structurally unable to report — a green build is - not evidence the types are correct. Covers the two classes: (1) hand-written - types that restate an authoritative source and disagree with it, caught by - substituting the derived type at a fixed commit and diffing `tsc` output; and - (2) the standing blind spots in the language and config — unchecked array/record - indexing, bivariant method parameters, covariant arrays, `any` absorption at - untyped boundaries, precise signatures fed `any` at every call site, ambient - `declare module` assertions that launder an `any` into a confident type, + not evidence the types are correct. Covers (1) hand-written types that restate + an authoritative source and disagree with it, caught by substituting the derived + type at a fixed commit and diffing `tsc` output; and (2) the standing blind + spots — unchecked indexing, bivariant method parameters, covariant arrays, `any` + absorption at untyped boundaries, ambient `declare module` assertions, excess-property checks that only fire on fresh literals, and external data asserted rather than validated. Also audits typing edits that quietly change - runtime behavior: - stripped `| undefined`, deleted default parameters, literals swapped for runtime - enum lookups, calls made optional so a throw becomes a silent no-op. Use when - reviewing a JS→TS migration, a PR that hand-writes types for values that already - have them, a "rename-only" refactor, or any PR claiming a change is mechanical. - Triggers on /mms-tsc-blindspots, or on phrases like "validate this TypeScript - migration", "is this type right", "does this type match the real shape", - "why didn't CI catch this type", "derive vs define", and "what can tsc not + runtime behavior (stripped `| undefined`, deleted defaults, literals swapped for + enum lookups, calls made optional). Use when reviewing a JS→TS migration, + hand-written types for already-typed values, a "rename-only" refactor, or any PR + claiming a change is mechanical. Triggers on /mms-tsc-blindspots, "validate this + TypeScript migration", "why didn't CI catch this type", and "what can tsc not check". maturity: experimental --- diff --git a/test/cli.test.mjs b/test/cli.test.mjs index 3e651e0e..9dba1709 100644 --- a/test/cli.test.mjs +++ b/test/cli.test.mjs @@ -1,6 +1,6 @@ import assert from 'node:assert/strict'; import { spawnSync } from 'node:child_process'; -import { copyFileSync, existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'; +import { copyFileSync, existsSync, lstatSync, mkdirSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'; import os from 'node:os'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; @@ -344,3 +344,103 @@ describe('tools/sync installer resolution', () => { assert.doesNotMatch(result.stdout, /STALE-OVERLAY/u, 'must not fall through to the overlay'); }); }); + +describe('source resolution and aliases', () => { + let root; + + before(() => { + root = mkdtempSync(path.join(os.tmpdir(), 'mms-resolve-')); + }); + + after(() => { + rmSync(root, { recursive: true, force: true }); + }); + + function writeSkill(source, name, body) { + const dir = path.join(source, 'domains', 'testing', 'skills', name); + mkdirSync(dir, { recursive: true }); + writeFileSync( + path.join(dir, 'skill.md'), + ['---', `name: ${name}`, 'description: Fixture skill', 'maturity: stable', '---', body].join('\n'), + ); + } + + function install(target, sources, extra = []) { + return spawnSync( + '/bin/bash', + [INSTALL, '--target', target, '--repo', 'core', ...sources.flatMap((s) => ['--source', s]), ...extra], + { encoding: 'utf8' }, + ); + } + + function isLink(file) { + try { + return lstatSync(file).isSymbolicLink(); + } catch { + return false; + } + } + + test('a later source directory without skill.md does not shadow an earlier skill', () => { + const base = path.join(root, 'base'); + const overlay = path.join(root, 'overlay'); + const target = path.join(root, 'target-shadow'); + mkdirSync(target, { recursive: true }); + writeSkill(base, 'shadowed', 'BASE-BODY'); + writeSkill(base, 'overridden', 'BASE-BODY'); + writeSkill(overlay, 'overridden', 'OVERLAY-BODY'); + const refs = path.join(overlay, 'domains', 'testing', 'skills', 'shadowed', 'references'); + mkdirSync(refs, { recursive: true }); + writeFileSync(path.join(refs, 'extra.md'), 'Overlay reference.\n'); + + const result = install(target, [base, overlay]); + + assert.equal(result.status, 0, `${result.stdout}\n${result.stderr}`); + const skills = path.join(target, '.agents', 'skills'); + assert.match(readFileSync(path.join(skills, 'mms-shadowed', 'SKILL.md'), 'utf8'), /BASE-BODY/u); + assert.match(readFileSync(path.join(skills, 'mms-overridden', 'SKILL.md'), 'utf8'), /OVERLAY-BODY/u); + assert.match(result.stderr, /no skill\.md in .*shadowed.*; keeping/u); + }); + + test('prune-stale removes unprefixed aliases of managed skills in every destination', () => { + const source = path.join(root, 'alias-src'); + const target = path.join(root, 'target-alias'); + mkdirSync(target, { recursive: true }); + writeSkill(source, 'current', 'CURRENT-BODY'); + assert.equal(install(target, [source]).status, 0); + const custom = path.join(root, 'custom-skill'); + mkdirSync(custom, { recursive: true }); + const destinations = ['.claude/skills', '.cursor/rules', '.agents/skills'].map((d) => path.join(target, d)); + for (const dir of destinations) symlinkSync('mms-current', path.join(dir, 'current')); + symlinkSync(custom, path.join(destinations[2], 'custom-link')); + + const result = install(target, [source], ['--prune-stale']); + + assert.equal(result.status, 0, `${result.stdout}\n${result.stderr}`); + for (const dir of destinations) { + assert.equal(isLink(path.join(dir, 'current')), false, `${dir}/current`); + assert.equal(existsSync(path.join(dir, 'mms-current')), true, `${dir}/mms-current`); + } + assert.equal(isLink(path.join(destinations[2], 'custom-link')), true); + }); + + test('prune-stale removes an alias whose managed target is pruned in the same run', () => { + const source = path.join(root, 'stale-src'); + const target = path.join(root, 'target-stale-alias'); + mkdirSync(target, { recursive: true }); + writeSkill(source, 'current', 'CURRENT-BODY'); + assert.equal(install(target, [source]).status, 0); + const skills = path.join(target, '.agents', 'skills'); + mkdirSync(path.join(skills, 'mms-gone'), { recursive: true }); + writeFileSync(path.join(skills, 'mms-gone', 'SKILL.md'), `${MANAGED_BANNER}\nBody.\n`); + symlinkSync('mms-gone', path.join(skills, 'gone')); + symlinkSync('mms-retired-already', path.join(skills, 'dangling')); + + const result = install(target, [source], ['--prune-stale']); + + assert.equal(result.status, 0, `${result.stdout}\n${result.stderr}`); + assert.equal(existsSync(path.join(skills, 'mms-gone')), false); + assert.equal(isLink(path.join(skills, 'gone')), false); + assert.equal(isLink(path.join(skills, 'dangling')), false); + }); +}); diff --git a/tools/install b/tools/install index acb05934..9e399a6b 100755 --- a/tools/install +++ b/tools/install @@ -471,13 +471,27 @@ is_managed_project_skill() { } remove_stale_project_skills() { - local parent label dir + local parent label dir target for entry in \ "$CLAUDE_DIR|.claude/skills" \ "$CURSOR_DIR|.cursor/rules" \ "$AGENTS_DIR|.agents/skills"; do parent="${entry%%|*}" label="${entry#*|}" + # An unprefixed alias (e.g. recipe-cook -> mms-recipe-cook) duplicates a + # managed skill under a second name; remove the link, never its target. + # Runs before the stale pass so an alias of a skill pruned below, or of a + # retired skill already removed (dangling link), is still recognised. + for dir in "$parent"/*; do + [[ -L "$dir" ]] || continue + target=$(readlink "$dir") + [[ "$target" == "${PREFIX}"* && "$target" != */* ]] || continue + if [[ -e "$parent/$target" ]]; then + is_managed_project_skill "$parent/$target" "$label" || continue + fi + action "$label/$(basename "$dir") (remove alias of managed $target)" + $DRY_RUN || rm -f "$dir" + done for dir in "$parent"/${PREFIX}*; do [[ -e "$dir" ]] || continue [[ -d "$dir" ]] || continue @@ -717,9 +731,13 @@ for src in "${SOURCES[@]}"; do RESOLVED_KEYS+=("$key") RESOLVED_DIRS+=("$skill_dir") RESOLVED_DOMAINS+=("$domain_name") - else + elif [[ -f "$skill_dir/skill.md" ]]; then RESOLVED_DIRS[$resolved_index]="$skill_dir" RESOLVED_DOMAINS[$resolved_index]="$domain_name" + else + # A later source's directory without skill.md is not a skill; letting it + # win would replace a working skill with one process_skill then skips. + echo "Warning: no skill.md in $skill_dir; keeping ${RESOLVED_DIRS[$resolved_index]}" >&2 fi done done