diff --git a/frontend/AGENTS.md b/frontend/AGENTS.md index 6edc277df..5a168292f 100644 --- a/frontend/AGENTS.md +++ b/frontend/AGENTS.md @@ -97,6 +97,8 @@ Auth UI note: the login page's "keep me signed in" option submits only `remember `/goal` and `/compact` are built-in composer commands, not skill activations. `src/components/workspace/input-box.tsx` intercepts `/goal`, `/goal clear`, and `/goal ` before normal chat submission, calling Gateway `GET/PUT/DELETE /api/threads/{thread_id}/goal`. Setting `/goal ` also submits the condition text as the next user task so the agent starts running immediately; status and clear do not start a run. Goal and compact requests are tied to the current `threadId` with an `AbortController`, so switching threads or unmounting the composer aborts in-flight requests and stale responses cannot update the new thread's composer state. The chat pages render `GoalStatus` above the composer from `AgentThreadState.goal`, with local optimistic state until the next stream `values` update arrives. `/compact` calls `POST /api/threads/{thread_id}/compact` to summarize older active context while leaving the full visible chat history intact; it is skipped on new/empty threads and blocked server-side while a run is in flight. Thread rename uses the same serialized state-write route; the rename dialog stays open and surfaces the server error when an active run returns 409. +The `/` skill list stays reachable after a skill is selected: typing `/` in the editable text beside the chip reopens it, and picking an entry swaps the chip rather than adding a second one, because the wire format carries exactly one leading `/skill`. That list offers skills only while a chip is selected — a builtin command owns the whole composer line, so `/goal` behind a selected skill would submit as chat text instead of running the command. The trigger itself is unchanged: a slash only opens the list at the start of the input (`getLeadingSlashSkillQuery`), pinned by `tests/e2e/chat.spec.ts`. + Human input requests are a structured message protocol layered on normal chat history. The backend writes request payloads to `ToolMessage.artifact.human_input`, `src/core/messages/human-input.ts` owns the runtime validators/types, and `src/components/workspace/messages/human-input-card.tsx` renders the reusable card. The protocol is versioned on the request side only: v1 covers `free_text` / `choice_with_other`, and v2 adds `form` (typed fields — text/textarea/number/select/multi_select/checkbox/date — with required-field validation in the card). Replies deliberately stay on the v1 response protocol: the form card submits a `response_kind: "text"` reply whose value is the human-readable summary plus one JSON block keyed by stable field names (`buildHumanInputFormSubmissionValue` — the readable part alone is ambiguous because labels/values may contain the separators), so the model can reconstruct the submitted mapping without a structured response kind. The validators reject unknown versions/modes (and field names colliding with JS `Object.prototype` members) so future protocol bumps degrade to the plain-text ToolMessage fallback rather than rendering a broken card. Form values are read through own-property access only (`readHumanInputFormValue`); select fields stay controlled from their empty-string placeholder state through selection; checkbox fields are native `` controls seeded to an explicit `false` (`buildInitialHumanInputFormValues`) so an untouched checkbox submits as "no" while a `required` checkbox keeps must-agree semantics (no HTML `required` attribute — native constraint validation would intercept the custom submit path), and form controls carry label/`htmlFor`, `aria-required` plus a visually-hidden localized "required" marker, and `aria-invalid`/error associations whose error node stays mounted while any field is still invalid. Composer-bypass closure: `deriveHumanInputThreadState` treats a visible plain human message as answering the latest unanswered request opened before it (only the latest — nothing guarantees a single outstanding request across runs, and closing all would silently swallow older decisions; an older request left open simply becomes the active card again). This lets current users bypass a structured form through the normal composer and preserves compatibility with old v1-only frontends that degrade a v2 request to plain text. `MessageList` owns answered/latest/pending state for visible cards, but derives answered responses from raw `thread.messages` because replies are hidden; pending cards clear when the hidden reply appears, when dispatch is dropped, or when a new `thread.error` reports an async stream failure. Page-level card submit callbacks must send a normal human message and put `hide_from_ui: true` plus the response payload in the fourth `sendMessage(..., options)` argument as `options.additionalKwargs`; the third argument remains run context such as `{ agent_name }`. Composer entry points remain enabled while a human-input request is open; a normal visible message intentionally bypasses the card and starts the next run without structured response metadata. Tool-calling AI messages can contain user-visible text as well as `tool_calls`. `core/messages/utils.ts` keeps these turns in an `assistant:processing` group, and `components/workspace/messages/message-group.tsx` must render the visible text as a processing step instead of treating the message as only tool metadata. This preserves provider text such as error explanations or "trying another approach" notes during tool-heavy runs. diff --git a/frontend/src/components/workspace/input-box-helpers.ts b/frontend/src/components/workspace/input-box-helpers.ts index 90f924b57..2798c50a0 100644 --- a/frontend/src/components/workspace/input-box-helpers.ts +++ b/frontend/src/components/workspace/input-box-helpers.ts @@ -1,4 +1,4 @@ -import type { Skill } from "@/core/skills"; +import { RESERVED_SLASH_SKILL_NAMES, type Skill } from "@/core/skills"; export { SUGGESTION_TEMPLATE_PLACEHOLDER_PATTERN, findSuggestionTemplatePlaceholder, @@ -167,9 +167,15 @@ export function getMatchingSkillSuggestions( builtinCommands: SlashSuggestion[], ): SlashSuggestion[] { const normalizedQuery = query.toLowerCase(); - const builtinCommandNames = new Set( - builtinCommands.map(({ name }) => name.toLowerCase()), - ); + // A name the slash parsers refuse must not be offered here either. Both + // parsers drop `RESERVED_SLASH_SKILL_NAMES` (the shared contract), and the + // builtin commands own their own names in the composer, so a skill carrying + // either one is unreachable: submitting it either runs the command or + // reaches the model as literal text with nothing activated. + const reservedNames = new Set([ + ...RESERVED_SLASH_SKILL_NAMES, + ...builtinCommands.map(({ name }) => name.toLowerCase()), + ]); const builtinMatches = builtinCommands.filter(({ name, description }) => { if (!normalizedQuery) { @@ -191,7 +197,7 @@ export function getMatchingSkillSuggestions( if (!skill.enabled) { return false; } - if (builtinCommandNames.has(name)) { + if (reservedNames.has(name)) { return false; } return !normalizedQuery || name.includes(normalizedQuery); diff --git a/frontend/src/components/workspace/input-box.tsx b/frontend/src/components/workspace/input-box.tsx index dad469888..73b412392 100644 --- a/frontend/src/components/workspace/input-box.tsx +++ b/frontend/src/components/workspace/input-box.tsx @@ -1296,21 +1296,30 @@ export function InputBox({ () => getGoalObjectiveCounter(textInput.value ?? ""), [textInput.value], ); - const skillSuggestions = useMemo( - () => - slashSkillQuery === null - ? [] - : getMatchingSkillSuggestions( - skills, - slashSkillQuery, - builtinSlashCommands, - ), - [builtinSlashCommands, skills, slashSkillQuery], - ); + const skillSuggestions = useMemo(() => { + if (slashSkillQuery === null) { + return []; + } + const matches = getMatchingSkillSuggestions( + skills, + slashSkillQuery, + builtinSlashCommands, + ); + // Builtin commands own the whole composer line, so they cannot be combined + // with a skill activation: `/goal` behind a selected skill would submit as + // chat text instead of running the command. Drop them from the result + // rather than withholding them from the helper, which needs the list to + // reserve their names — a skill named after a builtin is unusable for the + // mirrored reason, its submitted text runs the command, not the skill. + return selectedSlashSkill + ? matches.filter(({ kind }) => kind === "skill") + : matches; + }, [builtinSlashCommands, selectedSlashSkill, skills, slashSkillQuery]); + // A selected skill does not close the catalog: `/` reopens it so a skill can + // be found by browsing and swapped without first clearing the chip. const showSkillSuggestions = !disabled && textareaFocused && - !selectedSlashSkill && slashSkillQuery !== null && skillSuggestions.length > 0 && dismissedSkillSuggestionValue !== textInput.value; @@ -1896,6 +1905,16 @@ export function InputBox({ const handleInlineSkillKeyDown = useCallback( (event: KeyboardEvent) => { + // The catalog can be reopened from here, so its navigation keys must win + // over Enter-to-submit. Skip it mid-composition, where Enter belongs to + // the IME candidate rather than the list. + if (!isIMEComposing(event, inlineSkillComposingRef.current)) { + handleSkillSuggestionKeyDown(event); + if (event.defaultPrevented) { + return; + } + } + handleSelectedSlashSkillKeyDown(event); if (event.defaultPrevented) { return; @@ -1920,7 +1939,11 @@ export function InputBox({ event.currentTarget.closest("form")?.requestSubmit(); }, - [handleSelectedSlashSkillKeyDown, updateInlineSkillTextInput], + [ + handleSelectedSlashSkillKeyDown, + handleSkillSuggestionKeyDown, + updateInlineSkillTextInput, + ], ); const clearSelectedSlashSkill = useCallback(() => { diff --git a/frontend/tests/e2e/chat.spec.ts b/frontend/tests/e2e/chat.spec.ts index f27366b83..be99520d1 100644 --- a/frontend/tests/e2e/chat.spec.ts +++ b/frontend/tests/e2e/chat.spec.ts @@ -460,6 +460,136 @@ test.describe("Chat workspace", () => { .toBe("/data-analysis summarize this dataset"); }); + test("reopens the skill list with a slash after a skill is selected", async ({ + page, + }) => { + let submittedText: string | undefined; + await page.route("**/runs/stream", (route) => { + const body = route.request().postDataJSON() as { + input?: { messages?: Array<{ content?: unknown }> }; + }; + submittedText = textFromMessageContent( + body.input?.messages?.at(-1)?.content, + ); + return handleRunStream(route); + }); + + await page.goto("/workspace/chats/new"); + + const textarea = page.getByPlaceholder(/how can i assist you/i); + await expect(textarea).toBeVisible({ timeout: 15_000 }); + + await textarea.fill("/dat"); + await expect( + page.getByRole("option", { name: /data-analysis/i }), + ).toBeVisible(); + await textarea.press("Enter"); + await expect(page.getByText("/data-analysis")).toBeVisible(); + + const skillInput = page.getByRole("textbox", { + name: /how can i assist you/i, + }); + await expect(skillInput).toBeVisible(); + + await skillInput.pressSequentially("/"); + + const dataAnalysis = page.getByRole("option", { name: /data-analysis/i }); + const frontendDesign = page.getByRole("option", { + name: /frontend-design/i, + }); + await expect(dataAnalysis).toBeVisible(); + await expect(frontendDesign).toBeVisible(); + // Builtin commands own the whole composer line, so they stay out of the + // list while a skill is selected even though an empty query matches them. + await expect(page.getByRole("option", { name: /goal/i })).toBeHidden(); + + await skillInput.pressSequentially("fro"); + await expect(frontendDesign).toHaveAttribute("aria-selected", "true"); + + await skillInput.press("Enter"); + + await expect(page.getByText("/frontend-design")).toBeVisible(); + await expect(page.getByText("/data-analysis")).toBeHidden(); + + await skillInput.pressSequentially("polish the composer"); + await skillInput.press("Enter"); + + await expect + .poll(() => submittedText) + .toBe("/frontend-design polish the composer"); + }); + + test("does not offer a skill whose name a slash command owns", async ({ + page, + }) => { + // Registered after the shared mock, so it wins: nothing rejects these + // names when the skill is created. + await page.route("**/api/skills", (route) => + route.fulfill({ + status: 200, + contentType: "application/json", + body: JSON.stringify({ + skills: [ + { + name: "data-analysis", + description: "Analyze structured data and produce charts.", + category: "public", + enabled: true, + }, + { + name: "compact", + description: "A custom skill named after a builtin command.", + category: "custom", + enabled: true, + }, + { + name: "status", + description: "A custom skill named after a reserved command.", + category: "custom", + enabled: true, + }, + ], + }), + }), + ); + + await page.goto("/workspace/chats/new"); + + const textarea = page.getByPlaceholder(/how can i assist you/i); + await expect(textarea).toBeVisible({ timeout: 15_000 }); + + await textarea.fill("/comp"); + // Reserved outside chip mode: the builtin is offered, the skill is not. + await expect( + page.getByRole("option", { name: /compact/i }), + ).toHaveAccessibleName(/Compact earlier context/i); + + // A contract-reserved name has no builtin standing in for it, so the list + // is empty rather than showing a skill both slash parsers would refuse. + await textarea.fill("/stat"); + await expect(page.getByRole("option", { name: /status/i })).toBeHidden(); + + await textarea.fill("/dat"); + await expect( + page.getByRole("option", { name: /data-analysis/i }), + ).toBeVisible(); + await textarea.press("Enter"); + await expect(page.getByText("/data-analysis")).toBeVisible(); + + const skillInput = page.getByRole("textbox", { + name: /how can i assist you/i, + }); + await skillInput.pressSequentially("/comp"); + + // Reserved in chip mode too. Selecting it would set a `/compact` chip that + // `parseCompactCommand` intercepts on submit, so context compaction would + // run instead of the skill. + await expect(page.getByRole("option", { name: /compact/i })).toBeHidden(); + + await skillInput.fill("/stat"); + await expect(page.getByRole("option", { name: /status/i })).toBeHidden(); + }); + test("goal command sets a goal and starts an agent run", async ({ page }) => { let streamCalls = 0; await page.goto("/workspace/chats/new"); diff --git a/frontend/tests/unit/components/workspace/input-box-helpers.test.ts b/frontend/tests/unit/components/workspace/input-box-helpers.test.ts index b23b72cce..7c65edf3c 100644 --- a/frontend/tests/unit/components/workspace/input-box-helpers.test.ts +++ b/frontend/tests/unit/components/workspace/input-box-helpers.test.ts @@ -21,7 +21,7 @@ import { readGoalResponseError, type SlashSuggestion, } from "@/components/workspace/input-box-helpers"; -import type { Skill } from "@/core/skills"; +import { RESERVED_SLASH_SKILL_NAMES, type Skill } from "@/core/skills"; function makeSkill(name: string, enabled = true): Skill { return { @@ -333,6 +333,29 @@ describe("getMatchingSkillSuggestions", () => { ]); }); + it("excludes skills that collide with a reserved slash name", () => { + // The picker must not offer what the slash parsers refuse. Both sides drop + // these names, so such a skill can never activate — picking it would send + // literal text to the model with nothing loaded. + for (const reserved of RESERVED_SLASH_SKILL_NAMES) { + const result = getMatchingSkillSuggestions( + [makeSkill(reserved), makeSkill(`${reserved}-helper`)], + reserved, + builtins, + ); + + expect( + result.filter((s) => s.kind === "skill").map((s) => s.name), + ).toEqual([`${reserved}-helper`]); + } + }); + + it("keeps reserved names out even when no builtin commands are passed", () => { + const result = getMatchingSkillSuggestions([makeSkill("status")], "", []); + + expect(result).toEqual([]); + }); + it("caps the number of suggestions", () => { const skills = Array.from({ length: 10 }, (_, i) => makeSkill(`skill-${i}`),