For Quality

What already exists, per the PR's own CI workflow

.github/workflows/claude-mcp.yml stands up a real Postgres and Redis, applies migrations, creates both database roles, seeds two separate clinics plus dedicated boundary fixtures, starts the actual API, and then runs:

• 249 tests in packages/claude-mcp, including a dedicated two-clinic

leak suite (src/db/rls.test.ts) and "parity" tests for each tool (*.parity.test.ts) that compare the tool's answer against the API's own equivalent query for the same question.

• 259 tests in packages/server covering the new OAuth endpoints.
• Mutation testing, currently scoring 80.29% against a 75% break

threshold, over the database-independent modules only (config.ts, auth/types.ts, auth/rateLimit.ts, auth/oauth.ts, tools/types.ts, tools/permissions.ts — see stryker.config.json).

The workflow itself had, as of this PR, never run — it's new in this diff. Its own description says the first run will likely surface something "a clean container finds that our machines hid," which is a realistic and honest expectation for a CI job this new, not a red flag by itself — but it does mean this specific pipeline shouldn't yet be treated as a proven, stable gate.

Where the real edges are — and why they're specifically fixture-shaped

The most instructive thing in this PR for a QA reader is BUGS.md §3 and §4: multiple real bugs shipped with a fully green test suite, because the seeded data never exercised the branch the bug was in. Concretely:

• A boundary condition with no boundary row. amount < minAmount versus

amount <= minAmount return identical results unless some item sits exactly on its minAmount. No seeded item did, so the wrong operator passed every test until sql/004_boundary_fixtures.sql deliberately placed one there.

• A netting rule with nothing to net. Consumption tracking subtracts

USAGE_RETURN movements; the seed produced none, so deleting the subtraction logic entirely changed no test's outcome.

• An attribution rule with no unattributed rows. Every seeded purchase

order had a supplier, so the code path counting orders without one never ran.

• Permission narrowing nobody's seeded user needed. No seeded user held

VIEW_INVENTORY_PURCHASE without also holding VIEW_INVENTORY_VALUES, and none held VIEW_CREATED_INVENTORY — so two narrowing branches had zero coverage regardless of how many tests existed.

The general lesson for testing anything new in this package: a green suite proves nothing about a branch the data can't reach. Before trusting coverage here, ask specifically "does a fixture exist that sits on this exact line, or exercises this exact 'nobody' case?" — not just "is there a test for this."

What to test if you're extending this feature

• Any new comparison operator (<, <=, date ranges, etc.) needs a

fixture sitting exactly on the boundary, not just values clearly on either side.

• Any new permission-gated field needs a seeded user who holds the

narrower permission without the broader one it's often bundled with, and ideally one who holds neither.

• Any new tool should get a parity test against the equivalent API query,

not just an isolated assertion of its own output — the whole promise of this feature is "agrees with clinic-web," and that can only be verified by literally comparing the two.

• Anything touching the OAuth flow in the browser — cookies, redirects,

CSP — needs to be clicked through by hand, in more than one tab, per BUGS.md's closing rule. Unit and integration tests provably did not catch five of the real bugs on this branch, because they were browser-only failures after the server had already returned a 200.

Specific regression risks worth a manual check before any release

1. **Two tabs, one authorize attempt each, both left half-finished, then

both resumed.** This exact scenario broke the flow once already (BUGS.md §6b).

1. **A doctor logging in via the username+company screen with a pending

OAuth connection**, specifically because that screen navigates imperatively rather than waiting for a re-render — the general class of bug in §7 ("a guard on one code path proves nothing about a second code path that makes the same decision differently") is exactly the kind of thing likely to recur if this flow gets touched again without rereading this note. This is the one demonstrated directly in the Walkthrough.

1. A revoked or expired token retried immediately — should fail closed,

with no distinguishing error message (see Security hardening and bugs fixed).

1. The mcp_readonly role's grants, after any future migration — confirm

ALTER DEFAULT PRIVILEGES is actually still catching new tables, since a silently-missing grant would just look like the MCP mysteriously can't see a new table, not like a security failure.

1. **Whichever sandbox this eventually gets tested against has

OAUTH_ISSUER, OAUTH_MCP_RESOURCE, and OAUTH_CLINIC_WEB_LOGIN_URL actually set** — see the Walkthrough for how we confirmed the current sandbox does not, meaning the OAuth routes are silently unmounted there rather than reachable.