From 6a82324e8d7619a5e1c309a0e0e54d9d262cf1b2 Mon Sep 17 00:00:00 2001 From: Ibraheem Amin <68827140+DIodide@users.noreply.github.com> Date: Fri, 3 Jul 2026 22:51:37 -0400 Subject: [PATCH 1/3] refactor(convex): consolidate share/harness authorization into shareAuthCore (#157) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third deepening from the improve-codebase-architecture skill (design-it-twice → Design #2). The grant-based access model was mirrored across shares.ts and harnessShares.ts (two ~700-line files) — grantForToken, assertOwned*, resolveRole, activeGrantForCaller all near-identical — and the mirror had already rotted once (the sharedLocked cleanup fix landed on only one side). New shareAuthCore.ts owns the ONE security machine: empty-token→null guard, isActiveGrant (single impl) on the token lookup AND the per-user scan, opaque "Not found" (never leak existence), owner-wins-first, the cross-entity fk gate, editor-short-circuit / viewer-floor / max-role-wins, the truthy-userId guard (an anonymous caller rides the token path only — never matches a grantedToUserId===undefined row), and a required-projection sharedHeader (an unredacted egress is unrepresentable). Each entity wires it with concretely- typed closures via makeShareAuth(descriptor) — zero casts on the fk gate. Entity POLICY stays in the wrappers: publicMessage / publicHarnessProjection redaction, authorizeConversationWrite (owner|editor) vs editSharedHarness (editor-only + the sharedLocked lock + field allowlist), mint/rotate/list payloads, the harness lock-cleanup tail. Exported signatures unchanged (resolveConversationRole / authorizeConversationWrite / isActiveGrant re- exported from shares.ts) so messages.ts and other importers need zero edits. Behaviour-preserving: the 48 existing share/role tests (shares 32 + harnessShares 16) pass with ZERO expectation changes. New shareAuthCore.test.ts (12 cases) pins the invariants directly, incl. the anonymous-privilege- escalation negative and the cross-entity gate. convex 231 passed, tsc + biome clean. HOLD merge until AFTER v1.0 is cut (live authorization code — keep the release clean; a human should read the final diff). --- .../convex-backend/convex/harnessShares.ts | 136 +++------ .../convex/shareAuthCore.test.ts | 262 ++++++++++++++++++ .../convex-backend/convex/shareAuthCore.ts | 211 ++++++++++++++ packages/convex-backend/convex/shares.ts | 119 +++----- 4 files changed, 554 insertions(+), 174 deletions(-) create mode 100644 packages/convex-backend/convex/shareAuthCore.test.ts create mode 100644 packages/convex-backend/convex/shareAuthCore.ts diff --git a/packages/convex-backend/convex/harnessShares.ts b/packages/convex-backend/convex/harnessShares.ts index 4321f6b..31d5deb 100644 --- a/packages/convex-backend/convex/harnessShares.ts +++ b/packages/convex-backend/convex/harnessShares.ts @@ -2,12 +2,12 @@ import { v } from "convex/values"; import type { Doc, Id } from "./_generated/dataModel"; import { internalMutation, - type MutationCtx, mutation, type QueryCtx, query, } from "./_generated/server"; import { assertSystemPromptLength } from "./harnesses"; +import { makeShareAuth } from "./shareAuthCore"; import { ALLOWED_AVATAR_HOSTS, clampAuthorImageUrl, @@ -44,33 +44,25 @@ export { ALLOWED_AVATAR_HOSTS }; type HarnessRole = "owner" | "editor" | "viewer" | "none"; -/** Resolve the active grant for a public token, or null (never leaks existence). */ -async function harnessGrantForToken( - ctx: QueryCtx, - token: string, -): Promise | null> { - if (!token) return null; - const grant = await ctx.db - .query("harnessShareGrants") - .withIndex("by_token", (q) => q.eq("publicToken", token)) - .unique(); - if (!grant || !isActiveGrant(grant)) return null; - return grant; -} - -/** Load a harness and assert the caller owns it (opaque "Not found" otherwise). */ -async function assertOwnedHarness( - ctx: MutationCtx | QueryCtx, - harnessId: Id<"harnesses">, -): Promise> { - const identity = await ctx.auth.getUserIdentity(); - if (!identity) throw new Error("Unauthenticated"); - const harness = await ctx.db.get(harnessId); - if (!harness || harness.userId !== identity.subject) { - throw new Error("Not found"); - } - return harness; -} +// The harness-side share-authorization instance. All grant/role/token logic +// lives in the core (shareAuthCore); only the raw index queries and the +// fk/owner accessors are wired here, concretely typed (no casts on the gate). +const harnessAuth = makeShareAuth({ + grantByToken: (ctx, token) => + ctx.db + .query("harnessShareGrants") + .withIndex("by_token", (q) => q.eq("publicToken", token)) + .unique(), + grantById: (ctx, grantId: Id<"harnessShareGrants">) => ctx.db.get(grantId), + grantsForEntity: (ctx, harnessId: Id<"harnesses">) => + ctx.db + .query("harnessShareGrants") + .withIndex("by_harness", (q) => q.eq("harnessId", harnessId)) + .collect(), + getEntity: (ctx, harnessId: Id<"harnesses">) => ctx.db.get(harnessId), + grantEntityId: (grant) => grant.harnessId, + entityOwner: (harness) => harness.userId, +}); /** * The caller's role on a harness — owner / editor / viewer / none. Mirrors @@ -84,30 +76,7 @@ async function resolveHarnessRole( harnessId: Id<"harnesses">, token?: string, ): Promise { - const harness = await ctx.db.get(harnessId); - if (!harness) return "none"; - if (userId && harness.userId === userId) return "owner"; - - let best: HarnessRole = "none"; - if (token) { - const grant = await harnessGrantForToken(ctx, token); - if (grant && grant.harnessId === harnessId) { - if (grant.role === "editor") return "editor"; - best = "viewer"; - } - } - if (userId) { - const grants = await ctx.db - .query("harnessShareGrants") - .withIndex("by_harness", (q) => q.eq("harnessId", harnessId)) - .collect(); - for (const g of grants) { - if (!isActiveGrant(g) || g.grantedToUserId !== userId) continue; - if (g.role === "editor") return "editor"; - best = "viewer"; - } - } - return best; + return harnessAuth.resolveRole(ctx, userId, harnessId, token); } /** @@ -161,7 +130,7 @@ export const ensureHarnessPublicLink = mutation({ ownerImageUrl: v.optional(v.string()), }, handler: async (ctx, args) => { - const harness = await assertOwnedHarness(ctx, args.harnessId); + const harness = await harnessAuth.assertOwned(ctx, args.harnessId); if (args.token.length < MIN_TOKEN_LENGTH) { throw new Error("Share token too short"); } @@ -206,7 +175,7 @@ export const inviteHarnessByEmail = mutation({ ownerImageUrl: v.optional(v.string()), }, handler: async (ctx, args) => { - const harness = await assertOwnedHarness(ctx, args.harnessId); + const harness = await harnessAuth.assertOwned(ctx, args.harnessId); const email = normalizeEmail(args.email); if (!EMAIL_RE.test(email)) throw new Error("Enter a valid email address"); // Re-use an existing active invite for this email on this harness (an @@ -248,7 +217,7 @@ export const setHarnessShareRole = mutation({ handler: async (ctx, args) => { const grant = await ctx.db.get(args.grantId); if (!grant) throw new Error("Not found"); - await assertOwnedHarness(ctx, grant.harnessId); + await harnessAuth.assertOwned(ctx, grant.harnessId); await ctx.db.patch(args.grantId, { role: args.role }); }, }); @@ -257,7 +226,7 @@ export const setHarnessShareRole = mutation({ export const setHarnessLock = mutation({ args: { harnessId: v.id("harnesses"), locked: v.boolean() }, handler: async (ctx, args) => { - await assertOwnedHarness(ctx, args.harnessId); + await harnessAuth.assertOwned(ctx, args.harnessId); await ctx.db.patch(args.harnessId, { sharedLocked: args.locked }); }, }); @@ -272,7 +241,7 @@ export const rotateHarnessPublicLink = mutation({ ownerImageUrl: v.optional(v.string()), }, handler: async (ctx, args) => { - const harness = await assertOwnedHarness(ctx, args.harnessId); + const harness = await harnessAuth.assertOwned(ctx, args.harnessId); if (args.token.length < MIN_TOKEN_LENGTH) { throw new Error("Share token too short"); } @@ -306,7 +275,7 @@ export const revokeHarnessShareGrant = mutation({ handler: async (ctx, args) => { const grant = await ctx.db.get(args.grantId); if (!grant) return; // already gone — idempotent - await assertOwnedHarness(ctx, grant.harnessId); + await harnessAuth.assertOwned(ctx, grant.harnessId); await ctx.db.delete(args.grantId); // Revoking the LAST active grant one-by-one must leave the harness as // unshareHarness does — else a stale sharedLocked lingers and a later @@ -329,7 +298,7 @@ export const revokeHarnessShareGrant = mutation({ export const unshareHarness = mutation({ args: { harnessId: v.id("harnesses") }, handler: async (ctx, args) => { - await assertOwnedHarness(ctx, args.harnessId); + await harnessAuth.assertOwned(ctx, args.harnessId); const grants = await ctx.db .query("harnessShareGrants") .withIndex("by_harness", (q) => q.eq("harnessId", args.harnessId)) @@ -440,23 +409,19 @@ export const listMySharedHarnesses = query({ */ export const getSharedHarness = query({ args: { token: v.string() }, - handler: async (ctx, args) => { - const grant = await harnessGrantForToken(ctx, args.token); - if (!grant) return null; - const harness = await ctx.db.get(grant.harnessId); - if (!harness) return null; - const identity = await ctx.auth.getUserIdentity(); - const viewerIsOwner = - identity != null && harness.userId === identity.subject; - return { - harnessId: harness._id, - ...publicHarnessProjection(harness), - role: grant.role, - viewerIsOwner, - ownerName: grant.ownerName ?? null, - ownerImageUrl: grant.ownerImageUrl ?? null, - }; - }, + handler: async (ctx, args) => + harnessAuth.sharedHeader( + ctx, + args.token, + ({ entity: harness, grant, viewerIsOwner }) => ({ + harnessId: harness._id, + ...publicHarnessProjection(harness), + role: grant.role, + viewerIsOwner, + ownerName: grant.ownerName ?? null, + ownerImageUrl: grant.ownerImageUrl ?? null, + }), + ), }); /** @@ -518,23 +483,6 @@ export const listIncomingSharedHarnesses = query({ }, }); -/** Resolve a grant the caller may act on, from a token OR a bound grantId. */ -async function activeGrantForCaller( - ctx: QueryCtx, - userId: string, - token?: string, - grantId?: Id<"harnessShareGrants">, -): Promise | null> { - if (token) return await harnessGrantForToken(ctx, token); - if (grantId) { - const grant = await ctx.db.get(grantId); - if (!grant || !isActiveGrant(grant)) return null; - if (grant.grantedToUserId !== userId) return null; - return grant; - } - return null; -} - /** * Clone a shared harness into the caller's own account. ALWAYS allowed * regardless of lock. Drops ALL secrets: never copies authToken (recipient @@ -548,7 +496,7 @@ export const cloneSharedHarness = mutation({ handler: async (ctx, args) => { const identity = await ctx.auth.getUserIdentity(); if (!identity) throw new Error("Unauthenticated"); - const grant = await activeGrantForCaller( + const grant = await harnessAuth.activeGrantForCaller( ctx, identity.subject, args.token, diff --git a/packages/convex-backend/convex/shareAuthCore.test.ts b/packages/convex-backend/convex/shareAuthCore.test.ts new file mode 100644 index 0000000..72ba9cb --- /dev/null +++ b/packages/convex-backend/convex/shareAuthCore.test.ts @@ -0,0 +1,262 @@ +/** + * Unit tests for the share-authorization core in isolation (no Convex runtime): + * a hand-rolled in-memory descriptor + a mock ctx drive makeShareAuth directly, + * pinning the security invariants the two wrappers (shares.ts / harnessShares.ts) + * rely on. The 48 wrapper tests exercise the core end-to-end; these lock the + * highest-risk edges — especially the anonymous-privilege-escalation negative. + */ +import { describe, expect, it } from "vitest"; +import type { QueryCtx } from "./_generated/server"; +import { type GrantLike, makeShareAuth } from "./shareAuthCore"; + +type FakeGrant = GrantLike & { + _id: string; + entityId: string; + publicToken?: string; +}; +type FakeEntity = { _id: string; userId: string }; + +function setup(opts: { + entities: FakeEntity[]; + grants: FakeGrant[]; + identitySubject?: string | null; +}) { + let reads = 0; + const ctx = { + auth: { + getUserIdentity: async () => + opts.identitySubject == null ? null : { subject: opts.identitySubject }, + }, + } as unknown as QueryCtx; + const auth = makeShareAuth({ + grantByToken: async (_c, token) => { + reads++; + return opts.grants.find((g) => g.publicToken === token) ?? null; + }, + grantById: async (_c, id) => opts.grants.find((g) => g._id === id) ?? null, + grantsForEntity: async (_c, entityId) => + opts.grants.filter((g) => g.entityId === entityId), + getEntity: async (_c, id) => + opts.entities.find((e) => e._id === id) ?? null, + grantEntityId: (g) => g.entityId, + entityOwner: (e) => e.userId, + }); + return { ctx, auth, tokenReads: () => reads }; +} + +const OWNER = "user_owner"; +const ENTITY: FakeEntity = { _id: "e1", userId: OWNER }; + +describe("shareAuthCore.grantForToken", () => { + it("empty token short-circuits to null before any db read", async () => { + const { ctx, auth, tokenReads } = setup({ entities: [ENTITY], grants: [] }); + expect(await auth.grantForToken(ctx, "")).toBeNull(); + expect(tokenReads()).toBe(0); + }); + + it("an inactive (revoked/expired) grant returns null, like nonexistent", async () => { + const { ctx, auth } = setup({ + entities: [ENTITY], + grants: [ + { + _id: "g1", + entityId: "e1", + role: "editor", + publicToken: "tok-revoked", + revokedAt: 1, + }, + { + _id: "g2", + entityId: "e1", + role: "editor", + publicToken: "tok-expired", + expiresAt: 1, + }, + ], + }); + expect(await auth.grantForToken(ctx, "tok-revoked")).toBeNull(); + expect(await auth.grantForToken(ctx, "tok-expired")).toBeNull(); + expect(await auth.grantForToken(ctx, "tok-missing")).toBeNull(); + }); +}); + +describe("shareAuthCore.resolveRole", () => { + it("owner wins before any grant read (even with a viewer grant present)", async () => { + const { ctx, auth } = setup({ + entities: [ENTITY], + grants: [ + { _id: "g1", entityId: "e1", role: "viewer", grantedToUserId: OWNER }, + ], + }); + expect(await auth.resolveRole(ctx, OWNER, "e1")).toBe("owner"); + }); + + it("a null (anonymous) userId can NEVER match a grantedToUserId===undefined row", async () => { + // The critical privilege-escalation negative: a public-link / unbound-invite + // row (grantedToUserId undefined) must not confer anything to an anonymous + // caller who presents NO token. + const { ctx, auth } = setup({ + entities: [ENTITY], + grants: [ + { _id: "g-link", entityId: "e1", role: "editor", publicToken: "tok" }, // public link, no grantedToUserId + { _id: "g-invite", entityId: "e1", role: "editor" }, // unbound email invite + ], + identitySubject: null, + }); + expect(await auth.resolveRole(ctx, null, "e1")).toBe("none"); // no token → none + // WITH the public token an anonymous viewer rides only the token path: + expect(await auth.resolveRole(ctx, null, "e1", "tok")).toBe("editor"); + }); + + it("a token for another entity confers nothing (cross-entity gate)", async () => { + const other: FakeEntity = { _id: "e2", userId: "user_b" }; + const { ctx, auth } = setup({ + entities: [ENTITY, other], + grants: [ + { _id: "g1", entityId: "e2", role: "editor", publicToken: "tok-e2" }, + ], + }); + // The token is valid — but for e2. Resolving e1 with it yields none. + expect(await auth.resolveRole(ctx, "user_x", "e1", "tok-e2")).toBe("none"); + expect(await auth.resolveRole(ctx, "user_x", "e2", "tok-e2")).toBe( + "editor", + ); + }); + + it("editor beats viewer; a per-user grant addressed to the caller applies", async () => { + const { ctx, auth } = setup({ + entities: [ENTITY], + grants: [ + { + _id: "g1", + entityId: "e1", + role: "viewer", + grantedToUserId: "user_x", + }, + { + _id: "g2", + entityId: "e1", + role: "editor", + grantedToUserId: "user_x", + }, + ], + }); + expect(await auth.resolveRole(ctx, "user_x", "e1")).toBe("editor"); + expect(await auth.resolveRole(ctx, "user_y", "e1")).toBe("none"); + }); + + it("missing entity resolves to none (never throws)", async () => { + const { ctx, auth } = setup({ entities: [], grants: [] }); + expect(await auth.resolveRole(ctx, "user_x", "gone")).toBe("none"); + }); +}); + +describe("shareAuthCore.assertOwned", () => { + it("unauthenticated → Unauthenticated; wrong owner AND missing → identical Not found", async () => { + const anon = setup({ + entities: [ENTITY], + grants: [], + identitySubject: null, + }); + await expect(anon.auth.assertOwned(anon.ctx, "e1")).rejects.toThrow( + "Unauthenticated", + ); + const other = setup({ + entities: [ENTITY], + grants: [], + identitySubject: "user_b", + }); + await expect(other.auth.assertOwned(other.ctx, "e1")).rejects.toThrow( + "Not found", + ); + await expect(other.auth.assertOwned(other.ctx, "gone")).rejects.toThrow( + "Not found", + ); + }); + + it("owner gets the entity back", async () => { + const { ctx, auth } = setup({ + entities: [ENTITY], + grants: [], + identitySubject: OWNER, + }); + expect((await auth.assertOwned(ctx, "e1"))._id).toBe("e1"); + }); +}); + +describe("shareAuthCore.activeGrantForCaller", () => { + it("resolves via token, via a bound grantId, and denies a grantId owned by another", async () => { + const { ctx, auth } = setup({ + entities: [ENTITY], + grants: [ + { _id: "g-tok", entityId: "e1", role: "editor", publicToken: "tok" }, + { + _id: "g-mine", + entityId: "e1", + role: "viewer", + grantedToUserId: "user_x", + }, + { + _id: "g-other", + entityId: "e1", + role: "viewer", + grantedToUserId: "user_y", + }, + ], + }); + expect((await auth.activeGrantForCaller(ctx, "user_x", "tok"))?._id).toBe( + "g-tok", + ); + expect( + (await auth.activeGrantForCaller(ctx, "user_x", undefined, "g-mine")) + ?._id, + ).toBe("g-mine"); + expect( + await auth.activeGrantForCaller(ctx, "user_x", undefined, "g-other"), + ).toBeNull(); + expect(await auth.activeGrantForCaller(ctx, "user_x")).toBeNull(); + }); +}); + +describe("shareAuthCore.sharedHeader", () => { + it("returns null for an inactive token and never calls build", async () => { + let built = false; + const { ctx, auth } = setup({ + entities: [ENTITY], + grants: [ + { + _id: "g1", + entityId: "e1", + role: "viewer", + publicToken: "tok", + revokedAt: 1, + }, + ], + }); + const out = await auth.sharedHeader(ctx, "tok", () => { + built = true; + return { leaked: true }; + }); + expect(out).toBeNull(); + expect(built).toBe(false); + }); + + it("hands build a redacted header for a valid token", async () => { + const { ctx, auth } = setup({ + entities: [ENTITY], + grants: [ + { _id: "g1", entityId: "e1", role: "editor", publicToken: "tok" }, + ], + identitySubject: OWNER, + }); + const out = await auth.sharedHeader( + ctx, + "tok", + ({ grant, viewerIsOwner }) => ({ + role: grant.role, + viewerIsOwner, + }), + ); + expect(out).toEqual({ role: "editor", viewerIsOwner: true }); + }); +}); diff --git a/packages/convex-backend/convex/shareAuthCore.ts b/packages/convex-backend/convex/shareAuthCore.ts new file mode 100644 index 0000000..2d36c4b --- /dev/null +++ b/packages/convex-backend/convex/shareAuthCore.ts @@ -0,0 +1,211 @@ +/** + * Share-authorization core — the ONE implementation of the grant-based access + * model, shared by conversation sharing (shares.ts) and harness sharing + * (harnessShares.ts). + * + * Both entities have a near-identical authorization mechanism (an active-grant + * lookup by token, an owner assertion, and a role-precedence resolver) that was + * previously mirrored across two ~700-line files — and the mirror had already + * rotted once (a lock-cleanup fix landed on only one side). This module makes + * the security-critical logic live in one auditable place; the entity-specific + * POLICY (redaction projections, write rules, the harness lock, listing + * payloads) stays in the wrappers. + * + * Seam discipline: the ONLY things a caller provides are the raw db accessors + * (index queries + get) and the fk/owner field accessors, as concretely-typed + * closures on the caller's OWN generated Convex types. The core holds no direct + * `ctx.db` call and no cast — every activeness, precedence, opaque-null and + * cross-entity-isolation rule is centralized and type-checked here. + */ +import type { MutationCtx, QueryCtx } from "./_generated/server"; + +export type Role = "owner" | "editor" | "viewer" | "none"; + +type AnyCtx = QueryCtx | MutationCtx; + +/** The fields the core reads off any grant. Both shareGrants and + * harnessShareGrants documents satisfy this structurally. */ +export type GrantLike = { + role: "viewer" | "editor"; + grantedToUserId?: string; + revokedAt?: number; + expiresAt?: number; +}; + +/** + * A grant is active iff not revoked and not expired. Checked on EVERY use, + * never cached — Convex re-evaluates queries reactively, so a revoked/expired + * grant flips to inactive on the next read (revocation immediacy). + */ +export function isActiveGrant< + T extends { revokedAt?: number; expiresAt?: number }, +>(grant: T): boolean { + if (grant.revokedAt) return false; + if (grant.expiresAt && grant.expiresAt <= Date.now()) return false; + return true; +} + +/** + * Entity-specific wiring for `makeShareAuth`, provided as closures over the + * caller's concrete generated types (G = grant doc, E = entity doc, EId/GId = + * their id types). The core applies the activeness filter, the empty-token + * guard and the fk gate — the closures only do raw lookups + field reads. + */ +export interface ShareAuthDescriptor { + /** Raw `by_token` `.unique()` lookup — NO activeness filter (core adds it). */ + grantByToken(ctx: AnyCtx, token: string): Promise; + /** Raw `ctx.db.get(grantId)` — NO activeness filter (core adds it). */ + grantById(ctx: AnyCtx, grantId: GId): Promise; + /** Raw entity-index `.collect()` — NO filter (core adds it). */ + grantsForEntity(ctx: AnyCtx, entityId: EId): Promise; + /** Raw `ctx.db.get(entityId)`. */ + getEntity(ctx: AnyCtx, entityId: EId): Promise; + /** The grant's foreign key to its entity — powers the cross-entity gate. */ + grantEntityId(grant: G): EId; + /** The entity's owner user id. */ + entityOwner(entity: E): string; +} + +/** The header a shared read resolves before building its redacted projection. */ +export interface SharedHeader { + entity: E; + grant: G; + viewerSubject: string | null; + viewerIsOwner: boolean; +} + +export interface ShareAuth { + /** Active grant for a public token, or null. Empty token short-circuits to + * null BEFORE any read; inactive returns the same null as nonexistent, so a + * caller can never distinguish revoked from never-existed. Never throws. */ + grantForToken(ctx: AnyCtx, token: string): Promise; + /** Load the entity and assert the caller owns it. Unauthenticated → + * "Unauthenticated"; missing entity AND wrong owner both throw the + * byte-identical opaque "Not found" (never leak existence). */ + assertOwned(ctx: AnyCtx, entityId: EId): Promise; + /** Resolve (user, entity) → role. Owner wins first (before any grant read); a + * token confers a role ONLY when its grant's fk equals THIS entity id + * (cross-entity token confers nothing); editor short-circuits, viewer is a + * floor; per-user grants require active + grantedToUserId match. Missing + * entity → "none" (never throws). A null/empty userId rides ONLY the token + * path — it can never match a grantedToUserId===undefined row. */ + resolveRole( + ctx: AnyCtx, + userId: string | null, + entityId: EId, + token?: string, + ): Promise; + /** The active grant a caller holds — via public token, or a bound grantId + * they are the grantee of. Null otherwise. */ + activeGrantForCaller( + ctx: AnyCtx, + userId: string, + token?: string, + grantId?: GId, + ): Promise; + /** Resolve a shared read's header from a token, then hand it to `build` (the + * entity-specific redacted projection). `build` is REQUIRED — there is no + * overload returning a raw entity, so an unredacted egress is + * unrepresentable. Neutral null on any miss (invalid token / gone entity). */ + sharedHeader( + ctx: AnyCtx, + token: string, + build: (header: SharedHeader) => H | Promise, + ): Promise; +} + +export function makeShareAuth( + d: ShareAuthDescriptor, +): ShareAuth { + async function grantForToken(ctx: AnyCtx, token: string): Promise { + if (!token) return null; + const grant = await d.grantByToken(ctx, token); + if (!grant || !isActiveGrant(grant)) return null; + return grant; + } + + async function assertOwned(ctx: AnyCtx, entityId: EId): Promise { + const identity = await ctx.auth.getUserIdentity(); + if (!identity) throw new Error("Unauthenticated"); + const entity = await d.getEntity(ctx, entityId); + if (!entity || d.entityOwner(entity) !== identity.subject) { + throw new Error("Not found"); + } + return entity; + } + + async function resolveRole( + ctx: AnyCtx, + userId: string | null, + entityId: EId, + token?: string, + ): Promise { + const entity = await d.getEntity(ctx, entityId); + if (!entity) return "none"; + if (userId && d.entityOwner(entity) === userId) return "owner"; + + let best: Role = "none"; + // A token confers a role ONLY if it resolves to an active grant whose fk + // is THIS entity (a token for another entity confers nothing here). + if (token) { + const grant = await grantForToken(ctx, token); + if (grant && d.grantEntityId(grant) === entityId) { + if (grant.role === "editor") return "editor"; + best = "viewer"; + } + } + // Per-user grants addressed directly to this user. Guarded on truthy + // userId so an anonymous caller can NEVER match a grantedToUserId=== + // undefined row (public-link rows, unbound email invites). + if (userId) { + const grants = await d.grantsForEntity(ctx, entityId); + for (const grant of grants) { + if (!isActiveGrant(grant)) continue; + if (grant.grantedToUserId !== userId) continue; + if (grant.role === "editor") return "editor"; + best = "viewer"; + } + } + return best; + } + + async function activeGrantForCaller( + ctx: AnyCtx, + userId: string, + token?: string, + grantId?: GId, + ): Promise { + if (token) return await grantForToken(ctx, token); + if (grantId !== undefined) { + const grant = await d.grantById(ctx, grantId); + if (!grant || !isActiveGrant(grant)) return null; + if (grant.grantedToUserId !== userId) return null; + return grant; + } + return null; + } + + async function sharedHeader( + ctx: AnyCtx, + token: string, + build: (header: SharedHeader) => H | Promise, + ): Promise { + const grant = await grantForToken(ctx, token); + if (!grant) return null; + const entity = await d.getEntity(ctx, d.grantEntityId(grant)); + if (!entity) return null; + const identity = await ctx.auth.getUserIdentity(); + const viewerSubject = identity?.subject ?? null; + const viewerIsOwner = + viewerSubject !== null && d.entityOwner(entity) === viewerSubject; + return await build({ entity, grant, viewerSubject, viewerIsOwner }); + } + + return { + grantForToken, + assertOwned, + resolveRole, + activeGrantForCaller, + sharedHeader, + }; +} diff --git a/packages/convex-backend/convex/shares.ts b/packages/convex-backend/convex/shares.ts index 366ce76..296cdc9 100644 --- a/packages/convex-backend/convex/shares.ts +++ b/packages/convex-backend/convex/shares.ts @@ -2,8 +2,13 @@ import { v } from "convex/values"; import type { Doc, Id } from "./_generated/dataModel"; import type { MutationCtx, QueryCtx } from "./_generated/server"; import { internalQuery, mutation, query } from "./_generated/server"; +import { isActiveGrant, makeShareAuth } from "./shareAuthCore"; import { getOrCreateDefaultWorkspace } from "./workspaces"; +// Re-exported so harnessShares.ts's `import { isActiveGrant } from "./shares"` +// (and any other importer) keeps resolving after the move to the core. +export { isActiveGrant }; + /** * Conversation sharing. * @@ -28,44 +33,28 @@ import { getOrCreateDefaultWorkspace } from "./workspaces"; // require a healthy minimum so a caller can't pass a guessable short string. export const MIN_TOKEN_LENGTH = 32; -// Generic over shareGrants AND harnessShareGrants (same active semantics): -// active = not revoked and not expired. -export function isActiveGrant< - T extends { revokedAt?: number; expiresAt?: number }, ->(grant: T): boolean { - if (grant.revokedAt) return false; - if (grant.expiresAt && grant.expiresAt <= Date.now()) return false; - return true; -} - -/** Resolve the active grant for a public token, or null. */ -async function grantForToken( - ctx: QueryCtx, - token: string, -): Promise | null> { - if (!token) return null; - const grant = await ctx.db - .query("shareGrants") - .withIndex("by_token", (q) => q.eq("publicToken", token)) - .unique(); - if (!grant || !isActiveGrant(grant)) return null; - return grant; -} - -/** Load a conversation and assert the caller is its owner. Mirrors the - * `assertOwnedWorkspace` helper in workspaces.ts. */ -async function assertOwnedConversation( - ctx: MutationCtx | QueryCtx, - conversationId: Id<"conversations">, -): Promise> { - const identity = await ctx.auth.getUserIdentity(); - if (!identity) throw new Error("Unauthenticated"); - const convo = await ctx.db.get(conversationId); - if (!convo || convo.userId !== identity.subject) { - throw new Error("Not found"); - } - return convo; -} +// The conversation-side share-authorization instance. All grant/role/token +// logic lives in the core (shareAuthCore); only the raw index queries and the +// fk/owner accessors are wired here, concretely typed (no casts on the gate). +const chatAuth = makeShareAuth({ + grantByToken: (ctx, token) => + ctx.db + .query("shareGrants") + .withIndex("by_token", (q) => q.eq("publicToken", token)) + .unique(), + grantById: (ctx, grantId: Id<"shareGrants">) => ctx.db.get(grantId), + grantsForEntity: (ctx, conversationId: Id<"conversations">) => + ctx.db + .query("shareGrants") + .withIndex("by_conversation", (q) => + q.eq("conversationId", conversationId), + ) + .collect(), + getEntity: (ctx, conversationId: Id<"conversations">) => + ctx.db.get(conversationId), + grantEntityId: (grant) => grant.conversationId, + entityOwner: (convo) => convo.userId, +}); /** Public-safe projection of a stored message. Per product decision shared * links show the FULL transcript (text + reasoning + tool calls), but never @@ -108,34 +97,7 @@ export async function resolveConversationRole( conversationId: Id<"conversations">, token?: string, ): Promise { - const convo = await ctx.db.get(conversationId); - if (!convo) return "none"; - if (convo.userId === userId) return "owner"; - - let best: "viewer" | "none" = "none"; - - // A token only grants anything if it resolves to an active grant on THIS - // conversation (a token for another conversation confers nothing here). - if (token) { - const grant = await grantForToken(ctx, token); - if (grant && grant.conversationId === conversationId) { - if (grant.role === "editor") return "editor"; - best = "viewer"; - } - } - - // Per-user grants addressed directly to this user. - const grants = await ctx.db - .query("shareGrants") - .withIndex("by_conversation", (q) => q.eq("conversationId", conversationId)) - .collect(); - for (const g of grants) { - if (!isActiveGrant(g)) continue; - if (g.grantedToUserId !== userId) continue; - if (g.role === "editor") return "editor"; - best = "viewer"; - } - return best; + return chatAuth.resolveRole(ctx, userId, conversationId, token); } /** @@ -297,7 +259,7 @@ export const ensurePublicLink = mutation({ ownerImageUrl: v.optional(v.string()), }, handler: async (ctx, args) => { - const convo = await assertOwnedConversation(ctx, args.conversationId); + const convo = await chatAuth.assertOwned(ctx, args.conversationId); if (args.token.length < MIN_TOKEN_LENGTH) { throw new Error("Share token too short"); } @@ -346,7 +308,7 @@ export const setShareRole = mutation({ handler: async (ctx, args) => { const grant = await ctx.db.get(args.grantId); if (!grant) throw new Error("Not found"); - await assertOwnedConversation(ctx, grant.conversationId); + await chatAuth.assertOwned(ctx, grant.conversationId); await ctx.db.patch(args.grantId, { role: args.role }); }, }); @@ -365,7 +327,7 @@ export const rotatePublicLink = mutation({ ownerImageUrl: v.optional(v.string()), }, handler: async (ctx, args) => { - const convo = await assertOwnedConversation(ctx, args.conversationId); + const convo = await chatAuth.assertOwned(ctx, args.conversationId); if (args.token.length < MIN_TOKEN_LENGTH) { throw new Error("Share token too short"); } @@ -401,7 +363,7 @@ export const revokeShareGrant = mutation({ handler: async (ctx, args) => { const grant = await ctx.db.get(args.grantId); if (!grant) return; // already gone — idempotent - await assertOwnedConversation(ctx, grant.conversationId); + await chatAuth.assertOwned(ctx, grant.conversationId); await ctx.db.delete(args.grantId); }, }); @@ -410,7 +372,7 @@ export const revokeShareGrant = mutation({ export const unshareConversation = mutation({ args: { conversationId: v.id("conversations") }, handler: async (ctx, args) => { - await assertOwnedConversation(ctx, args.conversationId); + await chatAuth.assertOwned(ctx, args.conversationId); const grants = await ctx.db .query("shareGrants") .withIndex("by_conversation", (q) => @@ -529,7 +491,7 @@ export const listMySharedConversations = query({ export const getSharedConversation = query({ args: { token: v.string() }, handler: async (ctx, args) => { - const grant = await grantForToken(ctx, args.token); + const grant = await chatAuth.grantForToken(ctx, args.token); if (!grant) return null; const convo = await ctx.db.get(grant.conversationId); if (!convo) return null; @@ -558,8 +520,7 @@ export const getSharedConversation = query({ sandboxId = harness.daytonaSandboxId; } } - const viewerIsOwner = - identity != null && convo.userId === identity.subject; + const viewerIsOwner = identity != null && convo.userId === identity.subject; return { conversationId: convo._id, title: convo.title, @@ -589,7 +550,7 @@ export const getSharedConversation = query({ export const listSharedMessages = query({ args: { token: v.string() }, handler: async (ctx, args) => { - const grant = await grantForToken(ctx, args.token); + const grant = await chatAuth.grantForToken(ctx, args.token); if (!grant) return []; const messages = await ctx.db .query("messages") @@ -609,7 +570,7 @@ export const listSharedMessages = query({ export const getSharedFileUrl = query({ args: { token: v.string(), storageId: v.id("_storage") }, handler: async (ctx, args) => { - const grant = await grantForToken(ctx, args.token); + const grant = await chatAuth.grantForToken(ctx, args.token); if (!grant) return null; const messages = await ctx.db .query("messages") @@ -648,7 +609,7 @@ export const forkSharedConversation = mutation({ const identity = await ctx.auth.getUserIdentity(); if (!identity) throw new Error("Unauthenticated"); - const grant = await grantForToken(ctx, args.token); + const grant = await chatAuth.grantForToken(ctx, args.token); if (!grant) throw new Error("This shared chat is no longer available"); const source = await ctx.db.get(grant.conversationId); if (!source) throw new Error("This shared chat is no longer available"); @@ -698,9 +659,7 @@ export const forkSharedConversation = mutation({ // to the SHARED page (they don't own the original — navigating to it in // /chat would show an empty owner-gated conversation). Only public-link // grants carry a token; per-user grants don't. - ...(grant.publicToken - ? { forkedFromShareToken: grant.publicToken } - : {}), + ...(grant.publicToken ? { forkedFromShareToken: grant.publicToken } : {}), }); for (const msg of messagesToCopy) { From b6b74a5ae0dab77e37d7af43e7d78a90eec32994 Mon Sep 17 00:00:00 2001 From: DIodide Date: Fri, 3 Jul 2026 23:11:52 -0400 Subject: [PATCH 2/3] ci(deploy): install SKILLS_GITHUB_TOKEN as GITHUB_TOKEN in the backend .env MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gateway's SKILL.md backfill (skill_content.py) calls the GitHub API unauthenticated from one EC2 IP — 60 req/hr shared across every user. Write the SKILLS_GITHUB_TOKEN secret into the host .env on deploy (replace-on- deploy, so rotating the secret rotates the host; skipped when unset). The Convex-side skill imports get the same token via the Convex env directly. --- .github/workflows/backend-cd.yml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/.github/workflows/backend-cd.yml b/.github/workflows/backend-cd.yml index 5b43db3..30d6ca8 100644 --- a/.github/workflows/backend-cd.yml +++ b/.github/workflows/backend-cd.yml @@ -80,6 +80,13 @@ jobs: # REDIS_URL just disables live-following fan-out. grep -q '^REDIS_URL=' .env 2>/dev/null || \ echo 'REDIS_URL=redis://127.0.0.1:6379/${{ steps.env.outputs.redis_db }}' >> .env + # GitHub token for the gateway's SKILL.md backfill (raises the API + # rate limit from 60/hr/IP to 5000/hr). Replace-on-deploy so + # rotating the SKILLS_GITHUB_TOKEN secret rotates the host. + if [ -n '${{ secrets.SKILLS_GITHUB_TOKEN }}' ]; then + sed -i '/^GITHUB_TOKEN=/d' .env 2>/dev/null || true + echo 'GITHUB_TOKEN=${{ secrets.SKILLS_GITHUB_TOKEN }}' >> .env + fi python3.11 -m venv .venv 2>/dev/null || true .venv/bin/pip install -q -r requirements.txt sudo systemctl restart ${{ steps.env.outputs.service }} From 109a7a2c5a5d8e312c11e93df7618d313494731a Mon Sep 17 00:00:00 2001 From: Ibraheem Amin <68827140+DIodide@users.noreply.github.com> Date: Fri, 3 Jul 2026 23:34:36 -0400 Subject: [PATCH 3/3] feat(skills): browse + select nested GitHub-repo skills for a pack (#161) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Importing a repo like mattpocock/skills failed with "no skills found": the repo nests skills as skills///SKILL.md, but discovery only matched a fixed set of top-level bases (skills, .claude/skills, .agents/skills) exactly, so anything one folder deeper was invisible. Content-fetching already resolved nested skills by leaf-dir name (Convex viewer + gateway backfill both do), so only discovery was broken. Backend (convex/skills.ts): - discoverRepoSkills finds every SKILL.md at ANY depth in one git/trees call, derives a display "category" by stripping a recognized skills-root prefix (skills/.claude/.agents/.github), and skips node_modules. Leaf id must be unique within the repo (it's the fetch key) — first wins. - New listRepoSkills action: discovers + fetches + caches each SKILL.md (so previews are instant) and returns them with category + description + the repo's AGENTS.md/CLAUDE.md, for a choose-what-you-want flow. - importSkillRepo (curated templates, add-all) now shares the same discovery, so it benefits from nested support too. A repo that resolves but has no skills now reports "no skills found", not "repo not found". Frontend: - New RepoImportDialog: a two-pane browse-and-select modal — left is the discovered skills grouped by category with per-skill / per-group / select-all checkboxes and an added-already state; right is a live SKILL.md preview (rendered markdown, served from the browse cache so it's instant). Footer lets you also pull in AGENTS.md / CLAUDE.md. This brings repo import to parity with `npx skills add` (pick individual skills, see their contents). - The pack editor's "Import a GitHub repo" now opens this dialog (Browse) instead of a blind "Import all". Tests: convex/skills.test.ts stubs a nested (mattpocock-shaped) tree and pins discovery-at-depth, category derivation, node_modules exclusion, URL normalization, caching, and the no-skills vs not-found messages. convex 239 passed; web build + biome clean. --- .../web/src/components/repo-import-dialog.tsx | 394 ++++++++++++++++++ apps/web/src/components/skill-pack-editor.tsx | 47 ++- packages/convex-backend/convex/skills.test.ts | 148 +++++++ packages/convex-backend/convex/skills.ts | 370 ++++++++++------ 4 files changed, 832 insertions(+), 127 deletions(-) create mode 100644 apps/web/src/components/repo-import-dialog.tsx create mode 100644 packages/convex-backend/convex/skills.test.ts diff --git a/apps/web/src/components/repo-import-dialog.tsx b/apps/web/src/components/repo-import-dialog.tsx new file mode 100644 index 0000000..33fdf06 --- /dev/null +++ b/apps/web/src/components/repo-import-dialog.tsx @@ -0,0 +1,394 @@ +import { convexQuery, useConvexAction } from "@convex-dev/react-query"; +import { api } from "@harness/convex-backend/convex/_generated/api"; +import { useQuery } from "@tanstack/react-query"; +import { Check, FileText, Folder, Github, Loader2, Search } from "lucide-react"; +import { useMemo, useState } from "react"; +import toast from "react-hot-toast"; +import type { SkillEntry } from "../lib/skills"; +import { MarkdownMessage } from "./markdown-message"; +import { RoseCurveSpinner } from "./rose-curve-spinner"; +import { Button } from "./ui/button"; +import { Checkbox } from "./ui/checkbox"; +import { Dialog, DialogContent, DialogHeader, DialogTitle } from "./ui/dialog"; +import { Input } from "./ui/input"; + +type RepoSkill = { + skillId: string; + fullId: string; + category: string; + description: string; +}; + +type BrowseResult = { + source: string; + skills: RepoSkill[]; + agentsMd: string; + claudeMd: string; + discovered: number; + truncated: boolean; +}; + +export function RepoImportDialog({ + open, + onOpenChange, + initialSource, + existingNames, + onImport, +}: { + open: boolean; + onOpenChange: (open: boolean) => void; + initialSource: string; + /** Skill fullIds already in the pack — shown as added, not re-selectable. */ + existingNames: Set; + onImport: ( + skills: SkillEntry[], + meta: { agentsMd: string; claudeMd: string; source: string }, + ) => void; +}) { + const listRepoSkills = useConvexAction(api.skills.listRepoSkills); + const [source, setSource] = useState(initialSource); + const [browsing, setBrowsing] = useState(false); + const [result, setResult] = useState(null); + const [selected, setSelected] = useState>(new Set()); + const [preview, setPreview] = useState(null); + const [importAgents, setImportAgents] = useState(true); + const [importClaude, setImportClaude] = useState(true); + + const browse = async (raw?: string) => { + const repo = (raw ?? source).trim(); + if (!repo || browsing) return; + setSource(repo); + setBrowsing(true); + setResult(null); + setSelected(new Set()); + setPreview(null); + try { + const res = (await listRepoSkills({ source: repo })) as BrowseResult; + setResult(res); + // Pre-select everything not already in the pack — one click to add all. + setSelected( + new Set( + res.skills + .filter((s) => !existingNames.has(s.fullId)) + .map((s) => s.fullId), + ), + ); + setPreview(res.skills[0]?.fullId ?? null); + setImportAgents(!!res.agentsMd); + setImportClaude(!!res.claudeMd); + } catch (e) { + const data = + e && typeof e === "object" && "data" in e + ? (e as { data: unknown }).data + : undefined; + toast.error( + typeof data === "string" + ? data + : e instanceof Error + ? e.message + : "Couldn't read that repo", + ); + } finally { + setBrowsing(false); + } + }; + + // Group skills by category, preserving the server's sorted order. + const groups = useMemo(() => { + const map = new Map(); + for (const s of result?.skills ?? []) { + const key = s.category || ""; + const arr = map.get(key); + if (arr) arr.push(s); + else map.set(key, [s]); + } + return [...map.entries()]; + }, [result]); + + const selectableCount = + result?.skills.filter((s) => !existingNames.has(s.fullId)).length ?? 0; + const allSelected = selectableCount > 0 && selected.size === selectableCount; + + const toggle = (fullId: string) => + setSelected((prev) => { + const next = new Set(prev); + if (next.has(fullId)) next.delete(fullId); + else next.add(fullId); + return next; + }); + + const toggleGroup = (skills: RepoSkill[], on: boolean) => + setSelected((prev) => { + const next = new Set(prev); + for (const s of skills) { + if (existingNames.has(s.fullId)) continue; + if (on) next.add(s.fullId); + else next.delete(s.fullId); + } + return next; + }); + + const toggleAll = () => + setSelected( + allSelected + ? new Set() + : new Set( + (result?.skills ?? []) + .filter((s) => !existingNames.has(s.fullId)) + .map((s) => s.fullId), + ), + ); + + const addSelected = () => { + if (!result || selected.size === 0) return; + const skills: SkillEntry[] = result.skills + .filter((s) => selected.has(s.fullId)) + .map((s) => ({ name: s.fullId, description: s.description })); + onImport(skills, { + agentsMd: importAgents ? result.agentsMd : "", + claudeMd: importClaude ? result.claudeMd : "", + source: result.source, + }); + onOpenChange(false); + }; + + const previewQuery = useQuery({ + ...convexQuery(api.skills.getByName, { name: preview ?? "" }), + enabled: !!preview, + }); + + return ( + + + + + + Import skills from a GitHub repo + + + +
+
+ + setSource(e.target.value)} + onKeyDown={(e) => { + if (e.key === "Enter") browse(); + }} + placeholder="owner/repo (e.g. mattpocock/skills)" + className="pl-7" + /> +
+ +
+ + {/* Body */} +
+ {/* Left: selectable, grouped list */} +
+ {result ? ( + <> +
+ + + {result.discovered} skill + {result.discovered === 1 ? "" : "s"} found + {result.truncated && " (truncated)"} + +
+
+ {groups.map(([category, skills]) => { + const groupAll = skills.every( + (s) => + existingNames.has(s.fullId) || selected.has(s.fullId), + ); + return ( +
+ {category && ( + + )} + {skills.map((s) => { + const added = existingNames.has(s.fullId); + const isChecked = added || selected.has(s.fullId); + const isPreview = preview === s.fullId; + return ( +
+ + !added && toggle(s.fullId) + } + className="mt-0.5 shrink-0" + aria-label={`Select ${s.skillId}`} + /> + +
+ ); + })} +
+ ); + })} +
+ + ) : ( +
+ {browsing ? ( + + ) : ( + <> + +

+ Enter a repo and press Browse to see its skills. Nested + folders (e.g.{" "} + + skills/<category>/<name> + + ) are supported. +

+ + )} +
+ )} +
+ + {/* Right: live SKILL.md preview */} +
+ {preview ? ( + <> +
+ + + {preview.split("/").pop()} + + / SKILL.md +
+
+ {previewQuery.data?.detail ? ( + + ) : ( +
+ Loading… +
+ )} +
+ + ) : ( +
+ Select a skill to preview its SKILL.md. +
+ )} +
+
+ + {/* Footer */} +
+
+ {result?.agentsMd && ( + + )} + {result?.claudeMd && ( + + )} +
+ +
+ + + ); +} diff --git a/apps/web/src/components/skill-pack-editor.tsx b/apps/web/src/components/skill-pack-editor.tsx index 6926cd6..37a6c8c 100644 --- a/apps/web/src/components/skill-pack-editor.tsx +++ b/apps/web/src/components/skill-pack-editor.tsx @@ -16,6 +16,7 @@ import type { SkillPackTemplate } from "../lib/skill-pack-templates"; import { SKILL_PACK_TEMPLATES } from "../lib/skill-pack-templates"; import type { SkillEntry } from "../lib/skills"; import { RecommendedSkillsGrid } from "./recommended-skills-grid"; +import { RepoImportDialog } from "./repo-import-dialog"; import { SkillViewerDialog } from "./skill-viewer-dialog"; import { SkillsBrowser } from "./skills-browser"; import { Button } from "./ui/button"; @@ -65,6 +66,25 @@ export function SkillPackEditor({ const importRepo = useConvexAction(api.skills.importSkillRepo); const [repoInput, setRepoInput] = useState(""); const [importingRepo, setImportingRepo] = useState(null); + const [repoDialogOpen, setRepoDialogOpen] = useState(false); + + // Merge a repo-dialog selection into the pack, prefilling empty fields. + const addSkillsFromRepo = ( + imported: SkillEntry[], + meta: { agentsMd: string; claudeMd: string; source: string }, + ) => { + const seen = new Set(skills.map((s) => s.name)); + const fresh = imported.filter((s) => !seen.has(s.name)); + if (fresh.length > 0) setSkills((prev) => [...prev, ...fresh]); + if (!name.trim()) setName(meta.source.split("/").pop() ?? meta.source); + if (meta.agentsMd && !agentsMd.trim()) setAgentsMd(meta.agentsMd); + if (meta.claudeMd && !claudeMd.trim()) setClaudeMd(meta.claudeMd); + toast.success( + fresh.length > 0 + ? `Added ${fresh.length} skill${fresh.length === 1 ? "" : "s"}` + : "Those skills are already in the pack", + ); + }; const toggleSkill = (skill: SkillEntry) => setSkills((prev) => @@ -341,28 +361,25 @@ export function SkillPackEditor({ value={repoInput} onChange={(e) => setRepoInput(e.target.value)} onKeyDown={(e) => { - if (e.key === "Enter") importFromRepo(repoInput); + if (e.key === "Enter" && repoInput.trim()) + setRepoDialogOpen(true); }} - placeholder="owner/repo (e.g. greensock/gsap-skills)" + placeholder="owner/repo (e.g. mattpocock/skills)" className="pl-7" />

- Adds every skill in the repo's skills/ folder, plus - its AGENTS.md / CLAUDE.md. + Browse a repo's skills (nested folders supported), preview each + one, and pick which to add — plus its AGENTS.md / CLAUDE.md.

@@ -386,6 +403,14 @@ export function SkillPackEditor({ fullId={viewingSkillId} onClose={() => setViewingSkillId(null)} /> + + s.name))} + onImport={addSkillsFromRepo} + /> ); } diff --git a/packages/convex-backend/convex/skills.test.ts b/packages/convex-backend/convex/skills.test.ts new file mode 100644 index 0000000..f6e0bd9 --- /dev/null +++ b/packages/convex-backend/convex/skills.test.ts @@ -0,0 +1,148 @@ +import { convexTest } from "convex-test"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { api } from "./_generated/api"; +import schema from "./schema"; + +const modules = import.meta.glob("./**/*.*s"); + +function makeT() { + const raw = convexTest(schema, modules); + const asUser = (userId: string) => + raw.withIdentity({ subject: userId, issuer: "test" }); + return { raw, asUser }; +} + +function res( + body: unknown, + { status = 200, text }: { status?: number; text?: string } = {}, +) { + return { + ok: status >= 200 && status < 300, + status, + headers: { get: () => null }, + json: async () => body, + text: async () => text ?? "", + } as unknown as Response; +} + +const skillMd = (name: string, description: string) => + `---\nname: ${name}\ndescription: ${description}\n---\n\n# ${name}\n\nBody of ${name}.`; + +// A mattpocock/skills-shaped repo: SKILL.md nested under skills//. +const NESTED_TREE = { + truncated: false, + tree: [ + { type: "blob", path: "README.md" }, + { + type: "blob", + path: "skills/engineering/improve-codebase-architecture/SKILL.md", + }, + { type: "blob", path: "skills/engineering/tdd/SKILL.md" }, + { type: "blob", path: "skills/productivity/teach/SKILL.md" }, + // deeper nesting is still discovered + { type: "blob", path: "skills/misc/deep/nested/edit-article/SKILL.md" }, + // node_modules is ignored + { type: "blob", path: "node_modules/pkg/skills/x/SKILL.md" }, + ], +}; + +function installNestedRepoFetch() { + vi.stubGlobal("fetch", async (url: string) => { + if (url.includes("/git/trees/main")) return res(NESTED_TREE); + if (url.includes("/git/trees/master")) return res({}, { status: 404 }); + if (url.endsWith("/SKILL.md")) { + const id = url.split("/").slice(-2, -1)[0]; + return res(null, { text: skillMd(id, `Use ${id} when relevant.`) }); + } + // AGENTS.md / CLAUDE.md / anything else + return res(null, { status: 404 }); + }); +} + +afterEach(() => { + vi.unstubAllGlobals(); +}); + +describe("skills.listRepoSkills (nested-repo browse)", () => { + it("discovers SKILL.md at any depth and groups them by category", async () => { + installNestedRepoFetch(); + const { asUser } = makeT(); + const result = await asUser("u1").action(api.skills.listRepoSkills, { + source: "mattpocock/skills", + }); + + expect(result.source).toBe("mattpocock/skills"); + const byId = Object.fromEntries(result.skills.map((s) => [s.skillId, s])); + + // Nested skills the OLD fixed-base discovery missed are now found. + expect(byId["improve-codebase-architecture"]).toMatchObject({ + fullId: "mattpocock/skills/improve-codebase-architecture", + category: "engineering", + description: "Use improve-codebase-architecture when relevant.", + }); + expect(byId.tdd.category).toBe("engineering"); + expect(byId.teach.category).toBe("productivity"); + // The skills-root prefix is stripped; deeper folders remain as category. + expect(byId["edit-article"].category).toBe("misc/deep/nested"); + // node_modules is excluded. + expect(result.skills.some((s) => s.skillId === "x")).toBe(false); + expect(result.discovered).toBe(4); + }); + + it("caches each SKILL.md so previews are instant (getByName resolves)", async () => { + installNestedRepoFetch(); + const { raw, asUser } = makeT(); + await asUser("u1").action(api.skills.listRepoSkills, { + source: "mattpocock/skills", + }); + const detail = await raw.query(api.skills.getByName, { + name: "mattpocock/skills/tdd", + }); + expect(detail?.detail).toContain("Body of tdd."); + expect(detail?.skillName).toBe("tdd"); + }); + + it("accepts a full github.com URL and normalizes it", async () => { + installNestedRepoFetch(); + const { asUser } = makeT(); + const result = await asUser("u1").action(api.skills.listRepoSkills, { + source: "https://github.com/mattpocock/skills.git", + }); + expect(result.source).toBe("mattpocock/skills"); + expect(result.skills.length).toBe(4); + }); + + it("errors clearly when the repo has no SKILL.md files", async () => { + vi.stubGlobal("fetch", async (url: string) => { + if (url.includes("/git/trees/main")) + return res({ tree: [{ type: "blob", path: "README.md" }] }); + return res({}, { status: 404 }); + }); + const { asUser } = makeT(); + await expect( + asUser("u1").action(api.skills.listRepoSkills, { source: "owner/empty" }), + ).rejects.toThrow(/No skills found/); + }); + + it("rejects a malformed source", async () => { + const { asUser } = makeT(); + await expect( + asUser("u1").action(api.skills.listRepoSkills, { source: "not-a-repo" }), + ).rejects.toThrow(/owner\/repo/); + }); +}); + +describe("skills.importSkillRepo (add-all, shared discovery)", () => { + it("returns every nested skill as a pack entry (name = fullId)", async () => { + installNestedRepoFetch(); + const { asUser } = makeT(); + const result = await asUser("u1").action(api.skills.importSkillRepo, { + source: "mattpocock/skills", + }); + expect(result.imported).toBe(4); + expect(result.skills.map((s) => s.name)).toContain("mattpocock/skills/tdd"); + expect(result.skills.every((s) => typeof s.description === "string")).toBe( + true, + ); + }); +}); diff --git a/packages/convex-backend/convex/skills.ts b/packages/convex-backend/convex/skills.ts index e84586a..009075c 100644 --- a/packages/convex-backend/convex/skills.ts +++ b/packages/convex-backend/convex/skills.ts @@ -1,11 +1,12 @@ import { ConvexError, v } from "convex/values"; +import { internal } from "./_generated/api"; +import type { ActionCtx } from "./_generated/server"; import { action, internalMutation, internalQuery, query, } from "./_generated/server"; -import { internal } from "./_generated/api"; // ── skillDetails (full SKILL.md content, cached) ──────────────────── @@ -108,7 +109,6 @@ export const searchSkillsIndex = query({ }, }); - /** Upsert a batch of skills discovered from skills.sh search API */ export const upsertSkillsIndexBatch = internalMutation({ args: { @@ -254,7 +254,10 @@ async function fetchSkillMdFromRepo( const normalizedId = normalizeSkillId(skillId); const branches = ["main", "master"]; - const idsToTry = [skillId, ...(normalizedId !== skillId ? [normalizedId] : [])]; + const idsToTry = [ + skillId, + ...(normalizedId !== skillId ? [normalizedId] : []), + ]; // 1. Try direct paths (both branches) for (const branch of branches) { @@ -312,8 +315,7 @@ async function fetchSkillMdFromRepo( const dir = p.split("/").slice(-2, -1)[0] ?? ""; const normDir = dir.toLowerCase(); return ( - normalizedId.includes(normDir) || - normDir.includes(normalizedId) + normalizedId.includes(normDir) || normDir.includes(normalizedId) ); }); @@ -326,9 +328,7 @@ async function fetchSkillMdFromRepo( } // Check for a shallow SKILL.md (e.g. skill/SKILL.md at repo root) - const rootSkillMd = skillFiles.find( - (p) => p.split("/").length <= 2, - ); + const rootSkillMd = skillFiles.find((p) => p.split("/").length <= 2); if (rootSkillMd) { const mdResp = await fetch( `${ghRaw}/${source}/${branch}/${rootSkillMd}`, @@ -414,17 +414,50 @@ async function fetchRepoFile( return null; } +// Folder prefixes that read as a skills "root": stripped so the remaining +// folders become the human-facing category. Order matters (longest first). +const SKILL_ROOTS = [ + ".claude/skills", + ".agents/skills", + ".github/skills", + "skills", +]; + +type DiscoveredSkill = { skillId: string; path: string; category: string }; + +/** Drop a leading skills-root prefix from a skill's parent folders so the + * remainder reads as a category (e.g. ["skills","engineering"] → "engineering", + * ["skills"] → "", ["packages","x","skills","ui"] → "packages/x/skills/ui"). */ +function skillCategory(dirsAboveSkill: string[]): string { + for (const root of SKILL_ROOTS) { + const segs = root.split("/"); + if ( + dirsAboveSkill.length >= segs.length && + segs.every((s, i) => dirsAboveSkill[i] === s) + ) { + return dirsAboveSkill.slice(segs.length).join("/"); + } + } + return dirsAboveSkill.join("/"); +} + /** - * List the skill directory names in a repo via the git/trees API: any - * `//SKILL.md` where base is a known skills dir. One API call - * per branch — handles repos like greensock/gsap-skills (skills//SKILL.md). + * Discover every SKILL.md in a repo at ANY depth via one git/trees call per + * branch. Handles flat repos (`skills//SKILL.md`) AND nested, category- + * organized ones (`skills///SKILL.md`, any depth) like + * mattpocock/skills — the previous logic only matched a fixed set of top-level + * bases and silently found nothing in nested repos. Returns each skill's leaf + * id, full path (for a direct content fetch), and display category. */ -async function listRepoSkillIds( - source: string, -): Promise<{ ids: string[]; rateLimited: boolean; notFound: boolean }> { - const bases = new Set(["skills", ".agents/skills", ".claude/skills"]); +async function discoverRepoSkills(source: string): Promise<{ + skills: DiscoveredSkill[]; + rateLimited: boolean; + notFound: boolean; + truncated: boolean; +}> { let rateLimited = false; let notFound = false; + let sawRepo = false; for (const branch of ["main", "master"]) { try { const resp = await fetch( @@ -449,133 +482,239 @@ async function listRepoSkillIds( continue; } if (!resp.ok) continue; + // The branch resolved: the repo exists even if it has zero skills — so + // the caller reports "no skills found", not "repo not found" (a 404 on + // the OTHER branch must not override that). + sawRepo = true; const data = (await resp.json()) as { + truncated?: boolean; tree?: Array<{ path: string; type: string }>; }; - const ids = new Set(); + const seen = new Set(); + const skills: DiscoveredSkill[] = []; for (const e of data.tree ?? []) { if (e.type !== "blob" || !e.path.endsWith("/SKILL.md")) continue; + if (e.path.includes("node_modules/")) continue; const parts = e.path.split("/"); - const id = parts[parts.length - 2]; - const base = parts.slice(0, parts.length - 2).join("/"); - if (id && bases.has(base)) ids.add(id); + const skillId = parts[parts.length - 2]; + if (!skillId) continue; // a bare repo-root SKILL.md has no folder + // The leaf dir name is how both the viewer and the gateway backfill + // resolve a skill, so it must be unique within the repo; keep the + // first and skip a later collision rather than shadow it. + if (seen.has(skillId)) continue; + seen.add(skillId); + skills.push({ + skillId, + path: e.path, + category: skillCategory(parts.slice(0, parts.length - 2)), + }); } - if (ids.size > 0) { - return { ids: [...ids], rateLimited: false, notFound: false }; + if (skills.length > 0) { + skills.sort( + (a, b) => + a.category.localeCompare(b.category) || + a.skillId.localeCompare(b.skillId), + ); + return { + skills, + rateLimited: false, + notFound: false, + truncated: data.truncated === true, + }; } } catch { // try next branch } } - return { ids: [], rateLimited, notFound }; + return { + skills: [], + rateLimited, + notFound: sawRepo ? false : notFound, + truncated: false, + }; +} + +const MAX_REPO_IMPORT_SKILLS = 100; + +function normalizeRepoSource(raw: string): string { + return raw + .trim() + .replace(/^https?:\/\/github\.com\//i, "") + .replace(/\.git$/i, "") + .replace(/\/+$/, ""); } -const MAX_REPO_IMPORT_SKILLS = 60; +/** Normalize + validate a repo source, throwing a user-facing ConvexError. */ +function resolveRepoSource(raw: string): string { + const source = normalizeRepoSource(raw); + const segments = source.split("/"); + if ( + !/^[\w.-]+\/[\w.-]+$/.test(source) || + segments.some((s) => s === "." || s === "..") + ) { + // ConvexError (not Error): the deployment masks plain Errors as a + // generic "Server Error", so user-facing reasons must use ConvexError. + throw new ConvexError( + "Enter a GitHub repo as owner/repo (e.g. mattpocock/skills).", + ); + } + return source; +} + +/** Discover skills in a repo (throwing user-facing errors on rate-limit / not- + * found / empty), then fetch + cache + index each SKILL.md. Shared by the + * browse-and-select flow (listRepoSkills) and the add-all flow + * (importSkillRepo). */ +async function discoverAndCacheRepoSkills( + ctx: ActionCtx, + source: string, +): Promise<{ + skills: Array<{ + skillId: string; + fullId: string; + category: string; + description: string; + }>; + discovered: number; + truncated: boolean; +}> { + const { + skills: discovered, + rateLimited, + notFound, + truncated, + } = await discoverRepoSkills(source); + if (rateLimited) { + throw new ConvexError( + "GitHub's API rate limit was hit while reading the repo. Set a " + + "GITHUB_TOKEN environment variable on the Convex deployment to " + + "raise it (5000/hr), then try again.", + ); + } + if (discovered.length === 0) { + throw new ConvexError( + notFound + ? `Couldn't find a public GitHub repo "${source}" with any SKILL.md files.` + : `No skills found in ${source}. Expected SKILL.md files (e.g. skills//SKILL.md, at any depth).`, + ); + } + const limited = discovered.slice(0, MAX_REPO_IMPORT_SKILLS); + + const out: Array<{ + skillId: string; + fullId: string; + category: string; + description: string; + }> = []; + const indexEntries: Array<{ + skillId: string; + fullId: string; + source: string; + description: string; + installs: number; + }> = []; + + const CONCURRENCY = 6; + async function processSkill(d: DiscoveredSkill) { + const fullId = `${source}/${d.skillId}`; + // We already know the exact path from the tree, so fetch it directly; + // fall back to the resilient resolver only if that misses. + const detail = + (await fetchRepoFile(source, d.path)) ?? + (await fetchSkillMd(source, d.skillId)); + if (!detail) return; + const description = parseSkillDescription(detail); + await ctx.runMutation(internal.skills.upsertSkillDetail, { + name: fullId, + skillName: d.skillId, + description, + detail, + code: `npx skills add https://github.com/${source} --skill ${d.skillId}`, + }); + out.push({ skillId: d.skillId, fullId, category: d.category, description }); + indexEntries.push({ + skillId: d.skillId, + fullId, + source, + description, + installs: 0, + }); + } + for (let i = 0; i < limited.length; i += CONCURRENCY) { + await Promise.all(limited.slice(i, i + CONCURRENCY).map(processSkill)); + } + // Index the discovered skills so they're searchable in the catalog too. + if (indexEntries.length > 0) { + await ctx.runMutation(internal.skills.upsertSkillsIndexBatch, { + skills: indexEntries, + }); + } + out.sort( + (a, b) => + a.category.localeCompare(b.category) || + a.skillId.localeCompare(b.skillId), + ); + return { skills: out, discovered: discovered.length, truncated }; +} /** - * Import every skill in a GitHub repo (e.g. "greensock/gsap-skills") in one - * shot: discover the skill dirs, fetch + cache each SKILL.md, index them, and - * return them (plus the repo's top-level AGENTS.md / CLAUDE.md) so the caller - * can drop them into a skill pack. Reuses the same GitHub resolution as - * ensureSkillDetails (honors GITHUB_TOKEN for rate limits). + * Browse a GitHub repo's skills for the pack editor's select-what-you-want + * flow: discover every SKILL.md at any depth, fetch + cache each (so previews + * are instant), and return them grouped-ready (with a `category`) plus the + * repo's AGENTS.md / CLAUDE.md. The caller chooses which to add. */ -export const importSkillRepo = action({ +export const listRepoSkills = action({ args: { source: v.string() }, handler: async (ctx, args) => { const identity = await ctx.auth.getUserIdentity(); if (!identity) throw new Error("Unauthenticated"); + const source = resolveRepoSource(args.source); - // Accept a bare owner/repo or a github.com URL; normalize to owner/repo. - const source = args.source - .trim() - .replace(/^https?:\/\/github\.com\//i, "") - .replace(/\.git$/i, "") - .replace(/\/+$/, ""); - const segments = source.split("/"); - if ( - !/^[\w.-]+\/[\w.-]+$/.test(source) || - segments.some((s) => s === "." || s === "..") - ) { - // ConvexError (not Error): the deployment masks plain Errors as a - // generic "Server Error", so user-facing reasons must use ConvexError. - throw new ConvexError( - "Enter a GitHub repo as owner/repo (e.g. greensock/gsap-skills).", - ); - } + const [{ skills, discovered, truncated }, agentsMd, claudeMd] = + await Promise.all([ + discoverAndCacheRepoSkills(ctx, source), + fetchRepoFile(source, "AGENTS.md"), + fetchRepoFile(source, "CLAUDE.md"), + ]); - const { ids: skillIds, rateLimited, notFound } = - await listRepoSkillIds(source); - if (rateLimited) { - throw new ConvexError( - "GitHub's API rate limit was hit while reading the repo. Set a " + - "GITHUB_TOKEN environment variable on the Convex deployment to " + - "raise it (5000/hr), then try again.", - ); - } - if (skillIds.length === 0) { - throw new ConvexError( - notFound - ? `Couldn't find a public GitHub repo "${source}" with a skills/ folder.` - : `No skills found in ${source}. Expected SKILL.md files at skills//SKILL.md.`, - ); - } - const limited = skillIds.slice(0, MAX_REPO_IMPORT_SKILLS); + return { + source, + skills, + agentsMd: agentsMd ?? "", + claudeMd: claudeMd ?? "", + discovered, + truncated, + }; + }, +}); - const [agentsMd, claudeMd] = await Promise.all([ +/** + * Import EVERY skill in a GitHub repo in one shot (used by the curated + * templates). Discovers at any depth, fetches + caches each SKILL.md, and + * returns them (plus AGENTS.md / CLAUDE.md) as pack entries. + */ +export const importSkillRepo = action({ + args: { source: v.string() }, + handler: async (ctx, args) => { + const identity = await ctx.auth.getUserIdentity(); + if (!identity) throw new Error("Unauthenticated"); + const source = resolveRepoSource(args.source); + + const [{ skills, discovered }, agentsMd, claudeMd] = await Promise.all([ + discoverAndCacheRepoSkills(ctx, source), fetchRepoFile(source, "AGENTS.md"), fetchRepoFile(source, "CLAUDE.md"), ]); - const skills: Array<{ name: string; description: string }> = []; - const indexEntries: Array<{ - skillId: string; - fullId: string; - source: string; - description: string; - installs: number; - }> = []; - - const CONCURRENCY = 4; - async function processSkill(skillId: string) { - const name = `${source}/${skillId}`; - const detail = await fetchSkillMd(source, skillId); - if (!detail) return; - const description = parseSkillDescription(detail); - await ctx.runMutation(internal.skills.upsertSkillDetail, { - name, - skillName: skillId, - description, - detail, - code: `npx skills add https://github.com/${source} --skill ${skillId}`, - }); - skills.push({ name, description }); - indexEntries.push({ - skillId, - fullId: name, - source, - description, - installs: 0, - }); - } - for (let i = 0; i < limited.length; i += CONCURRENCY) { - await Promise.all(limited.slice(i, i + CONCURRENCY).map(processSkill)); - } - - // Index the discovered skills so they're searchable in the catalog too. - if (indexEntries.length > 0) { - await ctx.runMutation(internal.skills.upsertSkillsIndexBatch, { - skills: indexEntries, - }); - } - - // Stable order (the repo tree order isn't meaningful). - skills.sort((a, b) => a.name.localeCompare(b.name)); return { source, - skills, + skills: skills.map((s) => ({ + name: s.fullId, + description: s.description, + })), agentsMd: agentsMd ?? "", claudeMd: claudeMd ?? "", - discovered: skillIds.length, + discovered, imported: skills.length, }; }, @@ -620,7 +759,7 @@ export const ensureSkillDetails = action({ // skillId is the portion of fullId after the source prefix skillId = name.startsWith(source + "/") ? name.slice(source.length + 1) - : name.split("/").pop() ?? name; + : (name.split("/").pop() ?? name); } else { const parts = name.split("/"); skillId = parts.pop() ?? name; @@ -731,7 +870,6 @@ export const discoverSkillsFromSearch = action({ ), }, handler: async (ctx, args): Promise => { - const identity = await ctx.auth.getUserIdentity(); if (!identity) return 0; // Check which ones we already have @@ -742,9 +880,7 @@ export const discoverSkillsFromSearch = action({ ); const existingSet = new Set(existingIds); - const newSkills = args.skills.filter( - (s) => !existingSet.has(s.fullId), - ); + const newSkills = args.skills.filter((s) => !existingSet.has(s.fullId)); if (newSkills.length === 0) return 0; // Just insert index entries with empty descriptions — SKILL.md fetching @@ -773,7 +909,9 @@ export const searchForCreationAssistant = query({ const [byName, byDesc] = await Promise.all([ ctx.db .query("skillsIndex") - .withSearchIndex("search_skills", (q) => q.search("skillId", args.query)) + .withSearchIndex("search_skills", (q) => + q.search("skillId", args.query), + ) .take(20), ctx.db .query("skillsIndex")