docs: correct two statements that contradict what they describe - #7092
Merged
Conversation
Both surfaced on the v0.8.12 release PR (#7090). Neither is on code this branch's earlier work touched, but both are the same defect class it was already correcting — documentation that states something the code does not do. v2 knowledge documents list: the JSDoc said `workspaceId` "is dropped for the same reason as on the sibling chunk list". The sibling genuinely drops it — its query type does not even carry the field. This list keeps it, and an inline comment five lines below the binding says so and explains why: dropping it changes the fingerprint, so every cursor minted beforehand would be refused with a message telling the caller they altered a filter they never sent. The JSDoc now states the retention and defers to that note for the reason. v2-api-conventions skill: "Fifteen lists take the pair" — parsing `CURSOR_BINDINGS` gives 19 of 27 paged lists binding both `sortBy` and `sortOrder`. A hard-coded count drifts every time a list gains sorting, which is how this one went stale, so it now points at the test as the authoritative set instead of restating a number.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Collaborator
Author
|
@greptile review |
Collaborator
Author
|
@cubic review |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
Contributor
Greptile SummaryCorrects two documentation inconsistencies without changing runtime behavior.
Confidence Score: 5/5The PR appears safe to merge because both changes are documentation-only and accurately describe existing behavior. The pagination test enforces the referenced binding set, and the cursor implementation confirms that retaining
|
| Filename | Overview |
|---|---|
| .agents/skills/v2-api-conventions/SKILL.md | Replaces a drifting list count with a test-backed reference while preserving the existing ordering guidance. |
| apps/sim/app/api/v2/knowledge/[knowledgeBaseId]/documents/route.ts | Corrects JSDoc to match the existing cursor binding and its compatibility rationale. |
Reviews (1): Last reviewed commit: "docs: correct two statements that contra..." | Re-trigger Greptile
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
6 tasks
waleedlatif1
added a commit
that referenced
this pull request
Aug 26, 2026
…m knew (#7097) * fix(v2): stop six responses reporting less than the layer beneath them 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. * fix(v2): report a dispatch's two narrowings separately 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. * fix(v2): let a failed file read fail, and point truncation at its own 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.
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.
Follow-up to the 26 unresolved cubic threads on the v0.8.12 release PR (#7090).
None of the 26 are on code the recent de-slop work touched (#7060, #7062, #7063, #7082). I evaluated all of them by content rather than by path, since a finding can be relevant without landing on a changed line. Two are the same defect class that work was already correcting — documentation asserting something the code does not do — so they're fixed here. Two more looked like that class and turned out not to be; reasoning below so they aren't re-raised.
Fixed
1.
v2/knowledge/[knowledgeBaseId]/documents/route.ts— JSDoc contradicts the code and its own inline commentThe JSDoc said
workspaceId"is dropped for the same reason as on the sibling chunk list." The sibling genuinely drops it — its query type doesn't even carry the field. This list keeps it, and an inline comment five lines below the binding says so explicitly, with the reason: dropping it changes the cursor fingerprint, so every cursor minted before the change would be refused with a message telling the caller they altered a filter they never sent.Reworded to state the retention and defer to that note. The reasoning about why it's asserted scope rather than a filter was correct and is kept.
2.
.agents/skills/v2-api-conventions/SKILL.md— stale hard-coded count"Fifteen lists take the pair." Parsing
CURSOR_BINDINGSincontracts/v2/__tests__/list-pagination.test.tsgives 19 of 27 paged lists binding bothsortByandsortOrder. cubic's deeper point is the right one: a hard-coded count drifts every time a list gains sorting, which is exactly how this went stale. Now points at the test as the authoritative set instead of restating a number.Evaluated and declined
docs/en/cli/files.mdx—--recursive"no longer lists any spellings" — false positive at the layer proposed. The sentence traces to.describe()oncontracts/v2/files.ts:341, where the param isz.stringbool({ case: 'sensitive' })carrying.meta({ enum: [...V2_TRUE_VALUES, ...V2_FALSE_VALUES] })— 12 published, case-sensitive spellings. The sentence is true for the HTTP param. What's actually happening is the CLI doc renderer flattens a stringbool to a bare--recursiveflag and drops the enum, so the sentence references a list the CLI table doesn't print. Deleting a true sentence from the shared contract would degrade the API reference to patch a renderer gap. If anything, the fix belongs ingenerate-cli-docs.ts.docs/en/cli/credentials.mdx— "Credential to update or disconnect." ondelete— real imprecision, disproportionate fix. It comes fromv2CredentialParamsSchema, a single.describe()deliberately shared by both the update and delete contracts. Splitting it means a second schema plus regenerating a 13k-line generatedv2-api.tsand its mdx, for a P3 where the current wording is imprecise rather than wrong.Not ours
The remaining 22 are unrelated to this work — v1 admin billing upsert, the Microsoft Word lost-update races, v2 pagination/presenter/HEAD findings, chat error-policy disclosure, the webhook Slack fan-out ordering, cron lease renewal,
settings/navigation.tshardcodingcustomBlocks: true, and thedocs/denav entries. Several are P1/P2 and worth someone's attention — particularly thesettings/navigation.tsone, which describes a nav-vs-server disagreement the surrounding doc comment says the table exists to prevent.One note on adjacency:
#7063removed a stalevi.mock('@/app/api/webhooks/utils', ...)from the webhook trigger route test. I re-verified that module still doesn't exist, so the mock was inert and no coverage was lost. Separately,validateSlackSignature(lib/webhooks/providers/slack.ts:465) has zero test coverage — pre-existing, not caused by that removal, but worth recording.Verification
bun run type-checkclean, all 33 audits pass, 159 tests acrosslist-paginationandapp/api/v2/knowledgegreen. Both changes are comment/markdown only — no behavior change.