fix(api): close five defects found auditing the v2 migration against main - #6575
Open
waleedlatif1 wants to merge 10 commits into
Open
fix(api): close five defects found auditing the v2 migration against main#6575waleedlatif1 wants to merge 10 commits into
waleedlatif1 wants to merge 10 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryMedium Risk Overview Folder ceiling on writes. Creates now refuse at Uploads. A retried create-only PUT that gets 409/412 is treated as already committed instead of aborting and deleting the object. Signed transfer URLs advertise a bounded 1h Skills. Internal Other fixes. Column retype no longer nulls empty strings that the target type can hold ( Reviewed by Cursor Bugbot for commit 455e2e7. Configure here. |
Contributor
Greptile SummaryThe PR fixes five defects found while auditing the v2 migration, covering table-cell retyping, execution-slot cleanup, ambiguous direct-upload retries, folder limits, and skills-route consistency.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported skills batch partial-commit path now validates all items before performing the writes in one transaction.
|
| Filename | Overview |
|---|---|
| apps/sim/app/api/skills/route.ts | Replaces route-local per-item writes with the shared authenticated batch-upsert use case while preserving request and response shapes. |
| apps/sim/lib/skills/application/use-cases.ts | Defines the authorized skills batch operation and projects per-item audit metadata from the committed result. |
| apps/sim/lib/skills/orchestration/skill-lifecycle.ts | Validates the complete skills batch before delegating to the existing atomic persistence operation. |
| apps/sim/lib/uploads/client/upload-session.ts | Treats a create-only conflict on a retried PUT as an ambiguous prior success while retaining first-attempt conflict failures. |
| apps/sim/lib/execution/cancel-workflow-execution.ts | Releases the execution concurrency reservation before returning a terminal workflow-group cancellation conflict. |
| apps/sim/lib/table/columns/service.ts | Uses the dedicated cell-rewrite decision to preserve readable empty-string values during column retyping. |
| apps/sim/lib/folders/queries.ts | Adds direct transactional capacity checks used by folder mutation paths without requiring capped path-index materialization. |
| apps/sim/app/api/folders/[id]/duplicate/route.ts | Charges the complete duplicated subtree against the workspace folder ceiling before insertion and maps classified refusal errors. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Skills POST request] --> B[Authenticate principal]
B --> C[Load canonical workspace context]
C --> D[Authorize skills.upsert]
D --> E[Validate every batch item]
E --> F[Single database transaction]
F --> G{Any item fails?}
G -->|Yes| H[Rollback entire batch]
G -->|No| I[Commit all skill changes]
I --> J[Project audit entries]
J --> K[Return touched skills]
Reviews (2): Last reviewed commit: "fix(skills): apply an upsert batch in on..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
A type conversion rewrote every cell holding '' to null. Main only nulled a blank the target type could not read; '' is a real stored value that both string and json columns accept, so string->json and json->string silently destroyed those cells. Worse on a required target: countEmptyCells matches only a missing key, SQL NULL, or '[]', so '' passes the required guard and the rewrite then wrote null behind a constraint that had just succeeded. The per-cell decision is now the pure retypeCellRewrite, restoring main's rule: null a blank only when the target cannot read it, otherwise coerce.
…efused The stop-the-work effects (durable Redis abort record, queue-job cancel, in-process abort) all fire before the workflow-group sidecar is consulted, and none can be undone. When the sidecar refuses the claim we throw a conflict, which skipped releaseExecutionSlot and stranded the plan concurrency reservation until it expired. Every conflict return is a terminal-or-absent state - a missing log row, a log already completed or errored, or a terminal cell - so a refusal never means the run is still executing. The slot is released before the throw, keeping the exact success && !isPausedCancellationPath predicate rather than a blanket finally that would free reservations for live runs.
Main recovered an upload whose bytes committed but whose response was lost, via a verify endpoint. The session client retries the PUT instead, but every provider now signs a create-only precondition, so the retry returns 409/412, is classified non-retryable, and the session aborts - deleting the object that had already landed. A transient blip on the final ack cost the whole upload. A conflict on a retry attempt is now treated as our own earlier PUT having committed, and completion proceeds. That is safe because completeUploadSession independently verifies the object through assertObjectIdentity, which rejects on uploadId mismatch before anything durable is registered. A first-attempt conflict still fails loudly.
Readers bound the active path index at MAX_FOLDERS_PER_WORKSPACE and throw once a workspace exceeds it, but POST /api/folders reached createFolder, which has no maxFolderRows field and never counts. A workspace could therefore be driven past the ceiling, after which the 27 capped read sites failed on a state the product had allowed. createFolder now asserts room inside its transaction, right after the mutation lock, so the count cannot be raced. The refusal is a typed conflict rendering 409 with an actionable message rather than a 500. The check counts rows directly instead of loading the path index, so an already-over-cap workspace gets a clean refusal rather than a read error, and no reader gained a cap. folderMutationStatus also gained the payload_too_large mapping it was missing, which had been rendering a delete-cascade cap breach as an unexplained 500.
The internal route made the workspace authorization decision itself, never consulting the skills operation policy, never loading canonical workspace context, and recording an audit entry with no operation id or actor projection. v2 and Copilot already went through the use cases; only this surface did not. GET/POST/DELETE now authenticate, parse, call the shared use case, and present. Request and response shapes are unchanged. Two behavior changes fall out: a write against a deleted workspace is now refused with 404 rather than accepted, and permission-denial text matches the rest of the platform. Legacy internal-JWT auth is dropped because no principal kind expresses that caller and nothing calls it: the whole repo references /api/skills only in two comments, no tool declares an internalRoute to it, and the executor reads skills through a direct listSkills call rather than over HTTP.
…aths Folder duplication, admin workspace import, and workspace forking all inserted folders without consulting the ceiling that 27 read sites enforce, so any of them could leave a workspace whose reads then fail. Each now asserts room for the rows it is about to add rather than one at a time: duplication measures the whole subtree up front, forking counts its bulk insert, and import counts per segment because that is genuinely one row. assertFolderCollectionHasRoom gained an additionalRows notion for the bulk case, and short-circuits when nothing is being added so an over-cap workspace still reads and still syncs. Duplication deliberately does not take the folder mutation lock. Holding it across the copy would block folder creation workspace-wide for an unbounded time - there is no cap on workflows per subtree and duplicateWorkflow runs sequentially - and narrowing it is impossible because an advisory transaction lock cannot be released early; splitting the transaction would leave a half-copied tree on failure. A rare few-row overshoot near the ceiling is the better trade, and it matches what forking already does. A test asserts the lock is absent so re-adding it is a visible decision. Admin import gained the transaction and lock it never had. Its folder-full refusal escapes the per-workflow result list, because a full tree is a property of the workspace and would otherwise be buried as N failures behind a 200. The fork and promote routes had no catch at all, and withRouteHandler only classifies HttpError, so a refusal rendered as an opaque 500 - twice over, since drizzle wraps the throw. Both now project a classified conflict as 409 and rethrow anything unclassified.
…piry A single-PUT transfer was signed for the whole 24h upload-session TTL, because expiresAt was reused as both the session lifetime and the signing lifetime. Multipart part URLs in the same file kept 1h, and the pre-migration presigned route signed every PUT for 1h, so the widening was unintended rather than a policy change. No provider clamps below 24h. The PUT presign is now clamped at the provider boundary by a shared UPLOAD_URL_TTL_MS, which the part-URL path also uses so the two cannot drift. An expired PUT URL is deliberately not recoverable: unlike multipart, which re-signs per part call because its progress is durable, a PUT is not resumable, so an expired URL and an interrupted PUT have identical recovery. Nothing leaks, since every provider signs a create-only precondition. Clamping alone would have made the contract lie: the URL would die an hour before the session's advertised expiresAt, with nothing telling an integrator why the 403 happened. The PUT transfer now carries its own expiresAt, mirroring the multipart part-URL field. It is provider-dependent on purpose - cloud transfers report the clamped signature expiry, while the local data plane has no signature and admits against the session, so reporting an hour there would have been a new inaccuracy in the other direction.
The presigned upload routes and the internal skills adapters were both replaced during the v2 migration, leaving their implementations behind with no callers. Removed generatePresignedUploadUrl and verifyPresignedUploadReceipt with their three provider helpers, QUOTA_EXEMPT_STORAGE_CONTEXTS and the types it orphaned, and the performCreateSkill/performUpdateSkill/performDeleteSkill adapters with recordSkillEvent and statusForSkillOrchestrationError. Each was verified unreachable across apps, packages, scripts and ee, including barrel re-exports and string access, not just direct imports. recordSkillEvent needed the closest look, since deleting an audit writer can silently drop coverage. The use cases declare the same action, resource, and description, and the framework adds the operation and actor the old helper lacked; recordAudit back-fills actorName and actorEmail from the user table when both are omitted, so the one field the helper passed is not lost. The self-hosting architecture doc described a directUploadSupported flag on an endpoint that no longer exists, and now describes the upload-session flow that replaced it.
Reading a folder resource type's label or its lock support meant importing folderResourceConfig, which imports the db schema for every table it serves and from there reaches lib/table/service, the executor, and the tool registry. That mattered as soon as lib/folders/queries needed a label: queries is reached from workspace-file-manager, which is reached from the files and chat pages, so one import edge put roughly 4,700 modules into those page graphs and broke the tool-registry boundary audit. Labels and lock support now live in a leaf module that imports only a type, and config composes them so there is still one source of truth. The three folder routes that pulled the whole config in for a single boolean read the leaf instead.
The internal skills route looped the batch, calling an independently committing use case per item. A rejection on a later item left the earlier ones written and audited while the request reported failure - the compound-mutation rule in CLAUDE.md exists for exactly this. No new transaction plumbing was needed: upsertSkills already wraps its whole item loop in one db.transaction, so the partial commit came from calling it N times rather than once. upsertSkillBatch now validates and per-skill authorizes every item before issuing a single write, and createSkill and updateSkill became thin wrappers over it so v2 and Copilot keep one authority for the rules. The compound operation declares the read floor that skills.update already used, and the use case additionally authorizes skills.create when any item lacks an id, still ahead of every write. A read-only member who is a skill editor keeps their edit, and creates are not authorized more loosely than before. Audit projects one entry per committed skill, and analytics moved after the commit so nothing is reported for a rolled-back item. Note metadata.operation for these writes is now skills.upsert rather than skills.create/update; the action field still carries the distinction.
waleedlatif1
force-pushed
the
fix/v2-followup-hardening
branch
from
August 12, 2026 01:41
4d6602b to
455e2e7
Compare
Collaborator
Author
Collaborator
Author
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 455e2e7. Configure here.
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 #6560. Each commit is one independent defect found by diffing the v2 migration against
origin/mainand closing the two coverage gaps that audit had explicitly left open.Everything owned by #6564 is deliberately excluded —
archive.ts,workspace-file-folder-manager.ts,workspace-file-folders.ts, and the v1 tables duplicate-name status.1. Column retype silently nulled empty-string cells — data loss
The rewrite pass unconditionally mapped
''tonull.mainnulled a blank only when the target type could not read it, and''is a real stored value that bothstringandjsonaccept — sostring → jsonandjson → stringdestroyed every blank cell.Worse on a
requiredtarget:countEmptyCellsmatches only a missing key, SQLNULL, or'[]', so''passes the required guard and the rewrite then wrotenullbehind a constraint that had just succeeded.The decision is now the pure
retypeCellRewrite, restoring main's rule exactly.2. Refused group cancellation leaked a plan concurrency slot
The stop-the-work effects — durable Redis abort record, queue-job cancel, in-process abort — all fire before the workflow-group sidecar is consulted, and none can be undone. A refusal then threw past
releaseExecutionSlot, stranding the reservation until expiry.Every
conflictreturn is a terminal-or-absent state (missing log row, log alreadycompleted/error, terminal cell), so a refusal never means the run is still executing. The slot is released before the throw, keeping the exactsuccess && !isPausedCancellationPathpredicate rather than a blanketfinallythat would free reservations for live runs.3. An ambiguous PUT destroyed the object it had already uploaded
mainrecovered an upload whose bytes committed but whose response was lost, via a verify endpoint. The session client retries the PUT instead — but every provider now signs a create-only precondition, so the retry returns 409/412, is classified non-retryable, and the session aborts, deleting the object that had already landed.A conflict on a retry is now treated as our own earlier PUT having committed. Safe because
completeUploadSessionindependently verifies viaassertObjectIdentity, which rejects onuploadIdmismatch before anything durable is registered. A first-attempt conflict still fails loudly.4. Folder ceiling was enforced on readers but not on the create path
27 read sites bound the path index at 10,000 and throw past it, but
POST /api/foldersreachedcreateFolder, which has nomaxFolderRowsand never counts. A workspace could be driven past the ceiling, after which those reads failed on a state the product had allowed.createFoldernow asserts room inside its transaction, right after the mutation lock, so the count cannot be raced. Refusal is a typed conflict rendering 409 with an actionable message, not a 500. It counts rows directly rather than loading the path index, so an already-over-cap workspace gets a clean refusal rather than a read error — no reader gained a cap, nothing is bricked.Also adds the
payload_too_large → 413mappingfolderMutationStatuswas missing, which had been rendering a delete-cascade cap breach as an unexplained 500.5. Internal skills route bypassed the shared use cases
It made the workspace authorization decision itself, never consulted the skills operation policy, never loaded canonical workspace context, and wrote an audit entry with no operation id or actor. v2 and Copilot already went through the use cases.
Request and response shapes are unchanged. Two behavior changes fall out: a write against a deleted workspace is refused with 404 rather than accepted, and permission-denial text matches the rest of the platform.
Legacy internal-JWT auth is dropped because nothing uses it: the whole repo references
/api/skillsin exactly two comments, no tool declares aninternalRouteto it, and the executor reads skills through a directlistSkillscall rather than over HTTP.Verification
Every fix was proven by reverting it and watching the new tests go red; the red output is in the commit trail. Where a test could only guard against a wrong fix rather than the original bug, that is stated rather than counted as proof.
bun run type-check,check:api-validation,check:openapiall pass. Full suite: 23,080 passed.One pre-existing failure, not from this branch.
app/api/tools/file/manage/route.test.tsfails onstagingtoday: its mock still declarescreateWorkspaceFileFolderOperationwhilearchive.tsnow callsensureWorkspaceFileFolderPathOperation. #6564 already contains the one-line fix, and that file is in its scope, so it is untouched here.Written up, not changed
generatePresignedUploadUrl,verifyPresignedUploadReceipt,QUOTA_EXEMPT_STORAGE_CONTEXTS, and theperform*Skillhelpers now have zero non-test callers.architecture.mdxstill documents a removeddirectUploadSupportedfield.