Skip to content

Definition of Done

The pre-merge checklist that every engineer applies to every PR. The checklist below is also embedded in the GitHub PR template (see pr-template-reference.md).

For the principles this checklist enforces, see Engineering Charter. For the full catalog of practices, see Engineering Practices.

The Checklist

## Definition of Done

- [ ] Code passes lint, typecheck, and tests locally
- [ ] New code has tests for the happy path and at least one error case
- [ ] No `console.log`, no `any`, no `@ts-ignore` — or each has a justified `// @ts-expect-error: <reason>`
- [ ] No new secrets committed; gitleaks pre-commit passed
- [ ] If endpoint added or changed: OpenAPI spec updated in this PR
- [ ] If schema changed: migration file added in `migrations/`
- [ ] If new env var: added to `.env.example` and `configuration.md`
- [ ] If new external dependency: justified in description; license / size / maintenance checked
- [ ] If touching a legacy file (`routes/routes.js`, `routes/auth.js`): describe the boy-scout improvement
- [ ] If new endpoint: rate limit + auth role declared
- [ ] If new background job: idempotent, uses queue (not polling)
- [ ] CI is green
- [ ] Self-review: I read the diff before requesting review

The rest of this document explains each item: what it means, why it matters, and what counts as acceptable.

1. Code passes lint, typecheck, and tests locally

What it means: before pushing, run yarn lint, yarn typecheck (if TypeScript), and yarn test on your machine. They all pass.

Why: waiting for CI to surface preventable failures wastes everyone's time and clogs the queue.

Acceptable: all three exit with code 0.

Not acceptable: "it'll pass after one more push" — push it after it passes.

2. New code has tests for the happy path and at least one error case

What it means: the code you wrote is exercised by tests in this PR. At minimum: - One test for the expected-success path - One test for an error path (invalid input, missing resource, downstream failure, etc.)

Why: tests are the only durable proof that the code does what you intended. Without them, the next refactor will silently break it.

Acceptable: - Integration test for new endpoints (request → response, hits DB) - Unit test for new helpers / pure functions - Tests in the same PR as the code, not "follow-up"

Not acceptable: - "I tested manually" without an automated test - "I'll add tests in a follow-up PR" (the follow-up never happens) - Tests that pass without actually exercising the new code (verify with patch-coverage report)

Escape valve: truly trivial changes (a typo in a string, a doc update). Reviewer judgement.

3. No console.log, no any, no @ts-ignore

What it means: - Use platform/logger.ts (or the project's structured logger), not console.log / console.error / console.warn - Don't use any in TypeScript without an explicit // @ts-expect-error: <reason> comment - Don't use // @ts-ignore (use // @ts-expect-error instead so the suppression auto-detects when the underlying error is fixed)

Why: - console.log produces unstructured output that can't be queried in centralized log aggregation. With ~108 workers, this is the difference between debugging in seconds and debugging in hours. - any defeats the entire value of TypeScript. - @ts-ignore becomes a permanent rot point that no one notices when the underlying type is fixed.

Acceptable: - logger.info({ accountId, jobType }, 'Job processed') instead of console.log('processed', accountId) - // @ts-expect-error: third-party lib has wrong types; tracked in #1234 followed by the offending line

Not acceptable: - console.log left in code "for debugging" — that's what logger.debug() is for - as any casts to silence type errors

4. No new secrets committed

What it means: no API keys, tokens, passwords, or credentials appear as string literals in any committed file. Pre-commit gitleaks ran and passed.

Why: every secret committed is one that has to be rotated. Three secrets are already in the repo's history (Slack token, Polotno key, session secret) — the team is paying that rotation cost. Don't add more.

Acceptable: - process.env.API_KEY or conf.API_KEY reading from environment - Test fixtures with obviously-fake values ("sk_test_FAKE_...") that gitleaks recognizes as test data

Not acceptable: - A real API key, even "just temporarily" — there is no "just temporarily" in git history - A test fixture with a real-looking value that gitleaks might flag (use clearly-fake values)

5. If endpoint added or changed: OpenAPI spec updated

What it means: if you added a new HTTP endpoint, modified the request/response shape of an existing one, or changed authentication behavior, the OpenAPI spec (e.g., openapi.yaml or generated equivalent) is updated in the same PR.

Why: the spec is the contract with API consumers. Spec lagging behind code = clients break unexpectedly, SDK generation produces wrong types, frontend assumes shapes that don't exist.

Acceptable: - Spec updated to match new behavior; spec validates as well-formed - For new endpoints: full path, method, request schema, response schema, auth requirement

Not acceptable: - "I'll update the spec in a follow-up PR" — the spec drifts - "It's an internal endpoint" — internal endpoints still have a contract; document them

Escape valve: truly internal endpoints called only by other workers in the same process can have a lighter doc burden — but they should be in the spec under an internal: true flag at minimum.

6. If schema changed: migration file added in migrations/

What it means: if you added or modified any database table, column, index, or constraint, there is a versioned migration file in migrations/ that produces that change when run.

Why: the schema must be reproducible. Manual ALTER TABLE in production produces environment drift that is painful to fix. Migration files let any environment reach a known state.

Acceptable: - A migration file with both up (apply) and down (revert) steps - The migration runs cleanly on a fresh database - Code changes that depend on the schema change are in the same PR

Not acceptable: - Code that references a new column without the migration that creates it - A migration file with no down step (rollback is needed) - ALTER TABLE snippets in a comment for someone to "run manually"

7. If new env var: added to .env.example and configuration.md

What it means: every new process.env.<KEY> reference in source has a corresponding entry in: - .env.example — with a placeholder value - configuration.md — with a description, required/optional flag, and example value

Why: new engineers and new environments must be able to bootstrap from these two files. A new env var that lives only in someone's head is a future onboarding bug.

Acceptable: - Both files updated in this PR - Placeholder values clearly marked as placeholders (<your-key-here>)

Not acceptable: - "I'll update the docs later" - A .env.example entry with a real value

8. If new external dependency: justified in description

What it means: if you added a package to package.json, the PR description includes: - What the package does - What alternatives you considered (and why this one) - License (must be MIT / Apache-2.0 / BSD / ISC) - Approximate bundled size (use bundlephobia.com) - Last release date (must be within last 12 months) - Any known critical CVEs (run yarn audit after adding)

Why: the dependency tree is currently 91 packages with 14 fully unused. The default attitude toward adding deps must be "no" until justified.

Acceptable: - Paragraph in PR description covering all six items - The package is not on the deprecated list (request, moment, etc. — see Dependencies Inventory)

Not acceptable: - "It's small / trivial / standard" with no actual research - A package on the deprecated list

Escape valve: internal dev-only deps (e.g., a new ESLint plugin) have a lower bar — note it briefly in the PR.

9. If touching a legacy file: describe the boy-scout improvement

What it means: if your PR modifies code in routes/routes.js, routes/auth.js, or other identified legacy files, the PR description explains what you upgraded as part of the change.

Why: the modular-monolith refactor only converges if every legacy-file touch contributes to the upgrade. If feature work in legacy files leaves them unchanged in shape, the cleanup never finishes (the two-streams problem).

Acceptable: - "Extracted the /post-by-id handler into src/modules/content/. Added an integration test." - "This handler is small enough that I extracted the whole thing rather than patching it." - "Truly trivial change (typo fix); no extraction warranted." (Reviewer judges.)

Not acceptable: - Adding 50 lines to routes/routes.js without addressing the structure - "I'll extract it in a follow-up PR" without a tracking issue

10. If new endpoint: rate limit + auth role declared

What it means: new HTTP endpoints declare: - The required authentication / role (e.g., requireRole(['admin']) or requireAuthenticated) - A rate limit appropriate for the endpoint's cost (per-user, per-account, or per-IP) - For cost-bearing endpoints (AI generation, social posting, media render, etc.): a per-tenant quota check

Why: missing-auth bugs are the most common security regression. Missing rate limits are the most common cost-runaway. Both should be specified at the route level so they're greppable and reviewable.

Acceptable: - router.post('/x', requireRole(['user']), rateLimit({...}), handler) - A clear comment if an endpoint is intentionally public

Not acceptable: - An endpoint with no auth declaration that requires manual reading of the handler to know the auth posture - A cost-bearing endpoint with no rate limit "because we don't have many users yet"

11. If new background job: idempotent, uses queue (not polling)

What it means: new background workers: - Consume jobs from the queue (BullMQ / SQS / Temporal — whatever the platform uses), not by polling MySQL - Generate or accept an idempotency key for the work they do - Are safe to run multiple times for the same input — retries don't cause duplicate side effects

Why: the existing 100+ polling workers are an architectural failure flagged in the readiness doc. Without idempotency, retries on social-publishing jobs produce double-posts. We don't extend the failure mode with new code.

Acceptable: - A worker that calls queue.process('jobName', handler) - The handler derives an idempotency key from the job payload - External calls (social platforms, AI providers) include the idempotency key

Not acceptable: - A new setInterval polling MySQL - A new worker calling external APIs without an idempotency key - "I'll add idempotency in a follow-up" — without it, the worker is unsafe to retry

Escape valve: internal background tasks with no external side effects (e.g., a cleanup job that deletes expired in-memory cache entries) may be polling-based for now — but should still go through the queue if at all reasonable.

12. CI is green

What it means: all required CI checks pass. None of them are skipped, force-merged, or marked "ignore failure."

Why: CI is the team's collective signal that the change is safe to merge. Bypassing it teaches everyone that CI is optional, which destroys its signal value within weeks.

Acceptable: every check is green

Not acceptable: - Force-merging despite a failing check - Disabling a check to make a PR merge - Re-running failed checks repeatedly hoping they pass (flaky tests should be quarantined per Practice §C.5, not re-rolled)

Escape valve: hotfix-emergency PRs (production down) may bypass CI under engineering-lead approval, with an immediate follow-up to fix whatever was broken.

13. Self-review: I read the diff before requesting review

What it means: before clicking "request review," you opened the PR's "Files Changed" tab and read every line you modified — as a reviewer would.

Why: half of all PR feedback is preventable by the author re-reading their own diff. Doing this saves the reviewer's time and catches the obvious things (debug statements, unused imports, accidental file changes).

Acceptable: the diff has no debug statements, no commented-out code, no accidental file changes (like a .vscode/ directory).

Not acceptable: "I'll fix that in the next push" for things visible in the diff at the moment of opening the PR.

Edge cases & exceptions

What if a rule genuinely doesn't apply?

Some rules don't apply to every PR:

  • A doc-only PR doesn't need new tests
  • A PR that only deletes code doesn't need an OpenAPI update
  • A trivial typo fix doesn't need a regression test

Mark the irrelevant items as n/a in the PR description, with a one-line reason. Don't tick them as if they applied.

What if a rule applies but you can't comply?

This is the explicit-exception case. Document it in the PR:

## Exception
- [ ] *(can't comply)* Practice §C.4 patch coverage gate
- Reason: this PR refactors an internal helper that has no production callers yet
- Follow-up: tracked in #5678; will add tests when the helper is wired in

Exceptions are reviewed by an engineering lead, not just any reviewer.

What if the rule is wrong?

The practices are not commandments. If a rule produces silly outcomes in your case, raise it on the team channel or in a doc-update PR. Rules that produce silly outcomes get changed, not silently violated.

A note on review etiquette

For PR authors:

  • The Definition of Done is your responsibility, not the reviewer's. They check that you've done it; they don't do it for you.
  • A PR opened with broken CI signals "I didn't run this locally" — fix before opening.
  • Marking items as done that aren't actually done erodes trust. Don't do it.

For reviewers:

  • Cite the rule, not your opinion. "Practice §A.5 prohibits console.log" is clearer than "please remove this".
  • Reject silent non-compliance. Tick-the-box-without-doing-it is the worst outcome.
  • Ask "what's the boy-scout improvement?" on every legacy-file touch.
  • Don't approve until the checklist is honestly complete.

How this list evolves

This is a living document. New practices are added when:

  • A postmortem reveals a missing rule that would have prevented the incident
  • A new wave of practices opens (see adoption-waves)
  • A pattern of repeated reviewer feedback indicates a missing rule

Practices are retired when:

  • They consistently produce silly outcomes
  • The underlying problem is solved by tooling that makes the practice automatic
  • The codebase has matured past the issue the practice prevented

Each change to this list is itself a PR, reviewed by an engineering lead.