From 25d26c2bf7eaddb74cd22b127a684314843ce403 Mon Sep 17 00:00:00 2001 From: Multica Eve Date: Thu, 6 Aug 2026 16:26:23 +0800 Subject: [PATCH] fix(agent): send Pi prompts over stdin (MUL-5779) (#6485) Co-authored-by: Eve Co-authored-by: multica-agent --- .github/workflows/ci.yml | 7 +- server/pkg/agent/pi.go | 178 ++++++++++++++-- server/pkg/agent/pi_invocation.go | 5 +- .../pkg/agent/pi_invocation_windows_test.go | 9 +- server/pkg/agent/pi_stdin_unix_test.go | 196 ++++++++++++++++++ server/pkg/agent/pi_stdin_windows_test.go | 152 ++++++++++++++ server/pkg/agent/pi_test.go | 99 +++++++-- 7 files changed, 599 insertions(+), 47 deletions(-) create mode 100644 server/pkg/agent/pi_stdin_unix_test.go create mode 100644 server/pkg/agent/pi_stdin_windows_test.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3b449d83ef..b57157f94c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -330,8 +330,9 @@ jobs: - name: Test Windows agent launcher argv/stdin handling working-directory: server # Agent prompts must never reach a Windows launcher through argv: the - # official cursor-agent.ps1 ends in `& node.exe index.js $args`, and - # PowerShell re-serialises $args onto the child command line. Under + # official cursor-agent.ps1 and pi.ps1 launch native children with + # `$args`, and PowerShell re-serialises them onto the child command + # line. Under # Legacy native argument passing (powershell.exe 5.1, pwsh <= 7.2) a # prompt holding embedded quotes is re-tokenised and fragments like # `-X` become flags (#5649). Only a real PowerShell host proves this, @@ -340,7 +341,7 @@ jobs: # the backend job still runs the full package on Linux. # -v so a silent skip (no PowerShell host resolved, or a -run pattern # that stops matching) is visible in the log instead of passing as "ok". - run: go test ./pkg/agent -v -run '^(TestCursorExecutePromptSurvivesPowerShellShim|TestPlatformCursorInvocation|TestPlatformCopilotInvocation|TestPlatformPiInvocation)' -count=1 -timeout=5m + run: go test ./pkg/agent -v -run '^(TestCursorExecutePromptSurvivesPowerShellShim|TestPiExecutePromptSurvivesPowerShellShim|TestPlatformCursorInvocation|TestPlatformCopilotInvocation|TestPlatformPiInvocation)' -count=1 -timeout=5m - name: Test bounded Codex cleanup with inherited stdout descendant working-directory: server diff --git a/server/pkg/agent/pi.go b/server/pkg/agent/pi.go index 9648b56b63..e5c6266b3c 100644 --- a/server/pkg/agent/pi.go +++ b/server/pkg/agent/pi.go @@ -4,12 +4,14 @@ import ( "context" "encoding/json" "fmt" + "io" "log/slog" "os" "os/exec" "path/filepath" "regexp" "strings" + "sync" "time" ) @@ -173,6 +175,13 @@ func isPiToolNameByte(b byte) bool { } func (b *piBackend) Execute(ctx context.Context, prompt string, opts ExecOptions) (*Session, error) { + // Pi trims piped stdin before building its initial message. Reject an empty + // task here so whitespace-only input cannot turn into a successful process + // with no turn, no output, and an empty session. + if strings.TrimSpace(prompt) == "" { + return nil, fmt.Errorf("pi prompt must not be empty") + } + execName := b.cfg.ExecutablePath if execName == "" { execName = "pi" @@ -201,7 +210,7 @@ func (b *piBackend) Execute(ctx context.Context, prompt string, opts ExecOptions runCtx, cancel := runContext(ctx, timeout) - args := buildPiArgs(prompt, sessionPath, opts, b.cfg.Logger) + args := buildPiArgs(sessionPath, opts, b.cfg.Logger) argv0, cmdArgs := choosePiInvocation(execName, lookedUp, args, b.cfg.Logger) cmd := exec.CommandContext(runCtx, argv0, cmdArgs...) @@ -218,35 +227,47 @@ func (b *piBackend) Execute(ctx context.Context, prompt string, opts ExecOptions cancel() return nil, fmt.Errorf("pi stdout pipe: %w", err) } - // Attach an explicit stdin pipe so we can close it ourselves. Pi reads - // its prompt from argv (positional, see buildPiArgs) and never expects - // interactive input, but when the parent leaves cmd.Stdin nil and the - // daemon is run under systemd, Pi has been observed to block in its - // event loop awaiting stdin events instead of progressing to "done" - // (#2188). Closing the pipe immediately after Start delivers an - // explicit EOF on a FIFO, which unblocks Pi's readable side. + // Pi reads piped stdin to EOF as its initial prompt in print/JSON mode. + // Keeping user-controlled text off argv prevents the npm PowerShell shim + // from re-tokenising embedded quotes into CLI flags on Windows (#6457). + // The explicit close remains part of the #2188 contract too: under systemd, + // Pi has been observed to wait indefinitely when stdin never reaches EOF. stdin, err := cmd.StdinPipe() if err != nil { cancel() return nil, fmt.Errorf("pi stdin pipe: %w", err) } + var closeStdinOnce sync.Once + closeStdin := func() { closeStdinOnce.Do(func() { _ = stdin.Close() }) } cmd.Stderr = newLogWriter(b.cfg.Logger, "[pi:stderr] ") if err := cmd.Start(); err != nil { - _ = stdin.Close() + closeStdin() cancel() return nil, fmt.Errorf("start pi: %w", err) } - _ = stdin.Close() b.cfg.Logger.Info("pi started", "pid", cmd.Process.Pid, "cwd", opts.Cwd, "model", opts.Model) msgCh := make(chan Message, 256) resCh := make(chan Result, 1) - // Close stdout when the context is cancelled so scanner.Scan() unblocks. + // Write concurrently with stdout consumption. A large prompt can fill the + // stdin pipe while the child fills stdout; serialising those operations can + // deadlock both processes. Closing stdin signals the end of Pi's prompt. + writeErrCh := make(chan error, 1) + go func() { + _, err := io.WriteString(stdin, prompt) + closeStdin() + writeErrCh <- err + }() + + // Close both pipes when the context is cancelled. Closing stdin releases a + // writer blocked on a child that stopped reading; closing stdout releases the + // stream scanner. go func() { <-runCtx.Done() + closeStdin() _ = stdout.Close() }() @@ -363,6 +384,10 @@ func (b *piBackend) Execute(ctx context.Context, prompt string, opts ExecOptions waitErr := cmd.Wait() duration := time.Since(startTime) + // Wait closes the process pipes, so a prompt write still blocked when the + // child exited has returned by now. The writer sends exactly once. + writeErr := <-writeErrCh + if runCtx.Err() == context.DeadlineExceeded { finalStatus = "timeout" finalError = fmt.Sprintf("pi timed out after %s", timeout) @@ -372,6 +397,9 @@ func (b *piBackend) Execute(ctx context.Context, prompt string, opts ExecOptions } else if waitErr != nil && finalStatus == "completed" { finalStatus = "failed" finalError = fmt.Sprintf("pi exited with error: %v", waitErr) + } else if writeErr != nil && finalStatus == "completed" { + finalStatus = "failed" + finalError = fmt.Sprintf("pi prompt write failed: %v", writeErr) } b.cfg.Logger.Info("pi finished", "pid", cmd.Process.Pid, "status", finalStatus, "duration", duration.Round(time.Millisecond).String()) @@ -480,19 +508,78 @@ var piBlockedArgs = map[string]blockedArgMode{ "--session": blockedWithValue, // daemon manages the session path } +// piCustomArgModes mirrors Pi 0.83's built-in parser closely enough to +// distinguish option values from positional messages. Unknown long flags are +// extension flags and may take one optional value; unknown short flags are left +// intact so Pi can report them as it did before. +var piCustomArgModes = map[string]blockedArgMode{ + "--help": blockedStandalone, + "-h": blockedStandalone, + "--version": blockedStandalone, + "-v": blockedStandalone, + "--continue": blockedStandalone, + "-c": blockedStandalone, + "--resume": blockedStandalone, + "-r": blockedStandalone, + "--provider": blockedWithValue, + "--model": blockedWithValue, + "--api-key": blockedWithValue, + "--system-prompt": blockedWithValue, + "--append-system-prompt": blockedWithValue, + "--name": blockedWithValue, + "-n": blockedWithValue, + "--no-session": blockedStandalone, + "--session-id": blockedWithValue, + "--fork": blockedWithValue, + "--session-dir": blockedWithValue, + "--models": blockedWithValue, + "--no-tools": blockedStandalone, + "-nt": blockedStandalone, + "--no-builtin-tools": blockedStandalone, + "-nbt": blockedStandalone, + "--tools": blockedWithValue, + "-t": blockedWithValue, + "--exclude-tools": blockedWithValue, + "-xt": blockedWithValue, + "--thinking": blockedWithValue, + "--export": blockedWithValue, + "--extension": blockedWithValue, + "-e": blockedWithValue, + "--no-extensions": blockedStandalone, + "-ne": blockedStandalone, + "--skill": blockedWithValue, + "--prompt-template": blockedWithValue, + "--theme": blockedWithValue, + "--no-skills": blockedStandalone, + "-ns": blockedStandalone, + "--no-prompt-templates": blockedStandalone, + "-np": blockedStandalone, + "--no-themes": blockedStandalone, + "--no-context-files": blockedStandalone, + "-nc": blockedStandalone, + "--list-models": blockedOptionalValue, + "--verbose": blockedStandalone, + "--approve": blockedStandalone, + "-a": blockedStandalone, + "--no-approve": blockedStandalone, + "-na": blockedStandalone, + "--offline": blockedStandalone, +} + // buildPiArgs assembles the argv for a one-shot Pi invocation. // // Flags: // -// -p non-interactive mode (prompt is positional) +// -p non-interactive mode (prompt arrives on stdin) // --mode json emit one JSON event per line on stdout // --session session log file (created upfront, reused on resume) // --provider provider, when Model is "provider/id" // --model model identifier // -// Custom args appended before the positional prompt. The prompt is a -// positional argument and must be last. -func buildPiArgs(prompt, sessionPath string, opts ExecOptions, logger *slog.Logger) []string { +// The prompt is deliberately absent from argv. Pi reads a non-TTY stdin in +// print/JSON mode; using that supported path prevents Windows PowerShell's npm +// shim from re-tokenising prompt content into options. +func buildPiArgs(sessionPath string, opts ExecOptions, logger *slog.Logger) []string { args := []string{ "-p", "--mode", "json", @@ -519,11 +606,68 @@ func buildPiArgs(prompt, sessionPath string, opts ExecOptions, logger *slog.Logg // Pi loads the per-task AGENTS.md the daemon writes into the workdir, so // inlining the same runtime brief would duplicate it on every turn. // Verified against Pi 0.67.2 (MUL-5392). - args = append(args, filterCustomArgs(opts.CustomArgs, piBlockedArgs, logger)...) - args = append(args, prompt) + args = append(args, filterPiCustomArgs(opts.CustomArgs, logger)...) return args } +// filterPiCustomArgs removes @file and positional message inputs from +// custom_args. Once the task prompt is delivered on stdin, Pi concatenates the +// first positional message to stdin with no separator. Keeping those tokens +// would silently mutate or replace the task prompt. Option values remain +// supported, including values for extension-provided --long flags. +func filterPiCustomArgs(args []string, logger *slog.Logger) []string { + args = filterCustomArgs(args, piBlockedArgs, logger) + filtered := make([]string, 0, len(args)) + for i := 0; i < len(args); i++ { + arg := args[i] + if strings.HasPrefix(arg, "@") { + if logger != nil { + logger.Warn("custom_args: Pi file input would alter the stdin task prompt, skipping") + } + continue + } + if !strings.HasPrefix(arg, "-") { + if logger != nil { + logger.Warn("custom_args: Pi positional input would alter the stdin task prompt, skipping") + } + continue + } + + flag := arg + hasInlineValue := false + if idx := strings.Index(arg, "="); idx > 0 { + flag = arg[:idx] + hasInlineValue = true + } + filtered = append(filtered, arg) + if hasInlineValue { + continue + } + + mode, known := piCustomArgModes[flag] + if !known { + if strings.HasPrefix(flag, "--") { + mode = blockedOptionalValue + } else { + continue + } + } + switch mode { + case blockedWithValue: + if i+1 < len(args) { + filtered = append(filtered, args[i+1]) + i++ + } + case blockedOptionalValue: + if i+1 < len(args) && !strings.HasPrefix(args[i+1], "-") && !strings.HasPrefix(args[i+1], "@") { + filtered = append(filtered, args[i+1]) + i++ + } + } + } + return filtered +} + // splitPiModel parses a "provider/model" string into its parts. Plain // "model" strings pass through as (provider="", model="model"). func splitPiModel(s string) (provider, model string) { diff --git a/server/pkg/agent/pi_invocation.go b/server/pkg/agent/pi_invocation.go index c243f4c49f..b3ff04c4a7 100644 --- a/server/pkg/agent/pi_invocation.go +++ b/server/pkg/agent/pi_invocation.go @@ -18,8 +18,9 @@ import "log/slog" // Pi session JSONL records only the first line of the prompt // (#3306). To stay on the official launch path while avoiding that // re-tokenisation, we resolve pi.ps1 next to the .cmd and invoke -// PowerShell with `-File ` directly, letting Go pass each argv -// as a separate token. +// PowerShell with `-File ` directly. The task prompt now travels on +// stdin (#6457), but the rewrite remains necessary for multi-line custom +// option values and installations that still use the npm batch launcher. // // The Windows-specific behaviour is implemented in // pi_invocation_windows.go; on other platforms we fall through to a diff --git a/server/pkg/agent/pi_invocation_windows_test.go b/server/pkg/agent/pi_invocation_windows_test.go index 2a6c324a7f..51899a6440 100644 --- a/server/pkg/agent/pi_invocation_windows_test.go +++ b/server/pkg/agent/pi_invocation_windows_test.go @@ -13,11 +13,10 @@ import ( // TestPlatformPiInvocation_RewritesCmdLauncherToPowerShellFile is the core // Windows test: when LookPath resolves pi to the npm-installed .cmd // launcher and a sibling pi.ps1 exists, we should invoke PowerShell with -// -File and forward every original arg unchanged — including the -// multi-line positional prompt that would otherwise be mangled by the -// cmd.exe %* re-expansion inside pi.cmd. This is the regression test for -// #3306: daemon argv carried the full prompt, but Pi's session JSONL only -// recorded the first line. +// -File and forward every original arg unchanged — including a synthetic +// multi-line value that cmd.exe %* would otherwise mangle. The task prompt now +// travels on stdin (#6457), but this preserves the #3306 launcher guarantee for +// custom option values and keeps the historical failure mode covered. func TestPlatformPiInvocation_RewritesCmdLauncherToPowerShellFile(t *testing.T) { dir := t.TempDir() cmdPath := filepath.Join(dir, "pi.cmd") diff --git a/server/pkg/agent/pi_stdin_unix_test.go b/server/pkg/agent/pi_stdin_unix_test.go new file mode 100644 index 0000000000..7a15173da2 --- /dev/null +++ b/server/pkg/agent/pi_stdin_unix_test.go @@ -0,0 +1,196 @@ +//go:build unix + +package agent + +import ( + "context" + "fmt" + "log/slog" + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +// piStdinProbe runs the Pi backend against a fake CLI that records argv, +// drains stdin to EOF, and emits a minimal successful Pi JSON stream. +func piStdinProbe(t *testing.T, prompt string) ([]string, string, Result) { + t.Helper() + + dir := t.TempDir() + argvPath := filepath.Join(dir, "argv.txt") + stdinPath := filepath.Join(dir, "stdin.txt") + sessionPath := filepath.Join(dir, "session.jsonl") + + script := fmt.Sprintf(`#!/bin/sh +: > %[1]q +for a in "$@"; do printf '%%s\n' "$a" >> %[1]q; done +cat > %[2]q +printf '%%s\n' '{"type":"agent_start"}' +printf '%%s\n' '{"type":"turn_start"}' +printf '%%s\n' '{"type":"message_update","assistantMessageEvent":{"type":"text_delta","delta":"ok"}}' +printf '%%s\n' '{"type":"turn_end","message":{"role":"assistant","model":"test","usage":{"input":1,"output":1,"cacheRead":0,"cacheWrite":0,"totalTokens":2}}}' +`, argvPath, stdinPath) + + fakePath := filepath.Join(dir, "pi") + writeTestExecutable(t, fakePath, []byte(script)) + + backend, err := New("pi", Config{ExecutablePath: fakePath, Logger: slog.Default()}) + if err != nil { + t.Fatalf("New(pi): %v", err) + } + session, err := backend.Execute(t.Context(), prompt, ExecOptions{ + Timeout: 30 * time.Second, + ResumeSessionID: sessionPath, + Model: "cpa/grok-4.5-high", + }) + if err != nil { + t.Fatalf("Execute: %v", err) + } + go func() { + for range session.Messages { + } + }() + result := <-session.Result + + argvRaw, err := os.ReadFile(argvPath) + if err != nil { + t.Fatalf("read recorded argv: %v", err) + } + stdinRaw, err := os.ReadFile(stdinPath) + if err != nil { + t.Fatalf("read recorded stdin: %v", err) + } + argv := strings.Split(strings.TrimSuffix(string(argvRaw), "\n"), "\n") + return argv, string(stdinRaw), result +} + +// TestPiExecuteSendsBuilderPromptOnStdinNotArgv is the regression for #6457. +// The Agent Builder wire envelope contains JSON quotes and Markdown list +// markers that Windows PowerShell 5.1 can re-tokenise into Pi CLI flags. +func TestPiExecuteSendsBuilderPromptOnStdinNotArgv(t *testing.T) { + t.Parallel() + + prompt := "MULTICA_AGENT_BUILDER_INPUT\n" + + "{\n" + + ` "user_request": "专注本机 CPA 有关的所有工作",` + "\n" + + ` "current_draft": {"instructions": "Run go build -ldflags \"-X main.version=foo\""},` + "\n" + + ` "available_workspace_skills": [{"description": "- inspect local changes"}]` + "\n" + + "}" + + argv, stdinGot, result := piStdinProbe(t, prompt) + + if stdinGot != prompt { + t.Errorf("prompt did not arrive on stdin intact:\n got %q\n want %q", stdinGot, prompt) + } + for _, arg := range argv { + for _, needle := range []string{"MULTICA_AGENT_BUILDER_INPUT", "user_request", "-X", "inspect local"} { + if strings.Contains(arg, needle) { + t.Errorf("prompt fragment %q leaked into argv element %q; argv=%v", needle, arg, argv) + } + } + } + joined := strings.Join(argv, " ") + for _, want := range []string{"-p", "--mode json", "--session", "--provider cpa", "--model grok-4.5-high"} { + if !strings.Contains(joined, want) { + t.Errorf("expected %q in argv, got %v", want, argv) + } + } + if result.Status != "completed" || result.Output != "ok" { + t.Fatalf("result = %+v, want completed with output ok", result) + } +} + +// TestPiExecuteLargePromptDoesNotDeadlock forces both process pipes past +// capacity: the child floods stdout before reading stdin, while the parent +// writes a large prompt. Only concurrent writing and scanning can advance. +func TestPiExecuteLargePromptDoesNotDeadlock(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + stdinPath := filepath.Join(dir, "stdin.txt") + sessionPath := filepath.Join(dir, "session.jsonl") + script := fmt.Sprintf(`#!/bin/sh +yes '{"type":"noise","pad":"0123456789012345678901234567890123456789"}' | head -n 8000 +cat > %[1]q +printf '%%s\n' '{"type":"agent_start"}' +printf '%%s\n' '{"type":"turn_end","message":{"role":"assistant","model":"test","usage":{"input":1,"output":1,"cacheRead":0,"cacheWrite":0,"totalTokens":2}}}' +`, stdinPath) + fakePath := filepath.Join(dir, "pi") + writeTestExecutable(t, fakePath, []byte(script)) + + prompt := strings.Repeat("multica pi stdin payload 0123456789\n", 16_384) + if len(prompt) < 512*1024 { + t.Fatalf("test prompt too small: %d bytes", len(prompt)) + } + backend, err := New("pi", Config{ExecutablePath: fakePath, Logger: slog.Default()}) + if err != nil { + t.Fatalf("New(pi): %v", err) + } + session, err := backend.Execute(t.Context(), prompt, ExecOptions{ + Timeout: 30 * time.Second, + ResumeSessionID: sessionPath, + }) + if err != nil { + t.Fatalf("Execute: %v", err) + } + go func() { + for range session.Messages { + } + }() + result := <-session.Result + + stdinRaw, err := os.ReadFile(stdinPath) + if err != nil { + t.Fatalf("read recorded stdin: %v", err) + } + if string(stdinRaw) != prompt { + t.Errorf("large stdin prompt corrupted: got %d bytes, want %d", len(stdinRaw), len(prompt)) + } + if result.Status != "completed" { + t.Fatalf("status = %q, want completed; error=%q", result.Status, result.Error) + } +} + +// TestPiExecuteCancelReleasesBlockedPromptWrite pins cancellation cleanup. The +// child never reads stdin, so cancelling must close the pipe, release the +// blocked writer, and return an aborted result instead of hanging. +func TestPiExecuteCancelReleasesBlockedPromptWrite(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + fakePath := filepath.Join(dir, "pi") + writeTestExecutable(t, fakePath, []byte("#!/bin/sh\nsleep 120\n")) + prompt := strings.Repeat("blocked pi write payload 0123456789\n", 16_384) + + backend, err := New("pi", Config{ExecutablePath: fakePath, Logger: slog.Default()}) + if err != nil { + t.Fatalf("New(pi): %v", err) + } + ctx, cancel := context.WithCancel(t.Context()) + session, err := backend.Execute(ctx, prompt, ExecOptions{ + Timeout: 120 * time.Second, + ResumeSessionID: filepath.Join(dir, "session.jsonl"), + }) + if err != nil { + cancel() + t.Fatalf("Execute: %v", err) + } + go func() { + for range session.Messages { + } + }() + + time.Sleep(200 * time.Millisecond) + cancel() + + select { + case result := <-session.Result: + if result.Status != "aborted" { + t.Fatalf("status = %q, want aborted; error=%q", result.Status, result.Error) + } + case <-time.After(30 * time.Second): + t.Fatal("cancel did not release blocked Pi prompt write") + } +} diff --git a/server/pkg/agent/pi_stdin_windows_test.go b/server/pkg/agent/pi_stdin_windows_test.go new file mode 100644 index 0000000000..3ff3977125 --- /dev/null +++ b/server/pkg/agent/pi_stdin_windows_test.go @@ -0,0 +1,152 @@ +//go:build windows + +package agent + +import ( + "fmt" + "io" + "log/slog" + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +const ( + piShimHelperEnv = "MULTICA_PI_SHIM_HELPER" + piShimHelperArgvFile = "MULTICA_PI_SHIM_ARGV_FILE" + piShimHelperInFile = "MULTICA_PI_SHIM_STDIN_FILE" +) + +// TestPiShimHelperProcess is re-executed by the fake pi.ps1 as its native +// child. It records the argv and stdin that made it through PowerShell, emits a +// successful Pi event stream, and exits before the Go test framework writes to +// stdout. +func TestPiShimHelperProcess(t *testing.T) { + if os.Getenv(piShimHelperEnv) != "1" { + t.Skip("helper process; only runs when re-executed by the shim") + } + + var forwarded []string + for i, arg := range os.Args { + if arg == "--" { + forwarded = os.Args[i+1:] + break + } + } + if err := os.WriteFile(os.Getenv(piShimHelperArgvFile), []byte(strings.Join(forwarded, "\n")), 0o644); err != nil { + fmt.Fprintf(os.Stderr, "helper: write argv: %v\n", err) + os.Exit(1) + } + stdin, err := io.ReadAll(os.Stdin) + if err != nil { + fmt.Fprintf(os.Stderr, "helper: read stdin: %v\n", err) + os.Exit(1) + } + if err := os.WriteFile(os.Getenv(piShimHelperInFile), stdin, 0o644); err != nil { + fmt.Fprintf(os.Stderr, "helper: write stdin: %v\n", err) + os.Exit(1) + } + + fmt.Println(`{"type":"agent_start"}`) + fmt.Println(`{"type":"turn_end","message":{"role":"assistant","model":"test","usage":{"input":1,"output":1,"cacheRead":0,"cacheWrite":0,"totalTokens":2}}}`) + os.Exit(0) +} + +// TestPiExecutePromptSurvivesPowerShellShim exercises the complete Windows +// production boundary that #6457 crosses: +// +// Go -> powershell -File pi.ps1 -> native child +// +// The prompt must be absent from the argv PowerShell re-serialises and must be +// inherited by the native child on stdin. Every available PowerShell host is +// exercised because Windows PowerShell 5.1 uses the Legacy argument mode that +// exposed the bug, while current pwsh uses Standard mode. +func TestPiExecutePromptSurvivesPowerShellShim(t *testing.T) { + hosts := availablePowerShellHosts() + if len(hosts) == 0 { + t.Skip("no PowerShell host available") + } + for _, host := range hosts { + t.Run(filepath.Base(host), func(t *testing.T) { + stubPowerShell(t, host, true) + assertPiPromptSurvivesShim(t) + }) + } +} + +func assertPiPromptSurvivesShim(t *testing.T) { + t.Helper() + + self, err := os.Executable() + if err != nil { + t.Fatalf("locate test binary: %v", err) + } + dir := t.TempDir() + argvPath := filepath.Join(dir, "argv.txt") + stdinPath := filepath.Join(dir, "stdin.txt") + sessionPath := filepath.Join(dir, "session.jsonl") + + cmdPath := filepath.Join(dir, "pi.cmd") + writeFile(t, cmdPath, "@echo off\r\npowershell -NoProfile -ExecutionPolicy Bypass -File \"%~dp0pi.ps1\" %*\r\n") + ps1 := fmt.Sprintf(""+ + "$env:%s = '1'\r\n"+ + "$env:%s = '%s'\r\n"+ + "$env:%s = '%s'\r\n"+ + "& '%s' '-test.run=^TestPiShimHelperProcess$' '--' $args\r\n"+ + "exit $LASTEXITCODE\r\n", + piShimHelperEnv, + piShimHelperArgvFile, argvPath, + piShimHelperInFile, stdinPath, + self) + writeFile(t, filepath.Join(dir, "pi.ps1"), ps1) + + prompt := "MULTICA_AGENT_BUILDER_INPUT\n" + + `{"instructions":"Run go build -ldflags \"-X main.version=foo\"","description":"- local work"}` + backend, err := New("pi", Config{ExecutablePath: cmdPath, Logger: slog.Default()}) + if err != nil { + t.Fatalf("New(pi): %v", err) + } + session, err := backend.Execute(t.Context(), prompt, ExecOptions{ + Timeout: 60 * time.Second, + ResumeSessionID: sessionPath, + Model: "cpa/grok-4.5-high", + }) + if err != nil { + t.Fatalf("Execute: %v", err) + } + go func() { + for range session.Messages { + } + }() + result := <-session.Result + + argvRaw, err := os.ReadFile(argvPath) + if err != nil { + t.Fatalf("native child never recorded argv: %v; result=%+v", err, result) + } + stdinRaw, err := os.ReadFile(stdinPath) + if err != nil { + t.Fatalf("native child never recorded stdin: %v; result=%+v", err, result) + } + for _, arg := range strings.Split(strings.TrimSuffix(string(argvRaw), "\n"), "\n") { + for _, needle := range []string{"MULTICA_AGENT_BUILDER_INPUT", "instructions", "-X", "local work"} { + if strings.Contains(arg, needle) { + t.Errorf("prompt fragment %q leaked into native child argv element %q", needle, arg) + } + } + } + gotArgv := string(argvRaw) + for _, want := range []string{"-p", "--mode", "json", "--session", "--provider", "cpa", "--model", "grok-4.5-high"} { + if !strings.Contains(gotArgv, want) { + t.Errorf("expected %q to reach native child; argv=%q", want, gotArgv) + } + } + if string(stdinRaw) != prompt { + t.Errorf("prompt did not survive Go -> PowerShell -> native child:\n got %q\n want %q", string(stdinRaw), prompt) + } + if result.Status != "completed" { + t.Fatalf("status = %q, want completed; error=%q", result.Status, result.Error) + } +} diff --git a/server/pkg/agent/pi_test.go b/server/pkg/agent/pi_test.go index 2f48208617..4a2726b41b 100644 --- a/server/pkg/agent/pi_test.go +++ b/server/pkg/agent/pi_test.go @@ -14,7 +14,7 @@ func TestBuildPiArgsNoToolAllowlist(t *testing.T) { // Extension tools registered via Pi's registerTool() must not be // filtered out by a hardcoded --tools allowlist. Omitting --tools // lets Pi use its full tool registry. See #2379. - args := buildPiArgs("test prompt", "/tmp/session.jsonl", ExecOptions{}, slog.Default()) + args := buildPiArgs("/tmp/session.jsonl", ExecOptions{}, slog.Default()) for i, arg := range args { if arg == "--tools" { t.Errorf("buildPiArgs emits --tools %q; should not restrict tool registry (see #2379)", args[i+1]) @@ -23,7 +23,7 @@ func TestBuildPiArgsNoToolAllowlist(t *testing.T) { } func TestBuildPiArgsBasicFlags(t *testing.T) { - args := buildPiArgs("hello world", "/tmp/s.jsonl", ExecOptions{ + args := buildPiArgs("/tmp/s.jsonl", ExecOptions{ Model: "anthropic/claude-sonnet-4-20250514", }, slog.Default()) @@ -34,9 +34,10 @@ func TestBuildPiArgsBasicFlags(t *testing.T) { } } - // Prompt must be the last positional argument. - if args[len(args)-1] != "hello world" { - t.Errorf("prompt should be last arg, got %q", args[len(args)-1]) + for _, arg := range args { + if arg == "hello world" { + t.Fatalf("prompt leaked into argv: %v", args) + } } } @@ -44,7 +45,7 @@ func TestBuildPiArgsBasicFlags(t *testing.T) { // daemon never populates SystemPrompt for it (providerNeedsInlineSystemPrompt). // Forwarding it anyway would duplicate the whole runtime brief on every turn. func TestBuildPiArgsIgnoresSystemPrompt(t *testing.T) { - args := buildPiArgs("hello world", "/tmp/s.jsonl", ExecOptions{ + args := buildPiArgs("/tmp/s.jsonl", ExecOptions{ SystemPrompt: "the entire multica runtime brief", }, slog.Default()) @@ -60,7 +61,7 @@ func TestBuildPiArgsIgnoresSystemPrompt(t *testing.T) { func TestBuildPiArgsCustomArgsAppended(t *testing.T) { // Users can still restrict tools via custom_args if desired. - args := buildPiArgs("prompt", "/tmp/s.jsonl", ExecOptions{ + args := buildPiArgs("/tmp/s.jsonl", ExecOptions{ CustomArgs: []string{"--tools", "read,bash"}, }, slog.Default()) @@ -75,16 +76,67 @@ func TestBuildPiArgsCustomArgsAppended(t *testing.T) { } } -// TestPiExecuteAttachesStdinPipe verifies that the Pi backend spawns the -// child with an explicit stdin pipe (FIFO) instead of leaving cmd.Stdin -// nil. Without an explicit pipe, Pi has been observed to block under -// systemd waiting for stdin events (#2188); attaching and immediately -// closing a pipe delivers a clean EOF on a FIFO and unblocks Pi. +func TestBuildPiArgsFiltersCustomInputButKeepsOptionValues(t *testing.T) { + t.Parallel() + + args := buildPiArgs("/tmp/s.jsonl", ExecOptions{ + CustomArgs: []string{ + "--tools", "read,bash", + "positional-input", + "@prompt.md", + "--verbose", + "after-boolean", + "--extension-option", "extension-value", + "--thinking", "high", + "--offline", + "trailing-input", + }, + }, slog.Default()) + + joined := strings.Join(args, "\x00") + for _, unwanted := range []string{"positional-input", "@prompt.md", "after-boolean", "trailing-input"} { + if strings.Contains(joined, unwanted) { + t.Errorf("custom input %q should be filtered, got %v", unwanted, args) + } + } + for _, pair := range [][2]string{ + {"--tools", "read,bash"}, + {"--extension-option", "extension-value"}, + {"--thinking", "high"}, + } { + found := false + for i := 0; i+1 < len(args); i++ { + if args[i] == pair[0] && args[i+1] == pair[1] { + found = true + break + } + } + if !found { + t.Errorf("option/value %q %q missing from %v", pair[0], pair[1], args) + } + } +} + +func TestPiExecuteRejectsEmptyPrompt(t *testing.T) { + t.Parallel() + + backend, err := New("pi", Config{ExecutablePath: "/does/not/need/to/exist", Logger: slog.Default()}) + if err != nil { + t.Fatalf("New(pi): %v", err) + } + if _, err := backend.Execute(t.Context(), " \n\t ", ExecOptions{}); err == nil || !strings.Contains(err.Error(), "prompt must not be empty") { + t.Fatalf("Execute error = %v, want empty-prompt error", err) + } +} + +// TestPiExecuteAttachesStdinPipe verifies that the Pi backend spawns the child +// with an explicit stdin pipe, writes the task prompt, and closes it. Closing +// delivers both the end-of-prompt signal and the EOF that keeps Pi from +// blocking under systemd (#2188). // // The probe is structural rather than behavioral: a shell script in -// place of `pi` inspects /proc/self/fd/0 and only emits a valid event -// stream if stdin is a FIFO. If the fix regresses (stdin nil → /dev/null -// char device), the fake exits non-zero and the test fails. +// place of `pi` inspects /proc/self/fd/0, drains it to EOF, and only emits a +// valid event stream when both the pipe type and prompt are correct. func TestPiExecuteAttachesStdinPipe(t *testing.T) { t.Parallel() if runtime.GOOS != "linux" { @@ -96,14 +148,17 @@ func TestPiExecuteAttachesStdinPipe(t *testing.T) { fakePath := filepath.Join(t.TempDir(), "pi") script := "#!/bin/sh\n" + "kind=$(stat -c '%F' -L /proc/self/fd/0 2>/dev/null || echo unknown)\n" + + "payload=$(cat)\n" + "case \"$kind\" in\n" + " fifo|*pipe*)\n" + - " printf '%s\\n' '{\"type\":\"agent_start\"}'\n" + - " printf '%s\\n' '{\"type\":\"turn_end\",\"message\":{\"role\":\"assistant\",\"model\":\"test\",\"usage\":{\"input\":1,\"output\":1,\"cacheRead\":0,\"cacheWrite\":0,\"totalTokens\":2}}}'\n" + - " exit 0\n" + + " if [ \"$payload\" = 'prompt-over-stdin' ]; then\n" + + " printf '%s\\n' '{\"type\":\"agent_start\"}'\n" + + " printf '%s\\n' '{\"type\":\"turn_end\",\"message\":{\"role\":\"assistant\",\"model\":\"test\",\"usage\":{\"input\":1,\"output\":1,\"cacheRead\":0,\"cacheWrite\":0,\"totalTokens\":2}}}'\n" + + " exit 0\n" + + " fi\n" + " ;;\n" + "esac\n" + - "printf 'stdin was %s; expected fifo\\n' \"$kind\" >&2\n" + + "printf 'stdin was %s with payload %s; expected fifo and prompt\\n' \"$kind\" \"$payload\" >&2\n" + "exit 1\n" writeTestExecutable(t, fakePath, []byte(script)) @@ -114,7 +169,11 @@ func TestPiExecuteAttachesStdinPipe(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - session, err := backend.Execute(ctx, "prompt-ignored", ExecOptions{Timeout: 5 * time.Second}) + sessionPath := filepath.Join(t.TempDir(), "session.jsonl") + session, err := backend.Execute(ctx, "prompt-over-stdin", ExecOptions{ + Timeout: 5 * time.Second, + ResumeSessionID: sessionPath, + }) if err != nil { t.Fatalf("execute: %v", err) }