feat: member-derived skillset visibility (#1136)

.changeset/skillset-derived-visibility.md

@@ -0,0 +1,6 @@
+---
+"ornn-api": minor
+"ornn-web": minor
+---
+
+Skillset visibility is now derived from its member skills, never owner-set: a skillset is readable, usable, and discoverable only by callers who can read every member. Owners can no longer set a skillset's visibility — to widen reach, expose the underlying skills. The detail page shows a read-only Public/Restricted/Broken badge and warns the owner (with an in-product notification) when a member skill becomes unreadable and shrinks the skillset's reach.

ornn-api/src/bootstrap.ts

@@ -738,6 +738,12 @@ export async function bootstrap(
 
   // Skill routes — sharing is now a direct PUT /permissions write; the
   // audit signal is surfaced as a per-version label, not a gate.
+  // #1136 — forward reference: the skill routes fire a reactive skillset
+  // recompute on every visibility-affecting mutation, but `wireSkillsets`
+  // (which owns that recompute) is built below since it injects this skill
+  // service. The holder is filled in once skillsets is wired; the hook is
+  // only ever invoked at request time, long after boot.
+  const skillsetRecomputeHook: { fire?: (s: { guid: string; name: string }) => void } = {};
   const skillRoutes = createSkillRoutes({
     skillService,
     skillRepo,
@@ -755,6 +761,7 @@ export async function bootstrap(
     // backed by the lazily-populated user directory.
     resolveUser: async (userId) =>
       (await userDirectoryRepo.findByUserIds([userId]))[0] ?? null,
+    fireSkillsetRecompute: (changedSkill) => skillsetRecomputeHook.fire?.(changedSkill),
   });
 
   // ---- Domain: Skill Search ----
@@ -780,8 +787,17 @@ export async function bootstrap(
     // #1123 — transfer-ownership target validation, shared resolver.
     resolveUser: async (userId) =>
       (await userDirectoryRepo.findByUserIds([userId]))[0] ?? null,
+    // #1136 — owner notifications on member-access loss during recompute.
+    notificationService,
   });
+  // #1136 — bind the skill routes' reactive-recompute hook now that the
+  // skillset wiring (which owns it) exists.
+  skillsetRecomputeHook.fire = skillsets.fireSkillsetRecompute;
   await skillsets.ensureIndexes();
+  // #1136 — one-shot, idempotent backfill of the derived-visibility cache so
+  // skillsets created before the feature get correct `membersAllPublic` /
+  // `memberVisibilityState` without a manual migration.
+  await skillsets.backfillDerivedVisibility();
 
   // ---- Domain: Skill Generation ----
   const { service: generationService, routes: generationRoutes } =

ornn-api/src/domains/notifications/migration.test.ts

@@ -66,10 +66,13 @@ describe("dropLegacyNotificationCategories", () => {
       makeRow("legacy-1", "share.needs_justification"),
       makeRow("legacy-2", "share.needs_justification"),
       makeRow("legacy-3", "share.granted"),
-      // Current vocabulary — must survive.
+      // Current vocabulary — must survive (incl. the categories that the
+      // hand-maintained allow-list used to drop on reboot, #1136).
       makeRow("keep-1", "audit.completed"),
       makeRow("keep-2", "audit.risky_for_consumer"),
       makeRow("keep-3", "quota.credits_granted"),
+      makeRow("keep-4", "launchPromo.codeDelivered"),
+      makeRow("keep-5", "skillset.member_unreadable"),
     ]);
 
     await dropLegacyNotificationCategories(db);
@@ -83,13 +86,17 @@ describe("dropLegacyNotificationCategories", () => {
       "keep-1",
       "keep-2",
       "keep-3",
+      "keep-4",
+      "keep-5",
     ]);
-    // Every survivor is in the allowed vocabulary.
+    // Every survivor is in the canonical allowed vocabulary.
     for (const doc of remaining) {
       expect([
         "audit.completed",
         "audit.risky_for_consumer",
         "quota.credits_granted",
+        "launchPromo.codeDelivered",
+        "skillset.member_unreadable",
       ]).toContain(doc.category);
     }
   });

ornn-api/src/domains/notifications/migration.ts

@@ -3,11 +3,11 @@
  * out-of-vocabulary notifications.
  *
  * PR #198 removed the share/audit-gate workflow. The `NotificationCategory`
- * type was tightened at the same time:
+ * type was tightened at the same time. The current allowed vocabulary is
+ * the canonical `NOTIFICATION_CATEGORIES` list (imported below), so this
+ * migration and the type can never drift apart.
  *
- *   audit.completed | audit.risky_for_consumer | quota.credits_granted
- *
- * but pre-#198 rows in the `notifications` collection still carry dead
+ * Pre-#198 rows in the `notifications` collection still carry dead
  * categories like `share.needs_justification` and dead deep-links into
  * the removed `/shares/*` route tree (which now 404). They surface from
  * `GET /api/v1/notifications` and look broken to the user.
@@ -21,14 +21,15 @@
 
 import type { Db } from "mongodb";
 import { createLogger } from "../../shared/logger";
+import { NOTIFICATION_CATEGORIES } from "./types";
 
 const logger = createLogger("notificationsMigration");
 
-const ALLOWED_CATEGORIES = [
-  "audit.completed",
-  "audit.risky_for_consumer",
-  "quota.credits_granted",
-] as const;
+// Derive the allow-list from the canonical vocabulary (#1136) so a newly
+// added category is never deleted on the next boot. Previously this was a
+// hand-maintained copy that had already drifted (missing
+// `launchPromo.codeDelivered`).
+const ALLOWED_CATEGORIES = NOTIFICATION_CATEGORIES;
 
 export async function dropLegacyNotificationCategories(db: Db): Promise<void> {
   const collection = db.collection("notifications");

ornn-api/src/domains/notifications/service.test.ts

@@ -640,6 +640,44 @@ describe("NotificationService — emitters", () => {
     });
   });
 
+  test("notifySkillsetMemberUnreadable pins the owner-side payload (#1136)", async () => {
+    const { svc, notificationRepo } = makeService();
+    await svc.notifySkillsetMemberUnreadable({
+      ownerUserId: "owner-9",
+      skillsetGuid: "ss guid",
+      skillsetName: "My Bundle",
+      unreadableMembers: ["secret-tools@1.0"],
+    });
+    expect(notificationRepo.created).toHaveLength(1);
+    const sent = notificationRepo.created[0]!;
+    expect(sent.userId).toBe("owner-9");
+    expect(sent.category).toBe("skillset.member_unreadable");
+    expect(sent.title).toBe('A skill in your skillset "My Bundle" is no longer accessible to you');
+    // Single member → singular phrasing + the member listed.
+    expect(sent.body).toContain("1 member skill is");
+    expect(sent.body).toContain("secret-tools@1.0");
+    // Guid is URL-encoded into the deep link.
+    expect(sent.link).toBe("/skillsets/ss%20guid");
+    expect(sent.data).toEqual({
+      skillsetGuid: "ss guid",
+      skillsetName: "My Bundle",
+      unreadableMembers: ["secret-tools@1.0"],
+    });
+  });
+
+  test("notifySkillsetMemberUnreadable pluralizes for multiple members (#1136)", async () => {
+    const { svc, notificationRepo } = makeService();
+    await svc.notifySkillsetMemberUnreadable({
+      ownerUserId: "owner-9",
+      skillsetGuid: "g",
+      skillsetName: "Bundle",
+      unreadableMembers: ["a@1.0", "b@2.0"],
+    });
+    const sent = notificationRepo.created[0]!;
+    expect(sent.body).toContain("2 member skills are");
+    expect(sent.body).toContain("a@1.0, b@2.0");
+  });
+
   test("notifyQuotaCreditsGranted — with a note inlines the note in the body", async () => {
     const { svc, notificationRepo } = makeService();
     await svc.notifyQuotaCreditsGranted({

ornn-api/src/domains/notifications/service.ts

@@ -409,6 +409,39 @@ export class NotificationService {
     });
   }
 
+  /**
+   * Owner-side notification (#1136) fired when a member skill's visibility
+   * change costs the skillset OWNER read access — so the owner knows their
+   * skillset's reach shrank and can repair it (re-grant the skill, or
+   * publish a version without it). Coalesced to ONE notification per
+   * skillset per recompute (the caller passes the full unreadable set).
+   */
+  async notifySkillsetMemberUnreadable(params: {
+    ownerUserId: string;
+    skillsetGuid: string;
+    skillsetName: string;
+    unreadableMembers: string[];
+  }): Promise<void> {
+    const count = params.unreadableMembers.length;
+    const title = `A skill in your skillset "${params.skillsetName}" is no longer accessible to you`;
+    const body =
+      `${count === 1 ? "1 member skill is" : `${count} member skills are`} no longer readable by you, ` +
+      `so this skillset can no longer be used or discovered by the people who could before. ` +
+      `Re-gain access to the skill(s), or publish a new version of the skillset without them. ` +
+      `Affected: ${params.unreadableMembers.join(", ")}.`;
+    await this.emit(params.ownerUserId, {
+      category: "skillset.member_unreadable",
+      title,
+      body,
+      link: `/skillsets/${encodeURIComponent(params.skillsetGuid)}`,
+      data: {
+        skillsetGuid: params.skillsetGuid,
+        skillsetName: params.skillsetName,
+        unreadableMembers: params.unreadableMembers,
+      },
+    });
+  }
+
   private async emit(
     userId: string,
     payload: {

ornn-api/src/domains/notifications/types.ts

@@ -20,11 +20,24 @@
  * @module domains/notifications/types
  */
 
-export type NotificationCategory =
-  | "audit.completed"
-  | "audit.risky_for_consumer"
-  | "quota.credits_granted"
-  | "launchPromo.codeDelivered";
+/**
+ * Canonical notification-category vocabulary — the SINGLE source of truth.
+ * Both the `NotificationCategory` type and the boot migration's allow-list
+ * (`dropLegacyNotificationCategories`) derive from this array, so adding a
+ * category here can never be silently wiped by the out-of-vocabulary
+ * cleanup (the drift that previously left `launchPromo.codeDelivered`
+ * deletable on reboot — #1136).
+ */
+export const NOTIFICATION_CATEGORIES = [
+  "audit.completed",
+  "audit.risky_for_consumer",
+  "quota.credits_granted",
+  "launchPromo.codeDelivered",
+  // A member skill became unreadable to the skillset owner (#1136).
+  "skillset.member_unreadable",
+] as const;
+
+export type NotificationCategory = (typeof NOTIFICATION_CATEGORIES)[number];
 
 export interface NotificationDocument {
   readonly _id: string;

ornn-api/src/domains/skills/closure/resolver.ts

@@ -51,6 +51,13 @@ export interface ResolvedVersion {
   guid?: string;
   /** Package hash for the resolved version, when known. */
   skillHash?: string;
+  /**
+   * Whether the underlying skill is private, when known (#1136). Carried
+   * so callers resolving under SYSTEM can classify member visibility
+   * (public vs private vs unresolvable) without a second skill lookup.
+   * The closure algorithm itself ignores it.
+   */
+  isPrivate?: boolean;
   /** Direct dependency refs declared by this version. */
   dependsOn: string[];
 }

ornn-api/src/domains/skills/crud/routes.test.ts

@@ -121,6 +121,8 @@ interface BuildOpts {
   resolveUser?: (
     userId: string,
   ) => Promise<{ userId: string; email: string; displayName: string } | null>;
+  /** #1136 — capture reactive skillset-recompute hook invocations. */
+  onSkillsetRecompute?: (changedSkill: { guid: string; name: string }) => void;
 }
 
 function buildApp(opts: BuildOpts = {}) {
@@ -134,6 +136,7 @@ function buildApp(opts: BuildOpts = {}) {
     nyxidServiceClient,
     extraNyxidServices = [],
     resolveUser,
+    onSkillsetRecompute,
   } = opts;
 
   const skillRepo = {
@@ -150,6 +153,7 @@ function buildApp(opts: BuildOpts = {}) {
       { findVisibleToCaller: async () => null }) as unknown as SkillRoutesConfig["nyxidServiceClient"],
     extraNyxidServicesResolver: async () => extraNyxidServices,
     ...(resolveUser ? { resolveUser } : {}),
+    ...(onSkillsetRecompute ? { fireSkillsetRecompute: onSkillsetRecompute } : {}),
   };
 
   const app = new Hono();
@@ -1029,6 +1033,28 @@ describe("PUT /skills/:id/permissions", () => {
     expect(res.status).toBe(200);
     expect(calls).toContain("setSkillPermissions");
   });
+
+  test("fires the skillset recompute hook after a permissions change (#1136)", async () => {
+    const recomputed: Array<{ guid: string; name: string }> = [];
+    const app = buildApp({
+      userId: OWNER,
+      permissions: [UPDATE],
+      repo: { findByGuid: async () => skillDoc({ createdBy: OWNER }) },
+      service: {
+        setSkillPermissions: async () => detail({ createdBy: OWNER }),
+        getSkill: async () => detail({ createdBy: OWNER }),
+      },
+      onSkillsetRecompute: (s) => recomputed.push(s),
+    });
+    const res = await app.request("/api/v1/skills/guid-1/permissions", {
+      method: "PUT",
+      headers: { "content-type": "application/json" },
+      body: JSON.stringify({ isPrivate: false, sharedWithUsers: [], sharedWithOrgs: [] }),
+    });
+    expect(res.status).toBe(200);
+    expect(recomputed).toHaveLength(1);
+    expect(recomputed[0]!.guid).toBe("guid-1");
+  });
 });
 
 // ======================================================================
@@ -1323,6 +1349,21 @@ describe("DELETE /skills/:id", () => {
     const body = (await res.json()) as { data: { success: boolean } };
     expect(body.data.success).toBe(true);
   });
+
+  test("fires the skillset recompute hook with the deleted skill's name+guid (#1136)", async () => {
+    const recomputed: Array<{ guid: string; name: string }> = [];
+    const app = buildApp({
+      userId: OWNER,
+      permissions: [DELETE],
+      repo: { findByGuid: async () => skillDoc({ createdBy: OWNER }) },
+      service: { deleteSkill: async () => {} },
+      onSkillsetRecompute: (s) => recomputed.push(s),
+    });
+    const res = await app.request("/api/v1/skills/guid-1", { method: "DELETE" });
+    expect(res.status).toBe(200);
+    // Name captured before deletion so the reverse index still matches.
+    expect(recomputed).toEqual([{ guid: "guid-1", name: "demo-skill" }]);
+  });
 });
 
 // ======================================================================

ornn-api/src/domains/skills/crud/routes.ts

@@ -177,6 +177,14 @@ export interface SkillRoutesConfig {
   resolveUser?: (
     userId: string,
   ) => Promise<{ userId: string; email: string; displayName: string } | null>;
+  /**
+   * #1136 — fire-and-forget skillset derived-visibility recompute. Called
+   * after any skill mutation that changes the skill's readability (privacy
+   * flip, permission change, ownership transfer, nyxid-service bind,
+   * delete), so every skillset referencing the skill updates its derived
+   * visibility and owners are notified on access loss. No-op when unset.
+   */
+  fireSkillsetRecompute?: (changedSkill: { guid: string; name: string }) => void;
 }
 
 /** Marker prefix for synthetic NyxID-service ids. See `extraNyxidServices`. */
@@ -193,6 +201,7 @@ export function createSkillRoutes(config: SkillRoutesConfig): Hono<{ Variables:
     extraNyxidServicesResolver,
     mirrorService,
     resolveUser,
+    fireSkillsetRecompute,
   } = config;
   const app = new Hono<{ Variables: AuthVariables }>();
 
@@ -1108,6 +1117,12 @@ export function createSkillRoutes(config: SkillRoutesConfig): Hono<{ Variables:
       // umbrella decides between publish vs remove based on the new
       // `isPrivate` state.
       fireMirrorSync(guid);
+      // Only a privacy flip changes who can read the skill, so recompute
+      // dependent skillsets only then (#1136) — a content-only update
+      // leaves readability untouched.
+      if (isPrivate !== undefined) {
+        fireSkillsetRecompute?.({ guid, name: result.name });
+      }
 
       return c.json({ data: result, error: null });
     },
@@ -1174,6 +1189,8 @@ export function createSkillRoutes(config: SkillRoutesConfig): Hono<{ Variables:
       // Permissions change can flip eligibility — sync handles both
       // public→private (remove from mirror) and private→public (add).
       fireMirrorSync(guid);
+      // Grants change who can read the skill → recompute dependent skillsets (#1136).
+      fireSkillsetRecompute?.({ guid, name: updated.name });
 
       return c.json({ data: { skill: updated }, error: null });
     },
@@ -1241,6 +1258,9 @@ export function createSkillRoutes(config: SkillRoutesConfig): Hono<{ Variables:
       // Owner change doesn't affect mirror eligibility, but the cached author
       // labels on the mirror copy are now stale — resync to refresh them.
       fireMirrorSync(guid);
+      // Author change shifts read access (the prior owner keeps READ, but a
+      // skillset owner who relied on authoring it may now lose it) — recompute (#1136).
+      fireSkillsetRecompute?.({ guid, name: updated.name });
 
       return c.json({ data: { skill: updated }, error: null });
     },
@@ -1321,6 +1341,8 @@ export function createSkillRoutes(config: SkillRoutesConfig): Hono<{ Variables:
       // skill becomes mirror-eligible. Conversely an untie can leave a
       // system skill back at private. Sync handles both directions.
       fireMirrorSync(guid);
+      // A tie/untie can flip the skill's visibility → recompute skillsets (#1136).
+      fireSkillsetRecompute?.({ guid, name: updated.name });
 
       return c.json({ data: { skill: updated }, error: null });
     },
@@ -1466,6 +1488,10 @@ export function createSkillRoutes(config: SkillRoutesConfig): Hono<{ Variables:
       // Mirror cleanup: directly remove by name (skill doc is gone, so
       // the umbrella `syncSkill` would no-op).
       fireMirrorRemove(skillNameForMirror);
+      // The skill is gone → any skillset referencing it now has an
+      // unresolvable member; recompute + notify owners (#1136). Name+guid
+      // captured before deletion so the reverse index still matches.
+      fireSkillsetRecompute?.({ guid, name: skillNameForMirror });
 
       return c.json({ data: { success: true }, error: null });
     },