From cfa875d9d6c8980182a68c581fd0a9ca4dd3aee5 Mon Sep 17 00:00:00 2001 From: nova28 Date: Fri, 31 Jul 2026 02:03:18 +0800 Subject: [PATCH] feat(agents): accept command_args when creating custom TUI agents (#2053) --- .../agent/settings/controller/custom_tui.go | 4 +- .../settings/controller/custom_tui_test.go | 101 ++++++++++++++++++ .../settings/handlers/custom_tui_handlers.go | 21 ++-- .../handlers/custom_tui_handlers_test.go | 45 ++++++++ .../orchestrator/task_operations_test.go | 4 +- .../e2e/tests/chat/auto-scroll-toggle.spec.ts | 35 +++--- .../chat/mobile-auto-scroll-toggle.spec.ts | 56 ++++++++++ .../transcript-auto-scroll-stability/plan.md | 75 +++++++++++++ .../task-01-stabilize-recovery-test.md | 41 +++++++ .../task-02-freeze-native-transcript.md | 47 ++++++++ docs/specs/INDEX.md | 1 + docs/specs/ui/transcript-auto-scroll.md | 43 ++++++++ 12 files changed, 452 insertions(+), 21 deletions(-) create mode 100644 apps/backend/internal/agent/settings/controller/custom_tui_test.go create mode 100644 apps/backend/internal/agent/settings/handlers/custom_tui_handlers_test.go create mode 100644 docs/plans/transcript-auto-scroll-stability/plan.md create mode 100644 docs/plans/transcript-auto-scroll-stability/task-01-stabilize-recovery-test.md create mode 100644 docs/plans/transcript-auto-scroll-stability/task-02-freeze-native-transcript.md create mode 100644 docs/specs/ui/transcript-auto-scroll.md diff --git a/apps/backend/internal/agent/settings/controller/custom_tui.go b/apps/backend/internal/agent/settings/controller/custom_tui.go index b0a80e0022..cf6f19961b 100644 --- a/apps/backend/internal/agent/settings/controller/custom_tui.go +++ b/apps/backend/internal/agent/settings/controller/custom_tui.go @@ -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{ diff --git a/apps/backend/internal/agent/settings/controller/custom_tui_test.go b/apps/backend/internal/agent/settings/controller/custom_tui_test.go new file mode 100644 index 0000000000..b6d5a68357 --- /dev/null +++ b/apps/backend/internal/agent/settings/controller/custom_tui_test.go @@ -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) + } +} diff --git a/apps/backend/internal/agent/settings/handlers/custom_tui_handlers.go b/apps/backend/internal/agent/settings/handlers/custom_tui_handlers.go index e7f0f451ab..9cb977ecbe 100644 --- a/apps/backend/internal/agent/settings/handlers/custom_tui_handlers.go +++ b/apps/backend/internal/agent/settings/handlers/custom_tui_handlers.go @@ -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: diff --git a/apps/backend/internal/agent/settings/handlers/custom_tui_handlers_test.go b/apps/backend/internal/agent/settings/handlers/custom_tui_handlers_test.go new file mode 100644 index 0000000000..22d22830af --- /dev/null +++ b/apps/backend/internal/agent/settings/handlers/custom_tui_handlers_test.go @@ -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) + } +} diff --git a/apps/backend/internal/orchestrator/task_operations_test.go b/apps/backend/internal/orchestrator/task_operations_test.go index 38bf9d5357..c81199e79d 100644 --- a/apps/backend/internal/orchestrator/task_operations_test.go +++ b/apps/backend/internal/orchestrator/task_operations_test.go @@ -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) diff --git a/apps/web/e2e/tests/chat/auto-scroll-toggle.spec.ts b/apps/web/e2e/tests/chat/auto-scroll-toggle.spec.ts index d0cb6572d5..17c5b31f13 100644 --- a/apps/web/e2e/tests/chat/auto-scroll-toggle.spec.ts +++ b/apps/web/e2e/tests/chat/auto-scroll-toggle.spec.ts @@ -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 ({ diff --git a/apps/web/e2e/tests/chat/mobile-auto-scroll-toggle.spec.ts b/apps/web/e2e/tests/chat/mobile-auto-scroll-toggle.spec.ts index 91f348195e..180a2991fb 100644 --- a/apps/web/e2e/tests/chat/mobile-auto-scroll-toggle.spec.ts +++ b/apps/web/e2e/tests/chat/mobile-auto-scroll-toggle.spec.ts @@ -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); + }); }); diff --git a/docs/plans/transcript-auto-scroll-stability/plan.md b/docs/plans/transcript-auto-scroll-stability/plan.md new file mode 100644 index 0000000000..e1e9faceee --- /dev/null +++ b/docs/plans/transcript-auto-scroll-stability/plan.md @@ -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. diff --git a/docs/plans/transcript-auto-scroll-stability/task-01-stabilize-recovery-test.md b/docs/plans/transcript-auto-scroll-stability/task-01-stabilize-recovery-test.md new file mode 100644 index 0000000000..cf17160f39 --- /dev/null +++ b/docs/plans/transcript-auto-scroll-stability/task-01-stabilize-recovery-test.md @@ -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. diff --git a/docs/plans/transcript-auto-scroll-stability/task-02-freeze-native-transcript.md b/docs/plans/transcript-auto-scroll-stability/task-02-freeze-native-transcript.md new file mode 100644 index 0000000000..6c5f7d7abf --- /dev/null +++ b/docs/plans/transcript-auto-scroll-stability/task-02-freeze-native-transcript.md @@ -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. diff --git a/docs/specs/INDEX.md b/docs/specs/INDEX.md index c3bef98ed6..372943c4c7 100644 --- a/docs/specs/INDEX.md +++ b/docs/specs/INDEX.md @@ -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 | diff --git a/docs/specs/ui/transcript-auto-scroll.md b/docs/specs/ui/transcript-auto-scroll.md new file mode 100644 index 0000000000..b356500dd4 --- /dev/null +++ b/docs/specs/ui/transcript-auto-scroll.md @@ -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.