feat: add session-aware slash skill commands - #311

web/src/components/assistant-ui/session-composer.tsx (1)

158-159: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Replace the arbitrary type and spacing values with design tokens.

gap-[7px], pt-[11px], text-[10.5px], and text-[9px] are invented design values. The coding guidelines require type and spacing values to come from packages/ui/src/tokens.css and DESIGN.md.

If these values are unchanged code that this diff only re-indented under the new popover root, treat this as a follow-up rather than a blocker for this PR.

Also applies to: 191-193

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web/src/components/assistant-ui/session-composer.tsx` around lines 158 - 159,
Replace the arbitrary Tailwind spacing and typography values in the session
composer, including gap-[7px], pt-[11px], text-[10.5px], and text-[9px], with
the closest approved design tokens defined in tokens.css and DESIGN.md; preserve
the existing layout and styling behavior.

Source: Coding guidelines

web/src/components/assistant-ui/session-composer-command-menu.tsx (1)

224-226: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Reuse COMMAND_TRIGGER_CHARACTER for the char prop.

Line 40 defines COMMAND_TRIGGER_CHARACTER = "/" and Line 146 uses it for offset math. Line 225 hardcodes "/" again. If the trigger character changes, the two values can diverge.

♻️ Proposed change

-      char="/"
+      char={COMMAND_TRIGGER_CHARACTER}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web/src/components/assistant-ui/session-composer-command-menu.tsx` around
lines 224 - 226, Update the char prop on
ComposerPrimitive.Unstable_TriggerPopover to use the existing
COMMAND_TRIGGER_CHARACTER constant instead of a hardcoded slash, keeping the
trigger configuration and offset calculations consistent.

internal/transcript/ui_input_messages.go (1)

36-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Log the marshal failure instead of dropping metadata silently.

If json.Marshal fails, the function returns nil and the message loses its turn ID, message ID, goal, and skill invocations. No signal reaches an operator. The struct holds only strings, ints, and *acp.GoalPromptMeta, so failure is unlikely, but the guidelines require handling every error or documenting a justification.

Add a slog warning or a short comment that states why the error is safe to ignore.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/transcript/ui_input_messages.go` around lines 36 - 46, Update the
JSON marshaling error branch in the surrounding metadata helper to log a concise
slog warning before returning nil, preserving the existing return behavior while
exposing marshal failures to operators.

Source: Coding guidelines

web/src/components/assistant-ui/__tests__/session-thread.test.tsx (1)

1516-1522: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Assert menu closure by test ID instead of by label text.

screen.queryByText("Frontend QA") matches any node with that exact text. The assertion passes for the wrong reason if the label moves outside the menu, and it fails if the inserted token ever renders the label elsewhere. Query the menu container instead.

♻️ Proposed change

-    expect(screen.queryByText("Frontend QA")).not.toBeInTheDocument();
+    await waitFor(() => {
+      expect(screen.queryByTestId("composer-command-menu")).not.toBeInTheDocument();
+    });

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web/src/components/assistant-ui/__tests__/session-thread.test.tsx` around
lines 1516 - 1522, Update the menu-closure assertion in the session thread test
near the existing textarea and draft expectations to query the menu container by
its test ID, asserting that this container is absent from the document instead
of relying on the “Frontend QA” label text.

internal/command/slug.go (1)

16-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Simplify the always-true separator condition.

The allowed branch ends with continue. Execution reaches line 21 only when isAllowed is false. The condition unicode.IsSpace(character) || character > unicode.MaxASCII || !isAllowed is therefore always true, and the unicode import exists only for it. The current form suggests three distinct cases that do not exist.

♻️ Proposed simplification

-		if unicode.IsSpace(character) || character > unicode.MaxASCII || !isAllowed {
-			if builder.Len() > 0 && !wroteSeparator {
-				builder.WriteByte('-')
-				wroteSeparator = true
-			}
-		}
+		if builder.Len() > 0 && !wroteSeparator {
+			builder.WriteByte('-')
+			wroteSeparator = true
+		}

Remove the now-unused unicode import.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/command/slug.go` around lines 16 - 27, In the slug-building loop,
simplify the condition guarding separator insertion after the isAllowed branch
continues, since this path only handles disallowed characters. Remove the
redundant condition and delete the now-unused unicode import, while preserving
the existing separator behavior.

internal/command/command_test.go (1)

16-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a case for an unqualified skill that collides with a claimed bare name.

BuildCatalog drops an unqualified skill when its bare name is already claimed by a builtin or an agent (internal/command/catalog.go lines 66-72). That precedence branch has no coverage. The current cases only prove builtin-over-agent precedence and qualified-skill duplicates.

A regression that removed the continue would produce a second /goal or /review descriptor and no existing case would fail.

💚 Proposed added case

+	t.Run("Should drop an unqualified skill that collides with a claimed bare name", func(t *testing.T) {
+		t.Parallel()
+		catalog, err := commandpkg.BuildCatalog(
+			commandpkg.DefaultBuiltins(),
+			[]commandpkg.AgentSpec{{Name: "review", SourceID: "codex"}},
+			[]commandpkg.SkillSpec{{
+				Name: "review", Available: true,
+				Source: commandpkg.Source{Kind: "workspace", Scope: "workspace"},
+			}},
+		)
+		if err != nil {
+			t.Fatalf("BuildCatalog() error = %v", err)
+		}
+		got := make([]string, 0, len(catalog.Commands))
+		for _, descriptor := range catalog.Commands {
+			got = append(got, descriptor.CanonicalToken)
+		}
+		want := []string{"/goal", "/review"}
+		if strings.Join(got, ",") != strings.Join(want, ",") {
+			t.Fatalf("BuildCatalog() tokens = %#v, want %#v", got, want)
+		}
+		if catalog.Commands[1].Lane != commandpkg.LaneAgent {
+			t.Fatalf("/review lane = %q, want the agent to win the bare name", catalog.Commands[1].Lane)
+		}
+	})

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/command/command_test.go` around lines 16 - 90, Add a BuildCatalog
test case covering an unqualified available skill whose bare name matches an
existing builtin or agent, such as “goal” or “review”; assert the catalog
succeeds with only the claimed command descriptor and does not include a
duplicate skill token. Place the coverage alongside the existing ordering and
collision cases, preserving builtin/agent precedence.

Source: Path instructions

internal/session/manager_prompt_contract_test.go (1)

1112-1191: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

New test functions assert inline instead of using Should … subtests. Both new tests place all assertions directly in the test function body. The repository test conventions require t.Run("Should …") for every test case. The practical cost is that the first t.Fatalf aborts the whole function, so later independent assertions never run and never report.

• internal/session/manager_prompt_contract_test.go#L1112-L1191: split the four scenarios (operator expansion, authored-text preservation, untrusted-caller rejection, internal-prompt literal handling) into separate t.Run("Should …") blocks, so a failure in the first scenario does not hide the untrusted-ingress checks at lines 1148-1158. Also replace the err == nil check at line 1155 with errors.Is against the sentinel that validateCommandIngress returns.
• internal/api/core/session_stream_internal_test.go#L617-L667: wrap the body in t.Run("Should emit a revision-only wake frame when the catalog changes", …), and add a second subtest asserting that an unchanged catalog stays silent and returns the same revision.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/session/manager_prompt_contract_test.go` around lines 1112 - 1191,
The test in internal/session/manager_prompt_contract_test.go lines 1112-1191
needs four separate t.Run("Should …") subtests for operator
expansion/authored-text preservation, untrusted-caller rejection, and
internal-prompt literal handling; use errors.Is with the validateCommandIngress
sentinel for rejection checks. In
internal/api/core/session_stream_internal_test.go lines 617-667, wrap the
existing revision-only wake-frame assertions in the named subtest and add a
second subtest verifying unchanged catalogs emit no frame and retain the same
revision.

Sources: Coding guidelines, Path instructions

internal/cli/client_test.go (1)

2408-2412: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use a named subtest for the command-catalog assertion.

Place this check in t.Run("Should return the session command catalog", ...). This gives the command-catalog invariant a stable failure name.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/client_test.go` around lines 2408 - 2412, Wrap the
ListSessionCommands assertion in a named subtest using t.Run("Should return the
session command catalog", ...), keeping the existing command-catalog validation
and failure behavior unchanged.

Source: Path instructions

internal/cli/client_session_commands.go (1)

15-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Wrap CLI errors with operation context.

Both paths return raw errors. Add operation-specific context with %w so callers can identify the failed CLI step while preserving errors.Is and errors.As.

• internal/cli/client_session_commands.go#L15-L21: wrap the sessionScopedPath and doJSON errors.
• internal/cli/session_commands.go#L22-L32: wrap the clientFromDeps and ListSessionCommands errors.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/client_session_commands.go` around lines 15 - 21, Wrap errors
from sessionScopedPath and doJSON in
internal/cli/client_session_commands.go#L15-L21 with operation-specific context
using %w, preserving errors.Is and errors.As. Also wrap errors from
clientFromDeps and ListSessionCommands in
internal/cli/session_commands.go#L22-L32 with context identifying each failed
CLI operation.

Source: Coding guidelines

internal/daemon/native_tool_registry_skills.go (1)

142-149: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Build the skill payload once.

Line 143 calls core.SkillPayloadFromSkill(skill), and Line 148 replaces that value when a command ID is present. Select the payload before building the map.

♻️ Proposed refactor

+	commandID := strings.TrimSpace(input.CommandID)
+	skillPayload := any(core.SkillPayloadFromSkill(skill))
+	if commandID != "" {
+		skillPayload = sourceQualifiedSkillViewPayload(skill)
+	}
 	payload := map[string]any{
-		"skill":   core.SkillPayloadFromSkill(skill),
+		"skill":   skillPayload,
 		"content": content,
 	}
-	if commandID := strings.TrimSpace(input.CommandID); commandID != "" {
+	if commandID != "" {
 		payload["command_id"] = commandID
-		payload["skill"] = sourceQualifiedSkillViewPayload(skill)
 	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/daemon/native_tool_registry_skills.go` around lines 142 - 149, In
the payload construction flow, select the skill payload once before creating the
map: use core.SkillPayloadFromSkill(skill) by default and
sourceQualifiedSkillViewPayload(skill) when CommandID is non-empty. Then build
the map with the selected payload and only add command_id conditionally,
avoiding the current assignment and replacement in native tool registration.

internal/api/core/session_stream_transcript.go (1)

136-143: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consolidate the command-refresh cadence into one path and one constant.

Three call sites now hardcode time.Second. Both streaming loops own a commandRefresh ticker, and refreshTranscriptStream repeats the same elapsed-time gate. Because the ticker updates state.commandCheckedAt, the gate inside refreshTranscriptStream almost never fires, so the logic is duplicated without effect.

Define a named interval constant next to sessionStreamKeepAliveInterval, and keep the refresh in a single place.

Also applies to: 231-232, 283-284

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/api/core/session_stream_transcript.go` around lines 136 - 143,
Consolidate command-refresh timing around a single named interval constant
defined beside sessionStreamKeepAliveInterval. Replace the hardcoded time.Second
values at the streaming-loop ticker sites and in refreshTranscriptStream, then
remove the redundant elapsed-time gate from refreshTranscriptStream so command
refresh is performed only by the existing commandRefresh ticker path.

internal/session/command_parser.go (1)

21-28: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Consider skipping catalog resolution when the message contains no command token.

preparePromptSkillInvocations resolves the full command catalog for every non-goal prompt. If catalog resolution fails, the prompt fails, even when the message contains no slash token at all. Catalog resolution depends on workspace resolution and the skills registry, so an unrelated registry error blocks ordinary prompts.

A cheap pre-check for a slash token at a whitespace boundary would keep plain prompts working when the catalog is unavailable.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/session/command_parser.go` around lines 21 - 28, Update
preparePromptSkillInvocations to detect a slash command token at a whitespace
boundary before calling CommandCatalog; return no invocations immediately when
the authored message contains no such token. Preserve the existing catalog
resolution and ParseSkillInvocations flow for messages that may contain command
tokens.

internal/session/manager_prompt_submit.go (1)

218-233: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a test for the composed expansion and augmentation order.

promptDispatchMessage now expands skill invocations before augmentation, so the augmenter receives the expanded prompt instead of the authored prompt. The final dispatched text becomes the augmenter block, then the invoked-skills block, then the authored text.

The existing tests cover each stage alone. internal/daemon/prompt_skills_test.go asserts the augmenter preserves the trailing prompt, and the new command service test asserts Expand preserves the trailing authored text. Neither test covers the two stages together.

Add a case to the canonical prompt dispatch suite that runs both stages and asserts the final ordering and the preserved authored suffix.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/session/manager_prompt_submit.go` around lines 218 - 233, Add an
integration case to the canonical prompt dispatch test suite that configures
both skill expansion and input augmentation, then assert the augmenter receives
expanded content and the final dispatched text orders the augmenter block before
the invoked-skills block and authored prompt suffix. Preserve the existing
standalone stage tests and verify the authored trailing text remains intact.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes