chore(review): full-repo audit 2026-04-10 — fixes, reclaim tests, doc reconciliation

[adminbjkai]

cap5 — full-repo review and changelog

Date: 2026-04-10
Scope: Full-repo code + docs audit, targeted fixes, new test coverage, and doc reconciliation
Verification: pnpm typecheck (7/7 packages) + pnpm test (56/56 unit tests) all green after changes

This file is a point-in-time record of the review that produced the edits below. It is intentionally dated and should not be treated as living documentation. For ongoing status, see docs/status.md.

Verdict

The repo is in materially good shape. No critical bugs, no security-incident-class issues, no silently-broken code paths. The issues found are of two kinds: (1) small correctness and semantic-drift cleanups that were worth fixing immediately, and (2) cosmetic / on-disk cruft that is safe to leave but worth the user's attention on a future pass.

Changes applied in this review

1. Docker migrate service switched to the transactional Node runner

docker-compose.yml — the migrate service previously ran an ad-hoc shell psql for-loop over db/migrations/*.sql. That path did not track applied versions in schema_migrations, did not wrap each migration in a transaction, and was not the same code path used by pnpm db:migrate locally.

Now the service builds the repo Dockerfile and runs pnpm --filter @cap/db migrate, which invokes packages/db/scripts/migrate.mjs — the same transactional Node runner already used for local development. Migrations are idempotent, transactional, and tracked.

Why this matters: production-ish and local now share one migration code path, so "migrated successfully on my machine" means the same thing as "migrated successfully in the compose stack."

2. WORKER_CLAIM_BATCH_SIZE semantic fix

Before: the config key WORKER_CLAIM_BATCH_SIZE was read in two places. The worker's claim path never used it (the loop always claims exactly one job per tick), but the reclaim path was using it as the LIMIT on the expired-lease reclaim SQL — a different operation with different tuning needs.

After:

• packages/config/src/index.ts adds a WORKER_RECLAIM_BATCH_SIZE key (default 25) and documents WORKER_CLAIM_BATCH_SIZE as reserved/dormant — accepted for forward compatibility with a future batch-claiming worker loop, but currently inert.
• apps/worker/src/queue/claim.ts::reclaimExpiredLeases now reads WORKER_RECLAIM_BATCH_SIZE instead of WORKER_CLAIM_BATCH_SIZE.
• .env.example documents both keys with an explanatory block.
• CLAUDE.md and docs/status.md are corrected — the previous "loop claims one at a time despite WORKER_CLAIM_BATCH_SIZE existing in config" wording is replaced with an accurate description, and the status-doc TODO "decide whether WORKER_CLAIM_BATCH_SIZE should be used or removed" is marked resolved.

Why this matters: claim tuning and reclaim tuning are independent concerns. Conflating them under one key would have made a future attempt to raise reclaim throughput accidentally change claim semantics.

3. formatDuration / formatTimestamp deduped

apps/web/src/lib/format.ts had two byte-identical functions — formatDuration (2 call sites) and formatTimestamp (25+ call sites) — both doing the same hh:mm:ss/mm:ss formatting. Easy to let one drift from the other.

Now formatDuration is the single implementation and formatTimestamp is exported as an alias to it (export const formatTimestamp = formatDuration;). All existing call sites continue to compile and run unchanged; output is guaranteed to stay consistent. Tests pass (19/19 in apps/web).

4. S3 public-endpoint default aligned

apps/web-api/src/lib/s3.ts::getS3ClientAndBucket had a fallback default of "http://localhost:9000" for S3_PUBLIC_ENDPOINT. The packages/config zod default is "http://localhost:8922" (the docker-compose host-mapped MinIO API port). Drift between the two meant that a misconfigured env file could produce presigned URLs pointing at the wrong host.

Now both default to http://localhost:8922 with an inline comment explaining the alignment.

5. Auth docs cross-referenced

docs/auth-plan.md and docs/review-auth-system.md are related but serve different roles (living status doc vs. dated code-review sign-off). Rather than merging them and losing the review's date context, auth-plan.md now explicitly cross-references the review file under a "Related review" section so readers landing on either doc know which one to trust for current state.

6. Closed the reclaim-test gap — new apps/worker/src/queue/claim.test.ts

docs/status.md previously listed "reclaim / expired-lease worker behavior still needs direct coverage" as a highest-risk gap. This review adds a new claim.test.ts file under apps/worker/src/queue/ covering:

• claimOne() — parameters passed to CLAIM_SQL (limit, worker id, lease interval), returned-row mapping, null case when no job is available, and the CLAIM_SQL_WITH_EXCLUDE variant when excludeTypes is non-empty
• reclaimExpiredLeases() — that it uses WORKER_RECLAIM_BATCH_SIZE (not WORKER_CLAIM_BATCH_SIZE) as the LIMIT parameter, that it returns the rows reclaim surfaced (mix of retryable + dead), and that it respects a custom env value

Mocking strategy mirrors the existing lease.test.ts — mock withTransaction from @cap/db, assert on SQL text and bind parameters rather than spinning up a real postgres. Six new tests; worker suite goes from 18 to 24 passing.

7. Residual doc drift corrected

Several docs still carried the old "WORKER_CLAIM_BATCH_SIZE exists but is ignored" wording or stale localhost:9000 examples. Fixed:

• docs/development.md: the example-env block for host-side local dev now uses localhost:8922 for both S3_ENDPOINT and S3_PUBLIC_ENDPOINT (aligned with docker-compose.yml and packages/config), with an explanatory comment about when to use the docker internal hostname instead. The worker-tuning table now lists both WORKER_CLAIM_BATCH_SIZE (reserved/dormant) and WORKER_RECLAIM_BATCH_SIZE (default 25).
• docs/system.md: both places that mentioned WORKER_CLAIM_BATCH_SIZE (the queue section around line 372 and the capacity-planning section around line 501) now describe the split correctly.
• docs/cap5_implementation_plan.md: the "decide whether WORKER_CLAIM_BATCH_SIZE should become real worker concurrency or be removed" TODO is resolved and replaced with the split description; added the new claim/reclaim test coverage to the Completed list.
• README.md: removed an empty ## Tooling section that had a trailing header with no content, and linked the new review doc.

Findings raised and explicitly rejected after ground-truthing

During the review I ran several parallel sub-audits. A number of the claims they produced were wrong; I verified each against real files before acting. These are recorded so the same false alarms don't get re-raised later:

• "pnpm --filter @cap/web-api start will fail because there is no start script." — False. apps/web-api/package.json has "start": "node --enable-source-maps dist/index.js". Same for apps/worker.
• "HMAC code is duplicated between apps/worker/src/lib/hmac.ts and apps/web-api/src/lib/hmac.ts." — False. The worker module is outbound signing for webhooks we send; the web-api module is inbound verification for media-server callbacks. Opposite directions; not dedupable without introducing a shared package, which is not worth the churn at the current repo size.
• "DeletedVideoSkipError in apps/worker/src/handlers/shared.ts is dead code." — False. It is thrown and caught across handler files as part of the soft-delete race path.
• "JWT_SECRET being optional in the shared zod schema is a bug." — False. docs/review-auth-system.md explicitly calls this out as intentional: worker and media-server must not crash on a missing JWT secret because only web-api needs it. web-api validates at runtime when a request that needs auth is handled.
• "apps/worker/src/lib/s3.ts duplicates apps/web-api/src/lib/s3.ts." — False. The worker module only ever builds the internal S3 client (server-to-MinIO); the web-api module builds both internal and presign-capable clients. Different surfaces.
• "Number(job.id) in apps/web-api/src/routes/jobs.ts loses precision." — Theoretically true for values > 2^53, but job.id is a bigserial nowhere near that range in this single-tenant app. Not a current bug.

Cruft found but not removed (requires manual cleanup by user)

These items are on-disk noise that file-deletion permissions in this session prevented me from removing. They are safe to leave in place — none of them affect correctness — but they are worth cleaning up by hand on a future pass.

1. nanobanana/ — ~371 MB directory of AI-generated PDFs, images, and a node_modules/ from an unrelated side experiment. Already in .gitignore. Not referenced by any shipping code path. Safe to rm -rf nanobanana.
2. .DS_Store files — 18+ on disk, none tracked in git (git ls-files | grep DS_Store is empty). Safe to run find . -name .DS_Store -delete from the repo root.
3. apps/web/src/components/ChapterList.tsx — imported only by its own test file (apps/web/src/__tests__/ChapterList.test.tsx). Production uses ChapterListInline.tsx. Because the test file is 10 green tests exercising legitimate chapter-list behavior, I chose not to stub or delete — removing both files means losing test coverage of a component that, while currently unused, encodes a reasonable spec. If you want to delete it, delete both files together; the rest of the app compiles without them.

If you'd like any of these removed in a follow-up session where deletion is permitted, say the word.

Verification

• pnpm build:internal — green (config, db, logger)
• pnpm typecheck — green across all 7 workspace projects (web, web-api, worker, media-server, config, db, logger)
• pnpm --filter @cap/web test — 19/19 passing
• pnpm --filter @cap/worker test — 24/24 passing (was 18; +6 new in claim.test.ts)
• pnpm --filter @cap/web-api test — 13/13 passing
• pnpm --filter @cap/media-server test — 0 tests (expected; no unit tests defined)
• Total: 56/56 unit tests passing after changes.

Files changed

Edited:

CLAUDE.md
README.md
.env.example
docker-compose.yml
packages/config/src/index.ts
apps/worker/src/queue/claim.ts
apps/web-api/src/lib/s3.ts
apps/web/src/lib/format.ts
docs/status.md
docs/auth-plan.md
docs/system.md
docs/development.md
docs/cap5_implementation_plan.md

Added:

apps/worker/src/queue/claim.test.ts   (new — 6 tests for claimOne + reclaimExpiredLeases)
docs/review-2026-04-10.md             (new — this file)

[Copilot]

Pull request overview

Point-in-time full-repo audit update that aligns runtime behavior/config/docs, closes a worker reclaim-test gap, and fixes a pre-existing Web E2E failure by updating Playwright role queries to match ARIA tab semantics.

Changes:

• Docker compose migrate service now runs the transactional Node migration runner used locally.
• Worker config semantics split: introduce WORKER_RECLAIM_BATCH_SIZE and update reclaim SQL path + add direct unit tests for claimOne()/reclaimExpiredLeases().
• Web/UI consistency fixes: dedupe formatDuration/formatTimestamp, align S3 public endpoint defaults, and fix Playwright locators for ARIA tabs; reconcile docs accordingly.

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
scripts/fix-web-e2e.sh	Adds a helper script intended to reproduce/verify the Web E2E fix (but currently includes commit/push automation).
docker-compose.yml	Switches migrate service to run pnpm --filter @cap/db migrate using the transactional runner.
packages/config/src/index.ts	Adds WORKER_RECLAIM_BATCH_SIZE and documents WORKER_CLAIM_BATCH_SIZE as reserved/dormant.
apps/worker/src/queue/claim.ts	Uses WORKER_RECLAIM_BATCH_SIZE for reclaim LIMIT.
apps/worker/src/queue/claim.test.ts	Adds unit coverage for claimOne() and reclaimExpiredLeases() parameterization and SQL selection.
apps/web/src/lib/format.ts	Dedupes timestamp/duration formatting via alias export.
apps/web/e2e/layout.spec.ts	Updates Playwright queries to use role="tab" for VideoRail tabs.
apps/web-api/src/lib/s3.ts	Aligns S3_PUBLIC_ENDPOINT fallback default to localhost:8922.
apps/web-api/src/routes/auth.test.ts	Replaces any logger mock with a typed Logger-shaped helper.
.env.example	Documents WORKER_RECLAIM_BATCH_SIZE and clarifies dormant WORKER_CLAIM_BATCH_SIZE.
README.md	Removes empty section and links to the new dated review doc.
CLAUDE.md	Updates worker claim/reclaim configuration wording.
docs/system.md	Updates worker claim/reclaim config semantics in queue/capacity sections.
docs/status.md	Updates coverage/gaps to reflect new reclaim tests and config split.
docs/development.md	Aligns S3 endpoint examples and updates worker tuning table.
docs/cap5_implementation_plan.md	Marks reclaim tests/config split as completed; updates remaining work list.
docs/auth-plan.md	Cross-references the dated auth review doc.
docs/review-2026-04-10.md	Adds a dated audit/changelog record and documents the Web E2E breakage/fix.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

This script is hard-coded to a specific PR branch name and assumes it can freely git checkout the branch. If it's meant to live in-repo, consider making it branch-agnostic (operate on the current branch) or moving these PR-specific instructions into the review doc instead; as-is it will go stale and can disrupt a developer with local changes.

[Copilot]

The script performs destructive side effects (git commit and git push) as part of its normal flow. For a repository script this is risky/unexpected; consider removing the commit/push steps (leave the script as a repro/verification helper) or gating them behind an explicit flag/prompt so running the script can't accidentally publish changes.