[patrickbrandao]

Closes #108

What

A unit-suite check that no backend e2e spec can wipe a database without going through the #67 guard.

• packages/backend/test/e2e-db-guard-scan.ts: parses every .ts file under packages/backend/test with the TypeScript AST, so comments never count, and reports three rules:

  • unguarded-drop: the file wipes the database without calling resolveE2eDatabaseUrl() or createTestApp(). Wiping means dropSchema set to anything but a literal false (shorthand included), DROP SCHEMA/DROP DATABASE in a string or template, .dropDatabase() or .synchronize(true). A call is required; an import alone does not count.
  • database-url-fallback: process.env.DATABASE_URL ?? ... or || ....
  • default-database: a string naming localhost:5432 or mydatabase. e2e-db-guard.spec.ts is exempt, since it feeds those URLs to the guard to prove they are refused.

• packages/backend/test/e2e-db-guard-scan.spec.ts: runs in the unit suite (npm test, the jest roots already include test/). It does three things:

  • asserts the current tree is clean and that the walker actually sees the specs that drop the schema;
  • runs the negative fixtures;
  • copies the legacy fixture into a temp-dir tree and proves the tree scan then fails, naming file, line and rule.

• packages/backend/test/fixtures/e2e-db-guard/*.fixture holds three fixtures:

  • the legacy migration-spec pattern from refactor(backend): remove billing, Stripe and plan limits #52/refactor(backend): remove waitlist, CRM, Gemini Free, free models, GitHub stars and error pages #60/feat(backend)!: remove Autofix and the Phoenix healer from the backend #66: a ?? 'postgresql://...localhost:5432/mydatabase' fallback plus dropSchema: true;
  • a raw DROP SCHEMA reset;
  • the accepted guarded pattern.

  Their .fixture extension keeps them out of the scan, out of Jest and out of tsc.

• docs/dev/manifest-guide.md: the "Backend e2e database safety" section now describes the check.

The current tree passes with no exemption other than the guard's own spec.

Tests

Command	Result
npm run build --workspace=packages/shared	ok
npx tsc --noEmit -p tsconfig.json (backend)	ok
npx tsc --noEmit --strict ... test/e2e-db-guard-scan.ts test/e2e-db-guard-scan.spec.ts (test dir is outside the backend tsconfig)	ok
npm run lint --workspace=packages/backend	0 errors (323 existing warnings in src/, unchanged)
npx prettier --check on the new files	ok
npx jest (backend unit)	473 suites, 9066 tests passed
TZ=UTC npm run test:e2e:docker --workspace=packages/backend	70 suites, 592 tests passed

Test-only change. No runtime behavior changes, so qa-docker is not needed.

🤖 Generated with Claude Code

[patrickbrandao]

Review of #111: test(backend): fail the unit suite when an e2e spec bypasses the database guard

I reviewed git diff origin/main...527978fa80952daa7c7d19ef79a4eb9acff6c74e in a fresh detached worktree. The PR is one commit and changes 6 files: the scanner (test/e2e-db-guard-scan.ts), its spec, three .fixture files and a section of docs/dev/manifest-guide.md.

Summary

Nothing blocks this PR. I have one should-fix and four nits.

The PR is not high risk. It doesn't touch auth, crypto or key handling, has no migration, doesn't affect tenant isolation, and doesn't change governance files, .github/ or a release. It adds no outbound destination: the scanner only reads files and parses them with typescript. No product code changes, so I agree that qa-docker and a changeset aren't needed. The title follows Conventional Commits, and the diff adds no new manifest/mnfst identifiers.

Does it meet issue #108?

• Patterns from the issue are caught.
  • dropSchema: true without a call to resolveE2eDatabaseUrl()/createTestApp() is caught, including dropSchema: <expr>, 'dropSchema': true, shorthand and spread-in objects.
  • Raw DROP SCHEMA/DROP DATABASE is caught in string literals, templates and tagged templates.
  • process.env.DATABASE_URL ?? ... and || ... are caught, in both property and element access forms.
  • Strings naming localhost:5432 or mydatabase are caught.
  • An import of the guard without a call is not enough. Comments never count, because the scanner walks the AST.
• Negative test with fixtures. The legacy fixture reproduces the refactor(backend): remove billing, Stripe and plan limits #52/refactor(backend): remove waitlist, CRM, Gemini Free, free models, GitHub stars and error pages #60/feat(backend)!: remove Autofix and the Phoenix healer from the backend #66 pattern and asserts all three rules with their lines (6 and 15). The raw-SQL fixture covers DROP SCHEMA plus a || fallback. fails a tree scan when a legacy spec is added copies the legacy fixture into a temp tree and checks the test/nested/...:15 [unguarded-drop] output. The .fixture extension keeps these files out of the walker, out of testRegex in both Jest configs and out of tsc, since tsconfig.json excludes test.
• No false positives on the current tree. The tree test passes. The only exemptions are e2e-db-guard.spec.ts, and only for the default-database rule, and the scanner's own spec. A test checks that both exempt files exist. Every current file that drops a schema calls the guard or createTestApp(), and helpers.ts routes dropSchema through resolveE2eDatabaseUrl().
• It runs in CI. The unit Jest config has roots: [src, ../test] and testRegex: .*\.spec\.ts$, and the e2e config (.e2e-spec.ts$) doesn't pick it up. In the CI log of the Unit (backend) job on this SHA, I saw PASS test/e2e-db-guard-scan.spec.ts and Test Suites: 473 passed. typescript is a direct devDependency of the backend, so the import doesn't rely on hoisting.
• Bypass resistance. I ran 31 probe sources through scanSource. Details are below. For a defense-in-depth check whose main backstop is still the globalSetup guard, the coverage is reasonable. The one gap I think should be closed is S1.

Should-fix

S1. packages/backend/test/e2e-db-guard-scan.ts:115-117: TypeORM's dropSchema() method is not treated as a wipe.
.dropDatabase() counts as a wipe, but its sibling doesn't. queryRunner.dropSchema('public', true, true) is TypeORM's QueryRunner API for DROP SCHEMA ... CASCADE, and it passes the scanner (probe: await qr.dropSchema('public', true, true); gives []).

Failure scenario: a new migration spec opens its own DataSource with a URL that didn't go through the guard, such as another env var, or runs outside the e2e config. It resets with queryRunner.dropSchema('public', true, true). The scan stays green, so this is exactly the kind of spec #108 is meant to catch.

Fix: also push a drop when name === 'dropSchema' on a property-access call, and add a case to flags other ways to wipe the database.

Nits (no need to fix before merge)

N1. e2e-db-guard-scan.ts:114,171: the guard counts per file, not per connection.
A single createTestApp() or resolveE2eDatabaseUrl() call anywhere in a file approves every drop in it. For example, app = await createTestApp(); new DataSource({ url: process.env.OTHER_URL, dropSchema: true }) passes. So does a file that defines its own local createTestApp(). This matches the issue's wording ("sem resolveE2eDatabaseUrl()/createTestApp()"), and a second DataSource on DATABASE_URL is still covered at runtime by globalSetup. Still, consider either of these:

• documenting the limit in the header comment;
• later, requiring that createTestApp be imported from ./helpers.

N2. e2e-db-guard-scan.ts:39,138-141: the SQL rule looks at one string at a time.
These forms all escape it:

• concatenation: 'DROP ' + 'SCHEMA public';
• a template that splits the keyword: `DROP ${'SCHEMA'} ...`;
• SQL built from constants.

TRUNCATE, DROP TABLE and DROP OWNED BY CURRENT_USER CASCADE aren't covered either. The last one wipes as effectively as DROP SCHEMA. These forms are unlikely by accident and fall outside the issue's scope. DROP OWNED would be a cheap addition to the regex.

N3. e2e-db-guard-scan.ts:68-87,153-157: the fallback rule only sees ?? and ||.
These forms aren't flagged:

• process.env.DATABASE_URL ??= '...' and ||=;
• a ternary;
• a destructuring default (const { DATABASE_URL = '...' } = process.env);
• an alias (const env = process.env; env.DATABASE_URL ?? ...);
• process?.env?.DATABASE_URL ?? ....

When the fallback names localhost:5432/mydatabase, default-database still catches it, so only a fallback to some other URL escapes. ??=/||= are the most likely of these, and adding QuestionQuestionEqualsToken/BarBarEqualsToken to the check is a one-line change.

N4. e2e-db-guard-scan.ts:185-199: the walker scans only .ts under test/.

• test/support/*.cjs exist today and aren't scanned. The TS parser reads JS too, so .js/.cjs/.mjs could be included.
• test/jest-e2e.json has rootDir: ".." and testRegex: ".e2e-spec.ts$", so an *.e2e-spec.ts placed under src/ would run in the e2e suite without being scanned. There are none today, and the issue scopes the check to test/**. A one-line assertion would close this: no *.e2e-spec.ts outside test/.

Probe results (scanSource, rules returned)

Source	Result
spread { ...opts } with opts = { dropSchema: true }, dropSchema: undefined, !false, tagged template sql`DROP SCHEMA ...`, synchronize( true ), locally defined but uncalled createTestApp	unguarded-drop (caught)
qr.dropSchema('public', true, true)	none (S1)
createTestApp() plus a second DataSource with dropSchema: true; local createTestApp that drops	none (N1)
'DROP ' + 'SCHEMA', `DROP ${'SCHEMA'}`, TRUNCATE, DROP TABLE, DROP OWNED	none (N2)
??=, ||=, ternary, destructuring default, env alias, process?.env?.	none (N3)
/DROP SCHEMA/ regex literal, /* DROP SCHEMA */ comment	none (correct)
'postgresql://u:p@127.0.0.1:5432/postgres'	none (out of scope; the issue names localhost:5432)

Tests

• PR report: the PR lists shared build, backend tsc --noEmit, strict tsc on the new files, backend lint, prettier, backend unit (473 suites, 9066 tests) and test:e2e:docker (70 suites, 592 tests), all passing.
• CI on 527978f: every check-run is success: Lint, Unit (backend/frontend/shared/cli), Backend e2e (PostgreSQL 16), Compose smoke test, install.sh end-to-end, CodeQL and dependency-review.
• My local runs in the review worktree:
  • npx jest test/e2e-db-guard: 2 suites, 53 tests passed;
  • npx tsc --noEmit --strict --target es2022 on the two new files: clean;
  • npx prettier --check on the two new files: clean.
• qa-docker: not needed. This is a test-only change.

Docs

The new section of docs/dev/manifest-guide.md is in English. That is consistent with the rest of that inherited guide, including the #67 section right above it. It describes the rules and exemptions accurately.

Merge condition

Under aprovado, S1 must be fixed in this PR or tracked in an issue whose link is recorded in the PR before merge. N1-N4 are optional.

Head revisado: 527978f
Veredito: aprovado

[patrickbrandao]

Re-review of #111 at c1a95fa

This re-review covers the delta git diff 527978f c1a95fa (commit c1a95fa), which I read in a fresh detached worktree. The full PR is still 527978f plus this commit, and the delta touches only test/e2e-db-guard-scan.ts and its spec. My previous review of 527978f still applies to everything else.

S1: fixed

packages/backend/test/e2e-db-guard-scan.ts:115-120 now treats .dropSchema(...) as a wipe, the same way it treats .dropDatabase(). It only does so on a property-access call, so a bare function named dropSchema doesn't count. The detail reads .dropSchema(). e2e-db-guard-scan.spec.ts:135-137 adds await queryRunner.dropSchema('public', true, true); and expects ['unguarded-drop']. The current tree has no .dropSchema( calls, so the change adds no false positives. The tree test still passes.

New finding

N5 (nit): the change isn't described in the header comment or the guide. e2e-db-guard-scan.ts:9 and docs/dev/manifest-guide.md:302 still list .dropDatabase() as the only method-call wipe. Adding .dropSchema() to both keeps them accurate. This doesn't need to happen before merge.

N1-N4 from the previous review stay optional and unaddressed, as agreed. Nothing blocks the PR, and no should-fix is left open.

The PR is still not high risk. The change is test-only: no auth, crypto, migration, isolation, governance, workflow or release files, and no outbound calls. qa-docker and a changeset aren't needed.

Tests

• CI on c1a95fa: every check-run is success: Lint, Unit (backend/frontend/shared/cli), Backend e2e (PostgreSQL 16), Compose smoke test, install.sh end-to-end, CodeQL and dependency-review. The Unit (backend) log shows PASS test/e2e-db-guard-scan.spec.ts and Test Suites: 473 passed.
• My local runs in the review worktree:
  • npx jest test/e2e-db-guard: 2 suites, 53 tests passed, including the tree scan;
  • strict tsc --noEmit --target es2022 on the two files: clean;
  • prettier --check on the two files: clean.

Head revisado: c1a95fa
Veredito: aprovado