fix(console): a path with a space is one argument, not two - #363
Conversation
`kuna decompile` is the only surface that reaches the engine through a console script rather than an in-process call, and the console read every path with `s >> filename` -- whitespace-delimited, no quote rule. An unquoted path containing a space therefore split into two arguments: - `load file /home/u/test dir/a.out` took `/home/u/test` as a BFD target and loaded `dir/a.out`, failing with "Unable to recognize imagefile dir/a.out". With an explicit `--target` the tail was dropped entirely. - `openfile write` truncated the redirect at the split. The temp file lives in `std::env::temp_dir()`, which on Windows is `C:\Users\First Last\AppData\ Local\Temp` by default, so this half bit every binary on such an account -- and `sync_redirect_file` discarded the open/write error, so the C was silently written to a file named after the first path component, truncating whatever was already there. Quoting at the caller could not fix this: a reader with no quote rule takes the quote as part of the token. So the grammar learns one -- `CommandStream:: read_filename` accepts an optional double-quoted argument (`\"` and `\\` are escapes inside quotes; any other backslash stays literal, so a Windows path survives either spelling) and is byte-identical to `read_token` for unquoted input. It is wired into the commands that actually read a path: `load file`, `openfile write`, `openfile append`, `parse file`. The producers quote conditionally -- only a path containing whitespace or a quote -- so every script that works today stays byte-identical, including for an older `decomp_dbg` reached through `--decomp-dbg`. `scripts/decompile.py` carries the same fix. `sync_redirect_file` now reports a failed open or write on stderr instead of discarding it; that silence is what turned a mis-parsed path into silent data loss. Not gated behind an option: this cannot change emitted C for any input that works today (unquoted paths parse exactly as before), so it is a strict bug fix per AGENTS.md. Out of scope, noted for a follow-up: `load function <name>`, `option <name> <value>` and `kassert <args>` are interpolated into the same script unquoted and would split the same way on a value containing a space. Tests: 5 grammar unit tests (`read_filename`), 5 CLI script unit tests (`console_path`/`build_script`), and 2 real-engine end-to-end tests -- a binary under a spaced directory, and a spaced temp dir including an assertion that nothing was written to the truncation target. Both e2e tests were verified to fail without the fix. Four gates green: 675/675, stages 574/574, workspace suite, check-spec. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jp9d9GVacWoV338MB6Tysf
…annot carry
Review follow-up on the previous commit. Two defects, both in code that commit
added, plus the idiom pass they came out of.
**The redirect diagnostic repeated.** `sync_redirect_file` runs after EVERY
command while a redirect is open (`decomp_dbg.rs` run_console), so a target that
could not be written printed its error once per command until `closefile` -- and
the CLI truncates its transcript excerpt at 2000 chars, so the repeats ate the
budget that should carry the real reason. Reported once per target now, cleared
when the target changes or a write succeeds. Measured against a failing
redirect: 2 lines before (more with `--regions`), 1 after.
**A newline is the one whitespace character quoting cannot rescue.** The script
reaches `decomp_dbg` as lines, so an embedded `\n` ends the command however it is
quoted: the console saw an unterminated quote, took the head as the filename, and
answered `Unable to recognize imagefile <head>` -- which reads like a defect in
the binary rather than an impossible path. Not a regression (it failed the same
way before this branch), but it is the last hole in the family the branch closes,
so it is diagnosed up front for the binary path and for the temp paths, whose
directory is the caller's `TMPDIR`:
error: binary path contains a newline, which the decomp_dbg console script
(one command per line) cannot carry: "/tmp/nl\ndir/a.out"
**The producer and the consumer are now pinned to each other.** `console_path`
(kuna-cli) and `read_filename` (kuna-console) are an escaping pair in different
crates and nothing held them together; the crates are already linked, so the
round trip is now asserted in-process -- leading and trailing spaces, tabs,
doubled and undoubled Windows separators, an embedded quote -- plus the same
assertion against a whole generated `load file` line, including that the path
exhausts the line rather than leaving a second argument.
**Idiom.** `console_path` returned an unconditional `String`, allocating even
when handing back the input untouched, which is the overwhelmingly common case
given that quoting is deliberately conditional. It returns `Cow<'_, str>` now,
the shape `kuna_symbolnamebound::bound_scope_path` already uses for exactly this
("a no-op, borrowed and unallocated"). The saved allocation is not the point --
three per run -- the type stating the design is: `Cow::Borrowed` means this
script line is byte-identical to what kuna emitted before the fix, which is the
compatibility claim the whole conditional rests on. A test pins it as a contract
rather than an accident. Also: `read_filename` pre-allocates against its known
bound, and `console_path` scans bytes rather than decoding chars, so the producer
tests exactly the bytes `CommandStream::is_ws` splits on.
Gates: 675/675, stages 574/574, workspace suite green, check-spec green. clippy
clean over every changed region.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Jp9d9GVacWoV338MB6Tysf
The fix changes default behavior versus upstream -- `s >> filename` could only split on whitespace, and `sync_redirect_file` discarded its write error -- so it needs the two records this repo requires of a driver-tier divergence, the same pair DIV-88/89/90/91 carry: - `docs/history.md`: DIV-100, with the mechanism of both halves, why quoting at the caller could not have fixed either, and the evidence (0/675, 0 stage assertions, the two end-to-end probes that fail on the parent commit, the producer/consumer round trip that holds two crates' escaping rules together). - `docs/spec/00-overview.md`: the prose, next to the stdout boundary (DIV-89) -- the chapter that owns the console/driver seam. `make check-spec` green in lenient AND strict mode. `docs/cli.md` names the DIV so the user-facing contract is traceable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011cUTv4QXRtvx7JNHtTDfHZ
d4801ca to
f07bda7
Compare
Reviewed — correct, and both halves reproduce and closeVerified against a fresh
The diagnosis holds where it matters: quoting at the caller genuinely cannot fix a reader with no quote rule, so the grammar is the only place the fix can live; and Both end-to-end probes are load-bearing: reverting only Three things added before merge1. 2. 3. $ touch "$SCRATCH/outer"; TMPDIR="$SCRATCH/outer tmp" cargo test … a_spaced_temp_dir
the C was written to the truncated path …/scratchpad/outer, clobbering whatever was there
test a_spaced_temp_dir_still_yields_c ... FAILED
$ ls "$SCRATCH/outer"
ls: cannot access …: No such file or directory # the test deleted a file it never createdThe parent is now Gates (rebased onto
|
| Gate | Result |
|---|---|
make test |
PARITY OK — 675/675 |
make test-stages |
PARITY OK — 574/574 |
make rust-test |
green — 330 targets, 4,925 passed, 0 failed |
make check-spec |
green, lenient and strict |
make test-cli |
1/1 |
kuna catalog --check |
OK |
One note for the record: the PR body says nothing runs tests/cli/*.json. That changed with #364 — make test-cli is now a gate in CI. No probe is needed here (the loop promotes those; this fix carries real Rust integration tests), but the sentence is stale.
Squashing. Thanks — the write-up on both halves, especially the silent-clobber mechanism, made this quick to verify.
…tching `a_spaced_temp_dir_still_yields_c` derived its truncation target by splitting the scratch dir at its FIRST space, then removed that path unconditionally. The split only lands inside the test's own unique name while every component above it is space-free — and `std::env::temp_dir()` was the parent, so a developer whose system temp dir already contains a space (the very environment the fix is about) moved the split into THAT name: the probe failed spuriously and deleted a file it had never created. Verified: with `TMPDIR="…/outer tmp"` it reported "the C was written to the truncated path …/outer" and removed `…/outer`. The parent is now cargo's own per-target scratch (`CARGO_TARGET_TMPDIR`), which is free to choose because the child's temp dir is the `TMPDIR` this test passes it; the cleanup removes the target only when it is actually there, i.e. only what an unfixed build wrote; and an assert states the precondition rather than letting the clobber check pass vacuously. Both probes still fail on the parent commit with the Noelo-LabGH-362 diagnostics, and both now pass under a spaced `TMPDIR` as well as a plain one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011cUTv4QXRtvx7JNHtTDfHZ
f07bda7 to
970e3ba
Compare
Fixes #362.
What was broken
kuna decompileis the one surface that reaches the engine through a consolescript rather than an in-process call. Every path in that script was
interpolated unquoted, and the console reads a filename with
s >> filename—whitespace-delimited, no quote rule anywhere in the grammar. Any space in a
path split it into two arguments.
load file—read_tokenreturns the head, the command sees a second token,and re-reads: the head becomes the BFD target and the tail the filename.
With an explicit
--target(3 tokens) the tail was dropped entirely and the loadwas attempted against the prefix.
openfile write— the same defect on the output side, and the half that doesnot need a spaced binary path to bite. The temp file lives in
std::env::temp_dir(), which on Windows isC:\Users\First Last\AppData\Local\Tempby default, so on any Windows account with a space in the user name every
kuna decompilefailed, spaced binary path or not:And that half was silent data loss, not just a failed run.
sync_redirect_fileopened the truncated path with
.create(true).truncate(true)and discarded theerror, so the C was written to a file named after the first path component. The run
above created
/tmp/tmpcontaining the decompiled C; on Windows that path isC:\Users\John, and an existing file there is truncated with no diagnostic.Quoting at the caller could not have fixed this. A reader with no quote rule
takes the quote as part of the token:
Mechanism
The grammar learns a quote rule. New
CommandStream::read_filename(
kuna-console/src/interface.rs) layers an optional double-quoted form overread_token:read_token, eof latching included — thevendored corpus, every generated script and every hand-typed line without a quote
parse exactly as before;
\"and\\are escapes and every other backslash is literal, so"C:\Users\John Doe\a.out"and"C:\\Users\\John Doe\\a.out"both resolve to thesame path;
would have to invent a grammar error the C++ console has no wording for).
Wired into the four commands that actually read a path:
load file,openfile write,openfile append,parse file. (callgraph loadandload test fileareengine_unavailablestubs — nothing to fix.)The producers quote conditionally.
console_pathinkuna-cli/src/decompile.rsquotes only a path containing whitespace or a quote, so every script that works
today stays byte-identical — including for an older
decomp_dbgreached through--decomp-dbg, which would not understand a quote.scripts/decompile.pycarriesthe same helper, mirrored.
The silence goes away.
sync_redirect_filereports a failed open or write onstderr instead of discarding it. The CLI already forwards subprocess stderr into its
failure report, so a write that did not happen can no longer read as a decompiler
that produced nothing.
Scope
Only
kuna decompileand the console itself were affected.decompile-all,functions,decompile-projectandfidcallbootstrap_from_object(&binary, …)in-process, where the path is a Rust string that is never tokenized — verified
working on the same spaced path that broke
decompile. That asymmetry was its owntrap:
decompile-allsucceeded on a binarydecompilerejected as "unsupported".Deliberately out of scope, for a follow-up:
load function <name>,option <name> <value>andkassert <args>are interpolated into the same scriptunquoted and would split the same way on a value containing a space. They are symbol
and option values rather than paths, so routing them through a filename reader is
the wrong shape; they want their own decision. No known binary trips them today.
Not gated behind an option
Per AGENTS.md a feature ships behind a named option, a strict bug fix does not. This
cannot change emitted C for any input that works today — unquoted paths parse
byte-identically, and the CLI emits an unchanged script for every path without
whitespace. No
settableTablerow, no catalog count change. No measurable speeddelta: the added work is one
containsscan per path, per run.Tests
interface.rs(5)read_filename: quoted paths keep spaces; unquoted is byte-identical toread_token(value and eof) across four line shapes; both Windows spellings and an embedded\"; two quoted args still separate; unterminated quote terminates.decompile.rs(5)console_pathleaves ordinary paths alone and quotes/escapes when needed;build_scriptquotes the image, C and regions paths, quotes after a--target, and emits a quote-free script for ordinary paths.decompile_cli.rs(2)TMPDIRstill yields C and nothing was written to the truncation target.Both end-to-end tests were verified to fail without the fix, with the exact
diagnostics from #362:
They live in
decompile_cli.rsrather thantests/cli/because nothing currentlyruns
tests/cli/*.json(noted in thefunctions-json-sizePR body), and ratherthan
tests/stages/because the defect is in the CLI/console seam, not thedecompiler phase model — so no stages baseline re-record and no corpus file count
bump.
Gates
make testmake test-stagesmake rust-testmake check-specdocs/cli.mddocuments the behaviour underkuna decompile, including the quotedform for hand-written console scripts. No
docs/spec/chapter ownskuna-console— the chapters anchor onkuna-analysisand the phase folders — sothere is no spec chapter to update here.
Verified by hand
All succeed; the control run on an unspaced path is unchanged, and no file appears at
any truncation point.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Jp9d9GVacWoV338MB6Tysf