Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 \
Expand Down
34 changes: 24 additions & 10 deletions docs/development.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
46 changes: 29 additions & 17 deletions internal/llmadapters/pi_rpc.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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",
Expand All @@ -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)
Expand All @@ -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
Expand All @@ -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") {
Expand Down
48 changes: 47 additions & 1 deletion internal/llmadapters/pi_rpc_runtime_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand Down Expand Up @@ -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",
Expand All @@ -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")
Expand Down
Loading
Loading