Skip to content

Run all integration tests (in CI too), fix them, and fix the case-creation bug they exposed - #15

Merged
corlin merged 4 commits into
mainfrom
fix/integration-test-suite
Oct 4, 2026
Merged

corlin merged 4 commits into
mainfrom
fix/integration-test-suite

Conversation

@corlin

@corlin corlin commented Oct 4, 2026

Copy link
Copy Markdown
Owner

Why

scripts/test-postgres.sh ran a hand-kept list of 5 of the 14 integration files, and CI skipped every Postgres test. The other 9 had never run. Running them exposed one real product bug, plus stale tests and docs.

Product bug found: case creation failed under the real DB roles (b9905b9)

CaseService.create_case locked the quota row with SELECT … FOR UPDATE. PostgreSQL row locks need UPDATE privilege, and patent_evidence_app only has SELECT on organization_plan_quotas. So every case creation failed (permission denied for table organization_plan_quotas) in any deployment using the intended roles. Unit tests mock the DB, and the integration test for this path had never run.

Fix: serialize creations with the organization advisory lock (pg_advisory_xact_lock; re-entrant, and access.mutation already takes it) and read the quota without a row lock. This prevents concurrent over-allocation without granting the API role UPDATE on quotas. It's covered by test_case_lifecycle_and_quota_enforcement, which now runs (quota of 2 preserved).

Integration suite repaired (9480995)

What was wrong:

  • 6 business-flow tests (cases/documents, features, search, comparison, reports, full e2e) read PE_MIGRATION_DATABASE_URL, which nothing exports, so they always skipped. Their seeds targeted a schema the repo never had:
    • a non-existent credentials table
    • organizations.persisted_status
    • memberships.identity_id
    • missing display_name (NOT NULL since the first schema commit)
    • positional calls to the keyword-only login_with_totp
  • test_p0_02_e2e_lifecycle truncated tables that don't exist, called the bootstrap script with an old interface, and expected stale keys and status codes.
  • test_database_context depended on data seeded by another file, so it passed only in one file order.

What changed:

  • The script runs the whole integration/ directory, so files can no longer silently be left out. CI now runs the script.
  • test_support has shared helpers written against the real schema:
    • reset_database (derived from the catalog, so it can't go stale)
    • seed helpers for tenants and platform admins
    • login_with_existing_totp for repeat logins
    • api_settings now sets worker_database_url (the worker had been connecting with default credentials)
  • E2E expectations aligned with the implemented contracts. Each one was checked against the API code, the web client or a passing test before the test was changed:
    • provisioning needs Idempotency-Key, plan_key and a period
    • invitation_token is top-level
    • accept returns 201
    • TOTP confirm returns "confirmed"
    • organization.status is the effective status
    • the handoff path is per strategy

Result: 91 passed, 0 skipped. Before: 84 passed, with 9 files never executed.

Design decision for review: a member of a suspended organization gets 403, not 404

The e2e test expected a "safe 404", but it never actually exercised suspension:

  • It skipped the MFA challenge, so the request stopped at 401.
  • With MFA added, it then hit the login rate limiter (4 per minute, frozen clock).

Once fixed, the API returns 403 forbidden, so suspension is enforced. This matches OrganizationAccess: 404 for non-members (hiding that the organization exists) and 403 for members of a non-active organization (who already know it exists). I aligned the test with that rule. If you want suspended members to get 404 as well, that's a product change, not a test fix.

Docs and types (97fa7c7)

  • runbook-platform-bootstrap.md described an older script. It now matches the real one: positional email and display name, PATENT_EVIDENCE_* variables, no credential material printed, and a 30-minute handoff (not 10). local-development.md was already correct.
  • Web TotpConfirmResult.status was typed 'verified', but confirm returns 'confirmed'. No UI code branched on it; there are now separate types per endpoint.

Correction to earlier PRs

#6 said its integration test covered the upload checks. It never ran. It now runs and passes, so those checks are verified for the first time.

Merge note

#13 adds one file to the old 5-file list in test-postgres.sh. This PR replaces the list with the whole directory, so if they conflict, keep this PR's version; #13's tests are picked up automatically.

Validation

  • ./scripts/test-postgres.sh: 91 passed, including the downgrade/upgrade cycle
  • .venv/bin/pytest apps/api/tests/unit/: 373 passed
  • pnpm test:web: 62 passed; pnpm build:web: ok
  • verify-source-lock, verify-scaffold, verify-p0-02, verify-p0-e2e, docker compose config: ok

🤖 Generated with Claude Code

corlin and others added 4 commits October 4, 2026 18:04
CaseService.create_case locked the quota row with SELECT ... FOR UPDATE.
Row locks need UPDATE privilege, and patent_evidence_app may only SELECT
organization_plan_quotas (0001), so every case creation failed with
"permission denied for table organization_plan_quotas" in any deployment
using the intended roles. Unit tests mock the database and the
integration test covering this path had never run, so it went unnoticed.

Serialize creations with the organization advisory lock instead
(pg_advisory_xact_lock, re-entrant; access.mutation already takes it) and
read the quota without a row lock. This keeps concurrent creations from
exceeding the allowance without granting the API role UPDATE on quotas.
Covered by test_case_lifecycle_and_quota_enforcement, which now runs.

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

scripts/test-postgres.sh ran a hand-kept list of 5 of the 14 integration
files, and CI skipped every Postgres test. The other 9 had never run:

- 6 business-flow tests (cases/documents, features, search, comparison,
  reports, full e2e) read PE_MIGRATION_DATABASE_URL, which nothing exports,
  so they always skipped. Their seeds targeted a schema the repo never had:
  a non-existent "credentials" table, organizations.persisted_status,
  memberships.identity_id, missing display_name (NOT NULL since the first
  schema commit), and positional calls to the keyword-only
  login_with_totp.
- test_p0_02_e2e_lifecycle truncated non-existent tables, called the
  bootstrap script with an old interface, and expected stale response
  keys and status codes.
- test_database_context relied on data seeded by another file.

Changes:
- test-postgres.sh runs apps/api/tests/integration/ as a whole; CI runs
  the script.
- test_support: reset_database (catalog-derived, cannot go stale),
  seed_two_tenant_baseline, seed_admin_tenants, seed_platform_admin (real
  schema); login_with_totp returns the secret, plus
  login_with_existing_totp for repeat logins; api_settings sets
  worker_database_url (the worker was connecting with default creds).
- Each module seeds its own data; the business tests keep their IDs,
  emails and quotas (the case test's allowance of 2 is preserved).
- E2E tests aligned with the implemented contracts, each verified against
  the API code or a passing test: provisioning needs Idempotency-Key,
  plan_key and a period; invitation_token is top-level; accept returns
  201; TOTP confirm returns "confirmed"; organization.status is the
  effective status; the handoff path is per strategy.
- The suspension check was not testing suspension: it never completed MFA
  (401) and then hit the login rate limiter (4/min, frozen clock). It now
  completes MFA after letting a minute pass and asserts the implemented
  rule: members of a non-active organization get 403, non-members 404.

Result: 91 passed, 0 skipped (was 84 passed with 9 files never executed).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- runbook-platform-bootstrap.md described an older script: PE_* variables,
  no arguments, a printed identity ID and TOTP secret, and a 10-minute
  window. The script takes email and display name, reads
  PATENT_EVIDENCE_* variables, prints no credential material, and gives a
  30-minute handoff (MFA_HANDOFF_LIFETIME) for TOTP and a first privileged
  action. local-development.md was already correct.
- TotpConfirmResult typed status as 'verified', but /mfa/totp/confirm
  returns 'confirmed'; /mfa/challenge returns 'verified'. No UI code
  branched on it; the types now match each endpoint.

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

# Conflicts:
#	scripts/test-postgres.sh
@corlin
corlin merged commit 55bb455 into main Oct 4, 2026
1 check passed
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