Shell-escape every dynamic value in the companion wait commands - #108
Merged
Conversation
The bounded-wait contract added in #94 built its command by interpolating `workspaceRoot` (or `WORKTREE_ROOT`) straight into double quotes and `jobId` bare. Double quotes do not neutralize an embedded `"`, a backtick, or `$(...)`: a checkout at `/tmp/repo$(id -un)` sends the companion a substituted path, and one containing `"` fails to parse at all. These markdown files are executable instructions, so that was the recipe the controller followed. Apply implement.md's existing shellEscape doctrine — one dynamic value per argument, `--` before the first positional — to the wait, persisted-result, and recovery commands in all five commands that carry the contract. implement.md defined the doctrine but had never applied it to its own companion or git invocations; it now derives `rootArg` in Pre-flight and uses it throughout. `status` and `result` accept a bare `--` (neither sets stopAtFirstPositional), so the job id still lands in positionals[0] with the flags ahead of it. `"${CLAUDE_PLUGIN_ROOT}"` stays double quoted: the shell expands it rather than the model interpolating a value into it. The shared cross-command tests now pin the escaped recipe rather than the raw one, and a new test rejects any double-quoted interpolation other than CLAUDE_PLUGIN_ROOT, plus any raw root/job/thread value on a companion or git command line. Closes #107 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
patriyang
added a commit
that referenced
this pull request
Aug 11, 2026
#110) ## What `captureRepoStateIdentity` fingerprints a working-tree review target so a queued background review can refuse to run once the repository has moved under it. Building that fingerprint hashed content one path at a time: every dirty tracked path and every untracked regular file went through its own `git hash-object --no-filters -- <path>` `spawnSync`. That cost is `O(dirty paths)` process startups, paid inline on the `--background` enqueue path — the interactive call the user waits on before a job ID comes back. Measured before the change on a synthetic repo (N dirty tracked + N untracked files), with `git` instrumented through a PATH shim: | N (each kind) | `git hash-object` processes | wall clock | |---|---|---| | 50 | 100 | — | | 400 | 800 | 6639 ms | | 800 | 1600 | 10227 ms | Exactly `2N` process startups, ~6.4 ms per path, linear. ## How `captureWorkingTreeDigest` is now classify → batch → assemble: - **classify** — one `lstat` per tracked and untracked path decides whether it is a regular file (needs an OID) or a non-regular path (symlink, directory/gitlink, missing, other), which keeps its existing token logic untouched. - **batch** — `hashRegularFilesBatched` hashes the regular files from both sets in chunks bounded by path count (256) and cumulative UTF-8 path bytes (8 KiB, conservative enough to stay well under Windows' ~32767-character command line including the fixed arguments and quoting). `git hash-object` accepts multiple paths and emits one OID per line in argument order; a chunk's output is accepted only when the process succeeded **and** the non-empty line count equals the chunk length. - **assemble** — the digest is built in the original order with byte-identical token strings. A chunk that fails for any reason falls back to hashing that chunk's paths individually, so one bad path (deleted mid-run, unreadable) cannot poison its neighbours. The per-path failure result is carried through, so a tracked file that fails to hash still throws with the real spawn error or the actual status/signal/stderr — the same fidelity as the `gitChecked` call it replaced — while untracked files keep their tolerant `skipped:(...)` behavior and never throw. Same tree, after: | N (each kind) | `git hash-object` processes | wall clock | |---|---|---| | 400 | 4 | 130 ms | 51× faster at N=400, and the digest is **byte-identical** to what the old code produced for the same tree (`e4c0b753…c299a1` before and after). ## Semantics preserved Every per-path behavior is unchanged: `missing`, `symlink:<sha256 of readlink target>`, `submodule:<head>:<status>:<staged>:<unstaged>`, `other:<mode>:<size>`, and the untracked `inspectUntrackedFile` branch — including that the `MAX_UNTRACKED_BYTES` / binary / directory *display* limits still apply only to non-regular untracked paths and never affect regular-file identity. The digest composition (status output, then `\0tracked\0…`, then `\0untracked\0…`) is untouched. ## Tests Four new tests in `tests/git.test.mjs`: 1. **Process count** — a 300 + 300 tree, with a `git` shim on `PATH` logging each subcommand, asserts `hash-object` invocations stay a small constant. Verified red against the pre-change implementation: `expected at most 8 hash-object calls, got 600`. 2. **Chunk-to-OID alignment** — 300 files span more than one 256-path chunk; editing a file in the second chunk is still detected. 3. **Awkward paths** — spaces, non-ASCII, and a leading `-` in both the tracked and untracked sets, guarding the `--` separator and the argv batching. 4. **Mixed tree** — dirty tracked file, tracked symlink, untracked symlink pointing outside the repo, untracked binary, and an untracked file over `MAX_UNTRACKED_BYTES`: no drift right after capture, and each mutation still detected. Full suite: 352 tests, 0 failures (`npm test -- --test-concurrency=1`; the parallel default spawns enough app-servers to produce spurious broker/runtime failures in this repo). ## Notes - Rebased onto `main` after #108 merged mid-change; version bumped to 1.0.45 since 1.0.44 is already published with different plugin source. - Submodule fingerprinting still costs 4 git processes per dirty submodule directory. Out of scope here — that cost is per submodule, not per path. - The first `review --background` dispatch of this branch stalled on an in-flight `codegraph/codegraph_explore` call in the fresh worktree; a re-dispatch reviewed clean. Recorded as evidence on #105 rather than fixed here. Closes #102
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The #94 bounded-wait contract built its command by interpolating
workspaceRoot(orWORKTREE_ROOT) directly inside double quotes, withjobIdinterpolated bare:command: `node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" status -C "${workspaceRoot}" ${jobId} --wait --timeout-ms 240000 --json`Double quotes do not neutralize an embedded
", a backtick, or$(...). Verified against a real shell before fixing:/tmp/repo$(id -un)-C /tmp/repopatrickyang— the substitution ran/tmp/re`hostname`po/tmp/re"pobash: -c: line 0: unexpected EOF while looking for matching '"', exit 2/tmp/my repoThese markdown files are executable instructions, so this was the recipe the controller actually followed.
How
Applied
implement.md's existing shell-escaping doctrine — one dynamic value per argument,--before the first positional — to the wait, persisted-result, and recovery commands:Identical across all five commands, so one shared test still pins one recipe.
Scope note — five files, not the four the issue names.
implement.md:105carried the same unescaped wait command, and it is the file that defines the escaping doctrine. It now derivesrootArgin Pre-flight and uses it for every companion andgit -Cinvocation in the file, and escapes the--resume-idthread values too. Left alone deliberately:${ORIGINAL_BASE_SHA}..HEADin agit logdisplay command, and"${CLAUDE_PLUGIN_ROOT}", which the shell expands rather than the model interpolating into.--is safe here: neitherSTATUS_PARSE_CONFIGnorRESULT_PARSE_CONFIGsetsstopAtFirstPositional, so a bare--switchesparseArgsto passthrough and the job id still lands inpositionals[0]. Confirmed live —status -C <dir> --json -- nonexistent-jobproduces output identical to the un-separated form. Flags must precede the--, which is why the recipe orders them that way.Tests
Written first, red before the fix, and tightened rather than loosened as the issue asked:
every companion command is built from shell-escaped dynamic valuesrejects any"${name}"double-quoted interpolation other thanCLAUDE_PLUGIN_ROOT, and any rawworkspaceRoot/WORKTREE_ROOT/jobId/threadId/IMPLEMENTER_THREAD_IDon a line invokingcodex-companion.mjsorgit -CFull suite: 348 passed, 0 failed.
Prose only — nothing under
plugins/codex/scripts/changed. Version bumped to 1.0.44 as the plugin-source check requires.Closes #107
🤖 Generated with Claude Code