Skip to content

Test unconfigured-contract 502 path, guard single-primary wallet, dedupe pnpm allowlist, test shared/jwt - #161

Merged
dev-fani merged 2 commits into
fanilabs:mainfrom
driftsorbit:fix/testing-reliability-127-130
Aug 30, 2026
Merged

Test unconfigured-contract 502 path, guard single-primary wallet, dedupe pnpm allowlist, test shared/jwt#161
dev-fani merged 2 commits into
fanilabs:mainfrom
driftsorbit:fix/testing-reliability-127-130

Conversation

@driftsorbit

Copy link
Copy Markdown
Contributor

Four small, independent reliability/testing fixes.

Closes #130
Closes #129
Closes #128
Closes #127


#130 — 502 BLOCKCHAIN_ERROR-on-unconfigured is now tested, and no longer hidden from coverage

Each of deliveries, escrow, fleet, disputes, reputation has a createUnconfiguredContractClient() in its composition-root index.ts whose methods throw a BlockchainError when the module's *_CONTRACT_ID is blank (the .env.example default). docs/API_REFERENCE.md documents this 502 for every one of the five, but no integration spec exercised it, and vitest.config.ts excluded every index.ts from coverage so the branch was invisible there too.

  • One integration case per module in *-routes.integration.spec.ts: mints an access token with signAccessToken, POSTs a valid body to a build endpoint with the contract id unset (the test-process default), and asserts statusCode === 502, error.code === 'BLOCKCHAIN_ERROR', and that the message names the missing env var. Each fails if the module's fallback wiring (the config.X_CONTRACT_ID ? real : fallback ternary) is removed or the message drops the variable name. These run under the same describe.skipIf(!dbAvailable) gate as the rest of each file.
  • Coverage exclusion narrowed from a blanket src/**/index.ts to src/**/{domain,application,infrastructure,interface}/index.ts + src/shared/**/index.ts. Genuine barrel/re-export files stay excluded; the five module composition roots with real conditional logic are now reported.

#129 — concurrent wallet-link confirmations can't both become isPrimary

confirmWalletLink chose isPrimary from currentWallets.length === 0 then called create, with nothing at the DB level stopping two concurrent confirmations for a brand-new user from both winning that check.

  • Migration 20260830120000_wallet_address_single_primary adds a Postgres partial unique index: CREATE UNIQUE INDEX wallet_addresses_user_id_primary_key ON wallet_addresses(user_id) WHERE is_primary; — any number of non-primary rows per user, at most one primary. Prisma's schema DSL can't represent a filtered unique index, so it lives only in the hand-written migration; a comment on the WalletAddress model records this so nobody "resolves drift" by dropping it.
  • confirmWalletLink now, on the first-wallet path, catches the constraint violation and re-links the wallet as non-primary (after confirming a primary now exists), so the loser of the race still succeeds — "first wallet wins" — rather than getting a 500. Non-race behavior is unchanged.
  • Tests: unit test for the catch-and-retry-as-non-primary path (confirm-wallet-link.spec.ts); integration test firing two concurrent create({ isPrimary: true }) for one user and asserting exactly one survives as primary and the other can still be linked non-primary (prisma-repositories.integration.spec.ts).

#128 — the pnpm native-build allowlist was declared twice

package.json's pnpm.onlyBuiltDependencies and pnpm-workspace.yaml's allowBuilds held the identical six-package list. allowBuilds (pnpm v10.26.0+) is the replacement; onlyBuiltDependencies is removed in pnpm v11, so a packageManager bump past v11 could silently disable the allowlist and re-introduce the Phase 6 bcrypt/Prisma "native build scripts skipped on clean install" regression that docs/DEPLOYMENT.md § Status records fixing.

  • Removed pnpm.onlyBuiltDependencies from package.json; allowBuilds is now the single source of truth (with a comment explaining why).
  • Updated docs/DEPLOYMENT.md § Status item 3 to reflect the single location and the reason for the move.
  • Note: a clean pnpm install in a pnpm-v11 / Dockerfile-base container to re-confirm the six packages still build with only allowBuilds present is left for a maintainer with that environment — this PR is config-only for that item.

#127src/shared/jwt had no dedicated test

signAccessToken / verifyAccessToken / signRefreshToken / verifyRefreshToken — trusted by the shared HTTP auth guard on every protected route — were only covered incidentally by the auth module's flow tests. Adds src/shared/jwt/index.spec.ts testing all four directly: claim round-trips, wrong-secret rejection, expired-token rejection, tampered-payload rejection, and access/refresh purpose confusion in both directions.


Verification

  • pnpm run typecheck — no new errors (the repo has 52 pre-existing tsc --noEmit errors on main, all in files this PR doesn't touch; count unchanged).
  • pnpm exec vitest run src/shared/jwt/index.spec.ts src/modules/users/application/confirm-wallet-link.spec.ts — 17 passed.
  • All five route integration specs + the users repo integration spec collect cleanly; DB-gated cases skip locally (no database in the dev environment) and run in CI.
  • eslint and prettier clean on all changed files.

…upe pnpm allowlist; test shared/jwt

Closes fanilabs#130
Closes fanilabs#129
Closes fanilabs#128
Closes fanilabs#127

fanilabs#130 — Every module's `createUnconfiguredContractClient()` 502 BLOCKCHAIN_ERROR
fallback (the documented behavior when a `*_CONTRACT_ID` is blank) had no
test. Adds one integration case per module (deliveries, escrow, fleet,
disputes, reputation) that mints an access token, calls a build endpoint
with the contract id unset, and asserts a 502 whose `code` is
BLOCKCHAIN_ERROR and whose message names the missing env var — so a broken
ternary or a message losing the variable name is caught. Narrows
`vitest.config.ts`'s blanket `src/**/index.ts` coverage exclusion to
barrel/re-export index files only, so the five composition roots'
conditional wiring is no longer hidden from the coverage report.

fanilabs#129 — `confirmWalletLink` set `isPrimary` from a race-prone
check-then-create, so two concurrent confirmations for a brand-new user
could both become primary. Adds a Postgres partial unique index
(`wallet_addresses_user_id_primary_key ON wallet_addresses(user_id) WHERE
is_primary`) via a hand-written migration (Prisma's DSL can't express a
filtered unique index; a note on the model records why). `confirmWalletLink`
now catches the constraint violation on the first-wallet path and retries as
non-primary, preserving "first wallet wins" instead of 500-ing the loser.

fanilabs#128 — The native-build-script allowlist was declared twice: in
`package.json`'s `pnpm.onlyBuiltDependencies` and `pnpm-workspace.yaml`'s
`allowBuilds`. `onlyBuiltDependencies` is removed in pnpm v11, so a
`packageManager` bump past v11 could silently disable it and re-introduce
the Phase 6 bcrypt/Prisma build regression. Removes the `package.json` copy,
leaving `allowBuilds` as the single source of truth; notes the move in
`docs/DEPLOYMENT.md` § Status.

fanilabs#127 — `src/shared/jwt` (the token module every protected route trusts) had
no dedicated test. Adds `src/shared/jwt/index.spec.ts` covering all four
functions directly: claim round-trips, wrong secret, expired token,
tampered payload, and access/refresh purpose confusion.
@drips-wave

drips-wave Bot commented Aug 30, 2026

Copy link
Copy Markdown

@driftsorbit 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

@dev-fani
dev-fani merged commit 1ee53f6 into fanilabs:main Aug 30, 2026
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment