Skip to content

Lint and test in CI, clear lint errors, and enforce the hooks rules - #89

Draft
m-murphy wants to merge 32 commits into
fix/audit-mediumfrom
chore/agent-guardrails
Draft

m-murphy wants to merge 32 commits into
fix/audit-mediumfrom
chore/agent-guardrails

Conversation

@m-murphy

@m-murphy m-murphy commented Oct 6, 2026 •

Copy link
Copy Markdown
Member

Summary

Stacked on #88. Turns on the checks that were loaded but off, and fixes what they found.

 ci:verify
   typecheck
+  lint
   build
+  unit tests (api + web)
   docs stale-claims check
+.github/workflows: Checks runs ci:verify on every PR
+pre-commit: lint staged API and web files
  • Lint fixes: 30 API and 64 web no-unnecessary-condition errors. Where a guard exposed a type that lies about the wire, the type was fixed (a control source's definition and getSource's fields are optional; scan-move CSV cells can be missing).
  • React hooks rules on: 86 problems. Four components returned before their hooks (CollectionMove, ControlBatchWizard, ReferenceData, StatChart), so a permission or tab change mid-session changed the hook count and React threw.
  • Transactions: a lint rule rejects async callbacks passed to database.transaction, which commits at the callback's first await.
  • Schema snapshot test: fails when initial_schema.sql falls behind schema.ts (the fresh-install gap behind a3bddaa).
  • Agent docs: pointers to the package test and db READMEs, a trimmed domain doc, and .gitignore entries for local database copies and worktrees.

Evidence

  • Before: bun run lint reported 94 no-unnecessary-condition errors and 86 hooks problems, and CI ran neither lint nor tests.
    After: bun run ci:verify passes locally: typecheck, lint, build, API and web tests, docs check.
  • Before: snapshot test against the pre-a3bddaa initial_schema.sql → fails, reporting version 6 and the three missing qPCR tables.
    After: passes against the current snapshot.

Review follow-ups

  • Hooks and effects: chart colors are read for the theme being rendered, so the three exhaustive-deps disables are gone. Effects in the batch wizard and ContainerRegistration rerun only on their original triggers. Box and bag sheet expansion is set up on load, not on every refetch.
  • Shared code: WriteGuard gates CollectionMove and ControlBatchWizard; useCollectionSheets, defaultScannerConfiguration, one shared empty list, and insertedRow replace repeated code. Collection delete branches on every collection type explicitly.
  • Transaction lint: a type-aware rule flags any transaction callback that returns a Promise, including named async functions, and now covers tests.
  • Schema snapshot test: also compares types, NOT NULL, defaults, primary keys, indexes and foreign keys, and closes its database.
  • CI and hooks: superseded Checks runs are cancelled only on pull requests, so back-to-back merges to main both run. Pre-commit refuses a staged source file that also has unstaged edits, since eslint's type-aware rules read the working-tree copy.
  • Tests: scrollIntoView is stubbed in the web test setup instead of guarded in production code.
  • Evidence: bun run ci:verify passes: API 1087, web 948, contract 86.

Merge Danger

Door: two-way

Blast Radius: wide-but-shallow

Touches 87 files, mostly type narrowing and memoization. The behavioural changes are the four hook-order fixes and effects moved to useEffectEvent, whose triggers are unchanged. Every PR will now fail CI on a lint error or failing test. Pre-commit now refuses partially staged API and web source files.

🤖 Generated with Claude Code

m-murphy and others added 8 commits October 5, 2026 22:14
Index lookups that can miss at runtime (returning() rows, row-index maps)
now use .at(), so their guards type-check as needed instead of being
flagged. Casts that hid null now say so, and dead branches are removed.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Drizzle's bun-sqlite transaction commits at the callback's first await,
which db/README.md already warned about in prose; the audit still found
eleven call sites. The two remaining move transactions never awaited, so
their callbacks are now synchronous.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Where the rule exposed a type that lies about the wire, fix the type and
keep the guard:
- a control source's definition is null for batches without one, and the
  API omits nulls, so ContainerSourceInfo.definition is optional;
- GET /derivations/containers/:id/source has no derivation key for
  containers that were not derived, so getSource's fields are optional;
- scan-move CSV cells read through csvCell, since a column missing from
  the file is undefined despite the row's string index type.

Elsewhere the guards were dead: records that 404 when missing, list
routes that always send an array, TanStack isSuccess narrowing, arrays
already checked non-empty, and a map whose key 0 is always seeded.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ackage docs.

- ci:verify now runs lint and unit tests as well as typecheck, build,
  and the docs check; a new Checks workflow runs it on every PR.
- The pre-commit hook lints staged API and web source files.
- AGENTS.md points at the API test, web test, and db READMEs, which
  agents never opened in recent sessions, and describes the new checks.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
New installs apply the snapshot alone, so it must record the current
schema version and hold every Drizzle table and column. Against the
snapshot before a3bddaa this reports version 6 and the three missing
qPCR tables.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…covering.

- docs/agents/domain.md was the generic template (CONTEXT-MAP.md,
  ordering/billing example, /grill-with-docs); keep what applies here.
- db/README.md: the snapshot test backs step 6; step 7 covers the
  legacy migration fixtures; note the transaction lint rule.
- AGENTS.md: a UI label → code pointer, and state the web test runner
  positively (its hang warning sent agents to `timeout`, absent on macOS).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
eslint-plugin-react-hooks was loaded but none of its rules were on.
Turning on rules-of-hooks and exhaustive-deps found 86 problems:

- Four components returned early before their hooks (CollectionMove and
  ControlBatchWizard on !canWrite, ReferenceData on an unknown tab,
  StatChart on empty data), so a permission or tab change mid-session
  changed the hook count and React threw. The guards now sit in a thin
  outer component, or after the hooks.
- Effects that deliberately run on a narrower trigger than what they
  read (load-on-open, debounced search, defaults on type change) read
  through useEffectEvent, so their triggers are unchanged.
- Fallback arrays (`data ?? []`) feeding memos and effects are memoized,
  so effects such as the scanner-config default stop re-running every
  render.
- The rest: StudyStats bin helpers move to module scope, DataTable's
  cleanup uses the node it attached to, useCommands memoizes on
  destructured fields, and the three theme-keyed CSS-variable memos keep
  theme with a reason.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
These sat untracked at the repo root, so a broad git add could commit them.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
m-murphy and others added 21 commits October 6, 2026 16:48
…e entry point.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…types with the migrated schema.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…tests too.

The selector only caught async function literals written inline. A typed local rule
also catches a named async function and a sync callback that returns a Promise.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…helper.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ng a collection.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…s only when one changes.

useCommands now keeps the caller's deps object while its fields are shallow-equal, and
commands take the route as pathname and search, the only parts of the location they read.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…g `data ?? []` per page.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…seCollectionSheets.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ts only on their original triggers.

Moving these effects onto the full batchInfo and onto containerType made them run on
unrelated edits. Their bodies now read the rest through useEffectEvent.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ange, not every refetch.

The PR moved this effect from the sheet count to the whole sheet list, so it ran on every
refetch. The count alone missed a collection whose sheets were already cached with the
same count, so it now runs on the collection id and the count.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… exhaustive-deps disables.

The getters read CSS variables from the document root, which ThemeProvider updates in a
layout effect after children render, so a memo keyed on the theme kept the old theme's
colors. They now take the theme and read it from a probe element.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…er directly.

Handlers reload by bumping a counter the effect depends on, so loadData can be the
Effect Event and the loadInitialData wrapper goes.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ing it in Locations.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…definition 404s.

The PR dropped the client-side check and relied on the summary route's 404, which showed
whichever of two server messages arrived first, after a retry.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
# Conflicts:
#	packages/web/src/contexts/UserContext.tsx
…main.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ng their working-tree copy.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…erationalDatabase.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ate now that the test setup stubs it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…aultScannerConfiguration helper.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
m-murphy and others added 3 commits October 29, 2026 22:21
…WriteGuard.

CollectionMove now redirects read-only users with <Navigate> rather than an effect
that called navigate, as ControlBatchWizard already did.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts:
#	packages/web/src/components/StudyStats.tsx
#	packages/web/src/pages/BloodControls.tsx
With CI=true typescript-eslint infers a single run and builds one program from
the files on disk, so every case was checked as the placeholder fixture and the
invalid cases reported nothing on GitHub Actions.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.

1 participant