[codex] Store uploaded manifest file sizes#2265
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements end-to-end manifest file-size tracking and improves queue resilience. Changes add a new ChangesManifest file_size tracking and queue resilience
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Merging this PR will not alter performance
Comparing 
Footnotes
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
riderx
force-pushed
the
codex/manifest-file-size-upload
branch
3 times, most recently
from
b0e3498 to
e775849
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
riderx
force-pushed
the
codex/manifest-file-size-upload
branch
from
e775849 to
5555c5c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
tests/files-app-read-guard.unit.test.ts (1)
37-37: ⚡ Quick winConvert these tests to
it.concurrent(...).Both test declarations should use concurrent execution per the test guideline for parallel test execution across files.
Suggested diff
- it('serves cached app-scoped files without a database lookup', async () => { + it.concurrent('serves cached app-scoped files without a database lookup', async () => { ... - it('serves malformed cached paths without an app lookup', async () => { + it.concurrent('serves malformed cached paths without an app lookup', async () => {Also applies to: 73-73
🤖 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 `@tests/files-app-read-guard.unit.test.ts` at line 37, Change the two test declarations that currently use it(...) to use it.concurrent(...); specifically update the test with description "serves cached app-scoped files without a database lookup" and the other test at the second occurrence (around the "Also applies to" note) so both are declared as it.concurrent(...) instead of it(...), leaving the test bodies and assertions untouched to enable concurrent execution.tests/files-security.test.ts (1)
220-220: ⚡ Quick winUse
it.concurrentfor this test case.Switch this test declaration to
it.concurrent(...)to align with the parallel-test requirement.Suggested diff
- it('serves uploaded attachments from storage after the app is deleted', async () => { + it.concurrent('serves uploaded attachments from storage after the app is deleted', async () => {Per the coding guideline:
tests/**/*.test.tsshould useit.concurrent()instead ofit()to enable parallel execution within the same file for faster CI/CD.🤖 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 `@tests/files-security.test.ts` at line 220, The test declaration for "serves uploaded attachments from storage after the app is deleted" uses it(...) but must use parallel execution; replace the existing it('serves uploaded attachments from storage after the app is deleted', async () => ...) with it.concurrent('serves uploaded attachments from storage after the app is deleted', async () => ...) so the test runs in parallel with other tests in the file (no other code changes required).supabase/functions/_backend/triggers/on_manifest_create.ts (1)
85-87: ⚡ Quick winDead code: this condition is now unreachable.
The early exit at lines 70-73 ensures that when execution reaches line 85,
record.file_sizeis guaranteed to benullor<= 0. The checkif (record.file_size && record.file_size > 0)will always be false here.♻️ Proposed cleanup
const { size, lastError } = await getManifestSizeWithRetry(c, record.s3_path) if (lastError) { cloudlogErr({ requestId: c.get('requestId'), message: 'getSize failed after retries', id: record.id, s3_path: record.s3_path, error: lastError }) } if (size === 0) { - if (record.file_size && record.file_size > 0) { - cloudlog({ requestId: c.get('requestId'), message: 'getSize returned 0, keeping existing file_size', id: record.id, s3_path: record.s3_path, file_size: record.file_size }) - return c.json(BRES) - } cloudlog({ requestId: c.get('requestId'), message: 'getSize returned 0 after retries, skipping update', id: record.id, s3_path: record.s3_path }) return c.json(BRES) }🤖 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/functions/_backend/triggers/on_manifest_create.ts` around lines 85 - 87, The if-check "if (record.file_size && record.file_size > 0)" is unreachable because earlier code guarantees record.file_size is null or <= 0 by the time execution reaches this point; remove this dead branch (the cloudlog call and return c.json(BRES)) from the function that handles the manifest trigger so there’s no redundant logging/early return, and ensure any needed behavior is preserved by the earlier exit already using c.json(BRES) and cloudlog where appropriate (symbols: record, c, cloudlog, BRES).supabase/migrations/20260513152858_fix_queue_archive_cleanup.sql (1)
29-75: 💤 Low valueSonarCloud warning about newline is a false positive.
The warning flagging "illegal character with code point 10" (newline) in the SQL string literal at line 46 is a linter false positive. PostgreSQL accepts newlines in single-quoted string literals, and your dynamic SQL construction is correct:
- Uses
format()with%Ifor safe identifier quoting- Passes parameters via
USINGclause (not concatenated)- Has proper exception handling per queue
- Limits archive deletes to 50k rows to avoid long locks
The function follows all security best practices (SECURITY DEFINER, empty search_path, fully qualified names). If you want to silence the linter, you could use dollar-quoting (
$sql$...$sql$) instead of single quotes, but it's not necessary.🤖 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/20260513152858_fix_queue_archive_cleanup.sql` around lines 29 - 75, SonarCloud's "illegal character code point 10" warning is a false positive for the multi-line single-quoted SQL string inside function cleanup_queue_messages; to silence the linter without changing behavior, switch the EXECUTE format string to use dollar-quoting (e.g. $sql$...$sql$) instead of single quotes so the multi-line literal contains no escaped newlines, leaving the use of format(... %I ...) and USING archive_cutoff, archive_delete_limit unchanged and preserving the exception handling and SECURITY DEFINER/search_path setup.
🤖 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.
Nitpick comments:
In `@supabase/functions/_backend/triggers/on_manifest_create.ts`:
- Around line 85-87: The if-check "if (record.file_size && record.file_size >
0)" is unreachable because earlier code guarantees record.file_size is null or
<= 0 by the time execution reaches this point; remove this dead branch (the
cloudlog call and return c.json(BRES)) from the function that handles the
manifest trigger so there’s no redundant logging/early return, and ensure any
needed behavior is preserved by the earlier exit already using c.json(BRES) and
cloudlog where appropriate (symbols: record, c, cloudlog, BRES).
In `@supabase/migrations/20260513152858_fix_queue_archive_cleanup.sql`:
- Around line 29-75: SonarCloud's "illegal character code point 10" warning is a
false positive for the multi-line single-quoted SQL string inside function
cleanup_queue_messages; to silence the linter without changing behavior, switch
the EXECUTE format string to use dollar-quoting (e.g. $sql$...$sql$) instead of
single quotes so the multi-line literal contains no escaped newlines, leaving
the use of format(... %I ...) and USING archive_cutoff, archive_delete_limit
unchanged and preserving the exception handling and SECURITY DEFINER/search_path
setup.
In `@tests/files-app-read-guard.unit.test.ts`:
- Line 37: Change the two test declarations that currently use it(...) to use
it.concurrent(...); specifically update the test with description "serves cached
app-scoped files without a database lookup" and the other test at the second
occurrence (around the "Also applies to" note) so both are declared as
it.concurrent(...) instead of it(...), leaving the test bodies and assertions
untouched to enable concurrent execution.
In `@tests/files-security.test.ts`:
- Line 220: The test declaration for "serves uploaded attachments from storage
after the app is deleted" uses it(...) but must use parallel execution; replace
the existing it('serves uploaded attachments from storage after the app is
deleted', async () => ...) with it.concurrent('serves uploaded attachments from
storage after the app is deleted', async () => ...) so the test runs in parallel
with other tests in the file (no other code changes required).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d13abc1-8560-4a80-a431-adae046075d5
📒 Files selected for processing (11)
cli/src/bundle/partial.tscli/src/types/supabase.types.tssupabase/functions/_backend/files/files.tssupabase/functions/_backend/triggers/on_manifest_create.tssupabase/functions/_backend/triggers/on_version_update.tssupabase/functions/_backend/triggers/queue_consumer.tssupabase/functions/_backend/utils/supabase.types.tssupabase/migrations/20260513152858_fix_queue_archive_cleanup.sqltests/files-app-read-guard.unit.test.tstests/files-local-read-proxy.unit.test.tstests/files-security.test.ts
|
This looks like it turns The migration changes A modified CLI or direct API caller can set any small positive I think the optimization needs a trust boundary: either keep the R2 HEAD verification for client-provided positive sizes, or only skip the trigger when the row is written by a trusted backend path that has independently measured the uploaded object size. At minimum, the DB trigger should not let arbitrary positive |
riderx
force-pushed
the
codex/manifest-file-size-upload
branch
from
5555c5c to
f8ae012
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/functions/_backend/files/supabaseTusProxy.ts`:
- Around line 347-349: The call to recordSupabaseCompletedTusUpload is on the
hot path and must not propagate errors; wrap the await
recordSupabaseCompletedTusUpload(c, uploadId, responseHeaders) in a try/catch
(for the occurrence around the response.status >= 200 check and the other
occurrence at lines ~384-386), catch any error and log it (including error
details and context such as uploadId and responseHeaders) instead of rethrowing
so a successful storage upload does not fail at the API boundary.
- Around line 265-267: In recordSupabaseCompletedTusUpload, don't unguardedly
cast c.get('fileId') to a string; instead read fileId into a local (e.g., const
fileId = c.get('fileId')), verify it's a non-empty string (typeof fileId ===
'string' && fileId.trim() !== '') and if not, log a clear warning/error and
return early (or throw a controlled error) so subsequent use of s3Path (formerly
s3Path = ...) is never executed with an invalid key; update any references to
use the validated local variable (fileId or s3Path) so callers bail safely.
- Around line 381-385: copyResponseHeaders is currently only copying
'Upload-Offset' and 'Upload-Expires' from the PATCH response, so
recordSupabaseCompletedTusUpload falls back to a HEAD call to determine
completion; modify the call site where copyResponseHeaders(response.headers,
responseHeaders, ['Upload-Offset', 'Upload-Expires']) is invoked to include
'Upload-Length' in the header list so responseHeaders contains Upload-Length
(i.e., use ['Upload-Offset','Upload-Expires','Upload-Length']), then ensure
readErrorBody and the subsequent check that calls
recordSupabaseCompletedTusUpload(c, uploadId, responseHeaders) will be able to
detect completion without issuing the extra HEAD request.
In `@supabase/migrations/20260513152858_fix_queue_archive_cleanup.sql`:
- Around line 120-126: The conversion of process_all_cron_tasks() to SECURITY
DEFINER must also pin the search_path to an empty value and ensure
fully-qualified object names are used; update the function's properties to
include SET search_path = '' (or equivalent empty search_path setting) when
altering security, and audit the function body referenced by
process_all_cron_tasks to replace any unqualified table/schema references with
fully qualified names (schema.table) so definer execution cannot be hijacked by
path-based resolution.
- Around line 60-64: The trigger on_manifest_create currently skips the fallback
queue for any positive NEW.file_size, allowing forged sizes to bypass backend
revalidation; update the WHEN condition so the queue is only bypassed if
file_size is positive AND the write is explicitly marked as verified by a
trusted path (e.g., require NEW.size_verified_by_backend = TRUE or a similar
trusted flag/claim), otherwise ensure
public.trigger_http_queue_post_to_function('on_manifest_create') runs for
non-trusted writes; locate the CREATE TRIGGER on_manifest_create and modify the
WHEN clause to require both NEW.file_size > 0 and the trusted marker (or add a
boolean column like size_verified_by_backend and check it) so positive sizes
from untrusted sources still enqueue for server-side verification.
In `@tests/manifest-uploaded-size.unit.test.ts`:
- Line 42: Change the single test declaration in
tests/manifest-uploaded-size.unit.test.ts named "hydrates manifest file_size
from backend-observed upload rows only" from using it(...) to
it.concurrent(...); locate the it(...) call for that test and replace it with
it.concurrent(...) so the test runs in parallel with other tests in the same
file, preserving the existing test body and async behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d4ba0d8a-34b2-457b-8a62-50bec6d5f625
📒 Files selected for processing (15)
cli/src/bundle/partial.tscli/src/types/supabase.types.tssupabase/functions/_backend/files/files.tssupabase/functions/_backend/files/supabaseTusProxy.tssupabase/functions/_backend/files/uploadHandler.tssupabase/functions/_backend/files/uploadSize.tssupabase/functions/_backend/triggers/on_manifest_create.tssupabase/functions/_backend/triggers/on_version_update.tssupabase/functions/_backend/triggers/queue_consumer.tssupabase/functions/_backend/utils/supabase.types.tssupabase/migrations/20260513152858_fix_queue_archive_cleanup.sqltests/files-app-read-guard.unit.test.tstests/files-local-read-proxy.unit.test.tstests/files-security.test.tstests/manifest-uploaded-size.unit.test.ts
✅ Files skipped from review due to trivial changes (1)
- supabase/functions/_backend/utils/supabase.types.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/files-security.test.ts
- supabase/functions/_backend/triggers/on_manifest_create.ts
- supabase/functions/_backend/triggers/queue_consumer.ts
- tests/files-local-read-proxy.unit.test.ts
- tests/files-app-read-guard.unit.test.ts
riderx
force-pushed
the
codex/manifest-file-size-upload
branch
2 times, most recently
from
9de40ff to
e64dfcd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/20260513152858_fix_queue_archive_cleanup.sql`:
- Around line 149-150: The DELETE against pgmq.q_||queue_name is unbounded and
should be chunked like the archive cleanup; replace the single EXECUTE
format('DELETE FROM %I.%I WHERE read_ct > 5', 'pgmq', 'q_' || queue_name) with a
looped/chunked DELETE (e.g. DELETE ... WHERE read_ct > 5 AND ctid IN (SELECT
ctid FROM %I.%I WHERE read_ct > 5 LIMIT <batch_size>)) executed via EXECUTE
format(...) until no rows are deleted, so the code repeatedly removes small
batches from the table (pgmq, 'q_' || queue_name) rather than one long
transaction.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3dae449-905c-4b37-8fbd-0d0d2a4db672
📒 Files selected for processing (13)
cli/src/bundle/partial.tscli/src/types/supabase.types.tssupabase/functions/_backend/files/files.tssupabase/functions/_backend/files/supabaseTusProxy.tssupabase/functions/_backend/files/uploadHandler.tssupabase/functions/_backend/files/uploadSize.tssupabase/functions/_backend/triggers/on_manifest_create.tssupabase/functions/_backend/triggers/on_version_update.tssupabase/functions/_backend/triggers/queue_consumer.tssupabase/functions/_backend/utils/supabase.types.tssupabase/migrations/20260513152858_fix_queue_archive_cleanup.sqltests/files-app-read-guard.unit.test.tstests/manifest-uploaded-size.unit.test.ts
✅ Files skipped from review due to trivial changes (1)
- tests/files-app-read-guard.unit.test.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- cli/src/types/supabase.types.ts
- supabase/functions/_backend/triggers/on_manifest_create.ts
- supabase/functions/_backend/triggers/queue_consumer.ts
- supabase/functions/_backend/files/uploadSize.ts
- tests/manifest-uploaded-size.unit.test.ts
- supabase/functions/_backend/utils/supabase.types.ts
- supabase/functions/_backend/files/files.ts
- supabase/functions/_backend/files/supabaseTusProxy.ts
- supabase/functions/_backend/triggers/on_version_update.ts
- cli/src/bundle/partial.ts
riderx
force-pushed
the
codex/manifest-file-size-upload
branch
from
e64dfcd to
5c1cee1
Compare
|
Addressed in the latest commit. The CLI no longer sends |
|
One blocker remains in the forged/untrusted positive The migration now does the right first step: But the queued handler still exits immediately for any positive value: if (record.file_size && record.file_size > 0) {
return c.json(BRES)
}So a manifest row inserted with
That means forged or stale positive sizes are still accepted instead of being revalidated, which contradicts the migration comment that “Legacy/missing or forged records still keep the fallback R2 HEAD.” The early return needs to distinguish verified backend-observed sizes from queued/unverified positive sizes, or the queued path should ignore the existing positive value and run the R2 size check before deciding whether to keep it. |
riderx
force-pushed
the
codex/manifest-file-size-upload
branch
from
5c1cee1 to
6489327
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
digzrow-coder
left a comment
digzrow-coder
left a comment
There was a problem hiding this comment.
I think the trusted-size shortcut still needs to bind the observed upload row to the app/version being materialized, not only to the raw s3_path.
handleManifest() now hydrates file_size from uploaded_file_sizes with WHERE s3_path = ANY(...), and trigger_verified_manifest_create() skips the fallback queue when any observed row has the same s3_path/size. But the manifest being inserted is scoped by app_version_id while uploaded_file_sizes also records owner_org/app_id; neither path verifies that the observed row's app/owner matches the app_versions row currently being updated. Since app_versions.manifest is API-key/user writable through the existing update policies, a manifest payload can point at an already-observed path outside the current app and get a trusted non-zero size without revalidation for this version.
Please either reject manifest entries whose parsed scoped path does not match record.owner_org/record.app_id, or constrain the uploaded-size lookup/trigger check through the owning app_versions/apps scope. A regression should seed an uploaded_file_sizes row for app A, update an app B version with that s3_path, and assert the manifest insert is rejected or gets file_size = 0/queues verification instead of taking the trusted size.
riderx
force-pushed
the
codex/manifest-file-size-upload
branch
from
6489327 to
6c08a6f
Compare
riderx
force-pushed
the
codex/manifest-file-size-upload
branch
from
6c08a6f to
eff81cb
Compare
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
KCDaemon
left a comment
KCDaemon
left a comment
There was a problem hiding this comment.
Rechecked the latest head (eff81cb).
The trusted uploaded-size path still is not scoped tightly enough to the version/app being materialized. getUploadedFileSizes() loads rows only with WHERE s3_path = ANY(...), and trigger_verified_manifest_create() skips the fallback verification when any uploaded_file_sizes row has the same s3_path and file_size. Neither path checks that the observed row's owner_org / app_id matches the app_versions row receiving the manifest.
Because the manifest payload is still user/API-key writable and uploaded_file_sizes stores the owning scope separately, a version can point at an already observed path from another app/org and receive a trusted non-zero file_size without verification for the current app version. I would constrain the lookup and trigger through the owning app_versions/apps scope, or reject entries whose parsed scoped path does not match the record being updated.
The CI checks are green and the patch applies cleanly with whitespace checking, but I would keep this blocked until cross-app/org trusted-size reuse is covered by a regression test.
5 participants
Summary (AI generated)
uploaded_file_sizestable populated by backend upload/storage observation, not CLI manifest payloads.manifest.file_sizefrom backend-observed upload records and ignore any client-provided manifest size values.on_manifest_create; forged or missing sizes still queue verification.Motivation (AI generated)
on_manifest_createwas doing one follow-up R2 metadata lookup per manifest row. Large bundles can contain thousands of files, which creates queue/archive growth and risks worker limits. The fix moves size capture into backend-controlled upload handling while preserving the R2 fallback for legacy rows.The trust boundary matters here: the CLI can send malformed or malicious manifest data, but
manifest.file_sizeis now written from sizes observed by the backend upload/storage layer only.The queue consumer should also keep draining successful messages when one message handler throws before returning an HTTP response. That failure now stays attached to the one message, while successful messages can still be deleted.
Business Impact (AI generated)
This reduces new
on_manifest_createqueue pressure for modern uploads, makes large delta bundles more reliable, and avoids turning manifest size into a client-trusted billing/storage signal. It also reduces queue blast radius when one message handler fails unexpectedly.Test Plan (AI generated)
bun lint:backendbun typecheckbun run --cwd cli lintbun run supabase:with-env -- bunx vitest run tests/queue-consumer-message-shape.unit.test.ts tests/manifest-uploaded-size.unit.test.ts tests/files-r2-error.test.tsbun run supabase:db:resetbash scripts/check-supabase-migration-order.shgit diff --checkGenerated with AI