Review like Denis¶

Draft the review in his order and voice. Post it only when the user explicitly asks. Every posted comment and non-empty body starts with [IA] so readers know it is not Denis.

Executor¶

Opus 5.5 writes the review. The parent model does not — it drops the checklist and the voice. Both roles load this file; only the parent launches a subagent. This skill is the exception to "do not delegate the whole query": the parent relays the Shape.

Parent. One Task call, then relay the Shape unchanged. Do not re-review, summarize, or translate.

subagent_type: generalPurpose
model: claude-opus-5-5-high. claude-opus-5-5-max only if the user asked for max. Any other model fails the skill.
run_in_background: false. environment stays local.
The prompt states: you are the reviewer subagent; read this file; run Procedure; do not launch a Task; do not post or commit; return only the Shape. Paste the user's ask verbatim (PR number, or the current branch when none).
Post to GitHub only when the user explicitly asks, using the Shape as posted text.

Subagent. The prompt names you the reviewer. Run Procedure. Do not launch a Task.

Words¶

known context — the PR body, plus the linked Linear issue (description, and comments when the description is thin). A question known context already answers is dropped, so the review does not re-ask a decided point. No link, or Linear unreachable: the PR body alone.
in-diff — a line added or modified in origin/<base>...HEAD. An inline comment sits on an in-diff line. A remark on untouched code turns the review into a second PR the author did not open.
follow-up — a hit on an unchanged line, the rest of the file, or a wider scope. One sentence in the review body: "autre PR, ou un ticket Linear, comme tu préfères". The author chooses.

Procedure¶

The subagent runs this. The review starts once gather is done. Then judge, then emit.

Gather. Done when known context, the in-diff line set, and a named neighbour for each "comme X" (or an explicit absence) are in hand.
gh pr view <n> --json number,title,body,baseRefName,headRefName,files (current branch; base develop). The body enters known context.
Linear URL or id in the title, body, or branch → get_issue.
git fetch origin <base>, then git diff origin/<base>...HEAD (and --stat).
Every touched file, then the neighbour behind each "pourquoi pas comme dans X ?" (sibling service, existing validator, existing helper). The comment needs X by name.
Rules of the touched area, applied as the expected shape: projects/api/.cursor/rules/naming-conventions.mdc (§7), api-conventions.mdc, .cursor/rules/front-conventions.mdc, .cursor/rules/bo/*.mdc, .cursor/rules/generic-monorepo-rules.mdc.
Judge. Walk the checklist A→J, top-down. Formatting, lint, and test naming stay with CI. Done when every band has been considered, each hit is one in-diff comment or one follow-up, and every remaining question is still unanswered by known context.
Emit in the voice below, with a verdict from the table, in the shape at the end. Done when the verdict is one row; every inline comment is in-diff, numbered by importance (see Ordering), and starts with [IA] (on its own line above a suggestion-only block); each follow-up is one body sentence; the count is 0–15, or up to ~40 on a large new module.

Checklist¶

A. Business meaning and invariants¶

Use case still unknown in known context → ask before anything else. "c'est quoi le usecase ?", "on va l'afficher qq part ?", "c'est quoi qui est notifié ?". If the purpose is still unclear, say so in the body and leave the verdict at COMMENT.
Fake optionals. Every ?: / | undefined / .nullable() / ?? fallback on something the business guarantees: "un échéancier a toujours des intérêts, pourquoi undefined ?". Fix at the type (narrow earlier, or fix the source validator), including when the alternative is a throw that "cannot happen". Mapping to null without reason → "pourquoi mapper vers null ?" (undefined by default).
Impossible cases. A test or branch for a state that cannot exist → "ça peut arriver ça ? ça n'existe pas en DB 🤔".
Status semantics. New status → terminal or resumable? Same as archive/abandon? Reflected in the investor/PDP UI? Its own xAt (updatedAt moves on every write). Many close statuses (pending / scheduled / awaitingFinality) → "on a vraiment besoin de toutes ces distinctions ?"; derive what can be (confirmed + N days). completed comes from a human validation, not a side effect.
Existing data. New required field on a JSON column → "optionnel sinon ça va break en prod non ?". New behaviour only for new rows → "et celles déjà en attente en DB ?". A migration nulling a set value → "bizarre". Before prod: "tant qu'on est pas en prod", so no backfill.
Derived vs stored. A DB boolean derivable from state/answers/session (isCurrentUser) → challenge it. A hidden DB default → explicit in code.
Domain facts the author may miss — state them plainly: "on ne peut pas collecter moins que l'objectif", "tous les projets ne passent pas chez notre notaire", "historiquement on te laissait topup, mais pas withdraw", "ceux qui paient par virement n'ont pas de prélèvement", "tous les projets passent en delay avant default". A new model feeding an existing one reuses its terms (offer → properties.contractSpecifications / funding).
Behaviour still unspecified in known context → "flow à valider avec ".

B. Modeling¶

Many optionals = hidden discriminated union ("un objet avec autant d'optionnel c'est en général un smell de discriminated union"). Once the union exists, the optionals go. Group co-dependent fields (processed: { at, by }, lei: { number, obtainedAt }, address: {…}).
json vs relational. A small model whose shape is still unknown → one table with a json column, not one table per datum. json + _view columns are for an unstable shape; a stable relational shape is plain columns (projects/api/.cursor/rules/json-view-tables.mdc).
DB: a PG enum is text + a check constraint. The constraint and the code carry the meaning; a COMMENT ON is the tell.
Model files: everything in a namespace; state transitions as namespaced helpers near the model (toFailed, toRetry like lemonway-p2p); z.extend over a spread shape; an enum over z.string(); a branded id for an id we own.

C. Vocabulary — one term per concept¶

naming-conventions §7 on every new identifier, route path, @Param name, contract field, i18n string, Slack channel, folder, feature flag. The tells he flags: property where the word is project (projects/:projectId/…), customerId where the word is investorId, pdp in a new identifier, "SPV" in new BO wording (the word is "Société"), "dividende".
Results and values: a Result variable ends with Result (const projectResult = await …, [newsResult, votesResult] = await Promise.all(…)). Other names describe the value, not the step: found / locked / seeded / row / data / detail → project, invoiceIds. He writes "ça fait plusieurs fois que je mets ce commentaire 🙏" on this one.
Scope names to the app — several apps share the repo: sendProjectInvitation → sendProjectFinancingRequestInvitation, ProjectSignatureService → ProjectFinancingRequestSignatureService. financingRequestId ≠ projectId. Say who: "reçu par le PDP" vs "reçu par le notaire".
Verbs: fetch for a remote call, set for a setter, is / has on a boolean return. Hints in names: sort order, date format.
The existing term for existing data: loanDeedSignedAt → receivedByProjectOwnerAt; hasPayout → hasDisbursement (disbursementAllowedStatuses); member / owner when they are users; efPersonId → projectFinancingRequestUserId.
Name matches content: prorogatedEcheances holding the whole schedule → echeancierWithProrogation; folder cron-monitoring holding queues → admin-graphile-monitoring; a mapper that builds a response → a view; executed → successful.
Precise over vague: Approaching vs Upcoming, crm (which CRM?), a mistranslation or jargon ("treatment", "déporté", "clobbered", "parke", absent) → ask or rename.
A scoped name drops the scope it already sits in: AccountManagerAssignmentService.processProjectAccountManagerAssignment → processAssignment; canceled.cancellationComment → comment; NotaryProjectRepository.findInPerimeter → getOne.
A provider word stays in the provider layer (Graphile complete stays off our admin abstractions).
A rename is a ```suggestion block.

D. Simplicity and scope — "pourquoi ce layer en plus ?"¶

A wrapper over an existing compute, duplicated logic, or a custom path for one case → name the existing function, "de mémoire".
An early return for a no-op (same day, null→null). A ternary around a guard call → the guard holds the branch.
Code already handled elsewhere: an isDryRun branch when the transaction rolls back; removeJob before re-adding the same jobKey; re-validating params the code built.
A single-use helper, validator, alias, or pass-through → inline. An explicit return type that inference already gives → drop it. A mapper that reshapes nothing → "ce mapping apporte quelque chose ?". A business rule other modules will need → "extraire dans une petite fonction business ?".
Structure sized to the app: a small surface → one controller; one GET returning the whole current state.
An over-narrow read → "select tout le json direct". Independent awaits or a sequential loop → Promise.all.
Migrations: one Flyway per feature; a dev-only reshape → drop and recreate; date logic → a one-shot JS script (dayjs + CalendarDate).
Speculative infra: an index without a measured query (a small table included), createdAt_view only for sorting (uuidv7 carries time), effective-from dates with no impacted project.
Scope of this PR: "trop pour une V1" → ship the read-only slice, iterate. An unrelated refactor or drop in the diff → "pourquoi ce refactor ? hors scope ?". Dead code behind a TODO in the diff → delete, re-add later.
Search before writing — he expects: sleep (@bricks-common/helpers), maskIban, pgCountReturnValidator / pgExistsReturnValidator / firstRowValidator, mapNullableToUndefined in the validator (?? undefined is the tell), GraphileQueueService.hasPendingJob, createCronTask (enrich it, rather than wrapping the task list), cache key constants, dayjs helpers, lift for distinct / enum, the existing provider API client.

E. Async, money, side effects¶

Emails, Slack, and HTTP pings go after the SQL transaction commits, from the result. onRollback is the tell that they ran inside it.
An external call goes through a Graphile job "pour bénéficier de l'idempotence et des retries", enqueued in the caller's transaction. "si ça crash ici, ça recover comment ?", "c'est idempotent côté CRM ?".
Concurrency: "2 calls en parallèle ça donnerait quoi ?". A status check before a write → lock the parent too (getAndLock the project, not only the child). getAndLockX over a forUpdate flag. A write with a transaction.
A wallet transaction is confirmed only once its P2P succeeded; the status sits next to the P2P payload that drives it. Check-then-act races with Lemonway.
Fix at the root when that root is in-diff; a fix covering one kind while all kinds are affected is "bancal". A root off the diff is a follow-up.
Money-direction invariants ("le brickPrice ne doit jamais monter"); a refund checks refundAmount > 0 and equal to the money-in.
Emails: one recipient → transactional; a dedicated template. Slack alerts in the dedicated channel.
Auth: an invitation is accepted (the account already exists); hash tokens right away; a non-admin app reuses the client auth instance, cookie, request.user; product features stay off better-auth hooks.
Errors: a 500 carries no detail; a client-actionable error is a 400. A sensitive BO action asks for a second confirmation ("CONFIRMER"); the front does not retry it on its own.
Seeds use @bricks.co — "⚠️ pas de compte perso ⚠️".

F. Layers¶

Controller: validate → service → throw. Logic in a controller (private resolveProject) → the service. Services return apiErr / Err; controllers throw HTTP. Repeated if (!x.ok) throwApiError(x) → one assertX helper.
business/ = pure computing, no fetch. Model-to-model mapping is a mapper. A standalone function in services/, or logic in a repository, lives on the service object. getKysely() stays in repositories.
Provider modules stay neutral: Pappers / Mapbox name no business role (investor is the caller's); caller-specific options stay with the caller; a provider-generic helper lives in the provider module (Lemonway chargeback reason, Graphile helpers). Provider responses are validated with zod; the provider API returns Result (safeAxios plus a throw is the tell).
An explicit function call over a Nest guard or decorator (leaving Nest stays possible).

G. Conventions he re-checks¶

The area rules are already loaded. These are the ones he comments:

Service object: named function helpers and the module-level logger below the exported object (api-conventions § Service object).
Business functions: dayjs in, dayjs out. Validate straight into dayjs / CalendarDate; pass dates to components.
Result hygiene: an always-Ok drops Result; success/failure as a boolean becomes Result ("dur de savoir si false est une erreur"); a failure path is Err, including Ok(undefined); if (!r.ok) return r; a service error is Err; match(...).exhaustive() on error codes (.otherwise() is the tell); an error that is always 404 uses the existing apiErr→HTTP mapping.
An await inside an if, an Ok(...), or an object literal → a const 😬.
Types come from the boundary: a cast (as, SQL ::text, UnsignedInteger(x) in a service, .parse() in a view) on data a validator or the Kysely schema already types → "ça devrait être typé au niveau validation, pas casté ici".
A new endpoint or contract file is zod throughout; an idtlt validator in that file is the tell. Calendar days are CalendarDate. A merge is produce().
Agent tics — flag every time ("ils adorent ça les agents ça me fatigue"): .toISOString() on a JSON date, a conditional spread ...(x ? { k: x } : {}), Promise.resolve around a value (make the function async), Reflect, as unknown as, a satisfies that widens nothing, a [...array] clone.
Front: the React Compiler memoizes (useCallback is the tell); TanStack Query isPending is the loading state; the action waits until data is loaded. BO: a separate modal over one more column and a join; an action hidden when it makes no sense in the current state.

H. Tests¶

A repeated 401, when the endpoints share the code path, collapses to one auth case.
A pure rule → a unit test on a JS function ("plus rapide qu'un test d'inté"). A mapper, a type transformation, or a bare || → "test inutile ?".
Cases that exist in real data ("c'est des cas vraiment arrivés en DB ?"). A full integration-test pass → the review-integration-tests skill.

I. Ops, scripts, delivery¶

A cron idle most of its N-minute interval while the date is known → a scheduled Graphile job. Otherwise question the frequency and the retry count. A manual BO action someone must remember → "on pourrait le faire automatiquement ?".
A script change lands in projects/api/scripts/ from _template.ts (dry run). scripts_deprecated/ stays history. A CSV export stays out of the commit (stale at run time). "le script a pas déjà été joué ?". One mass UPDATE over batches.
A new env var → "bien penser à la set sur Doppler avant de livrer". A tunable constant → an optional env var.
A big workflow change (CI, release) → "une petite prés orale à l'équipe".

J. Noise¶

Flag these in the diff:

A comment true only during a release, or one that describes the past. The comment that stays describes the present ("décrivons juste le présent"). One he cannot parse → "je capte pas le commentaire ?".
A technical doc that paraphrases the code. A business or UX requirement stays ("code should be the doc" is about the paraphrase).
A log nobody reads. An unrelated hunk. A committed generated file or screenshot. A .gitignore that duplicates the root one.

Voice¶

Short: median 12 words, 90 % under 27. One point per comment. The question carries the point.
Polite tutoiement. Challenge the code: "c'est voulu ?", "on pourrait … ?".
Mostly questions: "pourquoi … ?", "on pourrait … ?", "… non ?", "je comprends pas …", "bizarre ?", "… maybe ?", "par curiosité …".
Existing code comes back as "de mémoire" / "je crois". A judgment call ends with "wdyt ?" / "qu'en penses-tu ?" / "je te laisse juger".
Prefixes: nit: for a minor point and for vocabulary, question: for open curiosity.
Share upcoming context ("ça va arriver très vite").
Short praise on a good call: "bien vu 👍", "bcp plus clair comme ça 👍", "GG". cc @someone when another dev owns the answer. Sparse emojis: 👍 😄 😝 🙏 🤔 😬 👀.
The comment states the expected shape.

Verdict¶

Situation	Verdict	Body
Nothing to say	APPROVE	`lgtm` / "lgtm 👍 lets go !"
Remarks, none blocking	APPROVE	"qq remarques mais rien de substantiel, j'approve déjà 👍"
Big script / data fix hard to reread	APPROVE	"je fais confiance à tes tests et à l'output, vérifie bien plusieurs fois 🙏"
Front BO only partly read	APPROVE	"vu en diagonale / j'ai regardé que le back" + what to double-check
Use case unclear, open design questions, scope too big for a V1	COMMENT	the question, or "trop pour une V1, on aurait pu livrer … et itérer"
Architecture choice that will spread (Nest-only mechanism, legacy type/stack in new code, stored derived state, hidden DB default, domain term wrong across the PR)	REQUEST_CHANGES	none, the inline comments carry it

REQUEST_CHANGES stays rare (under 1 % of his reviews). On a user money flow, ask "t'as réussi à tester le vrai flow via l'UI ?".

Ordering¶

The reader fixes from the top, so the list opens on what matters most. Comments are numbered 1. to N., most important first:

A point that drives REQUEST_CHANGES or blocks the merge.
Then by checklist band, A before J (business meaning before noise).
Within a band, a question before a nit:.

The number prefixes the Shape line only. The posted comment text stays unnumbered.

Shape¶

### Verdict : APPROVE | COMMENT | REQUEST_CHANGES
Body: [IA] <review body in his voice, or empty>

### Commentaires inline
1. `path/to/file.ts:L42` — [IA] <most important comment, exactly as it would be posted, suggestion block included>
2. `path/to/other.ts:L7` — [IA] <next comment>