Skip to content

docs: add testing strategy, migration workflow, error catalog, and security policy - #597

Merged
codebestia merged 2 commits into
codebestia:devfrom
Deb-Auth:docs/testing-migrations-security
Aug 31, 2026
Merged

docs: add testing strategy, migration workflow, error catalog, and security policy#597
codebestia merged 2 commits into
codebestia:devfrom
Deb-Auth:docs/testing-migrations-security

Conversation

@Deb-Auth

@Deb-Auth Deb-Auth commented Aug 30, 2026

Copy link
Copy Markdown

Description

Adds four contributor-facing documents that were missing from the repository, plus the matching rows in docs/README.md (the index requires every new .md file to be listed).

Documentation only — no source, schema, config, or workflow changes.

docs/testing.md — testing strategy and conventions (#545)

The cross-app testing philosophy and the conventions a contributor has to follow.

  • The no-external-services rule, stated explicitly with its reasoning: a test that needs Docker is a test nobody runs; a shared real Postgres or Redis makes the suite order-dependent; speed is what keeps contributors running the whole suite rather than one file. Approved substitutes are given per dependency — ioredis-mock for Redis (and null for the deliberate Redis-is-down fallbacks), a hand-built db mock for Postgres, LocalDiskObjectStore for S3/MinIO, fake-indexeddb plus the apps/web/src/test/setup.ts shims for the web suite, the pytest-mock fixtures in conftest.py for the AI agent, and Env::default() for contracts.
  • Notes that the backend CI service containers exist for the pnpm db:migrate step, not for the tests, and that security-ci.yml — which runs backend tests with no containers at all — is the standing proof the suite is service-free.
  • Per-app runners and commands: vitest for backend and web, pytest for the AI agent, cargo test for contracts, with the config file for each and the include/exclude patterns that decide which files get collected (including why dist/ is excluded, and that the web include silently skips .tsx test files).
  • The Drizzle mocking pattern: mocking ../db/index.js, ../db/schema.js, and drizzle-orm together, and why the module under test is imported with await import(...) after the mocks rather than a static import.
  • Why .values() sometimes has to be both thenable and expose .returning(): Drizzle's insert builder is itself awaitable and chainable. The message insert calls .returning() because it needs the generated id and createdAt; the envelope batch insert just awaits .values(...). A stub returning { returning } alone makes the awaited call resolve to a plain object and silently record nothing — the test passes while asserting on an insert that never happened. A stub returning only a promise makes .returning() throw. Includes the canonical dual-shape stub from e2ee.integration.test.ts, plus two cautions about the recording side effect firing twice.
  • Socket handlers go through the enveloped dispatch event, never a raw socket.on(type, ...) listener — there is no raw listener to grab, and a test written that way bypasses envelope validation, the auth gate, and eventId idempotency, which is exactly the surface those checks protect. Gives the dispatchEvent helper and the four things to preserve when copying it (unique eventId, current timestamp, socket.auth set first, register through the real registrar).
  • The shared in-process rate-limit counter trap: services/rateLimiter.ts keeps a localCounters map used whenever Redis is unavailable — which, in a suite that mocks redis to null, is always. Several tests hitting the same endpoint as the same subject share one budget, so a test passes alone and returns 429 in file order. Documents clearLocalRateLimitCounters() / resetRateLimitBucket(bucket) and the equivalent reset hooks for the prekey low-watermark latches and the presence offline-broadcast set.

apps/backend/docs/migrations.md — database migration workflow (#546)

  • schema.ts is the source of truth; migrations are generated from it and never hand-written first, because drizzle-kit diffs against the snapshot rather than the database and will re-emit or undo a change it has no record of. The one legitimate exception (backfills, CREATE INDEX CONCURRENTLY) is to edit the generated file, not to add an ungenerated one.
  • The full loop: edit → pnpm db:generatereview the emitted SQL (renames that arrive as drop-plus-add, NOT NULL without a default, type changes needing USING) → pnpm db:migrate, with a warning about db:push.
  • The drizzle/ layout: the .sql files, meta/_journal.json, and the per-migration snapshots. Calls out that _journal.json decides what actually runsdb:migrate reads the journal, not the directory listing, so a .sql file with no journal entry is silently skipped with no error or warning.
  • The merge hazard, concretely. drizzle-kit numbers from "one past the highest index I see" and knows nothing about other branches, so parallel branches produce colliding prefixes and duplicated journal entries. This is documented against the incident already in this repository's history: before d60b648, drizzle/ held seven distinct 0001_*.sql files, and the journal's idx: 1 entry contained three duplicate when/tag key pairs inside one object. Duplicate JSON keys are not an error — last one wins — so that entry ran exactly one migration. Thirteen .sql files, four journal entries: nine migrations silently skipped, and the history had to be squashed back to a single 0000_lean_scrambler.sql baseline.
  • The recommended resolution procedure: merge schema.ts on its own terms, take the base branch's drizzle/ wholesale, delete your own branch's migration and snapshot, regenerate, review, and verify against a scratch database. Plus five mechanical checks that catch a collision before it lands.
  • Notes that drizzle/meta/ is Prettier-ignored because it is generated output that drizzle-kit rewrites in its own format on the next generate.

SECURITY.md — vulnerability disclosure policy (#548)

For an E2EE product with on-chain funds, a finder currently has no private option at all.

  • A private channel — GitHub private vulnerability reporting (/security/advisories/new), with a maintainer-contact fallback — and response windows: acknowledgement in 3 business days, triage in 10, updates every 14 days, fix or documented mitigation for a confirmed high-severity issue within 90 days of triage.
  • Explicitly forbids public reporting via issues, pull requests, or discussions, with the reasoning: a public issue is an exploit advertisement visible from the moment it is filed, and a PR is worse because the diff explains the bug.
  • Scope, component by component: backend auth/authorization/ciphertext invariants/presigned URLs, web client crypto (X3DH, double ratchet, MLS, key handling, file encryption, identity trust), contracts (authorization, treasury and voting logic, arithmetic, the admin-gated upgrade), plus the AI agent and supply chain — with an explicit out-of-scope list and testing rules.
  • Cross-links docs/threat-model.md so reporters can distinguish a documented, accepted residual metadata risk (social graph, traffic analysis, presence patterns, device fingerprinting) from a real finding — and says plainly that content, key material, or session state being observable when the threat model says otherwise is exactly what we want to hear about.
  • A contract-specific path, because on-chain bugs may not be fixable by a redeploy: only token_transfer has an upgrade entrypoint, while group_treasury and proposals expose none at all, so fixing either means deploying a new contract, migrating state, and repointing every consumer. Funds already in a vulnerable instance may be unrecoverable. Reproduce in Env::default() or on testnet — never against a live treasury — and the strongest proof of concept is a failing #[test].

apps/backend/docs/contracts-error-catalog.md — error code and response catalog (#555)

Every error shape the backend returns, across both transports, in one place. There was previously nowhere to look up what a client can actually receive.

  • One REST table, deduplicated to a row per distinct (status, error) pair, giving the extra fields each carries and every route that emits it. Middleware errors (requireAuth, validate, transportSecurity, rateLimit) are split into their own table since they can accompany any authenticated route. Also covers the statuses that never appear as a literal res.status(...) call: the validateMessagePayload results behind POST /messages, and the checkEnvelopeProtocols 400/409 pair with its violations[] shape.
  • One socket table keyed by the emitted event value, including the non-obvious ones the issue calls out — device_set_mismatch (envelopes missing for the sender's own sibling devices), protocol_mismatch, rate_limited, and envelope_too_large — plus device_revoked and payload_too_large from the connection middleware. Two traps are documented explicitly: rate_limited carries the throttled event in limitedEvent, not event, and envelope_too_large is a code value that arrives inside a normal send_message payload rather than an event value of its own.
  • The two distinct socket error payload shapes. The dispatcher wraps its rejections in the standard event envelope ({ eventId, type, timestamp, payload }), while the security middleware and every messaging handler emit a bare { event, message, code? }. A client must handle both; the doc includes the normalising snippet. It also notes that three of the four dispatcher errors carry no event field at all, so handlers must not key on its presence.
  • Handshake failures, which are not error events — they reject during the Socket.IO handshake and surface on connect_error as a plain Error. A client listening only on error sees nothing for a bad token, a revoked device, or a blocked origin.
  • The rate-limit response shape, which turns out not to be one shape: the standard rateLimit() middleware sets RateLimit-Limit/Remaining/Reset plus Retry-After; the upload byte quota sets only Retry-After; the group-invite throttle sets no headers at all. The doc gives the safe client rule for reading a backoff from any of the three, and notes that RateLimit-* are also set on successful responses so a client can back off before being rejected.
  • Retryable vs terminal, including the cases where a naive retry is actively wrong: an auth nonce is single-use, so a 401 there means restarting from POST /auth/challenge rather than resubmitting; a message retry must reuse the same messageId or a committed-but-failed-looking send duplicates the message; and an MLS Commit epoch conflict must be rebuilt against the currentEpoch returned in the response.

Two findings surfaced while cataloguing. Both are documented rather than changed, since this PR is documentation only:

  • There is no global Express error handler. An exception escaping a route handler falls through to Express's built-in handler and returns an HTML error page with a 500, not { error } — so a client that assumes every non-2xx body is JSON will throw while parsing.
  • auditLogsRouter is never mounted. It is defined in src/routes/auditLogs.ts and exercised in auditLog.test.ts, but app.ts does not mount it, so its two errors are currently unreachable over HTTP. Listed in the catalog and flagged rather than silently presented as live.

Type of change

  • Bug fix
  • New feature
  • Documentation update
  • Other

Checklist

  • I have read the contributing guidelines
  • I have tested my changes locally
  • My code follows the project's coding standards

Verification

  • npx prettier --check passes on all four changed files. The repo-wide pnpm format:check reports a large number of pre-existing failures on files this PR does not touch; none of them are from this branch.
  • Every relative markdown link in the three new files and in the added index rows was resolved against the working tree — no broken links.
  • The error catalog was extracted mechanically from the source rather than transcribed by hand: every res.status(...).json(...) call across src/routes/ and src/middleware/ (180 call sites, deduplicated to 120 distinct pairs) and every socket.emit('error', ...) / createEnvelope('error', ...) site was parsed, then each was mapped back to its enclosing route. Two parser artefacts were hand-corrected against the source, and the dynamic-status call sites were expanded manually.
  • Every technical claim was checked against the source rather than assumed: the .values()/.returning() dual shape against e2ee.integration.test.ts, the dispatch-only handler path against socket/dispatcher.ts, the reset hooks against services/rateLimiter.ts, services/prekeyLowSignal.ts, and services/presence.ts, the journal incident against git show d60b648^:apps/backend/drizzle/meta/_journal.json, and the contract upgradeability claims against contracts/docs/concepts-upgrades.md.
  • The central claim of docs/testing.md was verified empirically by running pnpm --filter backend test on a machine with Docker stopped and no Postgres, Redis, or MinIO running.
  • docs/README.md was edited by hand rather than reformatted, so its diff is 8 added rows and nothing else.

Notes for the maintainer

  • SECURITY.md needs one repository setting to work. GitHub private vulnerability reporting must be enabled (Settings → Code security and analysis → Private vulnerability reporting), otherwise the advisory URL in the policy 404s for reporters. If you would prefer a security email address as the channel instead, say the word and I will swap it.
  • The response windows are a proposal rather than a commitment I can make on your behalf — happy to adjust any of the four.

Closes #545
Closes #546
Closes #548
Closes #555

@drips-wave

drips-wave Bot commented Aug 30, 2026

Copy link
Copy Markdown

@Deb-Auth Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

Adds three documents covering gaps in the contributor-facing documentation,
plus the matching rows in the documentation index.

docs/testing.md - cross-app testing philosophy and conventions:

- States the no-external-services rule explicitly, with the reasoning and
  the approved substitute for each dependency (ioredis-mock for Redis, a
  hand-built db mock for Postgres, LocalDiskObjectStore for S3/MinIO,
  fake-indexeddb and the web setup shims, pytest-mock fixtures for the AI
  agent, and the Soroban Env harness for contracts).
- Documents the per-app runners and commands: vitest for backend and web,
  pytest for the AI agent, cargo test for contracts, plus the include and
  exclude patterns that decide which files are collected.
- Explains the established Drizzle mocking pattern, including why the
  module under test is imported with a dynamic import after the mocks, and
  why .values() sometimes has to be both thenable and expose .returning():
  the message insert chains .returning() for the generated id while the
  envelope insert only awaits .values(), so a stub supporting one shape
  silently records nothing for the other.
- Documents that socket handlers must be driven through the enveloped
  dispatch event rather than a raw socket.on listener, since there is no
  raw listener and grabbing one bypasses envelope validation, the auth
  gate, and eventId idempotency.
- Notes the shared in-process rate-limit counters that leak between tests,
  with the reset hooks for that and the other module-level state.

apps/backend/docs/migrations.md - drizzle-kit migration workflow:

- Establishes schema.ts as the source of truth and migrations as generated
  output, never hand-written first, with the one documented exception.
- Documents the drizzle/ layout and meta/_journal.json as the file that
  decides what actually runs, and that a .sql file missing from it is
  silently skipped.
- Documents the merge hazard concretely against the incident already in
  this repository's history: seven colliding 0001_* files and a journal
  entry with duplicate JSON keys, which left nine migrations unlisted and
  forced a squash back to a single baseline.
- Gives the recommended conflict-resolution procedure - merge schema.ts,
  take the base branch's drizzle/ wholesale, drop your own migration, and
  regenerate - plus the checks that catch a collision before it lands.
- Notes that drizzle/meta/ is Prettier-ignored because it is generated
  output.

SECURITY.md - vulnerability disclosure policy:

- Gives GitHub private vulnerability reporting as the private channel,
  with acknowledgement, triage, update, and fix windows.
- States explicitly that vulnerabilities must not be reported through
  public issues, pull requests, or discussions, and why.
- Defines scope across the backend, web client crypto, contracts, AI
  agent, and supply chain, with an explicit out-of-scope list and testing
  rules.
- Cross-links docs/threat-model.md so reporters can tell an accepted
  residual metadata risk from a real finding.
- Adds a contract-specific path: only token_transfer is upgradeable, so a
  bug in group_treasury or proposals cannot be fixed by a redeploy and
  funds already in a vulnerable instance may be unrecoverable.

Closes codebestia#545
Closes codebestia#546
Closes codebestia#548
@Deb-Auth
Deb-Auth force-pushed the docs/testing-migrations-security branch from 921ecff to 5ed1162 Compare August 31, 2026 00:02
Adds apps/backend/docs/contracts-error-catalog.md, cataloguing every error
shape the backend returns across both transports, plus the matching rows in
the documentation index.

Contents:

- One REST table, deduplicated to a row per distinct (status, error) pair,
  listing the extra fields each carries and every route that emits it.
  Middleware-level errors are split into their own table since they can
  accompany any authenticated route.
- The dynamic REST statuses that do not appear as literal res.status calls:
  the validateMessagePayload results behind POST /messages, and the
  checkEnvelopeProtocols 400/409 pair with its violations array.
- Rate-limit responses, which do not have one shape: the standard
  rateLimit() middleware sets RateLimit-Limit/Remaining/Reset plus
  Retry-After, the upload byte quota sets only Retry-After, and the
  group-invite throttle sets no headers at all. Documents the safe client
  rule for reading a backoff from any of the three.
- Socket errors, keyed by the emitted event value, including the
  condition-valued ones: device_set_mismatch, protocol_mismatch,
  rate_limited, envelope_too_large, device_revoked, and payload_too_large.
  Notes that rate_limited carries the throttled event in limitedEvent
  rather than event, and that envelope_too_large is a code value rather
  than an event value.
- The two distinct socket error payload shapes: the dispatcher wraps its
  rejections in the standard event envelope, while the security middleware
  and every messaging handler emit a bare payload. Includes the normalising
  snippet a client needs to handle both.
- Handshake failures, which arrive on connect_error as a plain Error rather
  than as an error event, so a client listening only on error sees nothing.
- A retryable/terminal breakdown, including the cases where a naive retry
  is actively wrong: an auth nonce is single-use so a 401 there means
  restarting from the challenge, a message retry must reuse the same
  messageId to stay idempotent, and an MLS epoch conflict must be rebuilt
  against the returned currentEpoch.

Two findings surfaced while cataloguing, documented rather than changed:
there is no global Express error handler, so an exception escaping a route
returns Express's HTML page instead of a JSON body; and auditLogsRouter is
defined and tested but never mounted in app.ts, so its two errors are
currently unreachable over HTTP.

Closes codebestia#555
@Deb-Auth Deb-Auth changed the title docs: add testing strategy, migration workflow, and security policy docs: add testing strategy, migration workflow, error catalog, and security policy Aug 31, 2026
@codebestia
codebestia merged commit 7a7ee71 into codebestia:dev Aug 31, 2026
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