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
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ Start the background process, continue launching the other lanes, then drain the

The runner and its preflight have no implicit timeout. Do not invent a duration from role, mode, or a convenient round number; real implementation lanes can run for 90 minutes or much longer. Pass `--timeout` only when the user, an external service deadline, or a measured task contract supplies a real bound. That value starts at wrapper entry, before module loading and argument parsing, and remains one absolute deadline across setup, preflight, model execution, and output capture. It is never a fresh allowance per child, and long waits are armed in runtime-safe chunks without shortening the supplied deadline. Otherwise supervise liveness through the retained background task/session handle and cancel manually only on evidence that the run is dead. Cancel through that retained handle so the runner receives SIGINT or SIGTERM, sends it to an active child when one remains, stops waiting on inherited output pipes, removes the empty output reservation, and writes a `cancelled` receipt. Preserve that receipt; a retry is a new attempt with new unique output and receipt paths. Unchanged running state is not a dropout, and Claude's ten-minute foreground ceiling is never a reason to terminate a healthy lane.

Read-only mode maps to Claude plan mode with project-only settings and an explicit tool list, Codex's read-only sandbox, and Grok plan mode plus its `read-only` sandbox and read-oriented tool list. Grok's built-in read-only profile deliberately keeps its own state and system temporary directories writable, so point a read-only Grok lane at the actual checkout rather than a worktree under `/tmp`, `/var/tmp`, or the host's temporary directory. `isolated-write` maps to Claude `acceptEdits` with project-only settings, Codex `workspace-write`, and Grok `acceptEdits` plus its `workspace` sandbox and write-capable tool list. Give every writer only a dedicated worktree or output directory. Never route a writer into the primary checkout.
Read-only mode maps to Claude plan mode with project-only settings and an explicit tool list, Codex's read-only sandbox, and Grok auto mode plus its `read-only` sandbox and read-oriented tool list. Headless Grok cancels the whole turn on a permission prompt, which its `plan` mode raises for any shell command outside its built-in read-only list; auto mode reports a blocked call to the model instead. Grok's built-in read-only profile deliberately keeps its own state and system temporary directories writable, so point a read-only Grok lane at the actual checkout rather than a worktree under `/tmp`, `/var/tmp`, or the host's temporary directory. `isolated-write` maps to Claude `acceptEdits` with project-only settings, Codex `workspace-write`, and Grok auto mode plus its `workspace` sandbox and write-capable tool list. Grok uses auto mode in both access modes because `acceptEdits` raises the same turn-cancelling prompt for a shell command outside Grok's built-in list. Give every writer only a dedicated worktree or output directory. Never route a writer into the primary checkout.

Every concurrent external lane needs distinct prompt, output, and receipt paths. The launcher reserves output and receipt paths exclusively and refuses to overwrite them.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ describe("invocationCommand", () => {
"--reasoning-effort",
"xhigh",
"--permission-mode",
"plan",
"auto",
"--sandbox",
"read-only",
"--tools",
Expand Down Expand Up @@ -124,7 +124,7 @@ describe("invocationCommand", () => {
expect(grok.args).toEqual(
expect.arrayContaining([
"--permission-mode",
"acceptEdits",
"auto",
"--sandbox",
"workspace",
"--tools",
Expand Down
5 changes: 4 additions & 1 deletion plugins/pstack/skills/poteto-mode/scripts/runner/commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -128,8 +128,11 @@ export function invocationCommand(options: RunnerOptions): CommandSpec {
options.model,
"--reasoning-effort",
options.effort,
// Headless Grok cancels the whole turn on a permission prompt, in both access modes.
// Auto mode reports a blocked call to the model instead; the sandbox still confines
// writes (read-only, or workspace for isolated-write).
"--permission-mode",
permissionMode(options.mode),
"auto",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium · Exact candidate cb0b479 lacks mandated live test

AGENTS.md bars merge until the exact candidate is installed and its changed behavior passes a live run from the real user surface in every affected harness. The installed version, surface, action and result must be recorded. The PR says evidence for cb0b479 is still in progress, all three live boxes are unchecked, and the cited runs are prior heads 469ea9e and 2a731d2. This head's Grok auto invocation (line 135) and tail evidence (run.ts:847) have no recorded Claude Code or Codex run. The carried Claude-writer form of this demand is moot because the Claude args now match main, but the gate itself is still unmet. Also reported for this defect: - Exact-candidate live validation remains missing (plugins/pstack/skills/poteto-mode/scripts/runner/commands.ts:135): AGENTS.md requires installation and successful real-user-surface testing of the exact candidate in every affected harness, with version, surface, action, and observed result recorded before merge. The supplied PR metadata explicitly marks cb0b479's live testing as in progress and leaves all three evidence boxes unchecked. Earlier-head Grok runs and passing static checks do not satisfy that gate. This candidate must remain a draft until its changed invocation and receipt behavior have the required Claude Code and Codex evidence. - Exact Grok invocation lacks required live validation (plugins/pstack/skills/poteto-mode/scripts/runner/commands.ts:135): This changes Grok lanes to auto permission mode, but the supplied PR evidence leaves the exact-candidate installation and real-user-surface checks incomplete. Merging this candidate would violate the repository's mandatory delivery gate and could ship a Grok lane that cancels or drops out in an affected harness despite fixture tests passing.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Critical · Grok auto mode runs untrusted repo code unapproved

Line 135 replaces plan (read-only) and acceptEdits (isolated-write) with auto for Grok lanes. In the base, a shell call outside Grok's built-in read-only list raised a prompt, and headless Grok turned that into a cancelled turn. Repository scripts therefore never executed. Auto mode now approves ordinary commands; the prior live run showed a python3 write executing. A read-only reviewer lane on an untrusted PR that runs bun test would execute that PR's test code. Grok's sandbox is the only visible replacement for the removed gate. provider-dispatch.md:83 says Grok's read-only profile keeps system temp dirs and Grok's own state writable, and the runner sets no network restriction. The executed code can overwrite concurrent lanes' prompts, outputs or receipts under the temp dir and tamper with Grok state. If Grok's sandbox permits egress, it can also exfiltrate readable credentials. Grok's egress policy is outside the visible code and unverified.

"--sandbox",
grokSandbox(options.mode),
"--tools",
Expand Down
21 changes: 21 additions & 0 deletions plugins/pstack/skills/poteto-mode/scripts/runner/run.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,9 @@ if (name === "claude") {
console.log(JSON.stringify({type:"thread.started",thread_id:"o1"}));
console.log(JSON.stringify({type:"item.completed",item:{type:"agent_message",text:"CODEX_OK"}}));
console.log(JSON.stringify({type:"turn.completed",usage:{input_tokens:20,cached_input_tokens:5,output_tokens:3,reasoning_output_tokens:1}}));
} else if (process.env.FAKE_GROK_ERROR_RESULT === "1") {
console.log(JSON.stringify({type:"system",subtype:"init",skills:Array(500).fill("skill-name")}));
console.log(JSON.stringify({type:"result",subtype:"error_during_execution",is_error:true,stop_reason:"cancelled",errors:["cancelled"]}));
} else {
console.log(JSON.stringify({type:"assistant",message:{content:[{type:"text",text:"progress"}]}}));
console.log(JSON.stringify({type:"result",subtype:"success",is_error:false,result:"GROK_OK",session_id:"g1",usage:{input_tokens:30,output_tokens:4,total_tokens:34},total_cost_usd:0.02,modelUsage:{[model + "-build"]:{}}}));
Expand Down Expand Up @@ -244,6 +247,7 @@ beforeEach(() => {
delete process.env.FAKE_DESCENDANT_HOLDS_PIPES_MS;
delete process.env.FAKE_DESCENDANT_PID_PATH;
delete process.env.FAKE_SELF_SIGNAL;
delete process.env.FAKE_GROK_ERROR_RESULT;
});

afterEach(() => {
Expand All @@ -268,6 +272,7 @@ afterEach(() => {
delete process.env.FAKE_DESCENDANT_HOLDS_PIPES_MS;
delete process.env.FAKE_DESCENDANT_PID_PATH;
delete process.env.FAKE_SELF_SIGNAL;
delete process.env.FAKE_GROK_ERROR_RESULT;
rmSync(scratch, { recursive: true, force: true });
});

Expand Down Expand Up @@ -307,6 +312,22 @@ describe("runLane", () => {
});
});

it("keeps Grok's terminal error result in malformed-output evidence", async () => {
process.env.FAKE_GROK_ERROR_RESULT = "1";
const input = options("grok", "grok-error-result");
const result = await runLane(input);
expect(result.exitCode).toBe(65);
expect(existsSync(input.outputPath)).toBe(false);
const recorded = receipt(input.receiptPath);
expect(recorded).toMatchObject({
status: "malformed-output",
exitCode: 0,
error: { message: "grok reported an error result" },
});
expect(recorded.error?.evidence).toContain('"stop_reason":"cancelled","errors":["cancelled"]');
expect(recorded.error?.evidence.length).toBeLessThanOrEqual(4_000);
});

it("classifies an unavailable model without falling back", async () => {
process.env.FAKE_INVALID_MODEL = "1";
const input = options("codex");
Expand Down
7 changes: 6 additions & 1 deletion plugins/pstack/skills/poteto-mode/scripts/runner/run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,11 @@ function evidence(value: string): string {
return value.trim().slice(0, ERROR_EVIDENCE_LIMIT);
}

// A provider's terminal event ends its stream, so malformed output keeps the tail.
function trailingEvidence(value: string): string {
return value.trim().slice(-ERROR_EVIDENCE_LIMIT);
}

function removeIfExists(path: string): void {
if (existsSync(path)) unlinkSync(path);
}
Expand Down Expand Up @@ -839,7 +844,7 @@ async function executeLane(
costUsd: null,
error: {
message,
evidence: evidence(`${result.stderr}\n${result.stdout}`),
evidence: trailingEvidence(`${result.stderr}\n${result.stdout}`),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Stderr disappears from receipts

When a provider exits successfully but sends malformed output, a long stdout stream pushes all of stderr out of the 4,000-character receipt evidence. If stderr holds the useful error details, the receipt loses them. Keep space for stderr as well as the end of stdout.

},
});
}
Expand Down
Loading