diff --git a/.agents/skills/improve-ic-skill/SKILL.md b/.agents/skills/improve-ic-skill/SKILL.md index 0963f3d..682ad55 100644 --- a/.agents/skills/improve-ic-skill/SKILL.md +++ b/.agents/skills/improve-ic-skill/SKILL.md @@ -56,7 +56,7 @@ Read with fresh eyes. Common high-value areas: **Required frontmatter** — `metadata.title` and `metadata.category` are required by CI. Missing these blocks deployment. -For upstream-tracked skills (`motoko`, `migrating-motoko`, `migrating-motoko-enhanced`, `mops-cli`): read `.claude/upstream.md` carefully. Only modify icskills-owned sections freely; changes to shared content should also be filed upstream. +For upstream-tracked skills (`writing-motoko`, `migrating-motoko-actors`, `troubleshooting-motoko-migrations`, `reviewing-motoko`, `mops-cli`, `static-site`): read `.claude/upstream.md` carefully. Only modify icskills-owned sections freely; changes to shared content should also be filed upstream. ## Step 4 — Apply improvements @@ -176,4 +176,4 @@ The skill is ready for a PR. Include a brief summary of what changed and, if you ## Upstream-tracked skills -Before editing `motoko`, `migrating-motoko`, `migrating-motoko-enhanced`, or `mops-cli`, read `.claude/upstream.md`. It lists which sections are icskills-owned. You can freely improve owned sections. For shared content, improvements should also be filed as issues upstream so they flow back on the next sync. +Before editing `writing-motoko`, `migrating-motoko-actors`, `troubleshooting-motoko-migrations`, `reviewing-motoko`, `mops-cli`, or `static-site`, read `.claude/upstream.md`. It lists which sections are icskills-owned. You can freely improve owned sections. For shared content, improvements should also be filed as issues upstream so they flow back on the next sync. diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index 675bc13..5aa4a2f 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -117,7 +117,7 @@ All sync metadata lives in `.claude/upstream.md` — one entry per tracked skill Two tracking models are in use: - **Release-tracked** (`caffeinelabs/mops` → `mops-cli`; `dfinity/certified-assets` → `static-site`): pinned to a tagged release. When syncing, update the **Tag**, **Commit** (full SHA), and **Last synced** fields. Always store the **full commit SHA**, not just the tag name; annotated tags require a two-step dereference (tag object → commit SHA). -- **Commit-tracked** (`caffeinelabs/skills` → `writing-motoko`, `migrating-motoko-actors`, `troubleshooting-motoko-migrations`): this upstream publishes **no releases**, so it is pinned to a commit on the default branch. When syncing, update the **Commit** (full SHA) and **Last synced** fields, and the **Upstream version** field (read from each skill's `version:` frontmatter — a secondary signal that meaningful changes landed). There is no **Tag**. All three skills from this repo share one pinned commit. +- **Commit-tracked** (`caffeinelabs/skills` → `writing-motoko`, `migrating-motoko-actors`, `troubleshooting-motoko-migrations`, `reviewing-motoko`): this upstream publishes **no releases**, so it is pinned to a commit on the default branch. When syncing, update the **Commit** (full SHA) and **Last synced** fields, and the **Upstream version** field (read from each skill's `version:` frontmatter — a secondary signal that meaningful changes landed). There is no **Tag**. All four skills from this repo share one pinned commit. When the upstream SKILL.md references files that live within the upstream skill's folder (e.g. `examples.md`), list each with `→ references/.md` to show the icskills path. icskills always places such files under `references/` regardless of how upstream organises them. @@ -175,7 +175,7 @@ When new upstream content is available (a new release tag, or new commits on a c When syncing a skill from a new upstream release, verify all of these before committing: -- [ ] **`.claude/upstream.md` updated** — Commit (full SHA) + Last synced for every skill entry that shares the upstream repo, plus Tag (release-tracked) or Upstream version (commit-tracked). E.g. syncing `caffeinelabs/skills` means updating all three of `writing-motoko`, `migrating-motoko-actors`, and `troubleshooting-motoko-migrations` — they share one pinned commit. +- [ ] **`.claude/upstream.md` updated** — Commit (full SHA) + Last synced for every skill entry that shares the upstream repo, plus Tag (release-tracked) or Upstream version (commit-tracked). E.g. syncing `caffeinelabs/skills` means updating all four of `writing-motoko`, `migrating-motoko-actors`, `troubleshooting-motoko-migrations`, and `reviewing-motoko` — they share one pinned commit. - [ ] **All upstream skill folder files synced** — The sync issue diffs every file in the upstream skill folder (any type, not just `.md`) between old and new releases. Apply changes to all files that changed. If a file is new in upstream, add it to our `references/` directory. If a file was removed upstream, remove it from `references/` too. icskills always places reference files under `references/` regardless of how upstream organises them. - [ ] **Compatibility versions updated** — `compatibility:` frontmatter matches new feature requirements (e.g., `moc >= X.Y.Z, core >= A.B.C`) - [ ] **Version numbers in code examples** — All pinned versions in `mops.toml` snippets, `mops toolchain use` commands, and `mops add` examples reflect the new release @@ -183,7 +183,7 @@ When syncing a skill from a new upstream release, verify all of these before com - [ ] **Icskills-owned sections preserved** — Sections listed as owned in `.claude/upstream.md` are NOT overwritten from upstream - [ ] **Owned sections audited against upstream additions** — For each owned section, check whether the upstream diff now covers the same content. If upstream ships it, drop the icskills copy and remove the entry from `.claude/upstream.md` to avoid duplicating agent instructions. - [ ] **Icskills-only content audited** — Any content we have that is absent from the upstream diff must be either listed as owned in `.claude/upstream.md` or removed. Content not tracked there is a gap — file an upstream issue or add it to the owned list. -- [ ] **Cross-references use icskills skill names** — for `caffeinelabs/skills` the upstream and icskills names match (`writing-motoko`, `migrating-motoko-actors`, `troubleshooting-motoko-migrations`), so no rewrite is needed there. For other upstreams whose names differ, use the icskills name (e.g. `static-site`, not upstream's folder name) +- [ ] **Cross-references use icskills skill names** — for `caffeinelabs/skills` the upstream and icskills names match (`writing-motoko`, `migrating-motoko-actors`, `troubleshooting-motoko-migrations`, `reviewing-motoko`), so no rewrite is needed there. For other upstreams whose names differ, use the icskills name (e.g. `static-site`, not upstream's folder name) - [ ] **Experimental/removed features excluded** — If upstream removed a command or feature (e.g., `mops migrate new/freeze`), remove it from the skill - [ ] **Evals reviewed** — Open `evaluations/.json` and apply the same logic as any improvement: (1) add new eval cases for new pitfalls, new commands, changed defaults, or renamed APIs in the diff — these are exactly where agents will hallucinate without updated guidance; (2) always run every case you added or changed with baseline (`node scripts/evaluate-skills.js --eval `) and include those results in the PR — this verifies each new case passes with the skill and shows a real delta; (3) re-run untouched existing cases only if the diff modified content they cover. Collapse eval output in a `
` block. diff --git a/.claude/upstream.md b/.claude/upstream.md index 498cb20..d9237af 100644 --- a/.claude/upstream.md +++ b/.claude/upstream.md @@ -60,6 +60,24 @@ Upstream file paths and the tracking model (release-tag vs commit) are listed pe --- +## reviewing-motoko + +- **Upstream:** https://github.com/caffeinelabs/skills +- **Tracking model:** commit-based (no releases/tags). Same as `writing-motoko`. +- **Commit:** 90a2433578d0ea53fdbce3f51fb1dd3b8145ac04 +- **Upstream version:** 0.1.0 (skill frontmatter `version:`) +- **Last synced:** 2026-08-28 +- **Pin note:** added at `90a2433` while the other three `caffeinelabs/skills` entries were still pinned at `9274f9b` (the skill did not exist there). Pins realign on the next full `caffeinelabs/skills` sync — until then, a sync issue may re-show this skill's content as newly added; verify against the local file instead of re-applying. +- **Upstream files:** + - `skills/reviewing-motoko/SKILL.md` + - `skills/reviewing-motoko/references/state-and-persistence.md → references/state-and-persistence.md` + - `skills/reviewing-motoko/references/type-encoding.md → references/type-encoding.md` +- **icskills-owned sections (do not overwrite from upstream):** + - **Frontmatter (entire block):** same transform as `writing-motoko` (owned `description` tuned for repo-wide trigger evals, `license: Apache-2.0`, string `compatibility` (`moc >= 1.11.2, core >= 2.6.0, mops >= 3.0.0`), `metadata.title`/`category`). + - **`## Additional resources` → `## Additional References`** (section renamed), plus an extra `- **mops tooling**: Load \`mops-cli\` …` bullet not in upstream. + +--- + ## static-site - **Upstream:** https://github.com/dfinity/certified-assets diff --git a/evaluations/reviewing-motoko.json b/evaluations/reviewing-motoko.json new file mode 100644 index 0000000..9e23a7e --- /dev/null +++ b/evaluations/reviewing-motoko.json @@ -0,0 +1,52 @@ +{ + "skill": "reviewing-motoko", + "description": "Evaluations for the reviewing-motoko skill — design review of existing Motoko backends.", + "output_evals": [ + { + "name": "text-status-and-correlated-fields", + "prompt": "Design-review this Motoko stable type and report findings with a severity level each, as in a code review. Just the findings — no full rewrite, no deploy steps:\n\n```motoko\npublic type Task = {\n id : Nat;\n var status : Text; // \"open\", \"claimed\", or \"done\"\n hasDeadline : Bool;\n deadline : ?Time.Time;\n};\n```", + "expected_behaviors": [ + "Flags `status : Text` for a closed set of values as a defect and recommends a variant type (e.g. `{ #open; #claimed; #done }`) rather than runtime string comparison", + "Flags the correlated `hasDeadline : Bool` + `deadline : ?Time.Time` pair as encoding invalid states and recommends collapsing to a single field (the `?Time.Time` alone or a variant)", + "Notes that changing a type reachable from stable actor state requires a migration" + ] + }, + { + "name": "mixin-var-by-value", + "prompt": "Under Motoko enhanced migration my actor declares `var nextId : Nat;` (value supplied by the migration chain) and passes it to a mixin with `include TasksApi(nextId)`. The mixin increments it, but the actor's field never changes. Review this and give the finding plus the fix, briefly.", + "expected_behaviors": [ + "Identifies that `var` parameters are passed by value, so the mixin mutates a copy and the actor's field is never updated", + "Fix is to wrap the counter in a record (e.g. `let state : { var nextId : Nat }`) and pass that record binding to the mixin", + "Warns against constructing a fresh record literal at the include site (e.g. `include TasksApi({ var nextId = ... })`), which would again give the mixin its own copy" + ] + }, + { + "name": "stable-inline-initializer", + "prompt": "Code review, brief — one finding with severity plus the exact fix, nothing else: my Motoko project uses the enhanced migration chain (`[canisters.backend.migrations]` in mops.toml). In main.mo the actor declares this top-level field holding durable user data that must survive upgrades:\n\n```motoko\nlet tasks = Map.empty();\n```", + "expected_behaviors": [ + "Flags the inline initializer on a stable field as invalid under enhanced migration — stable fields are declared type-only (compiler reports M0250 / M0014)", + "The proposed fix is to declare the field type-only (`let tasks : Map.Map;`) and supply the initial value from the migration chain (the pending migration's NewActor)", + "The proposed fix keeps the field stable — it does not convert this durable user data to `transient`" + ] + } + ], + "trigger_evals": { + "description": "Queries to test whether the skill activates correctly. 'should_trigger' are review/audit/refactor tasks on existing Motoko code; 'should_not_trigger' are writing new code (writing-motoko), migration authoring (migrating-motoko-actors), tooling, or non-Motoko reviews.", + "should_trigger": [ + "Review my Motoko backend for design issues", + "Can you audit this Motoko canister code and report findings by severity?", + "Refactor this existing Motoko actor — the types feel sloppy", + "Do a code review of my .mo files before I merge", + "Is holding the task status as Text good design in my Motoko type?", + "My Motoko project is one monolithic main.mo — review the structure" + ], + "should_not_trigger": [ + "Write a Motoko canister with a counter", + "Fix this Motoko compilation error M0220", + "Write a migration to rename an actor field", + "How do I deploy my canister to mainnet?", + "Review my Rust canister for security issues", + "Run mops check on my project" + ] + } +} diff --git a/evaluations/writing-motoko.json b/evaluations/writing-motoko.json index 751d08a..9f13568 100644 --- a/evaluations/writing-motoko.json +++ b/evaluations/writing-motoko.json @@ -269,7 +269,8 @@ "Generate TypeScript bindings for my canister", "What is the cycles cost of a canister?", "How do I pin the moc version in mops.toml?", - "Run mops check on my project" + "Run mops check on my project", + "Review my existing Motoko backend for design issues and report findings by severity" ] } } diff --git a/scripts/sync-upstream-check.sh b/scripts/sync-upstream-check.sh index cf2e8a6..0b9bb01 100755 --- a/scripts/sync-upstream-check.sh +++ b/scripts/sync-upstream-check.sh @@ -36,11 +36,12 @@ fi # Local files live at skills// in this repo. case "$REPO" in caffeinelabs/skills) - # No releases/tags; tracked by commit. The three Motoko skills share one pinned commit. + # No releases/tags; tracked by commit. The four Motoko skills share one pinned commit. SKILLS_BASE_PATH="skills" SKILLS="writing-motoko:writing-motoko migrating-motoko-actors:migrating-motoko-actors -troubleshooting-motoko-migrations:troubleshooting-motoko-migrations" +troubleshooting-motoko-migrations:troubleshooting-motoko-migrations +reviewing-motoko:reviewing-motoko" ;; caffeinelabs/mops) SKILLS_BASE_PATH=".agents/skills" diff --git a/skills/reviewing-motoko/SKILL.md b/skills/reviewing-motoko/SKILL.md new file mode 100644 index 0000000..eeda986 --- /dev/null +++ b/skills/reviewing-motoko/SKILL.md @@ -0,0 +1,218 @@ +--- +name: reviewing-motoko +description: "Design review for existing Motoko backends — judging whether code is well designed, not how to write it. Covers invariants encoded in types (variants over Text, correlated fields, case _ smells), stable vs transient state discipline under enhanced migration, file structure (types.mo/lib/mixins/main.mo), and severity-ranked findings (Blocker/Warning/Note). Use when reviewing, auditing, critiquing, or refactoring existing .mo files, or when asked for code-review feedback on a Motoko canister. Do NOT use for writing new Motoko code or fixing compiler errors (writing-motoko), authoring migration files (migrating-motoko-actors), or deployment." +license: Apache-2.0 +compatibility: "moc >= 1.11.2, core >= 2.6.0, mops >= 3.0.0" +metadata: + title: Reviewing Motoko + category: Motoko +--- + +# Reviewing Motoko + +This skill judges whether Motoko code is **well designed**. It does not teach the language — load `writing-motoko` for syntax, `mo:core` APIs, and mechanical rules, and `migrating-motoko-actors` before changing anything that is stable state. + +Four questions decide a review, in this order — the first three because each can invalidate the next, the fourth last because it is the least consequential: + +1. **Is every invariant the code relies on encoded in a type?** Where the type system *can* rule out a bad state, leaving that to a runtime check or a comment is a defect, not a style preference. +2. **Is state declared with the right persistence?** Under enhanced migration, stable and transient are separate worlds. Blurring them produces state that silently stops updating. +3. **Does the code live in the right files?** `types.mo` / `lib/` / `mixins/` / `main.mo` is a contract about what may depend on what, not a folder convention. +4. **Does it read like Motoko?** Contextual dot notation, inferred implicits, no annotation the call already supplies, no classes. `mops check --fix` handles most of this; only what it cannot see is the reviewer's job. + +## Procedure + +**Run the mechanical pass first** — do not spend review attention on what the compiler already reports. + +```bash +mops check --fix +``` + +Its style auto-fixes (M0236 dot notation, M0237 redundant implicits, M0223 redundant instantiation) only fire where `[moc] args` enables `-W` for them; `writing-motoko`'s `references/project-setup.md` has the setup. Where they are off, the **Conventions** findings below are all yours to make by hand. + +Then read in this order — outside in, because a misplaced file makes every type inside it suspect: + +1. `mops.toml` — is there a `[canisters.*.migrations]` chain? If not, finding **B1** below outranks everything else. +2. **Structure** — the file layout, then each file's imports. +3. `types.mo` — every type declaration. This is where most findings are. +4. **State** — the actor fields in `main.mo`, and what each `include` passes. See [references/state-and-persistence.md](references/state-and-persistence.md). +5. **The API boundary** — every `public shared` / `public query` function: its parameter types, its return type, its authorization. +6. **Bodies** — last, and only for the smells under **Maintainability** below. + +## Severity + +Report findings with one of three levels. Do not inflate: a reviewer who calls everything a blocker gets ignored. + +| Level | Meaning | +| ------------ | --------------------------------------------------------------------------------------- | +| **Blocker** | Can corrupt or silently lose state, or lets a caller reach an unauthorized action. | +| **Warning** | An invariant the type system could enforce is left to runtime, a comment, or a convention. | +| **Note** | Readability, duplication, or naming. Fix while you are in the file; do not gate on it. | + +State the concrete failure, not the rule. "`status : Text` accepts `"complete"`, and the three call sites spell it `"completed"`" beats "prefer variants over strings". + +## Type encoding + +The rule: **if a value has a fixed set of shapes, the type must have that set of shapes.** Full recipes, including how to carry each change through a migration, are in [references/type-encoding.md](references/type-encoding.md). + +### T1. `Text` for a closed set of values — **Warning**, **Blocker** if it gates authorization + +The most common defect in generated Motoko. Every producer and consumer agrees on spelling by convention and the compiler checks nothing — no exhaustiveness, no error on a typo, no error when a new state is added and half the call sites are not updated. + +```motoko +public type Task = { id : Nat; var status : Text }; // WRONG +public type TaskStatus = { #open; #claimed : Principal; #done : { at : Time.Time } }; +``` + +Variants are shared, so this reaches the frontend as a discriminated union too. **Flag `Text` (or `Nat`, or `Bool`) whenever the code, a comment, or a doc lists the permitted values** — `== "..."` comparisons, a `switch` on a `Text` with a `case _`, role names held as `Text`. Keep `Text` for genuinely open sets, and for strings an external system owns. + +### T2. Correlated fields — **Warning** + +Two fields that are only valid together belong in one variant. A `Bool` plus an optional payload has four representable combinations and two meaningful ones. + +```motoko +// WRONG — { hasDeadline = true; deadline = null } compiles and means nothing +{ hasDeadline : Bool; deadline : ?Time.Time } + +// RIGHT +{ deadline : { #none; #due : Time.Time } } // or simply `deadline : ?Time.Time` +``` + +If a `?T` already says it, drop the `Bool` — do not introduce a variant that only restates `?T`. + +### T3. Wrong cardinality — **Note**, **Warning** when callers loop defensively + +Use the type that matches the real count. A `[T]` that is always empty-or-one makes every caller write a loop or an `if size == 1`; a `?T` tells them there is nothing to iterate. + +### T4. Errors as `Text` — **Warning** + +`Result` forces every caller to string-match to tell one failure from another. Make `Err` a variant with the data each case needs. See the `Result` section of `writing-motoko` for the full rule, including when to return `?T` and when to trap instead. + +### T5. `var` fields leaking into the API — **Blocker** (it does not compile) or **Warning** (it compiles but over-shares) + +A record with a `var` field is not shared, so it cannot be returned from a public function. Where a project already has the internal/view split (`Task` internal, `TaskView` shared), check that: + +- no public signature mentions the internal type; +- the view is **narrower** than the internal record, not a mechanical field-for-field copy. A view that re-exports an internal `Principal` list, an audit trail, or another user's data is an information leak that compiles. + +### T6. `case _` swallowing variants — **Warning** + +A catch-all arm is what turns "adding a variant tag is a compile error everywhere it matters" into "adding a variant tag silently takes the default branch". Keep `case _` only where the arms are genuinely uniform and the set is expected to grow (e.g. mapping many tags to one label). Anywhere a new tag would need new behaviour, enumerate the tags. + +### T7. Transparent aliases sold as safety — **Note** + +`type UserId = Principal` is a documentation alias: any `Principal` still type-checks where a `UserId` is expected. That is fine, and usually the right trade. Just do not treat it as an enforced distinction — and if two different id types are genuinely being confused at call sites, a one-field record (`{ userId : Principal }`) or a variant tag (`#user(Principal)`) is what actually enforces it, at the cost of changing the Candid shape. + +## State and persistence + +Under enhanced migration (EM) the actor's stable fields are declared **type-only** and every initial value comes from the migration chain. That separation is the point: it is what makes stable state impossible to initialize from something transient. Details in [references/state-and-persistence.md](references/state-and-persistence.md). + +### B1. No enhanced migration — **Blocker** + +No `[canisters..migrations]` chain in `mops.toml` means pre-EM persistence, where stable fields carry inline initializers — so a stable field can be initialized from a transient one: + +```motoko +transient let defaultQuota = 100; +let quota = defaultQuota; // stable; the initializer runs ONCE, on first install +``` + +Change `defaultQuota` to `250`, deploy, and `quota` is still `100` — forever, no warning. Point at `migrating-motoko-actors` for the conversion; everything below assumes EM. + +### S1. Inline initializer on a stable field — **Blocker** + +Under EM this is M0250 / M0014, so it will not compile — but it appears constantly in code copied from pre-EM examples and in half-finished refactors. The fix is never to add `transient`: move the initial value into the pending migration's `NewActor`. + +### S2. `transient` misused — **Warning** both ways + +Ask of every actor and mixin field: *if this is lost on upgrade, is the app still correct?* + +- **Missing `transient`**: caches, derived indexes, rate-limit counters, and anything rebuildable are being persisted. They then have to be migrated forever, and a stale cache survives the upgrade that was supposed to clear it. +- **Wrongly `transient`**: durable domain data silently resets to its initializer on every upgrade. Look for `transient` on anything a user would expect to still be there after a deploy. + +### S3. `var` actor field shared with a mixin — **Blocker** + +`var` parameters pass by value, so the mixin mutates a copy and the actor's field never changes. Wrap it in a record and pass the record. Also flag a fresh record literal at the `include` site (`include Api({ var n = state.n })`) — each mixin then gets its own copy. See `writing-motoko`. + +### S4. Stable state inside a `mixin` block — **Blocker** + +Every bare `let`/`var` at the top of a `mixin` is implicitly stable and traps at runtime with `IC0503`. State comes in as a parameter; constants go in a module; only `transient` is allowed inline. + +### S5. Migration hygiene — **Blocker** + +- A migration file importing anything other than `mo:core/...` — the chain replays forever, so a project import makes it wrong the moment that type changes. (`caffeineai-lints` catches this.) +- More than one pending migration in a build, or an edit to a migration that predates this build. +- A stable field in `main.mo` that no migration in the chain supplies (M0254 / M0267). + +An identity migration body is a **Warning** — the change was stable-compatible, so delete the file. [Full table](references/state-and-persistence.md#migration-hygiene). + +## Structure + +Layers, with dependencies pointing one way only: `main.mo` → `mixins/` → `lib/` → `types.mo`. + +| File | Holds | Must not hold | +| --------------- | ----------------------------------------------------- | -------------------------------------- | +| `types.mo` | Type declarations; small pure helpers on those types | State, endpoints, business rules | +| `lib/*.mo` | Domain logic as stateless modules, state as parameters | Public endpoints, actor fields | +| `mixins/*.mo` | Public endpoints: authorize, delegate, map to a view | Domain logic, stable state | +| `main.mo` | State declarations and `include`s | **Any public method**, any logic | +| `migrations/` | The frozen chain, one module per file | Project imports | + +- **A1 — Blocker/Warning:** a public method in `main.mo`. Move it to a mixin. +- **A2 — Warning:** a monolithic file. One `.mo` holding types, state, logic, and endpoints together. +- **A3 — Warning:** domain logic in a mixin body. If an endpoint's body is more than authorize → delegate → map, the middle belongs in `lib/`. +- **A4 — Warning:** wrong dependency direction — `lib/` importing a mixin, or `types.mo` importing either. +- **A5 — Note:** a mixin that is not a feature. Mixins split by *feature* (`mixins/Bookings.mo`), not by verb (`mixins/Getters.mo`). +- **A6 — Warning:** over-broad state injection. A mixin receiving state it never touches widens what a change can break; pass only the slices it uses. + +## Maintainability + +Most of this is caught in the last, bodies pass. The exception is **M1**, the authorization blocker — it is found during the API-boundary pass (step 5), not with the body smells below. + +- **M1 — Blocker:** a `public shared` function that does not check `caller`. Every state change and every read of another user's data needs authorization on the backend. +- **M2 — Warning:** `Nat` subtraction that is not provably non-negative at the operation. It traps. +- **M3 — Warning:** a trap where the caller could have recovered. Traps roll back the message and reach the client as an opaque reject; caller-fixable failures belong in a `Result` error variant. +- **M4 — Note:** duplicated internal→view mapping. One `toView` per type, in `lib/` or `types.mo`. +- **M5 — Note:** magic values — bare literals for limits, quotas, or durations. Name them in a module. +- **M6 — Note:** a comment asserting an invariant the type could carry. Every "must be non-empty" / "only set when …" comment is a type-encoding finding in disguise. Where Motoko cannot express the invariant, keep the comment but validate the value once at the boundary rather than re-checking it at every use. + +## Conventions + +Least consequential of the four axes, and `mops check --fix` fixes most of it where the `-W` flags are on — so raise these only after the passes above, and only where the tooling cannot. `writing-motoko` has the rules; this is what to look for. + +- **C1 — Note:** a module-function call where the function takes `self` — `List.add(list, x)`, `Principal.toText(p)`. M0236, auto-fixed. +- **C2 — Note:** an implicit passed explicitly (`map.add(Text.compare, k, v)`), or a type instantiation inference already resolved. M0237 / M0223, auto-fixed. +- **C3 — Warning:** type annotations on an inline `func` passed as a **call argument** — `xs.filter(func(x : Nat) : Bool { x > 1 })`. No diagnostic catches this, so it is the reviewer's job: the call already fixes both types, and the annotation duplicates them so they can drift. Write `xs.filter(func x = x > 1)`. Instantiate the *call* (`map`) when M0098 demands it — never the lambda. One exception: `: async ()` on an async callback is load-bearing. +- **C4 — Blocker:** a `class`, or any object holding functions, used as actor state. Functions are not stable, so it fails with M0131 (`declared stable but has non-stable type`). Outside state a class still is not the idiom here — a module taking `self` plus dot notation gets the same call sites and stays stable. + +## Refactoring + +Design fixes in Motoko are not free: **changing a type that is reachable from a stable actor field requires a migration**, and the chain is one-way. Before proposing a type change, establish whether that type is stable state. + +1. `mops check --fix` first, so the mechanical noise is gone. +2. **Batch the stable-state type changes** — one pending migration per build, so they land together. +3. **Write the migration** (`migrating-motoko-actors`). For `Text` → variant, decide explicitly what an unrecognized string becomes rather than inventing an `#unknown` tag. +4. **Move code before changing it** — relocating, then editing, keeps each diff reviewable. +5. `mops check --fix` until clean, then `mops build` once. + +## Reporting + +Group findings by severity, blockers first. Per finding: `file:line`, the level, the concrete failure it permits, and the change. Suppress findings the compiler already reports. + +```text +Blocker src/backend/mixins/Tasks.mo:34 (M1) + `claimTask` never reads `caller` — any principal can claim any task. + +Warning src/backend/types.mo:12 (T1) + `Task.status : Text` is a closed set, so the comparison at Tasks.mo:51 + spells it "complete" and is never true. Stable state — needs a migration. +``` + +If nothing is wrong, say so plainly. Do not manufacture findings to fill a report. + +## Additional References + +- [references/type-encoding.md](references/type-encoding.md) — recipes per invariant, and the migration each one needs +- [references/state-and-persistence.md](references/state-and-persistence.md) — why EM matters, the stable/transient question, migration hygiene +- `writing-motoko` — language mechanics, `mo:core` APIs, `Result`, dot notation +- **mops tooling**: Load `mops-cli` for `mops.toml` configuration and `mops check`/`mops build` details +- `migrating-motoko-actors` — writing the migration a type change requires +- `troubleshooting-motoko-migrations` — compatibility diagnostics that do not match the source diff --git a/skills/reviewing-motoko/references/state-and-persistence.md b/skills/reviewing-motoko/references/state-and-persistence.md new file mode 100644 index 0000000..5f3868c --- /dev/null +++ b/skills/reviewing-motoko/references/state-and-persistence.md @@ -0,0 +1,58 @@ +# Reviewing state and persistence + +Under enhanced migration (EM), every top-level `let`/`var` in an actor or mixin is **stable** unless prefixed `transient`, and stable fields are declared **type-only** — their values come from the migration chain. + +```motoko +actor { + let tasks : Map.Map; // stable; value from the chain + let state : { var nextId : Nat }; // stable; mutable counter in a record + transient var cache = Map.empty(); // rebuilt each start + include TasksApi(tasks, state); +} +``` + +## Why EM is not optional + +Pre-EM, stable fields carry inline initializers, so a stable field can be initialized from a transient one: + +```motoko +transient let defaultQuota = 100; +let quota = defaultQuota; // stable +``` + +The initializer runs **once, on first install**. Change `defaultQuota` to `250`, deploy, and `quota` is still `100` — forever, no warning, in code that reads as though the two are linked. The same shape appears with `Time.now()`, a config record, a computed limit: the transient side keeps moving and the stable side is frozen at whatever the first install saw. + +EM makes it unwritable. A stable field has no initializer, so the only way to set or change one is a migration — explicit, reviewed, applied exactly once. + +## The stable/transient question + +For every actor and mixin field: **if this vanished on upgrade, would the app still be correct?** + +- **Yes** → `transient`. Caches, derived indexes, memoized views, rate-limit windows, capability handles. Persisting them means migrating them forever and carrying stale entries across the upgrade meant to clear them. +- **No** → stable (no keyword). Anything a user expects to survive a deploy. + +Both directions are findings. Constants are the third case: a bare `let X = ...` in an actor or mixin is stable state, so a static value (literals, tuples, immutable records, function values) belongs in a **module**, and a non-static one — anything involving a call, operator, or control flow — has to be `transient let`. + +## Three ways mixin state silently decouples + +`var` parameters pass **by value**; records pass by reference. Each of these compiles, and none of the writes reach the actor: + +- `var nextId : Nat;` handed to a mixin — wrap it as `let state : { var nextId : Nat }` and pass the record. +- A record literal at the `include` site: `include TasksApi({ var nextId = state.nextId })` builds a fresh record. Pass the binding. +- A helper that reshapes the record: if the actor declares `{ var nextId : Nat }`, helpers must accept `{ var nextId : Nat }` and write `state.nextId`. Copying into `{ var counter = ... }` mutates only the copy. + +Where two mixins need the same state, declare it once and pass the same binding to both. + +A bare `let`/`var` at the top of a `mixin` is implicitly stable and traps at runtime with `IC0503`. It passes compilation and fails on the deployed canister, so it is always a blocker. + +## Migration hygiene + +| Finding | Level | Why | +| -------------------------------------------------------------- | ------- | ----------------------------------------------------------------------- | +| An import other than `mo:core/...` | Blocker | The chain replays forever; a project type that later changes breaks it. Caught by `caffeineai-lints`. | +| Two pending migrations in one build | Blocker | `check-limit = 1`. Fold the second into the first. | +| An edit to a migration that predates this build | Blocker | Applied migrations are tracked by module name; the edit never runs. | +| A stable field in `main.mo` no migration supplies | Blocker | Surfaces as M0254 / M0267 and breaks later upgrades. | +| A feature-shaped filename (`AddPriority.mo`) | Warning | Invites a second file per change instead of editing the pending one. Use `YYYYMMDD_HHMMSS.mo`. | +| An identity migration body | Warning | The change was stable-compatible; delete the file. | +| A magic constant in the body | Warning | Derive defaults from the migration input or a domain default. | diff --git a/skills/reviewing-motoko/references/type-encoding.md b/skills/reviewing-motoko/references/type-encoding.md new file mode 100644 index 0000000..28eb43d --- /dev/null +++ b/skills/reviewing-motoko/references/type-encoding.md @@ -0,0 +1,91 @@ +# Encoding invariants in Motoko types + +Motoko has variants, options, records, and generics — no refinement types, no non-empty list, no zero-cost newtype. So the goal is not "no invalid value can exist", it is **no invalid value can be written by accident**. Variants and options buy that almost everywhere. + +## The cost + +Everything reachable from a stable actor field is persisted, so changing such a type needs a migration, and the chain is one-way. Two consequences: get types right early, and batch the fixes (`check-limit = 1` allows one pending migration per build). Before proposing a type change, check whether the type is stable-reachable — if it is, the proposal includes the migration. + +## T1. Closed set held as `Text` + +**Spot it:** permitted values listed in a comment, compared with `==` against literals, or `switch`ed with a `case _` fallback. Statuses, roles, kinds, tiers, categories. + +`{ var status : Text }` fails three ways, none a compile error: a capitalisation mismatch, a comparison against a spelling that is never produced, and a new state falling silently into `case _`. + +Encode it as a variant, and move data that only exists in one state *inside* that state — so there is no `claimedBy : ?Principal` that can be `null` while the status says claimed: + +```motoko +public type TaskStatus = { #open; #claimed : Principal; #done : { at : Time.Time } }; +``` + +**Keep `Text`** when the set is genuinely open (display names, notes, search terms) or an external system owns the string. In the second case parse it into a variant once at the boundary and never `switch` on the raw string again. + +**Migration.** Map each known string to a tag. The value of writing it out is that it forces the decision about unrecognized strings; make that decision rather than adding an `#unknown` tag nobody removes. + +```motoko project=text-to-variant filepath=src/backend/migrations/20260101_000000.mo +import Map "mo:core/Map"; +import Nat "mo:core/Nat"; + +module { + type OldTask = { id : Nat; var status : Text }; + + // Payload-free: a raw Text source cannot populate a Principal or Time + // payload, so this step only lifts the tags. The payloaded form above + // is the end state, reached only when the source data carries the value. + type NewStatus = { #open; #claimed; #done }; + type NewTask = { id : Nat; var status : NewStatus }; + + type OldActor = { tasks : Map.Map }; + type NewActor = { tasks : Map.Map }; + + public func migration(old : OldActor) : NewActor { + let tasks = old.tasks.map( + func(_id, task) { + { + id = task.id; + // Unrecognized strings become #open — still actionable, and no record + // is silently dropped. Decide this explicitly per domain. + var status = switch (task.status) { + case ("claimed") { #claimed }; + case ("done") { #done }; + case _ { #open }; + }; + }; + } + ); + { tasks }; + }; +}; +``` + +## T2. Correlated fields + +A `Bool` beside an optional payload has four representable combinations and two meaningful ones. `{ isPaid : Bool; paidAt : ?Time.Time }` becomes `{ payment : { #unpaid; #paid : Time.Time } }`. + +Do not over-apply: where a plain `?T` already carries the invariant, use `?T`. The recipe is for two or more correlated fields, or when a tag needs a name an option cannot give it. + +## T3. Cardinality + +Exactly one → `T`. None or one → `?T`. Any number → `[T]`. A `[T]` that is always empty-or-one makes every caller loop or check `size() == 1`. + +Motoko has no non-empty array. `(T, [T])` encodes "at least one" at a real readability cost — use it only where an empty collection is a bug callers keep guarding against. + +## T4. Error types + +`Result` is T1 wearing a different hat: every caller, frontend included, has to string-match to tell failures apart. Make `Err` a variant whose tags carry what a caller needs to react. A `Text` payload *inside* a tag is fine as a human-readable message, never as the discriminator. + +See the `Result` section of `writing-motoko` for choosing between `Result`, `?T`, and a trap. + +## T5. Internal vs shared views + +Internal records carry `var` fields and `Map`/`Set`/`List` values, so they cannot cross a public signature and projects keep a second shared type per entity. + +Review the **narrowing**, not just that the split exists. A view mirroring every internal field is a leak that compiles: flag re-exported principal lists, audit trails, internal flags, and other users' data. Keep one `toView` per type — duplicated mappers drift. + +## T6. `case _` on variants + +A catch-all converts "adding a tag breaks every site that needs updating" into "adding a tag silently does the wrong thing", which is most of the value of the variant. Remove it where a new tag would need new behaviour; keep it where the arms are genuinely uniform and the set is expected to grow. + +## T7. Identifier types + +`type UserId = Principal` is a transparent alias — any `Principal` type-checks in its place. It documents intent and costs nothing, so do not flag it by default. Flag it only when two id types are actually being crossed at call sites; a one-field record or a variant tag enforces the distinction, at the cost of the Candid shape and a migration if the id is stable state. diff --git a/skills/writing-motoko/SKILL.md b/skills/writing-motoko/SKILL.md index 33f628d..8df0275 100644 --- a/skills/writing-motoko/SKILL.md +++ b/skills/writing-motoko/SKILL.md @@ -1,6 +1,6 @@ --- name: writing-motoko -description: "Motoko language pitfalls, modern syntax, and architecture patterns for the Internet Computer. Covers persistent actors, stable types, mo:core standard library, dot notation, mixins, and common compilation errors. Use when writing Motoko canister code, fixing Motoko compiler errors, or generating Motoko actors. Do NOT use for deployment, icp.yaml, or CLI commands." +description: "Motoko language pitfalls, modern syntax, and architecture patterns for the Internet Computer. Covers persistent actors, stable types, mo:core standard library, dot notation, mixins, and common compilation errors. Use when writing Motoko canister code, fixing Motoko compiler errors, or generating Motoko actors. Do NOT use for deployment, icp.yaml, or CLI commands; for design review or audit of existing Motoko code, load reviewing-motoko instead." license: Apache-2.0 compatibility: "moc >= 1.11.2, core >= 2.5.0, mops >= 3.0.0" metadata: