jj-preflight

name

jj-preflight

description

Pre-flight design review through JJ Philipp's lenses, applied before opening a Moov PR or while shaping a ticket. Use when about to open a PR on a moovfinancial repo (moov-money, events, transfers, card-gateway, notifications, frontend, infra, etc.), shaping a Linear ticket before coding, or self-reviewing a diff for design issues that would draw red-line review comments. Applies six durable principles — vocabulary-as-contract, resist invented machinery, events-as-spine, trust-as-verified-identity, smallest-viable-contract, reason-by-consequence — and outputs short lowercase questions in JJ's voice, not a checklist.

jj-preflight

A two-mode review skill that applies JJ Philipp's design taste as lenses, not patterns. The point is to catch what JJ would red-line before he sees it, so reviews stay at the nit level.

The six principles below were extracted from JJ's PR comments, Slack threads, and review history. They are the generators behind every specific comment he leaves — durable, not recency-biased.

How to invoke

• jj-preflight ticket <linear-id-or-description> — before coding starts
• jj-preflight diff (default if no args) — before pushing the PR; runs against git diff main

The six principles

Apply each as a lens. For each principle, ask: where does it bite on this work? Quote evidence from the change. If a principle clearly doesn't apply, skip it — don't pad.

P1 — Vocabulary is contract

Names aren't style; they're the type system at the domain level. If the name lies, the API lies. Renames are the single most common red-line.

Anchors:

• "call it a code, not token."
• "TOTP. The first T is for time."
• "is claim the flow? the web app?"
• "seems like a rename going in the wrong direction lol"
• "Can we rename it to recipientflow as we use the noun recipient to mean the person filling it out"

Lens questions:

• For every domain noun in the change, does it match the canonical term used in adjacent code and in the URL/contract?
• Does the name describe what the thing is in the domain, or just what shape it has?
• If a similar concept already has a name, are we reusing it?
• Does the rename go in the direction the rest of the domain is moving?

P2 — Resist invented machinery

Default to the aggregate that already exists. A new repo, table, model, status flag, ID, or validation layer must justify itself against what's already there. Mechanism without a concrete failure to prevent gets deleted.

Anchors:

• "Why do we need a repository when it can just be an event?"
• "Do we need another repo? It's just one row on the payout record."
• "the accepted status is kind of pointless as they can click through that any number of times, and really just there for analytics"
• "only need [idempotency] if sending the same request twice doesn't produce the same result"
• "I don't understand why it added all of this"

Lens questions:

• Is this new thing a column on an existing aggregate?
• What concrete failure does this mechanism prevent? If you can't name one, cut it.
• Is the status flag carrying real semantics, or just bookkeeping?
• Are we re-implementing something the framework or an existing helper already does?

P3 — Events are the spine; tables are projections

State changes are events first and rows second. If you can't name the event with an actor and a payload, you don't yet understand the change. The audit/legal answer to "did the user agree?" is the event log, not a status column.

Anchors:

• "Wheres the event produced?"
• "we honestly just need the event as the record of hitting the accept button"
• "incase someone says I didn't approve it, we can look up the event and say yes, yes you did from this ip address"
• "RecipientSessionStarted [not PageViewed]"
• "you can replace all but the payout_id with an actor"
• "next even after totp we'll want an event with how they were authenticated"

Lens questions:

• What event fires on this state change? What actor? What payload?
• Does the event make the table or status flag redundant?
• Is the event name a noun-past-tense describing a domain fact, or an activity/page-tracking string?
• Is the actor right, or are we stuffing redundant fields into the payload that the event header already carries?

P4 — Trust is a property of verified identity, propagated explicitly

Verified-vs-unverified is a hard boundary. Session context is plumbed continuously across the funnel, never re-invented per endpoint. Secrets have abuse surfaces measured in dollars, not policy.

Anchors:

• "but we don't know its them for real"
• "how legally binding is agreeing to allowing automated txts calls to a number where we haven't verified the number"
• "starting a new JWT session that we will upgrade as they move more into the app. We need to start a session with a sessionID we need to carry through the application for sardine and others checks"
• "lack of rate limiting which could leak the secret that's generating the code and cost us money"
• "Just would prefer it be a cookie thing as it keeps the token a bit more secure"
• "fine for now but we will want to limit that... don't want to leak too much info until after they've been verified"

Lens questions:

• Is the caller verified or unverified? What does the endpoint do differently based on that?
• Is a sessionID minted and propagated, so Sardine/fraud signals stay continuous?
• If this endpoint emits a secret (SMS, email, OTP, code), where's the rate-limit story?
• Does this PR leak info about a recipient/user before they're verified?
• Is a sensitive token returned in a body when a cookie would do?
• For consent/agreement flows: is the agreeing identity verified, or could someone else be agreeing on their behalf?

P5 — Ship the smallest viable contract; evolve in versions

v1 is deliberately undercooked with named deferrals. Don't gold-plate. The internal product (moov.money) consumes the public API like any customer — no internal shortcuts.

Anchors:

• "fine for now but we will want to limit that"
• "I would just add a blank for now"
• "I think later on we'll just add the old token to an argument on the /token call before I release docs for others to use it"
• "moov.money like any of our customers"
• "the payouts endpoints go on moov.money and are not part of the fake FI"
• "Its not ideal but I'll make it work to get something shown … we'll fix it in post"

Lens questions:

• Is there a smaller version we can ship now with a named follow-up?
• Are we gold-plating fields/options that v1 won't exercise?
• Does this belong on the public moov.money surface, or did we invent an internal route?
• If we're versioning a contract, is there a transformer/backport path?
• Have we explicitly named what's deferred, so it doesn't get lost?

P6 — Reason by consequence, not by rule

This is the style in which the other five get applied. When pushing back, cite the failure mode or a past incident. When advocating, propose the smaller alternative. Be willing to be wrong publicly.

Anchors:

• "issues we had once with checkouts" (cited as why recipient-ui shouldn't share a pod)
• "if someone else got the code, they could be agreeing for a number they don't control"
• "if they have the client_secret they already have a full surface... so..."
• "oh duh … sorry" / "I get it now" / "yeah its fine heh"

How to apply (this governs the skill's output voice):

• Frame findings as short lowercase questions, not declarations.
• Cite the consequence ("could cost us money", "leaks info pre-verification", "creates two transfers"), not the principle name.
• Hedge soft: "wonder if…", "isn't this…?", "am I missing something?"
• Stay willing to be wrong. The author may have a reason you don't see.

---

Mode A — ticket

Input: a Linear ticket ID (use mcp__linear-server__get_issue) or a short freeform description.

1. Read the ticket. Pull out:

   • User-visible nouns
   • State change(s) being introduced
   • New endpoints, new tables/repos, new event types
   • Trust boundary: who calls this and are they verified?
   • Anything labeled "track", "log", "status" that might secretly be an event

2. For each principle P1–P5, walk the lens questions against the ticket. Skip lenses that clearly don't apply (most tickets engage 2–4 lenses, not all five).

3. Output a short markdown brief in JJ's voice (P6): 6–12 short lowercase questions, grouped loosely but without principle labels. Where P5 suggests cutting scope, propose the smaller version and name the deferral.

4. End with one line: "answer these in the ticket or PR description before opening the PR."

Save the brief to .jj-preflight.md in the current worktree if it doesn't already exist; otherwise print to stdout.

---

Mode B — diff

Input: git diff main (or against the current branch's base).

Step 1: detect the quiet-PR case

If the diff is purely:

• renames / import path updates
• version bumps / dependency updates
• cross-repo plumbing or generated files
• pure infra (k8s manifests, Vault config) with no logic

…output a single line approved and stop. Mirror JJ's actual behavior: 16 of his 26 recent PR reviews were silent approvals on exactly these shapes.

Step 2: walk the diff through each principle

For each finding, attach a severity:

• [block] — violates P1, P2, P3, or P4 in a way that would draw a red-line. Examples: new persistence without justification; missing event for a state change; missing sessionID propagation; missing rate-limit on a secret-emitting endpoint; business logic in api_*.go handler; public unauthenticated short-code endpoint with no checksum/nanochk validation; name that lies about the contract.
• [nit] — would draw a comment but not block. Examples: wiring nil-guard pileups; time.Sleep in integration tests; hardcoded crypto algorithm where the key is self-describing; deferred infra cleanups; verbose test scaffolding.

Step 3: output

path:line [block|nit] short lowercase question. P6 voice. One line per finding. Cite consequences where they sharpen the point.

Step 4: mechanical evidence

While walking the diff, gather these as evidence for the principle-level findings — not as the skill's spine:

• rg for time.Sleep in any *_test.go touched (P2 nit)
• count if x == nil guards in environment_setup.go / services.go if touched; flag >2 (P2 nit)
• flag any new repo_*.go or model_*.go without a corresponding event change in the PR (P2 + P3 block)
• LSP/ast-grep: flag any new handler in api_*.go that calls a repo, emits an event, or branches on policy (P2 block — business logic belongs in service)
• flag handlers accepting a short code that don't call a checksum validator before DB lookup (P4 block)
• flag handlers calling the notifications client without a visible rate-limit check (P4 block)
• rg for domain terms with known canonical replacements in the touched scope (Token→Code in claim flows, OTP→TOTP where time-based, Claim→Recipient where the flow noun is the person) (P1 block)
• flag fields/options added that aren't referenced anywhere else in the diff (P5 nit — gold-plating)

If the diff passes all mechanical checks but still violates a principle, say so. Mechanical checks are not a sufficient pass.

---

Voice rules (P6, applied to skill output)

• Short lowercase questions. "why X?" "isn't this already Y?" "wonder if we need this."
• One line per finding when possible.
• Cite consequences, not rule names. Say "costs us money" not "violates P4."
• Hedge soft: "wonder if…", "hmm not sure here", "am I missing something?"
• Approve silently on mechanical PRs. Don't manufacture findings.
• Never say "JJ would say X." Frame as a question the author should answer.
• Occasional "lol" / "heh" is fine on nits. Never on blocks.

Anti-patterns for this skill

• Don't rewrite code. Provoke thought.
• Don't run style/lint checks. gofmt and existing linters cover those.
• Don't generate the PR description. moov-pr-workflow exists for that.
• Don't pad. If P3 doesn't apply, skip it. A short honest review beats a comprehensive fake one.
• Don't predict JJ. Apply the principles. The author owns the answer.