fix(personal-kits): fail closed on @agentkit overlays while the shared list is unset

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<string, LoadedBase>();
   const warnings: string[] = [];

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");
   });
 

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);
   });
 });
 

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/<name>` 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.
 

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.