fix(platform): close skill upload data-safety + UX gaps from review#1741
Merged
Conversation
Skills now ship as zip bundles uploaded through the UI, not edited in place. The old in-container editor (create / update / write-asset / delete-asset actions and dialogs) is removed; `uploadSkillBundle` reads the zip from Convex `_storage`, validates SKILL.md + paths + sizes, and atomically swaps it onto disk. This kills the deploy-time clobber risk that motivated `.tale-backup/skills-<ts>` snapshots — now removed from `tale deploy`, taking the noisy "invalid output path" warning with it. Also fixes empty-skills-for-new-org: `skills` was missing from the per-org filesystem scaffolder's DOMAINS list, so newly created organizations had no `skills/@<slug>/` seeded from the default org. A one-shot `backfill_skill_scaffolding` migration covers orgs created before this change.
The left file tree in the skill detail panel was wrapped in a block-display container that broke the flex height chain, so its overflow-y-auto never activated and tall bundles (e.g. pptx, 58 files) got silently clipped. Switch the wrapper to display: contents so the aside becomes a direct flex-row child and gets a bounded height via stretch.
Multi-agent review of the upload-only skills flow surfaced six merge blockers and several correctness/leak risks. This PR closes them in one bundled change. Data safety - Per-(orgId, slug) exclusion lock (`skillUploadClaim` table + claim/ release internal mutations) — concurrent `force: true` uploads to the same slug no longer race past the existence check and last-write-wins. - Storage ownership binding (`skillUploadIntent` table + presign / record / verify / delete mutations) — `uploadSkillBundle` now refuses any storageId not bound to the calling org, closing the cross-tenant read / delete path on `_storage`. - `_storage` blob lifecycle now wired through a single `cleanupUploadResources` helper — `needs_confirm`, dialog close, parse failure, and success-finally all delete both the blob and its intent row. No more orphans per replace-cycle / cancel. - `tale deploy` now mirrors the providers branch: `listContainerSkills` + `stageSkillsWithoutConflictingBundles` strips any slug already in the container so UI uploads survive subsequent host syncs. - `duplicateSkill` uses byte-preserving `readFileBufferSafe` + `atomicWriteBuffer` (new helpers in `convex/lib/file_io.ts`) instead of the prior UTF-8 round-trip that corrupted PNGs / PDFs / fonts. Emits a dedicated `duplicate_skill` audit action with the source slug. - Scaffold `copyTree` skips `@<slug>/` entries so creating a new org no longer recursively copies every existing org's tenant tree under the new org's namespace (skills/integrations/workflows). Agents/providers use raw slugs without the marker — follow-up. Correctness - Six empty-catch sites (CLAUDE.md hard rule) now log non-ENOENT errors: `walkSkillBundle` readdir/stat, `duplicateSkill` readdir, `dirHasFiles`, `copyTree` stat, `readSkillMd` lstat. `errnoCode` promoted from scaffold to shared `file_io`. - macOS Finder `__MACOSX/` (plus `.DS_Store` / `Thumbs.db`) is now filtered before `detectSingleTopLevelFolder` runs on both client and server — the most common Mac packaging workflow no longer fails with a misleading "missing SKILL.md" error. - Upload action audit row now carries `skillMdHash`, `totalBytes`, `license`, and `disableModelInvocation` so rollback investigation can tie an audit row to a bundle. - `audit_mutations.ALLOWED_ACTIONS` adds `duplicate_skill` so the new action audit fires through the validated enum. UX + i18n - Dialog now routes through `useUploadSkillBundle` (was raw `useAction`) so the skills list query invalidates on success — table updates without a manual refresh, replace flow's detail panel refetches. - Companion: `lib/config-watcher.ts` typeMap + path parser learn `skills`, so SSE-driven invalidation also covers them. - AbortController + isMountedRef on the dialog — mid-flight close cancels the blob POST and skips stale-state setState warnings. - ConvexError JSON unwrap (`extractErrorMessage`) — user toasts show the human message instead of a serialized error object. - Multi-file / folder / wrong-type drops now produce specific errors, and a window-level `dragover`/`drop` preventDefault stops a missed drop from navigating away from the page. - Preview step renders the full SKILL.md frontmatter (license, recommended Python/Node, disable-model-invocation, unknown-key count) and uses ICU plural for the file count. - "Replace bundle" button next to Duplicate / Delete in the detail panel — opens the upload dialog directly for the visible skill. - `skills.asset.loadNotFound` was an orphan reference (key removed in the prior PR); renamed to `skills.viewer.notFound` and re-added. - DE i18n hard-fail violations fixed per `TERMINOLOGY_DE.md`: `Wird hochgeladen…` → `Lade hoch…`, `Bundle wird gelesen…` → `Lese Bundle…`, `Zip` loanword aligned with `.zip-Datei`, `dropOrClick` aligned with the integrations dialog metaphor. Type signature widening: `requireOrgMembershipById` and `requireOrgAdminOrDeveloper` now accept `ActionCtx | MutationCtx` so the new V8 mutations can reuse the same auth + role gate as the existing actions. No behavior change to existing callers. Verified: `bun run check` green (lint 0 warnings, typecheck clean, 272 test files / 70,878 tests passed). Convex codegen re-run against local backend; new tables + mutations land in `_generated/api.d.ts`. Deferred to follow-up issues per scope minimization: same cross-org leak in agents/providers (raw-slug shape, needs different fix); `backfill_skill_scaffolding` single-page pagination + per-org try/catch (manual migration); audit-vs-fs atomicity; zip symlink permission bits; case-insensitive SKILL.md collision; BOM rejection; `parse-skill-bundle` test coverage.
…KILL.md parser `parseSkillMd` is imported from both the Convex Node runtime (where `Buffer` is a global) and the browser-side skill-upload parser (where it isn't). The frontmatter-size guard threw `ReferenceError: Buffer is not defined` the first time a user dropped a real bundle, surfacing as "SKILL.md frontmatter rejected: Buffer is not defined" in the dialog. `TextEncoder` is in lib.dom + lib.es2017 and is available on every runtime we ship to, so byte-length the way the YAML reader wants. Same behavior, isomorphic. Latent since the original upload-only-skills commit (e3af6f2); the existing skills.test.ts suite runs in Node so it didn't catch the browser path.
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (29)
📝 WalkthroughWalkthroughThis PR migrates skill management from in-place editing to bundle-based uploads. Users now upload ZIP files containing SKILL.md and assets; the client validates locally while the server re-validates, atomically stages, and swaps bundles in place. Old edit dialogs (SkillAssetEditorDialog, SkillAssetsSection, CreateSkillDialog) are removed entirely. The skill detail panel shifts to read-only with a "Replace bundle" action wired to a new multi-step SkillUploadDialog. Backend changes include new schema tables (skillUploadClaims for locking, skillUploadIntents for storage tracking), new mutations for presigned URLs and slot management, and uploadSkillBundle action for atomic file staging and swap. Supporting changes widen auth contexts, add file I/O helpers, update audit action types, and preserve uploaded bundles during deploy. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Comment |
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
Multi-agent review of the upload-only skills flow surfaced 6 merge blockers and several correctness/leak risks. This PR closes them all in one bundled change. Builds on top of e3af6f2ae (the original upload-only-skills feature).
Data safety
skillUploadClaimtable + claim/release internal mutations. Concurrentforce: trueuploads to the same slug no longer race past the existence check and last-write-wins.skillUploadIntenttable + presign / record / verify / delete mutations.uploadSkillBundlenow refuses anystorageIdnot bound to the calling org — closes the cross-tenant_storageread / delete path._storageblob lifecycle wired through a singlecleanupUploadResourceshelper.needs_confirm, dialog close, parse failure, write failure, and success-finally all delete both the blob and its intent row. No more orphans per replace-cycle / cancel.tale deploypreserves UI uploads via newlistContainerSkills+stageSkillsWithoutConflictingBundles(mirrors theprovidersbranch). Any slug already in the container is stripped from the host sync.duplicateSkillpreserves binary assets via newreadFileBufferSafe+ existingatomicWriteBuffer, replacing the prior UTF-8 round-trip that corrupted PNGs / PDFs / fonts. Emits dedicatedduplicate_skillaudit action.copyTreeskips@<slug>/entries — creating a new org no longer recursively copies every existing org's tenant tree into the new org's namespace (skills/integrations/workflows). Agents/providers use raw slugs without the marker — follow-up issue.Correctness
walkSkillBundlereaddir/stat,duplicateSkillreaddir,dirHasFiles,copyTreestat,readSkillMdlstat.errnoCodepromoted fromscaffold.tsto sharedconvex/lib/file_io.ts.__MACOSX/filter (plus.DS_Store/Thumbs.db) on both client + server parsers. The most common Mac packaging workflow no longer fails with a misleading "missing SKILL.md" error.skillMdHash,totalBytes,license,disableModelInvocationso rollback investigation can tie an audit row to a specific bundle.audit_mutations.ALLOWED_ACTIONSaddsduplicate_skill.UX + i18n
useUploadSkillBundle(was rawuseAction), so the skills list query invalidates on success — table updates without manual refresh; replace flow's detail panel refetches.lib/config-watcher.tstypeMap + path parser learnskillsso SSE-driven invalidation also covers them.extractErrorMessage) — user toasts show the human message instead of a serialized error object.dragover/droppreventDefault stops a missed drop from navigating away.skills.asset.loadNotFoundrenamed toskills.viewer.notFound; DE hard-failWird X…patterns fixed perTERMINOLOGY_DE.md(Wird hochgeladen…→Lade hoch…,Bundle wird gelesen…→Lese Bundle…);Ziploanword aligned with.zip-Datei;dropOrClickaligned with the integrations dialog metaphor.Type widening
requireOrgMembershipByIdandrequireOrgAdminOrDevelopernow acceptActionCtx | MutationCtxso the new V8 mutations can reuse the same auth + role gate. No behavior change to existing callers.Test plan
bun run checkgreen from repo root: lint (0 warnings), typecheck (0 errors), 272 test files / 70,878 tests passedbunx convex dev --onceregenerates_generated/api.d.tswith the new tables + mutations_storageblobLOCK_HELDtoast; on-disk bundle is the first writer's; exactly one audit rowneeds_confirm→ close confirm dialog →_storagerow is gone (Convex dashboard)assets/logo.png, duplicate it, compare sha256 — bytes must matchtale init+tale deploy→ container hasexamples/skills/pptx(first-time seed works)tale deployafter UI upload of same-slug skill → "Preserved 1 existing container skill" log line, UI upload not clobberedFollow-ups (deferred per scope minimization)
copyTreeleak applies to agents/providers (raw<slug>shape vs@<slug>— needs curatedexamples/source or domain-aware filter)backfill_skill_scaffoldingsingle-page pagination + per-org try/catch (manual migration, pre-existing pattern inmigrate_org_creators.tstoo)deleteSkillwrites audit beforerm)SKILL.mdcollision on case-insensitive filesystemsSKILL.mdrejected as malformedparse-skill-bundletest coverage (the parser is security-sensitive and has zero unit tests)Summary by CodeRabbit
New Features
Changes
Translations