Skip to content

Run post-review regression tests in CI via local Supabase stack#166

Open
maschlr wants to merge 26 commits intodevfrom
ms-ci-integration-tests
Open

Run post-review regression tests in CI via local Supabase stack#166
maschlr wants to merge 26 commits intodevfrom
ms-ci-integration-tests

Conversation

@maschlr
Copy link
Copy Markdown
Contributor

@maschlr maschlr commented Apr 19, 2026

Summary

  • Extends CI with a new integration job that spins up a Supabase local stack and Redis service container, then runs describe blocks tagged [integration].
  • Closes the coverage gap for three regression fixes from the parent PR (Add basic code quality infrastructure #156) that depend on real external systems: the PDF extraction error path (fix 1.2), the Supabase 23505 race in `ensureUserAndConversation` (fix 1.2 cont.), and the legacy `rawFiles` migration in `cleanValues` (fix 1.1).
  • Integration tests skip cleanly when their required env is absent (via `describeIf*` helpers), so the default `bun test` stays safe locally.

Why stacked on #156

The three new integration tests assert behaviors that the parent PR's fixes introduced. Basing here keeps the diff reviewable and lets the integration job verify #156's regression fixes against a real stack before either PR merges.

Test plan

  • Verify CI's new `integration` job appears in PR checks and runs green
  • Pull the branch locally, run `supabase start` + `bun test` — confirm full suite passes (142 tests)
  • Without Supabase running, run `bun test` — confirm 139 pass + 5 skip, no failures
  • Optional: revert the rawFiles strip in `src/db/cleanValues.ts` on a scratch branch; run integration job — expect test Improved client UI with dev‑only auth and Supabase session support #3 to fail (proves the coverage)

maschlr added 23 commits April 16, 2026 12:28
- openai: ^6.6.0 → ^6.34.0
- @anthropic-ai/sdk: ^0.66.0 → ^0.90.0

Both SDKs now include proper type definitions for web_search
tools, reasoning_effort, ContentBlockParam, and other features
that were previously worked around with @ts-ignore directives.
- Close unclosed if-block in sendDeepResearchMessage (line 298)
  that caused 3 cascading compilation errors
- Add missing return statement for the deep research response
- Change catch clauses from `err: any` to `err: unknown`
- Scope root tsconfig to src/ only; client/ and documentation/
  have their own tsconfigs
- Enable strict mode in client/tsconfig.json
- Add @types/papaparse for CSV parser types
- Fix implicit any params (parsers.ts, subscribe.ts)
- Fix possibly-undefined access (openrouter.ts, chat.worker.ts)
- Fix state.ts endStep missing required field
- Add Zod validation at JSON.parse boundaries (edison.ts)
- Remove stale @ts-ignore directives (canvas-polyfill.ts, edison.ts)
- Use declare global for globalThis polyfill augmentation
- Simplify fetchWithRetry to accept string | URL only (no callers
  pass Request)
- Type DB operation params with StateValues/ConversationStateValues
  instead of any; add DbState/DbConversationState interfaces
- Add parseLLMProviderName runtime validator, eliminate 5 @ts-ignore
  on LLM constructor calls across agents and workers
- Replace property-stuffing on Error with FallbackError class;
  use instanceof checks instead of (error as any).requiresFallback
- Make LLMAdapter.logDuration public to remove (this.adapter as any)
  casts in LLM provider
- Change abstract transformRequest/transformResponse signatures
  from any to unknown
- Add error/status fields to StateValues
- Add ElysiaRouteContext<TParams, TBody> in src/types/elysia.ts
- Add global Request augmentation so request.auth is typed everywhere
- Apply ElysiaRouteContext to 12 route handlers replacing ctx: any
- Remove (request as any).auth casts; use request.auth directly
- Replace error: any catches with unknown + instanceof narrowing
- Introduce PaperStatusResponse type for response building in
  paperStatusHandler
- Narrow FormData body via type-guard helpers instead of as casts
Four principles for agents working on the repo:
- Think before coding: state ambiguity, don't guess
- Simplicity first: no features beyond what was asked
- Surgical changes: every line traces to the request
- Goal-driven execution: tests define done
- Replace ws.data mutation pattern with per-client WeakMap<WsClient,
  WsConnectionState> keyed by the ws instance
- Define WsClient as a minimal structural interface (send, close,
  readyState) and use Elysia's ws via structural assignment
- Add IncomingMessage discriminated union (auth/subscribe/
  unsubscribe/ping) with a parseIncomingMessage runtime guard
- Remove all (ws.data as any) casts and Set<any>/Map<any, ...> state
- Properly handle readyState undefined case in cleanupDeadConnections
- Use SDK-provided types directly for OpenAI web_search tool,
  reasoning_effort, and ResponseStreamEvent discriminated union
- Type LLMRequest.format as ResponseFormatTextConfig from openai SDK
- Split extractSources logic to use typed ResponseFunctionWebSearch
  and its Search action shape
- Replace @ts-ignore on OpenRouter usage token fields with unified
  field access across usage/metrics.tokens shapes
- Validate OpenRouter responses via Zod schema at the fetch boundary
  instead of casting the parsed JSON
- Remove (anthropicRequest as any) on adaptive thinking and
  output_config (now typed in @anthropic-ai/sdk 0.90.0)
- Convert logger.error(msg, error as any) to pino's idiomatic
  logger.error({ err }, msg) in fileUpload/index.ts,
  embeddings/documentProcessor.ts, embeddings/vectorSearchWithDocs.ts,
  and storage/providers/s3.ts
- Replace (globalThis as any).__knowledgeVectorSearch with typed
  global augmentation via declare global
- Type getToolDefinitions() return as Tool[] from Anthropic SDK so
  it matches MessageCreateParams.tools without a cast
- Use request.auth directly in rateLimiter.ts now that Request is
  globally typed via ElysiaRouteContext module augmentation
- Replace (discovery as any).jobId with evidenceArray-first lookup
  in paper/prompts.ts; use property-in narrowing in generatePaper.ts
  for legacy discovery fallback
- Replace PAPER_GEN_LLM_PROVIDER cast with parseLLMProviderName
  validator
- Type Tool.execute with Message and unknown instead of any
- Type UploadedFile.metadata, TextChunk.metadata, and
  VectorSearchResult.metadata as Record<string, unknown>
- Type addVariablesToState with StateValues/ConversationStateValues
  overloads; type formatConversationHistory with a minimal message
  shape instead of any[]
- Narrow catch (err: any) to catch (err: unknown) + instanceof Error
  in chat/setup, files/description, files/index, jwt, paper/utils,
  storage/s3, embeddings/*, and LLM adapters
- Type middleware set/body params with concrete shapes
  ({ status?; headers }, body?: unknown)
- Type ConversationSetup records with DbState / DbConversationState
- Type createdMessage in deep-research/start via an alias matching
  createMessage's return shape
- Use the Anthropic Tool[] type for getToolDefinitions so
  MessageCreateParams.tools matches without a cast
- Parse OpenRouter response bodies from unknown via Zod / structural
  narrowing instead of as casts
- Retain a single documented 'messageContent: any' in generatePaper
  where widening LLMRequest.content to support Anthropic vision
  blocks would cascade through every adapter's SDK message type
- Add null guards for noUncheckedIndexedAccess on array/map access
  (build.ts env parser, useSessions realtime handlers, ChatPage
  optional research state chains, parseCitations regex matches,
  helpers UUID byte access)
- Replace React.CSSProperties with JSX.CSSProperties from preact
  in SuggestedSteps
- Narrow AuthContext.decodeJWTPayload via local variable + null check
  before atob()
- Handle File|undefined in useFileUpload, usePresignedUpload with
  explicit guards and ?? null defaults
- Add updated_at to client Message interface (already set by DB)
- Add sidecar .d.ts files for .jsx components (ChatInput,
  ErrorMessage, LoginScreen, Message, Sidebar, TypingIndicator,
  WelcomeScreen) with properly-typed Preact component signatures
  based on actual call sites
- Type IconButton onClick handler with Preact's TargetedMouseEvent
- Type useAutoScroll dependencies as readonly unknown[]
- Narrow useChatAPI poll result files via ChatResponse['files']
- Add MessageFileMetadata interface in lib/supabase and use for
  both dbMsg.files and Message.files
- Type ChatPage callbacks with Message/DBMessage from supabase and
  useSessions; catch err as unknown with instanceof narrowing
- Type state?: Record<string, unknown> on client Message interface
Replace Prettier with Biome for linting, formatting, and import sorting.
Add Husky pre-commit hook that runs Biome on staged files incrementally.
Add GitHub Actions CI with lint-format (PR changed files only) and test
jobs. Add initial test suites (58 tests) for core utilities: uuid, cache,
planningJsonExtractor, state, discovery, DOI, textUtils, authResolver.
Rewrite CLAUDE.md into concise agent-optimized AGENTS.md with symlink
for backwards compatibility.
- Restore rawFiles buffer/parsedText strip in cleanValues to migrate
  legacy Supabase state docs on next write (extracted to cleanValues.ts
  for testability).
- Replace `{ ...error }` spreads in setup.ts and description.ts with
  `instanceof Error` narrowing; the spread produced `{}` for standard
  Errors because message/code/stack are non-enumerable, breaking the
  23505 race-detection path and hiding real PDF/OCR/Excel extraction
  errors in logs.
- Chat worker logs a warning and skips the wait when the file-process
  queue is unavailable rather than silently proceeding.
- WebSocket cleanup treats undefined readyState as dead so connections
  from implementations that don't expose readyState are not leaked.
- LLM adapters (openrouter-alpha, edison) use Zod safeParse so provider
  schema drift surfaces as a logged warning / structured error instead
  of a ZodError that bypasses the FallbackError retry path.
- Export getState and parseIncomingMessage from the WebSocket handler
  to enable unit testing.
- retry.test.ts covers FallbackError shape, isRetryableError patterns,
  backoff delay bounds, fallback-config lookup, and withRetry paths
  (success, retryable, non-retryable, fallback-triggered, enableFallback
  disabled, and onRetry/onFallback callbacks).
- websocket handler.test.ts covers parseIncomingMessage (non-JSON,
  primitive JSON, missing/bad action, auth/subscribe/unsubscribe/ping
  shapes with field-type guards) and WeakMap-based state isolation.
- cleanValues.test.ts exercises all strip branches including the legacy
  rawFiles buffer/parsedText migration, input immutability, and empty/
  malformed-entry handling.
- authResolver.test.ts gains proper beforeEach/afterEach env restore
  (deleting when original was undefined), JWT happy-path + wrong-secret
  + expired cases using jose HS256 signing, constant-time-compare
  length-mismatch and same-length-wrong-value coverage, and strict-mode
  unauthenticated-rejection case.
- openrouter adapter warns on empty response.choices before returning
  "" so that an error-like 200 response is visible in logs instead of
  being treated as "no content".
- fetchWithRetry JSDoc documents the string|URL narrowing so future
  Request callers widen the type intentionally (and restore .clone()
  on retry) rather than silently reusing a consumed body.
- Deep-research status reader filters malformed legacy rawFiles
  (missing filename/mimeType) instead of emitting {filename:"",
  mimeType:""} entries that would mask corrupted state.
- Move isBodyRecord, asString, and extractFiles from chat.ts and
  deep-research/start.ts into src/utils/bodyParsing.ts; both routes
  now import from the shared module.
- Move the CreatedMessage type and initKnowledgeBase() call below
  imports in deep-research/start.ts per the module-level-imports rule.
- Drop the unused [key: string]: unknown index signature on Tool.execute
  input and the unused TBody generic on ElysiaRouteContext; both were
  escape hatches the type-strict refactor no longer needs.
- Remove pre-Biome eslint-disable comments in canvas-polyfill.ts.
Extract the stability-critical logic behind four post-review fixes into
pure functions so they can be tested without a running environment.

- cleanupDeadConnectionsIn(map) takes the connection map as a parameter;
  cleanupDeadConnections() is the zero-arg wrapper the cron caller uses.
  New tests cover OPEN/CLOSING/CLOSED/undefined readyState cases and
  empty-set conversation cleanup — including the regression fix where
  undefined readyState must be treated as dead.
- openrouter-extractors.ts now owns extractText / extractWebSearchResults
  and the OpenRouterResponse shape. Tests exercise empty-choices warn,
  URL-citation parsing, dedup, and skipped annotation types.
- parseOpenRouterResponse is the exported safeParse helper used by
  openrouter-alpha; tests cover valid, schema-mismatch (raw pass-through
  + warn), missing-url context, null input, and all-optional-fields.
- waitForPendingFiles is extracted from chat.worker.ts with injected
  queue, status reader, and sleep. Tests cover the regression fix
  (queue=null + pending files logs warn and returns), completed/ready/
  missing-job happy paths, failed/error file paths, multi-file
  iteration, and maxWaitMs bound.
Generated via `supabase init` then trimmed to enable only the services
that integration tests need:

- `[api]` (PostgREST) and `[db]` (Postgres) — required
- `[auth]` — kept enabled only because `supabase status -o env` omits
  SERVICE_ROLE_KEY/ANON_KEY when auth is off, and supabase-js requires
  a key string even for unauthenticated PostgREST reads
- `[realtime]`, `[studio]`, `[inbucket]`, `[storage]`, `[edge_runtime]`,
  `[analytics]` — disabled to keep `supabase start` fast (~30-60s cold)
  and memory-light

Local devs working on auth/storage flows can flip these back on
temporarily; do not commit that change.

A header comment in the config explains the intent.
Three describe blocks tagged [integration], gated by env helpers in
src/utils/__testHelpers__/integrationEnv.ts. They skip cleanly when
their required env is absent, so the default `bun test` remains safe
locally.

- description.integration.test.ts (fix 1.2): feeds a 44-byte corrupted
  PDF to the now-exported extractPDFText and asserts the returned
  string carries a real error message (not "unknown") and that
  logger.error was called with a truthy err.message. The `{ ...error }`
  spread regression would have made err.message undefined.

- setup.integration.test.ts (fix 1.2 cont.): fires two concurrent
  ensureUserAndConversation calls with the same conversationId but
  different userIds. Asserts exactly one wins and the loser gets the
  ownership-mismatch error — and that the generic
  create_conversation_failed error is NOT logged, proving the 23505
  branch handled the race.

- cleanValues.integration.test.ts (fix 1.1): seeds a legacy-shape row
  with rawFiles buffer + parsedText via createState, calls updateState
  with a legacy-shape payload, reads back, asserts buffer/parsedText
  stripped while filename/mimeType/metadata preserved.

DB-touching tests use dynamic imports for db/operations and db/client
because operations.ts instantiates the Supabase service client at
module load — loading it without env would throw even when the test
was meant to skip.
New `integration` job in the CI workflow, gated on the existing `test`
job passing so service time isn't wasted when the fast suite is red:

- Redis 7-alpine via GitHub Actions service container
- Supabase local stack via `supabase/setup-cli@v1` + `supabase start`;
  config.toml disables unused services for a fast cold start
- pdf-parse native deps (cairo/pango/jpeg/gif/rsvg) installed via apt
- `bun test --test-name-pattern '\[integration\]'` runs only the tagged
  suites; RUN_PDF_INTEGRATION=1 so the PDF test opts in
- SUPABASE_URL / SUPABASE_SERVICE_KEY / SUPABASE_ANON_KEY are exported
  from `supabase status -o env` via sed rename; the local stack
  generates its own keys so no repo secrets are needed
- `supabase stop --no-backup` in an always-run step keeps the runner
  clean on reruns

CLAUDE.md gains an "Integration tests" subsection documenting the
local workflow (supabase start → export env → bun test pattern).
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 19, 2026

Greptile Summary

This PR extends CI with an integration job that spins up a local Supabase stack and Redis service, then runs integration tests (tagged [integration]) covering three regression fixes from the parent PR: corrupted-PDF error messaging, the Supabase 23505 race in ensureUserAndConversation, and rawFiles stripping in cleanValues. The describeIfSupabase / describeIfPdf helpers ensure the full unit suite stays safe when services are absent.

Confidence Score: 5/5

Safe to merge; all findings are P2 style suggestions with no blockers.

All three inline comments are P2: unpinned CLI version, missing seed.sql (only affects local db reset, not CI), and a test that is behaviourally correct but doesn't strictly prove the 23505 branch fires. None block merge or introduce data correctness/security issues.

.github/workflows/ci.yml (pin Supabase CLI version) and supabase/config.toml (add or disable missing seed.sql reference).

Important Files Changed

Filename Overview
.github/workflows/ci.yml Adds integration job with Redis service container, Supabase CLI setup, env export, and filtered test run; version: latest for Supabase CLI is unpinned.
supabase/config.toml CI-tuned local stack config (API + DB only); references a seed.sql that doesn't exist, which will error on supabase db reset.
src/utils/testHelpers/integrationEnv.ts Clean env-gated describeIfSupabase / describeIfPdf helpers; flags evaluated at module load time, safe for both CI and local skip behaviour.
src/services/chat/tests/setup.integration.test.ts Tests 23505 race in ensureUserAndConversation; assertions are correct but may not strictly exercise the constraint-violation branch depending on timing.
src/db/tests/cleanValues.integration.test.ts Solid integration test seeding legacy rawFiles and asserting buffer/parsedText are stripped by updateState; cleanup and dynamic imports are correct.
src/services/files/tests/description.integration.test.ts Correctly tests the corrupted-PDF error path; uses describeIfPdf guard, spy on logger.error, and fixture file to assert concrete error message contract.
src/services/files/description.ts extractPDFText exported for integration testing; error handling uses instanceof Error guard instead of spread, closing the "unknown" regression.
AGENTS.md Documents integration test setup commands and CI job accurately; no issues.
supabase/.gitignore Correctly ignores .branches, .temp, and local env files; no issues.

Sequence Diagram

sequenceDiagram
    participant GH as GitHub Actions
    participant T as test job
    participant I as integration job
    participant SB as supabase start
    participant R as Redis service
    participant BT as bun test [integration]

    GH->>T: lint-format (PR only)
    GH->>T: bun test (unit)
    T-->>I: needs: [test]
    I->>R: start redis:7-alpine
    I->>SB: supabase start (migrations applied)
    SB-->>I: export SUPABASE_URL / SERVICE_KEY / ANON_KEY
    I->>BT: bun test --test-name-pattern '[integration]'
    Note over BT: describeIfSupabase → cleanValues test
    Note over BT: describeIfSupabase → ensureUserAndConversation race test
    Note over BT: describeIfPdf → extractPDFText corrupted-PDF test
    BT-->>I: results
    I->>SB: supabase stop --no-backup (always)
Loading
Prompt To Fix All With AI
This is a comment left during a code review.
Path: .github/workflows/ci.yml
Line: 74-75

Comment:
**Pin Supabase CLI version for reproducibility**

Using `version: latest` means any breaking release of the Supabase CLI will silently change CI behavior. Given that `supabase status -o env` output format and `supabase start` flag semantics have changed across CLI versions, an unpinned version is a latent stability risk for this job.

Consider pinning to the CLI version you used when writing these tests (e.g. `2.x` or the exact semver you tested against).

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: supabase/config.toml
Line: 67-73

Comment:
**Referenced `seed.sql` does not exist in the repo**

`sql_paths = ["./seed.sql"]` references a file that isn't present (no `supabase/seed.sql` was added in this PR or exists in the repo). `supabase start` only applies migrations, not seeds, so CI won't break — but any developer who runs `supabase db reset` locally will get a file-not-found error. Either create an empty `supabase/seed.sql` or set `enabled = false` to make the intent explicit.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: src/services/chat/__tests__/setup.integration.test.ts
Line: 36-57

Comment:
**Race test may not reliably exercise the 23505 code path**

`Promise.all` launches both calls concurrently, but if the JS event loop services them sequentially at the network level — e.g. userA's full round-trip to Supabase completes before userB's first request leaves — userB will hit the "conversation exists → ownership check → access denied" branch rather than the `23505` constraint path. The assertions (one winner, one loser, no `create_conversation_failed` log) pass either way, so the test is correct but does not strictly prove the 23505 fix. Adding a spy that confirms `conversation_ownership_mismatch` was logged (which only fires via the 23505 re-check branch) would make the assertion airtight.

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "Run integration tests in CI against a Su..." | Re-trigger Greptile

Comment thread .github/workflows/ci.yml
Comment on lines +74 to +75
- uses: supabase/setup-cli@v1
with:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Pin Supabase CLI version for reproducibility

Using version: latest means any breaking release of the Supabase CLI will silently change CI behavior. Given that supabase status -o env output format and supabase start flag semantics have changed across CLI versions, an unpinned version is a latent stability risk for this job.

Consider pinning to the CLI version you used when writing these tests (e.g. 2.x or the exact semver you tested against).

Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/ci.yml
Line: 74-75

Comment:
**Pin Supabase CLI version for reproducibility**

Using `version: latest` means any breaking release of the Supabase CLI will silently change CI behavior. Given that `supabase status -o env` output format and `supabase start` flag semantics have changed across CLI versions, an unpinned version is a latent stability risk for this job.

Consider pinning to the CLI version you used when writing these tests (e.g. `2.x` or the exact semver you tested against).

How can I resolve this? If you propose a fix, please make it concise.

Comment thread supabase/config.toml
Comment on lines +67 to +73
[db.seed]
# If enabled, seeds the database after migrations during a db reset.
enabled = true
# Specifies an ordered list of seed files to load during db reset.
# Supports glob patterns relative to supabase directory: "./seeds/*.sql"
sql_paths = ["./seed.sql"]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Referenced seed.sql does not exist in the repo

sql_paths = ["./seed.sql"] references a file that isn't present (no supabase/seed.sql was added in this PR or exists in the repo). supabase start only applies migrations, not seeds, so CI won't break — but any developer who runs supabase db reset locally will get a file-not-found error. Either create an empty supabase/seed.sql or set enabled = false to make the intent explicit.

Prompt To Fix With AI
This is a comment left during a code review.
Path: supabase/config.toml
Line: 67-73

Comment:
**Referenced `seed.sql` does not exist in the repo**

`sql_paths = ["./seed.sql"]` references a file that isn't present (no `supabase/seed.sql` was added in this PR or exists in the repo). `supabase start` only applies migrations, not seeds, so CI won't break — but any developer who runs `supabase db reset` locally will get a file-not-found error. Either create an empty `supabase/seed.sql` or set `enabled = false` to make the intent explicit.

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +36 to +57
test("exactly one winner; loser gets access-denied without generic error log", async () => {
const errorSpy = jest.spyOn(logger, "error").mockImplementation(() => undefined);

const [resultA, resultB] = await Promise.all([
ensureUserAndConversation(userIdA, conversationId),
ensureUserAndConversation(userIdB, conversationId),
]);

const winners = [resultA, resultB].filter((r) => r.success);
const losers = [resultA, resultB].filter((r) => !r.success);
expect(winners).toHaveLength(1);
expect(losers).toHaveLength(1);
expect(losers[0]!.error).toBe("Access denied: conversation belongs to another user");

// The race must be handled by the 23505 branch, NOT the generic
// create_conversation_failed arm. If the `{ ...err }` spread regression
// were still present, errCode would be undefined, the race would fall
// through to the error-logging path, and this assertion would fail.
const genericFailureCall = errorSpy.mock.calls.find(
([, msg]) => msg === "create_conversation_failed"
);
expect(genericFailureCall).toBeUndefined();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Race test may not reliably exercise the 23505 code path

Promise.all launches both calls concurrently, but if the JS event loop services them sequentially at the network level — e.g. userA's full round-trip to Supabase completes before userB's first request leaves — userB will hit the "conversation exists → ownership check → access denied" branch rather than the 23505 constraint path. The assertions (one winner, one loser, no create_conversation_failed log) pass either way, so the test is correct but does not strictly prove the 23505 fix. Adding a spy that confirms conversation_ownership_mismatch was logged (which only fires via the 23505 re-check branch) would make the assertion airtight.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/services/chat/__tests__/setup.integration.test.ts
Line: 36-57

Comment:
**Race test may not reliably exercise the 23505 code path**

`Promise.all` launches both calls concurrently, but if the JS event loop services them sequentially at the network level — e.g. userA's full round-trip to Supabase completes before userB's first request leaves — userB will hit the "conversation exists → ownership check → access denied" branch rather than the `23505` constraint path. The assertions (one winner, one loser, no `create_conversation_failed` log) pass either way, so the test is correct but does not strictly prove the 23505 fix. Adding a spy that confirms `conversation_ownership_mismatch` was logged (which only fires via the 23505 re-check branch) would make the assertion airtight.

How can I resolve this? If you propose a fix, please make it concise.

maschlr added 3 commits April 19, 2026 16:32
The `pull_request.branches: [main, dev]` filter excluded PRs targeting
any other base — including stacked PRs that target another feature
branch (e.g. #166 stacked on ms-add-basic-code-quality). Drop the
filter so every PR runs lint, test, and integration checks.

`push.branches` stays scoped to main and dev to avoid double-running
on every feature-branch push; PR events cover feature branches.
- Race test now scales to 10 concurrent callers and asserts a new
  logger.warn("conversation_create_race_23505") fired from setup.ts's
  23505 arm, so the test genuinely pins the race-handling branch instead
  of potentially passing via the ownership-mismatch pre-check path.
- CI env-export step fails fast if `supabase status -o env` stops
  emitting any of SUPABASE_URL / SUPABASE_SERVICE_KEY / SUPABASE_ANON_KEY,
  preventing silent green runs where describeIfSupabase degrades to skip.
- cleanValues integration test verifies the seed contains buffer /
  parsedText before asserting they are stripped post-update.
- integrationEnv helper rejects "", "undefined", "null", and whitespace
  rather than running suites with meaningless env values.
- Surface PostgREST cleanup errors instead of swallowing them, and guard
  against undefined ids when beforeEach throws mid-setup.
- Rework transient comment references ("fix 1.1/1.2", "{...err} spread
  regression") into invariant-focused wording so they stay accurate
  after parent PR merges.
- Minor: flatten nested ternary in PDF test via a local messageOf helper,
  assert errorSpy was called exactly once, and drop an unnecessary cast
  in cleanValues test.
HTTP round-trips through createUser serialize concurrent callers so the
winner typically commits its INSERT before any other caller reaches
getConversation — everyone else then takes the pre-check ownership
path and the 23505 arm never fires. The integration test's warn-count
assertion was therefore flaky and failed in CI.

- setup.race.test.ts: new deterministic unit test that uses mock.module
  on db/operations to throw a synthetic 23505 and drive the re-check to
  return a conflicting owner. Pins the conversation_create_race_23505
  warn + no-generic-failure contract on every run with no DB needed.
- setup.integration.test.ts: drop the warn-count assertion and the
  warnSpy that backed it; keep the concurrency-safety assertions
  (single winner, no generic failure log, DB row owned by the winner).
  Comment points at setup.race.test.ts for the branch-level coverage.
Base automatically changed from ms-add-basic-code-quality to dev April 22, 2026 12:34
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