Skip to content

Commit b2c6280

Browse files
saltbosaltbo
authored
fix(agent): validate installable skill refs
1 parent 3165561 commit b2c6280

10 files changed

Lines changed: 122 additions & 33 deletions

File tree

‎apps/web/server/routes.ts‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,12 @@
1-
import { AGENT_RUNTIMES, type CreateAgentInput, isBoardType, isValidUsername, parseScheduledAt, RESERVED_ROLES } from "@agent-kanban/shared";
1+
import {
2+
AGENT_RUNTIMES,
3+
type CreateAgentInput,
4+
findInvalidSkillRef,
5+
isBoardType,
6+
isValidUsername,
7+
parseScheduledAt,
8+
RESERVED_ROLES,
9+
} from "@agent-kanban/shared";
210
import { Hono } from "hono";
311
import { HTTPException } from "hono/http-exception";
412
import {
@@ -51,6 +59,17 @@ import type { Env } from "./types";
5159
const api = new Hono<{ Bindings: Env }>();
5260
const logger = createLogger("api");
5361

62+
function assertValidSkillRefs(skills: unknown) {
63+
if (skills === undefined) return;
64+
if (!Array.isArray(skills) || skills.some((skill) => typeof skill !== "string")) {
65+
throw new HTTPException(400, { message: "skills must be an array of source/repo@skill-name strings" });
66+
}
67+
const invalid = findInvalidSkillRef(skills);
68+
if (invalid) {
69+
throw new HTTPException(400, { message: `Invalid skill "${invalid}". Use source/repo@skill-name format.` });
70+
}
71+
}
72+
5473
function resolveActor(c: { get: (key: string) => any }): { actorType: string; actorId: string; sessionId: string | null } {
5574
const identity: string = c.get("identityType") || "machine";
5675
let actorId: string;
@@ -415,6 +434,7 @@ api.post("/api/agents", async (c) => {
415434
if (body.role && RESERVED_ROLES.has(body.role)) {
416435
throw new HTTPException(403, { message: `Role "${body.role}" is reserved for built-in agents` });
417436
}
437+
assertValidSkillRefs(body.skills);
418438
const ownerId = c.get("ownerId");
419439

420440
// Validate username uniqueness before GPG mutation
@@ -466,6 +486,7 @@ api.patch("/api/agents/:id", async (c) => {
466486
if (!existing) throw new HTTPException(404, { message: "Agent not found" });
467487
if (existing.builtin) throw new HTTPException(403, { message: "Built-in agents cannot be modified" });
468488
const body = await c.req.json();
489+
assertValidSkillRefs(body.skills);
469490
const agent = await updateAgent(c.env.DB, c.req.param("id"), body);
470491
return c.json(agent);
471492
});

‎apps/web/src/routes/AgentEditPage.tsx‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { AGENT_RUNTIMES, type AgentRuntime, RUNTIME_LABELS } from "@agent-kanban/shared";
1+
import { AGENT_RUNTIMES, type AgentRuntime, findInvalidSkillRef, RUNTIME_LABELS } from "@agent-kanban/shared";
22
import React, { useEffect, useRef, useState } from "react";
33
import { Link, useNavigate, useParams } from "react-router-dom";
44
import { AgentIdenticon } from "../components/AgentIdenticon";
@@ -67,6 +67,11 @@ export function AgentEditPage() {
6767
async function handleSave() {
6868
if (!name.trim()) return;
6969
setError(null);
70+
const invalidSkill = findInvalidSkillRef(skills);
71+
if (invalidSkill) {
72+
setError(`Invalid skill "${invalidSkill}". Use source/repo@skill-name format.`);
73+
return;
74+
}
7075
try {
7176
await updateAgent.mutateAsync({
7277
id: agent!.id,
@@ -187,7 +192,7 @@ export function AgentEditPage() {
187192
<legend className="text-[11px] font-mono font-medium text-content-tertiary uppercase tracking-[0.08em] mb-3">Workflow</legend>
188193
<div className="space-y-1.5">
189194
<Label>Skills</Label>
190-
<TagInput tags={skills} onChange={setSkills} placeholder="Type a skill and press Enter" />
195+
<TagInput tags={skills} onChange={setSkills} placeholder="owner/repo@skill-name" />
191196
</div>
192197
</fieldset>
193198

‎apps/web/src/routes/AgentNewPage.tsx‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import {
44
type AgentTemplate,
55
fetchTemplate,
66
fetchTemplateIndex,
7+
findInvalidSkillRef,
78
RUNTIME_LABELS,
89
type TemplateIndex,
910
} from "@agent-kanban/shared";
@@ -72,6 +73,11 @@ export function AgentNewPage() {
7273
async function handleCreate() {
7374
if (!username.trim()) return;
7475
setError(null);
76+
const invalidSkill = findInvalidSkillRef(skills);
77+
if (invalidSkill) {
78+
setError(`Invalid skill "${invalidSkill}". Use source/repo@skill-name format.`);
79+
return;
80+
}
7581
try {
7682
await createAgent.mutateAsync({
7783
name: name.trim() || undefined,
@@ -463,7 +469,7 @@ function FormStep(props: FormStepProps) {
463469
</div>
464470
<div className="space-y-1.5">
465471
<Label>Skills</Label>
466-
<TagInput tags={skills} onChange={setSkills} placeholder="Type a skill and press Enter" />
472+
<TagInput tags={skills} onChange={setSkills} placeholder="owner/repo@skill-name" />
467473
</div>
468474
</fieldset>
469475

‎packages/shared/src/types.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,16 @@ export function deriveUsername(name: string): string {
155155
return derived || "agent";
156156
}
157157

158+
const SKILL_REF_RE = /^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+@[A-Za-z0-9_.-]+$/;
159+
160+
export function isValidSkillRef(value: string): boolean {
161+
return SKILL_REF_RE.test(value);
162+
}
163+
164+
export function findInvalidSkillRef(skills: string[] | null | undefined): string | null {
165+
return skills?.find((skill) => !isValidSkillRef(skill)) ?? null;
166+
}
167+
158168
export const AGENT_RUNTIMES: readonly AgentRuntime[] = ["claude", "codex", "gemini", "copilot", "hermes"] as const;
159169

160170
export const RUNTIME_LABELS: Record<AgentRuntime, string> = {

‎tests/agent-kind-handoff-skills.test.ts‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -103,23 +103,23 @@ describe("create agent: --skills flag", () => {
103103
name: "Test Skills",
104104
runtime: "claude",
105105
role: "dev",
106-
skills: ["agent-kanban"],
106+
skills: ["saltbo/agent-kanban@agent-kanban"],
107107
});
108108

109109
expect(Array.isArray(agent.skills)).toBe(true);
110-
expect(agent.skills).toEqual(["agent-kanban"]);
110+
expect(agent.skills).toEqual(["saltbo/agent-kanban@agent-kanban"]);
111111
});
112112

113113
it("multiple skills are stored as array", async () => {
114114
const agent = await createTestAgent(db, "owner-skills", {
115115
username: "test-skills-multi",
116116
name: "Test Skills Multi",
117117
runtime: "claude",
118-
skills: ["agent-kanban", "trailofbits/skills@differential-review"],
118+
skills: ["saltbo/agent-kanban@agent-kanban", "trailofbits/skills@differential-review"],
119119
});
120120

121121
expect(agent.skills).toHaveLength(2);
122-
expect(agent.skills).toContain("agent-kanban");
122+
expect(agent.skills).toContain("saltbo/agent-kanban@agent-kanban");
123123
expect(agent.skills).toContain("trailofbits/skills@differential-review");
124124
});
125125
});
@@ -146,21 +146,21 @@ describe("update agent: --handoff-to, --skills flags", () => {
146146

147147
it("updates skills via updateAgent", async () => {
148148
const { updateAgent } = await import("../apps/web/server/agentRepo");
149-
const updated = await updateAgent(db, agentId, { skills: ["agent-kanban"] });
149+
const updated = await updateAgent(db, agentId, { skills: ["saltbo/agent-kanban@agent-kanban"] });
150150

151151
expect(updated).toBeTruthy();
152-
expect(updated!.skills).toEqual(["agent-kanban"]);
152+
expect(updated!.skills).toEqual(["saltbo/agent-kanban@agent-kanban"]);
153153
});
154154

155155
it("handoff_to and skills persist together", async () => {
156156
const { updateAgent, getAgent } = await import("../apps/web/server/agentRepo");
157157
await updateAgent(db, agentId, {
158158
handoff_to: ["final-leader"],
159-
skills: ["agent-kanban", "browse"],
159+
skills: ["saltbo/agent-kanban@agent-kanban", "trailofbits/skills@differential-review"],
160160
});
161161

162162
const fetched = await getAgent(db, agentId, "owner-update");
163163
expect(fetched!.handoff_to).toEqual(["final-leader"]);
164-
expect(fetched!.skills).toEqual(["agent-kanban", "browse"]);
164+
expect(fetched!.skills).toEqual(["saltbo/agent-kanban@agent-kanban", "trailofbits/skills@differential-review"]);
165165
});
166166
});

‎tests/agents/agents-new-custom.spec.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ test.describe("Agents Page", () => {
3333

3434
// expect: Workflow fieldset with 'Handoff to' and 'Skills' fields is visible
3535
await expect(page.getByRole("group", { name: "Workflow" })).toBeVisible();
36-
await expect(page.getByRole("textbox", { name: "Type a skill and press Enter" })).toBeVisible();
36+
await expect(page.getByRole("textbox", { name: "owner/repo@skill-name" })).toBeVisible();
3737

3838
// expect: A live preview card is shown on the right side
3939
await expect(page.getByText("Preview")).toBeVisible();

‎tests/agents/agents-new-skills-remove.spec.ts‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -6,25 +6,25 @@ import { signUpAndGetBoard } from "../helpers/auth";
66

77
test.describe("Agents Page", () => {
88
test("Agent creation — remove a skill tag", async ({ page }) => {
9-
// 1. Sign in, navigate to /agents/new, click 'Custom', add the skill 'python' using the Skills field
9+
// 1. Sign in, navigate to /agents/new, click 'Custom', add a skill using the Skills field
1010
await signUpAndGetBoard(page, `agents_rmskill_${Date.now()}@example.com`);
1111
await page.goto("/agents/new");
1212
await page.getByRole("button", { name: "Custom Build your own from" }).click();
1313

14-
const skillsInput = page.getByRole("textbox", { name: "Type a skill and press Enter" });
14+
const skillsInput = page.getByRole("textbox", { name: "owner/repo@skill-name" });
1515
await skillsInput.click();
16-
await skillsInput.fill("python");
16+
await skillsInput.fill("owner/skills@python");
1717
await page.keyboard.press("Enter");
1818

19-
// expect: A 'python' skill tag is shown
19+
// expect: A skill tag is shown
2020
const workflowGroup = page.getByRole("group", { name: "Workflow" });
21-
await expect(workflowGroup.getByText("python")).toBeVisible();
21+
await expect(workflowGroup.getByText("owner/skills@python")).toBeVisible();
2222

23-
// 2. Click the '×' button on the 'python' tag chip
23+
// 2. Click the remove button on the skill tag chip
2424
await workflowGroup.getByRole("button").click();
2525

26-
// expect: The 'python' tag is removed from the Skills field
27-
await expect(workflowGroup.getByText("python")).not.toBeVisible();
26+
// expect: The tag is removed from the Skills field
27+
await expect(workflowGroup.getByText("owner/skills@python")).not.toBeVisible();
2828

2929
// expect: The skills input placeholder is visible again
3030
await expect(skillsInput).toBeVisible();

‎tests/agents/agents-new-skills-tag.spec.ts‎

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -13,29 +13,28 @@ test.describe("Agents Page", () => {
1313

1414
// expect: Skills tag input is visible with placeholder text
1515
const workflowGroup = page.getByRole("group", { name: "Workflow" });
16-
const skillsInput = workflowGroup.getByRole("textbox", { name: "Type a skill and press Enter" });
16+
const skillsInput = workflowGroup.getByRole("textbox", { name: "owner/repo@skill-name" });
1717
await expect(skillsInput).toBeVisible();
1818

19-
// 2. Click the Skills field, type 'typescript', and press Enter
19+
// 2. Click the Skills field, type a valid skill ref, and press Enter
2020
await skillsInput.click();
21-
await skillsInput.fill("typescript");
21+
await skillsInput.fill("owner/skills@typescript");
2222
await page.keyboard.press("Enter");
2323

24-
// expect: A 'typescript' tag chip appears in the input
25-
await expect(workflowGroup.getByText("typescript")).toBeVisible();
24+
// expect: A skill tag chip appears in the input
25+
await expect(workflowGroup.getByText("owner/skills@typescript")).toBeVisible();
2626

2727
// expect: The text input clears for the next entry
2828
// After a tag is added, the placeholder is hidden but the input is still present
2929
const skillsInputAfterFirstTag = workflowGroup.locator('input[type="text"], input:not([type])').last();
3030
await expect(skillsInputAfterFirstTag).toHaveValue("");
3131

32-
// 3. Type 'react' and press Enter
33-
await skillsInputAfterFirstTag.fill("react");
32+
// 3. Type another valid skill ref and press Enter
33+
await skillsInputAfterFirstTag.fill("owner/skills@react");
3434
await page.keyboard.press("Enter");
3535

36-
// expect: A 'react' tag chip also appears
37-
// expect: Two skill tags are now visible: 'typescript' and 'react'
38-
await expect(workflowGroup.getByText("typescript")).toBeVisible();
39-
await expect(workflowGroup.getByText("react")).toBeVisible();
36+
// expect: Two skill tags are now visible
37+
await expect(workflowGroup.getByText("owner/skills@typescript")).toBeVisible();
38+
await expect(workflowGroup.getByText("owner/skills@react")).toBeVisible();
4039
});
4140
});

‎tests/routes.test.ts‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -394,6 +394,26 @@ describe("routes", () => {
394394
expect(res.status).toBe(403);
395395
});
396396

397+
it("POST /api/agents rejects malformed skill refs", async () => {
398+
const res = await apiRequest(
399+
"POST",
400+
"/api/agents",
401+
{ name: "Bad Skill", username: "bad-skill", runtime: "claude", skills: ["agent-kanban"] },
402+
apiKey,
403+
);
404+
expect(res.status).toBe(400);
405+
const body = (await res.json()) as any;
406+
expect(body.error.message).toContain('Invalid skill "agent-kanban"');
407+
});
408+
409+
it("PATCH /api/agents/:id rejects malformed skill refs", async () => {
410+
const jwt = await signLeaderSessionJWT();
411+
const res = await apiRequest("PATCH", `/api/agents/${agentId}`, { skills: ["trailofbits/skills"] }, jwt);
412+
expect(res.status).toBe(400);
413+
const body = (await res.json()) as any;
414+
expect(body.error.message).toContain('Invalid skill "trailofbits/skills"');
415+
});
416+
397417
// ─── Tasks ───
398418

399419
it("POST /api/tasks creates a task", async () => {

‎tests/shared.test.ts‎

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,13 @@
1-
import { AGENT_STATUSES, deriveUsername, isValidUsername, PRIORITIES, STALE_TIMEOUT_MS, TASK_ACTIONS } from "@agent-kanban/shared";
1+
import {
2+
AGENT_STATUSES,
3+
deriveUsername,
4+
findInvalidSkillRef,
5+
isValidSkillRef,
6+
isValidUsername,
7+
PRIORITIES,
8+
STALE_TIMEOUT_MS,
9+
TASK_ACTIONS,
10+
} from "@agent-kanban/shared";
211
import { describe, expect, it } from "vitest";
312

413
describe("isValidUsername", () => {
@@ -64,6 +73,25 @@ describe("deriveUsername", () => {
6473
});
6574
});
6675

76+
describe("isValidSkillRef", () => {
77+
it("accepts installable owner/repo@skill-name refs", () => {
78+
expect(isValidSkillRef("trailofbits/skills@differential-review")).toBe(true);
79+
expect(isValidSkillRef("obra/superpowers@verification-before-completion")).toBe(true);
80+
});
81+
82+
it("rejects short names and malformed refs", () => {
83+
expect(isValidSkillRef("agent-kanban")).toBe(false);
84+
expect(isValidSkillRef("trailofbits/skills")).toBe(false);
85+
expect(isValidSkillRef("trailofbits/skills@")).toBe(false);
86+
expect(isValidSkillRef("trailofbits/skills@bad skill")).toBe(false);
87+
});
88+
89+
it("returns the first invalid skill ref", () => {
90+
expect(findInvalidSkillRef(["owner/repo@good", "browse", "other/repo@good"])).toBe("browse");
91+
expect(findInvalidSkillRef(["owner/repo@good"])).toBeNull();
92+
});
93+
});
94+
6795
describe("shared constants", () => {
6896
it("TASK_ACTIONS includes all v2 actions", () => {
6997
expect(TASK_ACTIONS).toContain("assigned");

0 commit comments

Comments
 (0)