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 d15ed39..b7add9d 100644 --- a/apps/web/tests/personal-kits/kit-revisions.test.ts +++ b/apps/web/tests/personal-kits/kit-revisions.test.ts @@ -144,15 +144,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 a7b2f66..07ae5c3 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 `kit.api_disabled` and stored kits stay untouched. @@ -480,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. diff --git a/docs/personal-kits-api.md b/docs/personal-kits-api.md index 22e7650..2240b0e 100644 --- a/docs/personal-kits-api.md +++ b/docs/personal-kits-api.md @@ -98,8 +98,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.