5518890b18
* audit learnings: reshape reflection prompt + bake tool quirks into descriptions (#619) Cross-repo audit of the 48 repos with non-null learnings turned up two recurring failure modes: 1. ~25-30% of bullets across the most-active repos are pullfrog-tool quirks ("shell timeout is in milliseconds", "git args must be a JSON array", "create_pull_request_review drops out-of-hunk comments", "push_branch may report timeout when push succeeded", "checkout_pr shallow.lock retries", "commit_id needs full 40-char SHA"). These are universal across repos and should live in tool descriptions, not be rediscovered and stored 48 times. Tool descriptions now surface them. 2. Bullets are routinely 200-1000 chars (paragraph-length), and 12 of 48 repos are at the 10k cap. The reflection prompt now: caps bullets at ~240 chars (one specific fact), bans PR/review/commit/date-anchored facts that decay within weeks, bans tool-quirk learnings, and tells the agent that cap pressure means compress+prune existing bullets, not skip new findings. Co-authored-by: Cursor <cursoragent@cursor.com> * learnings: add server-generated TOC, fixed section taxonomy, raise cap to 100k (#707) Cap goes 10k → 100k. Reads stay bounded because the seeded file now opens with a server-generated table of contents listing every `## ` section's line range — agents read the TOC, then `read_file offset/limit` just the sections relevant to the current task instead of slurping the whole file. ## Section taxonomy (fixed) `## Build & test`, `## CI`, `## Conventions`, `## Architecture`, `## Gotchas`. Free-form `### ` sub-headings inside a section are fine. Pre-taxonomy free-text rows get wrapped in a `## Legacy` carve-out on first seed so they remain visible while the agent gradually re-curates them during reflection turns. ## Storage shape unchanged `Repo.learnings` still holds raw markdown (no schema migration). The TOC is a pure read-side affordance: prepended at seed time, stripped from the agent-edited file before persist. Markers `<!-- pullfrog-learnings-toc:* -->` delimit the strip region. Agent edits inside the markers are discarded. ## Round-trip semantics `seedLearningsFile` now returns `{ path, canonicalSeed }` where `canonicalSeed` is the post-TOC body — same shape `readLearningsFile` returns at end-of-run, so `persistLearnings` byte-compares them directly to skip the no-op PATCH. Empty-repo first runs end up with the section scaffold both as seed and as read-back, so untouched runs still short-circuit cleanly. ## Reflection prompt Adds explicit section-placement guidance (place each new bullet under the most relevant `## `; do NOT add new top-level headings; do NOT edit anything between the TOC markers). Carries forward the bullet hygiene from the previous commit: ≤240 chars per bullet, no pullfrog-tool quirks (those belong in tool descriptions), no PR/review/commit/date references. The "near cap" framing is replaced with "compress and prune within a section when it grows noisy" since the cap pressure that drove cramming is gone. Co-authored-by: Cursor <cursoragent@cursor.com> * anneal round 1: line-anchored taxonomy detect, partial-merge, line-boundary truncation, scaffold-empty UI Multi-lens review of the TOC + taxonomy diff surfaced a cluster of correctness and operational bugs. Fixes: - `hasAnyTaxonomyHeading` used `String.includes("## X")` which false-positives on `### X` (the `## ` substring sits inside `### `), prose containing `## CI`, fenced code documenting markdown, etc. Replaced with a line-anchored predicate that reuses `parseHeadings` so detection and TOC construction stay consistent. - The "any heading present → pass through verbatim" rule meant a body with one taxonomy heading would seed without the other four. Worse, requiring all five would flip a body back into Legacy when the agent legitimately pruned a section to empty. New `partial` kind: keep existing content in place, append missing sections in canonical order so the agent always has the full scaffold without losing pruning intent. - `stripLearningsToc` collapsed `\n{3,}` globally; `canonicalSeed` doesn't, so an untouched body with intentional triple-newline spacing would compare unequal and burn a spurious LearningsRevision row each run. Drop the global collapse — only the leading newlines that the strip itself introduces are normalized. - 100k truncation via `slice(0, 100_000)` could cut mid-line, breaking `parseHeadings` (whole-line `^## `) on the next seed and flipping a cut body back into Legacy. New `truncateAtLineBoundary` cuts at the last newline before the cap. - `LearningsSection.tsx` rendered a scaffold-only body as "has learnings" instead of the empty placeholder. Added a `hasOnlyEmptyScaffold` guard so the console behaves the same as pre-PR for the empty case. - Seed log line distinguishes `kind=structured/partial/legacy-wrapped/ empty` instead of `existing=yes/no`, so operators can spot legacy migration activity in logs. - New tests cover: substring false-positive (`### Build & test`, in-prose mentions), partial-taxonomy merge (no Legacy wrap), full-taxonomy structured pass-through, last-newline truncation, triple-newline preservation. Deferred (documented in PR body): deploy-ordering footgun (action before API), rollback for rows >10k, Gemini sanitizer dropping `description` on `anyOf` branches, reflection-on-failed-runs. Co-authored-by: Cursor <cursoragent@cursor.com> * anneal r2: hard-truncate fallback when line boundary discards >4k Round-2 review caught a regression in `truncateAtLineBoundary`: when the only newline within the first 100k chars sits near the start (e.g. one heading + 100k+ char single line — pathological pasted log dumps), the line-boundary cut discards almost all of the body. losing one partial line is preferable to losing kilobytes; threshold the fallback at 4k. Co-authored-by: Cursor <cursoragent@cursor.com> * move TOC out of file: prompt-side rendering, server-parsed headings drops the in-file TOC + fixed taxonomy in favor of: - file on disk = verbatim Repo.learnings (no markers, no scaffold) - server parses headings (mdast-util-from-markdown) at run-context time and returns them as RepoSettings.learningsHeadings - action renders heading TOC into the LEARNINGS prompt section as parenthesized line ranges like `Build & test (L1-L42)` with hierarchy via 2-space indent off the shallowest depth - reflection prompt teaches agent-curated structure with a soft 300-line per-section cap and explicit guidance to restructure flat legacy lists cuts 8 helpers (ensureSections, stripLearningsToc, assembleFile, buildTocBlock, parseHeadings, buildSectionScaffold, hasAnyTaxonomyHeading, LEARNINGS_SECTIONS) and the canonicalSeed round-trip dance. action seedLearningsFile is now { path } only; main.ts byte-compares the trimmed read-back against (current ?? "").trim() to gate the persist PATCH. truncateAtLineBoundary kept for safety. new tests: - test/learningsToc.test.ts (11 parser cases incl. fenced-code, blockquote, arbitrary h1-h6 nesting, startLine-points-at-heading invariant) - action/utils/learningsTocRender.test.ts (7 renderer cases) --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Colin McDonnell <colinmcd94@gmail.com>
354 lines
11 KiB
TypeScript
354 lines
11 KiB
TypeScript
// changes to shell security (filterEnv, spawnShell) should be reflected in wiki/security.md and docs/security.mdx
|
|
import { type ChildProcess, type StdioOptions, spawn, spawnSync } from "node:child_process";
|
|
import { randomUUID } from "node:crypto";
|
|
import { closeSync, openSync, writeFileSync } from "node:fs";
|
|
import { userInfo } from "node:os";
|
|
import { join } from "node:path";
|
|
import { setTimeout as sleep } from "node:timers/promises";
|
|
import { type } from "arktype";
|
|
import { ensureBrowserDaemon } from "../utils/browser.ts";
|
|
import { log } from "../utils/log.ts";
|
|
import { resolveEnv } from "../utils/secrets.ts";
|
|
import type { ToolContext } from "./server.ts";
|
|
import { execute, tool } from "./shared.ts";
|
|
|
|
export const ShellParams = type({
|
|
command: "string",
|
|
description: "string",
|
|
"timeout?": type.number.describe(
|
|
"Timeout in MILLISECONDS (not seconds). Default 30000 (30s), max 120000 (2m). e.g. timeout: 180000 for 3 minutes; timeout: 180 means 180ms and will kill the process almost immediately."
|
|
),
|
|
"working_directory?": "string",
|
|
"background?": "boolean",
|
|
});
|
|
|
|
type SpawnParams = {
|
|
command: string;
|
|
env: Record<string, string | undefined>;
|
|
cwd: string;
|
|
stdio: StdioOptions;
|
|
};
|
|
|
|
export type SandboxMethod = "unshare" | "sudo-unshare" | "none";
|
|
|
|
/** cached result of sandbox capability check */
|
|
let detectedSandboxMethod: SandboxMethod | undefined;
|
|
|
|
/** get the current sandbox method (for testing/diagnostics) */
|
|
export function getSandboxMethod(): SandboxMethod {
|
|
return detectSandboxMethod();
|
|
}
|
|
|
|
/** detect which sandbox method is available on this system */
|
|
function detectSandboxMethod(): SandboxMethod {
|
|
if (detectedSandboxMethod !== undefined) {
|
|
return detectedSandboxMethod;
|
|
}
|
|
|
|
// only attempt in CI environments - sandbox has overhead and is primarily for untrusted code
|
|
if (process.env.CI !== "true") {
|
|
detectedSandboxMethod = "none";
|
|
log.debug("sandbox disabled (CI !== true)");
|
|
return "none";
|
|
}
|
|
|
|
// try unprivileged unshare first (works on some systems)
|
|
try {
|
|
const result = spawnSync("unshare", ["--pid", "--fork", "--mount-proc", "true"], {
|
|
timeout: 5000,
|
|
stdio: "ignore",
|
|
});
|
|
if (result.status === 0) {
|
|
detectedSandboxMethod = "unshare";
|
|
log.debug("PID namespace isolation enabled (unprivileged unshare)");
|
|
return "unshare";
|
|
}
|
|
} catch {
|
|
// continue to try sudo
|
|
}
|
|
|
|
// sudo unshare (works on GHA runners)
|
|
try {
|
|
const result = spawnSync("sudo", ["unshare", "--pid", "--fork", "--mount-proc", "true"], {
|
|
timeout: 5000,
|
|
stdio: "ignore",
|
|
});
|
|
if (result.status === 0) {
|
|
detectedSandboxMethod = "sudo-unshare";
|
|
log.debug("PID namespace isolation enabled (sudo unshare)");
|
|
return "sudo-unshare";
|
|
}
|
|
} catch {
|
|
// no sandbox available
|
|
}
|
|
|
|
detectedSandboxMethod = "none";
|
|
log.info("PID namespace isolation not available");
|
|
return "none";
|
|
}
|
|
|
|
// strip inherited proc mount that sits underneath --mount-proc's overlay.
|
|
// --mount-proc mounts fresh proc on top, but `umount /proc` peels it off and exposes the
|
|
// host's proc with all host PIDs — allowing /proc/<pid>/environ exfiltration.
|
|
// double-umount removes both layers, then a clean mount gives only sandbox PIDs.
|
|
// on unprivileged systems where umount fails, --mount-proc still provides isolation
|
|
// (the agent also can't umount in that case).
|
|
const PROC_CLEANUP =
|
|
"umount /proc 2>/dev/null; umount /proc 2>/dev/null; mount -t proc proc /proc 2>/dev/null;";
|
|
|
|
function spawnShell(params: SpawnParams): ChildProcess {
|
|
const spawnOpts = { env: params.env, cwd: params.cwd, stdio: params.stdio, detached: true };
|
|
const sandboxMethod = detectSandboxMethod();
|
|
const ci = process.env.CI === "true";
|
|
|
|
if (ci && sandboxMethod === "none") {
|
|
throw new Error(
|
|
"pid namespace isolation is required in CI but unavailable (both unshare and sudo unshare failed)"
|
|
);
|
|
}
|
|
|
|
if (sandboxMethod === "unshare") {
|
|
return spawn(
|
|
"unshare",
|
|
["--pid", "--fork", "--mount-proc", "bash", "-c", `${PROC_CLEANUP} ${params.command}`],
|
|
spawnOpts
|
|
);
|
|
}
|
|
|
|
if (sandboxMethod === "sudo-unshare") {
|
|
const envArgs: string[] = [];
|
|
for (const [k, v] of Object.entries(params.env)) {
|
|
if (v !== undefined) {
|
|
envArgs.push(`${k}=${v}`);
|
|
}
|
|
}
|
|
// drop back to original user after PROC_CLEANUP so files aren't owned by root.
|
|
// sudo is only needed for unshare; the actual command should run as the normal user
|
|
// to avoid ownership mismatches with files created by the Node.js parent process.
|
|
const username = userInfo().username;
|
|
// su -p resets PATH on many Linux systems (ALWAYS_SET_PATH in /etc/login.defs).
|
|
// restore it from the SANDBOX_PATH env var that survives the su transition.
|
|
// biome-ignore lint/suspicious/noTemplateCurlyInString: we need to restore the PATH variable
|
|
const pathRestore = 'export PATH="${SANDBOX_PATH:-$PATH}"; ';
|
|
const escaped = (pathRestore + params.command).replace(/'/g, "'\\''");
|
|
envArgs.push(`SANDBOX_PATH=${params.env.PATH ?? ""}`);
|
|
return spawn(
|
|
"sudo",
|
|
[
|
|
"env",
|
|
...envArgs,
|
|
"unshare",
|
|
"--pid",
|
|
"--fork",
|
|
"--mount-proc",
|
|
"bash",
|
|
"-c",
|
|
`${PROC_CLEANUP} exec su -p -s /bin/bash ${username} -c '${escaped}'`,
|
|
],
|
|
{ ...spawnOpts, env: {} }
|
|
);
|
|
}
|
|
|
|
return spawn("bash", ["-c", params.command], spawnOpts);
|
|
}
|
|
|
|
/** kill process and its entire process group */
|
|
async function killProcessGroup(proc: ChildProcess): Promise<void> {
|
|
if (!proc.pid) return;
|
|
try {
|
|
process.kill(-proc.pid, "SIGTERM");
|
|
await new Promise((r) => setTimeout(r, 200));
|
|
process.kill(-proc.pid, "SIGKILL");
|
|
} catch {
|
|
try {
|
|
proc.kill("SIGKILL");
|
|
} catch {
|
|
/* already dead */
|
|
}
|
|
}
|
|
}
|
|
|
|
function getTempDir(): string {
|
|
const tempDir = process.env.PULLFROG_TEMP_DIR;
|
|
if (!tempDir) {
|
|
throw new Error("PULLFROG_TEMP_DIR not set");
|
|
}
|
|
return tempDir;
|
|
}
|
|
|
|
/** detect git as a command invocation (not as part of another word like .gitignore) */
|
|
function isGitCommand(command: string): boolean {
|
|
const trimmed = command.trim();
|
|
if (trimmed === "git" || trimmed.startsWith("git ")) return true;
|
|
if (trimmed.startsWith("sudo git")) return true;
|
|
return /[;&|]\s*(?:sudo\s+)?git(?:\s|$)/.test(trimmed);
|
|
}
|
|
|
|
export function ShellTool(ctx: ToolContext) {
|
|
return tool({
|
|
name: "shell",
|
|
description: `Execute shell commands securely. Environment is filtered to remove API keys and secrets.
|
|
|
|
Use this tool to:
|
|
- Run shell commands (ls, cat, grep, find, etc.)
|
|
- Execute build tools (npm, pnpm, cargo, make, etc.)
|
|
- Run tests and linters
|
|
|
|
Do NOT use this tool for git commands — use the dedicated git tools instead.`,
|
|
parameters: ShellParams,
|
|
execute: execute(async (params) => {
|
|
if (isGitCommand(params.command)) {
|
|
throw new Error(
|
|
"git commands are not allowed in the shell tool. use the dedicated git tools instead:\n" +
|
|
"- git: local operations (status, log, diff, add, commit, checkout, merge, rebase, etc.)\n" +
|
|
"- push_branch: push to remote (handles authentication)\n" +
|
|
"- git_fetch: fetch from remote (handles authentication)\n" +
|
|
"- checkout_pr: check out PR branches"
|
|
);
|
|
}
|
|
|
|
const timeout = Math.min(params.timeout ?? 30000, 120000);
|
|
const cwd = params.working_directory ?? process.cwd();
|
|
const env = resolveEnv(ctx.payload.shell === "enabled" ? "inherit" : "restricted");
|
|
|
|
if (params.command.includes("agent-browser")) {
|
|
const daemonError = ensureBrowserDaemon(ctx.toolState);
|
|
if (daemonError) {
|
|
return {
|
|
output: `browser daemon unavailable: ${daemonError}`,
|
|
exit_code: 1,
|
|
timed_out: false,
|
|
};
|
|
}
|
|
const binDir = ctx.toolState.browserDaemon?.binDir;
|
|
if (binDir) {
|
|
env.PATH = `${binDir}:${env.PATH ?? ""}`;
|
|
}
|
|
}
|
|
|
|
if (params.background) {
|
|
const tempDir = getTempDir();
|
|
const handle = `bg-${randomUUID().slice(0, 8)}`;
|
|
const outputPath = join(tempDir, `${handle}.log`);
|
|
const pidPath = join(tempDir, `${handle}.pid`);
|
|
const logFd = openSync(outputPath, "a");
|
|
let proc: ChildProcess;
|
|
try {
|
|
proc = spawnShell({
|
|
command: params.command,
|
|
env,
|
|
cwd,
|
|
stdio: ["ignore", logFd, logFd],
|
|
});
|
|
} finally {
|
|
closeSync(logFd);
|
|
}
|
|
if (!proc.pid) {
|
|
throw new Error("failed to start background process");
|
|
}
|
|
proc.unref();
|
|
writeFileSync(pidPath, `${proc.pid}\n`);
|
|
ctx.toolState.backgroundProcesses.set(handle, { pid: proc.pid, outputPath, pidPath });
|
|
return {
|
|
handle,
|
|
outputPath,
|
|
pidPath,
|
|
message: `started background process ${handle} (pid ${proc.pid})`,
|
|
};
|
|
}
|
|
|
|
const proc = spawnShell({
|
|
command: params.command,
|
|
env,
|
|
cwd,
|
|
stdio: ["ignore", "pipe", "pipe"],
|
|
});
|
|
|
|
let stdout = "",
|
|
stderr = "",
|
|
timedOut = false,
|
|
exited = false;
|
|
proc.stdout?.on("data", (chunk: Buffer) => {
|
|
stdout += chunk.toString();
|
|
});
|
|
proc.stderr?.on("data", (chunk: Buffer) => {
|
|
stderr += chunk.toString();
|
|
});
|
|
|
|
const timeoutId = setTimeout(async () => {
|
|
if (!exited) {
|
|
timedOut = true;
|
|
await killProcessGroup(proc);
|
|
}
|
|
}, timeout);
|
|
|
|
const exitCode = await new Promise<number | null>((resolve) => {
|
|
const done = (code: number | null) => {
|
|
exited = true;
|
|
clearTimeout(timeoutId);
|
|
resolve(code);
|
|
};
|
|
proc.on("exit", done);
|
|
proc.on("error", () => done(null));
|
|
});
|
|
|
|
let output = stderr ? (stdout ? `${stdout}\n${stderr}` : stderr) : stdout;
|
|
if (timedOut)
|
|
output = output
|
|
? `${output}\n[timed out after ${timeout}ms]`
|
|
: `[timed out after ${timeout}ms]`;
|
|
|
|
const finalExitCode = exitCode ?? (timedOut ? 124 : -1);
|
|
if (finalExitCode !== 0) {
|
|
log.info(`shell command failed with exit code ${finalExitCode}: ${params.command}`);
|
|
if (output) log.info(`output: ${output.trim()}`);
|
|
}
|
|
|
|
return {
|
|
output: output.trim(),
|
|
exit_code: finalExitCode,
|
|
timed_out: timedOut,
|
|
};
|
|
}),
|
|
});
|
|
}
|
|
|
|
export const KillBackgroundParams = type({
|
|
handle: type.string.describe("The handle of the background process to kill (e.g., bg-a1b2c3d4)"),
|
|
});
|
|
|
|
export function KillBackgroundTool(ctx: ToolContext) {
|
|
return tool({
|
|
name: "kill_background",
|
|
description: `Kill a background process by its handle. Use this to stop dev servers or other long-running processes started with shell({ background: true }).`,
|
|
parameters: KillBackgroundParams,
|
|
execute: execute(async (params) => {
|
|
const proc = ctx.toolState.backgroundProcesses.get(params.handle);
|
|
if (!proc) {
|
|
return {
|
|
success: false,
|
|
message: `no background process with handle ${params.handle}`,
|
|
};
|
|
}
|
|
|
|
try {
|
|
process.kill(-proc.pid, "SIGTERM");
|
|
} catch {
|
|
// already dead
|
|
}
|
|
await sleep(200);
|
|
try {
|
|
process.kill(-proc.pid, "SIGKILL");
|
|
} catch {
|
|
// already dead
|
|
}
|
|
|
|
ctx.toolState.backgroundProcesses.delete(params.handle);
|
|
return {
|
|
success: true,
|
|
message: `killed background process ${params.handle} (pid ${proc.pid})`,
|
|
};
|
|
}),
|
|
});
|
|
}
|