From 100d996e28c5cc7fcbe3dbd167a786bcb2712d48 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 25 Oct 2026 06:34:11 +0000 Subject: [PATCH 1/2] fix(personal-kits): fail closed on @agentkit overlays while the shared list is unset Kit releases are named without the ak- prefix, so the prefix rule alone protected no @agentkit kit release while SKILLX_SHARED_SKILL_ALIASES was unset. Until that list holds at least one valid identity, every in-place overlay of an @agentkit/* identity is now refused with 422 overlay.shared_alias, and the detail names the missing operator step. Entries that are not identities no longer count as a configured list. Forks are unaffected. Closes #125 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016RCDbVS52poV8kxgnXrrBY --- apps/web/app/lib/personal-kits/kit-verify.ts | 19 ++++++++++++++++--- .../tests/personal-kits/kit-revisions.test.ts | 17 ++++++++++++++--- .../personal-kits/proposal-detectors.test.ts | 19 ++++++++++++------- docs/deployment-guide.md | 5 ++++- docs/personal-kits-api.md | 12 ++++++++++-- 5 files changed, 56 insertions(+), 16 deletions(-) diff --git a/apps/web/app/lib/personal-kits/kit-verify.ts b/apps/web/app/lib/personal-kits/kit-verify.ts index 5171a54..68dd49d 100644 --- a/apps/web/app/lib/personal-kits/kit-verify.ts +++ b/apps/web/app/lib/personal-kits/kit-verify.ts @@ -21,6 +21,7 @@ import { sharedAliasViolation, } from "@skillx/contracts"; import type { PersonalKitDocument } from "@skillx/contracts"; +import { IDENTITY_PATTERN } from "@skillx/skill-tree/rules"; import { baseFailure, loadPinnedBase } from "./base-loader"; import type { LoadedBase } from "./base-loader"; import type { KitCaller } from "./kit-http"; @@ -38,17 +39,25 @@ const UNUSABLE_PARENT_STATES = new Set(["proposed", "rejected", "quarantined"]); /** AgentKit's reserved alias prefixes (`ak-`, `ak:`, any case). */ const RESERVED_ALIAS = /^ak[-:]/i; const nameOf = (identity: string) => identity.slice(identity.indexOf("/") + 1); +const reservedSkill = (identity: string, alias: string) => RESERVED_ALIAS.test(alias) || RESERVED_ALIAS.test(nameOf(identity)); +const OFFICIAL_SCOPE = "@agentkit/"; +/** The configured identities; entries that are not identities (aliases, typos, `none`) are dropped. */ +const sharedSkillList = (env: { SKILLX_SHARED_SKILL_ALIASES?: string }) => + new Set((env.SKILLX_SHARED_SKILL_ALIASES ?? "").split(/[\s,]+/).filter((s) => IDENTITY_PATTERN.test(s))); /** * Skills that may not be overlaid in place: those owned by AgentKit * `kits/core` or shipped by more than one official kit. Fails closed: any * skill whose directory alias or name starts with `ak-` or `ak:` is always * shared, and `SKILLX_SHARED_SKILL_ALIASES` (comma or whitespace separated - * identities) adds to that default. + * identities) adds to that default. While that list holds no valid + * identity, every `@agentkit/*` identity is shared too: kit releases carry + * no `ak-` prefix, so the prefix rule alone would protect none of them. */ export function sharedSkills(env: { SKILLX_SHARED_SKILL_ALIASES?: string }): (identity: string, alias: string) => boolean { - const listed = new Set((env.SKILLX_SHARED_SKILL_ALIASES ?? "").split(/[\s,]+/).filter((s) => s !== "")); - return (identity, alias) => listed.has(identity) || RESERVED_ALIAS.test(alias) || RESERVED_ALIAS.test(nameOf(identity)); + const listed = sharedSkillList(env); + return (identity, alias) => + listed.has(identity) || reservedSkill(identity, alias) || (listed.size === 0 && identity.startsWith(OFFICIAL_SCOPE)); } /** A foreign parent must be a public, activated, reviewed revision that was authored public. */ @@ -115,6 +124,10 @@ export async function verifyDocument(caller: KitCaller, kitName: string, doc: Pe const { env, userId } = caller; if (doc.kit !== kitName) throw kitError(KIT_CODES.documentInvalid, "kit", `document names ${doc.kit}, not ${kitName}`); const sharedViolation = sharedAliasViolation(doc, sharedSkills(env)); + // Name the operator step only when the unset list, not an ak- prefix, refused the first entry. + if (sharedViolation !== null && sharedSkillList(env).size === 0 && sharedAliasViolation(doc, reservedSkill)?.path !== sharedViolation.path) { + throw kitError(KIT_CODES.sharedAlias, sharedViolation.path, `${sharedViolation.detail}; the operator has not configured SKILLX_SHARED_SKILL_ALIASES, so every ${OFFICIAL_SCOPE}* skill is treated as shared until it is set`); + } if (sharedViolation !== null) throw sharedViolation; const bases = new Map(); const warnings: string[] = []; diff --git a/apps/web/tests/personal-kits/kit-revisions.test.ts b/apps/web/tests/personal-kits/kit-revisions.test.ts index da08c71..5dd98a2 100644 --- a/apps/web/tests/personal-kits/kit-revisions.test.ts +++ b/apps/web/tests/personal-kits/kit-revisions.test.ts @@ -122,15 +122,26 @@ describe("personal kit revisions", () => { expect(res.status).toBe(422); expect(res.body.error.code).toBe("overlay.shared_alias"); - // Unset, the guard fails closed on AgentKit's ak- / ak: skills; set, the list adds to that default. + // Unset, the guard fails closed on every @agentkit skill and names the missing operator step. + const unset = await k.call("revisions", { expectedGeneration: 1, idempotencyKey: k.idem(), document: official }); + expect(unset.status).toBe(422); + expect(unset.body.error.code).toBe("overlay.shared_alias"); + expect(JSON.stringify(unset.body.error)).toContain("SKILLX_SHARED_SKILL_ALIASES"); + expect(JSON.stringify(res.body.error)).not.toContain("SKILLX_SHARED_SKILL_ALIASES"); + // ak- / ak: skills stay shared whether or not the list is set. const ak = (name: string) => k.doc([{ ...mine, identity: `@agentkit/${name}`, base: { ...mine.base, package: `@agentkit/${name}` } }]); for (const [document, o] of [[ak("ak-plan"), {}], [ak("ak-cook"), {}], [ak("ak-plan"), shared]] as const) { const refused = await k.call("revisions", { expectedGeneration: 1, idempotencyKey: k.idem(), document }, o); expect(refused.status).toBe(422); expect(refused.body.error.code).toBe("overlay.shared_alias"); + expect(JSON.stringify(refused.body.error)).not.toContain("SKILLX_SHARED_SKILL_ALIASES"); } - // Other official skills pass the guard unless listed (this one then fails only on its unpublished base). - const unlisted = await k.call("revisions", { expectedGeneration: 1, idempotencyKey: k.idem(), document: official }); + // A fork is never an in-place overlay, so it passes the guard while the list is unset. + const fork = k.doc([{ ...official.skills[0], identity: "@alpha/plan-mine", mode: "fork", alias: "plan-mine" }]); + const forked = await k.call("revisions", { expectedGeneration: 1, idempotencyKey: k.idem(), document: fork }); + expect(forked.body.error.code).toBe("kit.base_unavailable"); + // Once the list is set, other official skills pass the guard unless listed (this one then fails only on its unpublished base). + const unlisted = await k.call("revisions", { expectedGeneration: 1, idempotencyKey: k.idem(), document: official }, { env: { SKILLX_SHARED_SKILL_ALIASES: "@agentkit/cook" } }); expect(unlisted.body.error.code).toBe("kit.base_unavailable"); }); diff --git a/apps/web/tests/personal-kits/proposal-detectors.test.ts b/apps/web/tests/personal-kits/proposal-detectors.test.ts index f4fe39a..ae086a7 100644 --- a/apps/web/tests/personal-kits/proposal-detectors.test.ts +++ b/apps/web/tests/personal-kits/proposal-detectors.test.ts @@ -51,18 +51,23 @@ describe("forbidden keys through the envelope", () => { }); describe("shared skill guard", () => { - it("treats every ak- and ak: skill as shared when unset, and adds the configured list", () => { - const unset = sharedSkills({}); - expect(unset("@agentkit/ak-plan", "ak-plan")).toBe(true); - expect(unset("@agentkit/plan", "AK:plan")).toBe(true); - expect(unset("@agentkit/AK-x", "x")).toBe(true); - expect(unset("@agentkit/plan", "plan")).toBe(false); - expect(unset("@agentkit/akplan", "akplan")).toBe(false); + it("treats every @agentkit, ak- and ak: skill as shared when unset, and only ak- plus the list once configured", () => { + // Aliases, typos and mixed case are not identities, so they leave the list empty. + for (const unset of [sharedSkills({}), sharedSkills({ SKILLX_SHARED_SKILL_ALIASES: " ,\n" }), sharedSkills({ SKILLX_SHARED_SKILL_ALIASES: "plan none @AgentKit/cook" })]) { + expect(unset("@agentkit/ak-plan", "ak-plan")).toBe(true); + expect(unset("@agentkit/plan", "AK:plan")).toBe(true); + expect(unset("@agentkit/plan", "plan")).toBe(true); + expect(unset("@agentkit/akplan", "renamed")).toBe(true); + expect(unset("@alpha/plan", "plan")).toBe(false); + expect(unset("@alpha/AK-x", "x")).toBe(true); + expect(unset("@alpha/agentkit", "agentkit")).toBe(false); + } const listed = sharedSkills({ SKILLX_SHARED_SKILL_ALIASES: "@agentkit/plan,\n@agentkit/cook" }); expect(listed("@agentkit/plan", "plan")).toBe(true); expect(listed("@agentkit/cook", "cook")).toBe(true); expect(listed("@agentkit/ak-review", "ak-review")).toBe(true); expect(listed("@agentkit/other", "other")).toBe(false); + expect(listed("@agentkit/akplan", "akplan")).toBe(false); }); }); diff --git a/docs/deployment-guide.md b/docs/deployment-guide.md index 7029bb3..e326e6a 100644 --- a/docs/deployment-guide.md +++ b/docs/deployment-guide.md @@ -470,7 +470,10 @@ document in R2 under `statuses/sha256/`. Rollback: the header SQL of **Personal kits API.** `/api/v1/kits/...` needs migrations 0015 and 0016 (personal kit tables and proposals) plus the release read stack (`SKILLX_RELEASES_API`). Production runs with `SKILLX_PERSONAL_KITS_API` set to -`"1"` in `apps/web/wrangler.jsonc`. Before enabling it on another deployment, +`"1"` in `apps/web/wrangler.jsonc`. Until the Worker var +`SKILLX_SHARED_SKILL_ALIASES` lists at least one identity, every in-place +overlay of an `@agentkit/*` skill answers `422 overlay.shared_alias` (see the +[personal kits API](./personal-kits-api.md)). Before enabling it on another deployment, confirm that `wrangler d1 migrations list skillx-db --remote` reports nothing pending. Rollback: set the flag to `"0"` and redeploy; every route answers 404 and stored kits stay untouched. diff --git a/docs/personal-kits-api.md b/docs/personal-kits-api.md index 0361660..dd72089 100644 --- a/docs/personal-kits-api.md +++ b/docs/personal-kits-api.md @@ -93,8 +93,16 @@ without an explicit user approval. closed: every skill whose name or directory alias starts with `ak-` or `ak:` (any case) is always shared, whether or not `SKILLX_SHARED_SKILL_ALIASES` is set. That variable (comma or whitespace - separated identities) adds further shared skills to this default. Other - `@agentkit/*` skills may be overlaid. AgentKit kit composition owns the + separated identities) adds further shared skills to this default; entries + that are not identities (bare aliases such as `plan`, mixed case) are + ignored. While it holds no valid identity, every `@agentkit/*` identity is + shared too, because kit releases carry no `ak-` prefix (`@agentkit/plan`); + the 422 detail then says the operator has not configured the variable. The + guard runs on every revision write (manual, rebase, conflict resolve, + proposal), not on activate, rollback or lock, so a stored revision that + already overlays such a skill stays usable but cannot be rebased until the + overlay becomes a fork. Once the variable is set, other `@agentkit/*` + skills may be overlaid. AgentKit kit composition owns the canonical list: the non-`ak-` skills that `kits/core` or more than one official kit ships. Operators set the variable from that list; SkillX never derives it. From f6b5a310d7911987a1771d7418c3f5849150b15d Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 25 Oct 2026 06:39:04 +0000 Subject: [PATCH 2/2] docs(deploy): describe the fail-closed shared-skill guard before kit releases Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016RCDbVS52poV8kxgnXrrBY --- docs/deployment-guide.md | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/docs/deployment-guide.md b/docs/deployment-guide.md index 2e68f03..07ae5c3 100644 --- a/docs/deployment-guide.md +++ b/docs/deployment-guide.md @@ -483,13 +483,13 @@ pending. Rollback: set the flag to `"0"` and redeploy; every route answers 404 AgentKit's shared-skill list first: the non-`ak-` skills that `kits/core` or more than one official kit ships (for example `@agentkit/plan`). Kit releases are published without the `ak-` prefix (`@agentkit/plan`; `@agentkit/ak-*` is -`400 identity.invalid`), so only the `ak-` names are shared by default. While -the variable is unset, a public personal kit may overlay any other -`@agentkit/` skill in place. That is by design -([personal kits API](./personal-kits-api.md)) and the result stays -`local-unverified`, but it lets a third-party kit override a skill every -official kit shares. Confirm with an overlay of a listed identity, which must -answer `422 overlay.shared_alias`. Kit releases also need skillx-sh 0.7.0 or +`400 identity.invalid`). While the variable holds no valid identity, the +guard fails closed and refuses every in-place overlay of an `@agentkit/*` +skill, including single-kit skills that may legitimately be overlaid +([personal kits API](./personal-kits-api.md)); setting the list is what +unblocks those. Use identities, not bare aliases: an entry such as `plan` is +ignored. Confirm with an overlay of a listed identity, which must answer +`422 overlay.shared_alias` without naming the variable in its detail. Kit releases also need skillx-sh 0.7.0 or later on the client: 0.6.0 and earlier report `skillx.release/v2` manifests as failed verification, so say so in the release notes of the first kit release.