ncmec: tighten email field role from STRING to EMAIL_ADDRESS scalar (#842)
* ncmec: tighten `email` field role from STRING to EMAIL_ADDRESS scalar
Follow-up to #840 and #841. Now that `@roostorg/coop-types@2.4.0` ships
a dedicated `EMAIL_ADDRESS` scalar, switch the email field role plumbing
to require it instead of any STRING-typed field. Mirrors the two-PR
pattern PR #559 → PR #583 used for `IP_ADDRESS`.
- Bump `@roostorg/coop-types` to ^2.4.0 in server and client
- Add `EMAIL_ADDRESS` to `ScalarType`, `SignalInputType`, `FieldType`
SDL enums; regenerate codegen
- Add `[ScalarTypes.EMAIL_ADDRESS]` handler in `fieldTypeHandlers.ts`
(string coerce + trim; null on empty; format validation deferred per
Caleb's note on #840)
- `FieldRoleToScalarType.email`: STRING → EMAIL_ADDRESS
- Client `schemaFieldRolesFieldTypes.EMAIL`: String → EmailAddress
- Migration: drop the `valid_email_field_field_type` CHECK constraint
(which required STRING) and replace with one requiring EMAIL_ADDRESS
- Exhaustive-switch updates for `ManualReviewJobFieldsComponent` and
`RuleTestModal`
- `EmailAddressArbitrary` for fast-check test fixtures, with culturally
diverse example names on RFC 2606 reserved domains
Migration safety: any existing email_field mapping to a STRING field
will fail the new constraint. #840 merged hours before this; no adopter
should have configured the role yet. Operators between #840 and this
migration who did configure it will need to either change their field
type to EMAIL_ADDRESS or null out the mapping before applying.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* client: add EMAIL_ADDRESS cases to three exhaustive switches
CI on #842 failed `npm run lint` (= `tsc --noEmit`) because widening
`ScalarType` to include `EMAIL_ADDRESS` left three client switches
incomplete. The `JsonValue` errors in `itemTypeCodeSampleUtils.ts`
were downstream of `generateFakeScalarFieldValue` not handling the
new scalar.
- `signal.ts`: comparators for `EmailAddress` mirror `String` / `Url` / `IpAddress`
- `RuleInsightsSamplesTable.tsx`: stringify (mirrors `IpAddress`)
- `itemTypeUtils.ts:generateFakeScalarFieldValue`: returns a fake email
on RFC 2606 `example.com`
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* ncmec: data-migrate stale email_field mappings before tightening CHECK
Addresses Tao's review on #842: the original "no adopter has had time to
configure the role" assumption was wrong. Deployments between #840 and
this migration may have mapped `email_field` to a STRING field; the new
CHECK constraint would reject those rows and block the migration entirely.
Add a DO-block data migration that runs in the same transaction before
the CHECK swap. For each item_type whose `email_field` doesn't point at
an EMAIL_ADDRESS-typed field in `fields`, NULL out the mapping and emit
a NOTICE naming the row (id, name, org_id) so operators can reconfigure
via the admin UI if needed. The underlying STRING field's data is left
untouched; only the role pointer is cleared.
Header comment updated to reflect the new data-migration step.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* db: forward postgres NOTICE events to console.warn during migrations
Addresses Tao's review on #842: the migration's `RAISE NOTICE` lines
were being swallowed because Sequelize's pg dialect doesn't surface
server-side messages by default. Operators running `npm run db:update`
saw the migration succeed but had no signal about which adopter
configurations had been cleared.
Wire a Sequelize `afterConnect` hook in `pg-base.ts` that attaches a
`notice` listener to every pg client the pool opens, forwarding each
message to `console.warn` prefixed with the severity. Future migrations
that use `RAISE NOTICE` (or `RAISE WARNING`) will also benefit, not just
this one.
Update the migration's header comment to direct operators to scan the
`db:update` output for `[postgres NOTICE]` lines after applying, so the
cleared mappings don't go unnoticed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* ncmec: wire HMA hashes into outgoing report's fileDetails.originalFileHash (#844)
NCMEC's CyberTipline accepts an `originalFileHash[]` element with a
`hashType` attribute per entry. HMA already computes per-image hashes
at item-submission time (`HMAHashBankService.hashContentFromUrl`,
stored on the image object alongside `url`), but the NCMEC report
builder discarded them.
- Add `OriginalFileHash` type and `originalFileHash?: OriginalFileHash[]`
to `FileDetails` (positioned after `industryClassification`, before
`ipCaptureEvent`, in line with NCMEC XSD ordering for forensic fields).
- Add `hashes?: Record<string, string>` to the internal `Media` type so
the report assembly can pass them through to `#upload`.
- `buildSubmitReportParamsFromDecision` now extracts hashes from the
matching image in the item data (walks scalar, array, and map
containers). New `extractHashesForUrl` helper.
- `#upload` converts the hashes via `toOriginalFileHashes` and includes
the resulting array on `fileDetails` when non-empty.
- New `toOriginalFileHashes` exported helper handles trimming,
empty-value filtering, and the algorithm-name uppercasing for the
`hashType` attribute.
Tests:
- 4 new in `buildSubmitReportParamsFromDecision.test.ts` covering the
scalar IMAGE case, ARRAY-of-IMAGE container traversal, URL-mismatch
omission, and missing-hashes omission.
- 4 new in `ncmecReporting.test.ts` for `toOriginalFileHashes`'s
precedence rules (uppercasing, trimming, empty-value drops,
undefined-on-empty).
This is one of the P1 items from the audit in #843.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* db: also lower client_min_messages so RAISE NOTICE actually surfaces
Initial fix only attached a notice listener, but verified locally that
NOTICE messages from RAISE NOTICE inside PL/pgSQL DO blocks were still
being filtered out. Root cause: Sequelize raises `client_min_messages`
to WARNING by default, which makes postgres filter NOTICE-severity
messages server-side before they reach the client. The listener was
correctly attached but never fired for NOTICEs.
In the same afterConnect hook, also issue `SET client_min_messages =
'notice'` so postgres sends NOTICE messages to the client. Verified
locally: per-row NOTICEs from the email-field-clearing migration now
appear in `db:update` output as expected.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* ncmec: wire webhook fileDetails.hash into outgoing report's originalFileHash (#850)
Closes the follow-up gap noted in PR #844. The additional-info webhook
can return a single `{ hash, hashType }` per media item; #844 wired
HMA-sourced hashes through but webhook-sourced hashes were still
dropped because:
1. `MediaAdditionalInfo` (the internal type that `#upload` consumes)
didn't declare `fileDetails`; the validation-schema-typed
`NcmecAdditionalInfoResponse` has it, but the data was erased at
the boundary between webhook response and internal use.
2. `toOriginalFileHashes` took only the HMA map as input.
Changes:
- Add `fileDetails?: { hash; hashType }` to `MediaAdditionalInfo`.
- Refactor `toOriginalFileHashes` to take both sources via an opts
object (`hmaHashes`, `webhookFileDetails`). Hashes from both are
combined and deduped on (`hashType` uppercase, trimmed hash value)
so a webhook returning the same algorithm as HMA produces one entry
in the outgoing report, not two.
- Update `#upload` call site to pass both sources.
- Remove dead `fileDetails: { ipCaptureEvent: [] }` from the
no-webhook fallback; it never matched any consumed shape and
nothing read it (the `ipCaptureEvent` lives at the top level of
`MediaAdditionalInfo`, not nested).
Tests: 6 new cases covering webhook-only, both sources, exact-match
dedup, same-algorithm-different-hash kept-both, whitespace-only
webhook hash dropped, and case-insensitive dedup on algorithm name.
The toOriginalFileHashes test block is moved to its own file
(`toOriginalFileHashes.test.ts`) to keep `ncmecReporting.test.ts`
under the 500-line max-lines limit.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
test: handle concurrent queries in the transactional test harness (#826)
* fix: serialize concurrent queries in transactional test harness
The transactional test harness pins a single Postgres connection per test
and rewrites the app's BEGIN/COMMIT/ROLLBACK to savepoints, tracking the
active savepoint with a single shared depth counter. That counter is only
-safe- if the app's transaction-control statements arrive strictly one
after another. The app, however, routinely runs concurrent transactions
(Promise.all of transactionWithRetry, background eventually-consistent
cache refreshers), which interleaves BEGIN/COMMIT across coroutines on
the one pinned connection. A COMMIT then releases the *wrong* savepoint
("savepoint coop_test_sp_N does not exist"), aborting the outer test
transaction and corrupting every later read in the test.
Fix: wrap every query to the pinned connection — transaction control AND
data, from connect().query and pool.query — in a promise-chained queue so
statements execute one at a time in arrival order. This only serializes
concurrent work *within* a single test (each test already has its own
pg.Client); it does not affect cross-test parallelism. openSavepoint /
closeSavepoint / rejectUnsupportedTransactionControl are unchanged.
Resolves the savepoint-corruption failures seen in CI run 27951870325
(wrong item-type version in moderationConfigService.test.ts, the
NoResultError / driver-has-already-been-destroyed cascade in the MRT
suites).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix: serialize whole logical transactions in test harness to prevent savepoint corruption
The transactional test harness serialized queries at the individual SQL
statement level, which fixed wire-protocol interleaving but not savepoint
stack corruption: overlapping Kysely transactions could interleave
BEGIN(A), BEGIN(B), …, COMMIT(A), and A's COMMIT would release B's
savepoint (it read the shared savepointDepth pointing at B's savepoint).
A concurrent rollback could then discard another transaction's committed
work.
Replace per-statement serialization with a transaction-scoped mutex:
BEGIN acquires it (waiting on any in-flight transaction), COMMIT/ROLLBACK
releases it when savepointDepth returns to 0. Overlapping transactions
now run strictly one-after-another, so savepoints stay properly nested.
Handle nested transactions: a BEGIN arriving while the mutex is held is
recognized as nested (concurrent BEGINs are still blocked in the FIFO
queue) and opens a savepoint directly without re-acquiring. COMMIT/ROLLBACK
only releases the mutex at the outermost depth.
Remove the finally block around closeSavepoint so a failed COMMIT/ROLLBACK
leaves the mutex held, allowing Kysely's catch-block ROLLBACK to still see
an in-flight transaction instead of masking the original error.
Add two integration tests:
- concurrent rollback isolation: 5 concurrent transactions, one throws,
only its row is missing
- nested transactions: transactionWithRetry inside transactionWithRetry
does not deadlock
* test: verify FIFO commit order instead of circular id ordering
The 'serializes concurrent commits' test inserted id (the dispatch index)
and asserted select id order by id == [0..15]. Since id was both the value
and the ORDER BY key, the assertion held under any write order — proving
nothing. Add a seq serial column and order by it so the check reflects
actual commit/write order.
* small cleanup
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
add NCMEC field + integration tests (#887)
* test(ncmec): add field coverage and submission tests
Add XML field-coverage regression tests for NCMEC report builders,
including XSD-order locks for the ipCaptureEvent ordering regression
class. Add a submitReport integration test using a stubbed fetchHTTP seam
against real Postgres, plus a fixture helper for NCMEC org settings.
Canonicalise webhook-sourced ipCaptureEvent objects before XML rendering
so webhook key order cannot produce out-of-order CyberTip XML.
Co-Authored-By: pi
* fix(ncmec): correct fileDetails ipCaptureEvent XSD ordering; harden tests
buildFileDetailsObject emits <ipCaptureEvent> in the wrong position relative
to <industryClassification>/<originalFileHash>. PR #869 moved it BEFORE
industryClassification, claiming NCMEC's live validator required that — but
a probe against exttest.cybertip.org proves the opposite:
- Variant A (#869 order, ipCaptureEvent BEFORE industryClassification):
REJECTED — cvc-complex-type.2.4.a: Invalid content was found starting
with element 'industryClassification'. One of '{deviceId, details,
additionalInfo}' is expected.
- Variant B (canonical XSD order, ipCaptureEvent AFTER
industryClassification/originalFileHash): ACCEPTED, responseCode=0.
#869 misread the original #856 error (which fires when an element lands in
the trailing deviceId/details/additionalInfo slot) and moved ipCaptureEvent
backwards to a position NCMEC actually rejects. #869's own PR description
admits the live re-test was never run before merge. This commit restores the
canonical XSD order, which the live validator accepts.
Reorders buildFileDetailsObject and the FileDetails type so ipCaptureEvent
follows industryClassification and originalFileHash, and updates the
ncmecReporting.builders.test.ts 'preserves XSD insertion order (Appendix C)'
assertion to anchor the correct order (the test had been updated in #869 to
anchor the wrong one).
Also includes test-hygiene improvements to the NCMEC #843 test work:
- fieldCoverage.test.ts: strengthen the renderXml assertion from a bare
length>0 check to per-scenario structural XML markers (present/absent),
so the js2xml serialization path is actually exercised. Remove the dead
XSD.fileDetails array (buildSubmitReportObject's <report> envelope never
contains <fileDetails>; its ordering is locked in
ncmecReporting.builders.test.ts).
- ncmec-submission.integ.test.ts: extract a shared RecordedCall type,
replace submitCall! non-null assertions with an early-fail type guard,
and type the stub generic against the real FetchHTTP/CoopRequestQuery
contract (removing the as-never param, inline cast, and final broad
as-unknown-as-FetchHTTP). Requires exporting HandleResponseBody from
networkingService (additive type-only export).
Co-Authored-By: pi
Fix ESLint warnings for `server` (#331)
* server: Remove stale eslint-disable and simplify nullish operations
This patch fixes 5 ESLint warnings.
The max-lines disable in actionStatistics.ts no longer suppresses any
warning and only adds noise.
In apiKey.ts, "||" is replaced with "??" for description and lastUsedAt
fallbacks. "||" coerces any falsy value (including empty string: "") to
the default, whereas "??" only fires on null/undefined - which may be
the actual intent here. An empty-string description would be silently
discarded with "||".
In integrationRegistry/index.ts and RetryFailedNcmecDecisionsJob.ts,
null-guarded assignment blocks (if (x == null) { x = ... }) are
collapsed to "??=" assignments, which is the idiomatic shorthand for
exactly that pattern and makes the intent immediately readable.
* server: Type apiKey resolvers with generated GQLQueryResolvers/GQLMutationResolvers
The Query and Mutation objects in apiKey.ts are typed as any, which
removes all type-checking on resolver arguments and the context object.
The codegen pipeline already produces GQLQueryResolvers and
GQLMutationResolvers with the correct signatures. Using those types
means wrong argument names, missing fields, or context misuse will be
caught at compile time rather than at runtime.
Error names must match a GraphQL union member because gqlErrorResult
uses them as type name. 'InternalServerError' is a valid CoopErrorName
but not a member of the RotateApiKeyResponse or
RotateWebhookSigningKeyResponse unions, so Apollo can't resolve the
type.
* server: Cast GET route arrays to ControllerRouteList to remove per-route any
route.get returns Route<never, ...> which is invariant with Route<any, any>
in ControllerRouteList, so individual routes needed as Route<any, ...> casts.
Move the cast to the routes array level using as ControllerRouteList, which
is already covered by the centralized eslint-disable on the type definition.
* server: Fix passport typing without unsafe casts
The SAML strategy callbacks passed Sequelize model instances to
done(), which expects "Record<string, unknown>". The original code used
"as any" to fix the mismatch; simply removing those casts broke the
build because Sequelize models carry no string index signature.
Use "user.toJSON()" instead - it returns a plain object that
genuinely satisfies "Record<string, unknown>" at runtime.
For serializeUser, augment "Express.User" in decs.d.ts with the
single field it actually accesses ("id: string").
* server: Parameterise Kysely instances with concrete schema types
PostgresAnalyticsAdapter and ClickhouseKyselyAdapter declare their
Kysely fields as Kysely<any>, which disables query-builder type-
checking for every table access and column reference in those adapters.
PostgresAnalyticsAdapter is parameterised with Kysely<AnalyticsSchema>
and ClickhouseKyselyAdapter with Kysely<Record<string, unknown>>, so
Kysely can enforce table and column existence at compile time rather
than at runtime. Stub method signatures using ...args: any[] are
replaced with unknown[] so that the interface contract is actually
enforced on the concrete classes.
* server: Remove as-any casts in analytics loggers by completing AnalyticsSchema
RuleExecutionLogger, ReportingRuleExecutionLogger, RoutingRuleExecutionLogger,
and ContentApiLogger cast table-name strings and row arrays to any when calling
bulkWrite because those table names were not yet keys of AnalyticsSchema. The
casts silenced the errors but also prevented the compiler from catching a
misspelled or removed table name.
Adding the missing tables (REPORTING_SERVICE.REPORTING_RULE_EXECUTIONS,
MANUAL_REVIEW_TOOL.ROUTING_RULE_EXECUTIONS) to AnalyticsSchema and widening
CONTENT_API_REQUESTS to {[key:string]:unknown} makes the constraint real and
allows all call-site casts to be deleted or narrowed to their correct types.
PostgresAnalyticsAdapter.flushTable used a cast that typed rows as a union
of all schema table types. Kysely requires an intersection of required fields
across those types, which the open-ended {[key:string]:unknown} entries
(introduced in this commit) cannot satisfy. Instead, bulkWrite now stores a
pre-built, fully-typed insert closure alongside each row buffer while it
still knows the concrete table type (flushTable just calls that closure).
* server: Replace deprecated SEMATTRS_EXCEPTION_* otel constants
@opentelemetry/semantic-conventions deprecated the SEMATTRS_EXCEPTION_*
identifiers in favor of ATTR_EXCEPTION_*. The two names refer to the same
attribute keys, so this is a pure rename.
Clears three @typescript-eslint/no-deprecated warnings.
* server: Drop unnecessary conditionals and type assertions
Small, mechanical cleanups flagged by
@typescript-eslint/no-unnecessary-condition and
@typescript-eslint/no-unnecessary-type-assertion. Each change either
removes a guard the type system has already proven redundant, or removes
an assertion that doesn't change the inferred type.
* routes/integration_logos/serveIntegrationLogo*.ts: typed the express
sendFile callback as "(err?: Error)" and simplified "err != null" to a
truthiness check. Matches express's actual contract.
* routes/reporting/submitReport.ts: dropped an always-truthy "hashes &&"
guard.
* rule_engine/RuleEngine.ts: dropped "?? []" on a non-nullable return.
* utils/sql.ts: removed two "SelectQueryBuilder<...>" casts the inference
now handles cleanly.
* server: lower NCMEC file-annotation mapping complexity from 21 to 3
up a FileAnnotations object one case at a time. That tripped the
complexity rule (max 20) and made the mapping itself hard to audit.
Replaced with a single
"Record<NCMECFileAnnotationType, keyof FileAnnotations>" lookup table
plus a small for-loop. The table is the single source of truth for the
NCMEC-enum -> field-name correspondence, and TypeScript's exhaustiveness
checking on the Record key forces every enum variant to be mapped.
* server: Replace "any" with real types in production code
Each case below previously used "any" (or "as any") to paper over a
typing problem. Where a cast is still needed, it goes through the
narrowest meaningful type.
- condition_evaluator/conditionSet.ts: typed the working array as
"Array<ReadonlyDeep<LeafConditionWithResult | ConditionSetWithResult>>"
and replaced two "as any" casts with a single documented narrowing
cast at the return site. The public type is a discriminated union over
arrays-of-leaves vs arrays-of-sets, which can't be pushed into during
the loop; runtime contents stay uniform with the input conditions.
- condition_evaluator/leafCondition.ts: typed the signal-result generic
as "SignalResult<SignalOutputType>", removing both an "as any" and an
unnecessary "as SignalOutputType" assertion at the call time.
- graphql/datasources/UserApi.ts: typed the login/signUp/logout passport
glue using the generated "GQLMutation-Args" types and a small
structural context type (instead of "any").
- services/apiKeyService/apiKeyService.ts: derived "ApiKeyRow" from
"Selectable<ApiKeyServicePg['public.api_keys']>" (Kysely) and used it
as the param type of "mapDbRecordToApiKeyRecord".
- services/manualReviewToolService/modules/CommentOperations.ts:
replaced "as any" / "as any[]" with "as JobId" / "as JobId[]" using
the existing branded type.
- services/ruleAnomalyDetectionService/getRuleAnomalyDetectionStatistics.ts:
typed warehouse rows as "Record<string, unknown>" with explicit
field-level casts, and the binding accumulator as "string[]".
- services/ruleAnomalyDetectionService/detectRulePassRateAnomaliesJob.ts:
preserved the runtime guard against missing org rows by indexing
through "Partial<typeof orgsForChangedRules>". keyBy's "Record<string, T>"
return type lies about lookups always succeeding.
- bin/run-worker-or-job.ts: replaced "(container as any)[name]" with a
typed "container[name as keyof Dependencies] as WorkerOrJob" lookup.
* server: Scope "any" suppressions to documented TODOs
Three locations have "any" casts that can't be replaced today without
upstream typing work that's already tracked by TODOs.
- graphql/modules/insights.ts: resolver maps five times
"getRulePassingContentSamples" results onto types that require "tags"
and "policies" fields the datasource doesn't yet populate. The fix
belongs in the datasource's typing, not in each resolver. Kept the
localized "as any" cast and the existing TODO comments
- graphql/resolvers.ts: same shape of pre-existing TODO on
"getAllRuleInsights" return type
- services/analyticsQueries/ItemHistoryQueries.ts: the data warehouse
schema isn't modeled in TS, so the Kysely instance is intentionally
"Kysely<any>" (so the string-based column selections pass type-check).
* server: Tighten test mocks and clean up test-file "any" usage
Tests that mock private methods or attach properties to typed services
need escape hatches that production code shouldn't. This patch replaces
"any" with real types where possible.
- condition_evaluator/conditionSet.test.ts: wrap RuleEvaluationContext /
SafeTracer mock stubs with "eslint-disable" / "eslint-enable" and a
comment.
- services/derivedFieldsService/helpers.test.ts: the test intentionally
passes invalid inputs to exercise error paths. Wrapped that block in a
paired "eslint-disable" / "eslint-enable" for "no-explicit-any" with a
comment.
- services/manualReviewToolService/modules/CommentOperations.test.ts:
replaced "as any" on job ids with "as JobId"; switched the
"(commentOps as any).getRelatedJobIds(...)" private-method probe to
bracket access ("commentOps['getRelatedJobIds'](...)"). Same access
pattern at runtime without a need for casting.
- services/orgAwareSignalExecutionService/signalExecutionService.test.ts:
extended two existing narrow "eslint-disable" directives to also cover
"no-explicit-any" for jest mock assignments. Added a paired
"eslint-disable" / "eslint-enable" block where the original
next-line directive couldn't reach the multi-line cast.
- services/ruleAnomalyDetectionService/getRuleAnomalyDetectionStatistics.test.ts:
typed the mock query's "tracer" parameter as "unknown". Kept the
unavoidable "jest.fn() as any" for the typed mock fixture and "{} as any"
for the unused tracer arg, both with narrow per-line disables that name
the rule and the reason.
- test/extendExpect.ts: replaced "(toMatchSnapshot as any).call" with
"toMatchSnapshot.call(this as SnapshotContext, ...)" using
jest-snapshot's exported "Context" type. Added an explicit return type
to satisfy "promise-function-async" (the inferred sync|async union was
tripping the rule).
* server: Keep single-case exhaustiveness switches, scope the warning
Three switches dispatch on a discriminated union that today has only one
variant: AggregationsService.evaluateAggregation, serializeAggregation,
and the derivedFieldsService recipe-operation reducer. With one variant
each case label is trivially equal to the discriminant, which trips
@typescript-eslint/no-unnecessary-condition.
The switches stay because they're load-bearing: when a new variant is
added to the union, the existing case stops being exhaustive and the
"default: assertUnreachable(...)" branch surfaces the gap at compile
time.
* Fix merge conflicts
* server: Drop residual Sequelize toJSON() calls
The two SAML strategy callbacks still called `user.toJSON()` left over
from the Sequelize User instance. `kyselyUserFindByEmail` now returns
a plain `GraphQLUserParent` object.
* server: Fix unreachable null check in MRT job
The "fresh deploy" guard checked "lastTimestamp.last_insert == null",
which triggered @typescript-eslint/no-unnecessary-condition: the Kysely
schema types "last_insert" as a non-null "Date", matching the SQL
column ("NOT NULL DEFAULT '-infinity'").
This patch replaces "== null" with
"Number.isFinite(last_insert.valueOf())". This removes the dead branch
(and the lint warning) and makes the fresh-deploy path actually fall
back to "new Date(0)" as intended.
* address PR review feedback
---------
Co-authored-by: Juan S. Mrad <juansmrad@gmail.com>
test: isolate server tests via transaction rollback (#732)
* test: isolate DB-backed server tests via transaction rollback
Introduce a transaction-rollback test harness so DB-backed tests get
automatic per-test isolation with no hand-written cleanup, replacing the
fixture-teardown (and hardcoded-id) patterns that leaked state across runs.
Mechanism (server/test/harness/transactionalPgPool.ts):
- A pg.Pool facade that pins one connection and rewrites the app's
BEGIN/COMMIT/ROLLBACK to SAVEPOINTs, so an outer test transaction rolls
everything back -- including the app's own makeKyselyTransactionWithRetry
transactions. Also exposes pool.query for the express-session store.
Adoption:
- makeMockedServer is now transactional: real Postgres (mocked ClickHouse)
wired through the rolled-back connection, exposing rollback()/shutdown().
getBottleContainerWithIOMocks gains an optional kyselyPool override for it.
- makeTransactionalTestWithFixture drives rollback from the test lifecycle,
so fixtures return only their data -- no cleanup.
- Convert the makeMockedServer-tier tests and detectRulePassRateAnomalies
to the harness, dropping all per-test cleanup.
- Extract getPgConnectionParams so the harness reads the same DATABASE_* env
vars as the app.
Postgres-only: Scylla/ClickHouse/Redis writes are not rolled back; tests
touching those keep uid-based isolation.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* test: tighten harness query types and guard savepoint underflow
Replace blanket no-explicit-any suppressions on the transactional pool's
query path with proper pg types (string | QueryConfig, unknown[]), and
guard COMMIT/ROLLBACK rewrites against savepoint-depth underflow by
asserting depth > 0 and only decrementing after the SQL succeeds.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test: guarantee pinned-connection cleanup with try/finally
Wrap each transactionalPgPool integ test body in try/finally so a
mid-test throw can't leak the pinned Postgres connection; closing it in
finally also aborts any still-open outer transaction.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test: reject unsupported transaction-control statements in pg harness
Strip trailing semicolons before matching transaction-control statements
(so `COMMIT;` is rewritten too), and reject transaction-control commands
the savepoint rewrite doesn't handle (END, ABORT, SAVEPOINT, RELEASE,
COMMIT/ROLLBACK PREPARED, PREPARE TRANSACTION) with an explanatory error
instead of silently forwarding them and defeating per-test isolation.
Extract the savepoint open/close logic into helpers to cut duplication,
and add a regression test covering the new rejection behaviour.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
roost.tools