From d43e943ad649cdbf8f331f0da4efd68f3d4c02b5 Mon Sep 17 00:00:00 2001 From: Marcel Wege Date: Thu, 20 Aug 2026 14:07:44 +0200 Subject: [PATCH] feat(#577): skill ownership + lifecycle model (P1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the model layer for scope-owned, tamper-evident skills: - migrations/0040_skill_ownership_lifecycle.sql: owner_scope, lifecycle_status (draft/reviewed/published/archived), manifest_signature, manifest_signed_at columns on the existing skills table (0003). - src/services/skillLifecycle.ts: pure model — SkillOwnerScope (ScopeId restricted to personal/group/org), the lifecycle transition matrix, requiredCapabilities parsing from frontmatter (throws SkillManifestError with a field-naming message on malformed input, never silently drops — #690 guard), canonical manifest serialization + HMAC-SHA256 sign/verify. - src/services/skillLifecycleStore.ts: thin Postgres store over the new columns (assignPersonalOwner, transition), standalone from AgentGraphStore to keep this phase's surface to src/services/skill*. - test/skillLifecycle.test.ts (30 tests): exhaustive 4x4 transition matrix, byte-exact canonical manifest lock, capability-order independence, capability case-sensitivity (mirrors #724's role-key precedent), tamper detection, malformed-signature handling. - test/skillOwnershipLifecycleStore.pg.test.ts (6 tests, pg-gated): full draft->reviewed->published->archived flow against real Postgres, publish-gate rejection, re-signing on every transition. --- .../0040_skill_ownership_lifecycle.sql | 44 +++ middleware/src/services/skillLifecycle.ts | 275 ++++++++++++++ .../src/services/skillLifecycleStore.ts | 169 +++++++++ middleware/test/skillLifecycle.test.ts | 336 ++++++++++++++++++ .../skillOwnershipLifecycleStore.pg.test.ts | 158 ++++++++ 5 files changed, 982 insertions(+) create mode 100644 middleware/migrations/0040_skill_ownership_lifecycle.sql create mode 100644 middleware/src/services/skillLifecycle.ts create mode 100644 middleware/src/services/skillLifecycleStore.ts create mode 100644 middleware/test/skillLifecycle.test.ts create mode 100644 middleware/test/skillOwnershipLifecycleStore.pg.test.ts diff --git a/middleware/migrations/0040_skill_ownership_lifecycle.sql b/middleware/migrations/0040_skill_ownership_lifecycle.sql new file mode 100644 index 00000000..da0e7199 --- /dev/null +++ b/middleware/migrations/0040_skill_ownership_lifecycle.sql @@ -0,0 +1,44 @@ +-- #577 P1 — skill ownership + lifecycle model. +-- +-- Today's `skills` table (0003) is a flat registry row: no owner, no +-- lifecycle, no tamper-evidence. #577 turns a skill into a scope-owned, +-- shareable artifact by adding four columns: +-- +-- owner_scope — the skill's HOME, as a `ScopeId` wire string +-- (`personal:` / `group:` / `org:` — see +-- `@omadia/channel-sdk` `formatSessionScope`). Only these three kinds are +-- valid skill owners (enforced in application code by +-- `isSkillOwnerScope`, not by a CHECK — the wire grammar lives in the +-- channel-sdk package, not SQL). NULLable: pre-existing rows (imported +-- via the file-import pipeline, #391) predate ownership and stay +-- unowned until a later migration step assigns them a home — same +-- "nullable, backfills lazily" posture as `content_hash` in 0004. +-- lifecycle_status — `draft → reviewed → published → archived` +-- (#577 Kernkonzept #2). Defaults every row, existing and new, to +-- `draft` — publishing is an explicit, gated action +-- (`skillLifecycle.ts`), never an implicit consequence of this migration. +-- manifest_signature / manifest_signed_at — the HMAC-SHA256 signature over +-- the skill's canonical manifest (`skillLifecycle.ts` `canonicalSkillManifest`) +-- and when it was computed. NULL until the first lifecycle transition +-- signs the row. Tamper evidence: any of slug / name / owner_scope / +-- lifecycle_status / content_hash / requiredCapabilities changing without +-- a re-sign is what review/promote (#577 P3) will refuse. +-- +-- Deliberately NOT added here: a `required_capabilities` column. Frontmatter +-- already carries it (`frontmatter.requiredCapabilities`, #577 Kernkonzept +-- #1) and `skillLifecycle.ts` reads it from there — a second column would be +-- a second source of truth for the same fact. + +ALTER TABLE skills ADD COLUMN IF NOT EXISTS owner_scope TEXT; + +ALTER TABLE skills ADD COLUMN IF NOT EXISTS lifecycle_status TEXT + NOT NULL DEFAULT 'draft' + CHECK (lifecycle_status IN ('draft', 'reviewed', 'published', 'archived')); + +ALTER TABLE skills ADD COLUMN IF NOT EXISTS manifest_signature TEXT; +ALTER TABLE skills ADD COLUMN IF NOT EXISTS manifest_signed_at TIMESTAMPTZ; + +CREATE INDEX IF NOT EXISTS skills_owner_scope_idx ON skills(owner_scope); +CREATE INDEX IF NOT EXISTS skills_lifecycle_status_idx ON skills(lifecycle_status); + +-- rollback: ALTER TABLE skills DROP COLUMN IF EXISTS manifest_signed_at; ALTER TABLE skills DROP COLUMN IF EXISTS manifest_signature; ALTER TABLE skills DROP COLUMN IF EXISTS lifecycle_status; ALTER TABLE skills DROP COLUMN IF EXISTS owner_scope; diff --git a/middleware/src/services/skillLifecycle.ts b/middleware/src/services/skillLifecycle.ts new file mode 100644 index 00000000..7eca2a96 --- /dev/null +++ b/middleware/src/services/skillLifecycle.ts @@ -0,0 +1,275 @@ +/** + * #577 P1 — skill ownership + lifecycle model. + * + * Pure, synchronous decision logic for turning a registry skill (today's flat + * `skills` row, see `skillImport.ts`) into a scope-owned artifact with a + * tamper-evident lifecycle: + * + * - `SkillOwnerScope` restricts `ScopeId` (from `@omadia/channel-sdk`, #575) + * to the three kinds that can own a skill: `personal`, `group` (= team) + * and `org`. A skill cannot be owned by a `conversation`, a `system` + * origin or `unscoped` — those aren't homes, they're turn-scoped or absent. + * - `SKILL_LIFECYCLE_TRANSITIONS` is the ONLY place that decides which + * status moves are legal. `draft → reviewed → published → archived` is the + * forward path; `reviewed → draft` (a failed review sends it back) and + * `published → archived` are the only other edges. Every other pair + * (including same-status "transitions" and archived → anything) is + * illegal — archived is terminal, matching the Kernkonzept's "removed ones + * archived not deleted" posture for the git-pack step (#577, out of scope + * for P1). + * - `canonicalSkillManifest` fixes the exact bytes that get HMAC-signed. + * "Canonical" means a FIXED field order, a FIXED join, and FIXED + * normalization rules for the one field that is a set + * (`requiredCapabilities`: sorted for order-independence, but never + * case-folded — same precedent as role keys in #724, where canonicalizing + * a case-SENSITIVE identifier by lowercasing it silently merges two + * different values). An unpinned canonical form makes every signature + * verification a coin flip; this module pins it and locks it with a + * byte-exact test (`test/skillLifecycle.test.ts`). + * + * What this module does NOT do (by design, deferred to later #577 phases): + * - it never talks to Postgres (`skillLifecycleStore.ts` does that); + * - it never talks to `GrantStore` (#575) — `missingRequiredCapabilities` + * takes an already-resolved `granted` set, so P3 (sharing + promotion) is + * the one place that has to know how a capability got granted; + * - it never resolves shadowing across scopes (P2, `skillLoader.ts`). + */ + +import { createHmac, timingSafeEqual } from 'node:crypto'; +import type { ScopeId } from '@omadia/channel-sdk'; + +// ── Ownership ──────────────────────────────────────────────────────────── + +/** The `ScopeId` kinds that can own a skill: a user, a team, or the org. */ +export type SkillOwnerScope = Extract; + +/** Narrows a `ScopeId` to the subset that is valid skill ownership. */ +export function isSkillOwnerScope(scope: ScopeId): scope is SkillOwnerScope { + return scope.kind === 'personal' || scope.kind === 'group' || scope.kind === 'org'; +} + +// ── Lifecycle status ───────────────────────────────────────────────────── + +export const SKILL_LIFECYCLE_STATUSES = ['draft', 'reviewed', 'published', 'archived'] as const; +export type SkillLifecycleStatus = (typeof SKILL_LIFECYCLE_STATUSES)[number]; + +/** + * The complete legal-edge set. Anything not listed here — including every + * "stay put" pair and every edge out of `archived` — is illegal. Expressed as + * an explicit allowlist (not a linear "next status" function) so a reviewer + * can read the whole state machine in one place, and so the exhaustive test + * matrix in `skillLifecycle.test.ts` has something concrete to diff against. + */ +const SKILL_LIFECYCLE_EDGES: ReadonlySet = new Set([ + 'draft->reviewed', + 'reviewed->draft', // failed review — sent back for edits + 'reviewed->published', + 'published->archived', +]); + +/** Whether `from -> to` is a legal lifecycle move. Pure, total, no side effects. */ +export function canTransitionSkillLifecycle( + from: SkillLifecycleStatus, + to: SkillLifecycleStatus, +): boolean { + return SKILL_LIFECYCLE_EDGES.has(`${from}->${to}`); +} + +// ── Required capabilities (frontmatter-sourced, #577 Kernkonzept #1) ─────── + +/** + * Thrown when `frontmatter.requiredCapabilities` is present but malformed. + * Distinct from `TypeError`/`Error` so callers (and tests) can assert on + * `instanceof SkillManifestError` rather than string-matching a generic + * error. Every throw site names the exact field and the value it rejected — + * the #690 lesson: a parser that silently drops malformed input instead of + * raising must never come back, and a test that only checks "it throws" + * without checking the message can't catch a regression to a *different*, + * wrong error. + */ +export class SkillManifestError extends Error { + constructor(message: string) { + super(message); + this.name = 'SkillManifestError'; + } +} + +/** + * Read+validate `frontmatter.requiredCapabilities`. Absent is valid (empty + * skill, no gate) and returns `[]`. Present-but-malformed throws + * `SkillManifestError` naming the exact offending field — it NEVER silently + * coerces or drops, because a silently-dropped required capability is a + * publish gate that looks satisfied when it isn't. + */ +export function requiredCapabilitiesFromFrontmatter( + frontmatter: Record, +): string[] { + const raw = frontmatter['requiredCapabilities']; + if (raw === undefined) return []; + if (!Array.isArray(raw)) { + throw new SkillManifestError( + `frontmatter.requiredCapabilities must be an array of strings, got ${typeof raw}`, + ); + } + const seen = new Set(); + const result: string[] = []; + raw.forEach((entry, index) => { + if (typeof entry !== 'string' || entry.trim().length === 0) { + throw new SkillManifestError( + `frontmatter.requiredCapabilities[${index}] must be a non-empty string, got ${JSON.stringify(entry)}`, + ); + } + const trimmed = entry.trim(); + if (!seen.has(trimmed)) { + seen.add(trimmed); + result.push(trimmed); + } + }); + return result; +} + +/** + * Which of `required` are missing from `granted`. Empty result = the publish + * gate is satisfied. Deliberately a pure set-difference over an + * already-resolved `granted` set — the caller (P3) is responsible for + * resolving that set via `GrantStore` (#575); this function has no opinion on + * WHERE a capability came from, only whether it's present. Case-sensitive: + * capability identifiers are not role keys, but the same rule applies — + * never fold case on a value this module didn't mint. + */ +export function missingRequiredCapabilities( + required: readonly string[], + granted: ReadonlySet, +): string[] { + return required.filter((c) => !granted.has(c)); +} + +/** Whether every required capability is present in `granted`. */ +export function canPublishSkill(required: readonly string[], granted: ReadonlySet): boolean { + return missingRequiredCapabilities(required, granted).length === 0; +} + +// ── Canonical manifest + HMAC signature ───────────────────────────────── + +/** The facts a skill's tamper-evident signature covers. */ +export interface SkillManifestInput { + readonly slug: string; + readonly name: string; + /** Wire form of the owner `ScopeId` (`formatSessionScope` output). */ + readonly ownerScope: string; + readonly status: SkillLifecycleStatus; + /** sha256 over {frontmatter, body} — see `@omadia/orchestrator` `computeSkillHash`. */ + readonly contentHash: string; + readonly requiredCapabilities: readonly string[]; +} + +/** + * The canonical, byte-exact serialization that gets HMAC-signed. + * + * Fixed field order (an ARRAY of pairs, never an object — so this function's + * output can never depend on V8's key-insertion-order behavior), fixed `\n` + * join between fields and `=` between key/value, and exactly one + * normalization rule: `requiredCapabilities` is deduped and sorted by plain + * string comparison (codepoint order — NOT locale-aware, NOT case-folded) so + * that two frontmatter documents differing only in capability *order* + * produce the same manifest, while two differing in capability *case* + * ('Foo' vs 'foo') do NOT collapse into one. Every other field is a scalar + * already owned by its producer (`ownerScope` from `formatSessionScope`, + * `contentHash` from `computeSkillHash`) and is carried through verbatim. + * + * Changing this function's output for any existing input is a signing-format + * break: every previously-issued signature stops verifying. Locked byte-exact + * in `test/skillLifecycle.test.ts` for exactly that reason. + */ +export function canonicalSkillManifest(input: SkillManifestInput): string { + const capabilities = [...new Set(input.requiredCapabilities)].sort((a, b) => (a < b ? -1 : a > b ? 1 : 0)); + const fields: readonly [string, string][] = [ + ['slug', input.slug], + ['name', input.name], + ['ownerScope', input.ownerScope], + ['status', input.status], + ['contentHash', input.contentHash], + ['requiredCapabilities', capabilities.join(',')], + ]; + return fields.map(([key, value]) => `${key}=${value}`).join('\n'); +} + +/** HMAC-SHA256 (hex) over the canonical manifest. */ +export function signSkillManifest(input: SkillManifestInput, key: string): string { + return createHmac('sha256', key).update(canonicalSkillManifest(input), 'utf8').digest('hex'); +} + +/** + * Constant-time signature verification. Returns `false` (never throws) on any + * mismatch, including a malformed/wrong-length `signature` — `timingSafeEqual` + * throws on unequal buffer lengths, which this guards against explicitly so a + * garbage signature can't turn a `verify` call into an uncaught exception. + */ +export function verifySkillManifestSignature( + input: SkillManifestInput, + signature: string, + key: string, +): boolean { + const expectedHex = signSkillManifest(input, key); + let expected: Buffer; + let actual: Buffer; + try { + expected = Buffer.from(expectedHex, 'hex'); + actual = Buffer.from(signature, 'hex'); + } catch { + return false; + } + if (expected.length !== actual.length || expected.length === 0) return false; + return timingSafeEqual(expected, actual); +} + +// ── Combined transition decision ──────────────────────────────────────── + +export type SkillLifecycleTransitionResult = + | { + readonly ok: true; + readonly status: SkillLifecycleStatus; + readonly signature: string; + readonly signedAt: Date; + } + | { readonly ok: false; readonly reason: 'invalid-transition' } + | { readonly ok: false; readonly reason: 'missing-capabilities'; readonly missing: readonly string[] } + | { readonly ok: false; readonly reason: 'invalid-owner-scope' }; + +/** + * The single decision point that combines all three P1 invariants: is the + * status move legal, is the owner scope a valid skill home, and — only when + * the target is `published` — are all required capabilities granted. On + * success it also re-signs the manifest at the NEW status, so a caller can + * never persist a status change without an up-to-date signature (the + * tamper-evidence guarantee: `manifest_signature` always covers the row's + * CURRENT `lifecycle_status`, never a stale one). + * + * `granted` is ignored for every target other than `published` — reviewing or + * archiving a skill never needs a capability lookup, so callers moving + * between non-publish states can pass an empty set. + */ +export function transitionSkillLifecycle(args: { + readonly manifest: Omit; + readonly ownerScope: ScopeId; + readonly currentStatus: SkillLifecycleStatus; + readonly targetStatus: SkillLifecycleStatus; + readonly granted: ReadonlySet; + readonly signingKey: string; +}): SkillLifecycleTransitionResult { + if (!isSkillOwnerScope(args.ownerScope)) { + return { ok: false, reason: 'invalid-owner-scope' }; + } + if (!canTransitionSkillLifecycle(args.currentStatus, args.targetStatus)) { + return { ok: false, reason: 'invalid-transition' }; + } + if (args.targetStatus === 'published') { + const missing = missingRequiredCapabilities(args.manifest.requiredCapabilities, args.granted); + if (missing.length > 0) { + return { ok: false, reason: 'missing-capabilities', missing }; + } + } + const manifestAtTarget: SkillManifestInput = { ...args.manifest, status: args.targetStatus }; + const signature = signSkillManifest(manifestAtTarget, args.signingKey); + return { ok: true, status: args.targetStatus, signature, signedAt: new Date() }; +} diff --git a/middleware/src/services/skillLifecycleStore.ts b/middleware/src/services/skillLifecycleStore.ts new file mode 100644 index 00000000..675347ac --- /dev/null +++ b/middleware/src/services/skillLifecycleStore.ts @@ -0,0 +1,169 @@ +/** + * #577 P1 — Postgres-backed store for the ownership + lifecycle columns added + * by `migrations/0040_skill_ownership_lifecycle.sql`. + * + * Deliberately a THIN, standalone store over raw SQL rather than a new method + * on `AgentGraphStore` (`packages/harness-orchestrator`): the ownership + + * lifecycle surface is owned by #577 end-to-end (model, migration, store), + * and staying out of the orchestrator package keeps this phase's blast radius + * to `src/services/skill*` — see the #577 phase-cut's binding surface + * separation from the parallel #578 (Keychain) session. `computeSkillHash` is + * still reused from `@omadia/orchestrator` (same hash the import pipeline + * already computes) rather than reimplemented here. + */ + +import type { Pool } from 'pg'; +import { computeSkillHash } from '@omadia/orchestrator'; +import { formatSessionScope, parseSessionScope, type ScopeId } from '@omadia/channel-sdk'; + +import { + canonicalSkillManifest, + isSkillOwnerScope, + requiredCapabilitiesFromFrontmatter, + transitionSkillLifecycle, + type SkillLifecycleStatus, + type SkillLifecycleTransitionResult, +} from './skillLifecycle.js'; + +export interface SkillOwnershipLifecycleRow { + readonly id: string; + readonly slug: string; + readonly name: string; + readonly frontmatter: Record; + readonly body: string; + /** Wire form of the owner `ScopeId` (`personal:…` / `group:…` / `org:…`). Null = unowned. */ + readonly ownerScope: string | null; + readonly lifecycleStatus: SkillLifecycleStatus; + readonly manifestSignature: string | null; + readonly manifestSignedAt: Date | null; +} + +interface SkillOwnershipLifecycleDbRow { + id: string; + slug: string; + name: string; + frontmatter: Record | null; + body: string | null; + owner_scope: string | null; + lifecycle_status: SkillLifecycleStatus; + manifest_signature: string | null; + manifest_signed_at: Date | null; +} + +function mapRow(r: SkillOwnershipLifecycleDbRow): SkillOwnershipLifecycleRow { + return { + id: r.id, + slug: r.slug, + name: r.name, + frontmatter: r.frontmatter ?? {}, + body: r.body ?? '', + ownerScope: r.owner_scope, + lifecycleStatus: r.lifecycle_status, + manifestSignature: r.manifest_signature, + manifestSignedAt: r.manifest_signed_at, + }; +} + +/** + * Thrown by `transition()` when the move is illegal, the publish gate isn't + * satisfied, or the row has no valid owner scope to sign against. Carries the + * structured reason from `transitionSkillLifecycle` so a route layer (P3) can + * map it to the right HTTP status without string-matching `message`. + */ +export class SkillLifecycleTransitionRejected extends Error { + readonly reason: Exclude['reason']; + readonly missing?: readonly string[]; + + constructor(result: Exclude) { + super( + result.reason === 'missing-capabilities' + ? `skill publish blocked: missing capabilities [${result.missing.join(', ')}]` + : `skill lifecycle transition rejected: ${result.reason}`, + ); + this.name = 'SkillLifecycleTransitionRejected'; + this.reason = result.reason; + if (result.reason === 'missing-capabilities') this.missing = result.missing; + } +} + +export class PgSkillOwnershipLifecycleStore { + constructor(private readonly pool: Pool) {} + + async getSkill(skillId: string): Promise { + const result = await this.pool.query( + `SELECT id, slug, name, frontmatter, body, owner_scope, lifecycle_status, + manifest_signature, manifest_signed_at + FROM skills WHERE id = $1`, + [skillId], + ); + return result.rows[0] ? mapRow(result.rows[0]) : undefined; + } + + /** + * Assign a PERSONAL owner to a still-unowned draft skill. This is the ONLY + * direct-assignment path: #577 Kernkonzept #5 forbids creating a skill + * directly in team/org scope — those homes are reached only through the + * admin-gated promotion route (P3). Refuses to reassign an already-owned + * skill (call the — not-yet-built — promotion path for that) and refuses a + * non-draft target (ownership must be settled before review begins, since + * `ownerScope` is part of what gets signed). + */ + async assignPersonalOwner(skillId: string, owner: Extract): Promise { + const result = await this.pool.query( + `UPDATE skills SET owner_scope = $2, updated_at = now() + WHERE id = $1 AND owner_scope IS NULL AND lifecycle_status = 'draft'`, + [skillId, formatSessionScope(owner)], + ); + if ((result.rowCount ?? 0) === 0) { + const existing = await this.getSkill(skillId); + if (!existing) throw new Error(`skill ${skillId} not found`); + if (existing.ownerScope !== null) { + throw new Error(`skill ${skillId} already has an owner scope (${existing.ownerScope}) — reassignment requires promotion`); + } + throw new Error(`skill ${skillId} is not a draft (status: ${existing.lifecycleStatus}) — cannot assign an owner`); + } + } + + /** + * Move a skill's lifecycle status, re-signing its manifest on success. + * Throws `SkillLifecycleTransitionRejected` for every rejected move — never + * returns a "false-ish" result a caller could accidentally ignore. + */ + async transition( + skillId: string, + targetStatus: SkillLifecycleStatus, + opts: { readonly granted: ReadonlySet; readonly signingKey: string }, + ): Promise { + const row = await this.getSkill(skillId); + if (!row) throw new Error(`skill ${skillId} not found`); + if (row.ownerScope === null) { + throw new SkillLifecycleTransitionRejected({ ok: false, reason: 'invalid-owner-scope' }); + } + const ownerScopeParsed = parseSessionScope(row.ownerScope); + const contentHash = computeSkillHash(row.frontmatter, row.body); + const requiredCapabilities = requiredCapabilitiesFromFrontmatter(row.frontmatter); + + const result = transitionSkillLifecycle({ + manifest: { slug: row.slug, name: row.name, ownerScope: row.ownerScope, contentHash, requiredCapabilities }, + ownerScope: ownerScopeParsed, + currentStatus: row.lifecycleStatus, + targetStatus, + granted: opts.granted, + signingKey: opts.signingKey, + }); + if (!result.ok) throw new SkillLifecycleTransitionRejected(result); + + const updated = await this.pool.query( + `UPDATE skills SET lifecycle_status = $2, manifest_signature = $3, manifest_signed_at = $4, updated_at = now() + WHERE id = $1 + RETURNING id, slug, name, frontmatter, body, owner_scope, lifecycle_status, + manifest_signature, manifest_signed_at`, + [skillId, result.status, result.signature, result.signedAt], + ); + if (!updated.rows[0]) throw new Error(`skill ${skillId} vanished during transition`); + return mapRow(updated.rows[0]); + } +} + +/** Re-exported for callers that only need to verify a row without a Pool (e.g. a webhook). */ +export { canonicalSkillManifest, isSkillOwnerScope }; diff --git a/middleware/test/skillLifecycle.test.ts b/middleware/test/skillLifecycle.test.ts new file mode 100644 index 00000000..27155ec9 --- /dev/null +++ b/middleware/test/skillLifecycle.test.ts @@ -0,0 +1,336 @@ +import { describe, it } from 'node:test'; +import { strict as assert } from 'node:assert'; + +import { + SKILL_LIFECYCLE_STATUSES, + SkillManifestError, + canPublishSkill, + canTransitionSkillLifecycle, + canonicalSkillManifest, + isSkillOwnerScope, + missingRequiredCapabilities, + requiredCapabilitiesFromFrontmatter, + signSkillManifest, + transitionSkillLifecycle, + verifySkillManifestSignature, + type SkillManifestInput, +} from '../src/services/skillLifecycle.js'; + +// ── Ownership ────────────────────────────────────────────────────────────── + +describe('isSkillOwnerScope', () => { + it('accepts personal, group and org scopes', () => { + assert.equal(isSkillOwnerScope({ kind: 'personal', userId: 'u1' }), true); + assert.equal(isSkillOwnerScope({ kind: 'group', groupRef: 'team-a' }), true); + assert.equal(isSkillOwnerScope({ kind: 'org', orgId: 'byte5' }), true); + }); + + it('rejects conversation, system and unscoped scopes', () => { + assert.equal(isSkillOwnerScope({ kind: 'conversation', conversationId: 'c1' }), false); + assert.equal(isSkillOwnerScope({ kind: 'system', origin: 'routine', id: 'r1' }), false); + assert.equal(isSkillOwnerScope({ kind: 'unscoped', reason: 'absent' }), false); + }); +}); + +// ── Lifecycle transition matrix ───────────────────────────────────────────── + +describe('canTransitionSkillLifecycle', () => { + const LEGAL: ReadonlySet = new Set([ + 'draft->reviewed', + 'reviewed->draft', + 'reviewed->published', + 'published->archived', + ]); + + it('allows exactly the documented edges and rejects every other pair in the full matrix', () => { + for (const from of SKILL_LIFECYCLE_STATUSES) { + for (const to of SKILL_LIFECYCLE_STATUSES) { + const expected = LEGAL.has(`${from}->${to}`); + assert.equal( + canTransitionSkillLifecycle(from, to), + expected, + `${from} -> ${to} should be ${expected ? 'legal' : 'illegal'}`, + ); + } + } + }); + + it('has no self-transitions', () => { + for (const status of SKILL_LIFECYCLE_STATUSES) { + assert.equal(canTransitionSkillLifecycle(status, status), false, `${status} -> ${status}`); + } + }); + + it('archived is terminal — no edge leaves it', () => { + for (const to of SKILL_LIFECYCLE_STATUSES) { + assert.equal(canTransitionSkillLifecycle('archived', to), false, `archived -> ${to}`); + } + }); + + it('draft cannot jump straight to published or archived', () => { + assert.equal(canTransitionSkillLifecycle('draft', 'published'), false); + assert.equal(canTransitionSkillLifecycle('draft', 'archived'), false); + }); +}); + +// ── requiredCapabilities parsing — #690 silent-drop guard ────────────────── + +describe('requiredCapabilitiesFromFrontmatter', () => { + it('returns [] when the key is absent', () => { + assert.deepEqual(requiredCapabilitiesFromFrontmatter({}), []); + }); + + it('trims and dedupes valid entries, preserving first-seen order', () => { + assert.deepEqual( + requiredCapabilitiesFromFrontmatter({ requiredCapabilities: [' foo ', 'bar', 'foo'] }), + ['foo', 'bar'], + ); + }); + + it('throws SkillManifestError with a field-naming message when the key is not an array', () => { + assert.throws( + () => requiredCapabilitiesFromFrontmatter({ requiredCapabilities: 'foo' }), + (err: unknown) => { + assert.ok(err instanceof SkillManifestError); + assert.match(err.message, /frontmatter\.requiredCapabilities must be an array of strings, got string/); + return true; + }, + ); + }); + + it('throws SkillManifestError naming the exact index and value for a non-string entry', () => { + assert.throws( + () => requiredCapabilitiesFromFrontmatter({ requiredCapabilities: ['ok', 123, 'also-ok'] }), + (err: unknown) => { + assert.ok(err instanceof SkillManifestError); + assert.match( + err.message, + /frontmatter\.requiredCapabilities\[1\] must be a non-empty string, got 123/, + ); + return true; + }, + ); + }); + + it('throws SkillManifestError for a blank-string entry rather than silently keeping it', () => { + assert.throws( + () => requiredCapabilitiesFromFrontmatter({ requiredCapabilities: ['ok', ' '] }), + (err: unknown) => { + assert.ok(err instanceof SkillManifestError); + assert.match(err.message, /frontmatter\.requiredCapabilities\[1\]/); + return true; + }, + ); + }); +}); + +describe('missingRequiredCapabilities / canPublishSkill', () => { + it('reports nothing missing when everything required is granted', () => { + assert.deepEqual(missingRequiredCapabilities(['a', 'b'], new Set(['a', 'b', 'c'])), []); + assert.equal(canPublishSkill(['a', 'b'], new Set(['a', 'b', 'c'])), true); + }); + + it('reports exactly the ungranted capabilities', () => { + assert.deepEqual(missingRequiredCapabilities(['a', 'b', 'c'], new Set(['b'])), ['a', 'c']); + assert.equal(canPublishSkill(['a', 'b', 'c'], new Set(['b'])), false); + }); + + it('is case-sensitive — granting the lowercase form does not satisfy an uppercase requirement', () => { + assert.deepEqual(missingRequiredCapabilities(['Foo'], new Set(['foo'])), ['Foo']); + }); +}); + +// ── Canonical manifest — byte-exact lock ──────────────────────────────────── + +const BASE_MANIFEST: SkillManifestInput = { + slug: 'incident-runbook', + name: 'Incident Runbook', + ownerScope: 'personal:u-42', + status: 'draft', + contentHash: 'deadbeef', + requiredCapabilities: ['mcp.web-search', 'mcp.email-send'], +}; + +describe('canonicalSkillManifest', () => { + it('produces the exact locked byte form for a fixed input', () => { + assert.equal( + canonicalSkillManifest(BASE_MANIFEST), + 'slug=incident-runbook\n' + + 'name=Incident Runbook\n' + + 'ownerScope=personal:u-42\n' + + 'status=draft\n' + + 'contentHash=deadbeef\n' + + 'requiredCapabilities=mcp.email-send,mcp.web-search', + ); + }); + + it('is independent of requiredCapabilities input order', () => { + const reordered: SkillManifestInput = { + ...BASE_MANIFEST, + requiredCapabilities: ['mcp.email-send', 'mcp.web-search'], + }; + assert.equal(canonicalSkillManifest(BASE_MANIFEST), canonicalSkillManifest(reordered)); + }); + + it('dedupes repeated capabilities', () => { + const withDupe: SkillManifestInput = { + ...BASE_MANIFEST, + requiredCapabilities: ['mcp.web-search', 'mcp.email-send', 'mcp.web-search'], + }; + assert.equal(canonicalSkillManifest(BASE_MANIFEST), canonicalSkillManifest(withDupe)); + }); + + it('does NOT case-fold capabilities — "Foo" and "foo" stay distinct entries', () => { + const manifest: SkillManifestInput = { ...BASE_MANIFEST, requiredCapabilities: ['Foo', 'foo'] }; + assert.match(canonicalSkillManifest(manifest), /requiredCapabilities=Foo,foo/); + }); + + it('changes output when any single field changes (differential mutation check)', () => { + const baseline = canonicalSkillManifest(BASE_MANIFEST); + const variants: SkillManifestInput[] = [ + { ...BASE_MANIFEST, slug: 'other-slug' }, + { ...BASE_MANIFEST, name: 'Other Name' }, + { ...BASE_MANIFEST, ownerScope: 'org:byte5' }, + { ...BASE_MANIFEST, status: 'reviewed' }, + { ...BASE_MANIFEST, contentHash: 'cafebabe' }, + { ...BASE_MANIFEST, requiredCapabilities: ['mcp.web-search'] }, + ]; + for (const variant of variants) { + assert.notEqual(canonicalSkillManifest(variant), baseline, JSON.stringify(variant)); + } + }); +}); + +// ── HMAC signature ─────────────────────────────────────────────────────── + +describe('signSkillManifest / verifySkillManifestSignature', () => { + const KEY = 'test-signing-key'; + + it('round-trips: a fresh signature verifies against the same manifest + key', () => { + const sig = signSkillManifest(BASE_MANIFEST, KEY); + assert.equal(verifySkillManifestSignature(BASE_MANIFEST, sig, KEY), true); + }); + + it('is deterministic for the same input', () => { + assert.equal(signSkillManifest(BASE_MANIFEST, KEY), signSkillManifest(BASE_MANIFEST, KEY)); + }); + + it('rejects a signature computed with a different key', () => { + const sig = signSkillManifest(BASE_MANIFEST, KEY); + assert.equal(verifySkillManifestSignature(BASE_MANIFEST, sig, 'wrong-key'), false); + }); + + it('rejects when ANY manifest field is tampered after signing (tamper-evidence)', () => { + const sig = signSkillManifest(BASE_MANIFEST, KEY); + const tampered: SkillManifestInput[] = [ + { ...BASE_MANIFEST, status: 'published' }, + { ...BASE_MANIFEST, ownerScope: 'org:someone-else' }, + { ...BASE_MANIFEST, contentHash: 'tampered-hash' }, + { ...BASE_MANIFEST, requiredCapabilities: [] }, + ]; + for (const variant of tampered) { + assert.equal(verifySkillManifestSignature(variant, sig, KEY), false, JSON.stringify(variant)); + } + }); + + it('rejects a malformed (non-hex / wrong-length) signature without throwing', () => { + assert.equal(verifySkillManifestSignature(BASE_MANIFEST, 'not-hex-!!', KEY), false); + assert.equal(verifySkillManifestSignature(BASE_MANIFEST, 'ab', KEY), false); + assert.equal(verifySkillManifestSignature(BASE_MANIFEST, '', KEY), false); + }); +}); + +// ── Combined transition decision ──────────────────────────────────────── + +describe('transitionSkillLifecycle', () => { + const manifest: Omit = { + slug: 'incident-runbook', + name: 'Incident Runbook', + ownerScope: 'personal:u-42', + contentHash: 'deadbeef', + requiredCapabilities: ['mcp.web-search'], + }; + const KEY = 'test-signing-key'; + + it('rejects an illegal status move before even looking at capabilities', () => { + const result = transitionSkillLifecycle({ + manifest, + ownerScope: { kind: 'personal', userId: 'u-42' }, + currentStatus: 'draft', + targetStatus: 'published', + granted: new Set(['mcp.web-search']), + signingKey: KEY, + }); + assert.deepEqual(result, { ok: false, reason: 'invalid-transition' }); + }); + + it('rejects an invalid owner scope even for an otherwise-legal move', () => { + const result = transitionSkillLifecycle({ + manifest, + ownerScope: { kind: 'conversation', conversationId: 'c1' }, + currentStatus: 'draft', + targetStatus: 'reviewed', + granted: new Set(), + signingKey: KEY, + }); + assert.deepEqual(result, { ok: false, reason: 'invalid-owner-scope' }); + }); + + it('blocks publish when a required capability is not granted, naming it', () => { + const result = transitionSkillLifecycle({ + manifest, + ownerScope: { kind: 'personal', userId: 'u-42' }, + currentStatus: 'reviewed', + targetStatus: 'published', + granted: new Set(), + signingKey: KEY, + }); + assert.deepEqual(result, { ok: false, reason: 'missing-capabilities', missing: ['mcp.web-search'] }); + }); + + it('does not require capabilities for a non-publish move (draft -> reviewed)', () => { + const result = transitionSkillLifecycle({ + manifest, + ownerScope: { kind: 'personal', userId: 'u-42' }, + currentStatus: 'draft', + targetStatus: 'reviewed', + granted: new Set(), + signingKey: KEY, + }); + assert.equal(result.ok, true); + }); + + it('on success, re-signs the manifest AT THE NEW status — the signature is not reusable across statuses', () => { + const toReviewed = transitionSkillLifecycle({ + manifest, + ownerScope: { kind: 'personal', userId: 'u-42' }, + currentStatus: 'draft', + targetStatus: 'reviewed', + granted: new Set(), + signingKey: KEY, + }); + assert.equal(toReviewed.ok, true); + if (!toReviewed.ok) return; + + const expectedDraftSig = signSkillManifest({ ...manifest, status: 'draft' }, KEY); + const expectedReviewedSig = signSkillManifest({ ...manifest, status: 'reviewed' }, KEY); + assert.notEqual(toReviewed.signature, expectedDraftSig); + assert.equal(toReviewed.signature, expectedReviewedSig); + }); + + it('succeeds publishing once every required capability is granted', () => { + const result = transitionSkillLifecycle({ + manifest, + ownerScope: { kind: 'personal', userId: 'u-42' }, + currentStatus: 'reviewed', + targetStatus: 'published', + granted: new Set(['mcp.web-search']), + signingKey: KEY, + }); + assert.equal(result.ok, true); + if (result.ok) { + assert.equal(result.status, 'published'); + assert.ok(result.signedAt instanceof Date); + } + }); +}); diff --git a/middleware/test/skillOwnershipLifecycleStore.pg.test.ts b/middleware/test/skillOwnershipLifecycleStore.pg.test.ts new file mode 100644 index 00000000..1271b451 --- /dev/null +++ b/middleware/test/skillOwnershipLifecycleStore.pg.test.ts @@ -0,0 +1,158 @@ +import { strict as assert } from 'node:assert'; +import { dirname, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { after, before, describe, it } from 'node:test'; + +import { Pool } from 'pg'; + +import { AgentGraphStore, runMultiOrchestratorMigrations } from '@omadia/orchestrator'; + +import { probePgTest } from './_helpers/pgTestDb.js'; +import { + PgSkillOwnershipLifecycleStore, + SkillLifecycleTransitionRejected, +} from '../src/services/skillLifecycleStore.js'; + +/** + * PG-gated coverage for #577 P1's ownership + lifecycle columns + * (`migrations/0040_skill_ownership_lifecycle.sql`) and their store + * (`skillLifecycleStore.ts`). Deliberately a SEPARATE file from + * `skillLifecycleStore.pg.test.ts` (pre-existing, Wave 0 content-hash + * coverage over `AgentGraphStore.upsertSkill`) so neither suite's fixtures + * collide. Skips when no test Postgres is reachable, same posture as every + * other `.pg.test.ts` in this tree. + */ +const SLUG_PREFIX = 'p577-ownership-test-'; +const migrationsDir = resolve(dirname(fileURLToPath(import.meta.url)), '..', 'migrations'); + +const { url: PG_URL, reachable: pgAvailable } = await probePgTest({ + label: 'skillOwnershipLifecycleStore', + vars: ['GRAPH_PG_TEST_URL', 'MEMORY_PG_TEST_URL', 'WS5_PG_TEST_URL'], +}); +const probePool = new Pool({ connectionString: PG_URL }); + +describe('PgSkillOwnershipLifecycleStore (pg)', { skip: !pgAvailable }, () => { + const pool = probePool; + let graphStore: AgentGraphStore; + let store: PgSkillOwnershipLifecycleStore; + const KEY = 'test-signing-key'; + + async function cleanup(): Promise { + await pool.query('DELETE FROM skills WHERE slug LIKE $1', [`${SLUG_PREFIX}%`]); + } + + before(async () => { + await runMultiOrchestratorMigrations(pool, undefined, migrationsDir); + await cleanup(); + graphStore = new AgentGraphStore(pool); + store = new PgSkillOwnershipLifecycleStore(pool); + }); + + after(async () => { + await cleanup(); + await pool.end(); + }); + + async function seedDraftSkill(suffix: string, requiredCapabilities: readonly string[] = []): Promise { + const row = await graphStore.upsertSkill({ + slug: `${SLUG_PREFIX}${suffix}`, + name: `Skill ${suffix}`, + body: 'body', + frontmatter: requiredCapabilities.length > 0 ? { requiredCapabilities } : {}, + }); + return row.id; + } + + it('a freshly imported skill has no owner and starts in draft', async () => { + const id = await seedDraftSkill('fresh'); + const row = await store.getSkill(id); + assert.equal(row?.ownerScope, null); + assert.equal(row?.lifecycleStatus, 'draft'); + assert.equal(row?.manifestSignature, null); + }); + + it('assignPersonalOwner sets owner_scope on a draft, unowned skill', async () => { + const id = await seedDraftSkill('assign'); + await store.assignPersonalOwner(id, { kind: 'personal', userId: 'u-1' }); + const row = await store.getSkill(id); + assert.equal(row?.ownerScope, 'personal:u-1'); + }); + + it('refuses to reassign an already-owned skill', async () => { + const id = await seedDraftSkill('reassign'); + await store.assignPersonalOwner(id, { kind: 'personal', userId: 'u-1' }); + await assert.rejects( + () => store.assignPersonalOwner(id, { kind: 'personal', userId: 'u-2' }), + /already has an owner scope/, + ); + const row = await store.getSkill(id); + assert.equal(row?.ownerScope, 'personal:u-1', 'original owner is untouched'); + }); + + it('transition() throws SkillLifecycleTransitionRejected on an unowned skill', async () => { + const id = await seedDraftSkill('unowned'); + await assert.rejects( + () => store.transition(id, 'reviewed', { granted: new Set(), signingKey: KEY }), + (err: unknown) => { + assert.ok(err instanceof SkillLifecycleTransitionRejected); + assert.equal(err.reason, 'invalid-owner-scope'); + return true; + }, + ); + }); + + it('draft -> reviewed -> published -> archived signs at each step and persists the new status', async () => { + const id = await seedDraftSkill('lifecycle', ['mcp.web-search']); + await store.assignPersonalOwner(id, { kind: 'personal', userId: 'u-lifecycle' }); + + const reviewed = await store.transition(id, 'reviewed', { granted: new Set(), signingKey: KEY }); + assert.equal(reviewed.lifecycleStatus, 'reviewed'); + assert.ok(reviewed.manifestSignature); + assert.ok(reviewed.manifestSignedAt instanceof Date); + + // Publish is blocked until the required capability is granted. + await assert.rejects( + () => store.transition(id, 'published', { granted: new Set(), signingKey: KEY }), + (err: unknown) => { + assert.ok(err instanceof SkillLifecycleTransitionRejected); + assert.equal(err.reason, 'missing-capabilities'); + assert.deepEqual(err.missing, ['mcp.web-search']); + return true; + }, + ); + const stillReviewed = await store.getSkill(id); + assert.equal(stillReviewed?.lifecycleStatus, 'reviewed', 'rejected transition does not mutate status'); + + const published = await store.transition(id, 'published', { + granted: new Set(['mcp.web-search']), + signingKey: KEY, + }); + assert.equal(published.lifecycleStatus, 'published'); + assert.notEqual(published.manifestSignature, reviewed.manifestSignature, 're-signed at the new status'); + + const archived = await store.transition(id, 'archived', { granted: new Set(), signingKey: KEY }); + assert.equal(archived.lifecycleStatus, 'archived'); + + await assert.rejects( + () => store.transition(id, 'draft', { granted: new Set(), signingKey: KEY }), + (err: unknown) => { + assert.ok(err instanceof SkillLifecycleTransitionRejected); + assert.equal(err.reason, 'invalid-transition'); + return true; + }, + 'archived is terminal', + ); + }); + + it('a re-signed manifest changes when the underlying skill body changes (content_hash drift)', async () => { + const id = await seedDraftSkill('drift'); + await store.assignPersonalOwner(id, { kind: 'personal', userId: 'u-drift' }); + const before1 = await store.transition(id, 'reviewed', { granted: new Set(), signingKey: KEY }); + + // Edit the body directly (bypassing the store, as an operator edit would). + await graphStore.updateSkill(id, { body: 'a very different body' }); + + const back = await store.transition(id, 'draft', { granted: new Set(), signingKey: KEY }); + assert.notEqual(back.manifestSignature, before1.manifestSignature, 'signature tracks content_hash drift'); + }); +});