Merge pull request #3 from corbitsdev/fix/artifacts-guardrails-correctness

fix(artifacts): guardrails and correctness (CL-4686)

Author: TheGreatAxios

.github/workflows/test.yml

@@ -24,6 +24,9 @@ jobs:
 
     env:
       ARTIFACT_DATABASE_URL: postgres://postgres:postgres@localhost:5457/artifact_core
+      # Package suite TRUNCATEs tables / some migration tests DROP SCHEMA — both
+      # refuse unless opted in against an allowlisted ephemeral database name.
+      ALLOW_DESTRUCTIVE_ARTIFACT_TESTS: "1"
 
     steps:
       - uses: actions/checkout@v4
@@ -38,14 +41,17 @@ jobs:
       - name: check-deps
         run: bun run --cwd packages/artifacts scripts/check-deps.ts
 
+      # typecheck builds first: examples import @corbits/artifacts through the
+      # published dist types, same path a consumer resolves.
       - name: typecheck
         run: bun run typecheck
 
       - name: unit + integration tests
         run: bun run --cwd packages/artifacts test:coverage
 
-      # Emits dist/, which is also what the reference host resolves through —
-      # so this proves the published artifact, not just the sources.
+      # dist/ is already emitted by typecheck; re-run for a clean package build
+      # so acceptance and the node consumer smoke test do not depend on typecheck
+      # having side-effected the tree.
       - name: build
         run: bun run build
 

.gitignore

@@ -9,3 +9,8 @@ coverage/
 # npm pack output
 *.tgz
 .worktrees/
+
+# Dispatch orchestration artifacts (local agent runs)
+dispatch/
+.agent-state/
+.intercode/

ARCHITECTURE.md

@@ -123,15 +123,30 @@ which store is installed.
 Four physical tables — `artifact`, `artifact_version`, `upload`,
 `mail_attachment_ref` — plus this package's own migration ledger.
 
-**Hard control-plane foreign keys, by design.** `tenant_id` references
-`public.tenant(id)` (`ON DELETE CASCADE` — a deleted tenant takes its artifacts
-with it) and `principal_id` / `owner_principal_id` reference
+**Hard control-plane foreign keys, by design.** `tenant_id` is `NOT NULL` and
+references `public.tenant(id)` (`ON DELETE CASCADE` — a deleted tenant takes its
+artifacts with it) and `principal_id` / `owner_principal_id` reference
 `public.principal(id)` (`ON DELETE SET NULL` — a removed principal detaches its
 artifacts rather than destroying them). This package is coupled to Interchange:
 it mounts on Interchange-shaped hosts only, and the host's own migrations must
 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
+single-column constraints applied by a ledgered migration — free at write time.
+
+**Principal↔tenant alignment is host-owned.** The package FKs each column into
+the control plane independently; it does **not** enforce that `principal_id` (or
+`owner_principal_id`) belongs to the same tenant as `tenant_id`. A multi-table
+trigger or composite FK into `public.principal` would couple every write to a
+control-plane lookup and is deliberately out of scope. The host's
+`resolvePrincipal` is the authority: it returns the `(tenantId, principalId)`
+pair every route and tool write stamps, so a correctly mounted host never
+plants a cross-tenant principal. Operators cleaning legacy rows before the
+`tenant_id NOT NULL` migration must assign a valid tenant or delete orphans —
+the migration fails with an explicit message if null `tenant_id` rows remain.
+
 **`kind` is free-form text, not a pg enum,** validated at the application edge.
 New kinds cost no migration. What is *not* free-form is the import allowlist:
 `POST /api/artifacts` may only mint `link` or `document`, so an untrusted caller
@@ -186,6 +201,22 @@ every boot of every replica.
   fresh databases diverge silently. Ship a new migration instead. The column is
   `NOT NULL`, so the guarantee is unconditional: there is no unrecorded row for
   the runner to adopt and wave through.
+- Event timestamps (`created_at`, `updated_at`, `archived_at`) are
+  **`timestamptz`**. The initial create migration still lays them down as
+  zoneless `timestamp`; a follow-on migration retypes them with
+  `USING col AT TIME ZONE 'UTC'`, treating existing walls as the UTC clocks the
+  package always assumed. List keyset cursors project through
+  `AT TIME ZONE 'UTC'` and compare with `::timestamptz`, so paging and date
+  filters stay on the absolute instant under any session `TimeZone`. Rollback is
+  the reverse cast (`TYPE timestamp USING col AT TIME ZONE 'UTC'`) plus a new
+  ledgered migration — never edit a shipped one.
+- A later ledgered migration sets `artifact.tenant_id NOT NULL` and adds the
+  version/size CHECKs. If null-tenant rows still exist, that migration raises
+  before altering the column so the operator can clean them up first.
+- Empty ledger + pre-existing package objects fails closed
+  (`MigrationAdoptError`). `{ adopt: true }` records checksums without re-DDL
+  only after shape validation: tables, column types, required nullability, and
+  the named CHECK constraints. Column presence alone is not enough.
 
 **The package owns its own Postgres schema.** Every table, index and the ledger
 live in `artifacts`, created by the runner and qualified in every

CONTRIBUTING.md

@@ -14,6 +14,13 @@ Nothing is mocked at the database boundary. Migrations, indexes, keyset cursors,
 version-bump race and tenant scoping are asserted against a live server, because that is
 the only place they are true.
 
+The harness truncates package tables between tests and drops the package schema in a
+couple of migration cases. Those paths are fail-closed: set
+`ALLOW_DESTRUCTIVE_ARTIFACT_TESTS=1` and point `ARTIFACT_DATABASE_URL` at an allowlisted
+ephemeral database (`artifact_core`, or any name ending in `_test`). Without both, the
+suite throws before mutating. The gate itself is pure URL/env parsing and is covered by
+unit tests that do not need Postgres.
+
 ## The reference host is the acceptance suite, not a demo
 
 `examples/reference-host` mounts the package on a real `@intx/hub-api` app against a

README.md

@@ -22,6 +22,10 @@ bun install
 docker run -d --name corbits-artifact-pg -p 5457:5432 \
   -e POSTGRES_PASSWORD=postgres -e POSTGRES_DB=artifact_core postgres:16
 
+# The package suite TRUNCATEs tables and some migration tests DROP SCHEMA.
+# Both refuse unless you opt in and the database name is allowlisted.
+export ALLOW_DESTRUCTIVE_ARTIFACT_TESTS=1
+
 bun run test:package     # dependency check, then unit + integration
 bun run build            # dist/ (JS + .d.ts)
 bun run test:acceptance  # builds, then the acceptance scenarios
@@ -34,7 +38,10 @@ run stops meaning anything.
 
 Tests and the example expect
 `postgres://postgres:postgres@localhost:5457/artifact_core`; override with
-`ARTIFACT_DATABASE_URL`.
+`ARTIFACT_DATABASE_URL`. Destructive package tests additionally require
+`ALLOW_DESTRUCTIVE_ARTIFACT_TESTS=1` and an allowlisted database name
+(`artifact_core`, or any name ending in `_test`). See the package README Development
+section for the full gate contract.
 
 ## Conventions
 

bun.lock

@@ -18,7 +18,7 @@
     "@intx/hub-sessions": "0.2.2",
     "@intx/types": "0.2.2",
     "arktype": "2.1.29",
-    "drizzle-orm": "0.45.1",
+    "drizzle-orm": "0.45.2",
     "hono": "4.12.32",
     "hono-openapi": "1.2.0",
     "postgres": "3.4.9"

examples/reference-host/package.json

@@ -34,7 +34,7 @@ import {
   type ResolvedPrincipal,
   type ContentStore,
   type Identity,
-  type SerializedArtifact,
+  type SerializedArtifactBase,
 } from "@corbits/artifacts";
 
 export const DATABASE_URL =
@@ -130,7 +130,7 @@ function createIdentity(db: ArtifactDb): Identity {
 }
 
 /** Display-only decorator. Adds a label, never changes what is returned. */
-async function decorate(_tenantId: string, rows: SerializedArtifact[]) {
+async function decorate(_tenantId: string, rows: readonly SerializedArtifactBase[]) {
   for (const row of rows) {
     (row as Record<string, unknown>).generatedByLabel =
       typeof row.source.generatedBy === "string" ? row.source.generatedBy : null;
@@ -371,7 +371,7 @@ export async function createReferenceHost(): Promise<ReferenceHost> {
 
   return {
     db,
-    tenant,
+    tenantId: tenant,
     agentPrincipal,
     scope: () => ({
       tenantId: tenant,

examples/reference-host/src/index.ts

@@ -197,7 +197,7 @@ describe("list: keyset paging, creatorKind, and the archived toggle", () => {
     await host.db.execute(sql`
       INSERT INTO "artifacts"."artifact" ("tenant_id", "principal_id", "owner_principal_id",
         "kind", "title", "content", "source", "version")
-      VALUES (${host.tenant}, ${host.agentPrincipal}, ${host.agentPrincipal}, 'document',
+      VALUES (${host.tenantId}, ${host.agentPrincipal}, ${host.agentPrincipal}, 'document',
         'Agent memo', 'written by an agent', '{"origin":"agent"}'::jsonb, 1)
     `);
 
@@ -455,7 +455,7 @@ describe("pdf parsing is the host's, and the module's contract with it holds", (
     const row = await host.db.transaction((tx) =>
       createFileArtifact(tx, InlineContentStore, {
         scope: host.scope(),
-        ownerPrincipalId: host.scope().principal,
+        ownerPrincipalId: host.scope().principalId,
         filename: "report.pdf",
         mimeType: "application/pdf",
         bytes: PDF,
@@ -484,7 +484,7 @@ describe("pdf parsing is the host's, and the module's contract with it holds", (
       host.db.transaction((tx) =>
         createFileArtifact(tx, InlineContentStore, {
           scope: host.scope(),
-          ownerPrincipalId: host.scope().principal,
+          ownerPrincipalId: host.scope().principalId,
           filename: "logo.svg",
           mimeType: "image/svg+xml",
           bytes: new Uint8Array(Buffer.from("<svg/>")),
@@ -503,7 +503,7 @@ describe("a skill-draft is invisible over the mounted host", () => {
     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.tenant}, ${draftAuthor}, ${draftAuthor}, 'skill-draft', 'Scratch',
+      VALUES (${host.tenantId}, ${draftAuthor}, ${draftAuthor}, 'skill-draft', 'Scratch',
         'draft body', '{"origin":"agent"}'::jsonb, 1)
       RETURNING "id"
     `);

examples/reference-host/test/acceptance.test.ts

@@ -7,7 +7,7 @@
     "examples/*"
   ],
   "scripts": {
-    "typecheck": "tsc --noEmit",
+    "typecheck": "bun run build && tsc --noEmit",
     "build": "bun run --cwd packages/artifacts build",
     "test": "bun run test:package && bun run test:acceptance",
     "test:package": "bun run --cwd packages/artifacts test",
@@ -17,5 +17,8 @@
     "@types/bun": "1.1.14",
     "@types/node": "22.10.5",
     "typescript": "5.7.2"
+  },
+  "overrides": {
+    "drizzle-orm": "0.45.2"
   }
 }