Skip to content

Commit 620f7d1

Browse files
dmarticusclaude
dmarticus
and
claude
committed
fix(agents): address PR feedback on agent bundle editor
- Drop /i flag on SKILL_PATH_RE so uppercase skill ids (e.g. skills/MySkill/SKILL.md) are rejected instead of silently captured. - Parameterise parseBundleInput tests with it.each per team convention and add an uppercase-skill-id rejection case. - In EditableMarkdownBody, key the editor on editable.path at the call site for file-switch resets, and guard the in-effect draft sync with !editing so a concurrent bulk-import refetch doesn't silently wipe the textarea mid-edit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 80153b4 commit 620f7d1

3 files changed

Lines changed: 93 additions & 78 deletions

File tree

‎packages/ui/src/features/agent-applications/components/AgentBundleImportDialog.test.ts‎

Lines changed: 83 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -1,87 +1,100 @@
11
import { describe, expect, it } from "vitest";
2-
import { parseBundleInput } from "./AgentBundleImportDialog";
2+
import { type ParsedBundle, parseBundleInput } from "./AgentBundleImportDialog";
33

4-
describe("parseBundleInput", () => {
5-
it("returns an error for empty input", () => {
6-
const out = parseBundleInput("");
7-
expect(out.ok).toBe(false);
8-
});
4+
type Expected =
5+
| { ok: true; value: ParsedBundle }
6+
| { ok: false; errorMatch?: RegExp };
97

10-
it("parses a single agent.md block", () => {
11-
const out = parseBundleInput(
12-
"--- agent.md ---\nYou are the growth review agent.\n",
13-
);
14-
expect(out).toEqual({
8+
const cases: Array<{ name: string; input: string; expected: Expected }> = [
9+
{
10+
name: "rejects empty input",
11+
input: "",
12+
expected: { ok: false },
13+
},
14+
{
15+
name: "parses a single agent.md block",
16+
input: "--- agent.md ---\nYou are the growth review agent.\n",
17+
expected: {
1518
ok: true,
1619
value: { agent_md: "You are the growth review agent." },
17-
});
18-
});
19-
20-
it("parses multiple skill blocks", () => {
21-
const out = parseBundleInput(
22-
[
23-
"--- skills/research/SKILL.md ---",
24-
"Research body",
25-
"--- skills/draft-post/SKILL.md ---",
26-
"Draft body",
27-
].join("\n"),
28-
);
29-
expect(out).toEqual({
20+
},
21+
},
22+
{
23+
name: "parses multiple skill blocks",
24+
input: [
25+
"--- skills/research/SKILL.md ---",
26+
"Research body",
27+
"--- skills/draft-post/SKILL.md ---",
28+
"Draft body",
29+
].join("\n"),
30+
expected: {
3031
ok: true,
3132
value: {
3233
skills: [
3334
{ id: "research", body: "Research body" },
3435
{ id: "draft-post", body: "Draft body" },
3536
],
3637
},
37-
});
38-
});
38+
},
39+
},
40+
{
41+
name: "parses agent.md plus skills together",
42+
input: [
43+
"--- agent.md ---",
44+
"Main prompt",
45+
"",
46+
"--- skills/research/SKILL.md ---",
47+
"Research body",
48+
].join("\n"),
49+
expected: {
50+
ok: true,
51+
value: {
52+
agent_md: "Main prompt",
53+
skills: [{ id: "research", body: "Research body" }],
54+
},
55+
},
56+
},
57+
{
58+
name: "tolerates CRLF line endings",
59+
input: "--- agent.md ---\r\nMain prompt\r\n",
60+
expected: { ok: true, value: { agent_md: "Main prompt" } },
61+
},
62+
{
63+
name: "rejects an unsupported file path",
64+
input: "--- tools/foo/source.ts ---\nconsole.log('hi')\n",
65+
expected: { ok: false, errorMatch: /Unsupported file path/ },
66+
},
67+
{
68+
name: "rejects skill ids with spaces",
69+
input: "--- skills/Bad Id/SKILL.md ---\nbody\n",
70+
expected: { ok: false },
71+
},
72+
{
73+
name: "rejects skill ids with uppercase letters",
74+
input: "--- skills/MySkill/SKILL.md ---\nbody\n",
75+
expected: { ok: false },
76+
},
77+
{
78+
name: "ignores leading content before the first header",
79+
input: [
80+
"# notes for myself, not in any block",
81+
"--- agent.md ---",
82+
"Prompt",
83+
].join("\n"),
84+
expected: { ok: true, value: { agent_md: "Prompt" } },
85+
},
86+
];
3987

40-
it("parses agent.md plus skills together", () => {
41-
const out = parseBundleInput(
42-
[
43-
"--- agent.md ---",
44-
"Main prompt",
45-
"",
46-
"--- skills/research/SKILL.md ---",
47-
"Research body",
48-
].join("\n"),
49-
);
50-
expect(out.ok).toBe(true);
51-
if (out.ok) {
52-
expect(out.value.agent_md).toBe("Main prompt");
53-
expect(out.value.skills).toEqual([
54-
{ id: "research", body: "Research body" },
55-
]);
88+
describe("parseBundleInput", () => {
89+
it.each(cases)("$name", ({ input, expected }) => {
90+
const out = parseBundleInput(input);
91+
if (expected.ok) {
92+
expect(out).toEqual(expected);
93+
return;
5694
}
57-
});
58-
59-
it("tolerates CRLF line endings", () => {
60-
const out = parseBundleInput("--- agent.md ---\r\nMain prompt\r\n");
61-
expect(out).toEqual({ ok: true, value: { agent_md: "Main prompt" } });
62-
});
63-
64-
it("rejects an unsupported file path", () => {
65-
const out = parseBundleInput(
66-
"--- tools/foo/source.ts ---\nconsole.log('hi')\n",
67-
);
68-
expect(out.ok).toBe(false);
69-
if (!out.ok) expect(out.error).toMatch(/Unsupported file path/);
70-
});
71-
72-
it("rejects skill ids with disallowed characters", () => {
73-
const out = parseBundleInput("--- skills/Bad Id/SKILL.md ---\nbody\n");
7495
expect(out.ok).toBe(false);
75-
});
76-
77-
it("ignores leading content before the first header", () => {
78-
const out = parseBundleInput(
79-
[
80-
"# notes for myself, not in any block",
81-
"--- agent.md ---",
82-
"Prompt",
83-
].join("\n"),
84-
);
85-
expect(out).toEqual({ ok: true, value: { agent_md: "Prompt" } });
96+
if (!out.ok && expected.errorMatch) {
97+
expect(out.error).toMatch(expected.errorMatch);
98+
}
8699
});
87100
});

‎packages/ui/src/features/agent-applications/components/AgentBundleImportDialog.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import { useMemo, useState } from "react";
55
import { useImportAgentDraftBundle } from "../hooks/useImportAgentDraftBundle";
66

77
const HEADER_RE = /^---\s*(.+?)\s*---\s*$/;
8-
const SKILL_PATH_RE = /^skills\/([a-z0-9-]+)\/SKILL\.md$/i;
8+
const SKILL_PATH_RE = /^skills\/([a-z0-9-]+)\/SKILL\.md$/;
99

1010
export interface ParsedBundle {
1111
agent_md?: string;

‎packages/ui/src/features/agent-applications/components/AgentConfigurationPane.tsx‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1746,6 +1746,9 @@ function BundleFileBody({
17461746
if (editable) {
17471747
return (
17481748
<EditableMarkdownBody
1749+
// Remount when the user picks a different file so we start fresh
1750+
// instead of carrying draft/edit state across paths via an effect.
1751+
key={editable.path}
17491752
file={file}
17501753
emptyLabel={emptyLabel}
17511754
editable={editable}
@@ -1786,14 +1789,13 @@ function EditableMarkdownBody({
17861789
editable.revisionId,
17871790
);
17881791

1789-
// Reset local state when the underlying content changes — covers the
1790-
// post-save refetch landing and navigation between files (two files with
1791-
// byte-identical content is vanishingly rare). We deliberately don't reset
1792-
// mutation state; a fresh `mutate()` clears any inline error anyway.
1792+
// Pull in upstream content changes (initial bundle load, post-save refetch),
1793+
// but only while the user isn't actively editing — otherwise an unrelated
1794+
// refetch (e.g. a concurrent bulk import) would silently wipe their draft.
1795+
// File-switch resets are handled by `key={editable.path}` at the call site.
17931796
useEffect(() => {
1794-
setDraft(initial);
1795-
setEditing(false);
1796-
}, [initial]);
1797+
if (!editing) setDraft(initial);
1798+
}, [initial, editing]);
17971799

17981800
if (!editing) {
17991801
return (

0 commit comments

Comments
 (0)