fix(v2-api): standardization - #6542
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview v2 logs now use v2 file download no longer streams generation source bytes for AI-generated documents. Downloads gated by HTTP verbs: knowledge base update and bulk table row update move from PUT to PATCH (contracts, routes, OpenAPI). DocCompileUserError and doc-not-ready helpers move to leaf modules to avoid pulling OpenAPI / contracts: file download documents 409/413; contract discovery is recursive over the whole contracts tree with an explicit allowlist for undocumented upload data-plane routes; v2 list pagination split is pinned in Reviewed by Cursor Bugbot for commit 4cef8ee. Configure here. |
Greptile SummaryThe PR standardizes several v2 API paths and closes parity gaps with established file-download and log-display behavior.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/copilot/tools/server/files/doc-compile-error.ts | Extracts DocCompileUserError into an import-free leaf module, with all current consumers using the same alias specifier. |
| apps/sim/lib/workspace-files/application/download-workspace-file.ts | Resolves generated documents to bounded compiled artifacts while retaining direct streaming for ordinary files. |
| apps/sim/lib/logs/application/get-public-log.ts | Materializes public log details through the display projection and supplies the personal API-key subject. |
| apps/sim/lib/logs/application/list-public-logs.ts | Applies display-safe materialization to requested list details with bounded concurrency. |
| scripts/check-openapi-specs.ts | Recursively discovers v2 contracts across the contracts tree and validates explicit undocumented-route exemptions. |
Reviews (3): Last reviewed commit: "fix(v2-api): correct three inaccurate cl..." | Re-trigger Greptile
…urce Two regressions shipped with the v2 API (#5273) where v2 diverged from the v1 path it replaced, plus the hardening that fell out of auditing them. **v2 logs bypassed secret redaction.** `getPublicLog` and `listPublicLogs` called raw `materializeExecutionData`, while every other reader — v1 list and detail, CSV export, `fetch-log-detail`, both data-drain sources — calls `materializeExecutionDataForDisplay`, which applies the resolved-secret provenance projection. Both v2 routes then serialize `traceSpans` and `finalOutput` straight onto the wire, so unredacted secrets could reach the public API. Swapped to the display projection and threaded the principal's subject user into the read context. **v2 file download served generation source.** `GET /api/v2/files/{fileId}` streamed `file.key` raw. AI-generated docs store their generation source as the primary file, so a raw download yields source text under a `.pdf` name — a file the recipient cannot open. Generated docs now resolve to their compiled artifact; ordinary uploads still stream and are never materialized, gated on the recorded generation-source type rather than the extension. The resolve is capped at MAX_RENDERED_DOCUMENT_BYTES, and a still-compiling artifact returns a retryable 409 rather than a 500. Also in this change: - Reconcile the two v2 verbs that used PUT for PATCH semantics: `PUT /v2/knowledge/{id}` and `PUT /v2/tables/{tableId}/rows` are both all-optional partial updates. Breaking for API-key clients, but the surface is dark-launched behind the `v2-api` gate and no in-repo caller issues PUT. - Close the OpenAPI coverage blind spot that hid two routes: contract discovery was a non-recursive read of the flat `contracts/v2/` directory, so a contract in a subdirectory — or beside its non-v2 siblings, which is where the uploads contracts live — escaped the gate. The sweep is now recursive over the whole contracts tree, and the two upload data-plane routes are named in an explicit allowlist with reasons and staleness guards. - Extract `needsRenderedArtifact` so the "recorded type is authoritative, extension is fallback" rule has one home instead of being duplicated. - Extract `DocCompileUserError` into a leaf module so recognizing it no longer drags `app/api/**` and `next/server` into application modules. - Correct the stale pagination docstring in `contracts/v2/shared.ts` and pin the paged/full-set split in a test so it cannot drift again.
Review follow-up. - Use the `@/lib/...` alias for `doc-compile-error` in the three modules that imported it relatively. The repo requires absolute imports, and having all four consumers share one specifier also removes any chance of two module instances resolving apart and breaking `instanceof`. - Memoize the v2 list-pagination sweep. It re-imported the whole contracts tree once per test and timed out against the default 10s limit under load; it now sweeps once and declares an explicit timeout. Its failure message also still pointed at an enumeration in `v2/shared.ts` that this branch replaced with a pointer to the test itself.
4cef8ee to
fad352e
Compare
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit fad352e. Configure here.
None of these change behavior; each is a comment or test-config assertion that was not true as written. - The artifact resolver's TSDoc implied the byte cap prevents an oversized artifact being materialized. It does not: the artifact-store fetch is not streaming-bounded, so the bytes are resident before the ceiling rejects them. Say what it actually guarantees. - `v2/shared.ts` pointed at per-contract documentation for the two lists that still filter in memory. Neither contract documents it, so name the two lists and what they do inline instead of pointing at a page that does not exist. - The knowledge update contract said "every field of the body is optional"; `workspaceId` is required. Narrow the claim to mutable fields. - Scope the pagination sweep's extended timeout to the one test that pays for it, so a genuine hang in the other two surfaces in 10s rather than 60s.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 8e596b3. Configure here.
Two regressions shipped with the v2 API (#5273) where v2 diverged from the v1 path it replaced, plus the hardening that fell out of auditing the whole v2 surface for coverage gaps.
The two regressions
v2 logs bypassed secret redaction.
getPublicLogandlistPublicLogscalled rawmaterializeExecutionData. Every other reader in the codebase — v1 list and detail, CSV export,fetch-log-detail, and both data-drain sources — callsmaterializeExecutionDataForDisplay, which appliesprojectExecutionDataForDisplay, the resolved-secret provenance redaction. Both v2 routes then serializetraceSpansandfinalOutputstraight onto the wire (app/api/v2/logs/[runId]/route.ts:46-47,app/api/v2/logs/route.ts:80-84), so unredacted resolved secrets could reach the public API.The projection's own docstring says rows whose provenance is missing or malformed must yield "structural-only content rather than data that cannot be proven safe." v2 skipped that, and also omitted
userIdfrom the read context, which the projection consumes. Fixed by swapping to the display projection and threading the principal's subject user through.Scope limiter: the routes cherry-pick only those two fields, so server-internal keys never reached the wire. Exposure was confined to trace spans and final output — which is also where resolved secrets are most likely to appear.
v2 file download served generation source.
GET /api/v2/files/{fileId}useddownloadWorkspaceFileStream, which streamsfile.keyraw. AI-generatedpdf/docx/pptx/xlsxstore their generation source as the primary file, so as the comment atlib/uploads/utils/file-utils.server.ts:391puts it, a raw download "yields source text under a.pdfname — the file the recipient cannot open." v1, the internal ZIP route, and/api/files/serveall compile first; v2 was the only surface that didn't.Generated docs now resolve to their compiled artifact. Three things kept this from trading one bug for another:
isRenderableDocumentNamewould have matched every.pdfand turned large uploaded files into full in-memory buffers on a public GET. The gate is the recorded generation-source content type, with the extension as a fallback only for legacy records that carry no type — the same predicate the sibling ZIP path already used, now extracted asneedsRenderedArtifact.MAX_RENDERED_DOCUMENT_BYTES. A source is text and orders of magnitude smaller than what it renders to, so the record's declared size bounds nothing.Also included
PUT /v2/knowledge/{id}andPUT /v2/tables/{tableId}/rowsboth take all-optional partial-update bodies; the row one is literally described in its contract as "a row-data patch applied to every matching row." The other three v2 PUTs are genuine replace/upsert and are unchanged.contracts/v2/directory. The hole was two-dimensional: recursing alone wouldn't have caught the two undocumented upload routes, because their contracts live incontracts/upload-sessions.ts, outsidev2/entirely. The sweep is now recursive over the whole contracts tree (128 → 130 contracts), and those two data-plane routes are named in an explicit allowlist with reasons plus staleness guards.DocCompileUserErrorextracted to a leaf module so recognizing it no longer dragsapp/api/**andnext/serverinto application modules.contracts/v2/shared.ts, which understated the paged list count by ten, and pinned the paged/full-set split in a test so it can't drift again.Breaking change
The PUT → PATCH rename is breaking for external API-key clients, which will start getting 405. Called out deliberately rather than buried: the v2 surface is dark-launched behind the
v2-apifeature gate (when off, every route 404s as if it does not exist), it is three commits old, and no in-repo caller issues PUT to either path. If you'd rather keep PUT as a deprecated alias, say so and I'll add it back.Honest caveats
doc-not-readymodule traces clean, butdownload-workspace-file.tsstill reachesapp/api/**transitively viaworkspace-file-manager. That edge predates this change; the change adds no new one./api/files/servealready do, and the not-ready case returns 409 rather than blocking, but it is new behavior for v2.GET /v2/logs?includeTraceSpans=true. v1 pays the identical cost on the same path today, so this is parity, not a new tax — but memoizing the decrypted registry per provenance envelope is a worthwhile follow-up.Related, not included
#6540 is a third regression from #5273, same shape: a route moved from raw body to contract-parsed body and Zod silently stripped a private field the contract never declared. It is already open, green, and against
staging, so it should land on its own rather than be duplicated here.Worth adding to that PR: the KB-search break has a second, quieter victim.
lib/guardrails/validate_hallucination.tsalso queries that route and always builds the provenance metadata, but swallows the 400 into{ context: [] }— so the hallucination guardrail evaluates claims against empty knowledge-base context and returns a confident wrong verdict, with nothing surfaced to the user.I swept every other tool declaring
modelInputand every route #5273 touched for the same bug class. Beyond those three, the count is zero.Verification
bun run type-check, 1,850 tests across the affected suites, Biome,bun run check:openapi(7 specs / 128 operations / 130 contracts),bun run check:api-validation, and the client-boundary check all pass. Every new test was verified to fail against the unfixed code and pass after — including the one asserting an uploaded PDF still streams, which catches the memory regression the first draft of this fix would have introduced.