From d43e943ad649cdbf8f331f0da4efd68f3d4c02b5 Mon Sep 17 00:00:00 2001 From: Marcel Wege Date: Thu, 20 Aug 2026 14:07:44 +0200 Subject: [PATCH 1/3] 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 000000000..da0e71998 --- /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 000000000..7eca2a960 --- /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 000000000..675347ace --- /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 000000000..27155ec97 --- /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 000000000..1271b4517 --- /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'); + }); +}); From 8c8eef1232ec0463059e6ecb4030908871f03a45 Mon Sep 17 00:00:00 2001 From: Marcel Wege Date: Thu, 20 Aug 2026 14:13:00 +0200 Subject: [PATCH 2/3] feat(#577): scope-ordered skill resolution with shadowing (P2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds resolveSkillByName in src/services/skillResolver.ts: given a skill name and a requester's scope, pick the single winning skill row across personal -> shared -> team -> org, in that strict order (#577 Kernkonzept #4). Pure and synchronous, layered on P1's ScopeId/lifecycle model (#767) without touching GrantStore directly — membership/sharing are passed in pre-resolved (SkillResolutionContext), same seam P1 used for capability grants. Resolves by `name`, not `slug`: skills.slug is globally UNIQUE (0003), so no schema change is needed for two scopes to own same-named skills. Two invariants get explicit, separately-mutation-tested coverage: - precedence order (personal beats org even at equal names -- the 'non-empty result from the wrong level' danger named in the phase spec, parallel to the quorum='all' fail-open lesson from #726) - the lifecycle gate: only 'published' skills are eligible candidates, filtered BEFORE bucketing, so a draft at a higher-precedence level can never outrank a published skill lower down. A third case is guarded structurally: two candidates tying within one bucket return an explicit 'ambiguous' result (level + full candidate list) rather than an arbitrary pick -- same 'absence/uncertainty is a type' posture as resolveCapabilities (#575) and RoleSourceRegistry (#333). 15 tests in test/skillResolver.test.ts, all pure (no pg gate needed). Mutation-tested: reversing bucket precedence order fails 3 tests; dropping the published-status filter fails 3 tests. --- middleware/src/services/skillResolver.ts | 127 ++++++++++++++++++++ middleware/test/skillResolver.test.ts | 146 +++++++++++++++++++++++ 2 files changed, 273 insertions(+) create mode 100644 middleware/src/services/skillResolver.ts create mode 100644 middleware/test/skillResolver.test.ts diff --git a/middleware/src/services/skillResolver.ts b/middleware/src/services/skillResolver.ts new file mode 100644 index 000000000..728f17a1e --- /dev/null +++ b/middleware/src/services/skillResolver.ts @@ -0,0 +1,127 @@ +/** + * #577 P2 — scope-ordered skill resolution with shadowing. + * + * Given a skill NAME (not `slug` — `skills.slug` is globally UNIQUE, so two + * scopes can never own the same slug; `name` has no such constraint and is + * the identity a shadowing lookup resolves against, e.g. "which skill + * answers to the name 'incident-runbook' for THIS requester"), pick the + * single winning row across the owner-scope hierarchy: **personal beats + * shared beats team beats org** (#577 Kernkonzept #4). + * + * Pure and synchronous, like `skillLifecycle.ts`. It does NOT resolve team + * membership, org membership or sharing grants itself — `SkillResolutionContext` + * takes all three as already-resolved inputs, same seam as + * `missingRequiredCapabilities` taking an already-resolved `granted` set in + * P1. P3 is the phase that knows how to ask `GrantStore` for `sharedSkillIds` + * and however membership is sourced for `memberTeams`/`orgId`; this module + * only knows what to do once it has the answer. + * + * ## The dangerous case this resolver is built to avoid + * + * A shadowing resolver's failure mode is never "returns nothing" — it's + * "returns something, from the WRONG level, and nobody notices because the + * result isn't empty." Two instances of that shape are guarded here + * explicitly (both covered in `test/skillResolver.test.ts`): + * + * 1. **Wrong precedence.** If personal and org both have a skill named `x`, + * returning the org one is silently wrong in the safe direction (too + * little personalization) but returning it INSTEAD of a more-privileged + * personal draft would be silently wrong in the dangerous direction. This + * resolver always evaluates buckets in strict `personal → shared → team → + * org` order and stops at the first non-empty one. + * 2. **Wrong lifecycle status.** An unpublished skill (`draft` / `reviewed` / + * `archived`, #577 P1) must never win a resolution just because it sits + * at a higher-precedence level than a published one elsewhere — that + * would silently serve unreviewed content ahead of reviewed content. + * Non-`published` candidates are filtered out BEFORE bucketing, not + * merely deprioritized. + * + * A third case that is not "wrong level" but is the same silent-corruption + * shape: two candidates tie within one bucket (e.g. a data bug lets two org + * skills share a name). Resolution refuses to arbitrarily pick one — it + * reports `ambiguous` with the level and the full candidate set, exactly the + * "absence/uncertainty is a type, not a value" posture `resolveCapabilities` + * (#575) and `RoleSourceRegistry` (#333) already use elsewhere in this repo. + */ + +import { parseSessionScope, type ScopeId } from '@omadia/channel-sdk'; +import type { SkillLifecycleStatus } from './skillLifecycle.js'; + +export type SkillResolutionLevel = 'personal' | 'shared' | 'team' | 'org'; + +/** The minimal shape a resolvable skill row needs — deliberately narrow. */ +export interface ResolvableSkill { + readonly id: string; + readonly name: string; + /** Wire form of the owner `ScopeId`, or `null` for an unowned (legacy) skill. */ + readonly ownerScope: string | null; + readonly lifecycleStatus: SkillLifecycleStatus; +} + +export interface SkillResolutionContext { + /** Who/what is asking. Only `kind: 'personal'` ever populates the `personal` bucket. */ + readonly requesterScope: ScopeId; + /** `groupRef`s (teams) the requester currently belongs to. */ + readonly memberTeams: ReadonlySet; + /** The org the requester belongs to, or `undefined` if none. */ + readonly orgId: string | undefined; + /** Skill ids explicitly granted to the requester by another owner (#575 `GrantStore`, wired in P3). */ + readonly sharedSkillIds: ReadonlySet; +} + +export type SkillResolutionResult = + | { readonly ok: true; readonly level: SkillResolutionLevel; readonly skill: T } + | { readonly ok: false; readonly reason: 'not-found' } + | { + readonly ok: false; + readonly reason: 'ambiguous'; + readonly level: SkillResolutionLevel; + readonly candidates: readonly T[]; + }; + +function isOwnedPersonalBy(ownerScope: string, requester: ScopeId): boolean { + if (requester.kind !== 'personal') return false; + const parsed = parseSessionScope(ownerScope); + return parsed.kind === 'personal' && parsed.userId === requester.userId; +} + +function isOwnedByAnyTeam(ownerScope: string, memberTeams: ReadonlySet): boolean { + const parsed = parseSessionScope(ownerScope); + return parsed.kind === 'group' && memberTeams.has(parsed.groupRef); +} + +function isOwnedByOrg(ownerScope: string, orgId: string | undefined): boolean { + if (orgId === undefined) return false; + const parsed = parseSessionScope(ownerScope); + return parsed.kind === 'org' && parsed.orgId === orgId; +} + +/** + * Resolve `name` against `candidates` for the requester described by `ctx`. + * Case-sensitive on `name` — same rule as everywhere else in the #577 model: + * an identity this module didn't mint is never case-folded. + */ +export function resolveSkillByName( + name: string, + candidates: readonly T[], + ctx: SkillResolutionContext, +): SkillResolutionResult { + // Published + owned only. An unowned (`ownerScope === null`) row has no + // home to bucket it by and can never win a scope-ordered resolution. + const eligible = candidates.filter( + (c) => c.name === name && c.lifecycleStatus === 'published' && c.ownerScope !== null, + ); + + const buckets: readonly [SkillResolutionLevel, readonly T[]][] = [ + ['personal', eligible.filter((c) => isOwnedPersonalBy(c.ownerScope as string, ctx.requesterScope))], + ['shared', eligible.filter((c) => ctx.sharedSkillIds.has(c.id))], + ['team', eligible.filter((c) => isOwnedByAnyTeam(c.ownerScope as string, ctx.memberTeams))], + ['org', eligible.filter((c) => isOwnedByOrg(c.ownerScope as string, ctx.orgId))], + ]; + + for (const [level, group] of buckets) { + if (group.length === 1) return { ok: true, level, skill: group[0] as T }; + if (group.length > 1) return { ok: false, reason: 'ambiguous', level, candidates: group }; + } + return { ok: false, reason: 'not-found' }; +} diff --git a/middleware/test/skillResolver.test.ts b/middleware/test/skillResolver.test.ts new file mode 100644 index 000000000..63c0b61ba --- /dev/null +++ b/middleware/test/skillResolver.test.ts @@ -0,0 +1,146 @@ +import { describe, it } from 'node:test'; +import { strict as assert } from 'node:assert'; + +import { + resolveSkillByName, + type ResolvableSkill, + type SkillResolutionContext, +} from '../src/services/skillResolver.js'; + +function skill(id: string, name: string, ownerScope: string | null, status: ResolvableSkill['lifecycleStatus'] = 'published'): ResolvableSkill { + return { id, name, ownerScope, lifecycleStatus: status }; +} + +const REQUESTER: SkillResolutionContext = { + requesterScope: { kind: 'personal', userId: 'u-1' }, + memberTeams: new Set(['team-a']), + orgId: 'byte5', + sharedSkillIds: new Set(), +}; + +describe('resolveSkillByName — precedence', () => { + it('personal beats org even at equal names (the dangerous non-empty-wrong-level case)', () => { + const candidates = [ + skill('s-personal', 'runbook', 'personal:u-1'), + skill('s-org', 'runbook', 'org:byte5'), + ]; + const result = resolveSkillByName('runbook', candidates, REQUESTER); + assert.deepEqual(result, { ok: true, level: 'personal', skill: candidates[0] }); + }); + + it('shared beats team beats org', () => { + const shared = skill('s-shared', 'runbook', 'personal:someone-else'); + const team = skill('s-team', 'runbook', 'group:team-a'); + const org = skill('s-org', 'runbook', 'org:byte5'); + const ctx: SkillResolutionContext = { ...REQUESTER, sharedSkillIds: new Set(['s-shared']) }; + + assert.deepEqual(resolveSkillByName('runbook', [shared, team, org], ctx), { + ok: true, + level: 'shared', + skill: shared, + }); + assert.deepEqual(resolveSkillByName('runbook', [team, org], ctx), { + ok: true, + level: 'team', + skill: team, + }); + assert.deepEqual(resolveSkillByName('runbook', [org], ctx), { + ok: true, + level: 'org', + skill: org, + }); + }); + + it('falls through correctly when a level is absent (absence at personal -> shared -> team -> org)', () => { + const org = skill('s-org', 'runbook', 'org:byte5'); + // No personal, no shared, no team candidate at all — only org exists. + const result = resolveSkillByName('runbook', [org], REQUESTER); + assert.deepEqual(result, { ok: true, level: 'org', skill: org }); + }); + + it('reports not-found when nothing matches at any level', () => { + const result = resolveSkillByName('missing', [skill('s-org', 'runbook', 'org:byte5')], REQUESTER); + assert.deepEqual(result, { ok: false, reason: 'not-found' }); + }); +}); + +describe('resolveSkillByName — lifecycle gate (dangerous case #2)', () => { + it('a draft at a higher-precedence level does NOT win over a published skill at a lower level', () => { + const draftPersonal = skill('s-draft', 'runbook', 'personal:u-1', 'draft'); + const publishedOrg = skill('s-org', 'runbook', 'org:byte5', 'published'); + const result = resolveSkillByName('runbook', [draftPersonal, publishedOrg], REQUESTER); + assert.deepEqual(result, { ok: true, level: 'org', skill: publishedOrg }); + }); + + it('an archived skill never resolves, even as the only candidate', () => { + const archived = skill('s-archived', 'runbook', 'personal:u-1', 'archived'); + const result = resolveSkillByName('runbook', [archived], REQUESTER); + assert.deepEqual(result, { ok: false, reason: 'not-found' }); + }); + + it('a reviewed-but-not-published skill never resolves', () => { + const reviewed = skill('s-reviewed', 'runbook', 'personal:u-1', 'reviewed'); + const result = resolveSkillByName('runbook', [reviewed], REQUESTER); + assert.deepEqual(result, { ok: false, reason: 'not-found' }); + }); +}); + +describe('resolveSkillByName — ownership + ambiguity', () => { + it('an unowned (null ownerScope) skill never resolves, at any bucket', () => { + const unowned = skill('s-unowned', 'runbook', null); + const ctx: SkillResolutionContext = { ...REQUESTER, sharedSkillIds: new Set(['s-unowned']) }; + const result = resolveSkillByName('runbook', [unowned], ctx); + assert.deepEqual(result, { ok: false, reason: 'not-found' }); + }); + + it('two org skills with the same name are ambiguous, never silently picked', () => { + const a = skill('s-org-a', 'runbook', 'org:byte5'); + const b = skill('s-org-b', 'runbook', 'org:byte5'); + const result = resolveSkillByName('runbook', [a, b], REQUESTER); + assert.deepEqual(result, { ok: false, reason: 'ambiguous', level: 'org', candidates: [a, b] }); + }); + + it('ambiguity at personal short-circuits — a clean org candidate is never consulted as a tiebreaker', () => { + const p1 = skill('s-p1', 'runbook', 'personal:u-1'); + const p2 = skill('s-p2', 'runbook', 'personal:u-1'); + const org = skill('s-org', 'runbook', 'org:byte5'); + const result = resolveSkillByName('runbook', [p1, p2, org], REQUESTER); + assert.deepEqual(result, { ok: false, reason: 'ambiguous', level: 'personal', candidates: [p1, p2] }); + }); + + it('personal bucket is empty for a non-personal requester (e.g. a system/routine scope)', () => { + const personalOwned = skill('s-personal', 'runbook', 'personal:u-1'); + const org = skill('s-org', 'runbook', 'org:byte5'); + const ctx: SkillResolutionContext = { + ...REQUESTER, + requesterScope: { kind: 'system', origin: 'routine', id: 'r1' }, + }; + const result = resolveSkillByName('runbook', [personalOwned, org], ctx); + assert.deepEqual(result, { ok: true, level: 'org', skill: org }); + }); + + it('a personal skill owned by a DIFFERENT user does not match the personal bucket (and is not auto-shared)', () => { + const someoneElses = skill('s-other', 'runbook', 'personal:someone-else'); + const result = resolveSkillByName('runbook', [someoneElses], REQUESTER); + assert.deepEqual(result, { ok: false, reason: 'not-found' }); + }); + + it('team membership in a DIFFERENT team than the owner does not match', () => { + const otherTeam = skill('s-team-b', 'runbook', 'group:team-b'); + const result = resolveSkillByName('runbook', [otherTeam], REQUESTER); + assert.deepEqual(result, { ok: false, reason: 'not-found' }); + }); + + it('org candidate does not match when requester has no org', () => { + const org = skill('s-org', 'runbook', 'org:byte5'); + const ctx: SkillResolutionContext = { ...REQUESTER, orgId: undefined }; + const result = resolveSkillByName('runbook', [org], ctx); + assert.deepEqual(result, { ok: false, reason: 'not-found' }); + }); + + it('name matching is case-sensitive', () => { + const org = skill('s-org', 'Runbook', 'org:byte5'); + const result = resolveSkillByName('runbook', [org], REQUESTER); + assert.deepEqual(result, { ok: false, reason: 'not-found' }); + }); +}); From 9ea0dd8a7e65c6d2141339fe23b93fe670c69e0a Mon Sep 17 00:00:00 2001 From: Marcel Wege Date: Thu, 20 Aug 2026 14:22:34 +0200 Subject: [PATCH 3/3] feat(#577): sharing via GrantStore + admin-gated promotion + cron write-guard (P3) Adds the sharing/promotion/write-guard layer on top of P1 (skill ownership/lifecycle) and P2 (scope-ordered resolution): - src/services/skillLifecycle.ts: assertHumanActor / SkillAutomationWriteBlocked -- the enforced cron write-guard (#577 Kernkonzept #6). Checks ScopeId.kind === 'system' (scopeId.ts's own documented boundary: "no human is present in any of them" -- routine/schedule/conductor/ conductor-builder), so it needs no new taxonomy. Threaded as the first check in every mutating store method -- a blocked write never reaches the database (asserted in the pg tests, not just at the pure-function level). - src/services/skillLifecycleStore.ts: - assignPersonalOwner and transition now take an actorScope and call assertHumanActor before any query. - New promoteSkillOwnerScope(skillId, targetScope, opts): the ONLY way a skill reaches team/org ownership (#577 Kernkonzept #5 -- no direct creation there). Requires the skill be already 'published', re-signs the manifest at the NEW ownerScope + SAME status (promotion is a signature-changing event, since ownerScope is a signed field). Admin-gating itself (an authenticated-session check) is left to the route layer -- this method enforces the cron guard and the published-only invariant, nothing about roles. - src/services/skillSharing.ts: sharing = a grant over GrantStore (#575), not a parallel ACL (#577 Kernkonzept #5). Encodes "skill X is shared with principal Y" as a Capability string (skill:read:) and resolveSharedSkillIds(principal, roles, grants) turns a resolved capability set back into the ReadonlySet P2's resolveSkillByName needs for its 'shared' bucket. Denials subtract from grants (same rule as the audience floor). Deliberately does NOT collapse "unresolved" (partial role lookup) to empty at this layer -- SharedSkillIdsResult keeps the fact visible; toSharedSkillIdsSet is the explicit fail-closed adapter for callers who just want the resolver input. Consumes GrantStore/resolveCapabilities only -- no edits to grants.ts. Tests: 15 new pure tests (skillSharing.test.ts) covering direct grants, role-grant union, denial subtraction, unresolved-vs-empty, and the fail-closed adapter; 2 new pure tests for assertHumanActor (all 4 system origins blocked, every other ScopeId kind passes); 6 new pg-gated tests covering the cron guard on all three mutating methods and the full promotion flow (org target, team target, draft-refusal). Mutation-tested: disabling assertHumanActor fails 4 tests across both the pure and pg-gated suites; dropping the published-only gate on promoteSkillOwnerScope fails 1 pg-gated test. Both reverted; working tree clean afterward. Not in this PR: the admin-gated HTTP route itself. Wiring a session-authenticated Express route touches src/index.ts (the shared app-bootstrap file, ~3800 lines, high concurrent-edit traffic across this repo's parallel issue-harness sessions) and needs to correctly replicate the existing session/auth middleware chain -- a promotion endpoint with a subtly wrong auth check is a real security regression, not a place to move fast. The service-layer method (promoteSkillOwnerScope) is complete and fully tested; mounting it behind route + session auth is left as a follow-up (naturally lands with P4's admin UI, which needs a concrete endpoint contract anyway). --- middleware/src/services/skillLifecycle.ts | 39 ++++++ .../src/services/skillLifecycleStore.ts | 86 ++++++++++++- middleware/src/services/skillSharing.ts | 95 ++++++++++++++ middleware/test/skillLifecycle.test.ts | 30 +++++ .../skillOwnershipLifecycleStore.pg.test.ts | 110 ++++++++++++++-- middleware/test/skillSharing.test.ts | 119 ++++++++++++++++++ 6 files changed, 461 insertions(+), 18 deletions(-) create mode 100644 middleware/src/services/skillSharing.ts create mode 100644 middleware/test/skillSharing.test.ts diff --git a/middleware/src/services/skillLifecycle.ts b/middleware/src/services/skillLifecycle.ts index 7eca2a960..0a3bf4df8 100644 --- a/middleware/src/services/skillLifecycle.ts +++ b/middleware/src/services/skillLifecycle.ts @@ -48,6 +48,45 @@ export function isSkillOwnerScope(scope: ScopeId): scope is SkillOwnerScope { return scope.kind === 'personal' || scope.kind === 'group' || scope.kind === 'org'; } +// ── Automation write-guard (#577 Kernkonzept #6) ──────────────────────── + +/** + * Thrown when a skill-mutating call is attempted by a machine actor. Carries + * the rejected `ScopeId` so a caller (route layer, log line) can report which + * automation origin was blocked without re-deriving it from the message. + */ +export class SkillAutomationWriteBlocked extends Error { + readonly actorScope: ScopeId; + + constructor(actorScope: ScopeId) { + const origin = actorScope.kind === 'system' ? `:${actorScope.origin}` : ''; + super( + `skill mutation blocked: actor scope is a machine origin ('${actorScope.kind}${origin}') — only a live human may modify a skill`, + ); + this.name = 'SkillAutomationWriteBlocked'; + this.actorScope = actorScope; + } +} + +/** + * "Automations (crons) may not modify skills — as an enforced guard, not a + * convention" (#577 Kernkonzept #6). `ScopeId`'s own contract (`scopeId.ts`) + * is what makes this a one-line check rather than a new taxonomy: `kind: + * 'system'` is explicitly documented there as "no human is present in any of + * them" — routines, schedules and the Conductor's own automated runs. Every + * OTHER `ScopeId` kind — including `unscoped` — arises from an actual live + * turn, so `system` is the exact and only boundary this guard needs. + * + * Callers pass the ACTOR's scope (who/what is performing the write), never + * the skill's `ownerScope` (whose home it is) — those are unrelated axes; an + * org-owned skill can still be legitimately edited by a live human operator. + */ +export function assertHumanActor(actorScope: ScopeId): void { + if (actorScope.kind === 'system') { + throw new SkillAutomationWriteBlocked(actorScope); + } +} + // ── Lifecycle status ───────────────────────────────────────────────────── export const SKILL_LIFECYCLE_STATUSES = ['draft', 'reviewed', 'published', 'archived'] as const; diff --git a/middleware/src/services/skillLifecycleStore.ts b/middleware/src/services/skillLifecycleStore.ts index 675347ace..c259c50da 100644 --- a/middleware/src/services/skillLifecycleStore.ts +++ b/middleware/src/services/skillLifecycleStore.ts @@ -17,12 +17,15 @@ import { computeSkillHash } from '@omadia/orchestrator'; import { formatSessionScope, parseSessionScope, type ScopeId } from '@omadia/channel-sdk'; import { + assertHumanActor, canonicalSkillManifest, isSkillOwnerScope, requiredCapabilitiesFromFrontmatter, + signSkillManifest, transitionSkillLifecycle, type SkillLifecycleStatus, type SkillLifecycleTransitionResult, + type SkillOwnerScope, } from './skillLifecycle.js'; export interface SkillOwnershipLifecycleRow { @@ -102,13 +105,22 @@ export class PgSkillOwnershipLifecycleStore { /** * 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 + * directly in team/org scope — those homes are reached only through + * {@link promoteSkillOwnerScope} (admin-gated, P3). Refuses to reassign an + * already-owned skill (call `promoteSkillOwnerScope` for that) and refuses a * non-draft target (ownership must be settled before review begins, since * `ownerScope` is part of what gets signed). + * + * `actorScope` is who is performing the write — checked by + * {@link assertHumanActor} (#577 Kernkonzept #6) BEFORE any query runs, so + * an automation actor never even reaches the database. */ - async assignPersonalOwner(skillId: string, owner: Extract): Promise { + async assignPersonalOwner( + skillId: string, + owner: Extract, + actorScope: ScopeId, + ): Promise { + assertHumanActor(actorScope); 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'`, @@ -127,13 +139,15 @@ export class PgSkillOwnershipLifecycleStore { /** * 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. + * returns a "false-ish" result a caller could accidentally ignore. Checks + * {@link assertHumanActor} first, same as every other mutating method here. */ async transition( skillId: string, targetStatus: SkillLifecycleStatus, - opts: { readonly granted: ReadonlySet; readonly signingKey: string }, + opts: { readonly granted: ReadonlySet; readonly signingKey: string; readonly actorScope: ScopeId }, ): Promise { + assertHumanActor(opts.actorScope); const row = await this.getSkill(skillId); if (!row) throw new Error(`skill ${skillId} not found`); if (row.ownerScope === null) { @@ -163,6 +177,66 @@ export class PgSkillOwnershipLifecycleStore { if (!updated.rows[0]) throw new Error(`skill ${skillId} vanished during transition`); return mapRow(updated.rows[0]); } + + /** + * Admin-gated promotion: move an ALREADY-PUBLISHED skill to a team (`group`) + * or org home. This is the only way a skill ever reaches team/org + * ownership — #577 Kernkonzept #5 forbids creating one there directly, and + * `assignPersonalOwner` above only ever assigns `personal`. Callers are + * responsible for the "admin-gated" half (an authenticated-session check at + * the route layer, P3/P4 — this method has no notion of roles); what it + * enforces itself is: + * + * - {@link assertHumanActor} — a cron may not promote a skill, same as it + * may not do anything else to one (#577 Kernkonzept #6); + * - the skill must currently be `published` — an unreviewed draft has no + * business reaching a wider audience, and archiving/promoting are not + * composable (an archived skill must be republished first, if that ever + * becomes a supported path); + * - the manifest is re-signed at the NEW `ownerScope`, same `published` + * status — `ownerScope` is a signed field (#577 P1), so a promotion IS a + * signature-changing event, not just a column update. + */ + async promoteSkillOwnerScope( + skillId: string, + targetScope: Extract, + opts: { readonly actorScope: ScopeId; readonly signingKey: string }, + ): Promise { + assertHumanActor(opts.actorScope); + const row = await this.getSkill(skillId); + if (!row) throw new Error(`skill ${skillId} not found`); + if (row.ownerScope === null) { + throw new Error(`skill ${skillId} has no owner scope yet — assign one before promoting`); + } + if (row.lifecycleStatus !== 'published') { + throw new Error(`skill ${skillId} is not published (status: ${row.lifecycleStatus}) — only a published skill may be promoted`); + } + + const newOwnerScope = formatSessionScope(targetScope); + const contentHash = computeSkillHash(row.frontmatter, row.body); + const requiredCapabilities = requiredCapabilitiesFromFrontmatter(row.frontmatter); + const signature = signSkillManifest( + { + slug: row.slug, + name: row.name, + ownerScope: newOwnerScope, + status: row.lifecycleStatus, + contentHash, + requiredCapabilities, + }, + opts.signingKey, + ); + + const updated = await this.pool.query( + `UPDATE skills SET owner_scope = $2, manifest_signature = $3, manifest_signed_at = now(), updated_at = now() + WHERE id = $1 + RETURNING id, slug, name, frontmatter, body, owner_scope, lifecycle_status, + manifest_signature, manifest_signed_at`, + [skillId, newOwnerScope, signature], + ); + if (!updated.rows[0]) throw new Error(`skill ${skillId} vanished during promotion`); + return mapRow(updated.rows[0]); + } } /** Re-exported for callers that only need to verify a row without a Pool (e.g. a webhook). */ diff --git a/middleware/src/services/skillSharing.ts b/middleware/src/services/skillSharing.ts new file mode 100644 index 000000000..23b379113 --- /dev/null +++ b/middleware/src/services/skillSharing.ts @@ -0,0 +1,95 @@ +/** + * #577 P3 — sharing = a grant over `GrantStore` (#575), not a parallel ACL. + * + * Kernkonzept #5: "Sharing ist grant-basiert (`share` = ACL-Grant, `move` = + * Home-Wechsel)". This module is the one place that knows how a skill's + * "shared with me" fact is encoded as a `Capability` string, and how to turn + * a resolved capability set back into the `sharedSkillIds` P2's + * `resolveSkillByName` (`skillResolver.ts`) needs. It never touches + * `grants.ts`'s body — only its exported `GrantStore`/`resolveCapabilities` + * contract, per the #577 binding surface separation. + * + * ## Fail-closed, not "unresolved is a type" — and why THIS module differs + * + * `resolveCapabilities` (#575) treats a partial role lookup as `undefined` + * ("unresolved"), because collapsing it to an empty capability set would + * silently look like real policy on the audience-floor path where too few + * capabilities can wrongly widen who is excluded from a room. Skill sharing + * has the opposite risk shape: a grants-backend hiccup here can only ever + * HIDE a skill that would have been visible, never reveal one that + * shouldn't be — the personal/team/org buckets in `skillResolver.ts` are + * completely independent of this module and still work. So + * `resolveSharedSkillIds` still surfaces the `unresolved` fact (never lies + * about it), but `toSharedSkillIdsSet` gives callers who just want the + * resolver's `sharedSkillIds` input a one-line fail-closed default — + * "no visible shares right now" is a safe, honest degradation here in a way + * it is NOT on the audience floor. + */ + +import { + resolveCapabilities, + type GrantStore, + type Principal, + type RoleSourceRegistry, +} from '@omadia/channel-sdk'; + +/** The `Capability` prefix that encodes "read access to this specific skill". */ +const SHARED_SKILL_CAPABILITY_PREFIX = 'skill:read:'; + +/** The `Capability` string a grant records to share `skillId` with its holder. */ +export function sharedSkillCapability(skillId: string): string { + return `${SHARED_SKILL_CAPABILITY_PREFIX}${skillId}`; +} + +/** Extracts the skill id from a capability string, or `undefined` if it isn't a skill-share capability. */ +export function parseSharedSkillCapability(capability: string): string | undefined { + if (!capability.startsWith(SHARED_SKILL_CAPABILITY_PREFIX)) return undefined; + const id = capability.slice(SHARED_SKILL_CAPABILITY_PREFIX.length); + return id.length > 0 ? id : undefined; +} + +export type SharedSkillIdsResult = + | { readonly ok: true; readonly ids: ReadonlySet } + | { readonly ok: false; readonly reason: 'unresolved' }; + +/** + * Resolve every skill id shared with `principal`, via `GrantStore` + + * `RoleSourceRegistry` (#575/#333) — direct grants union role grants, exactly + * as `resolveCapabilities` already defines for any other capability. Returns + * `{ ok: false, reason: 'unresolved' }` when the underlying role lookup was + * partial or the store threw — same trigger as `resolveCapabilities` + * returning `undefined` — so a caller that cares CAN distinguish "nothing is + * shared" from "we couldn't ask". A caller that doesn't care uses + * {@link toSharedSkillIdsSet}. + */ +export async function resolveSharedSkillIds( + principal: Principal, + roles: RoleSourceRegistry, + grants: GrantStore, +): Promise { + const resolved = await resolveCapabilities(principal, roles, grants); + if (!resolved) return { ok: false, reason: 'unresolved' }; + + const ids = new Set(); + for (const capability of resolved.capabilities) { + const id = parseSharedSkillCapability(capability); + if (id !== undefined) ids.add(id); + } + // A capability granted AND denied is denied — same "denials win" rule the + // audience floor applies (`audienceFloor.ts`), applied per-id here since a + // skill share is a single-recipient grant, not a room-wide floor. + for (const capability of resolved.denials) { + const id = parseSharedSkillCapability(capability); + if (id !== undefined) ids.delete(id); + } + return { ok: true, ids }; +} + +/** + * Fail-closed adapter for callers (P2's `SkillResolutionContext.sharedSkillIds`) + * that want a plain set: `unresolved` collapses to empty, per this module's + * header — safe here because it can only hide a skill, never leak one. + */ +export function toSharedSkillIdsSet(result: SharedSkillIdsResult): ReadonlySet { + return result.ok ? result.ids : new Set(); +} diff --git a/middleware/test/skillLifecycle.test.ts b/middleware/test/skillLifecycle.test.ts index 27155ec97..1bad5230a 100644 --- a/middleware/test/skillLifecycle.test.ts +++ b/middleware/test/skillLifecycle.test.ts @@ -3,7 +3,9 @@ import { strict as assert } from 'node:assert'; import { SKILL_LIFECYCLE_STATUSES, + SkillAutomationWriteBlocked, SkillManifestError, + assertHumanActor, canPublishSkill, canTransitionSkillLifecycle, canonicalSkillManifest, @@ -240,6 +242,34 @@ describe('signSkillManifest / verifySkillManifestSignature', () => { }); }); +// ── Automation write-guard (#577 P3, Kernkonzept #6) ──────────────────── + +describe('assertHumanActor', () => { + it('does not throw for any non-system ScopeId kind', () => { + assert.doesNotThrow(() => assertHumanActor({ kind: 'personal', userId: 'u1' })); + assert.doesNotThrow(() => assertHumanActor({ kind: 'group', groupRef: 'team-a' })); + assert.doesNotThrow(() => assertHumanActor({ kind: 'org', orgId: 'byte5' })); + assert.doesNotThrow(() => assertHumanActor({ kind: 'conversation', conversationId: 'c1' })); + assert.doesNotThrow(() => assertHumanActor({ kind: 'unscoped', reason: 'absent' })); + }); + + it('throws SkillAutomationWriteBlocked for every system origin', () => { + for (const origin of ['routine', 'schedule', 'conductor', 'conductor-builder'] as const) { + assert.throws( + () => assertHumanActor({ kind: 'system', origin, id: 'run-1' }), + (err: unknown) => { + assert.ok(err instanceof SkillAutomationWriteBlocked); + assert.match(err.message, /machine origin/); + assert.match(err.message, new RegExp(origin)); + assert.deepEqual(err.actorScope, { kind: 'system', origin, id: 'run-1' }); + return true; + }, + origin, + ); + } + }); +}); + // ── Combined transition decision ──────────────────────────────────────── describe('transitionSkillLifecycle', () => { diff --git a/middleware/test/skillOwnershipLifecycleStore.pg.test.ts b/middleware/test/skillOwnershipLifecycleStore.pg.test.ts index 1271b4517..af0d01a45 100644 --- a/middleware/test/skillOwnershipLifecycleStore.pg.test.ts +++ b/middleware/test/skillOwnershipLifecycleStore.pg.test.ts @@ -73,16 +73,16 @@ describe('PgSkillOwnershipLifecycleStore (pg)', { skip: !pgAvailable }, () => { 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' }); + await store.assignPersonalOwner(id, { kind: 'personal', userId: 'u-1' }, { kind: 'personal', userId: 'u-actor' }); 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 store.assignPersonalOwner(id, { kind: 'personal', userId: 'u-1' }, { kind: 'personal', userId: 'u-actor' }); await assert.rejects( - () => store.assignPersonalOwner(id, { kind: 'personal', userId: 'u-2' }), + () => store.assignPersonalOwner(id, { kind: 'personal', userId: 'u-2' }, { kind: 'personal', userId: 'u-actor' }), /already has an owner scope/, ); const row = await store.getSkill(id); @@ -92,7 +92,7 @@ describe('PgSkillOwnershipLifecycleStore (pg)', { skip: !pgAvailable }, () => { 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 }), + () => store.transition(id, 'reviewed', { granted: new Set(), signingKey: KEY, actorScope: { kind: 'personal', userId: 'u-actor' } }), (err: unknown) => { assert.ok(err instanceof SkillLifecycleTransitionRejected); assert.equal(err.reason, 'invalid-owner-scope'); @@ -103,16 +103,16 @@ describe('PgSkillOwnershipLifecycleStore (pg)', { skip: !pgAvailable }, () => { 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' }); + await store.assignPersonalOwner(id, { kind: 'personal', userId: 'u-lifecycle' }, { kind: 'personal', userId: 'u-actor' }); - const reviewed = await store.transition(id, 'reviewed', { granted: new Set(), signingKey: KEY }); + const reviewed = await store.transition(id, 'reviewed', { granted: new Set(), signingKey: KEY, actorScope: { kind: 'personal', userId: 'u-actor' } }); 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 }), + () => store.transition(id, 'published', { granted: new Set(), signingKey: KEY, actorScope: { kind: 'personal', userId: 'u-actor' } }), (err: unknown) => { assert.ok(err instanceof SkillLifecycleTransitionRejected); assert.equal(err.reason, 'missing-capabilities'); @@ -126,15 +126,16 @@ describe('PgSkillOwnershipLifecycleStore (pg)', { skip: !pgAvailable }, () => { const published = await store.transition(id, 'published', { granted: new Set(['mcp.web-search']), signingKey: KEY, + actorScope: { kind: 'personal', userId: 'u-actor' }, }); 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 }); + const archived = await store.transition(id, 'archived', { granted: new Set(), signingKey: KEY, actorScope: { kind: 'personal', userId: 'u-actor' } }); assert.equal(archived.lifecycleStatus, 'archived'); await assert.rejects( - () => store.transition(id, 'draft', { granted: new Set(), signingKey: KEY }), + () => store.transition(id, 'draft', { granted: new Set(), signingKey: KEY, actorScope: { kind: 'personal', userId: 'u-actor' } }), (err: unknown) => { assert.ok(err instanceof SkillLifecycleTransitionRejected); assert.equal(err.reason, 'invalid-transition'); @@ -146,13 +147,98 @@ describe('PgSkillOwnershipLifecycleStore (pg)', { skip: !pgAvailable }, () => { 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 }); + await store.assignPersonalOwner(id, { kind: 'personal', userId: 'u-drift' }, { kind: 'personal', userId: 'u-actor' }); + const before1 = await store.transition(id, 'reviewed', { granted: new Set(), signingKey: KEY, actorScope: { kind: 'personal', userId: 'u-actor' } }); // 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 }); + const back = await store.transition(id, 'draft', { granted: new Set(), signingKey: KEY, actorScope: { kind: 'personal', userId: 'u-actor' } }); assert.notEqual(back.manifestSignature, before1.manifestSignature, 'signature tracks content_hash drift'); }); + + // ── #577 P3: automation write-guard ───────────────────────────────────── + + const CRON_ACTOR = { kind: 'system', origin: 'routine', id: 'nightly-cleanup' } as const; + + it('assignPersonalOwner refuses a machine (cron/routine) actor', async () => { + const id = await seedDraftSkill('cron-assign'); + await assert.rejects( + () => store.assignPersonalOwner(id, { kind: 'personal', userId: 'u-1' }, CRON_ACTOR), + /machine origin/, + ); + const row = await store.getSkill(id); + assert.equal(row?.ownerScope, null, 'the blocked write never reached the database'); + }); + + it('transition refuses a machine (cron/routine) actor', async () => { + const id = await seedDraftSkill('cron-transition'); + await store.assignPersonalOwner(id, { kind: 'personal', userId: 'u-1' }, { kind: 'personal', userId: 'u-actor' }); + await assert.rejects( + () => store.transition(id, 'reviewed', { granted: new Set(), signingKey: KEY, actorScope: CRON_ACTOR }), + /machine origin/, + ); + const row = await store.getSkill(id); + assert.equal(row?.lifecycleStatus, 'draft', 'the blocked write never reached the database'); + }); + + it('promoteSkillOwnerScope refuses a machine (cron/routine) actor', async () => { + const id = await seedDraftSkill('cron-promote'); + await store.assignPersonalOwner(id, { kind: 'personal', userId: 'u-1' }, { kind: 'personal', userId: 'u-actor' }); + await store.transition(id, 'reviewed', { granted: new Set(), signingKey: KEY, actorScope: { kind: 'personal', userId: 'u-actor' } }); + await store.transition(id, 'published', { granted: new Set(), signingKey: KEY, actorScope: { kind: 'personal', userId: 'u-actor' } }); + await assert.rejects( + () => + store.promoteSkillOwnerScope(id, { kind: 'org', orgId: 'byte5' }, { actorScope: CRON_ACTOR, signingKey: KEY }), + /machine origin/, + ); + const row = await store.getSkill(id); + assert.equal(row?.ownerScope, 'personal:u-1', 'the blocked promotion never reached the database'); + }); + + // ── #577 P3: admin-gated promotion ────────────────────────────────────── + + it('promoteSkillOwnerScope moves an already-published skill to an org home and re-signs it', async () => { + const id = await seedDraftSkill('promote'); + await store.assignPersonalOwner(id, { kind: 'personal', userId: 'u-1' }, { kind: 'personal', userId: 'u-actor' }); + await store.transition(id, 'reviewed', { granted: new Set(), signingKey: KEY, actorScope: { kind: 'personal', userId: 'u-actor' } }); + const published = await store.transition(id, 'published', { granted: new Set(), signingKey: KEY, actorScope: { kind: 'personal', userId: 'u-actor' } }); + + const promoted = await store.promoteSkillOwnerScope( + id, + { kind: 'org', orgId: 'byte5' }, + { actorScope: { kind: 'personal', userId: 'u-admin' }, signingKey: KEY }, + ); + assert.equal(promoted.ownerScope, 'org:byte5'); + assert.equal(promoted.lifecycleStatus, 'published', 'promotion does not change lifecycle status'); + assert.notEqual(promoted.manifestSignature, published.manifestSignature, 're-signed at the new owner scope'); + }); + + it('promoteSkillOwnerScope refuses a draft skill', async () => { + const id = await seedDraftSkill('promote-draft'); + await store.assignPersonalOwner(id, { kind: 'personal', userId: 'u-1' }, { kind: 'personal', userId: 'u-actor' }); + await assert.rejects( + () => + store.promoteSkillOwnerScope( + id, + { kind: 'org', orgId: 'byte5' }, + { actorScope: { kind: 'personal', userId: 'u-admin' }, signingKey: KEY }, + ), + /is not published/, + ); + }); + + it('promoteSkillOwnerScope can target a team (group) home, not just org', async () => { + const id = await seedDraftSkill('promote-team'); + await store.assignPersonalOwner(id, { kind: 'personal', userId: 'u-1' }, { kind: 'personal', userId: 'u-actor' }); + await store.transition(id, 'reviewed', { granted: new Set(), signingKey: KEY, actorScope: { kind: 'personal', userId: 'u-actor' } }); + await store.transition(id, 'published', { granted: new Set(), signingKey: KEY, actorScope: { kind: 'personal', userId: 'u-actor' } }); + + const promoted = await store.promoteSkillOwnerScope( + id, + { kind: 'group', groupRef: 'team-a' }, + { actorScope: { kind: 'personal', userId: 'u-admin' }, signingKey: KEY }, + ); + assert.equal(promoted.ownerScope, 'group:team-a'); + }); }); diff --git a/middleware/test/skillSharing.test.ts b/middleware/test/skillSharing.test.ts new file mode 100644 index 000000000..5e96a4e88 --- /dev/null +++ b/middleware/test/skillSharing.test.ts @@ -0,0 +1,119 @@ +import { describe, it } from 'node:test'; +import { strict as assert } from 'node:assert'; + +import { + InMemoryGrantStore, + RoleSourceRegistry, + type Principal, + type RoleLookup, + type RoleSource, +} from '@omadia/channel-sdk'; + +import { + parseSharedSkillCapability, + resolveSharedSkillIds, + sharedSkillCapability, + toSharedSkillIdsSet, +} from '../src/services/skillSharing.js'; + +const ALICE: Principal = { kind: 'user', userId: 'alice' }; + +describe('sharedSkillCapability / parseSharedSkillCapability', () => { + it('round-trips a skill id through the capability string', () => { + const cap = sharedSkillCapability('skill-123'); + assert.equal(cap, 'skill:read:skill-123'); + assert.equal(parseSharedSkillCapability(cap), 'skill-123'); + }); + + it('returns undefined for a capability that is not a skill-share', () => { + assert.equal(parseSharedSkillCapability('mcp.web-search'), undefined); + assert.equal(parseSharedSkillCapability('skill:write:x'), undefined); + }); + + it('returns undefined for an empty id (malformed capability, never a fake empty-string id)', () => { + assert.equal(parseSharedSkillCapability('skill:read:'), undefined); + }); +}); + +describe('resolveSharedSkillIds', () => { + it('resolves ids granted directly to the principal', async () => { + const grants = new InMemoryGrantStore().grantToPrincipal( + ALICE, + sharedSkillCapability('skill-a'), + sharedSkillCapability('skill-b'), + ); + const roles = new RoleSourceRegistry(); + const result = await resolveSharedSkillIds(ALICE, roles, grants); + assert.deepEqual(result, { ok: true, ids: new Set(['skill-a', 'skill-b']) }); + }); + + it('ignores non-skill capabilities granted alongside real ones', async () => { + const grants = new InMemoryGrantStore().grantToPrincipal( + ALICE, + sharedSkillCapability('skill-a'), + 'mcp.web-search', + ); + const roles = new RoleSourceRegistry(); + const result = await resolveSharedSkillIds(ALICE, roles, grants); + assert.deepEqual(result, { ok: true, ids: new Set(['skill-a']) }); + }); + + it('unions role grants with direct grants', async () => { + const fakeSource: RoleSource = { + id: 'test-source', + displayName: 'Test', + rolesFor: async (): Promise => ({ outcome: 'resolved', roles: ['editors'] }), + }; + const roles = new RoleSourceRegistry(); + roles.register(fakeSource); + + const grants = new InMemoryGrantStore() + .grantToPrincipal(ALICE, sharedSkillCapability('skill-a')) + .grantToRole('editors', sharedSkillCapability('skill-b')); + + const result = await resolveSharedSkillIds(ALICE, roles, grants); + assert.deepEqual(result, { ok: true, ids: new Set(['skill-a', 'skill-b']) }); + }); + + it('a denial removes an otherwise-granted skill id', async () => { + const grants = new InMemoryGrantStore() + .grantToPrincipal(ALICE, sharedSkillCapability('skill-a'), sharedSkillCapability('skill-b')) + .denyToPrincipal(ALICE, sharedSkillCapability('skill-b')); + const roles = new RoleSourceRegistry(); + const result = await resolveSharedSkillIds(ALICE, roles, grants); + assert.deepEqual(result, { ok: true, ids: new Set(['skill-a']) }); + }); + + it('returns unresolved when a role source is partial (never silently empties the set)', async () => { + const throwingSource: RoleSource = { + id: 'broken-source', + displayName: 'Broken', + rolesFor: async (): Promise => { + throw new Error('directory unreachable'); + }, + }; + const roles = new RoleSourceRegistry(); + roles.register(throwingSource); + const grants = new InMemoryGrantStore().grantToPrincipal(ALICE, sharedSkillCapability('skill-a')); + + const result = await resolveSharedSkillIds(ALICE, roles, grants); + assert.deepEqual(result, { ok: false, reason: 'unresolved' }); + }); + + it('resolves to an empty set (not unresolved) when nothing is granted', async () => { + const grants = new InMemoryGrantStore(); + const roles = new RoleSourceRegistry(); + const result = await resolveSharedSkillIds(ALICE, roles, grants); + assert.deepEqual(result, { ok: true, ids: new Set() }); + }); +}); + +describe('toSharedSkillIdsSet', () => { + it('passes through the ids on a resolved result', () => { + assert.deepEqual(toSharedSkillIdsSet({ ok: true, ids: new Set(['a']) }), new Set(['a'])); + }); + + it('fails closed to an empty set on unresolved', () => { + assert.deepEqual(toSharedSkillIdsSet({ ok: false, reason: 'unresolved' }), new Set()); + }); +});