diff --git a/README.md b/README.md index af26931..b924a71 100644 --- a/README.md +++ b/README.md @@ -137,6 +137,15 @@ discovery; and `--no-context-files`, `--no-approve`, `--no-skills`, `--no-prompt-templates`, `--no-themes`, and `--no-session`. It returns an incompatible-runtime error when any control is unavailable. +Every CR-owned Pi request disables `AGENTS.md`/`CLAUDE.md` context discovery, +supplies its own system prompt, and passes an explicit empty +`--append-system-prompt` source. This also suppresses ambient `SYSTEM.md` and +`APPEND_SYSTEM.md` inputs, including those in the selected agent directory. +CR leaves Pi's selected agent directory and authentication storage in place; +credentials are never copied into disposable scratch state, so Pi can persist +OAuth refresh updates to its original store. Resource discovery remains +disabled, and only reviewers load the CR-owned read-only tool extension. + ```bash cr init --non-interactive \ --llm-provider pi \ diff --git a/docs/development.md b/docs/development.md index 6f3334d..fa56b18 100644 --- a/docs/development.md +++ b/docs/development.md @@ -122,18 +122,32 @@ The tests make no paid or external calls: - Each test gives Pi a fresh `PI_CODING_AGENT_DIR` and `HOME` with `PI_OFFLINE=1`. - The only model is a scripted OpenAI-compatible endpoint on loopback, with a - dummy key. + dummy key stored in the selected directory's `auth.json`. - Reviewer tool calls run the real `__pi-review-tool` helper, dispatched to the test binary by `TestMain`. -- Hostile instructions, extensions, MCP servers, and skills are planted under - the fixture `HOME`'s default `.pi/agent` location and in the reviewer - checkout, while Pi reads the fresh agent directory. The tests fail if any of - them reaches a provider request or runs. - -This shows that the fixture is isolated, not that production runs are. CR does -not replace the developer's own Pi agent directory, so its global instructions -can still reach prompts -([#642](https://github.com/open-cli-collective/codereview-cli/issues/642)). +- Hostile instructions, extensions, and MCP servers are planted in the + actually selected agent directory as well as under the fixture `HOME`'s + default `.pi/agent` location and in the reviewer checkout. Hostile skills + are also planted under the fixture home and checkout. The tests fail if any + resource reaches a provider request or runs. +- Both structured and reviewer cases plant each selected `AGENTS.md`, + `CLAUDE.md`, `AGENTS.override.md`, `SYSTEM.md`, and `APPEND_SYSTEM.md` + independently, so context-file precedence cannot hide an uncovered input. + +The Pi 1.0 prompt-resource contract is explicit: `--no-context-files` disables +context discovery, CR's `--system-prompt` suppresses `SYSTEM.md` discovery, +and `--append-system-prompt` with an empty argument supplies an explicit CLI +source list that suppresses `APPEND_SYSTEM.md` discovery while contributing no +text. See Pi's pinned [argument parser](https://github.com/earendil-works/pi/blob/v1.0.0/packages/coding-agent/src/cli/args.ts) +and [resource loader](https://github.com/earendil-works/pi/blob/v1.0.0/packages/coding-agent/src/core/resource-loader.ts). + +CR does not redirect `PI_CODING_AGENT_DIR`, `HOME`, or Pi's credential store to +achieve isolation. Authentication, model configuration, and OAuth refresh +persistence continue to use the selected directory. The ordinary subprocess +test verifies that a synthetic auth-state update remains in the selected store +after scratch cleanup; it does not exercise a real OAuth provider or refresh. +All installed-runtime evidence is local/mock-provider evidence, not deployed +validation or an operating-system sandbox claim. The CI `pi-runtime` job installs the Makefile version on the Node release it pins and runs the same target. It is not a required check. To move to a new Pi diff --git a/internal/llmadapters/pi_rpc.go b/internal/llmadapters/pi_rpc.go index 3d82139..e4ff37a 100644 --- a/internal/llmadapters/pi_rpc.go +++ b/internal/llmadapters/pi_rpc.go @@ -241,6 +241,7 @@ func (a *PiRPCAdapter) preflightReviewerRuntime(parent context.Context) error { args := append(append([]string(nil), a.commandArgsPrefix...), "--mode", "rpc", "--system-prompt", piRPCReviewerSystemPrompt, + "--append-system-prompt", "", "--no-builtin-tools", "--tools", piRPCReviewerToolNames, "--extension", extensionPath, @@ -401,6 +402,13 @@ func (a *PiRPCAdapter) buildArgs(req Request, extensionPath string) ([]string, e args := []string{ "--mode", "rpc", "--system-prompt", systemPrompt, + // An explicit empty CLI append source suppresses APPEND_SYSTEM.md + // discovery in Pi 1.0. --no-context-files separately suppresses + // AGENTS.md/CLAUDE.md; --system-prompt overrides SYSTEM.md discovery. + // Keep the selected agent directory intact so authentication refreshes + // continue to persist in the user's original credential store. + "--append-system-prompt", "", + "--no-context-files", "--no-extensions", "--no-skills", "--no-prompt-templates", @@ -411,7 +419,7 @@ func (a *PiRPCAdapter) buildArgs(req Request, extensionPath string) ([]string, e args = append(args, "--no-tools") } else { args[3] = piRPCReviewerSystemPrompt - args = append(args, "--no-builtin-tools", "--no-context-files", "--no-approve", "--tools", piRPCReviewerToolNames, "--extension", extensionPath) + args = append(args, "--no-builtin-tools", "--no-approve", "--tools", piRPCReviewerToolNames, "--extension", extensionPath) } if req.Model != "" { args = append(args, "--model", req.Model) @@ -424,21 +432,22 @@ func (a *PiRPCAdapter) buildArgs(req Request, extensionPath string) ([]string, e func (a *PiRPCAdapter) validateArgs(args []string, req Request, extensionPath string) error { allowedFlags := map[string]bool{ - "--mode": true, - "--system-prompt": true, - "--no-tools": false, - "--no-builtin-tools": false, - "--no-context-files": false, - "--no-approve": false, - "--tools": true, - "--no-extensions": false, - "--extension": true, - "--no-skills": false, - "--no-prompt-templates": false, - "--no-themes": false, - "--no-session": false, - "--model": true, - "--thinking": true, + "--mode": true, + "--system-prompt": true, + "--append-system-prompt": true, + "--no-tools": false, + "--no-builtin-tools": false, + "--no-context-files": false, + "--no-approve": false, + "--tools": true, + "--no-extensions": false, + "--extension": true, + "--no-skills": false, + "--no-prompt-templates": false, + "--no-themes": false, + "--no-session": false, + "--model": true, + "--thinking": true, } if err := validateAllowedFlags("pi_rpc", args, allowedFlags); err != nil { return err @@ -451,11 +460,14 @@ func (a *PiRPCAdapter) validateArgs(args []string, req Request, extensionPath st if flagValue(args, "--mode") != "rpc" { return fmt.Errorf("%w: pi_rpc must use rpc mode", ErrUnsafeSubprocessConfig) } - for _, flag := range []string{"--system-prompt", "--no-extensions", "--no-skills", "--no-prompt-templates", "--no-themes", "--no-session"} { + for _, flag := range []string{"--system-prompt", "--append-system-prompt", "--no-context-files", "--no-extensions", "--no-skills", "--no-prompt-templates", "--no-themes", "--no-session"} { if !containsFlag(args, flag) { return fmt.Errorf("%w: missing %s", ErrUnsafeSubprocessConfig, flag) } } + if appendPrompt, ok := flagValueOK(args, "--append-system-prompt"); !ok || appendPrompt != "" { + return fmt.Errorf("%w: pi_rpc must use an explicit empty append system prompt", ErrUnsafeSubprocessConfig) + } wantSystemPrompt := piRPCSystemPrompt if req.ReviewerWorkspace == nil { if !containsFlag(args, "--no-tools") || containsFlag(args, "--no-builtin-tools") || containsFlag(args, "--tools") || containsFlag(args, "-t") || containsFlag(args, "--extension") { diff --git a/internal/llmadapters/pi_rpc_runtime_test.go b/internal/llmadapters/pi_rpc_runtime_test.go index e904541..bd6c510 100644 --- a/internal/llmadapters/pi_rpc_runtime_test.go +++ b/internal/llmadapters/pi_rpc_runtime_test.go @@ -82,6 +82,47 @@ func TestPiRPCRuntimeStructuredJSONWithoutTools(t *testing.T) { } } +// Plant each instruction independently so Pi's AGENTS/CLAUDE precedence cannot +// hide an uncovered source. The selected directory also contains the fixture's +// real models.json and dummy auth.json, which must continue to be used. +func TestPiRPCRuntimeSelectedAgentInstructionIsolation(t *testing.T) { + for _, reviewer := range []bool{false, true} { + mode := "structured" + if reviewer { + mode = "reviewer" + } + t.Run(mode, func(t *testing.T) { + for _, filename := range []string{"AGENTS.md", "CLAUDE.md", "AGENTS.override.md", "SYSTEM.md", "APPEND_SYSTEM.md"} { + t.Run(filename, func(t *testing.T) { + fixture := newPiRuntimeFixture(t, piRuntimeAnswer(`{"ok":true}`, piRuntimeUsage{prompt: 10, completion: 1})) + for _, candidate := range []string{"AGENTS.md", "CLAUDE.md", "SYSTEM.md", "APPEND_SYSTEM.md"} { + if err := os.Remove(filepath.Join(fixture.agentDir, candidate)); err != nil { // #nosec G703 -- synthetic instruction fixtures only. + t.Fatalf("Remove(selected instruction): %v", err) + } + } + fixture.writeFile(filepath.Join(fixture.agentDir, filename), piRuntimeHostileMarker+" selected "+filename+"\n") + req := Request{Model: piRuntimeModel, Prompt: `Return {"ok":true}.`} + wantPrompt := piRPCSystemPrompt + if reviewer { + req.ReviewerWorkspace = fixture.reviewerWorkspace() + wantPrompt = piRPCReviewerSystemPrompt + } + response, err := fixture.run(fixture.adapter(piRuntimeTaskTimeout), req) + if err != nil || string(response.StructuredOutput) != `{"ok":true}` { + t.Fatalf("run output = %q (err %v), want CR response", response.StructuredOutput, err) + } + requests := fixture.provider.requests() + if len(requests) != 1 || !strings.Contains(requests[0].systemText(), wantPrompt) { + t.Fatalf("provider requests = %d, want one with the CR prompt", len(requests)) + } + // assertIsolated checks every request for the marker and the + // dummy selected-store authorization, including tool turns. + }) + } + }) + } +} + func TestPiRPCRuntimeRecoversTransientProviderFailure(t *testing.T) { fixture := newPiRuntimeFixture(t, piRuntimeFailure(http.StatusServiceUnavailable, "fixture transient overload"), @@ -374,7 +415,6 @@ func newPiRuntimeFixture(t *testing.T, steps ...piRuntimeStep) *piRuntimeFixture piRuntimeProviderName: map[string]any{ "baseUrl": fixture.provider.server.URL + "/v1", "api": "openai-completions", - "apiKey": piRuntimeAPIKey, "models": []any{map[string]any{ "id": piRuntimeModelID, "name": "CR runtime fixture", @@ -389,10 +429,16 @@ func newPiRuntimeFixture(t *testing.T, steps ...piRuntimeStep) *piRuntimeFixture }, }, }) + fixture.writeJSON(filepath.Join(fixture.agentDir, "auth.json"), map[string]any{ + piRuntimeProviderName: map[string]any{"type": "api_key", "key": piRuntimeAPIKey}, + }) fixture.writeJSON(filepath.Join(fixture.agentDir, "settings.json"), map[string]any{ "retry": map[string]any{"enabled": true, "maxRetries": 1, "baseDelayMs": 10}, "enableInstallTelemetry": false, }) + // The actual selected directory must reject instruction resources without + // hiding its model configuration, credentials, or refresh destination. + fixture.plantHostileResources(fixture.agentDir, "selected-agent") // Resources at Pi's default global location. The agent directory override // must keep every one of them out of the run. fixture.plantHostileResources(filepath.Join(home, ".pi", "agent"), "global") diff --git a/internal/llmadapters/pi_rpc_test.go b/internal/llmadapters/pi_rpc_test.go index c373781..729943a 100644 --- a/internal/llmadapters/pi_rpc_test.go +++ b/internal/llmadapters/pi_rpc_test.go @@ -73,7 +73,8 @@ func TestPiRPCLaunchSafetyAndSuccess(t *testing.T) { assertFlagValue(t, record.AdapterArgs, "--model", "opencode-go/kimi-k2.6") assertFlagValue(t, record.AdapterArgs, "--thinking", "max") assertFlagValue(t, record.AdapterArgs, "--system-prompt", piRPCSystemPrompt) - for _, flag := range []string{"--no-tools", "--no-extensions", "--no-skills", "--no-prompt-templates", "--no-themes", "--no-session"} { + assertPiRPCEmptyAppend(t, record.AdapterArgs) + for _, flag := range []string{"--no-tools", "--no-context-files", "--no-extensions", "--no-skills", "--no-prompt-templates", "--no-themes", "--no-session"} { if !containsFlag(record.AdapterArgs, flag) { t.Fatalf("args = %#v, want %s", record.AdapterArgs, flag) } @@ -120,6 +121,86 @@ func TestPiRPCStreamRecordsDuration(t *testing.T) { assertSlowSuccessDuration(t, response) } +// This subprocess double models an auth.json refresh without using credentials +// or a provider. Prompt isolation must leave the selected store in place, so +// its updated synthetic state survives invocation scratch cleanup. +func TestPiRPCPreservesSelectedAgentAuthenticationState(t *testing.T) { + for _, reviewer := range []bool{false, true} { + name := "structured" + if reviewer { + name = "reviewer" + } + t.Run(name, func(t *testing.T) { + root := t.TempDir() + agentDir := filepath.Join(root, "selected-agent") + home := filepath.Join(root, "home") + repoDir := filepath.Join(root, "repo") + scratchDir := filepath.Join(root, "scratch") + for _, dir := range []string{agentDir, home, repoDir, scratchDir} { + if err := os.Mkdir(dir, 0o700); err != nil { + t.Fatalf("Mkdir: %v", err) + } + } + authPath := filepath.Join(agentDir, "auth.json") + if err := os.WriteFile(authPath, []byte(`{"fixture":"initial synthetic auth state"}`), 0o600); err != nil { + t.Fatalf("WriteFile(synthetic auth): %v", err) + } + recordPath := filepath.Join(root, "record.json") + adapter := NewPiRPCAdapter(PiRPCOptions{ + Command: os.Args[0], + commandArgsPrefix: piRPCHelperPrefix(), + Env: append(piRPCHelperEnv("success", recordPath), + "PI_CODING_AGENT_DIR="+agentDir, + "HOME="+home, + "LLM_PI_RPC_SYNTHETIC_AUTH_REFRESH=1", + ), + Timeout: 5 * time.Second, + ScratchDirFactory: func() (string, func() error, error) { + dir, err := os.MkdirTemp(scratchDir, "invocation-*") + return dir, func() error { return os.RemoveAll(dir) }, err + }, + }) + req := Request{Model: "fixture/model", Prompt: "fixture prompt"} + wantPrompt := piRPCSystemPrompt + if reviewer { + wantPrompt = piRPCReviewerSystemPrompt + diffPath := filepath.Join(root, "diff.patch") + if err := os.WriteFile(diffPath, []byte("fixture diff\n"), 0o600); err != nil { + t.Fatalf("WriteFile(diff): %v", err) + } + req.ReviewerWorkspace = &ReviewerWorkspaceRequest{ + RepoDir: repoDir, ScratchDir: scratchDir, DiffPath: diffPath, MaxToolOutputBytes: 2048, + } + } + stream, err := adapter.Start(context.Background(), req) + if err != nil { + t.Fatalf("Start: %v", err) + } + if _, err := stream.Wait(context.Background()); err != nil { + t.Fatalf("Wait: %v", err) + } + record := readPiRPCRecord(t, recordPath) + if record.Env["PI_CODING_AGENT_DIR"] != agentDir || record.Env["HOME"] != home { + t.Fatalf("selected agent/home = %q/%q, want %q/%q", record.Env["PI_CODING_AGENT_DIR"], record.Env["HOME"], agentDir, home) + } + assertFlagValue(t, record.AdapterArgs, "--system-prompt", wantPrompt) + if appendPrompt, present := flagValueOK(record.AdapterArgs, "--append-system-prompt"); !present || appendPrompt != "" { + t.Fatalf("append system prompt = %q (present %v), want explicit empty input", appendPrompt, present) + } + if !containsFlag(record.AdapterArgs, "--no-context-files") { + t.Fatalf("args = %#v, want context discovery disabled", record.AdapterArgs) + } + data, err := os.ReadFile(authPath) // #nosec G304 -- synthetic test-owned auth state only. + if err != nil || string(data) != `{"fixture":"refreshed synthetic auth state"}` { + t.Fatalf("synthetic auth state = %q (err %v), want refresh in original selected store", data, err) + } + if entries, err := os.ReadDir(scratchDir); err != nil || len(entries) != 0 { + t.Fatalf("scratch entries = %v (err %v), want cleanup without auth state loss", entries, err) + } + }) + } +} + func TestPiRPCFailureLeavesDurationZero(t *testing.T) { recordPath := filepath.Join(t.TempDir(), "record.json") adapter := NewPiRPCAdapter(PiRPCOptions{ @@ -202,6 +283,7 @@ func TestPiRPCReviewerWorkspaceLaunchUsesOnlyCROwnedTools(t *testing.T) { t.Fatalf("args = %#v, reviewer extension tools must remain enabled", record.AdapterArgs) } assertFlagValue(t, record.AdapterArgs, "--tools", piRPCReviewerToolNames) + assertPiRPCEmptyAppend(t, record.AdapterArgs) reviewerPrompt := flagValue(record.AdapterArgs, "--system-prompt") for _, instruction := range []string{"Invoke cr_diff before cr_read, cr_search, or cr_list", "If cr_diff fails", "view=symlink", "assignment.base_only_symlink_paths", "ordinary cr_read and no view", "payload_omitted_reason", "keep that path skipped", "payload_size=0 is an inspected empty payload"} { if !strings.Contains(reviewerPrompt, instruction) { @@ -603,6 +685,7 @@ func TestPiRPCReviewerPreflightUsesEmptyDiscoveryDisabledDirectory(t *testing.T) } assertFlagValue(t, record.AdapterArgs, "--mode", "rpc") assertFlagValue(t, record.AdapterArgs, "--tools", piRPCReviewerToolNames) + assertPiRPCEmptyAppend(t, record.AdapterArgs) for _, flag := range []string{"--no-builtin-tools", "--no-extensions", "--no-skills", "--no-prompt-templates", "--no-themes", "--no-context-files", "--no-approve", "--no-session"} { if !containsFlag(record.AdapterArgs, flag) { t.Fatalf("preflight args = %#v, want %s", record.AdapterArgs, flag) @@ -1791,6 +1874,10 @@ func TestPiRPCRejectsUnsafeSpecs(t *testing.T) { {name: "tools allowlist", args: append([]string{"--tools", "read"}, args...)}, {name: "missing no tools", args: removeFlag(args, "--no-tools")}, {name: "missing no extensions", args: removeFlag(args, "--no-extensions")}, + {name: "missing context disable", args: removeFlag(args, "--no-context-files")}, + {name: "missing append override", args: removeFlagWithValue(args, "--append-system-prompt")}, + {name: "nonempty append override", args: replaceFlagValue(args, "--append-system-prompt", "ambient instructions")}, + {name: "duplicate append override", args: append(append([]string(nil), args...), "--append-system-prompt", "")}, {name: "unexpected flag", args: append([]string{"--unexpected"}, args...)}, {name: "text mode", args: replaceFlagValue(args, "--mode", "text")}, {name: "wrong system prompt", args: replaceFlagValue(args, "--system-prompt", "be loose")}, @@ -1817,6 +1904,9 @@ func TestPiRPCRejectsUnsafeReviewerSpecs(t *testing.T) { }{ {name: "missing builtin disable", args: removeFlag(args, "--no-builtin-tools")}, {name: "missing context disable", args: removeFlag(args, "--no-context-files")}, + {name: "missing append override", args: removeFlagWithValue(args, "--append-system-prompt")}, + {name: "nonempty append override", args: replaceFlagValue(args, "--append-system-prompt", "ambient instructions")}, + {name: "duplicate append override", args: append(append([]string(nil), args...), "--append-system-prompt", "")}, {name: "missing project approval disable", args: removeFlag(args, "--no-approve")}, {name: "all tools disabled", args: append(removeFlagWithValue(args, "--tools"), "--no-tools")}, {name: "native bash added", args: replaceFlagValue(args, "--tools", piRPCReviewerToolNames+",bash")}, @@ -1906,6 +1996,14 @@ func TestPiRPCHelperProcess(_ *testing.T) { } os.Exit(0) } + if os.Getenv("LLM_PI_RPC_SYNTHETIC_AUTH_REFRESH") == "1" { + // Only this opt-in fixture passes a synthetic selected store and home. + record.Env["PI_CODING_AGENT_DIR"] = os.Getenv("PI_CODING_AGENT_DIR") + record.Env["HOME"] = os.Getenv("HOME") + data, _ := json.Marshal(record) + _ = os.WriteFile(recordPath, data, 0o600) // #nosec G703 -- fixture-owned record path. + _ = os.WriteFile(filepath.Join(record.Env["PI_CODING_AGENT_DIR"], "auth.json"), []byte(`{"fixture":"refreshed synthetic auth state"}`), 0o600) // #nosec G703 -- opt-in synthetic fixture store, never real credentials. + } // Pi RPC stays alive after agent_settled and shuts down when stdin closes. stdinClosed := make(chan struct{}) go func() { @@ -2189,3 +2287,12 @@ func TestPiRPCToolUseKillsProcessGroup(t *testing.T) { return !processExists(pid) }) } + +// Absence is distinct from an explicitly empty append source in Pi. +func assertPiRPCEmptyAppend(t *testing.T, args []string) { + t.Helper() + value, present := flagValueOK(args, "--append-system-prompt") + if !present || value != "" || countPiRPCFlag(args, "--append-system-prompt") != 1 { + t.Fatalf("append system prompt = %q (present %v) in %#v, want exactly one empty source", value, present, args) + } +}