diff --git a/packages/tests/fixtures/inline-scripts/perl-overfull-hbox-read.sh b/packages/tests/fixtures/inline-scripts/perl-overfull-hbox-read.sh new file mode 100644 index 0000000..32cf649 --- /dev/null +++ b/packages/tests/fixtures/inline-scripts/perl-overfull-hbox-read.sh @@ -0,0 +1,27 @@ +perl -e ' +open(F, ") { + chomp; + next unless $_; + my @syns = split(/,\s*/); + @syns = map { s/^\s+|\s+$//g; $_ } @syns; + foreach my $s (@syns) { + $map{lc($s)} = \@syns; + } +} + +open(IN, "; +close(IN); + +pos($orig_text) = 0; +while ($orig_text =~ /\b([a-zA-Z]+)\b/g) { + my $w = $1; + my $lw = lc($w); + if (exists $map{$lw}) { + my $p = pos($orig_text) - length($w); + print "pos $p: $w -> " . join(",", @{$map{$lw}}) . "\n"; + } +} +' \ No newline at end of file diff --git a/packages/tests/fixtures/inline-scripts/perl-overfull-hbox.sh b/packages/tests/fixtures/inline-scripts/perl-overfull-hbox.sh new file mode 100644 index 0000000..f2d2e48 --- /dev/null +++ b/packages/tests/fixtures/inline-scripts/perl-overfull-hbox.sh @@ -0,0 +1,66 @@ +perl -e ' +open(F, ") { + chomp; + next unless $_; + my @syns = split(/,\s*/); + @syns = map { s/^\s+|\s+$//g; $_ } @syns; + foreach my $s (@syns) { + $map{lc($s)} = \@syns; + } +} + +open(IN, "; +close(IN); + +my @tokens; +pos($orig_text) = 0; +while ($orig_text =~ /(\b[a-zA-Z]+\b)/g) { + my $w = $1; + my $start = pos($orig_text) - length($w); + my $lw = lc($w); + if (exists $map{$lw}) { + push @tokens, { + word => $w, + start => $start, + len => length($w), + syns => $map{$lw} + }; + } +} + +sub test_text { + my ($choices) = @_; # array ref of index into syns for each token + my $new_text = $orig_text; + # Apply replacements from end to start to not mess up offsets + for (my $i = $#tokens; $i >= 0; $i--) { + my $t = $tokens[$i]; + my $c = $choices->[$i]; + my $syn = $t->{syns}[$c]; + # Preserve capitalization of original word if possible + if ($t->{word} =~ /^[A-Z]/) { + $syn = ucfirst(lc($syn)); + } else { + $syn = lc($syn); + } + substr($new_text, $t->{start}, $t->{len}, $syn); + } + open(OUT, ">input.tex") or die; + print OUT $new_text; + close(OUT); + + system("pdflatex main.tex > /dev/null 2>&1"); + open(LOG, "; + close(LOG); + my @overfull = ($log =~ /(Overfull \\hbox .*)/g); + return scalar(@overfull); +} + +# Test all zeros (original words) +my @choices = (0) x scalar(@tokens); +print "Original overfull count: " . test_text(\@choices) . "\n"; +' \ No newline at end of file diff --git a/packages/tests/runtime/commandEffects.test.ts b/packages/tests/runtime/commandEffects.test.ts index 55cd418..80bb2d5 100644 --- a/packages/tests/runtime/commandEffects.test.ts +++ b/packages/tests/runtime/commandEffects.test.ts @@ -1,5 +1,13 @@ import { describe, test, expect } from "bun:test"; import { classifyCommand, commandOf } from "../../../runtime/toolEffects"; +import { blockedInPlanMode } from "../../../runtime/planMode"; + +/** A `run_terminal` command captured verbatim from a benchmark trial log. */ +async function trialCommand(name: string): Promise { + return await Bun.file( + new URL(`../fixtures/inline-scripts/${name}.sh`, import.meta.url), + ).text(); +} describe("commands that change files", () => { // Taken verbatim from a benchmark run, where the agent used the file tools @@ -36,6 +44,68 @@ describe("commands that change files", () => { }); }); +/** + * Inline scripts, which plan mode's second gate rests on. + * + * `run_terminal` stays available while planning, so an interpreter invoked with + * `-c`/`-e` is the way a write reaches disk with the writing tools withheld. The + * whole script sits inside one quoted run, so nothing below is visible to the + * segment rules — these patterns are the only thing looking at it. + */ +describe("inline scripts that change files", () => { + test.each([ + // Perl's idiom is a redirect inside the mode string, not a `w`. + ["perl two-arg open for writing", `perl -e 'open(OUT, ">input.tex"); print OUT $t;'`], + ["perl two-arg open for appending", `perl -e 'open(LOG, ">>run.log"); print LOG $t;'`], + ["perl three-arg open", `perl -e 'open(my $fh, ">", $file) or die;'`], + ["ruby File.write", `ruby -e 'File.write("out.txt", data)'`], + // Shelling out builds its argument at runtime, so there is nothing to read. + ["perl system", `perl -e 'system("pdflatex main.tex > /dev/null 2>&1");'`], + ["perl qx", `perl -e 'my $out = qx(make -j4);'`], + ["python subprocess", `python3 -c "import subprocess; subprocess.run(['make'])"`], + ["node child_process", `node -e "require('child_process').execSync('make')"`], + ["python os.system", `python3 -c "import os; os.system('make')"`], + ])("%s writes", (_label, command) => { + expect(classifyCommand(command).writes).toBe(true); + }); + + test.each([ + // The `>` addition must not read a read-mode open as a write. + ["perl open for reading", `perl -e 'open(F, ") { print; }'`], + ["ruby File.read", `ruby -e 'puts File.read("notes.txt")'`], + ["a comparison", `node -e "if (width > 100) console.log('wide')"`], + ["a right shift", `python3 -c "print(value >> 16)"`], + // Backticks are why the shell-out test is a list of named calls rather than + // anything that runs a program: a template literal is not a subshell. + ["a template literal", "node -e 'console.log(`width ${w}`)'"], + ["reading a file", `python3 -c "print(open('notes.txt').read())"`], + // `system` qualified by something other than `os` is usually not a subshell. + ["platform.system", `python3 -c "import platform; print(platform.system())"`], + ["a method named system", `perl -e 'my $rc = $obj->system(1);'`], + // qx/qy/qz/qw are a quaternion's components, so this is division. + ["quaternion arithmetic", `python3 -c "print(qx / qw, qy/n)"`], + ])("%s does not write", (_label, command) => { + expect(classifyCommand(command).writes).toBe(false); + }); + + test("the perl script that got through, verbatim", async () => { + // jobs/tb2-post-1.1/overfull-hbox__Jk3CkEc, call 30 of 58: thirteen scripts + // of this shape rewrote input.tex through `open(OUT, ">input.tex")` and ran + // pdflatex through `system(...)`. The classifier flagged none of them. + const command = await trialCommand("perl-overfull-hbox"); + expect(classifyCommand(command).writes).toBe(true); + expect(blockedInPlanMode("run_terminal", { command })).toBe(true); + }); + + test("its read-only sibling from the same trial still passes", async () => { + // Call 27, three iterations earlier: the same parsing preamble, reading + // both files and printing. Plan mode has to keep letting this through. + const command = await trialCommand("perl-overfull-hbox-read"); + expect(classifyCommand(command).writes).toBe(false); + expect(blockedInPlanMode("run_terminal", { command })).toBe(false); + }); +}); + describe("commands that verify", () => { test.each([ ["bun test", "bun test packages/tests"], diff --git a/runtime/codeEffects.test.ts b/runtime/codeEffects.test.ts index 63b1fe8..7795ffc 100644 --- a/runtime/codeEffects.test.ts +++ b/runtime/codeEffects.test.ts @@ -98,9 +98,35 @@ describe("classifyCode — what interpreter source does", () => { expect(classifyCode(code).writes).toBe(true); }); + test("a bare system() does", () => { + // Bare so that Perl's and Ruby's form is caught on the `run_terminal` + // path, which shares this pattern. + expect(codeShellsOut("system('make')")).toBe(true); + expect(codeShellsOut("my $rc = system('make');")).toBe(true); + }); + test("ordinary source does not", () => { expect(codeShellsOut("total = sum(values)")).toBe(false); }); + + test("a qualified system() does not", () => { + // `platform.system()` names the operating system and reads nothing, and + // it is common enough in inspection code that grading it as a subshell + // would refuse ordinary plan-mode reads. + expect(codeShellsOut("print(platform.system())")).toBe(false); + expect(codeShellsOut("root = filesystem(path)")).toBe(false); + expect(codeShellsOut("$obj->system(1)")).toBe(false); + }); + + test("qx with a bracket delimiter does, with a slash does not", () => { + // `qx`, `qy`, `qz` and `qw` are a quaternion's components, so a slash + // after `qx` is division far more often than it is Perl's backtick + // synonym — and refusing arithmetic is the worse of the two failures. + expect(codeShellsOut("my $out = qx(make -j4);")).toBe(true); + expect(codeShellsOut("my $out = qx{make -j4};")).toBe(true); + expect(codeShellsOut("norm = qx / qw")).toBe(false); + expect(codeShellsOut("x, y = qx/n, qy/n")).toBe(false); + }); }); describe("verifies", () => { diff --git a/runtime/toolEffects.ts b/runtime/toolEffects.ts index fdbdada..a745829 100644 --- a/runtime/toolEffects.ts +++ b/runtime/toolEffects.ts @@ -158,9 +158,15 @@ const INLINE_SCRIPT = /\b(python3?|node|bun|deno|ruby|perl)\b[^|;]*\s-(c|e)\b/; * Checked against the raw command, before quoted runs are stripped — the whole * script lives inside those quotes. Restricted to calls that name a file, so * `process.stdout.write` is not mistaken for one. + * + * The mode string is the signal, and not every language spells it with a letter: + * Perl writes `open(OUT, ">input.tex")` and appends with `">>"`, so `>` sits + * alongside `w` and `a`. It is only read immediately after the opening quote of + * an argument to `open`, which is why `open(F, " 100` are both untouched. */ const INLINE_WRITE = - /(\bopen\s*\([^)]*['"][wa]|writeFileSync|\bwriteFile\s*\(|\bfs\.write|Bun\.write|write_text|shutil\.(copy|move)|os\.(remove|rename|makedirs|mkdir))/; + /(\bopen\s*\([^)]*['"][wa>]|writeFileSync|\bwriteFile\s*\(|\bfs\.write|Bun\.write|write_text|\bFile\.write\b|shutil\.(copy|move)|os\.(remove|rename|makedirs|mkdir))/; export function classifyCommand(command: string): CommandEffect { if (!command.trim()) return { writes: false, verifies: false }; @@ -171,7 +177,17 @@ export function classifyCommand(command: string): CommandEffect { // The benchmark run leaned heavily on inline scripts — 110 node and 106 // python3 invocations — so a file written from inside one is a real edit // path, not an edge case. - if (INLINE_SCRIPT.test(command) && INLINE_WRITE.test(command)) { + // + // Shelling out counts as writing here for the reason `codeShellsOut` gives: + // the argument is built at runtime, so there is nothing to read and + // unrecognised means destructive. `CODE_SUBPROCESS` is the same test the REPL + // path already applies to source, and an inline script is the shorter-lived + // version of a REPL session. Both run against the raw command, before quoted + // runs are stripped, because the script lives inside those quotes. + if ( + INLINE_SCRIPT.test(command) && + (INLINE_WRITE.test(command) || CODE_SUBPROCESS.test(command)) + ) { writes = true; } @@ -250,9 +266,26 @@ export function codeOf(args: Record): string { * - Does it shell out? A `subprocess.run`, `os.system` or `execSync` can run * anything at all, and the argument is usually built at runtime, so there is * nothing here to read. Unrecognised means destructive, as everywhere else. + * + * Perl and Ruby spell it bare, so `system(` is matched unqualified — but only + * where nothing precedes it, because a qualified one is usually something else + * entirely: `platform.system()` names the operating system and reads nothing, + * and `$obj->system(1)` is a method that happens to share the name. `os.system` + * is therefore listed by name rather than reached by the bare rule. + * + * Two spellings that do run a program are deliberately absent, because their + * delimiters are ambiguous with arithmetic in the languages that share this + * pattern, and refusing an ordinary plan-mode read is the worse failure: + * + * - A backtick, which is a template literal in JavaScript: + * `node -e 'console.log(`w ${x}`)'`. + * - `qx` with a slash delimiter, because `qx`, `qy`, `qz` and `qw` are the + * standard names for a quaternion's components, so `norm = qx / qw` and + * `qx/n, qy/n` are division. Only `qx(` and `qx{` are matched, which no + * arithmetic produces. */ const CODE_SUBPROCESS = - /(\bsubprocess\b|\bos\.system\s*\(|\bos\.popen\s*\(|\bchild_process\b|\bexecSync\s*\(|\bspawnSync\s*\(|Bun\.\$)/; + /(\bsubprocess\b|\bos\.system\s*\(|(?])system\s*\(|\bos\.popen\s*\(|\bqx\s*[({]|\bchild_process\b|\bexecSync\s*\(|\bspawnSync\s*\(|Bun\.\$)/; /** * Source that checks something works.