Skip to content

Guardrails: type-aware lint, unused-code and duplication checks in CI (backend campaign 14) - #107

Closed
theobong wants to merge 6 commits into
chore/backend-campaign-13-performancefrom
chore/backend-campaign-14-guardrails
Closed

theobong wants to merge 6 commits into
chore/backend-campaign-13-performancefrom
chore/backend-campaign-14-guardrails

Conversation

@theobong

Copy link
Copy Markdown
Member

What changed

Guardrails that keep the campaign's gains: type-aware lint rules (floating and misused promises, exhaustive switches, non-Error throws and rejections, string coercion of objects, redundant types and unnecessary assertions), noUnusedLocals, and new steps in the existing CI build job for formatting, the dynamic-SQL guard, unused code (knip), duplication (jscpd) and the email worker's tests. The code changes are the ones those checks demanded, none of which changes behavior.

Before you start

  • Where: staging (civfix.dev, admin.civfix.dev) after this merges to main
  • Sign in as: your own account with an emailed code; an operator session on admin.civfix.dev
  • Stacked on Performance: batched statements, indexes and event-loop fixes (backend campaign 13) #106 (performance). Read only this PR's diff against that branch, commit by commit.
  • Size: 1156 counted lines across 205 files. Campaign PR; the author approved PRs over the 400-line cap for this cleanup. Most of it is the mechanical removal of type assertions the checker already proves (133 files, first commit).

Verify

Nothing new to verify in the app: the checks run in CI. Every step below is a regression check.

Regression

Requesting your data — [Web]

  1. Open "Settings", then "Account", then "Request my data" (once only) — Expect: "We emailed a copy of your data to ." and the email has the same sections as before

Admin reports — [Admin]

  1. On the Dashboard click "Open reports", open a report and click "Send to city" — Expect: "Sent to "

Photo reports — [Web] [Mobile]

  1. Click "Report", add a photo, place the pin, pick a category, type a title and click "Drop pin" — Expect: "Pin dropped. We're on it."; the photo shows on the report after about 30 seconds

Findings addressed

Campaign PR 14 (guardrails).

  • Type-aware lint (api and media worker; shared preset typed() in the config package): no-floating-promises, no-misused-promises and await-thenable found nothing, and they now keep it that way. Also on: no-for-in-array, no-implied-eval, only-throw-error, prefer-promise-reject-errors, restrict-template-expressions, no-base-to-string, no-redundant-type-constituents, switch-exhaustiveness-check (a default counts as exhaustive) and no-unnecessary-type-assertion.
  • noUnusedLocals in the base tsconfig.
  • CI build job gains: format check (prettier; Markdown excluded, since prose and tables are laid out by hand), the dynamic-SQL guard (check:sql, until now a manual script), knip, jscpd and the email worker's install and tests (it has its own lockfile). No job was added or renamed, so the required checks are unchanged.
  • knip findings resolved: 43 exports nothing imported are no longer exported; unused re-exports removed; an unused testcontainers devDependency removed (the Postgres module still brings it); the media worker's storage and error-reporting dependencies are declared as used by the bundled api code; two ops scripts documented in runbooks are entries rather than dead files.
  • jscpd measures 0.17% duplicated lines in src today; the threshold is 0.5%, so growth fails and today passes.

Decisions for the reviewer

  • Behavior-neutral by construction: assertion removal is erased at compile time; 70 test sites that relied on an assertion to type response.json() now pass the type argument instead (json<T>()), so their types stay identical. The one runtime-visible edit is String(status) in an admin error message's default branch, identical for the string values that reach it; the data export's compile-time section check now also names the array the export iterates (the same array).
  • 33 targeted lint disables, each with its reason: 9 assertions the checker needs inside vi.hoisted factories, a rollback sentinel thrown on purpose in the demo seeders, String() fallbacks in diagnostics, one deliberate pass-through of a non-Error rejection, tests that must reject with SMTP-shaped plain objects, and the test helper that builds predicates with new Function.
  • Left off on purpose: return-await (fixing it changes which errors a catch sees), unbound-method, no-unnecessary-condition and require-await (86 to 400 findings each, mostly noise), and noImplicitReturns.
  • Three pairs of constants share a value but name different settings (request timeout and shutdown wait, the route deadline and the stale-claim window, the socket queue cap and the handshake buffer); tests pin their equality and a doc names one of them, so they stay two names, marked as intended aliases for knip.
  • Two files knip reports as unused are kept and listed: the event consents repository (its removal is an open decision) and the jurisdiction handle backfill (the only writer of that column).
  • .git-blame-ignore-revs is not added: it must name the squash commit of Backend formatting pass with the repo's Prettier config #91 (formatting), which exists only once Backend formatting pass with the repo's Prettier config #91 merges.
  • When Backend test safety net: SQL transcripts, CSRF pairing, authz and characterization tests #92 merges and flows into this stack, its new tests need their import paths updated to the moved modules, its transcript snapshot regenerated and four assertions it contains dropped for the new lint rule; a scratch merge confirmed that list.

User-visible copy changes

None.

Tests changed

  • 133 test and source files lose type assertions the checker proves (compile-time only); 70 test sites switch as T on json() to json<T>() (same types).
  • Unused imports and locals removed from test files; one test harness types its injected send error as Error (all callers already pass one).
  • Four test helpers stop exporting constants nothing imports.
  • No assertion was changed, skipped or removed.

Verification

  • Every commit typechecks on its own (api and media worker).
  • At this branch: pnpm lint (type-aware, about 110 s), pnpm typecheck, pnpm format:check, pnpm check:sql, pnpm knip, pnpm dup:check and the email worker's 16 tests pass.
  • api unit suite by path: 439 files, all green apart from the local SQL-recorder cases that Backend test safety net: SQL transcripts, CSRF pairing, authz and characterization tests #92 will regenerate (7370 passed, 3 skipped); the regenerated transcripts show no statement changed (924 byte-identical; the 11 removed cases are helpers that are no longer exported, still recorded through their callers). media-worker unit: 300 passed, 2 failed (the known arm64 ffprobe helper issue fixed in Backend test safety net: SQL transcripts, CSRF pairing, authz and characterization tests #92).
  • New dependencies: knip (ISC) and jscpd (MIT); their transitive licenses are MIT, ISC, BSD-3-Clause, 0BSD and Apache-2.0.
  • The integration suite was not run (it needs Docker); CI runs it once this PR is retargeted to main.

Staging checks

  • https://api.civfix.dev/readyz answers ready (no migration in this PR).

Not covered

  • The new CI steps themselves run only on GitHub; they were run locally with the same commands.

🤖 Generated with Claude Code

@byteful byteful closed this Sep 24, 2026
@byteful
byteful deleted the chore/backend-campaign-14-guardrails branch September 24, 2026 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants