Security/p0 hardening#18
Open
dmvasheka wants to merge 8 commits into
Open
Conversation
- .github/workflows/ci.yml: lint/typecheck/test/build via turbo on PR+push to main; gitleaks secret scan; pnpm audit (non-blocking for now) - .dockerignore: .env* and local state no longer enter the Railway image build context (COPY . . previously baked local secrets into layers) - Node engines >=18 -> >=20 (+.nvmrc); fixes apps/docs (Next 16) typecheck - repair broken scripts: web check-types used Next15-only 'next typegen'; db/ai lint had no eslint config and eslint v8 vs flat-config v9 conflict - fix all 24 lint warnings in packages/ai and packages/db (proper types instead of any, globalEnv declared in turbo.json) - web lint ratcheted at 30 pre-existing warnings (new ones fail CI); burn-down tracked in IMPROVEMENTS - turbo: add test task; root 'pnpm test' Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…try init - GET /api/health: 200 ok/degraded (Redis is non-fatal cache), 503 when DB is unreachable; for Railway healthcheck and uptime monitors (TDD, 4 tests) - Sentry initializes only when SENTRY_DSN is set; tracesSampleRate via env Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Covers cursor validation (incl. injection-style input), LIKE wildcard escaping, translation overlay with per-field fallback, and DB error paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- @repo/ai reports token usage for every chat/embedding call via an onAiUsage hook (no signature changes for the 7 existing call sites) - AiUsageService records endpoint/model/tokens/cost into ai_usage (migration 20260612000001; best-effort — never breaks a request) - AiBudgetGuard returns 429 on /api/chat and /api/movies/:id/explain once today's spend reaches OPENAI_DAILY_BUDGET_USD (unset = unlimited) - embedding_model column on movies/tv_shows + backfill, so a future embedding-model switch knows what to re-embed - chat already defaults to gpt-4o-mini via OPENAI_CHAT_MODEL_SMALL env - explain endpoint also throttled 10/min (was unlimited) TDD: 11 new tests (pricing, recording, fail-open paths, budget guard) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s race Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
More reviews will be available in 17 minutes and 12 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughAdds CI and Node 20 pinning, .dockerignore, Turborepo/globalEnv, health endpoint and Redis/DB checks, conditional Sentry init, AI usage eventing and ai_usage persistence, daily budget guard enforcing OPENAI_DAILY_BUDGET_USD (429), DB/type migrations, tests, and docs updates. ChangesSecurity Hardening P1 Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
SESSION_RESUME.md (1)
35-40:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftP0 closure blockers are identified but key rotation timeline is critical.
Lines 35–40 list actions without which P0 is not closed:
- Key rotation (overdue since 2026-01-16) — BLOCKER
- Railway ADMIN_API_KEY setup
- Coordinated API+Web deployment
- Create PR
The key rotation is overdue for ~5 months. This is a critical security issue. IMPROVEMENTS.md §17.1 (line 2048) documents the overdue task, and SESSION_RESUME.md correctly calls it out.
Recommendation: Escalate key rotation to a hard requirement before merging PR. Current exposed keys (per IMPROVEMENTS.md §8.7, line 1089: "4 critical, 2 high-risk") should be rotated immediately upon PR approval.
Clarification: Line 39 notes "Локально уже добавлен в
.envэтой машины" — implies machine-specific key. Confirm with team whether prod ADMIN_API_KEY should be per-environment (dev/staging/prod) or same across all (not recommended due to key compromise risk). Document the decision.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SESSION_RESUME.md` around lines 35 - 40, SESSION_RESUME.md calls out an overdue key rotation for ADMIN_API_KEY and other exposed keys; before merging, rotate the compromised keys immediately and escalate rotation to a hard requirement: generate new ADMIN_API_KEY (openssl rand -hex 32), update Railway variable ADMIN_API_KEY and each environment's secret (.env entries per-environment rather than reusing one key), revoke old keys, deploy API+Web together, and update the PR security/p0-hardening description plus IMPROVEMENTS.md §17.1 and §8.7 to record rotation status and the decision about per-environment keys and machine-specific .env handling.
🧹 Nitpick comments (3)
IMPROVEMENTS.md (1)
2091-2093: 💤 Low valueClarify lint-warning fix status and test for regressions.
Line 2091 claims "Исправлены все 24 lint-warnings в packages/ai и packages/db", but the note implies these were blocking CI. If all 24 are fixed, the CI should pass cleanly. Line 2093 notes "
⚠️ web lint — храповик--max-warnings 30" — implying web lint still has warnings (30 threshold).For clarity in docs:
- Explicitly state: "packages/ai and packages/db: 0 warnings (fixed)" vs. "apps/web: 30 warnings (throttle threshold set)".
- Ensure CI actually fails on any new lint warnings in api/ai/db packages.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@IMPROVEMENTS.md` around lines 2091 - 2093, Update the IMPROVEMENTS.md text to explicitly state packages/ai and packages/db have 0 lint warnings (e.g., "packages/ai: 0 warnings (fixed); packages/db: 0 warnings (fixed)") and clarify apps/web retains 30 warnings with the throttle note ("apps/web: 30 warnings (throttle threshold set)"). Then modify the CI lint job (the workflow that runs lint for api/ai/db packages) to fail on any new warnings by setting the linter invocation for api/ai/db to use zero-tolerance (e.g., eslint/tsc with --max-warnings=0 or equivalent) so new lint warnings will break the build; ensure job names referencing api/ai/db reflect this change.apps/api/src/ai-usage/__tests__/ai-budget.guard.spec.ts (1)
15-53: ⚡ Quick winAdd a regression test for “service throws → fail-open”.
Please add a case where
getTodaySpendUsd()rejects/throws and assertcanActivate()still resolvestrue. That locks in the intended availability behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/ai-usage/__tests__/ai-budget.guard.spec.ts` around lines 15 - 53, Add a test to assert AiBudgetGuard.fail-open behavior when the usage service throws: create a guard using a usage service whose getTodaySpendUsd() returns a rejected promise (or throws), call guard.canActivate(ctx()) and assert it resolves to true; reference AiBudgetGuard, usageServiceWithSpend (or a similar mocked service), getTodaySpendUsd, and canActivate so the new test mirrors the existing tests for budget absent/invalid but uses a throwing/rejecting getTodaySpendUsd to validate availability is preserved.apps/api/src/movies/__tests__/movies.service.autocomplete.spec.ts (1)
98-100: ⚡ Quick winStrengthen the error-path assertion to prevent false positives.
rejects.toBeTruthy()can pass for unrelated rejection shapes. Assert the expected error message/type so this test protects the real contract.Proposed change
- await expect(service.autocomplete('test')).rejects.toBeTruthy(); + await expect(service.autocomplete('test')).rejects.toThrow('db down');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/movies/__tests__/movies.service.autocomplete.spec.ts` around lines 98 - 100, The test currently uses a loose assertion rejects.toBeTruthy() which can mask unrelated errors; update the expectation to assert the exact error shape/message returned when Supabase errors: after setting queryState.result = { data: null, error: { message: 'db down' } }, change the assertion to expect service.autocomplete('test') to reject with an Error (or the specific error class) containing 'db down' (e.g., rejects.toThrow('db down') or rejects.toMatchObject({ message: 'db down' })), so the test for autocomplete on Service.autocomplete validates the specific error contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 24-25: The checkout steps currently use actions/checkout@v4
without disabling credential persistence; update both checkout steps (the
actions/checkout@v4 uses at the top-level and the second checkout in the other
job) to include persist-credentials: false so the runner does not write the
token to local git config—locate the checkout steps referencing
actions/checkout@v4 and add persist-credentials: false to each step.
- Around line 24-30: The workflow uses mutable action tags (e.g.,
actions/checkout@v4, pnpm/action-setup@v4, actions/setup-node@v4) which can be
altered by upstream tag updates; replace each mutable tag with the corresponding
immutable commit SHA for that action (fetch the current commit SHA from the
action's GitHub repository and substitute it in the uses: field), and do the
same for the other occurrences noted in the comment (lines referencing `@v`* such
as the entries around lines 50–55, 59–63) so all actions are pinned to exact
SHAs (keep the same action owner/name but use the full commit SHA).
- Around line 1-9: The workflow is missing an explicit least-privilege
permissions block; add a top-level permissions mapping under the "name: CI"
workflow (and if necessary override per job) that only grants the specific
scopes the workflow actually needs (for example: permissions: contents: read,
packages: read) instead of relying on defaults, and for any job that requires
additional rights adjust that job's permissions block inside "jobs" to narrowly
grant only what it needs.
In `@apps/api/src/ai-usage/__tests__/ai-usage.service.spec.ts`:
- Around line 16-18: The Supabase mock in the test discards the arguments to
select() and gte(), so update the mock to capture and assert those args for
getTodaySpendUsd(): replace the current select: () => ({ gte: () =>
Promise.resolve(state.selectResult) }) with a jest.fn-based mock that records
the select(columns) argument and returns an object whose gte(column, value) is
also a jest.fn that asserts column === 'created_at' (or the expected column) and
that value equals the start-of-day date your code uses (e.g., new
Date(...).toISOString() or state.startOfToday), then returns
Promise.resolve(state.selectResult); ensure you reference the getTodaySpendUsd()
call in the test and assert the mock functions were called with the expected
parameters.
In `@apps/api/src/ai-usage/ai-budget.guard.ts`:
- Around line 22-30: The guard currently fails closed if
AiUsageService.getTodaySpendUsd() throws; wrap the spend lookup and comparison
in a try/catch inside the method containing the existing logic (where spend is
assigned and compared to budget), call this.logger.warn or this.logger.error
with the caught error for observability, and on any exception return true to
preserve the fail-open (availability-first) behavior instead of throwing
HttpException(HttpStatus.TOO_MANY_REQUESTS); keep the existing warn+throw path
when spend is successfully retrieved and exceeds budget.
In `@apps/api/src/ai-usage/ai-usage.service.ts`:
- Around line 63-72: getTodaySpendUsd currently selects all cost_usd rows and
reduces in Node; change it to perform a DB-side aggregation (SUM) to avoid
fetching every row: in the getTodaySpendUsd method/query against the ai_usage
table, replace the .select('cost_usd').gte('created_at',
startOfDay.toISOString()) call with a single aggregate select like
.select('sum(cost_usd) as total') (or DB client equivalent), filter by
created_at the same way, then read the returned total value (handle
null/undefined by returning 0) and preserve existing error logging
(this.logger.warn on error) and numeric return type.
In `@IMPROVEMENTS.md`:
- Line 2126: Update the note about migration files into an explicit actionable
item: either convert the buried clarification into a checklist TODO like "[ ]
Consolidate migrations: move the single file in packages/db/supabase/migrations
into the canonical supabase/migrations/ directory" or mark it completed "[x]" if
you already consolidated; then remove the duplicate in
packages/db/supabase/migrations so only supabase/migrations/*.sql remain as the
canonical location per guidelines and ensure the IMPROVEMENTS.md line
referencing "packages/db/supabase/migrations" is updated to reflect the final
state.
In `@packages/ai/src/usage.ts`:
- Around line 6-10: AiUsageEvent currently only allows 'chat'|'embedding',
causing generateMovieExplanation() to report as 'chat' and hide explain requests
in ai_usage.endpoint; update the AiUsageEvent type to include a distinct kind
(e.g., 'movie_explanation' or 'explain_movie'), change
generateMovieExplanation() to emit that new kind, and ensure any code that
persists event.kind into ai_usage.endpoint (the API module that writes ai_usage)
continues to accept and store the new value so explain requests are tracked
separately from chat.
In `@packages/db/src/supabase.client.ts`:
- Line 34: The connectivity probe uses supabase.from('movies').select('count'),
which requests a non-existent column and can yield false negatives; change the
probe to request existing data instead (e.g. use
supabase.from('movies').select('*') or select('id', { count: 'exact' }) and add
a lightweight limit(1)) so the query verifies connectivity without relying on a
non-existent "count" column; update the call to replace
supabase.from('movies').select('count') with one of these alternatives in the
connectivity probe function.
In `@SESSION_RESUME.md`:
- Line 3: Update the ambiguous branch description and clarify environment
variable setup: change the phrase "включает и P1" to "включает также реализацию
P1 (инженерная база)" in the Branch line, and add explicit instructions for
ADMIN_API_KEY and deployment coordination — state that local developers should
generate their own ADMIN_API_KEY (e.g., via openssl) and store it in .env (which
remains out of git), while Production/Staging must set the same ADMIN_API_KEY
for both API and Web via Railway Variables UI or a secrets manager; also add
deployment guidance to coordinate API and Web releases (or provide a
compatibility/fallback plan so new API returning 401 doesn’t break an old Web)
to avoid mismatched inter-service auth.
In `@supabase/migrations/20260612000001_add_ai_usage.sql`:
- Around line 7-9: Add DB-level CHECK constraints to prevent negative values on
the usage/accounting columns: add CHECK (prompt_tokens >= 0), CHECK
(completion_tokens >= 0), and CHECK (cost_usd >= 0). Implement these in the
migration that defines or alters the table containing prompt_tokens,
completion_tokens, and cost_usd (use explicit constraint names like
chk_prompt_tokens_nonneg, chk_completion_tokens_nonneg, chk_cost_usd_nonneg) so
that the schema enforces non-negative invariants in addition to the existing NOT
NULL and DEFAULT clauses.
---
Outside diff comments:
In `@SESSION_RESUME.md`:
- Around line 35-40: SESSION_RESUME.md calls out an overdue key rotation for
ADMIN_API_KEY and other exposed keys; before merging, rotate the compromised
keys immediately and escalate rotation to a hard requirement: generate new
ADMIN_API_KEY (openssl rand -hex 32), update Railway variable ADMIN_API_KEY and
each environment's secret (.env entries per-environment rather than reusing one
key), revoke old keys, deploy API+Web together, and update the PR
security/p0-hardening description plus IMPROVEMENTS.md §17.1 and §8.7 to record
rotation status and the decision about per-environment keys and machine-specific
.env handling.
---
Nitpick comments:
In `@apps/api/src/ai-usage/__tests__/ai-budget.guard.spec.ts`:
- Around line 15-53: Add a test to assert AiBudgetGuard.fail-open behavior when
the usage service throws: create a guard using a usage service whose
getTodaySpendUsd() returns a rejected promise (or throws), call
guard.canActivate(ctx()) and assert it resolves to true; reference
AiBudgetGuard, usageServiceWithSpend (or a similar mocked service),
getTodaySpendUsd, and canActivate so the new test mirrors the existing tests for
budget absent/invalid but uses a throwing/rejecting getTodaySpendUsd to validate
availability is preserved.
In `@apps/api/src/movies/__tests__/movies.service.autocomplete.spec.ts`:
- Around line 98-100: The test currently uses a loose assertion
rejects.toBeTruthy() which can mask unrelated errors; update the expectation to
assert the exact error shape/message returned when Supabase errors: after
setting queryState.result = { data: null, error: { message: 'db down' } },
change the assertion to expect service.autocomplete('test') to reject with an
Error (or the specific error class) containing 'db down' (e.g.,
rejects.toThrow('db down') or rejects.toMatchObject({ message: 'db down' })), so
the test for autocomplete on Service.autocomplete validates the specific error
contract.
In `@IMPROVEMENTS.md`:
- Around line 2091-2093: Update the IMPROVEMENTS.md text to explicitly state
packages/ai and packages/db have 0 lint warnings (e.g., "packages/ai: 0 warnings
(fixed); packages/db: 0 warnings (fixed)") and clarify apps/web retains 30
warnings with the throttle note ("apps/web: 30 warnings (throttle threshold
set)"). Then modify the CI lint job (the workflow that runs lint for api/ai/db
packages) to fail on any new warnings by setting the linter invocation for
api/ai/db to use zero-tolerance (e.g., eslint/tsc with --max-warnings=0 or
equivalent) so new lint warnings will break the build; ensure job names
referencing api/ai/db reflect this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dee62b59-a982-4305-abcb-cef229a4278b
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (39)
.dockerignore.env.example.github/workflows/ci.yml.nvmrcCLAUDE.mdIMPROVEMENTS.mdSESSION_RESUME.mdapps/api/package.jsonapps/api/src/ai-usage/__tests__/ai-budget.guard.spec.tsapps/api/src/ai-usage/__tests__/ai-usage.service.spec.tsapps/api/src/ai-usage/ai-budget.guard.tsapps/api/src/ai-usage/ai-usage.module.tsapps/api/src/ai-usage/ai-usage.service.tsapps/api/src/app.module.tsapps/api/src/chat/chat.controller.tsapps/api/src/health/__tests__/health.service.spec.tsapps/api/src/health/health.controller.tsapps/api/src/health/health.module.tsapps/api/src/health/health.service.tsapps/api/src/instrument.tsapps/api/src/main.tsapps/api/src/movies/__tests__/movies.service.autocomplete.spec.tsapps/api/src/movies/movies.controller.tsapps/api/src/utils/__tests__/cursor.utils.spec.tsapps/web/package.jsonpackage.jsonpackages/ai/eslint.config.mjspackages/ai/package.jsonpackages/ai/src/chat.tspackages/ai/src/embeddings.tspackages/ai/src/index.tspackages/ai/src/openai.client.tspackages/ai/src/usage.tspackages/db/eslint.config.mjspackages/db/package.jsonpackages/db/src/supabase.client.tspackages/eslint-config/base.jssupabase/migrations/20260612000001_add_ai_usage.sqlturbo.json
Comment on lines
+22
to
+30
| const spend = await this.aiUsageService.getTodaySpendUsd(); | ||
| if (spend >= budget) { | ||
| this.logger.warn(`Daily AI budget exhausted: $${spend.toFixed(4)} >= $${budget}`); | ||
| throw new HttpException( | ||
| 'Daily AI budget exhausted, try again tomorrow', | ||
| HttpStatus.TOO_MANY_REQUESTS, | ||
| ); | ||
| } | ||
| return true; |
There was a problem hiding this comment.
Preserve fail-open behavior if spend lookup throws.
The guard currently fails closed on unexpected AiUsageService exceptions. Wrap the spend lookup/comparison in try/catch and allow the request on error to match the documented availability-first contract.
Suggested patch
- const spend = await this.aiUsageService.getTodaySpendUsd();
- if (spend >= budget) {
- this.logger.warn(`Daily AI budget exhausted: $${spend.toFixed(4)} >= $${budget}`);
- throw new HttpException(
- 'Daily AI budget exhausted, try again tomorrow',
- HttpStatus.TOO_MANY_REQUESTS,
- );
- }
- return true;
+ try {
+ const spend = await this.aiUsageService.getTodaySpendUsd();
+ if (spend >= budget) {
+ this.logger.warn(`Daily AI budget exhausted: $${spend.toFixed(4)} >= $${budget}`);
+ throw new HttpException(
+ 'Daily AI budget exhausted, try again tomorrow',
+ HttpStatus.TOO_MANY_REQUESTS,
+ );
+ }
+ return true;
+ } catch (error) {
+ const message = error instanceof Error ? error.message : String(error);
+ this.logger.warn(`Budget check failed open: ${message}`);
+ return true;
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/ai-usage/ai-budget.guard.ts` around lines 22 - 30, The guard
currently fails closed if AiUsageService.getTodaySpendUsd() throws; wrap the
spend lookup and comparison in a try/catch inside the method containing the
existing logic (where spend is assigned and compared to budget), call
this.logger.warn or this.logger.error with the caught error for observability,
and on any exception return true to preserve the fail-open (availability-first)
behavior instead of throwing HttpException(HttpStatus.TOO_MANY_REQUESTS); keep
the existing warn+throw path when spend is successfully retrieved and exceeds
budget.
Comment on lines
+7
to
+9
| prompt_tokens INTEGER NOT NULL DEFAULT 0, | ||
| completion_tokens INTEGER NOT NULL DEFAULT 0, | ||
| cost_usd NUMERIC(12, 8) NOT NULL DEFAULT 0, |
There was a problem hiding this comment.
Add DB-level non-negative constraints for usage accounting fields.
Line 7–9 store financial/accounting data but currently allow negative values. A bad insert can undercount daily spend and weaken budget enforcement. Enforce invariants in schema, not only application code.
Suggested migration hardening
CREATE TABLE IF NOT EXISTS ai_usage (
id BIGSERIAL PRIMARY KEY,
endpoint TEXT NOT NULL, -- 'chat' | 'embedding' | ...
model TEXT NOT NULL,
- prompt_tokens INTEGER NOT NULL DEFAULT 0,
- completion_tokens INTEGER NOT NULL DEFAULT 0,
- cost_usd NUMERIC(12, 8) NOT NULL DEFAULT 0,
+ prompt_tokens INTEGER NOT NULL DEFAULT 0 CHECK (prompt_tokens >= 0),
+ completion_tokens INTEGER NOT NULL DEFAULT 0 CHECK (completion_tokens >= 0),
+ cost_usd NUMERIC(12, 8) NOT NULL DEFAULT 0 CHECK (cost_usd >= 0),
user_id UUID, -- optional attribution
created_at TIMESTAMPTZ NOT NULL DEFAULT now()
);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@supabase/migrations/20260612000001_add_ai_usage.sql` around lines 7 - 9, Add
DB-level CHECK constraints to prevent negative values on the usage/accounting
columns: add CHECK (prompt_tokens >= 0), CHECK (completion_tokens >= 0), and
CHECK (cost_usd >= 0). Implement these in the migration that defines or alters
the table containing prompt_tokens, completion_tokens, and cost_usd (use
explicit constraint names like chk_prompt_tokens_nonneg,
chk_completion_tokens_nonneg, chk_cost_usd_nonneg) so that the schema enforces
non-negative invariants in addition to the existing NOT NULL and DEFAULT
clauses.
…h ways Applied via Supabase Management API (project had been PAUSED by free tier — production was down; restored first): - 20260501152500_add_search_vectors (smart-search Phase 1 DB layer) - 20260505000001_restore_match_translations - 20260612000001_add_ai_usage (embedding_model backfill: 7697 movies, 3030 tv) Drift fixes: - 20260130000001_add_imdb_fields recovered from prod history into the repo (was applied via dashboard, never committed) - migration history table synced (17 entries) - generated-types.ts regenerated; ai_usage added to manual types.ts (manual/generated consolidation tracked in IMPROVEMENTS §18.5) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@supabase/migrations/20260130000001_add_imdb_fields.sql`:
- Around line 6-11: Add DB-level CHECK constraints to enforce imdb_rating is
between 0.0 and 10.0 and imdb_votes is non-negative for both tables: create
named constraints on movies (e.g., chk_movies_imdb_rating_range,
chk_movies_imdb_votes_nonneg) and on tv_shows (e.g.,
chk_tv_shows_imdb_rating_range, chk_tv_shows_imdb_votes_nonneg) using ALTER
TABLE ... ADD CONSTRAINT ... CHECK (imdb_rating >= 0.0 AND imdb_rating <= 10.0)
and CHECK (imdb_votes >= 0); ensure the migration handles existing constraints
safely (check for constraint existence or use a conditional DO block) so
movies.imdb_rating, movies.imdb_votes, tv_shows.imdb_rating and
tv_shows.imdb_votes are validated at the DB level.
- Line 21: The migration contains a SQL statement "COMMENT ON COLUMN
tv_shows.imdb_votes IS 'Number of IMDb votes'" missing a trailing semicolon
which breaks parsing; update the COMMENT ON COLUMN statement for table tv_shows
and column imdb_votes (the line with COMMENT ON COLUMN tv_shows.imdb_votes IS
'Number of IMDb votes') to include a terminating semicolon so the migration file
parses and applies correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e9abbe9d-2237-480e-af66-a3b2a3c1bce7
📒 Files selected for processing (6)
IMPROVEMENTS.mdSESSION_RESUME.mdapps/api/src/ai-usage/ai-usage.service.tspackages/db/src/generated-types.tspackages/db/src/types.tssupabase/migrations/20260130000001_add_imdb_fields.sql
✅ Files skipped from review due to trivial changes (4)
- packages/db/src/types.ts
- SESSION_RESUME.md
- IMPROVEMENTS.md
- packages/db/src/generated-types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/src/ai-usage/ai-usage.service.ts
Comment on lines
+6
to
+11
| ALTER TABLE movies ADD COLUMN IF NOT EXISTS imdb_rating DECIMAL(3,1); | ||
| ALTER TABLE movies ADD COLUMN IF NOT EXISTS imdb_votes INTEGER; | ||
| -- Add IMDb fields to tv_shows table | ||
| ALTER TABLE tv_shows ADD COLUMN IF NOT EXISTS imdb_id TEXT; | ||
| ALTER TABLE tv_shows ADD COLUMN IF NOT EXISTS imdb_rating DECIMAL(3,1); | ||
| ALTER TABLE tv_shows ADD COLUMN IF NOT EXISTS imdb_votes INTEGER; |
There was a problem hiding this comment.
Add DB-level constraints for IMDb value ranges.
The comments (Lines 17 and 20) define imdb_rating as 0.0-10.0, and imdb_votes should never be negative, but no constraints enforce this. Invalid values can be persisted.
Suggested fix
ALTER TABLE tv_shows ADD COLUMN IF NOT EXISTS imdb_votes INTEGER;
+
+ALTER TABLE movies
+ ADD CONSTRAINT movies_imdb_rating_range_chk
+ CHECK (imdb_rating IS NULL OR (imdb_rating >= 0.0 AND imdb_rating <= 10.0)) NOT VALID;
+ALTER TABLE movies
+ ADD CONSTRAINT movies_imdb_votes_nonnegative_chk
+ CHECK (imdb_votes IS NULL OR imdb_votes >= 0) NOT VALID;
+
+ALTER TABLE tv_shows
+ ADD CONSTRAINT tv_shows_imdb_rating_range_chk
+ CHECK (imdb_rating IS NULL OR (imdb_rating >= 0.0 AND imdb_rating <= 10.0)) NOT VALID;
+ALTER TABLE tv_shows
+ ADD CONSTRAINT tv_shows_imdb_votes_nonnegative_chk
+ CHECK (imdb_votes IS NULL OR imdb_votes >= 0) NOT VALID;Also applies to: 17-21
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@supabase/migrations/20260130000001_add_imdb_fields.sql` around lines 6 - 11,
Add DB-level CHECK constraints to enforce imdb_rating is between 0.0 and 10.0
and imdb_votes is non-negative for both tables: create named constraints on
movies (e.g., chk_movies_imdb_rating_range, chk_movies_imdb_votes_nonneg) and on
tv_shows (e.g., chk_tv_shows_imdb_rating_range, chk_tv_shows_imdb_votes_nonneg)
using ALTER TABLE ... ADD CONSTRAINT ... CHECK (imdb_rating >= 0.0 AND
imdb_rating <= 10.0) and CHECK (imdb_votes >= 0); ensure the migration handles
existing constraints safely (check for constraint existence or use a conditional
DO block) so movies.imdb_rating, movies.imdb_votes, tv_shows.imdb_rating and
tv_shows.imdb_votes are validated at the DB level.
CI was red: pnpm/action-setup 'version: 9' conflicts with packageManager in
package.json — version key removed, packageManager is the single source.
Review fixes:
- workflow: top-level 'permissions: contents: read', persist-credentials:
false on both checkouts, all actions pinned to commit SHAs (annotated
tags dereferenced), gitleaks PR comments disabled to match permissions
- @repo/ai: distinct 'explain' usage kind so explanations are tracked
separately from chat in ai_usage.endpoint
- testConnection probe: select('count') referenced a non-existent column
(every health check would report DB down) -> select('id').limit(1)
- ai-usage spec asserts select/gte arguments (created_at, UTC midnight)
- migrations: trailing semicolon in recovered add_imdb_fields; duplicate
packages/db/supabase/migrations/003 removed (identical to 20260117000001);
new 20260612000002_add_check_constraints (non-negative tokens/cost,
imdb_rating 0..10, imdb_votes >= 0) — applied to prod, history synced
- docs: ADMIN_API_KEY setup and API+Web deploy coordination in SESSION_RESUME
Skipped finding #5 (budget guard fail-closed on exception): incorrect —
getTodaySpendUsd() never throws by design (internal catch returns 0,
covered by the 'returns 0 when the query fails' test).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jest resolves workspace packages through their dist entrypoints; on a clean CI runner the test task ran before any build, so module resolution failed. test now dependsOn ^build (reproduced locally by removing packages/*/dist). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.