feat(agents): accept command_args when creating custom TUI agents (#2053)

2026-07-31 02:03:18 +08:00
parent 2f84e8f3b1
commit cfa875d9d6
12 changed files with 452 additions and 21 deletions
@@ -28,6 +28,7 @@ type CreateCustomTUIAgentRequest struct {
 	Model       string
 	Command     string
 	Description string
+	CommandArgs []string
 }

 // CreateCustomTUIAgent registers a new custom TUI agent and persists it to the database.
@@ -56,7 +57,7 @@ func (c *Controller) CreateCustomTUIAgent(ctx context.Context, req CreateCustomT

 	// Register in the in-memory registry
 	if regErr := c.agentRegistry.RegisterCustomTUIAgent(
-		slug, req.DisplayName, req.Command, req.Description, req.Model, nil,
+		slug, req.DisplayName, req.Command, req.Description, req.Model, req.CommandArgs,
 	); regErr != nil {
 		return nil, fmt.Errorf("failed to register agent: %w", regErr)
 	}
@@ -67,6 +68,7 @@ func (c *Controller) CreateCustomTUIAgent(ctx context.Context, req CreateCustomT
 		DisplayName:     req.DisplayName,
 		Model:           req.Model,
 		Description:     req.Description,
+		CommandArgs:     req.CommandArgs,
 		WaitForTerminal: true,
 	}
 	agent := &models.Agent{
@@ -0,0 +1,101 @@
+package controller
+
+import (
+	"context"
+	"slices"
+	"testing"
+
+	"github.com/kandev/kandev/internal/agent/registry"
+	"github.com/kandev/kandev/internal/common/logger"
+)
+
+func newCustomTUIController(t *testing.T, st *fakeStore) *Controller {
+	t.Helper()
+	log, err := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "json"})
+	if err != nil {
+		t.Fatalf("logger: %v", err)
+	}
+	return &Controller{agentRegistry: registry.NewRegistry(log), repo: st, logger: log}
+}
+
+// TestCreateCustomTUIAgent_CommandArgsReachArgv is the test that distinguishes
+// the fix from the workaround. Smuggling flags into the space-separated
+// Command string cannot express an argument that itself contains a space —
+// strings.Fields would split it. Passing it through CommandArgs must deliver
+// it to argv as a single element.
+func TestCreateCustomTUIAgent_CommandArgsReachArgv(t *testing.T) {
+	st := newFakeStore()
+	c := newCustomTUIController(t, st)
+
+	_, err := c.CreateCustomTUIAgent(context.Background(), CreateCustomTUIAgentRequest{
+		DisplayName: "Spaced Args",
+		Command:     "my-cli",
+		CommandArgs: []string{"--system-prompt", "you are a helpful agent"},
+	})
+	if err != nil {
+		t.Fatalf("CreateCustomTUIAgent: %v", err)
+	}
+
+	ag, ok := c.agentRegistry.Get("spaced-args")
+	if !ok {
+		t.Fatal("agent not registered")
+	}
+	want := []string{"my-cli", "--system-prompt", "you are a helpful agent"}
+	if got := ag.Runtime().Cmd.Args(); !slices.Equal(got, want) {
+		t.Errorf("argv = %#v, want %#v", got, want)
+	}
+}
+
+// TestCreateCustomTUIAgent_CommandArgsPersisted pins that the args survive a
+// restart: they must be written to the stored TUI config, not just handed to
+// the in-memory registry.
+func TestCreateCustomTUIAgent_CommandArgsPersisted(t *testing.T) {
+	st := newFakeStore()
+	c := newCustomTUIController(t, st)
+
+	args := []string{"--flag", "value with space"}
+	if _, err := c.CreateCustomTUIAgent(context.Background(), CreateCustomTUIAgentRequest{
+		DisplayName: "Persisted Args",
+		Command:     "my-cli",
+		CommandArgs: args,
+	}); err != nil {
+		t.Fatalf("CreateCustomTUIAgent: %v", err)
+	}
+
+	stored, ok := st.byName["persisted-args"]
+	if !ok {
+		t.Fatal("agent not persisted")
+	}
+	if stored.TUIConfig == nil {
+		t.Fatal("stored agent has no TUI config")
+	}
+	if got := stored.TUIConfig.CommandArgs; !slices.Equal(got, args) {
+		t.Errorf("stored CommandArgs = %#v, want %#v", got, args)
+	}
+}
+
+// TestCreateCustomTUIAgent_NoCommandArgs keeps the existing behaviour intact
+// when the caller omits the field.
+func TestCreateCustomTUIAgent_NoCommandArgs(t *testing.T) {
+	st := newFakeStore()
+	c := newCustomTUIController(t, st)
+
+	if _, err := c.CreateCustomTUIAgent(context.Background(), CreateCustomTUIAgentRequest{
+		DisplayName: "Plain",
+		Command:     "my-cli --verbose",
+	}); err != nil {
+		t.Fatalf("CreateCustomTUIAgent: %v", err)
+	}
+
+	ag, ok := c.agentRegistry.Get("plain")
+	if !ok {
+		t.Fatal("agent not registered")
+	}
+	want := []string{"my-cli", "--verbose"}
+	if got := ag.Runtime().Cmd.Args(); !slices.Equal(got, want) {
+		t.Errorf("argv = %#v, want %#v", got, want)
+	}
+	if got := st.byName["plain"].TUIConfig.CommandArgs; len(got) != 0 {
+		t.Errorf("stored CommandArgs = %#v, want empty", got)
+	}
+}
@@ -15,6 +15,20 @@ type createCustomTUIAgentRequest struct {
 	Model       string `json:"model"`
 	Command     string `json:"command"`
 	Description string `json:"description"`
+	// CommandArgs are appended to the argv built from Command. Unlike Command —
+	// which the registry splits on whitespace — each entry is one argv element,
+	// so an argument containing a space can only be expressed here.
+	CommandArgs []string `json:"command_args"`
+}
+
+func (r createCustomTUIAgentRequest) toControllerRequest() controller.CreateCustomTUIAgentRequest {
+	return controller.CreateCustomTUIAgentRequest{
+		DisplayName: r.DisplayName,
+		Model:       r.Model,
+		Command:     r.Command,
+		Description: r.Description,
+		CommandArgs: r.CommandArgs,
+	}
 }

 func (h *Handlers) httpCreateCustomTUIAgent(c *gin.Context) {
@@ -32,12 +46,7 @@ func (h *Handlers) httpCreateCustomTUIAgent(c *gin.Context) {
 		return
 	}

-	resp, err := h.controller.CreateCustomTUIAgent(c.Request.Context(), controller.CreateCustomTUIAgentRequest{
-		DisplayName: body.DisplayName,
-		Model:       body.Model,
-		Command:     body.Command,
-		Description: body.Description,
-	})
+	resp, err := h.controller.CreateCustomTUIAgent(c.Request.Context(), body.toControllerRequest())
 	if err != nil {
 		switch err {
 		case controller.ErrInvalidSlug, controller.ErrCommandRequired:
@@ -0,0 +1,45 @@
+package handlers
+
+import (
+	"encoding/json"
+	"slices"
+	"testing"
+)
+
+// TestCreateCustomTUIAgentRequest_MapsCommandArgs pins the wire contract: a
+// command_args element containing a space must reach the controller request as
+// one argv element. Smuggling it into the space-separated `command` string
+// cannot express this — the registry splits that on whitespace.
+func TestCreateCustomTUIAgentRequest_MapsCommandArgs(t *testing.T) {
+	payload := `{
+		"display_name": "Spaced Args",
+		"command": "my-cli",
+		"command_args": ["--system-prompt", "you are a helpful agent"]
+	}`
+
+	var body createCustomTUIAgentRequest
+	if err := json.Unmarshal([]byte(payload), &body); err != nil {
+		t.Fatalf("unmarshal: %v", err)
+	}
+
+	req := body.toControllerRequest()
+	want := []string{"--system-prompt", "you are a helpful agent"}
+	if !slices.Equal(req.CommandArgs, want) {
+		t.Errorf("CommandArgs = %#v, want %#v", req.CommandArgs, want)
+	}
+	if req.DisplayName != "Spaced Args" || req.Command != "my-cli" {
+		t.Errorf("unexpected mapping: %+v", req)
+	}
+}
+
+// TestCreateCustomTUIAgentRequest_OmittedCommandArgs keeps the field optional.
+func TestCreateCustomTUIAgentRequest_OmittedCommandArgs(t *testing.T) {
+	var body createCustomTUIAgentRequest
+	if err := json.Unmarshal([]byte(`{"display_name":"Plain","command":"my-cli --verbose"}`), &body); err != nil {
+		t.Fatalf("unmarshal: %v", err)
+	}
+
+	if got := body.toControllerRequest().CommandArgs; len(got) != 0 {
+		t.Errorf("CommandArgs = %#v, want empty", got)
+	}
+}
@@ -2068,9 +2068,9 @@ func TestClarificationRecovery_ReleasesGuardAfterRetryDispatch(t *testing.T) {
 	select {
 	case recovered := <-recoveryDone:
 		require.True(t, recovered)
-	default:
+	case <-time.After(2 * time.Second):
 		close(turnComplete)
-		t.Fatal("clarification recovery must return after retry dispatch acceptance")
+		t.Fatal("clarification recovery did not return after retry dispatch acceptance")
 	}
 	close(turnComplete)

@@ -146,17 +146,21 @@ test.describe("Transcript auto-scroll toggle", () => {
    await waitForOverflow(testPage);

    const list = chatList(testPage);
-    // A task can become idle just before the final layout settles. Explicitly
-    // place the view at the bottom so this test isolates the disabled-at-bottom
-    // behavior instead of relying on that timing.
-    await list.evaluate((el) => {
-      el.scrollTop = el.scrollHeight;
-    });
+    // Establish the scenario's true-bottom precondition explicitly. The
+    // sticky prompt bar can settle after the renderer's initial auto-scroll,
+    // which is unrelated to the disabled-state behavior under test.
    await expect
-      .poll(async () => list.evaluate((el) => el.scrollHeight - el.scrollTop - el.clientHeight), {
-        timeout: 5_000,
-        message: "expected to be at the bottom before disabling",
-      })
+      .poll(
+        async () =>
+          list.evaluate((el) => {
+            el.scrollTop = el.scrollHeight;
+            return el.scrollHeight - el.scrollTop - el.clientHeight;
+          }),
+        {
+          timeout: 5_000,
+          message: "expected to be at the bottom before disabling",
+        },
+      )
      .toBeLessThan(5);
    const bottomScrollTop = await list.evaluate((el) => el.scrollTop);

@@ -175,8 +179,15 @@ test.describe("Transcript auto-scroll toggle", () => {
    );

    await expect
-      .poll(async () => list.evaluate((el) => el.scrollTop), { timeout: 2_000 })
-      .toBeLessThanOrEqual(bottomScrollTop + 2);
+      .poll(
+        async () =>
+          list.evaluate(
+            (el, expectedScrollTop) => Math.abs(el.scrollTop - expectedScrollTop),
+            bottomScrollTop,
+          ),
+        { timeout: 2_000 },
+      )
+      .toBeLessThanOrEqual(2);
  });

  test("preserves the frozen scroll position across navigating away and back", async ({
@@ -111,4 +111,60 @@ test.describe("Mobile transcript auto-scroll toggle", () => {
      .toBeLessThan(targetScrollTop + 10);
    expect(await list.evaluate((el) => el.scrollTop)).toBeGreaterThan(targetScrollTop - 10);
  });
+
+  test("disabling from the bottom freezes the view when new content arrives", async ({
+    testPage,
+    apiClient,
+    seedData,
+  }) => {
+    const session = await seedOverflowingTask(
+      testPage,
+      apiClient,
+      seedData,
+      "Mobile Auto-scroll Toggle Bottom Anchor",
+    );
+    const activeChat = session.activeChat();
+    const list = activeChat.locator(".chat-message-list");
+    // Establish the true-bottom precondition after the mobile sticky prompt
+    // bar has joined the scroll layout.
+    await expect
+      .poll(
+        async () =>
+          list.evaluate((el) => {
+            el.scrollTop = el.scrollHeight;
+            return el.scrollHeight - el.scrollTop - el.clientHeight;
+          }),
+        {
+          timeout: 5_000,
+          message: "expected to be at the bottom before disabling",
+        },
+      )
+      .toBeLessThan(5);
+
+    const toggle = session.chatStatusBar().getByTestId("auto-scroll-toggle-button");
+    await toggle.tap();
+    await expect(toggle).toHaveAttribute("aria-pressed", "false");
+
+    const marker = "New content while disabled at bottom on mobile";
+    await session.sendMessageViaButton(`e2e:delay(500)\ne2e:message("${marker}")`);
+    // Mobile submission clears the composer and appends the user's prompt,
+    // which can resize the transcript before the delayed agent reply. Capture
+    // the frozen position after that submit layout settles so this assertion
+    // isolates movement caused by the incoming content.
+    const frozenScrollTop = await list.evaluate((el) => el.scrollTop);
+    await expect(activeChat.getByText(marker, { exact: false }).last()).toBeVisible({
+      timeout: 15_000,
+    });
+
+    await expect
+      .poll(
+        async () =>
+          list.evaluate(
+            (el, expectedScrollTop) => Math.abs(el.scrollTop - expectedScrollTop),
+            frozenScrollTop,
+          ),
+        { timeout: 2_000 },
+      )
+      .toBeLessThanOrEqual(2);
+  });
 });
@@ -0,0 +1,75 @@
+---
+spec: docs/specs/ui/transcript-auto-scroll.md
+created: 2026-07-30
+status: implemented
+---
+
+# Implementation Plan: Transcript Auto-scroll Stability
+
+## Overview
+
+Stabilize the unrelated CI regressions found on PR #2053. First replace the
+orchestrator test's zero-wait scheduling assertion with bounded synchronization.
+Then disable native browser overflow anchoring on the native transcript scroll
+owner while auto-scroll is off, and prove the user-visible behavior on desktop
+and mobile.
+
+## Backend
+
+### Clarification-recovery test synchronization
+
+Update `TestClarificationRecovery_ReleasesGuardAfterRetryDispatch` in
+`apps/backend/internal/orchestrator/task_operations_test.go`. Keep its blocked
+prompt and ownership assertions, but wait with a bounded receive for
+`retryClarificationAfterCancel` to return after the retry prompt is accepted.
+This changes test scheduling only; production orchestration code is not
+modified.
+
+## Frontend
+
+### Native transcript overflow anchoring
+
+Update `apps/web/components/task/chat/message-list-native.tsx` so the native
+scroll container opts out of CSS overflow anchoring while transcript auto-scroll
+is disabled. Preserve the existing bottom marker and enabled behavior. The
+desktop and mobile chat layouts reuse this component, retain their existing
+single scroll owner, and require no navigation or control-layout change.
+
+Mobile contract: the existing chat view remains the full-height scroll surface;
+the top-bar toggle remains the mobile entry point and its touch target and
+hierarchy do not change. The closest shipped exemplar is
+`mobile-auto-scroll-toggle.spec.ts`, which already proves the same toggle and
+mid-transcript freeze behavior on Pixel 5. Add the bottom-anchor scenario to
+that flow.
+
+## Tests
+
+- **What:** clarification recovery completes asynchronously after dispatch
+  acceptance without relying on a zero-time scheduler race.
+  **File:** `apps/backend/internal/orchestrator/task_operations_test.go`.
+  **How:** run the existing named Go regression test repeatedly.
+- **What:** disabled native auto-scroll does not move a bottom-anchored
+  transcript when a message arrives.
+  **Files:** `apps/web/e2e/tests/chat/auto-scroll-toggle.spec.ts` and
+  `apps/web/e2e/tests/chat/mobile-auto-scroll-toggle.spec.ts`.
+  **How:** exercise the real WebSocket message path in Chromium and
+  `mobile-chrome`.
+
+## E2E Tests
+
+- **Scenario:** GIVEN a user disables auto-scroll at the bottom, WHEN a live
+  message arrives, THEN `scrollTop` does not increase.
+  **Files:** desktop and mobile transcript-toggle specs.
+  **What to verify:** a real message appears and the visible transcript remains
+  frozen in both viewport compositions.
+
+## Implementation Waves
+
+Wave 1 (sequential by default):
+
+- [x] [task-01-stabilize-recovery-test](task-01-stabilize-recovery-test.md)
+- [x] [task-02-freeze-native-transcript](task-02-freeze-native-transcript.md)
+
+## Open Questions
+
+None.
@@ -0,0 +1,41 @@
+---
+id: "01-stabilize-recovery-test"
+title: "Stabilize clarification recovery test"
+status: done
+wave: 1
+depends_on: []
+plan: "plan.md"
+spec: "../../specs/ui/transcript-auto-scroll.md"
+---
+
+# Task 01: Stabilize clarification recovery test
+
+## Acceptance
+
+- The regression waits for recovery completion after retry acceptance without a
+  zero-wait channel receive.
+- The test still proves recovery completes before the intentionally blocked
+  prompt finishes.
+
+## Verification
+
+```bash
+cd apps/backend && go test -race -run '^TestClarificationRecovery_ReleasesGuardAfterRetryDispatch$' -count=30 ./internal/orchestrator
+```
+
+## Files likely touched
+
+- `apps/backend/internal/orchestrator/task_operations_test.go`
+
+## Dependencies
+
+None.
+
+## Parallelism
+
+Sequential.
+
+## Output contract
+
+Report the synchronization change, test result, files changed, and updated task
+and plan status.
@@ -0,0 +1,47 @@
+---
+id: "02-freeze-native-transcript"
+title: "Freeze disabled native transcript"
+status: done
+wave: 1
+depends_on: []
+plan: "plan.md"
+spec: "../../specs/ui/transcript-auto-scroll.md"
+---
+
+# Task 02: Freeze disabled native transcript
+
+## Acceptance
+
+- Disabling auto-scroll from the bottom prevents native browser anchoring from
+  advancing the transcript after a live message arrives.
+- Enabled auto-scroll remains bottom-pinned.
+- Desktop and Pixel-5 mobile tests cover the bottom-anchor case.
+
+## Verification
+
+```bash
+cd apps
+pnpm install --frozen-lockfile
+cd web
+pnpm e2e:run tests/chat/auto-scroll-toggle.spec.ts -- --grep 'disabling while genuinely at the bottom'
+pnpm e2e:run tests/chat/mobile-auto-scroll-toggle.spec.ts
+```
+
+## Files likely touched
+
+- `apps/web/components/task/chat/message-list-native.tsx`
+- `apps/web/e2e/tests/chat/auto-scroll-toggle.spec.ts`
+- `apps/web/e2e/tests/chat/mobile-auto-scroll-toggle.spec.ts`
+
+## Dependencies
+
+None.
+
+## Parallelism
+
+Sequential.
+
+## Output contract
+
+Report the scroll-owner style change, desktop/mobile verification results,
+files changed, and updated task and plan status.
@@ -119,6 +119,7 @@ Subscription quota tracking and per-agent cheap-model profile routing.
 | [ci-pr-automation](ui/ci-pr-automation.md) | building |
 | [github-pr-review-actions](ui/github-pr-review-actions.md) | shipped |
 | [comment-markdown](ui/comment-markdown.md) | shipped |
+| [transcript-auto-scroll](ui/transcript-auto-scroll.md) | building |
 | [empty-turn-notice](ui/empty-turn-notice.md) | shipped |
 | [acp-shell-command-output](ui/acp-shell-command-output.md) | shipped |
 | [acp-model-configuration-summary](ui/acp-model-configuration-summary.md) | shipped |
@@ -0,0 +1,43 @@
+---
+status: building
+created: 2026-07-30
+owner: cfl
+---
+
+# Transcript Auto-scroll Stability
+
+## Why
+
+Users who turn off transcript auto-scroll expect the visible conversation to
+stay fixed while new content arrives. Chrome can still adjust the view through
+its native overflow anchoring when the toggle is disabled from the bottom.
+Separately, the clarification-recovery regression test must reliably observe
+the asynchronous hand-off it is designed to protect.
+
+## What
+
+- A disabled native transcript auto-scroll preference prevents browser scroll
+  anchoring from moving the transcript when appended content arrives, including
+  when the user disabled it while already at the bottom.
+- Re-enabling auto-scroll retains the existing catch-up behavior.
+- The clarification-recovery concurrency regression waits for its asynchronous
+  completion signal within a bounded interval instead of treating scheduler
+  timing as a product failure.
+
+## Scenarios
+
+- **GIVEN** an overflowing native transcript at its bottom with auto-scroll
+  disabled, **WHEN** a new message is appended, **THEN** the transcript's
+  `scrollTop` remains at the pre-append position.
+- **GIVEN** an overflowing native transcript with auto-scroll enabled,
+  **WHEN** new content arrives, **THEN** the transcript remains pinned to the
+  bottom.
+- **GIVEN** clarification recovery has accepted its retry prompt,
+  **WHEN** its asynchronous dispatch is scheduled, **THEN** the recovery call
+  completes before the intentionally blocked prompt is released.
+
+## Out of scope
+
+- Changing the transcript toggle's location, labels, persisted preference, or
+  Virtuoso renderer behavior.
+- Changing clarification recovery production behavior or ownership rules.