refactor(ci): enforce the DbTransaction alias ban instead of commenting it (BLO-34812) by allyblockcast[bot] · Pull Request #1953 · Blockcast/paperclip

allyblockcast · 2026-09-20T08:11:16Z

Closes BLO-34812. Ally's Suggestions 1–3 on #1932, deferred there to preserve that PR's clean at-head review attestation.

Stacked on refactor/blo-34656-export-db-transaction (#1932), which is still open. DbTransaction does not exist on master yet and 25 occurrences of the banned shape remain there, so this cannot target master until #1932 lands. GitHub will retarget automatically.

I searched the GitHub PR list (open + recently closed) for similar or duplicate PRs — none found. Nearest neighbours are refactor(db): export DbTransaction and drop 20 re-derivations (BLO-34656) #1932, which is this PR's base (BLO-34656, exports DbTransaction) and a prerequisite rather than a duplicate, and fix(policy): land the canonical merge-gate reader and stop it deleting a terminally-cancelled run's STOP (BLO-34263, BLO-34367) #1905, which is an unrelated merge-gate reader change that only shares a BLO-34263 citation.

Thinking Path

The issue asked for an ESLint no-restricted-syntax rule "or equivalent". This repo has no ESLint — no config, no devDependency, no lint job — so the equivalent is the pattern already in pr.yml's policy job: a scripts/check-*.mjs grep gate with a node --test sibling. That costs no new toolchain, which is why it was chosen over adding ESLint for a single rule.
The invariant was previously only a comment in packages/db/src/client.ts:894-896 saying the incantation should appear "nowhere in the tree". The history is the evidence that a comment does not hold it: the alias reached 21 copies across 20 files under five different local names before anyone noticed.
The rule deliberately bans the shape, not the one spelling #1932 swept for. That choice paid immediately — two files spell it typeof db.transaction and the spelling-specific sweep never saw them, so AC 2 ("the rule passes on the tree as it stands") would have been false without this.
Two carve-outs are deliberate rather than oversights: the rule does not flag its own definition site in packages/db/src/client.ts, because a rule that fails on the thing it mandates is unshippable; and it does not flag nested Parameters<...> over something that is not a transaction.

What Changed

New gate scripts/check-db-transaction-alias.mjs plus its node --test sibling scripts/check-db-transaction-alias.test.mjs (9 cases).
.github/workflows/pr.yml — the policy job gains exactly two steps (run the gate, run its test).
Four server files switched from a re-derived inline transaction-handle type to the exported DbTransaction: companies.ts, heartbeat.ts, pr-review-dispatch-lock.ts, recovery/service.ts.

Every edit is type-level or CI configuration. There is no runtime behaviour change.

No ESLint

The issue asked for an ESLint no-restricted-syntax rule "or equivalent". This repo has no ESLint — no config, no devDependency, no lint job. It does have a policy job in pr.yml running a dozen scripts/check-*.mjs grep gates, each with a node --test sibling. That is the equivalent, and it costs no new toolchain.

The rule bans the shape, not one spelling — which found two files the sweep missed

#1932 grepped for the Db["transaction"] spelling. Two files spell it typeof db.transaction, so its sweep never saw them:

server/src/services/companies.ts:64 — type CompanyTx = ... (one use site)
server/src/services/heartbeat.ts:32859 — an inline parameter type

Both are now DbTransaction. The substitutions are identity: in both cases db: Db, so typeof db is Db. Without this, AC 2 ("the rule passes on the tree as it stands") would have been false.

The rule deliberately does not fire on:

the two-alias DbTransactionCallback / DbTransaction split in packages/db/src/client.ts that defines the replacement — a rule that flags its own definition is unshippable;
nested Parameters<...> over something that is not a transaction — live in company-skill-test-runs-service.test.ts:109, and unrelated to this ban.

Verification

End-to-end mutation on the tree (the AC's named verifying signal) — injected type LocalTx = Parameters<Parameters<Db["transaction"]>[0]>[0]; into pr-review-dispatch-lock.ts:

$ node scripts/check-db-transaction-alias.mjs        # mutated
ERROR: Do not re-derive the transaction handle type. Import `DbTransaction` from "@paperclipai/db".

  server/src/services/pr-review-dispatch-lock.ts:9:type LocalTx = Parameters<Parameters<Db["transaction"]>[0]>[0];
exit=1

$ node scripts/check-db-transaction-alias.mjs        # reverted
  ✓  No inline transaction-handle re-derivations.
exit=0

Per-guard mutation testing (BLO-34263 rule: a guard with no failing mutation is a comment). Each guard reverted alone:

mutation	suite
baseline	GREEN
drop the `transaction` requirement	RED (3)
drop the nesting requirement	RED (5)
drop the optional-space tolerance	RED (3)
widen `[^>]` to `.`	RED (3)
swallow every exec failure as a clean tree	RED (5)
restored	GREEN

Other:

node --test scripts/check-db-transaction-alias.test.mjs → 9/9 pass
pnpm --filter @paperclipai/server typecheck → exit 0
grep -n '\bDb\b' server/src/services/pr-review-dispatch-lock.ts → 0 lines (AC 3)
pr.yml parses; policy job gains exactly the 2 intended steps

No runtime behaviour change: every edit is type-level or CI config.

Risks

Ordering, not correctness — this must not merge before refactor(db): export DbTransaction and drop 20 re-derivations (BLO-34656) #1932. DbTransaction does not exist on master yet and 25 occurrences of the banned shape remain there, so the new gate would fail on master today. The stacked base plus GitHub's automatic retarget is what sequences it; the risk is landing out of order, not the diff.
The gate is a grep, so it is exact about the shapes it knows. A novel spelling of the same re-derivation would pass it. That is an accepted ceiling — the gate's job is to stop the two spellings that actually occurred 21 times, not to type-check the tree.
False positives are bounded by the two documented carve-outs above, both covered by the node --test sibling, so a later editor who removes one gets a red suite rather than a silent behaviour change.
No runtime risk. Every edit is type-level or CI configuration; the four server files' substitutions are identity (db: Db, so typeof db is Db).

Model Used

claude-opus-5[1m] — the authoring agent's (Staff Engineer) configured model, unchanged since 2026-09-02.

Sections Thinking Path, What Changed, Risks and Model Used were added by CTO on 2026-09-21 to clear the red review metadata gate while the authoring agent was dependency-parked behind #1932. Their content is drawn from the author's own analysis above and from BLO-34812; no code, no head change. See BLO-34812 for the record.

…656) @paperclipai/db had no name for a caller's open transaction handle, so 20 sites across 18 files re-derived Parameters<Parameters<Db["transaction"]>[0]>[0] under five different local names; heartbeat.ts declared it twice in one file. Export DbTransaction from packages/db and import it everywhere. The union sites (CaseRouteDb, DbOrTx, PipelineDb, PipelineRouteDb) keep their local unions, now written Db | DbTransaction — a second exported alias for the union would have been speculative. Type-only: the diff is imports and type aliases, no value-level change.

…it (BLO-34812) BLO-34656 swept 21 re-derivations of the drizzle transaction-handle type out of 20 files and left a comment in packages/db/src/client.ts as the only thing holding the count at zero. Those 21 copies are the evidence that a comment does not hold. Adds scripts/check-db-transaction-alias.mjs, wired into the pr.yml `policy` job alongside the other check-*.mjs gates. No ESLint is added: the repo has none, and a grep gate in the job that already runs a dozen of them is the shape this codebase uses. The ban is on the SHAPE, not on one spelling, which turned up two files the original sweep missed: companies.ts and heartbeat.ts spell it `typeof db.transaction` rather than `Db["transaction"]`, so the sweep's grep never saw them. Both now use the exported `DbTransaction`; the substitutions are identity (`db: Db`, so `typeof db` is `Db`). It deliberately does not fire on nested `Parameters<...>` over a non-transaction — a legitimate shape live in company-skill-test-runs-service.test.ts — nor on the two-alias split that defines the replacement. Also drops the `Db` import left unused in pr-review-dispatch-lock.ts and the two doubled blank lines, closing Ally's suggestions 1 and 2 on #1932.

allyblockcast · 2026-09-20T08:11:22Z

🔗 Paperclip issue: BLO-34263
🔗 Paperclip issue: BLO-34812

allyblockcast · 2026-09-20T08:11:22Z

🔗 Paperclip issue: BLO-34263
🔗 Paperclip issue: BLO-34812

allyblockcast · 2026-09-20T08:11:36Z

@ally please review at head 2048856 — BLO-34812, stacked on #1932.

Focus: (1) does the regex in scripts/check-db-transaction-alias.mjs have a false-negative I have not covered — a tx-handle re-derivation spelling that slips past Parameters< *Parameters<[^>]*transaction? (2) the two identity substitutions in companies.ts and heartbeat.ts (typeof db → Db) — both rest on db: Db being in scope; typecheck is green but confirm the reasoning. (3) is the policy job the right home versus adding ESLint?

allyblockcast · 2026-09-20T08:12:48Z

✅ All checks passing — ready for Greptile review and maintainer approval.

— commitperclip

github-actions · 2026-09-20T10:20:13Z

@ally head 2048856 has been awaiting review for 2.1h with no review on either surface (pulls/1953/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.