From 3dd70498c1af1cf2f958a6cc6950f603a8801b54 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Fri, 25 Sep 2026 06:57:27 -0700 Subject: [PATCH] refactor!: remove skill-draft, web_site handling and mail attachments skill-draft is an ordinary kind and web_site content is stored as given. The mail-attachment routes, helpers and table are gone: 0001 no longer creates mail_attachment_ref, a new migration drops it from upgraded databases, and an end-to-end test upgrades a database written by the published 0.1.0. windowContent is no longer exported. --- CHANGELOG.md | 19 ++ CONTRIBUTING.md | 25 +- README.md | 11 +- bun.lock | 3 + examples/reference-host/src/index.ts | 4 +- migrations/0001_artifacts.sql | 24 -- migrations/0003_drop_mail_attachment_ref.sql | 1 + package.json | 1 + src/artifacts.test.ts | 35 +-- src/artifacts.ts | 58 +---- src/index.ts | 26 -- src/list.test.ts | 13 +- src/mail-attachments.test.ts | 234 ------------------ src/mail-attachments.ts | 236 ------------------- src/mount.test.ts | 234 +----------------- src/mount.ts | 116 +-------- src/schema.ts | 33 --- src/sidecar-bundle.test.ts | 4 +- src/sidecar-bundle.ts | 1 - src/test-helpers.ts | 14 +- src/tools.test.ts | 114 ++------- src/tools.ts | 56 +---- src/web-site.test.ts | 114 --------- src/web-site.ts | 129 ---------- src/workflow-mount.test.ts | 11 +- src/workflow-mount.ts | 9 +- tests/lib/db-harness.ts | 12 +- tests/migrations.test.ts | 2 +- tests/reference-host.test.ts | 90 +------ tests/upgrade-from-0.1.0.test.ts | 89 +++++++ 30 files changed, 212 insertions(+), 1506 deletions(-) create mode 100644 migrations/0003_drop_mail_attachment_ref.sql delete mode 100644 src/mail-attachments.test.ts delete mode 100644 src/mail-attachments.ts delete mode 100644 src/web-site.test.ts delete mode 100644 src/web-site.ts create mode 100644 tests/upgrade-from-0.1.0.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 929ca19..9a67f59 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -165,6 +165,25 @@ always called out under their own heading. authorization concept on top of the platform's — the failure mode this PR exists to remove. If a real need for it surfaces, it belongs in Interchange's grant model, not a per-package workaround. +- `SKILL_DRAFT_KIND` is removed. `skill-draft` is no longer a reserved kind: + create, list, find-by-title and every read treat it like any other `kind`. +- `web_site` handling is removed: `web_site` content is no longer normalized + on write, `readArtifact` no longer takes `path` or returns a site summary, + `artifact_read_chunk` no longer refuses it, and the sidecar's + `artifact_read` no longer forwards `path`. `WEB_SITE_KIND`, + `WEB_SITE_MAX_FILES`, `WEB_SITE_MAX_PATH_LENGTH`, `WEB_SITE_MAX_TOTAL_BYTES`, + `WebSiteContentError`, `normalizeWebSiteContent`, `normalizeWebSitePath`, + `parseWebSiteContentJson`, `serializeWebSiteContent`, + `summarizeWebSiteContent`, `WebSiteContent` and `WebSiteReadSummary` are no + longer exported. +- Mail attachment references are removed: `POST` and `GET + /instances/:instanceId/mail-attachments`, `saveMailAttachmentRefs`, + `listMailAttachmentRefs`, `MAIL_ATTACHABLE_KINDS`, + `MailAttachmentKindError`, `MAX_MAIL_ATTACHMENT_BYTES`, + `MAX_MAIL_ATTACHMENTS_PER_MAIL` and `MailAttachmentRefRow`. The new + `0003_drop_mail_attachment_ref` migration drops the `mail_attachment_ref` + table and its rows. +- `windowContent` is no longer exported; it is internal to the tool reads. ## [0.1.0] — first release diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 81f9365..44b5a39 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -99,8 +99,7 @@ How the package is put together and why. Mount options and snippets are in the ### Where the routes are served -The core registers root-relative paths (`/artifacts*`, -`/instances/:id/mail-attachments`) and takes no base path, so the *mount point* +The core registers root-relative paths (`/artifacts*`) and takes no base path, so the *mount point* is the host's decision. The convention every `@corbits/*-core` package documents, and every example here demonstrates, is **`/api`** — the same prefix Interchange serves its own routes under (`app.route("/api/me", …)`, @@ -112,8 +111,7 @@ app.route("/api", createArtifactRoutes({ db, contentStore, requireGrant })); ``` which serves `/api/artifacts`, `/api/artifacts/:id`, -`/api/artifacts/:id/versions`, `/api/artifacts/:id/download`, and -`/api/instances/:instanceId/mail-attachments`. Returning a sub-app rather than +`/api/artifacts/:id/versions`, and `/api/artifacts/:id/download`. Returning a sub-app rather than taking a base path keeps the factory free of a configurable base path. Everything else it needs arrives through `deps` or the host's request context. @@ -231,15 +229,12 @@ which store is installed. | `download.ts` | One download path over the three storage conventions. | | `content-store.ts` | The two shipped `ContentStore` implementations. | | `tools.ts` | Agent-facing tool definitions and windowed artifact reads (caller tenant only). | -| `web-site.ts` | The `web-site` kind's content encoding and validation. | -| `mail-attachments.ts` | Artifact↔message associations. | | `ports.ts` | The `ContentStore` type and the shared `ResolvedPrincipal` shape. | -| `schema.ts` / `migrations.ts` | The four tables, and the DDL that creates them. | +| `schema.ts` / `migrations.ts` | The three tables, and the DDL that creates them. | ### Data model -Four physical tables — `artifact`, `artifact_version`, `upload`, -`mail_attachment_ref`. +Three physical tables — `artifact`, `artifact_version`, `upload`. **Hard control-plane foreign keys, by design.** `tenant_id` is `NOT NULL` and references the host's `tenant(id)` (`ON DELETE CASCADE` — a deleted tenant takes its @@ -251,7 +246,7 @@ have run before `runArtifactMigrations`. The internal key — `artifact_version.artifact_id` — cascades with its artifact. **Cheap row-local CHECKs.** `artifact.version` and `artifact_version.version` -must be ≥ 1; `upload.size` and `mail_attachment_ref.size` must be ≥ 0. These are +must be ≥ 1; `upload.size` must be ≥ 0. These are single-column constraints — free at write time. **Principal↔tenant alignment is host-owned.** The package FKs each column into @@ -327,9 +322,7 @@ for every possible way to write an artifact. **`upload` is never a standalone resource.** There is no `POST /uploads`; every upload eagerly mints its artifact, and the row is reachable only through -`source.upload.id`. `mail_attachment_ref` carries no bytes at all — the file -already *is* an artifact, and the ref only records which artifacts rode with -which message. +`source.upload.id`. The list index is `(tenant_id, updated_at, id)`. The `id` is the list's tie-break and must be *in* the index, or the keyset cursor's row-value @@ -370,7 +363,7 @@ host schema's `tenant` / `principal` (see the data model). ### Boundaries -Owned by this package: the four tables and their migrations; the HTTP surface, +Owned by this package: the three tables and their migrations; the HTTP surface, its validation and its status codes; the version and archive semantics; the upload **gate** (`createFileArtifact` takes `policy` as a required argument and refuses anything outside it before the `ContentStore` is touched); and the @@ -397,6 +390,6 @@ authenticated `tenant`/`principal` on the request context; the host's zero-dependency default, not a recommendation at scale; a large corpus wants a `ContentStore` over object storage. - **List paging caps at 100** rows (default 20). -- **One 404 covers four causes** for a resolved caller — never minted, - malformed, a `skill-draft`, or another tenant's. Distinguishing them would be +- **One 404 covers three causes** for a resolved caller — never minted, + malformed, or another tenant's. Distinguishing them would be an existence oracle. Expect no more detail than that from the API. diff --git a/README.md b/README.md index ca98989..975d7ce 100644 --- a/README.md +++ b/README.md @@ -34,7 +34,7 @@ Returns a `Hono` sub-app the host mounts with `app.route`, alongside | --- | --- | --- | | `db` | `ArtifactDb` | Artifacts are stored there. `createArtifactDb` opens a handle for a host with none; a hub that already has one passes it through. | | `contentStore` | `ContentStore` | Blob storage for file bytes. `InlineContentStore` (exported by this package) fits a minimal host; bring your own store for object storage. | -| `requireGrant` | `RequireGrant` | The host's grant middleware factory. This package implements no ownership or membership policy of its own. Creating an artifact requires `create` on `artifact:*`; revising or archiving one requires `write` or `archive` on `artifact:`. Recording mail-attachment references needs only a principal. | +| `requireGrant` | `RequireGrant` | The host's grant middleware factory. This package implements no ownership or membership policy of its own. Creating an artifact requires `create` on `artifact:*`; revising or archiving one requires `write` or `archive` on `artifact:`. | | `countSegments` | `ArtifactCountSegments` (optional) | Named predicates over `ArtifactListRow` for `GET /artifacts/counts` (e.g. bucket by `kind`). The taxonomy is entirely host-owned; omitted, the route still answers with the tenant-wide `all` total. | | `onArtifactCreated` | `(tx, row, scope) => Promise` (optional) | Runs inside the transaction that creates each artifact. This is where the host mints grants for the new row, e.g. `write` and `archive` on `artifact:` for its creator. The package mints none itself. | | `decorate` | `(tenantId, rows) => Promise` (optional) | Adds display-only fields to serialized rows on the way out (provenance labels, host joins). It must never change which rows are returned or who may see them. | @@ -118,6 +118,15 @@ export function buildAssistant(sources: readonly InferencePreference[]) { When the host deploys this agent definition, it must bind the agent's `hub` credential to the agent's hub token. The tools send every request through that credential, so without the binding they cannot reach the hub. +## Upgrading from 0.1.0 + +### Breaking + +- The `skill-draft` kind is no longer reserved. It is an ordinary `kind` string, created, listed and read like any other. +- The `web_site` kind has no special handling: its content is stored as given, and `artifact_read` no longer takes `path` or returns a site summary. The `web-site` exports (`WEB_SITE_KIND`, `WebSiteContentError`, `normalizeWebSiteContent` and the rest) are removed. +- `/instances/:instanceId/mail-attachments`, `saveMailAttachmentRefs`, `listMailAttachmentRefs` and the other mail-attachment exports are removed, and `runArtifactMigrations` drops the `mail_attachment_ref` table. +- `windowContent` is no longer exported. + ## Contributing See [CONTRIBUTING.md](./CONTRIBUTING.md). diff --git a/bun.lock b/bun.lock index a50144f..5d49e49 100644 --- a/bun.lock +++ b/bun.lock @@ -9,6 +9,7 @@ "arktype": "^2.2.3", }, "devDependencies": { + "@corbits/artifacts-0.1.0": "npm:@corbits/artifacts@0.1.0", "@intx/agent": "0.4.0", "@intx/authz": "0.4.0", "@intx/db": "0.4.0", @@ -65,6 +66,8 @@ "@better-fetch/fetch": ["@better-fetch/fetch@1.3.1", "", {}, "sha512-ABkD1WhyfPZprKRQI3bhATjeiFuNWC9PXhfGWqL+sg/gKrM977oFrYkdb4msM3hgUGonr7KlOsOFT5TU2rht9g=="], + "@corbits/artifacts-0.1.0": ["@corbits/artifacts@0.1.0", "", { "dependencies": { "@hono/standard-validator": "^0.2.3" }, "peerDependencies": { "@intx/agent": "^0.4.0", "@intx/hub-api": "^0.4.0", "@intx/types": "^0.4.0", "arktype": "^2.1.29", "drizzle-orm": "^0.45.2", "hono": "^4.12.32", "hono-openapi": "^1.2.0", "postgres": "^3.4.9" } }, "sha512-yO+zgbfWg+ZEv+wia81piyAvWWm1zO5d8Cor+NIkGoV3yaY0jes5rNiq6UmSaTzLqhYyZBu1KNckXoNwzp4FQA=="], + "@gar/promise-retry": ["@gar/promise-retry@1.0.3", "", {}, "sha512-GmzA9ckNokPypTg10pgpeHNQe7ph+iIKKmhKu3Ob9ANkswreCx7R3cKmY781K8QK3AqVL3xVh9A42JvIAbkkSA=="], "@hono/standard-validator": ["@hono/standard-validator@0.2.3", "", { "peerDependencies": { "@standard-schema/spec": "^1.0.0", "hono": ">=3.9.0" } }, "sha512-bp9vHu6Va6SfMHC3D4ZLBbT/woi+AZ9CRdTXQu3kLJuLh2W/Gb9UO4hijS+BQAGFXi4EGpXdetxpzwTAawSVeg=="], diff --git a/examples/reference-host/src/index.ts b/examples/reference-host/src/index.ts index e9cd724..ba8fe77 100644 --- a/examples/reference-host/src/index.ts +++ b/examples/reference-host/src/index.ts @@ -167,7 +167,7 @@ export async function createReferenceHost(): Promise { } await runArtifactMigrations(config, { schema: "public" }); await db.execute( - sql`TRUNCATE TABLE "artifacts"."artifact", "artifacts"."artifact_version", "artifacts"."upload", "artifacts"."mail_attachment_ref" CASCADE`, + sql`TRUNCATE TABLE "artifacts"."artifact", "artifacts"."artifact_version", "artifacts"."upload" CASCADE`, ); await db.execute(sql`DELETE FROM "principal" WHERE "tenant_id" IN (SELECT "id" FROM "tenant" WHERE "slug" = 'reference')`); @@ -304,7 +304,7 @@ export async function createReferenceHost(): Promise { }); // Mounted @corbits/* modules serve under `/api`, matching Interchange's // own convention (`app.route("/api/me", …)`). The core registers its - // routes root-relative (`/artifacts*`, `/instances/:id/mail-attachments`). + // routes root-relative (`/artifacts*`). // The host wraps them in its own `api` sub-app so the principal middleware // below stays scoped to `/api`. Served paths: `/api/artifacts*` — no `/v1` // segment, no vendor prefix. diff --git a/migrations/0001_artifacts.sql b/migrations/0001_artifacts.sql index dcce632..ba4d142 100644 --- a/migrations/0001_artifacts.sql +++ b/migrations/0001_artifacts.sql @@ -58,30 +58,6 @@ CREATE INDEX IF NOT EXISTS "upload_tenant_idx" ON "artifacts"."upload" ("tenant_ --> statement-breakpoint CREATE INDEX IF NOT EXISTS "upload_principal_idx" ON "artifacts"."upload" ("principal_id"); --> statement-breakpoint -CREATE TABLE IF NOT EXISTS "artifacts"."mail_attachment_ref" ( - "id" text PRIMARY KEY DEFAULT gen_random_uuid()::text, - "tenant_id" text NOT NULL REFERENCES "public"."tenant"("id") ON DELETE CASCADE, - "principal_id" text REFERENCES "public"."principal"("id") ON DELETE SET NULL, - "instance_id" text NOT NULL, - "mail_id" text NOT NULL, - "artifact_id" text NOT NULL, - "name" text NOT NULL, - "mime_type" text NOT NULL, - "size" integer NOT NULL, - "created_at" timestamptz NOT NULL DEFAULT now(), - CONSTRAINT "mail_attachment_ref_mail_id_artifact_id" UNIQUE ("mail_id", "artifact_id"), - CONSTRAINT "mail_attachment_ref_size_gte_0" CHECK ("size" >= 0) -); ---> statement-breakpoint -CREATE INDEX IF NOT EXISTS "mail_attachment_ref_instance_idx" - ON "artifacts"."mail_attachment_ref" ("instance_id"); ---> statement-breakpoint -CREATE INDEX IF NOT EXISTS "mail_attachment_ref_tenant_idx" - ON "artifacts"."mail_attachment_ref" ("tenant_id"); ---> statement-breakpoint -CREATE INDEX IF NOT EXISTS "mail_attachment_ref_principal_idx" - ON "artifacts"."mail_attachment_ref" ("principal_id"); ---> statement-breakpoint -- A database from before 0.1.0 that never booted on 0.1.0 still lacks the -- columns 0.1.0 added and keeps zoneless timestamps; bring it to 0.1.0's shape. ALTER TABLE "artifacts"."artifact" diff --git a/migrations/0003_drop_mail_attachment_ref.sql b/migrations/0003_drop_mail_attachment_ref.sql new file mode 100644 index 0000000..0ef7da0 --- /dev/null +++ b/migrations/0003_drop_mail_attachment_ref.sql @@ -0,0 +1 @@ +DROP TABLE IF EXISTS "artifacts"."mail_attachment_ref"; diff --git a/package.json b/package.json index e7eb16b..3fd89f2 100644 --- a/package.json +++ b/package.json @@ -79,6 +79,7 @@ } }, "devDependencies": { + "@corbits/artifacts-0.1.0": "npm:@corbits/artifacts@0.1.0", "@intx/types": "0.4.0", "@types/bun": "1.1.14", "@types/node": "22.10.5", diff --git a/src/artifacts.test.ts b/src/artifacts.test.ts index 623882b..bf66d77 100644 --- a/src/artifacts.test.ts +++ b/src/artifacts.test.ts @@ -18,7 +18,7 @@ import { writeArtifactVersion, } from "./artifacts.js"; import { artifact, artifactVersion } from "./schema.js"; -import { seedArtifact, seedSkillDraft, SCOPE, testDb } from "./test-helpers.js"; +import { seedArtifact, SCOPE, testDb } from "./test-helpers.js"; describe("create", () => { test("writes version 1 eagerly, so a pinned read of v1 resolves immediately", async () => { @@ -37,22 +37,6 @@ describe("create", () => { }); }); - test("refuses to mint a skill-draft", async () => { - const db = await testDb(); - await expect( - db.transaction((tx) => - createArtifact(tx, { - scope: SCOPE, - ownerPrincipalId: null, - kind: "skill-draft", - title: "x", - content: "y", - source: { origin: "agent" }, - }), - ), - ).rejects.toThrow(/skill-draft/); - }); - test("a failure after the bytes are written rolls the whole artifact back", async () => { const db = await testDb(); await expect( @@ -72,18 +56,6 @@ describe("create", () => { const rows = await db.select().from(artifactVersion); expect(rows.length).toBe(0); }); - - test("normalizes web_site content through its schema", async () => { - const db = await testDb(); - const row = await seedArtifact(db, { - kind: "web_site", - content: JSON.stringify({ files: { "/index.html": "

hi

" } }), - }); - expect(JSON.parse(row.content)).toEqual({ - entry: "index.html", - files: { "index.html": "

hi

" }, - }); - }); }); describe("versioning", () => { @@ -209,15 +181,12 @@ describe("find by title", () => { expect((await findArtifactByTitle(db, "acme", "Report"))?.artifactId).toBe(older.id); }); - test("never returns an archived or skill-draft artifact", async () => { + test("never returns an archived artifact", async () => { const db = await testDb(); const row = await seedArtifact(db, { title: "Hidden" }); await setArtifactArchived(db, row, true); - await seedSkillDraft(db, "Scratch"); expect(await findArtifactByTitle(db, "acme", "Hidden")).toBeNull(); - expect(await findArtifactByTitle(db, "acme", "Scratch")).toBeNull(); - expect(await findArtifactByTitle(db, "acme", "Report", "skill-draft")).toBeNull(); }); test("honors a kind filter", async () => { diff --git a/src/artifacts.ts b/src/artifacts.ts index 8dbb77d..d9c86f8 100644 --- a/src/artifacts.ts +++ b/src/artifacts.ts @@ -13,7 +13,6 @@ import { isNull, lt, lte, - ne, or, sql, type SQL, @@ -21,18 +20,6 @@ import { import type { ArtifactDb, ArtifactTx } from "./db.js"; import { artifact, artifactVersion, type ArtifactRow } from "./schema.js"; import type { ResolvedPrincipal } from "./ports.js"; -import { - parseWebSiteContentJson, - serializeWebSiteContent, - WEB_SITE_KIND, -} from "./web-site.js"; - -/** - * Internal skill-authoring scratch. Every surface here treats it as NOT FOUND - * rather than forbidden: its existence is not the caller's business, and a 403 - * would leak that an id is real. - */ -export const SKILL_DRAFT_KIND = "skill-draft"; /** * Max title length (JavaScript string length) accepted on create/revise. @@ -42,8 +29,7 @@ export const MAX_ARTIFACT_TITLE_LENGTH = 512; /** * Max body size in UTF-8 bytes on create/revise. Sized above MAX_UPLOAD_BYTES - * so base64 data-URL expansion for file artifacts still fits, and above the - * web_site total budget. + * so base64 data-URL expansion for file artifacts still fits. */ export const MAX_ARTIFACT_CONTENT_BYTES = 15 * 1024 * 1024; @@ -210,14 +196,6 @@ export function serializeArtifactListItem( return serializeArtifactBase(row); } -/** `web_site` content is round-tripped through its schema; other kinds pass through. */ -function normalizeContentForKind(kind: string, content: string): string { - if (kind === WEB_SITE_KIND) { - return serializeWebSiteContent(parseWebSiteContentJson(content)); - } - return content; -} - /** * sha256 (hex) over the UTF-8 bytes of `content`. Used for every text/URL * artifact write; a blob-backed file artifact's first version instead passes @@ -269,18 +247,14 @@ export async function createArtifact( tx: ArtifactTx, args: CreateArtifactArgs, ): Promise { - if (args.kind === SKILL_DRAFT_KIND) { - throw new Error("skill-draft artifacts are not created through this module"); - } - const content = normalizeContentForKind(args.kind, args.content); - assertArtifactFieldSizes({ title: args.title, content }); + assertArtifactFieldSizes({ title: args.title, content: args.content }); assertVersionMetadataShape({ metadata: args.metadata, parentVersionIds: args.parentVersionIds, }); const metadata = args.metadata ?? null; const parentVersionIds = args.parentVersionIds ?? null; - const contentSha256 = args.contentSha256 ?? sha256Hex(content); + const contentSha256 = args.contentSha256 ?? sha256Hex(args.content); const now = new Date(); const [row] = await tx @@ -291,7 +265,7 @@ export async function createArtifact( ownerPrincipalId: args.ownerPrincipalId, kind: args.kind, title: args.title, - content, + content: args.content, source: args.source, version: 1, metadata, @@ -306,7 +280,7 @@ export async function createArtifact( artifactId: row.id, version: 1, title: args.title, - content, + content: args.content, authorId: args.scope.principalId, metadata, parentVersionIds, @@ -349,7 +323,7 @@ export class VersionConflictError extends Error { * instead of both computing the same next version; the (artifactId, version) * unique index is the second half of that guard. * - * Archived and skill-draft artifacts present as NOT FOUND — an agent holding a + * Archived artifacts present as NOT FOUND — an agent holding a * stale id must not silently revise something the user put away. */ async function reviseArtifactVersion( @@ -388,11 +362,7 @@ async function reviseArtifactVersion( .for("update") .limit(1); - if ( - !existing || - existing.archivedAt !== null || - existing.kind === SKILL_DRAFT_KIND - ) { + if (!existing || existing.archivedAt !== null) { throw new ArtifactNotFoundError(args.artifactId); } @@ -405,10 +375,7 @@ async function reviseArtifactVersion( const version = existing.version + 1; const title = args.title ?? existing.title; - const content = - args.content === undefined - ? existing.content - : normalizeContentForKind(existing.kind, args.content); + const content = args.content ?? existing.content; if (args.content !== undefined) { assertArtifactFieldSizes({ content }); } @@ -763,8 +730,6 @@ export async function listArtifacts( const conditions: SQL[] = [ eq(artifact.tenantId, tenantId), filters.archived ? isNotNull(artifact.archivedAt) : isNull(artifact.archivedAt), - // Never listed, even under an explicit kind=skill-draft filter. - ne(artifact.kind, SKILL_DRAFT_KIND), ]; // ILIKE metacharacters in user input are escaped so a `%` searches for a @@ -823,11 +788,9 @@ async function selectArtifactByTitle( title: string, kind?: string, ): Promise<{ artifactId: string; version: number } | null> { - if (kind === SKILL_DRAFT_KIND) return null; const conditions: SQL[] = [ eq(artifact.tenantId, tenantId), eq(artifact.title, title), - ne(artifact.kind, SKILL_DRAFT_KIND), isNull(artifact.archivedAt), ]; if (kind !== undefined) conditions.push(eq(artifact.kind, kind)); @@ -907,10 +870,9 @@ export type FindOrVersionArtifactResult = { * same title. A caller for a *different* tenant, kind, or title is never * blocked by this lock — the key is scoped to the exact triple. * - * Archived and skill-draft artifacts are invisible to the lookup, same as + * Archived artifacts are invisible to the lookup, same as * `findArtifactByTitle`: an archived match does not get silently revived, and - * a skill-draft is never adopted as the target of a public write. Both cases - * create a fresh artifact instead. + * a fresh artifact is created instead. */ export async function findOrVersionArtifact( db: ArtifactDb, diff --git a/src/index.ts b/src/index.ts index 6569673..d42f93e 100644 --- a/src/index.ts +++ b/src/index.ts @@ -29,7 +29,6 @@ export type { ArtifactRow, ArtifactVersionRow, UploadRow, - MailAttachmentRefRow, } from "./schema.js"; export type { @@ -63,7 +62,6 @@ export { serializeArtifact, serializeArtifactListItem, setArtifactArchived, - SKILL_DRAFT_KIND, writeArtifactVersion, } from "./artifacts.js"; export type { @@ -95,34 +93,10 @@ export type { UploadPolicy } from "./uploads.js"; export { DOWNLOADABLE_ARTIFACT_KINDS, resolveDownload } from "./download.js"; export type { Download, DownloadFailure } from "./download.js"; -export { - listMailAttachmentRefs, - MAIL_ATTACHABLE_KINDS, - MailAttachmentKindError, - MAX_MAIL_ATTACHMENT_BYTES, - MAX_MAIL_ATTACHMENTS_PER_MAIL, - saveMailAttachmentRefs, -} from "./mail-attachments.js"; - export { ARTIFACT_TOOL_DEFINITIONS, linkFileArtifact, readArtifact, readArtifactChunk, - windowContent, } from "./tools.js"; export type { ArtifactReadResult, ArtifactToolDefinition } from "./tools.js"; - -export { - normalizeWebSiteContent, - normalizeWebSitePath, - parseWebSiteContentJson, - serializeWebSiteContent, - summarizeWebSiteContent, - WEB_SITE_KIND, - WEB_SITE_MAX_FILES, - WEB_SITE_MAX_PATH_LENGTH, - WEB_SITE_MAX_TOTAL_BYTES, - WebSiteContentError, -} from "./web-site.js"; -export type { WebSiteContent, WebSiteReadSummary } from "./web-site.js"; diff --git a/src/list.test.ts b/src/list.test.ts index 7712df1..2476927 100644 --- a/src/list.test.ts +++ b/src/list.test.ts @@ -9,7 +9,7 @@ import { serializeArtifactListItem, setArtifactArchived, } from "./artifacts.js"; -import { seedArtifact, seedSkillDraft, testDb } from "./test-helpers.js"; +import { seedArtifact, testDb } from "./test-helpers.js"; import type { ArtifactDb } from "./db.js"; /** Parse a raw query string the way the route does, failing the test on error. */ @@ -63,17 +63,6 @@ describe("list filters", () => { expect(archived.rows.map((r) => r.id)).toEqual([hidden.id]); }); - test("never lists a skill-draft, even under an explicit kind filter", async () => { - const db = await testDb(); - await seedSkillDraft(db, "Scratch"); - await seedArtifact(db, { title: "Real" }); - - expect((await listArtifacts(db, "acme", {})).rows.length).toBe(1); - expect( - (await listArtifacts(db, "acme", { kind: "skill-draft" })).rows.length, - ).toBe(0); - }); - test("is tenant-scoped", async () => { const db = await testDb(); await seedArtifact(db, { title: "Ours" }); diff --git a/src/mail-attachments.test.ts b/src/mail-attachments.test.ts deleted file mode 100644 index 9da6033..0000000 --- a/src/mail-attachments.test.ts +++ /dev/null @@ -1,234 +0,0 @@ -import { describe, expect, test } from "bun:test"; -import { type } from "arktype"; -import { - listMailAttachmentRefs, - MailAttachmentKindError, - MailAttachmentRefSchema, - MAX_MAIL_ATTACHMENT_BYTES, - saveMailAttachmentRefs, - SaveMailAttachmentRefsSchema, -} from "./mail-attachments.js"; -import { seedArtifact, SCOPE, testDb } from "./test-helpers.js"; - -describe("MailAttachmentRefSchema size bounds", () => { - test("accepts a non-negative integer size within the upload ceiling", () => { - const ok = MailAttachmentRefSchema({ - artifactId: "a1", - name: "a.pdf", - type: "application/pdf", - size: 0, - }); - expect(ok instanceof type.errors).toBe(false); - const atCap = MailAttachmentRefSchema({ - artifactId: "a1", - name: "a.pdf", - type: "application/pdf", - size: MAX_MAIL_ATTACHMENT_BYTES, - }); - expect(atCap instanceof type.errors).toBe(false); - }); - - test("rejects a non-integer size", () => { - const bad = MailAttachmentRefSchema({ - artifactId: "a1", - name: "a.pdf", - type: "application/pdf", - size: 1.5, - }); - expect(bad instanceof type.errors).toBe(true); - }); - - test("rejects a size above the upload ceiling", () => { - const bad = MailAttachmentRefSchema({ - artifactId: "a1", - name: "a.pdf", - type: "application/pdf", - size: MAX_MAIL_ATTACHMENT_BYTES + 1, - }); - expect(bad instanceof type.errors).toBe(true); - }); - - test("rejects a negative size", () => { - const bad = MailAttachmentRefSchema({ - artifactId: "a1", - name: "a.pdf", - type: "application/pdf", - size: -1, - }); - expect(bad instanceof type.errors).toBe(true); - }); - - test("rejects an empty contentType", () => { - const bad = MailAttachmentRefSchema({ - artifactId: "a1", - name: "a.pdf", - type: "", - size: 1, - }); - expect(bad instanceof type.errors).toBe(true); - }); - - test("rejects an empty filename", () => { - const bad = MailAttachmentRefSchema({ - artifactId: "a1", - name: "", - type: "application/pdf", - size: 1, - }); - expect(bad instanceof type.errors).toBe(true); - }); -}); - -describe("saveMailAttachmentRefs integrity", () => { - test("refuses a non-file kind and writes nothing", async () => { - const db = await testDb(); - const doc = await seedArtifact(db, { kind: "document", title: "notes.txt" }); - - await expect( - saveMailAttachmentRefs(db, { - scope: SCOPE, - instanceId: "inst-1", - body: { - mailId: "mail-kind", - attachments: [ - { - artifactId: doc.id, - name: "notes.txt", - type: "text/plain", - size: 4, - }, - ], - }, - }), - ).rejects.toBeInstanceOf(MailAttachmentKindError); - - expect(await listMailAttachmentRefs(db, SCOPE, "inst-1")).toEqual([]); - }); - - test("refuses a missing artifact id and writes nothing", async () => { - const db = await testDb(); - const { ArtifactNotFoundError } = await import("./artifacts.js"); - - await expect( - saveMailAttachmentRefs(db, { - scope: SCOPE, - instanceId: "inst-1", - body: { - mailId: "mail-missing", - attachments: [ - { - artifactId: "00000000-0000-4000-8000-000000000000", - name: "ghost.pdf", - type: "application/pdf", - size: 1, - }, - ], - }, - }), - ).rejects.toBeInstanceOf(ArtifactNotFoundError); - - expect(await listMailAttachmentRefs(db, SCOPE, "inst-1")).toEqual([]); - }); - - test("stores artifact-canonical name/type/size, not client-supplied lies", async () => { - const db = await testDb(); - const file = await seedArtifact(db, { - kind: "file", - title: "canonical.pdf", - source: { - origin: "imported", - upload: { - id: "u-canon", - filename: "canonical.pdf", - mimeType: "application/pdf", - size: 42, - }, - }, - }); - - await saveMailAttachmentRefs(db, { - scope: SCOPE, - instanceId: "inst-1", - body: { - mailId: "mail-canon", - attachments: [ - { - artifactId: file.id, - // Deliberately wrong — durable truth must win. - name: "liar.bin", - type: "application/octet-stream", - size: 1, - }, - ], - }, - }); - - expect(await listMailAttachmentRefs(db, SCOPE, "inst-1")).toEqual([ - { - mailId: "mail-canon", - artifactId: file.id, - name: "canonical.pdf", - type: "application/pdf", - size: 42, - }, - ]); - }); - - test("one non-attachable kind in a batch refuses the whole batch", async () => { - const db = await testDb(); - const file = await seedArtifact(db, { - kind: "file", - title: "a.pdf", - source: { - origin: "imported", - upload: { - filename: "a.pdf", - mimeType: "application/pdf", - size: 12, - }, - }, - }); - const doc = await seedArtifact(db, { kind: "document", title: "memo" }); - - await expect( - saveMailAttachmentRefs(db, { - scope: SCOPE, - instanceId: "inst-1", - body: { - mailId: "mail-batch", - attachments: [ - { - artifactId: file.id, - name: "a.pdf", - type: "application/pdf", - size: 12, - }, - { - artifactId: doc.id, - name: "memo", - type: "text/plain", - size: 4, - }, - ], - }, - }), - ).rejects.toBeInstanceOf(MailAttachmentKindError); - - expect(await listMailAttachmentRefs(db, SCOPE, "inst-1")).toEqual([]); - }); - - test("SaveMailAttachmentRefsSchema rejects a non-integer size at the body edge", () => { - const bad = SaveMailAttachmentRefsSchema({ - mailId: "mail-1", - attachments: [ - { - artifactId: "a1", - name: "a.pdf", - type: "application/pdf", - size: 3.14, - }, - ], - }); - expect(bad instanceof type.errors).toBe(true); - }); -}); diff --git a/src/mail-attachments.ts b/src/mail-attachments.ts deleted file mode 100644 index 1df0048..0000000 --- a/src/mail-attachments.ts +++ /dev/null @@ -1,236 +0,0 @@ -import "./arktype.js"; -import { and, eq, inArray, ne } from "drizzle-orm"; -import { type } from "arktype"; -import { ArtifactNotFoundError, SKILL_DRAFT_KIND } from "./artifacts.js"; -import type { ArtifactDb, ArtifactTx } from "./db.js"; -import { artifact, mailAttachmentRef } from "./schema.js"; -import type { ResolvedPrincipal } from "./ports.js"; -import { uploadRefFromSource } from "./content-store.js"; -import { MAX_UPLOAD_BYTES } from "./uploads.js"; - -// Caps are generous working bounds, not business rules: they exist so one -// request cannot bloat the table with megabyte-long names or an unbounded -// attachment list. -export const MAX_MAIL_ATTACHMENTS_PER_MAIL = 100; - -/** - * Per-attachment byte ceiling on the request body. Matches the upload path so a - * caller cannot claim a size the store would never have accepted. When the - * referenced artifact carries a larger durable size (already on disk), that - * artifact truth is what is stored — this bound only gates the request field. - */ -export const MAX_MAIL_ATTACHMENT_BYTES = MAX_UPLOAD_BYTES; - -/** - * Kinds that may ride as a mail attachment. File/image are the kinds - * `createFileArtifact` mints; everything else (document, link, web_site, …) is - * a different resource class and is refused at create time. - */ -export const MAIL_ATTACHABLE_KINDS: ReadonlySet = new Set([ - "file", - "image", -]); - -/** - * A visible artifact whose kind is outside the attachable allowlist. Distinct - * from `ArtifactNotFoundError` so the route can answer 400 (bad association) - * rather than 404 (existence oracle collapse for ids the caller cannot see). - */ -export class MailAttachmentKindError extends Error { - constructor(readonly artifactId: string, readonly kind: string) { - super( - `Artifact "${artifactId}" has kind "${kind}", which cannot be a mail attachment`, - ); - this.name = "MailAttachmentKindError"; - } -} - -export const MailAttachmentRefSchema = type({ - artifactId: "string > 0", - name: "0 < string <= 512", - type: "0 < string <= 255", - // Integer only: the column is `integer`, and a float would silently truncate. - // Upper bound matches the upload ceiling so the request cannot claim a size - // the store refuses to mint. - size: type.keywords.number.integer - .atLeast(0) - .atMost(MAX_MAIL_ATTACHMENT_BYTES), -}); - -export const SaveMailAttachmentRefsSchema = type({ - mailId: "0 < string <= 512", - attachments: MailAttachmentRefSchema.array() - .atLeastLength(1) - .atMostLength(MAX_MAIL_ATTACHMENTS_PER_MAIL), -}); -export type SaveMailAttachmentRefs = typeof SaveMailAttachmentRefsSchema.infer; - -type AttachableArtifact = { - id: string; - kind: string; - title: string; - source: unknown; -}; - -/** - * Prefer the artifact's durable upload metadata over client-supplied fields so - * list payloads and the stored row always match the same truth a download would - * serve. Client name/type/size fill gaps only when the artifact has no - * `source.upload` bag (e.g. a hand-seeded file row in tests). - */ -function canonicalAttachmentMeta( - row: AttachableArtifact, - client: { name: string; type: string; size: number }, -): { name: string; type: string; size: number } { - const upload = uploadRefFromSource(row.source); - const name = - (upload?.filename && upload.filename.length > 0 - ? upload.filename - : row.title.trim().length > 0 - ? row.title - : client.name - ).slice(0, 512); - const contentType = - upload?.mimeType && upload.mimeType.length > 0 - ? upload.mimeType.slice(0, 255) - : client.type; - const size = - upload !== null && - typeof upload.size === "number" && - Number.isInteger(upload.size) && - upload.size >= 0 - ? upload.size - : client.size; - return { name, type: contentType, size }; -} - -/** - * Every `artifactId` in the body must name an attachable artifact this caller - * can actually see, or the whole request is refused. - * - * This table is an artifact↔message association — and an association - * to something that is not an artifact of this tenant is not one. Unvalidated, - * `POST /instances/:id/mail-attachments` accepted ANY string: it happily - * recorded a reference to another tenant's artifact id and answered 201, which - * both wrote a cross-tenant edge into the table and told the caller (by way of - * a durable row that the matching GET reads back) that the id was worth - * keeping. The check is the same one every detail route makes, and the answer - * is deliberately the same 404 for an unknown id, a malformed one, a - * skill-draft and another tenant's — this route must not become the existence - * oracle the detail routes stopped being. - * - * Non-attachable kinds (document, link, …) that *are* visible are a different - * class of failure: the id is real and tenant-owned, but the kind is outside - * the file/image allowlist. Those throw `MailAttachmentKindError` (400). - * - * `mail_attachment_ref.artifact_id` is `text` with no database FK — lifecycle - * is enforced here in the same transaction as the insert (visibility + kind + - * write). A package-local FK onto `artifacts.artifact(id)` is feasible without - * host-schema redesign, but deferred to avoid colliding with broader schema - * migrations; app-level transactional checks are the current invariant. - */ -async function loadAttachableArtifacts( - tx: ArtifactTx, - scope: ResolvedPrincipal, - artifactIds: string[], -): Promise> { - const wanted = [...new Set(artifactIds)]; - if (wanted.length === 0) return new Map(); - const rows = await tx - .select({ - id: artifact.id, - kind: artifact.kind, - title: artifact.title, - source: artifact.source, - }) - .from(artifact) - .where( - and( - inArray(artifact.id, wanted), - eq(artifact.tenantId, scope.tenantId), - ne(artifact.kind, SKILL_DRAFT_KIND), - ), - ); - const byId = new Map(rows.map((r) => [r.id, r])); - for (const id of wanted) { - const row = byId.get(id); - if (row === undefined) throw new ArtifactNotFoundError(id); - if (!MAIL_ATTACHABLE_KINDS.has(row.kind)) { - throw new MailAttachmentKindError(id, row.kind); - } - } - return byId; -} - -/** - * Record which artifacts rode along with a message. No bytes move: the file - * already IS an artifact, and this is purely the artifact↔message association - * a transcript replays to rehydrate its attachment chips. Idempotent on - * (mailId, artifactId) so a retried send does not duplicate chips. - * - * Visibility, kind allowlist, and insert run in one transaction so a concurrent - * delete/re-kind cannot leave a dangling ref that the pre-check would have - * refused. Throws `ArtifactNotFoundError` if any referenced artifact is not - * visible to `scope`, or `MailAttachmentKindError` if a visible artifact is - * not file/image; nothing is written in either case. Stored name/type/size are - * taken from the artifact's durable upload metadata when present. - */ -export async function saveMailAttachmentRefs( - db: ArtifactDb, - args: { - scope: ResolvedPrincipal; - instanceId: string; - body: SaveMailAttachmentRefs; - }, -): Promise { - await db.transaction(async (tx) => { - const byId = await loadAttachableArtifacts( - tx, - args.scope, - args.body.attachments.map((a) => a.artifactId), - ); - await tx - .insert(mailAttachmentRef) - .values( - args.body.attachments.map((a) => { - const row = byId.get(a.artifactId)!; - const meta = canonicalAttachmentMeta(row, a); - return { - tenantId: args.scope.tenantId, - principalId: args.scope.principalId, - instanceId: args.instanceId, - mailId: args.body.mailId, - artifactId: a.artifactId, - name: meta.name, - mimeType: meta.type, - size: meta.size, - }; - }), - ) - .onConflictDoNothing(); - }); -} - -export async function listMailAttachmentRefs( - db: ArtifactDb, - scope: ResolvedPrincipal, - instanceId: string, -): Promise< - { mailId: string; artifactId: string; name: string; type: string; size: number }[] -> { - return await db - .select({ - mailId: mailAttachmentRef.mailId, - artifactId: mailAttachmentRef.artifactId, - name: mailAttachmentRef.name, - type: mailAttachmentRef.mimeType, - size: mailAttachmentRef.size, - }) - .from(mailAttachmentRef) - .where( - and( - eq(mailAttachmentRef.instanceId, instanceId), - eq(mailAttachmentRef.tenantId, scope.tenantId), - ), - ); -} diff --git a/src/mount.test.ts b/src/mount.test.ts index 6524fcd..bf7aeb0 100644 --- a/src/mount.test.ts +++ b/src/mount.test.ts @@ -20,7 +20,7 @@ import { import type { ArtifactDb } from "./db.js"; import type { CreateArtifactRoutesDeps } from "./mount.js"; import type { ResolvedPrincipal } from "./ports.js"; -import { seedArtifact, seedSkillDraft, SCOPE, testDb } from "./test-helpers.js"; +import { seedArtifact, SCOPE, testDb } from "./test-helpers.js"; /** Places tenant/principal on the context the way a real host's session * middleware does, without pinning it to any one `requireGrant` wiring. */ @@ -449,11 +449,10 @@ describe("every way of not getting an artifact is indistinguishable", () => { [`/artifacts/${id}/download`, {}], ]; - test("skill-draft, ghost id, cross-tenant and malformed id agree on all six routes", async () => { + test("ghost id, cross-tenant and malformed id agree on all six routes", async () => { const db = await testDb(); const app = host(db); const cases: [string, string][] = [ - ["skill-draft", await seedSkillDraft(db, "scratch")], ["ghost id", "00000000-0000-4000-8000-000000000000"], ["cross-tenant", (await seedArtifact(db, { tenantId: "other" })).id], ["malformed id", "not-a-uuid"], @@ -723,64 +722,6 @@ describe("versions", () => { const res = await host(db).request(`/artifacts/${row.id}/versions`, json({ content: "x" })); expect(res.status).toBe(404); }); - - // Named for exactly what it asserts: every single-artifact route. - // These all funnel through `loadScoped`, and the point of the test is that - // none of them can drift away from the choke point unnoticed. - test("a skill-draft is 404 on read, versions, revise, archive, unarchive and download", async () => { - const db = await testDb(); - const id = await seedSkillDraft(db, "scratch"); - const app = host(db); - - const GET = {} as const; - const POST = { method: "POST" } as const; - const routes: [string, RequestInit][] = [ - [`/artifacts/${id}`, GET], - [`/artifacts/${id}/versions`, GET], - [`/artifacts/${id}/versions`, json({ content: "x" })], - [`/artifacts/${id}/versions/1`, GET], - [`/artifacts/${id}/archive`, POST], - [`/artifacts/${id}/unarchive`, POST], - [`/artifacts/${id}/download`, GET], - ]; - for (const [path, init] of routes) { - const res = await app.request(path, init); - expect({ - route: `${init.method ?? "GET"} ${path}`, - status: res.status, - body: await res.json(), - }).toEqual({ - route: `${init.method ?? "GET"} ${path}`, - status: 404, - body: { error: "Artifact not found" }, - }); - } - }); - - // The archive route mutates before it answers, so a mere status assertion - // would still pass if the row had already been changed. Read the row back. - test("a refused skill-draft archive leaves the row untouched", async () => { - const db = await testDb(); - const id = await seedSkillDraft(db, "scratch"); - expect((await host(db).request(`/artifacts/${id}/archive`, { method: "POST" })).status).toBe( - 404, - ); - const rows = await db.execute<{ archived_at: Date | null }>( - sql`SELECT "archived_at" FROM "artifacts"."artifact" WHERE "id" = ${id}`, - ); - expect(rows[0]!.archived_at).toBeNull(); - }); - - // A skill-draft is invisible, not merely un-writable: the id must not be - // distinguishable from one that was never minted. - test("an unknown id and a skill-draft id are indistinguishable", async () => { - const db = await testDb(); - const app = host(db); - const draft = await app.request(`/artifacts/${await seedSkillDraft(db, "scratch")}`); - const unknown = await app.request("/artifacts/00000000-0000-4000-8000-000000000000"); - expect(draft.status).toBe(unknown.status); - expect(await draft.json()).toEqual(await unknown.json()); - }); }); describe("GET /artifacts/:id/versions/:version", () => { @@ -1345,146 +1286,6 @@ describe("download over HTTP", () => { }); }); -describe("mail attachment references", () => { - // Auth before body parse on the write route. - test("POST is 403 for an unauthenticated caller even with an empty or invalid body", async () => { - const db = await testDb(); - const app = host(db, { principal: null }); - for (const body of ["", "{", "{}", "null"]) { - const res = await app.request("/instances/inst-1/mail-attachments", { - method: "POST", - headers: { "content-type": "application/json" }, - body, - }); - expect({ body, status: res.status, json: await res.json() }).toEqual({ - body, - status: 403, - json: { error: "Tenant not accessible" }, - }); - } - }); - - test("records and lists an artifact↔message association, idempotently", async () => { - const db = await testDb(); - const app = host(db); - const file = await seedArtifact(db, { kind: "file", title: "a.pdf" }); - - const body = { - mailId: "mail-1", - attachments: [ - { artifactId: file.id, name: "a.pdf", type: "application/pdf", size: 12 }, - ], - }; - expect((await app.request("/instances/inst-1/mail-attachments", json(body))).status).toBe( - 201, - ); - expect((await app.request("/instances/inst-1/mail-attachments", json(body))).status).toBe( - 201, - ); - - const listed = (await ( - await app.request("/instances/inst-1/mail-attachments") - ).json()) as { refs: unknown[] }; - expect(listed.refs).toEqual([ - { mailId: "mail-1", artifactId: file.id, name: "a.pdf", type: "application/pdf", size: 12 }, - ]); - }); - - test("rejects a malformed body and refuses the WRITE without a principal", async () => { - const db = await testDb(); - expect( - ( - await host(db).request( - "/instances/inst-1/mail-attachments", - json({ mailId: "", attachments: [] }), - ) - ).status, - ).toBe(400); - // The write is a mutation, so 403. The matching READ is a collection read - // and answers an empty 200 — see the no-principal route-class block below. - expect( - ( - await host(db, { principal: null }).request( - "/instances/inst-1/mail-attachments", - json({ - mailId: "mail-1", - attachments: [ - { artifactId: "a", name: "a.pdf", type: "application/pdf", size: 1 }, - ], - }), - ) - ).status, - ).toBe(403); - }); - - // The route used to accept ANY string as an artifactId with no existence and - // no tenant check, so a reference to another tenant's artifact was recorded - // and answered 201. This table is an artifact↔message association; an - // association to something that is not this tenant's artifact is not one. - test("an artifactId the caller cannot see is refused, and nothing is written", async () => { - const db = await testDb(); - const app = host(db); - const foreign = await seedArtifact(db, { tenantId: "other" }); - const draft = await seedSkillDraft(db, "scratch"); - - const post = (artifactId: string, mailId: string) => - app.request( - "/instances/inst-1/mail-attachments", - json({ - mailId, - attachments: [ - { artifactId, name: "a.pdf", type: "application/pdf", size: 1 }, - ], - }), - ); - - for (const [cause, artifactId] of [ - ["ghost id", "00000000-0000-4000-8000-000000000000"], - ["cross-tenant", foreign.id], - ["skill-draft", draft], - ["malformed id", "not-a-uuid"], - ] as [string, string][]) { - const res = await post(artifactId, `mail-${cause}`); - expect({ cause, status: res.status, body: await res.json() }).toEqual({ - cause, - status: 404, - body: { error: "Artifact not found" }, - }); - } - - const listed = (await ( - await app.request("/instances/inst-1/mail-attachments") - ).json()) as { refs: unknown[] }; - expect(listed.refs).toEqual([]); - }); - - // All-or-nothing: one bad reference in a batch refuses the batch, so a - // caller cannot smuggle a foreign id alongside a legitimate one. - test("one unusable reference in a batch refuses the whole batch", async () => { - const db = await testDb(); - const app = host(db); - const mine = await seedArtifact(db, { kind: "file", title: "a.pdf" }); - const foreign = await seedArtifact(db, { tenantId: "other" }); - - const res = await app.request( - "/instances/inst-1/mail-attachments", - json({ - mailId: "mail-1", - attachments: [ - { artifactId: mine.id, name: "a.pdf", type: "application/pdf", size: 1 }, - { artifactId: foreign.id, name: "b.pdf", type: "application/pdf", size: 1 }, - ], - }), - ); - expect(res.status).toBe(404); - - const listed = (await ( - await app.request("/instances/inst-1/mail-attachments") - ).json()) as { refs: unknown[] }; - expect(listed.refs).toEqual([]); - }); -}); - describe("post-commit side effects never turn a committed write into a 500", () => { // Enrichment runs against HOST-supplied decorator after the transaction commits. // A throwing host must not make a durable mutation report failure: the client @@ -1550,10 +1351,6 @@ describe("no-principal response: every route matches the cross-core rule", () => const list = await app.request("/artifacts"); expect(list.status).toBe(200); expect(await list.json()).toEqual({ artifacts: [], nextCursor: null }); - - const refs = await app.request("/instances/inst-1/mail-attachments"); - expect(refs.status).toBe(200); - expect(await refs.json()).toEqual({ refs: [] }); }); // Detail reads name one artifact, and whether it exists is not a signed-out @@ -1598,19 +1395,6 @@ describe("no-principal response: every route matches the cross-core rule", () => expect( (await app.request(`/artifacts/${row.id}/unarchive`, { method: "POST" })).status, ).toBe(403); - expect( - ( - await app.request( - "/instances/inst-1/mail-attachments", - json({ - mailId: "mail-1", - attachments: [ - { artifactId: row.id, name: "a.pdf", type: "application/pdf", size: 1 }, - ], - }), - ) - ).status, - ).toBe(403); }); // A refused mutation must also not have happened. A 403 that still wrote @@ -1645,20 +1429,6 @@ describe("hardening regressions", () => { expect(checks).toEqual([{ resource: `artifact:${row.id}`, action: "write" }]); }); - test("revising a web_site with invalid content is 400, not 404", async () => { - const db = await testDb(); - const app = host(db); - const row = await seedArtifact(db, { - kind: "web_site", - content: JSON.stringify({ entry: "index.html", files: { "index.html": "

" } }), - }); - const res = await app.request( - `/artifacts/${row.id}/versions`, - json({ content: JSON.stringify({ files: {} }) }), - ); - expect(res.status).toBe(400); - }); - test("kind must agree with mode on import", async () => { const db = await testDb(); const app = host(db); diff --git a/src/mount.ts b/src/mount.ts index 1ab28c2..bcebf2f 100644 --- a/src/mount.ts +++ b/src/mount.ts @@ -21,7 +21,6 @@ import { serializeArtifact, serializeArtifactListItem, setArtifactArchived, - SKILL_DRAFT_KIND, VersionConflictError, writeArtifactVersion, type ArtifactListRow, @@ -37,12 +36,6 @@ import { type ArtifactCountSegments, } from "./counts.js"; import { artifactPreviewHeaders, resolveArtifactPreview } from "./preview.js"; -import { - listMailAttachmentRefs, - MailAttachmentKindError, - saveMailAttachmentRefs, - SaveMailAttachmentRefsSchema, -} from "./mail-attachments.js"; import type { ArtifactRow } from "./schema.js"; import type { ResolvedPrincipal, ContentStore } from "./ports.js"; import { @@ -56,7 +49,6 @@ import { UnsupportedUploadTypeError, type UploadPolicy, } from "./uploads.js"; -import { WebSiteContentError } from "./web-site.js"; export type CreateArtifactRoutesDeps = { db: ArtifactDb; @@ -254,14 +246,14 @@ export function createArtifactRoutes({ }; /** - * Confirm the id names a real, in-tenant, non-skill-draft artifact — or + * Confirm the id names a real, in-tenant artifact — or * answer the same 404 `loadScoped` does — BEFORE `requireGrant` runs. * * Must run between `principalRequired` and `requireGrant`: a real * `requireGrant` (Interchange's `authorize()`) has no existence check of its * own — it just asks whether the caller holds a grant naming the resource - * string built from the URL param, real artifact or not. A ghost id, a - * skill-draft, and another tenant's artifact all name a resource the caller + * string built from the URL param, real artifact or not. A ghost id and + * another tenant's artifact both name a resource the caller * holds no grant for, so without this check they would deny with the SAME * 403 a real artifact the caller merely lacks permission on gets — losing * the one thing single-artifact routes guarantee: a caller who cannot see @@ -274,7 +266,7 @@ export function createArtifactRoutes({ // principalRequired already ran; a null scope here would mean it didn't. if (!scope) return c.json({ error: "Forbidden" }, 403); const row = await getArtifact(db, c.req.param("id")!); - if (!row || row.kind === SKILL_DRAFT_KIND || row.tenantId !== scope.tenantId) { + if (!row || row.tenantId !== scope.tenantId) { return c.json({ error: "Artifact not found" }, 404); } await next(); @@ -332,7 +324,7 @@ export function createArtifactRoutes({ * Load an artifact and confirm the caller may see it, or produce the reply. * Every single-artifact route funnels through here: no principal answers 403 * before any id is looked at (collection reads instead answer an empty 200). - * A missing id, a malformed id, a skill-draft, and another tenant's artifact + * A missing id, a malformed id, and another tenant's artifact * all collapse to the same 404 so the route is not an existence oracle. */ async function loadScoped( @@ -345,7 +337,7 @@ export function createArtifactRoutes({ if (!scope) return { response: c.json({ error: "Forbidden" }, 403) }; const row = await getArtifact(db, c.req.param("id")!); - if (!row || row.kind === SKILL_DRAFT_KIND || row.tenantId !== scope.tenantId) { + if (!row || row.tenantId !== scope.tenantId) { return { response: c.json({ error: "Artifact not found" }, 404) }; } return { row, scope }; @@ -357,7 +349,7 @@ export function createArtifactRoutes({ tags: ["Artifacts"], summary: "List artifacts in the caller's tenant", description: - "Newest-updated first by default. Supports query/kind/owner/date filters, an `updatedAt__id` keyset cursor, and an archived-only toggle. skill-draft artifacts are never listed. List is discovery only: each item omits `content` (fetch the body via GET /artifacts/:id, download, or tools).", + "Newest-updated first by default. Supports query/kind/owner/date filters, an `updatedAt__id` keyset cursor, and an archived-only toggle. List is discovery only: each item omits `content` (fetch the body via GET /artifacts/:id, download, or tools).", parameters: [ { name: "query", in: "query", required: false, schema: { type: "string" } }, { name: "sort", in: "query", required: false, schema: { type: "string" } }, @@ -474,9 +466,6 @@ export function createArtifactRoutes({ if (error instanceof ArtifactSizeError) { return c.json({ error: error.message }, 400); } - if (error instanceof WebSiteContentError) { - return c.json({ error: error.message }, 400); - } throw error; } }, @@ -659,7 +648,7 @@ export function createArtifactRoutes({ 403: { description: "No resolvable principal" }, 404: { description: - "Artifact not found — also the answer for a malformed id, a skill-draft, and another tenant's artifact", + "Artifact not found — also the answer for a malformed id and another tenant's artifact", }, }, }), @@ -714,7 +703,7 @@ export function createArtifactRoutes({ 403: { description: "No resolvable principal" }, 404: { description: - "Artifact not found — also the answer for a malformed id, a skill-draft, another tenant's artifact, or an unknown version", + "Artifact not found — also the answer for a malformed id, another tenant's artifact, or an unknown version", }, }, }), @@ -750,7 +739,7 @@ export function createArtifactRoutes({ tags: ["Artifacts"], summary: "Revise an artifact, creating a new version", description: - "Locks the row FOR UPDATE and bumps version by one; a unique (artifactId, version) index backstops a racing writer. Archived and skill-draft artifacts present as not found. `metadata` is optional and opaque; when omitted, the prior version's metadata carries forward, and an explicit `null` clears it. An optional `expectedVersion` is checked under the same lock: a mismatch answers 409 and writes nothing.", + "Locks the row FOR UPDATE and bumps version by one; a unique (artifactId, version) index backstops a racing writer. Archived artifacts present as not found. `metadata` is optional and opaque; when omitted, the prior version's metadata carries forward, and an explicit `null` clears it. An optional `expectedVersion` is checked under the same lock: a mismatch answers 409 and writes nothing.", parameters: [idParam], responses: { 200: { description: "New version created" }, @@ -807,9 +796,6 @@ export function createArtifactRoutes({ if (error instanceof ArtifactSizeError) { return c.json({ error: error.message }, 400); } - if (error instanceof WebSiteContentError) { - return c.json({ error: error.message }, 400); - } throw error; } }, @@ -944,87 +930,5 @@ export function createArtifactRoutes({ }, ); - app.post( - "/instances/:instanceId/mail-attachments", - describeRoute({ - tags: ["Artifacts"], - summary: "Associate file artifacts with a sent message", - description: - "Records which artifacts were attached to a message so a transcript can rehydrate its chips after reload. No bytes move — the files are already artifacts. Idempotent per (mailId, artifactId).", - parameters: [ - { name: "instanceId", in: "path", required: true, schema: { type: "string" } }, - ], - responses: { - 201: { description: "References recorded" }, - 400: { - description: - "Invalid request body, or a referenced artifact is not an attachable file/image kind", - }, - 403: { description: "Tenant not accessible" }, - 404: { description: "A referenced artifact is not visible to the caller" }, - 413: { description: "Declared Content-Length over the content ceiling" }, - }, - }), - async (c) => { - const scope = await scopeFor(c); - if (!scope) return c.json({ error: "Tenant not accessible" }, 403); - if (contentLengthOverCeiling(c)) { - return c.json( - { - error: `Request body exceeds the ${MAX_ARTIFACT_CONTENT_BYTES} byte limit`, - }, - 413, - ); - } - const raw = await readJson(c); - const body = SaveMailAttachmentRefsSchema(raw); - if (body instanceof type.errors) return c.json({ error: body.summary }, 400); - try { - await saveMailAttachmentRefs(db, { - scope, - instanceId: c.req.param("instanceId")!, - body, - }); - } catch (err) { - // Same body as every detail route, so naming another tenant's artifact - // here is indistinguishable from naming one that never existed. - if (err instanceof ArtifactNotFoundError) { - return c.json({ error: "Artifact not found" }, 404); - } - // Visible but wrong kind: the id is real to this tenant, so 400 rather - // than collapsing into the 404 existence oracle. - if (err instanceof MailAttachmentKindError) { - return c.json({ error: err.message }, 400); - } - throw err; - } - return c.json({}, 201); - }, - ); - - app.get( - "/instances/:instanceId/mail-attachments", - describeRoute({ - tags: ["Artifacts"], - summary: "List artifact↔message associations for an instance", - parameters: [ - { name: "instanceId", in: "path", required: true, schema: { type: "string" } }, - ], - responses: { - 200: { description: "All references for this instance" }, - }, - }), - async (c) => { - // A collection read, so the no-member asymmetry makes this an empty 200: - // a caller with no resolvable principal has no references, which is a - // fact, not a refusal. It names no artifact, so answering leaks nothing. - const scope = await scopeFor(c); - if (!scope) return c.json({ refs: [] }); - return c.json({ - refs: await listMailAttachmentRefs(db, scope, c.req.param("instanceId")!), - }); - }, - ); - return app; } diff --git a/src/schema.ts b/src/schema.ts index c84208c..4ff44b7 100644 --- a/src/schema.ts +++ b/src/schema.ts @@ -166,39 +166,6 @@ export const upload = artifactsSchema.table( ], ); -/** - * An artifact↔message association. Carries no bytes: the file already IS an - * artifact, and this only records which artifacts rode along with which - * message so a transcript can rehydrate its attachment chips. - */ -export const mailAttachmentRef = artifactsSchema.table( - "mail_attachment_ref", - { - id: surrogateId(), - tenantId: tenantRef("tenant_id").notNull(), - // Nullable so a removed principal SET NULLs instead of blocking the delete. - principalId: principalRef("principal_id"), - instanceId: text("instance_id").notNull(), - mailId: text("mail_id").notNull(), - artifactId: text("artifact_id").notNull(), - name: text("name").notNull(), - mimeType: text("mime_type").notNull(), - size: integer("size").notNull(), - createdAt: timestamp("created_at", { withTimezone: true }).notNull().defaultNow(), - }, - (t) => [ - unique("mail_attachment_ref_mail_id_artifact_id").on( - t.mailId, - t.artifactId, - ), - index("mail_attachment_ref_instance_idx").on(t.instanceId), - index("mail_attachment_ref_tenant_idx").on(t.tenantId), - index("mail_attachment_ref_principal_idx").on(t.principalId), - check("mail_attachment_ref_size_gte_0", sql`${t.size} >= 0`), - ], -); - export type ArtifactRow = typeof artifact.$inferSelect; export type ArtifactVersionRow = typeof artifactVersion.$inferSelect; export type UploadRow = typeof upload.$inferSelect; -export type MailAttachmentRefRow = typeof mailAttachmentRef.$inferSelect; diff --git a/src/sidecar-bundle.test.ts b/src/sidecar-bundle.test.ts index 464e29c..3b2a4fa 100644 --- a/src/sidecar-bundle.test.ts +++ b/src/sidecar-bundle.test.ts @@ -221,8 +221,8 @@ describe("every declared tool maps onto a route", () => { ], [ "artifact_read", - { artifactId: "a1", path: "index.html" }, - "/api/workflow-artifacts/artifacts/a1/read?path=index.html", + { artifactId: "a1", version: 2 }, + "/api/workflow-artifacts/artifacts/a1/read?version=2", ], ]; diff --git a/src/sidecar-bundle.ts b/src/sidecar-bundle.ts index 02ea251..5936533 100644 --- a/src/sidecar-bundle.ts +++ b/src/sidecar-bundle.ts @@ -84,7 +84,6 @@ function requestFor(name: string, args: Record): Request_ | und method: "GET", path: `/artifacts/${encodeURIComponent(String(args["artifactId"] ?? ""))}/read${query({ version: args["version"], - path: args["path"], })}`, }; case "artifact_read_chunk": diff --git a/src/test-helpers.ts b/src/test-helpers.ts index 03a6b02..849cdfe 100644 --- a/src/test-helpers.ts +++ b/src/test-helpers.ts @@ -166,7 +166,7 @@ export async function testDb(): Promise { await runArtifactMigrations(databaseConfig(DATABASE_URL), { schema: "public" }); } await db.execute( - sql`TRUNCATE TABLE "artifacts"."artifact", "artifacts"."artifact_version", "artifacts"."upload", "artifacts"."mail_attachment_ref" CASCADE`, + sql`TRUNCATE TABLE "artifacts"."artifact", "artifacts"."artifact_version", "artifacts"."upload" CASCADE`, ); return db; } @@ -199,15 +199,3 @@ export async function seedArtifact( }), ); } - -/** Bypasses `createArtifact` so a test can plant a kind the module refuses to mint. */ -export async function seedSkillDraft(db: ArtifactDb, title: string): Promise { - const rows = await db.execute<{ id: string }>(sql` - INSERT INTO "artifacts"."artifact" ("tenant_id", "principal_id", "owner_principal_id", - "kind", "title", "content", "source", "version") - VALUES (${SCOPE.tenantId}, ${SCOPE.principalId}, ${SCOPE.principalId}, - 'skill-draft', ${title}, 'draft body', '{"origin":"agent"}'::jsonb, 1) - RETURNING "id" - `); - return rows[0]!.id; -} diff --git a/src/tools.test.ts b/src/tools.test.ts index f394b9f..97edf49 100644 --- a/src/tools.test.ts +++ b/src/tools.test.ts @@ -12,45 +12,57 @@ import { readArtifact, readArtifactChunk, SAFE_ENCODED_BUDGET, - windowContent, } from "./tools.js"; -import { seedArtifact, seedSkillDraft, SCOPE, testDb } from "./test-helpers.js"; +import { seedArtifact, SCOPE, testDb } from "./test-helpers.js"; + +async function read(content: string, offset?: number, limit?: number) { + const db = await testDb(); + const row = await seedArtifact(db, { content }); + if (offset === undefined) return await readArtifact(db, { scope: SCOPE, artifactId: row.id }); + return await readArtifactChunk(db, { + scope: SCOPE, + artifactId: row.id, + offset, + ...(limit !== undefined ? { limit } : {}), + }); +} -const base = { artifactId: "a1", title: "T", kind: "document", version: 1 }; const encoded = (value: unknown) => JSON.stringify(value, null, 2).length; describe("read windowing", () => { - test("returns short content whole, with no chunk metadata", () => { - const result = windowContent(base, "short body"); + test("returns short content whole, with no chunk metadata", async () => { + const result = await read("short body"); expect(result.content).toBe("short body"); expect(result.contentLength).toBeUndefined(); expect(result.continuation).toBeUndefined(); }); - test("chunks content longer than the default read limit", () => { + test("chunks content longer than the default read limit", async () => { const content = "x".repeat(DEFAULT_READ_LIMIT + 500); - const result = windowContent(base, content); + const result = await read(content); expect(result.contentLength).toBe(content.length); expect(result.chunkStart).toBe(0); expect(result.continuation).toContain(`offset=${result.chunkEnd}`); }); - test("shrinks a chunk whose JSON encoding would blow the budget", () => { + test("shrinks a chunk whose JSON encoding would blow the budget", async () => { // Every character escapes to two, so a raw slice at the default limit // encodes to well over the budget unless the window shrinks. const content = "\n".repeat(DEFAULT_READ_LIMIT * 2); - const result = windowContent(base, content); + const result = await read(content); expect(encoded(result)).toBeLessThanOrEqual(SAFE_ENCODED_BUDGET); expect(result.chunkEnd!).toBeLessThan(DEFAULT_READ_LIMIT); expect(result.continuation).toBeDefined(); }); - test("walking the continuation offsets reads the whole content exactly once", () => { + test("walking the continuation offsets reads the whole content exactly once", async () => { const content = "abcdefghij".repeat(2000); + const db = await testDb(); + const row = await seedArtifact(db, { content }); let offset = 0; let assembled = ""; for (let guard = 0; guard < 100; guard += 1) { - const result = windowContent(base, content, offset, 3000); + const result = await read(content, offset, 3000); assembled += result.content; if (result.continuation === undefined) break; offset = result.chunkEnd!; @@ -58,8 +70,8 @@ describe("read windowing", () => { expect(assembled).toBe(content); }); - test("an offset past the end yields an empty final chunk", () => { - const result = windowContent(base, "abc", 99, 10); + test("an offset past the end yields an empty final chunk", async () => { + const result = await read("abc", 99, 10); expect(result.content).toBe(""); expect(result.continuation).toBeUndefined(); }); @@ -101,14 +113,6 @@ describe("artifact_read", () => { ).rejects.toThrow(/Version 7 not found/); }); - test("a skill-draft is not found, not forbidden", async () => { - const db = await testDb(); - const id = await seedSkillDraft(db, "scratch"); - await expect( - readArtifact(db, { scope: SCOPE, artifactId: id }), - ).rejects.toBeInstanceOf(ArtifactNotFoundError); - }); - test("an artifact in another tenant is not found", async () => { const db = await testDb(); const row = await seedArtifact(db, { tenantId: "other" }); @@ -118,68 +122,6 @@ describe("artifact_read", () => { }); }); -describe("web_site reads", () => { - const site = JSON.stringify({ - entry: "index.html", - files: { "index.html": "

Hi

", "style.css": "body{}" }, - }); - - test("an unpinned read returns the structure, not the bundle", async () => { - const db = await testDb(); - const row = await seedArtifact(db, { kind: "web_site", content: site }); - - const result = await readArtifact(db, { scope: SCOPE, artifactId: row.id }); - expect(result).toMatchObject({ - summary: { - kind: "web_site", - entry: "index.html", - files: [ - { path: "index.html", bytes: 11 }, - { path: "style.css", bytes: 6 }, - ], - totalBytes: 17, - }, - }); - expect("content" in result).toBe(false); - }); - - test("a path read returns that one file, normalizing the path", async () => { - const db = await testDb(); - const row = await seedArtifact(db, { kind: "web_site", content: site }); - - const result = await readArtifact(db, { - scope: SCOPE, - artifactId: row.id, - path: "/style.css", - }); - expect(result).toMatchObject({ path: "style.css", content: "body{}" }); - }); - - test("a path outside the bundle is an error, and traversal is refused", async () => { - const db = await testDb(); - const row = await seedArtifact(db, { kind: "web_site", content: site }); - - await expect( - readArtifact(db, { scope: SCOPE, artifactId: row.id, path: "nope.js" }), - ).rejects.toThrow(/File not found in web_site artifact/); - await expect( - readArtifact(db, { - scope: SCOPE, - artifactId: row.id, - path: "../secret", - }), - ).rejects.toThrow(/traversal/); - }); - - test("chunked reads are refused for web_site with a pointer to the right tool", async () => { - const db = await testDb(); - const row = await seedArtifact(db, { kind: "web_site", content: site }); - await expect( - readArtifactChunk(db, { scope: SCOPE, artifactId: row.id }), - ).rejects.toThrow(/use artifact_read/); - }); -}); - describe("artifact_read_chunk", () => { test("honors an explicit offset and limit", async () => { const db = await testDb(); @@ -313,12 +255,6 @@ describe("artifact_link_file", () => { expect(rows[0]!.n).toBe("0"); }); - test("it cannot be used to mint a skill-draft", async () => { - const db = await testDb(); - await expect( - linkFileArtifact(db, linkArgs({ kind: "skill-draft" })), - ).rejects.toThrow(); - }); test("a linked artifact is readable through artifact_read", async () => { const db = await testDb(); diff --git a/src/tools.ts b/src/tools.ts index 5e7e430..d043555 100644 --- a/src/tools.ts +++ b/src/tools.ts @@ -4,17 +4,9 @@ import { ArtifactNotFoundError, createArtifact, getArtifactVersion, - SKILL_DRAFT_KIND, } from "./artifacts.js"; import { artifact, type ArtifactRow } from "./schema.js"; import type { ResolvedPrincipal } from "./ports.js"; -import { - parseWebSiteContentJson, - normalizeWebSitePath, - summarizeWebSiteContent, - WEB_SITE_KIND, - type WebSiteReadSummary, -} from "./web-site.js"; /** * An agent runtime caps a tool result at ~10K characters and spills the rest to @@ -35,7 +27,6 @@ export type ArtifactReadResult = { chunkStart?: number; chunkEnd?: number; continuation?: string; - path?: string; }; const encodedLength = (value: unknown) => JSON.stringify(value, null, 2).length; @@ -68,7 +59,7 @@ function chunk( * small enough and no window was asked for; otherwise a chunk shrunk (by the * measured overshoot ratio, so it converges fast) until it encodes small enough. */ -export function windowContent( +function windowContent( base: ReadBase, content: string, offset?: number, @@ -100,8 +91,7 @@ export function windowContent( /** * Resolve an artifact for an agent read, honoring a version pin. Reads are - * always confined to the caller's tenant; there is no tenant override. A - * skill-draft reads as NOT FOUND, not forbidden. + * always confined to the caller's tenant; there is no tenant override. */ async function resolveForRead( db: ArtifactDb, @@ -118,7 +108,7 @@ async function resolveForRead( and(eq(artifact.id, args.artifactId), eq(artifact.tenantId, args.scope.tenantId)), ) .limit(1); - if (!row || row.kind === SKILL_DRAFT_KIND) { + if (!row) { throw new ArtifactNotFoundError(args.artifactId); } @@ -151,35 +141,20 @@ async function resolveForRead( }; } -/** - * `artifact_read`: whole (budgeted) content, or — for `web_site` — a structural - * summary, or one file's content when `path` is given. Reading the raw JSON - * bundle of a site is never useful to a model and always blows the budget. - */ +/** `artifact_read`: whole (budgeted) content. */ export async function readArtifact( db: ArtifactDb, args: { scope: ResolvedPrincipal; artifactId: string; version?: number; - path?: string; }, -): Promise { +): Promise { const { base, content } = await resolveForRead(db, args); - if (base.kind !== WEB_SITE_KIND) return windowContent(base, content); - - if (args.path === undefined) { - return { ...base, summary: summarizeWebSiteContent(content) }; - } - const path = normalizeWebSitePath(args.path); - const file = parseWebSiteContentJson(content).files[path]; - if (file === undefined) { - throw new Error(`File not found in web_site artifact: ${path}`); - } - return { ...windowContent(base, file), path }; + return windowContent(base, content); } -/** `artifact_read_chunk`: one bounded character range. Not for `web_site`. */ +/** `artifact_read_chunk`: one bounded character range. */ export async function readArtifactChunk( db: ArtifactDb, args: { @@ -191,11 +166,6 @@ export async function readArtifactChunk( }, ): Promise { const { base, content } = await resolveForRead(db, args); - if (base.kind === WEB_SITE_KIND) { - throw new Error( - "artifact_read_chunk does not support web_site artifacts; use artifact_read for a summary or pass path to read one file", - ); - } return windowContent( base, content, @@ -219,8 +189,7 @@ export async function readArtifactChunk( * the file itself and calls `createFileArtifact` instead. * * Like every other create path: the artifact and its version 1 land in one - * transaction, `web_site` content is normalized, and skill-draft is refused by - * `createArtifact`. + * transaction through `createArtifact`. */ export async function linkFileArtifact( db: ArtifactDb, @@ -286,7 +255,7 @@ export const ARTIFACT_TOOL_DEFINITIONS: readonly ArtifactToolDefinition[] = [ kind: { type: "string", description: - "Artifact kind, such as document, email, memo, note, or web_site for a multi-file static site stored as JSON { entry?, files: { path: content } }.", + "Artifact kind, such as document, email, memo, or note.", }, content: { type: "string", description: "The full text content." }, metadata: { @@ -333,11 +302,6 @@ export const ARTIFACT_TOOL_DEFINITIONS: readonly ArtifactToolDefinition[] = [ type: "number", description: "Optional version to read. Defaults to the latest.", }, - path: { - type: "string", - description: - "For kind=web_site only: return one file's content at this relative path. Without path, web_site reads return a summary.", - }, }, required: ["artifactId"], }, @@ -346,7 +310,7 @@ export const ARTIFACT_TOOL_DEFINITIONS: readonly ArtifactToolDefinition[] = [ name: "artifact_read_chunk", sideEffect: "read", description: - "Read one bounded chunk of an artifact's content by character range. Pass the offset named in the prior result's 'continuation' field, and keep going until a result has no 'continuation'. Not supported for kind=web_site.", + "Read one bounded chunk of an artifact's content by character range. Pass the offset named in the prior result's 'continuation' field, and keep going until a result has no 'continuation'.", inputSchema: { type: "object", properties: { diff --git a/src/web-site.test.ts b/src/web-site.test.ts deleted file mode 100644 index 88b0a52..0000000 --- a/src/web-site.test.ts +++ /dev/null @@ -1,114 +0,0 @@ -import { describe, expect, test } from "bun:test"; -import { - normalizeWebSiteContent, - normalizeWebSitePath, - parseWebSiteContentJson, - serializeWebSiteContent, - summarizeWebSiteContent, - WEB_SITE_MAX_FILES, - WEB_SITE_MAX_PATH_LENGTH, - WEB_SITE_MAX_TOTAL_BYTES, - WebSiteContentError, -} from "./web-site.js"; - -describe("path normalization", () => { - test("strips leading slashes and converts backslashes", () => { - expect(normalizeWebSitePath("/assets\\app.css")).toBe("assets/app.css"); - expect(normalizeWebSitePath(" index.html ")).toBe("index.html"); - }); - - test("refuses traversal, empty segments, and an empty path", () => { - for (const bad of ["../secret", "a/../../b", "a//b", " ", "/"]) { - expect(() => normalizeWebSitePath(bad)).toThrow(WebSiteContentError); - } - }); -}); - -describe("content validation", () => { - test("defaults the entry to index.html and normalizes every path", () => { - expect( - normalizeWebSiteContent({ files: { "/index.html": "

", "a\\b.css": "x" } }), - ).toEqual({ entry: "index.html", files: { "index.html": "

", "a/b.css": "x" } }); - }); - - test("requires the entry file to exist in the bundle", () => { - expect(() => - normalizeWebSiteContent({ entry: "main.html", files: { "index.html": "

" } }), - ).toThrow(/entry file "main.html" is not present/); - }); - - test("refuses an empty bundle and two paths that normalize to one", () => { - expect(() => normalizeWebSiteContent({ files: {} })).toThrow(/at least one file/); - expect(() => - normalizeWebSiteContent({ files: { "index.html": "a", "/index.html": "b" } }), - ).toThrow(/duplicate path after normalization/); - }); - - test("enforces the file-count ceiling", () => { - const files: Record = { "index.html": "

" }; - for (let i = 0; i < WEB_SITE_MAX_FILES; i += 1) files[`f${i}.css`] = "x"; - expect(() => normalizeWebSiteContent({ files })).toThrow(/max file count/); - }); - - test("enforces the total-size ceiling", () => { - expect(() => - normalizeWebSiteContent({ files: { "index.html": "x".repeat(4_500_001) } }), - ).toThrow(/total size exceeds/); - }); - - test("refuses a path longer than WEB_SITE_MAX_PATH_LENGTH", () => { - const longName = "a".repeat(WEB_SITE_MAX_PATH_LENGTH + 1); - expect(() => normalizeWebSitePath(longName)).toThrow(WebSiteContentError); - expect(() => normalizeWebSitePath(longName)).toThrow(/path exceeds max length/); - // Empty body must not let an over-long path slip through. - expect(() => - normalizeWebSiteContent({ files: { [longName]: "" } }), - ).toThrow(/path exceeds max length/); - }); - - test("counts path bytes toward the total-size ceiling (empty-body paths cannot bypass)", () => { - // Body alone is under the ceiling; body + path UTF-8 exceeds it. - // Without counting path bytes, this payload would be accepted. - const path = "p".repeat(200); - const body = "x".repeat(WEB_SITE_MAX_TOTAL_BYTES - 100); - expect(() => - normalizeWebSiteContent({ files: { [path]: body } }), - ).toThrow(/total size exceeds/); - }); - - test("rejects invalid JSON and a payload of the wrong shape", () => { - expect(() => parseWebSiteContentJson("{oops")).toThrow(WebSiteContentError); - expect(() => parseWebSiteContentJson("{oops")).toThrow(/must be a JSON string/); - expect(() => parseWebSiteContentJson('{"files":"nope"}')).toThrow(/content invalid/); - }); - - test("serialize then parse is a fixed point", () => { - const raw = serializeWebSiteContent({ files: { "/index.html": "

" } }); - expect(parseWebSiteContentJson(raw)).toEqual({ - entry: "index.html", - files: { "index.html": "

" }, - }); - }); -}); - -describe("summary", () => { - test("reports each file's byte length, sorted, with the total", () => { - const raw = serializeWebSiteContent({ - files: { "index.html": "

Hi

", "a.css": "body{}" }, - }); - expect(summarizeWebSiteContent(raw)).toEqual({ - kind: "web_site", - entry: "index.html", - files: [ - { path: "a.css", bytes: 6 }, - { path: "index.html", bytes: 11 }, - ], - totalBytes: 17, - }); - }); - - test("counts bytes, not characters, for multi-byte content", () => { - const raw = serializeWebSiteContent({ files: { "index.html": "é" } }); - expect(summarizeWebSiteContent(raw).totalBytes).toBe(2); - }); -}); diff --git a/src/web-site.ts b/src/web-site.ts deleted file mode 100644 index 92881bd..0000000 --- a/src/web-site.ts +++ /dev/null @@ -1,129 +0,0 @@ -import "./arktype.js"; -import { type } from "arktype"; - -/** A multi-file static site stored as one artifact whose content is JSON. */ -export const WEB_SITE_KIND = "web_site"; - -export const WEB_SITE_MAX_FILES = 64; -export const WEB_SITE_MAX_TOTAL_BYTES = 4_500_000; -/** Max length (UTF-16 code units) of a normalized web_site file path. */ -export const WEB_SITE_MAX_PATH_LENGTH = 1024; - -export const WebSiteContentSchema = type({ - "entry?": "string", - files: "Record", -}); -export type WebSiteContent = typeof WebSiteContentSchema.infer; - -export class WebSiteContentError extends Error { - constructor(message: string) { - super(message); - this.name = "WebSiteContentError"; - } -} - -/** Strip leading slashes, normalize separators, and reject traversal. */ -export function normalizeWebSitePath(path: string): string { - const normalized = path.trim().replace(/\\/g, "/").replace(/^\/+/, ""); - if (normalized.length === 0) { - throw new WebSiteContentError("file path must not be empty"); - } - for (const segment of normalized.split("/")) { - if (segment === "..") { - throw new WebSiteContentError(`invalid path (traversal): ${path}`); - } - // "." segments survive as aliases ("./index.html" vs "index.html") that - // the duplicate check misses and static servers collapse at serve time. - if (segment === ".") { - throw new WebSiteContentError(`invalid path ("." segment): ${path}`); - } - if (segment.length === 0) { - throw new WebSiteContentError(`invalid path (empty segment): ${path}`); - } - } - if (normalized.length > WEB_SITE_MAX_PATH_LENGTH) { - throw new WebSiteContentError( - `path exceeds max length (${WEB_SITE_MAX_PATH_LENGTH}): ${path.length} characters`, - ); - } - return normalized; -} - -const byteLength = (text: string) => new TextEncoder().encode(text).byteLength; - -/** Normalize every path, default the entry, and enforce the size ceilings. */ -export function normalizeWebSiteContent( - content: WebSiteContent, -): Required { - const files: Record = {}; - let total = 0; - for (const [rawPath, fileContent] of Object.entries(content.files)) { - const path = normalizeWebSitePath(rawPath); - if (path in files) { - throw new WebSiteContentError( - `duplicate path after normalization: ${path}`, - ); - } - files[path] = fileContent; - // Path UTF-8 counts toward the total so empty-body huge keys cannot bypass. - total += byteLength(path) + byteLength(fileContent); - } - - const paths = Object.keys(files); - if (paths.length === 0) { - throw new WebSiteContentError("web_site must include at least one file"); - } - if (paths.length > WEB_SITE_MAX_FILES) { - throw new WebSiteContentError( - `web_site exceeds max file count (${WEB_SITE_MAX_FILES})`, - ); - } - if (total > WEB_SITE_MAX_TOTAL_BYTES) { - throw new WebSiteContentError( - `web_site total size exceeds ${WEB_SITE_MAX_TOTAL_BYTES} bytes`, - ); - } - - const entry = normalizeWebSitePath(content.entry ?? "index.html"); - if (!(entry in files)) { - throw new WebSiteContentError(`entry file "${entry}" is not present in files`); - } - return { entry, files }; -} - -const ParsedWebSiteContent = type("string.json.parse") - .to(WebSiteContentSchema) - .pipe(normalizeWebSiteContent); - -export function parseWebSiteContentJson(raw: string): Required { - const result = ParsedWebSiteContent(raw); - if (result instanceof type.errors) { - throw new WebSiteContentError(`web_site content invalid: ${result.summary}`); - } - return result; -} - -export function serializeWebSiteContent(content: WebSiteContent): string { - return JSON.stringify(normalizeWebSiteContent(content)); -} - -export type WebSiteReadSummary = { - kind: typeof WEB_SITE_KIND; - entry: string; - files: { path: string; bytes: number }[]; - totalBytes: number; -}; - -/** What an agent gets from an unpinned `web_site` read: shape, not payload. */ -export function summarizeWebSiteContent(rawJson: string): WebSiteReadSummary { - const content = parseWebSiteContentJson(rawJson); - const files = Object.entries(content.files) - .map(([path, text]) => ({ path, bytes: byteLength(text) })) - .sort((a, b) => a.path.localeCompare(b.path)); - return { - kind: WEB_SITE_KIND, - entry: content.entry, - files, - totalBytes: files.reduce((sum, f) => sum + f.bytes, 0), - }; -} diff --git a/src/workflow-mount.test.ts b/src/workflow-mount.test.ts index b625471..f9a596c 100644 --- a/src/workflow-mount.test.ts +++ b/src/workflow-mount.test.ts @@ -7,7 +7,7 @@ import { } from "./workflow-mount.js"; import { InlineContentStore } from "./content-store.js"; import { getArtifact } from "./artifacts.js"; -import { seedArtifact, seedSkillDraft, testDb } from "./test-helpers.js"; +import { seedArtifact, testDb } from "./test-helpers.js"; import type { ArtifactDb } from "./db.js"; const RUN_SCOPE: ResolvedWorkflowRunScope = { @@ -209,15 +209,6 @@ describe("GET /artifacts/:id", () => { expect(foreignRes.status).toBe(404); expect(ghostRes.status).toBe(404); }); - - test("404s a skill-draft row", async () => { - const db = await testDb(); - const draftId = await seedSkillDraft(db, "scratch"); - const app = host(db); - - const res = await app.request(`/artifacts/${draftId}`, { headers: authed }); - expect(res.status).toBe(404); - }); }); describe("POST /artifacts/binary", () => { diff --git a/src/workflow-mount.ts b/src/workflow-mount.ts index 2ebb13c..13f7a32 100644 --- a/src/workflow-mount.ts +++ b/src/workflow-mount.ts @@ -27,7 +27,6 @@ import { MetadataShape, serializeArtifact, serializeArtifactListItem, - SKILL_DRAFT_KIND, writeArtifactVersion, type SerializedArtifact, type SerializedArtifactListItem, @@ -431,7 +430,7 @@ export function mountWorkflowArtifacts( const scope = c.get("workflowRunScope"); const artifactId = c.req.param("id"); const existing = await getArtifact(db, artifactId); - if (existing === null || existing.tenantId !== scope.tenantId || existing.kind === SKILL_DRAFT_KIND) { + if (existing === null || existing.tenantId !== scope.tenantId) { return c.json({ error: "Artifact not found" }, 404); } const written = await writeArtifactVersion(db, { @@ -450,13 +449,11 @@ export function mountWorkflowArtifacts( app.get("/artifacts/:id/read", async (c) => { const scope = c.get("workflowRunScope"); - const path = c.req.query("path"); try { const data = await readArtifact(db, { scope: { tenantId: scope.tenantId, principalId: scope.principalId }, artifactId: c.req.param("id"), ...parseVersionQuery(c.req.query("version")), - ...(path !== undefined && path !== "" ? { path } : {}), }); return c.json({ data }); } catch (err) { @@ -485,9 +482,9 @@ export function mountWorkflowArtifacts( const artifactId = c.req.param("id"); const row = await getArtifact(db, artifactId); // Fetch-then-check, exactly mirroring the tenant routes' single-artifact - // handlers: an id from another tenant, or a skill-draft, reads back + // handlers: an id from another tenant reads back // identically to an id that never existed — never a distinguishable 403. - if (row === null || row.tenantId !== scope.tenantId || row.kind === SKILL_DRAFT_KIND) { + if (row === null || row.tenantId !== scope.tenantId) { return c.json({ error: "Artifact not found" }, 404); } const artifact: SerializedArtifact = serializeArtifact(row); diff --git a/tests/lib/db-harness.ts b/tests/lib/db-harness.ts index a151b9d..86b5d9d 100644 --- a/tests/lib/db-harness.ts +++ b/tests/lib/db-harness.ts @@ -52,8 +52,14 @@ async function admin(run: (sql: postgres.Sql) => Promise): Promise { } } -/** Creates `artifact__test`, migrates it, and drops it on `close`. */ -export async function createTestDb(): Promise { +/** + * Creates `artifact__test`, applies Interchange's migrations and then + * `migrateArtifacts` (this package's by default), and drops it on `close`. + */ +export async function createTestDb( + migrateArtifacts: (config: DBConfig) => Promise = (config) => + runArtifactMigrations(config, { schema: "public" }), +): Promise { assertDestructiveArtifactTestsAllowed(DATABASE_URL); const name = `artifact_${randomUUID().replaceAll("-", "").slice(0, 12)}_test`; await admin((sql) => sql.unsafe(`CREATE DATABASE "${name}"`)); @@ -66,7 +72,7 @@ export async function createTestDb(): Promise { database: name, }; await runMigrations(config, { schema: "public" }); - await runArtifactMigrations(config, { schema: "public" }); + await migrateArtifacts(config); const handle = createDB(config); return { db: handle.db, diff --git a/tests/migrations.test.ts b/tests/migrations.test.ts index 326155e..123bf9e 100644 --- a/tests/migrations.test.ts +++ b/tests/migrations.test.ts @@ -3,7 +3,7 @@ import { sql } from "drizzle-orm"; import { createArtifactDb, runArtifactMigrations } from "../src/index.js"; import { connectionString, createTestDb, seedActor, type TestDb } from "./lib/db-harness.js"; -const TABLES = ["artifact", "artifact_version", "mail_attachment_ref", "upload"]; +const TABLES = ["artifact", "artifact_version", "upload"]; let testDb: TestDb; diff --git a/tests/reference-host.test.ts b/tests/reference-host.test.ts index 22cb85c..d1eb1fb 100644 --- a/tests/reference-host.test.ts +++ b/tests/reference-host.test.ts @@ -172,25 +172,6 @@ describe.each<[string, ContentStore]>([ expect(attached.headers.get("content-disposition")).toStartWith("attachment;"); expect(inline.headers.get("content-disposition")).toStartWith("inline;"); }); - - test("mail_attachment_ref is an idempotent artifact↔message association", async () => { - const pdfId = uploaded[1]!.id; - const body = { - mailId: `mail-${name}`, - attachments: [ - { artifactId: pdfId, name: "deck.pdf", type: "application/pdf", size: PDF.length }, - ], - }; - // Posted twice: the file already IS an artifact, so no bytes move and the - // second post must record nothing new. - for (let i = 0; i < 2; i += 1) { - await app.request(`/api/instances/inst-${name}/mail-attachments`, postJson(body)); - } - const refs = await json<{ refs: { artifactId: string }[] }>( - await app.request(`/api/instances/inst-${name}/mail-attachments`), - ); - expect(refs.refs.map((r) => r.artifactId)).toEqual([pdfId]); - }); }); // Only DataUrlContentStore keeps its bytes IN `content`, so it is the store @@ -435,12 +416,6 @@ describe("no session", () => { expect((await json<{ artifacts: unknown[] }>(res)).artifacts).toEqual([]); }); - test("the mail-attachment collection read is also an empty 200, not a 403", async () => { - const res = await host.request("/api/instances/inst-InlineContentStore/mail-attachments"); - expect(res.status).toBe(200); - expect((await json<{ refs: unknown[] }>(res)).refs).toEqual([]); - }); - test("creating is refused 403", async () => { const res = await host.request( "/api/artifacts", @@ -516,32 +491,6 @@ describe("a cross-tenant request fails closed", () => { } }); - test("a mail-attachment reference to another tenant's artifact is refused", async () => { - const [foreign] = await host.db.execute<{ id: string }>(sql` - SELECT "id" FROM "artifacts"."artifact" WHERE "title" = 'Other tenant secret' LIMIT 1 - `); - const res = await host.request( - "/api/instances/inst-cross/mail-attachments", - postJson({ - mailId: "mail-cross", - attachments: [ - { - artifactId: foreign!.id, - name: "secret.pdf", - type: "application/pdf", - size: 1, - }, - ], - }), - ); - expect(res.status).toBe(404); - - const refs = await json<{ refs: unknown[] }>( - await host.request("/api/instances/inst-cross/mail-attachments"), - ); - expect(refs.refs).toEqual([]); - }); - // The archived artifact left behind by the archive scenarios above is // restored there; nothing here mutates, so no cleanup is needed. }); @@ -605,7 +554,7 @@ describe("pdf parsing is the host's, and the module's contract with it holds", ( filename: "report.pdf", mimeType: "application/pdf", bytes: PDF, - // The chat/mail attachment surface, whose route the HOST owns — and + // The chat attachment surface, whose route the HOST owns — and // which is still gated by this module's allowlist, because `policy` is // a required argument. policy: PARSED_DOCUMENT_POLICY, @@ -641,43 +590,6 @@ describe("pdf parsing is the host's, and the module's contract with it holds", ( }); }); -describe("a skill-draft is invisible over the mounted host", () => { - // End to end, on a real host rather than a unit-test Hono app: the - // kind is not addressable by ANY single-artifact route. - test("every detail route answers 404", async () => { - const draftAuthor = host.agentPrincipal; - const [draft] = await host.db.execute<{ id: string }>(sql` - INSERT INTO "artifacts"."artifact" ("tenant_id", "principal_id", "owner_principal_id", - "kind", "title", "content", "source", "version") - VALUES (${host.tenantId}, ${draftAuthor}, ${draftAuthor}, 'skill-draft', 'Scratch', - 'draft body', '{"origin":"agent"}'::jsonb, 1) - RETURNING "id" - `); - const id = draft!.id; - const routes: [string, RequestInit][] = [ - [`/api/artifacts/${id}`, {}], - [`/api/artifacts/${id}/versions`, {}], - [`/api/artifacts/${id}/versions`, postJson({ content: "x" })], - [`/api/artifacts/${id}/versions/1`, {}], - [`/api/artifacts/${id}/archive`, { method: "POST" }], - [`/api/artifacts/${id}/unarchive`, { method: "POST" }], - [`/api/artifacts/${id}/download`, {}], - ]; - for (const [path, init] of routes) { - const res = await host.request(path, init); - expect({ route: `${init.method ?? "GET"} ${path}`, status: res.status }).toEqual({ - route: `${init.method ?? "GET"} ${path}`, - status: 404, - }); - } - - const listed = await json<{ artifacts: { id: string }[] }>( - await host.request("/api/artifacts?limit=100"), - ); - expect(listed.artifacts.some((a) => a.id === id)).toBe(false); - }); -}); - describe("the migration runner is re-runnable", () => { test("re-running destroys no data", async () => { await runArtifactMigrations(host.config, { schema: "public" }); diff --git a/tests/upgrade-from-0.1.0.test.ts b/tests/upgrade-from-0.1.0.test.ts new file mode 100644 index 0000000..dd52376 --- /dev/null +++ b/tests/upgrade-from-0.1.0.test.ts @@ -0,0 +1,89 @@ +// A database migrated and written by the published 0.1.0 package upgrades in +// place under this version's runArtifactMigrations. +import { afterAll, beforeAll, describe, expect, test } from "bun:test"; +import { sql } from "drizzle-orm"; +import * as v010 from "@corbits/artifacts-0.1.0"; +import { runArtifactMigrations } from "../src/index.js"; +import { + artifactApp, + connectionString, + createTestDb, + grant, + seedActor, + type Actor, + type TestDb, +} from "./lib/db-harness.js"; + +const PDF = new Uint8Array([0x25, 0x50, 0x44, 0x46, 0x2d, 0x31, 0x2e, 0x37, 0x00, 0xff]); + +let testDb: TestDb; +let actor: Actor; +let fileId: string; + +beforeAll(async () => { + testDb = await createTestDb(async (config) => { + const legacy = v010.createArtifactDb(connectionString(config)); + try { + await v010.runArtifactMigrations(legacy.db); + } finally { + await legacy.close(); + } + }); + actor = await seedActor(testDb.db, "acme"); + await grant(testDb.db, actor, "artifact:*", "write"); + const scope = { tenantId: actor.tenant.id, principalId: actor.principal.id }; + + const legacy = v010.createArtifactDb(connectionString(testDb.config)); + try { + const file = await legacy.db.transaction((tx) => + v010.createFileArtifact(tx, v010.InlineContentStore, { + scope, + ownerPrincipalId: scope.principalId, + filename: "deck.pdf", + mimeType: "application/pdf", + bytes: PDF, + policy: v010.ARTIFACT_UPLOAD_POLICY, + }), + ); + fileId = file.id; + await v010.saveMailAttachmentRefs(legacy.db, { + scope, + instanceId: "inst-1", + body: { + mailId: "mail-1", + attachments: [ + { artifactId: file.id, name: "deck.pdf", type: "application/pdf", size: PDF.length }, + ], + }, + }); + } finally { + await legacy.close(); + } + + await runArtifactMigrations(testDb.config, { schema: "public" }); +}); + +afterAll(async () => { + await testDb?.close(); +}); + +describe("upgrading a 0.1.0 database", () => { + test("drops mail_attachment_ref and the 0.1.0 migration ledger", async () => { + const rows = await testDb.db.execute<{ table_name: string }>(sql` + SELECT table_name FROM information_schema.tables + WHERE table_schema = 'artifacts' + ORDER BY table_name + `); + expect(rows.map((row) => row.table_name)).toEqual([ + "artifact", + "artifact_version", + "upload", + ]); + }); + + test("a 0.1.0 upload still downloads its bytes", async () => { + const res = await artifactApp(testDb.db, actor).request(`/api/artifacts/${fileId}/download`); + expect(res.status).toBe(200); + expect(new Uint8Array(await res.arrayBuffer())).toEqual(PDF); + }); +});