Cf Review

Common Fabric Code Review

This skill is a map and a statement of values, not a recipe. It assumes you already know how to read and review code; it tells you the things about this repo you cannot derive — where its value and its footguns are, and how we like review done.

All paths are relative to the repo root.

What we value in a review

• Changeset-scoped. Review the diff and its immediate effects — for a large change, its blast radius — not a repo-wide audit. Focus on the changeset; keep it fast.
• Loud, with intention. Flag bugs, regressions, and broken principles unmistakably; let nits stay quiet. We ship many PRs — aim for sound, not pristine.
• Report-first, then offer to post. Always show the report, then offer to post it — signed as yourself (the agent and model, on the human's behalf). Skip posting when it isn't worth it.
• An aid to thinking. Help people decide with eyes open: surface what matters — especially a spec or principle a change violates — clearly enough to act on. The human owns the merge.

The north star for coherence

When someone — human or LLM — searches for the words they've heard and lands on a doc, comment, or example, they must not be misled by something that no longer matches how the runtime works.

That is the bar — not uniform vocabulary. Wording drift is a slow Boy-Scout fix: nudge what you touch, propose big sweeps as follow-ups. When a change leaves an authoritative source actively wrong — a normative spec especially — surface it loudly; that it is known matters more than which bucket it lands in.

Two kinds of change — know which you're reviewing

This repo holds both the platform (the runtime and packages that execute programs — ordinary, untransformed TypeScript) and patterns (the programs themselves, the semantic equivalent of apps). They review differently:

• Patterns are authored in TypeScript, then transformed by an extensive pipeline of custom transformers into a much more complex form that satisfies the framework's structure, so the pattern can build a reactive graph evaluated under live subscriptions. The source is far from what runs, so a change — to a pattern or to the runtime code that executes it — can span many abstraction levels with implications that aren't obvious in isolation.
• Platform code (most of packages/**) is plain TypeScript: review it as you would any TS codebase, plus this repo's conventions and the anti-duplication map below.

Two failure modes recur when moving fast without the whole picture:

1. Fighting the framework (patterns, and the runtime that serves them) — when the idiom is hard to find, code reaches for try/catch, singletons, manual subscriptions, async/await in handlers, or a transformer escape hatch. (Dimension 4.)
2. Re-forking core machinery (platform and patterns alike) — re-deriving hashing, serialization, cloning, or identity instead of reusing the one canonical home. The subtle, expensive footguns. (Dimension 3.)

Step 1 — Establish scope, then pick your depth

Auto-detect the target: no argument → the local branch vs main (BASE=$(git merge-base HEAD origin/main), then read git diff "$BASE" and git log "$BASE"..HEAD for intent); a number → that PR (gh pr view <N>, gh pr diff <N>). Say in a couple of lines what the change does and why, and whether its scope is coherent or has leaked. Unclear motivation is a question for the author, not a defect.

Then pick depth from the diff size — reading a large change set file-by-file burns context and loses the forest:

• Detail mode (small/medium — a package or two): read the changed code.
• Scope-and-theory mode (large — many files, several packages): build a top-down theory of intent and blast radius rather than reading every file. Deep-read only the load-bearing files (core abstraction, API / signature changes, hashing / serialization / cloning / identity), and fan out read-only subagents to search the rest — callers the diff missed, docs / examples for touched concepts, prior art it may be forking. Synthesize into one report, test the change against that theory, and state your coverage (deep-read / sampled / delegated).

Step 2 — Where to look

Order findings by impact. Each dimension is a generative principle plus seed tells — examples to prime you, not a checklist to stop at. The space is larger than any list here.

1. Correctness & regressions — the loudest category

A confirmed bug or regression is Blocking. Watch especially for:

• Tests bent to fit a regression — an existing test weakened or retrofitted so new (possibly wrong) behavior passes. A test edited to assert a bug is Blocking.
• Reactivity hazards (patterns) — reactive self-feedback (a cf-* control bound via $value / $checked whose handler writes the same cell back), .get() on computed / lift results, new Writable(reactiveValue). Defer to docs/common/ai/pattern-critique-guide.md for the full ruleset; cite it.
• Determinism (patterns) — Date.now() / Math.random() / authored timers in a lift, computed, or pattern body. These built-ins are gated by the sandbox: allowed only inside a handler (the clock coarsened to one-second resolution) and throwing a TimeCapabilityError elsewhere. Reading the clock reactively in a computed is the interval #now/N wish — flag bare #now used as a clock: it durably freezes at the piece's first-ever load. A read that survives into a re-running computation is a bug regardless.
• Time-based waits (any code) — sleep / setTimeout used to "wait for" a result instead of awaiting the actual event or signal. Almost never justified (animations aside) and a prime source of CI flakiness: what takes X today takes 10X under load someday.

2. Coherence ripple

Apply the north-star test, bounded to what the diff touches. If the change moves runtime semantics — a primitive's behavior, a transformer rule, a public signature, the meaning of a core concept — find what still describes the old behavior (docs under docs/common/, example patterns and the catalog, JSDoc on the changed symbols, package READMEs, normative specs) and flag it. A change that violates a documented invariant (transformer target-language boundary, module-graph cleanliness, "make invalid states unrepresentable") is a loud, Blocking finding even if the code "works." Fix what the diff touches; propose wider sweeps as follow-ups — don't boil the ocean.

For cf-harness runtime-semantics changes, include the system map, which reads as authoritative but is never a source of truth, in that coherence sweep and follow its update procedure in packages/cf-harness/docs/system-map/README.md.

docs/history/ is exempt from the coherence sweep: those are frozen point-in-time records and are supposed to describe old behavior. The reverse check applies instead — a diff that edits the content of a docs/history/ document (anything beyond its metadata header or a mechanical link fix) is a finding; see docs/history/README.md. Also flag a diff that completes a plan or supersedes a design without archiving the now-historical document (docs/README.md, "Moving a document from live to historical").

3. Anti-duplication

We move fast; under that pressure core machinery gets silently re-forked, and in a few domains that is brutal to debug because divergence stays invisible until it isn't. Re-implementing one of these is Blocking; reuse the canonical home. Point to the module rather than trusting a symbol name remembered here — names drift, so confirm against the package's actual exports when you rely on it (this table is the highest-value and highest-rot content in the skill):

hash, serialize, and clone are the three we re-fork most — the tree already carries several SHA-256s (e.g. content-hash vs packages/toolshed/lib/sha2.ts vs inline crypto.subtle.digest), so steer new code to the canonical home rather than cargo-culting a neighbor. A new such definition isn't automatically wrong, but the PR must justify why the canonical home doesn't fit; a silent fork is Blocking.

4. Framework-fit

Spot code fighting the framework instead of using it, and give two outputs: the fix toward the idiom, and — when the confusion was avoidable — the doc or structure gap that caused it (a follow-up; that is how the mistake stops recurring). Tells (seeds, drawn from docs/development/DEVELOPMENT.md):

• over-eager try/catch that swallows errors a caller should see (throwing is fine; fatal errors should propagate);
• new singletons / module-global mutable state (breaks multiple instances + tests);
• ambiguous any (or abuse of unknown) away from a serialization boundary; types that admit invalid intermediate states;
• working around the transformer (manual graph wiring the transformers would do, imperative escapes from the target language);
• async escapes in patterns (async/await in handlers; awaiting generateText / generateObject instead of using .result; new Stream() / .subscribe()).

When a transformer behavior is in doubt, read the emitted output before reasoning from source: deno task cf check <file>.tsx --show-transformed [--no-run]. If the transformer is the real culprit, the fix is to diagnose it and file a transformer bug with a repro — not to hand-build a workaround.

5. Changeset hygiene

Stray cruft is trivial to fix but shouldn't merge: leftover debug logging / *.log / scratch notes, commented-out code, .only on tests, "temp" / "HACK" stopgaps, abandoned TODOs. A new workspace package must register in the root deno.jsonc and carry its own tasks.test, in the manifest Deno resolves — a deno.json shadows a deno.jsonc beside it. Without one the root runner refuses to start and names the member. Run deno fmt and deno lint on touched files.

A doc comment detached from its declaration belongs here too, and a diff is the one place it is easy to see. A definition added directly under an existing doc comment takes that comment over and leaves the declaration it was written for undocumented; a // note wedged in between splits the contract from its subject, unless it is a label picking out one member of an overload set, which is allowed. Two doc comments in a row are the loudest tell — only the nearer one survives into rendered documentation. See docs/development/code-comment-style.md, "Where one goes".

A documented declaration with nothing blank after it is that same defect read downward, and it is quieter, because the comment is still next to the thing it was written for. The comment reaches on past it, so the members added under it look documented when they are not — which is exactly the shape a diff appending one member to a run of them puts in front of you. See the same document, "The blank line below". An overload set counts as one declaration here, so the blank line falls after the implementation — or, for a set with none to close it, after its last signature — and never between the signatures.

A missing or misplaced file header is that same defect one level up, and is equally a thing a diff shows you. A file header is a doc comment at the very top, above the first import and the first export; a new file carrying none, or carrying one written as a // block or parked below the import block, is a finding nothing else in the file will point you at. See the same document, "File headers", for the four kinds of file that carry none by design — a unit test file among them, since a header that would only name the file under test is left out — before writing the finding.

6. Craft & conventions

Mostly Improvement / Nit — don't drown the report in these. Dead code, unused exports, superfluous abstraction, unclear names. Types: no needless any; no needless casts (an as Something that isn't required for correctness) and no unjustified as unknown as Something, especially one erasing Immutable<T> / Readonly<T>.

For the rest the authority is docs/development/DEVELOPMENT.md, § Style & Conventions: follow it and point to it rather than relisting it. Its subsections seed what to look for — e.g. named exports, JSDoc on exports and public members, import grouping and collation, @commonfabric/api xor /interface, module-graph hygiene. Two of them seed nothing any gate will ever raise, which is where a diff-reading reviewer earns their keep: § Classes — #privateName over TypeScript's private, a class exposing no enumerable properties, and the member order; the wrong form compiles and type-checks clean, and an erased private leaves an own enumerable property behind — and § Word choice, American spelling and one word per concept, which reaches comments and error and log messages as much as it reaches documents.

skills/writing-code/SKILL.md is what an author is handed for this same material. A finding here that it does not cover is a gap in the skill; say so.

7. Test rigor — the special-attention area

We modify tests constantly but often can't say why a given test exists. For each touched or added test, name the principle it guards; if you can't, that's a finding (likely incidental, or testing an implementation detail). A good test exposes the intended behavior and reads like a sentence — a user story, not an assertion on internals; prefer BDD style (@std/testing/bdd describe / it that read as sentences) and expect() over assert*() — a direction we apply inconsistently today, so nudge toward it rather than treat it as a gate. Check the cases cover the actual semantic change and its edge / empty / error states, and that a removed test dropped dead coverage, not real coverage. Follow the repo's testing conventions (docs/common/ai/pattern-testing-guide.md); run targeted tests while reviewing.

Step 3 — Verify, don't speculate

Run only what confirms a finding (CI already keeps main green, so you're checking the delta): deno task check (types), deno lint, deno test <file> (targeted; whole suite is deno task test), deno task integration <pkg>, and --show-transformed for transformer questions. Tag each finding verified (you ran it / read the line) or suspected (needs author confirmation). Never present a guess as a bug.

Step 4 — Rank, report, offer to post

Use the repo's severity taxonomy (docs/common/ai/pattern-critique-guide.md: critical / major / minor / info). The report's only required shape:

• a one-line header — scope · coverage (deep-read / sampled / delegated) · checks run;
• 🔴 Blocking (resolve or consciously accept before merge), 🟡 Improvements (non-blocking), ⚪ Nits (optional; omit if Blocking exists) — each finding gives location · what · why it matters · concrete fix · verified/suspected;
• Questions for the author only if motivation or scope is unclear;
• Possible follow-ups as proposals, for a human to file;
• a one-line verdict.

Let the findings drive the format — don't pad to fill a template. Lead with the worst thing; if the change is clean, say so in two lines and stop.

Then offer to post (after showing the report), signed as yourself: a body opening with self-attribution — e.g. "cf-review via Claude <model>, on behalf of @<handle>" (use the human's real GitHub handle from gh / the PR context; omit the @ if unsure) — via gh pr review --comment, or inline comments through the reviews API for Blocking / Improvements with Nits left in the summary. Skip posting when it isn't worth it.

Anchor every finding to the context it responds to: a threaded reply (gh api .../pulls/<n>/comments/<id>/replies) when answering an existing thread, otherwise an inline comment on the file and line it addresses — gh api repos/<owner>/<repo>/pulls/<n>/comments -F body=@file -f commit_id=<head-sha> -f path=<file> -F line=<n> -f side=RIGHT. A top-level gh pr comment detaches the argument from what it argues about; reviewers read threads in-diff, and a floating comment is easy to miss and hard to answer in context. Find anchor lines by grepping the PR-head version of the file.

Canonical references

• Skill-authoring philosophy (why this skill is shaped this way): docs/development/skill-authoring.md
• Pattern rules + severity taxonomy: docs/common/ai/pattern-critique-guide.md
• Design principles & idioms: docs/development/DEVELOPMENT.md
• What an author is handed for the same conventions: skills/writing-code/SKILL.md
• Comment style, // and JSDoc alike, file headers included: docs/development/code-comment-style.md
• Transformer semantics: docs/specs/ts-transformer/README.md
• Reactivity model: docs/common/concepts/reactivity.md
• Debugging & gotchas: docs/development/debugging/README.md