Conversation
a3b3310 to
b33241b
Compare
b33241b to
301e53e
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 AI Review
Reconciled five Claude findings and one Codex finding into five entries. The shared deep-object serialization issue is confirmed as major; the override drift risk and two convention/style issues are also confirmed. The claimed notebook content round-trip incompatibility is refuted by the generated types and validation path.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🟠 MAJOR | packages/api/scripts/openapi-overrides.json:707 |
query-serialization |
claude+codex | The generated listNotebooks client cannot correctly serialize its deep-object filter and pagination parameters, so filtering and cursor pagination will not follow the endpoint contract; the README also omits this newly affected operation. |
| 🟡 MINOR | packages/api/scripts/openapi-overrides.json:682 |
maintainability |
claude | The remove-and-add overrides mask future upstream changes to the notebook paths and schemas from the hourly drift detector. |
| ⚪ NIT | packages/api/scripts/openapi-overrides.json:686 |
conventions |
claude | The override comment exposes internal rollout context and references notebook push/pull commands that do not yet exist in the repository. |
| ⚪ NIT | packages/api/scripts/openapi-overrides.json:691 |
style |
claude | The new override block is stylistically inconsistent with existing entries in its Unicode escaping and placement of the $comment field. |
Refuted findings (kept for transparency, not posted as review comments)
packages/api/scripts/openapi-overrides.json:1282(api-design): A notebook content value returned by the API cannot be passed to create or update without manually removing schema_version and other response-only fields.
Refuted: The generated response content type is structurally assignable to the request content type: it has all required request fields, and extra properties do not prevent assigning an existing typed value in TypeScript. The client also validates inputs through the request schema before building the request at packages/api/src/internal/client.ts:547-552. The cited asymmetry therefore does not establish the claimed type-checking or manual-stripping failure.
Stats
Claude findings: 5 · Codex findings: 1 · Confirmed: 4 · Refuted: 1 · Uncertain: 0
Models: claude-opus-5 + gpt-5.6-sol · Trigger: auto · Workflow run
This review runs once per PR. A maintainer can request another with a /ai-review comment.
| "op": "remove", | ||
| "path": "/paths/~1v2~1projects~1{ref}~1notebooks" | ||
| }, | ||
| { | ||
| "$comment": "supabase/infrastructure ships the v2 notebook routes ahead of the platform. Pinned here so `supabase notebooks push/pull` has a typed client; the paired remove keeps the add idempotent once the deployed spec carries them, at which point this whole block should be deleted rather than left masking upstream.", | ||
| "op": "add", | ||
| "path": "/paths/~1v2~1projects~1{ref}~1notebooks", |
There was a problem hiding this comment.
🟡 MINOR · maintainability · source: claude
The remove-and-add overrides mask future upstream changes to the notebook paths and schemas from the hourly drift detector.
Evidence: packages/api/scripts/download-openapi.ts:483-488 applies overrides before writing the generated snapshot. Its remove operation silently ignores missing pointers at lines 219-225, while add inserts the pinned value at lines 215-217. Thus an upstream notebook definition is removed and replaced before drift is evaluated.
Suggested fix: Add a guard that detects when these pinned paths or schemas appear upstream and fails regeneration until the override is removed or reconciled.
| "style": "deepObject" | ||
| }, | ||
| { | ||
| "in": "query", | ||
| "name": "page", | ||
| "required": false, | ||
| "schema": { | ||
| "properties": { | ||
| "after": { | ||
| "minLength": 1, | ||
| "type": "string" | ||
| }, | ||
| "before": { | ||
| "minLength": 1, | ||
| "type": "string" | ||
| }, | ||
| "size": { | ||
| "maximum": 100, | ||
| "minimum": 1, | ||
| "type": "integer" | ||
| } | ||
| }, | ||
| "type": "object" | ||
| }, | ||
| "style": "deepObject" |
There was a problem hiding this comment.
🟠 MAJOR · query-serialization · source: claude+codex
The generated listNotebooks client cannot correctly serialize its deep-object filter and pagination parameters, so filtering and cursor pagination will not follow the endpoint contract; the README also omits this newly affected operation.
Evidence: The parameters declare style deepObject at packages/api/scripts/openapi-overrides.json:707 and :731, but packages/api/src/generated/contracts.ts:17304 stores only the names. packages/api/src/internal/client.ts:448-456 then JSON-stringifies object values via normalizeUrlValue at lines 272-281 rather than producing bracketed keys. packages/api/README.md:168-172 lists only the three pre-existing affected operations.
Suggested fix: Preserve query-parameter style metadata during generation and encode deepObject values as bracketed keys, with request-level coverage. Until fixed, add v2-list-notebooks to the README limitation.
| "path": "/paths/~1v2~1projects~1{ref}~1notebooks" | ||
| }, | ||
| { | ||
| "$comment": "supabase/infrastructure ships the v2 notebook routes ahead of the platform. Pinned here so `supabase notebooks push/pull` has a typed client; the paired remove keeps the add idempotent once the deployed spec carries them, at which point this whole block should be deleted rather than left masking upstream.", |
There was a problem hiding this comment.
⚪ NIT · conventions · source: claude
The override comment exposes internal rollout context and references notebook push/pull commands that do not yet exist in the repository.
Evidence: The comment names supabase/infrastructure, describes its shipping order relative to the platform, and references supabase notebooks push/pull. No notebook command exists under apps or packages outside the newly generated API artifacts. trusted/CLAUDE.md:101 requires comments in this public repository to omit internal context.
Suggested fix: Describe only the externally observable reason and retirement condition, without naming the internal repository, rollout order, or unshipped commands.
| "path": "/paths/~1v2~1projects~1{ref}~1notebooks", | ||
| "value": { | ||
| "get": { | ||
| "description": "Returns a cursor-paginated list of the project notebooks. Notebook bodies are omitted \u2014 read a single notebook to get its cells.", |
There was a problem hiding this comment.
⚪ NIT · style · source: claude
The new override block is stylistically inconsistent with existing entries in its Unicode escaping and placement of the $comment field.
Evidence: Line 691 is the file's only Unicode escape, while the generated snapshot emits the same em dash literally. The new entry places $comment first at line 686, whereas existing commented overrides place it last at lines 594, 619, 661, and 679.
Suggested fix: Use consistent Unicode representation and place $comment consistently with the existing override entries.
Pins the five `/v2/projects/{ref}/notebooks` routes into the OpenAPI
overrides and regenerates the contracts, effect client, and spec snapshot
so `list`/`get`/`create`/`update`/`delete` notebook operations are
available as typed operations.
supabase/infrastructure ships these routes ahead of the platform spec, so
the override is an add paired with a remove that keeps it idempotent once
the deployed spec carries them — at which point the whole block should be
deleted rather than left masking upstream.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
301e53e to
dcf75a2
Compare
avallete
left a comment
avallete
left a comment
There was a problem hiding this comment.
Approve. The generated notebook routes are what pull and push call. List, get, create, update, and delete all behaved against a staging project. No change requested on this commit.
This branch has not been deployed
2 participants
Adds the five
/v2/projects/{ref}/notebooksroutes — list, get, create, update, delete — to the generated Management API client, so the notebook commands later in this stack have a typed client to drive.Pure codegen. The routes are live on
api.supabase.com, sogeneratepicks them up from the published v2 spec; no override block is involved.How to try it
pnpm --filter @supabase/api run generate git diff --exit-code -- packages/api/src/generated # regeneration is reproducibleNotes
row_limiton database cells andtime_rangeon log cells in notebook responses.page/filteron these operations arestyle: deepObject, which the client does not serialize correctly yet. Fixed in the next PR in this stack.