diff --git a/apps/backend/cmd/mock-agent/background_test.go b/apps/backend/cmd/mock-agent/background_test.go index 5a37cbce68..e4b477804d 100644 --- a/apps/backend/cmd/mock-agent/background_test.go +++ b/apps/backend/cmd/mock-agent/background_test.go @@ -7,10 +7,10 @@ import ( acp "github.com/coder/acp-go-sdk" ) -// TestParseBackgroundDuration pins the /background argument parsing, including -// the regression Copilot flagged on PR #3: a unit-bearing value like "1m" must -// not be mangled into "1ms" by the bare-seconds fallback. -func TestParseBackgroundDuration(t *testing.T) { +// TestParseCommandDuration pins command argument parsing, including the +// regression Copilot flagged on PR #3: a unit-bearing value like "1m" must not +// be mangled into "1ms" by the bare-seconds fallback. +func TestParseCommandDuration(t *testing.T) { const def = 8 * time.Second cases := []struct { name string @@ -19,17 +19,19 @@ func TestParseBackgroundDuration(t *testing.T) { }{ {"no argument uses default", "/background", def}, {"bare number is seconds", "/background 12", 12 * time.Second}, + {"slow bare number is seconds", "/slow 60", 60 * time.Second}, {"explicit seconds", "/background 20s", 20 * time.Second}, {"explicit minutes (regression: not 1ms)", "/background 1m", time.Minute}, {"explicit hours", "/background 2h", 2 * time.Hour}, {"explicit milliseconds", "/background 500ms", 500 * time.Millisecond}, {"unparseable falls back to default", "/background soon", def}, {"zero falls back to default", "/background 0", def}, + {"negative falls back to default", "/background -1s", def}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - if got := parseBackgroundDuration(tc.cmd, def); got != tc.want { - t.Fatalf("parseBackgroundDuration(%q) = %v, want %v", tc.cmd, got, tc.want) + if got := parseCommandDuration(tc.cmd, def); got != tc.want { + t.Fatalf("parseCommandDuration(%q) = %v, want %v", tc.cmd, got, tc.want) } }) } diff --git a/apps/backend/cmd/mock-agent/handler.go b/apps/backend/cmd/mock-agent/handler.go index d15c93f31c..58d0ffbd70 100644 --- a/apps/backend/cmd/mock-agent/handler.go +++ b/apps/backend/cmd/mock-agent/handler.go @@ -322,7 +322,7 @@ const asyncSubagentAgentID = "agent_e2e_async_lifecycle" // The teardown variant intentionally omits step 5 so execution termination is // the only evidence available to reconcile the registration. func emitAsyncSubagentLifecycle(e *emitter, cmd string, emitCompletion bool) { - d := parseBackgroundDuration(cmd, 20*time.Second) + d := parseCommandDuration(cmd, 20*time.Second) toolCallID := nextToolID() e.launchAsyncSubagentTool( toolCallID, @@ -350,7 +350,7 @@ func emitAsyncSubagentLifecycle(e *emitter, cmd string, emitCompletion bool) { // It is intentionally different from /background, whose foreground prompt stays // open for the entire delay, so E2E can cover settled+background explicitly. func emitDetachedBackgroundWork(e *emitter, cmd string) { - d := parseBackgroundDuration(cmd, 8*time.Second) + d := parseCommandDuration(cmd, 8*time.Second) e.text("Launching detached background work; this foreground turn is complete.") taskToolID := nextToolID() @@ -390,7 +390,7 @@ func emitSleep(e *emitter, cmd string) { // while the session still reads RUNNING. When the hold elapses the subagent // completes, the foreground resumes, and the turn ends (→ done). func emitBackgroundWork(e *emitter, cmd string) { - d := parseBackgroundDuration(cmd, 8*time.Second) + d := parseCommandDuration(cmd, 8*time.Second) e.text("Kicking off background work; I'll keep going in the background.") @@ -416,13 +416,11 @@ func emitBackgroundWork(e *emitter, cmd string) { e.text("Background work complete.") } -// parseBackgroundDuration reads the optional duration argument of a /background -// command, returning def when it is absent or unparseable. A value carrying an -// explicit unit is honored as-is (`1m`, `500ms`, `2h`); a bare number is treated -// as seconds (`8` → 8s). The explicit-unit parse is tried FIRST: appending "s" -// to a unit-bearing value like `1m` would otherwise parse as the valid-but-wrong -// "1ms" (1 millisecond) and never reach the correct interpretation. -func parseBackgroundDuration(cmd string, def time.Duration) time.Duration { +// parseCommandDuration reads an optional command duration, returning def when +// it is absent or unparseable. Explicit units are honored as-is; bare numbers +// are treated as seconds. The explicit-unit parse is tried first so a value +// like `1m` is not misread as the valid-but-wrong `1ms`. +func parseCommandDuration(cmd string, def time.Duration) time.Duration { parts := strings.Fields(cmd) if len(parts) < 2 { return def @@ -456,13 +454,7 @@ func emitCrash(e *emitter, model string) { // emitSlowResponse generates a response with configurable total duration. func emitSlowResponse(e *emitter, prompt, model string) { - totalDuration := 5 * time.Second - parts := strings.Fields(prompt) - if len(parts) >= 2 { - if d, err := time.ParseDuration(parts[1]); err == nil && d > 0 { - totalDuration = d - } - } + totalDuration := parseCommandDuration(prompt, 5*time.Second) steps := 5 stepDelay := totalDuration / time.Duration(steps) diff --git a/apps/backend/cmd/mock-agent/slow_test.go b/apps/backend/cmd/mock-agent/slow_test.go new file mode 100644 index 0000000000..53d7fda06c --- /dev/null +++ b/apps/backend/cmd/mock-agent/slow_test.go @@ -0,0 +1,16 @@ +package main + +import "testing" + +func TestSlowResponseBareNumberUsesSeconds(t *testing.T) { + e, updates := newTestEmitter() + emitSlowResponse(e, "/slow 50ms", "mock-fast") + + const want = "Running slow response (50ms total)..." + for _, update := range updates.getUpdates() { + if getTextContent(update) == want { + return + } + } + t.Fatalf("slow response did not report %q", want) +} diff --git a/docs/plans/mock-agent-slow-duration/plan.md b/docs/plans/mock-agent-slow-duration/plan.md new file mode 100644 index 0000000000..052b7dbc33 --- /dev/null +++ b/docs/plans/mock-agent-slow-duration/plan.md @@ -0,0 +1,75 @@ +--- +spec: docs/specs/mock-agent-slow-duration/spec.md +created: 2026-08-08 +status: done +--- + +# Implementation Plan: Mock-agent slow command duration syntax + +## Overview + +Extend the mock agent's existing duration parsing so `/slow 60` resolves to +60 seconds while preserving explicit-unit durations and the five-second +fallback. Reuse the already-tested explicit-unit-first parsing behavior used by +the background commands, then add focused parser and handler coverage without +changing the ACP command advertisement or any frontend code. + +## Backend + +### Shared duration parsing + +- `apps/backend/cmd/mock-agent/handler.go` + - Generalize the existing background-duration parser, or extract its argument + parsing into a clearly command-independent helper. + - Try an explicit Go duration first (`60s`, `500ms`, `2m`) so unit-bearing + values are not altered. + - Treat a positive bare numeric value as seconds (`60` → `60s`). + - Keep the caller-provided default for missing, zero, negative, or malformed + values. + - Route `emitSlowResponse` through this parser with a five-second default and + preserve the existing background-command behavior. + - Preserve the existing `/background 12` behavior already covered by the + parser table; the new behavior is limited to the `/slow` caller. + +## Tests + +- **What:** Duration argument resolution for `/slow` and existing background + commands, including bare seconds, explicit units, defaults, and invalid + values. + **File:** `apps/backend/cmd/mock-agent/background_test.go` (rename or + generalize the existing parser test as needed). + **How:** Table-driven unit test; the `/slow 60` case must fail before the + parser change because it currently resolves to the supplied eight-second + default rather than 60 seconds. +- **What:** The `/slow` handler uses the resolved duration in its simulated + response path. + **File:** `apps/backend/cmd/mock-agent/slow_test.go` (new) or the nearest + existing mock-agent handler test file. + **How:** Invoke the handler with a short explicit duration to avoid a + 60-second test, and assert that the response reports that resolved duration; + the parser table separately covers the 60-second bare-number contract. + +No frontend or E2E changes are planned: the ACP command transport and existing +slash-command UI flow are unchanged, and this repair is confined to the mock +agent's backend fixture behavior. + +## Verification Results + +- `cd apps/backend && go test -run '^TestSlowResponseBareNumberUsesSeconds$' -count=1 ./cmd/mock-agent` failed before the production change with the expected five-second marker, then passed after the change. +- `cd apps/backend && go test -run 'Test(ParseCommandDuration|Slow)' ./cmd/mock-agent` — passed (`12 passed`). +- `cd apps/backend && go test ./cmd/mock-agent` — passed (`193 passed`). +- `make -C apps/backend build-mock-agent` — passed; the host mock-agent binary + was rebuilt. The containers E2E project was not run, so the Linux binary was + not required. +- `gofmt -w cmd/mock-agent/handler.go cmd/mock-agent/background_test.go cmd/mock-agent/slow_test.go` — passed. +- `git diff --check` — passed. + +## Implementation Waves And Parallel Candidates + +Wave 1 (sequential): + +- [x] [task-01-slow-duration-parser](task-01-slow-duration-parser.md) + +## Open Questions + +None. diff --git a/docs/plans/mock-agent-slow-duration/task-01-slow-duration-parser.md b/docs/plans/mock-agent-slow-duration/task-01-slow-duration-parser.md new file mode 100644 index 0000000000..ae1b471381 --- /dev/null +++ b/docs/plans/mock-agent-slow-duration/task-01-slow-duration-parser.md @@ -0,0 +1,78 @@ +--- +id: "01-slow-duration-parser" +title: "Support unitless slow durations" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/mock-agent-slow-duration/spec.md" +--- + +# Task 01: Support unitless slow durations + +Implement the `/slow` duration parsing repair after adding the regression +coverage. + +## Acceptance + +- `/slow 60` resolves to 60 seconds, while `/slow 60s`, `/slow 500ms`, and + `/slow 2m` retain their explicit durations. +- `/slow` and invalid, zero, or negative arguments retain the five-second + default; existing `/background` duration behavior remains unchanged. +- The mock-agent response path uses the resolved duration and reports it using + the existing output format. + +## Verification + +Run the focused parser/handler tests first, then the complete mock-agent +package: + +```bash +cd apps/backend +go test -run 'Test(ParseCommandDuration|Slow)' ./cmd/mock-agent +go test ./cmd/mock-agent +``` + +## Files likely touched + +- `apps/backend/cmd/mock-agent/handler.go` +- `apps/backend/cmd/mock-agent/background_test.go` +- `apps/backend/cmd/mock-agent/slow_test.go` + +## Dependencies + +None. + +## Parallelism + +Sequential. The parser and its tests share the same behavior and should land as +one focused change. + +## Inputs + +- `docs/specs/mock-agent-slow-duration/spec.md` +- `docs/plans/mock-agent-slow-duration/plan.md` +- Existing command-duration parser coverage in + `apps/backend/cmd/mock-agent/background_test.go` +- `apps/backend/cmd/mock-agent/AGENTS.md` + +## Output contract + +Report the final files changed, the red and green targeted test results, the +complete mock-agent package result, any risks, and synchronized task/plan +statuses. Do not change the ACP command advertisement, frontend code, or +unrelated mock-agent commands. + +## Results + +- RED: `cd apps/backend && go test -run '^TestSlowResponseBareNumberUsesSeconds$' -count=1 ./cmd/mock-agent` — failed as expected because `/slow 1` reported the five-second path (`0 passed, 1 failed`). +- GREEN: `cd apps/backend && go test -run '^TestSlowResponseBareNumberUsesSeconds$' -count=1 ./cmd/mock-agent` — passed (`1 passed`). +- Focused: `cd apps/backend && go test -run 'Test(ParseCommandDuration|Slow)' ./cmd/mock-agent` — passed (`12 passed`). +- Package: `cd apps/backend && go test ./cmd/mock-agent` — passed (`193 passed`). +- Formatting: `gofmt -w cmd/mock-agent/handler.go cmd/mock-agent/background_test.go cmd/mock-agent/slow_test.go` — passed. +- Diff hygiene: `git diff --check` — passed. +- `make -C apps/backend build-mock-agent` — passed; the host mock-agent binary + was rebuilt. The containers E2E project was not run, so the Linux binary was + not required. +- Review follow-up: the handler-path test uses `/slow 50ms` to keep the suite + fast while the parser table covers `/slow 60`. diff --git a/docs/specs/INDEX.md b/docs/specs/INDEX.md index 3864f793c7..6bf7f66c9c 100644 --- a/docs/specs/INDEX.md +++ b/docs/specs/INDEX.md @@ -221,6 +221,7 @@ System pages (Radarr/Sonarr-style) for status, disk usage, database maintenance, | Spec | Status | |---|---| +| [mock-agent-slow-duration](mock-agent-slow-duration/spec.md) | shipped | | [session-subscription-recovery](session-subscription-recovery/spec.md) | draft | | [npm-nightly-channel](npm-nightly-channel/spec.md) | shipped | | [agent-resume-runtime-recovery](agent-resume-runtime-recovery/spec.md) | shipped | diff --git a/docs/specs/mock-agent-slow-duration/spec.md b/docs/specs/mock-agent-slow-duration/spec.md new file mode 100644 index 0000000000..da18d99ca9 --- /dev/null +++ b/docs/specs/mock-agent-slow-duration/spec.md @@ -0,0 +1,53 @@ +--- +status: shipped +created: 2026-08-08 +owner: codex +--- + +# Mock-agent slow command duration syntax + +## Why + +The mock agent's `/slow` command is used to keep a deterministic agent turn +active while testing queueing, cancellation, and session behavior. Users expect +an argument such as `/slow 60` to mean 60 seconds, but the current command +silently falls back to its five-second default because the parser requires an +explicit duration unit. + +## What + +- `/slow` with no duration keeps its existing five-second default. +- `/slow ` interprets the bare number as seconds. For example, + `/slow 60` holds the turn for 60 seconds. +- `/slow ` continues to accept explicit Go duration forms, + including `/slow 60s`, `/slow 500ms`, and `/slow 2m`. +- Missing, zero, negative, or malformed duration arguments continue to use the + five-second default rather than failing the prompt. + +## Command contract + +The command is exposed through the mock agent's ACP available-command +advertisement as `slow`. Its prompt form is `/slow [duration]`. The resolved +duration controls the total simulated turn delay, and the response reports the +resolved duration using the existing mock-agent output format. + +## Scenarios + +- **GIVEN** the mock agent is ready, **WHEN** it receives `/slow 60`, **THEN** + it simulates a 60-second turn rather than using the five-second default. +- **GIVEN** the mock agent is ready, **WHEN** it receives `/slow 60s`, **THEN** + it simulates the same 60-second turn. +- **GIVEN** the mock agent is ready, **WHEN** it receives `/slow 500ms`, **THEN** + it preserves the explicit 500-millisecond duration. +- **GIVEN** the mock agent is ready, **WHEN** it receives `/slow`, **THEN** it + uses the five-second default. +- **GIVEN** the mock agent is ready, **WHEN** it receives `/slow 0`, a negative + value, or malformed text, **THEN** it uses the five-second default. + +## Out of scope + +- Adding a `/send` command; `/send 60` is not a mock-agent command. +- Changing the default duration or the behavior of `/sleep`, `/background`, or + other mock-agent commands. +- Changing the existing ACP command advertisement or its input hint. +- Adding persistence, configuration, or a feature flag for the duration.