fix(v2): stop six responses reporting less than the layer beneath them knew - #7097
Merged
Conversation
…m knew
Findings from cubic's review of the v0.8.12 release PR, all on the v2 surface.
- `GET /workflows/{id}/runs/{runId}` documented `includeFileBase64` as
requiring `includeOutput`, and the read honours that — files are projected
inside the `includeOutput` branch alone. Nothing enforced it, so the flag
parsed, was accepted, and was then dropped: a 200 carrying no files and no
reason why. Now a 400 naming the missing flag, matching how `GET
/billing/logs` refuses a window bound its period will not read.
- `POST /files/{id}/unzip` rendered a malformed or over-cap archive as 500.
`ArchiveError` had no arm in the v2 policy, though the internal extract route
beside it has mapped the same failures to 400/413 all along.
- Both workflow-MCP lists cut their tool inventory at a ceiling and published
`nextCursor: null` regardless. The use cases were already returning
`truncated`; only the v2 presenters dropped it, while the copilot handler
published it. A reconciling caller read a partial set as the complete one.
- A table dispatch scoped by filter reported neither the filter nor its
exclusions, and `rowIds: undefined` documented itself as "every eligible
row" — so a filtered run and an unfiltered one were indistinguishable. The
compiled filter stays unpublished, as before; `selectAll` and
`excludeRowIds` name the distinction.
- `GET /files/uploads/{id}` promised the registered file after finalization and
passed null unconditionally, so a caller polling a transfer it lost track of
could watch a session reach `completed` and never learn what it created.
- `HEAD` on a run file resolved the file downstream of authorization, so it
answered 200 for an id the `GET` beside it would 404.
- v2 bulk table delete audited `FOLDER_DELETED.resourceId` from the
caller-keyed projection, writing a display path where the single-folder
delete writes the canonical id.
Two further findings needed no change: the audit-log cursor scope is bound by
`member_user_id_unique` and a membership check, so the cross-organization
replay it described cannot arise; and both documentation findings had already
been fixed on staging by #7092.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Greptile SummaryThe PR corrects several v2 API responses that previously discarded information already available from underlying application layers.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/app/api/v2/tables/presenters.ts | Publishes filtered and exclusion-based dispatch narrowing while preserving the distinction from unrestricted and explicit-row runs. |
| apps/sim/lib/api/contracts/v2/tables.ts | Extends the public dispatch-scope contract with optional filtering and exclusion metadata. |
| apps/sim/lib/uploads/upload-session/application.ts | Resolves the registered file for completed upload sessions after reauthorization. |
| apps/sim/lib/workflows/application/download-workflow-run-file.ts | Resolves the addressed run file during context loading so HEAD and GET agree on resource existence. |
| apps/sim/lib/workspace-files/api/route-policies.ts | Maps malformed and over-limit archive failures to actionable client error responses. |
| apps/sim/lib/table/application/bulk.ts | Separates caller-facing path projection from canonical identifiers used by deletion audits. |
Reviews (2): Last reviewed commit: "fix(v2): let a failed file read fail, an..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 26 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Review follow-up. `selectAll` was set from the stored filter alone, but the run rejects only `rowIds` *with* `excludeRowIds` — so an exclusion set with no filter is a scope a caller can create, and the walk applies it. Those dispatches published exclusions with no discriminator beside them, which is the shape the flag existed to rule out. One flag could not cover both: an exclusion-only scope is neither filtered nor unnarrowed. So the two narrowings are reported as what they are — `filtered` for the unpublished stored predicate, `excludeRowIds` for the deselections — and every combination is now distinguishable from a run over every eligible row. `excludeRowIds` mirrors the walk's own condition and is withheld beside `rowIds`, where the dispatcher would ignore it.
… check Two review follow-ups. `getWorkspaceFile` logs and returns null on a read failure unless told otherwise, so a transient database error would have reported a finalized upload as fileless — to the one caller polling to learn what it created, who would then stop, having been told there was nothing. Read with `throwOnError` so the failure surfaces and the poll can retry, matching how the sibling record read loads the same row. A file genuinely deleted still answers null, which is a different question with a different answer. The server list pointed callers at `/tools` for an authoritative inventory without saying that endpoint applies the same ceiling. It now names the `truncated` flag this PR added there, so "authoritative" has a condition attached instead of being asserted.
Collaborator
Author
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
1 participant
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.
Summary
The remaining real findings from cubic's review of the v0.8.12 release PR (#7090), all on the v2 API surface. Verified each against the code before fixing; two needed no change.
Six of the seven share a shape worth naming: the layer underneath already knew, and the response threw it away.
includeFileBase64silently did nothing withoutincludeOutput. The contract has always said it requires it, and the read honours that — files are projected inside theincludeOutputbranch alone. Nothing enforced it, so the flag parsed, was accepted, and was then dropped: a200carrying no files and no reason why. Now a400naming the missing flag, matching howGET /billing/logsrefuses a window bound its period will not read.base64MaxBytesrode on the same projection and is covered too.POST /files/{fileId}/unzipanswered500for a bad archive.ArchiveErrorhad no arm in the v2 error policy, though the internal extract route beside it has mapped the same failures to400/413all along.statusForArchiveErrorstays the single place a reason is classified.truncated, and the copilot handler already publishes it — only the v2 presenters dropped it, whilenextCursor: nulltold a reconciling caller the set was complete. The tools list now returnstruncated; the servers list returnstoolNamesTruncated, kept separate from "there is another page" so it does not fire on every page with a successor.rowIds: undefineddocumented itself as "every eligible row", but a select-all dispatch has no row list either — andPOSTon that same path acceptsfilterandexcludeRowIds, so a caller could create a scope the resource then denied having. The compiled filter stays unpublished (it is stored in a different grammar from the predicate the request was written in, and a test had deliberately locked that in);selectAllandexcludeRowIdsname the distinction without reproducing it.GET /files/uploads/{uploadId}always returnedfile: null, though the contract promises the registered file after finalization andcompletedFileIdwas sitting on the record. That endpoint exists for a caller who lost track of a transfer — the one caller who cannot get the id any other way.HEADon a run file 200'd for ids that do not exist. It resolved the file downstream of authorization, so it disagreed with theGETbeside it. The lookup moved into context resolution, matching howreadTableDispatchresolves its dispatch id.resourceId. It projected from the caller-keyed list, soFOLDER_DELETEDrecorded/SaleswhereDELETE /api/folders/[id]records the canonical id — two spellings of one resource that no query could join.Needed no change
membercarriesuniqueIndex('member_user_id_unique')andresolveEnterpriseAuditAccessrefuses any organization the caller is not a member of, so it cannot arise. Reasoning posted on the thread.Type of Change
Testing
OrchestrationError(which every policy already renders) rather than theArchiveErrorthe use case actually throws, and theHEADtest mocked the use case and madeauthorizereject — proving only that the route renders a rejection. The newHEADcoverage sits at the use-case level, where the real decision is made.apps/sim33,288 tests,sim-cli607 tests.bun run lint, all 33 audits (check:audits), andtype-checkin both workspaces pass.generate:openapirejected the new response fields until their examples were updated, which is the gate working.Checklist