fix: let the engine find its own installed logger instead of filing nothing - #63
Merged
Merged
Conversation
…othing
Measured on a live run of the installed plugin: neither craftRoot nor
CLAUDE_PLUGIN_ROOT was set in the logger agent's shell, so the prelude refused
and the run filed no record at all. The refusal itself is right — resolving the
logger relative to the reviewed repository would execute that repository's own
script with the user's privileges, which is why the `:-.` fallback was removed —
but the assumption before it was false: the code said an installed plugin has
the env var set for it, and it does not. The cost is not one lost record but
ZERO records in that mode, and it is invisible: the store stays full of older
runs, so neither the file count nor the index shows the hole. Only the report
says so, and only to whoever reads it. Measurement across engine revisions is
the thing that quietly stops being possible.
So the engine now looks for its own installed copy before giving up, and the
search is deliberately narrow: only under the user's own plugin cache, and only
for the exact version the engine is stamped with. A 0.18.1 engine can never log
through a 0.16.0 script and file records describing a run that build never made.
The version comes off the record being filed rather than a parameter of its own,
so the copy the logger is found by cannot drift from the version the record
claims. The reviewed repository is never a candidate — verified by planting a
lib/craft-log-run.mjs in the working directory and watching the block refuse.
Order matters and was wrong at first: the search is a FALLBACK, not an override.
Written the other way round the loop overwrote a good path from
CLAUDE_PLUGIN_ROOT with whatever the cache held, so a launch from a checkout
would have logged through some other installed version without a word.
Three executed tests, each falsified separately by breaking exactly its property:
a planted install is found when the environment says nothing; a version with no
installed copy refuses rather than borrowing another; and the environment wins
when it resolves.
Two existing tests failed on this change and both were asserting the SPELLING of
the old prelude rather than its property — one pinned the exact former one-liner,
the other banned the `:-` operator outright when what is dangerous is a default
PATH (`${VAR:-.}` supplies a directory; `${VAR:-}` expands to nothing). Rewritten
as properties: the environment is consulted first, no default path exists, and an
unresolvable logger stops the command by name. A third now pins HOME, since a
test whose outcome depends on what is installed on the machine running it is not
a test.
… its version too Cold review found the recurring pattern three times in the previous commit. The search had a SECOND candidate — the marketplace directory — and it is not a versioned release but a git clone that moves on its own, which is exactly what the version pin exists to prevent. On a machine whose clone had fetched a newer build, an engine stamped 0.18.0 would have executed that newer script, which stamps engineRevision and craftCommit from its own build, while the record body said 0.18.0. A record misdescribing which engine ran is worse than no record, because it gets counted. One candidate now, version-pinned, and a test proves an unpinned copy is refused even when it is the only one present. The fix reached two of the three logger commands in review.js. LOGGER_PRELUDE was computed at line 128 with no version — CRAFT_VERSION is declared six hundred lines below and moving it is release-please's business — so the prior-round read kept refusing exactly as before while the record write and the checkpoints found their script. Deferring the prelude to a function called at use time lets all three agree. And the assertions that were supposed to guard all this inspected the VERSIONLESS prelude, so the search block was not in the string under test at all: adding `$PWD` as a candidate left both tests whose stated job is "the working directory is never a candidate" fully green. They now assert the prelude as the engine actually emits it, and ban every shape that names the current directory — `$PWD`, `$(pwd)`, a `:-` default supplying a path, a bare relative — not just the one literal the old assertion knew. Two more from the same review: CLAUDE_CONFIG_DIR is honoured, since a session configured that way keeps its plugins elsewhere and hardcoding ~/.claude would have left that user with the defect unfixed and no sign of it; and the version is quoted into the shell rather than interpolated raw. The version is a source literal today and nothing external reaches it, but it was the one place in this file bypassing the quoting every other interpolation goes through. The harness's plugin-cache layout is someone else's surface, relied on here for the first time, so it is now pinned by observation in the realm (@nick/craft #48) and referenced from the code — including the distinction that decides the fix: a versioned directory asserts its version, a marketplace checkout asserts nothing.
…from its payload Third round on this branch. The first finding is the one that matters: the hole the `:-.` removal closed had re-entered by another door. `[ -f ]` runs in the logger agent's cwd, but `node "$CRAFT_LOGGER"` runs AFTER the cd into the reviewed repository — so a RELATIVE $CLAUDE_CONFIG_DIR makes that repository a search candidate. A user with a project-local config dir and a hostile repo shipping .claude/plugins/cache/craft/craft/<version>/lib/ craft-log-run.mjs — the version is public in the manifest — would have executed it with their own privileges. A non-absolute value is now refused outright rather than normalized: guessing what the user meant is how this class keeps coming back. Executed with a real relative value, because the relative-ness arrives from the environment and can never appear as a literal in the emitted string — no amount of matching the prelude text could have seen it, which is also why the string-based "no cwd candidate" assertions could not. Second: `checkpointPrompt` took the version as a plumbing argument with a silent '' default, and it was duly forgotten at one of three call sites — deleting it there left every gate green while that engine's checkpoints refused exactly as before the fix, with finalize and prior-round succeeding beside them. It derives from the payload now, the way the record builder derives from the record, so there is nothing to forget. craftVersion rides on every checkpoint slice as a result, which it should have anyway: a slice that cannot say which build wrote it is a slice `recover` cannot place. Third: the version reached the shell unquoted in the refusal message. The search itself was quoted, so the injection sat in the one place nobody looks — the error path. Both are quoted now. Worth recording, because it cost a round: the test written to catch that injection first reported a breach that was not there. The refusal message quotes the version back verbatim, so a naive /PWNED/ match cannot tell execution from mention. It now asserts on a line that IS the word. A falsifier that goes red is no more trustworthy than one that stays green — both need reading.
Fourth round. The invariant this branch states — nothing may resolve the logger
against the reviewed repository — was applied to one of the three ways
CRAFT_LOGGER gets set.
An explicit `craftRoot` returned EARLY, before the absoluteness check and before
the refusal. `craftRoot` arrives in the model-composed args string, so a review
launched with `craftRoot=.` against an untrusted repo shipping
lib/craft-log-run.mjs emitted `CRAFT_LOGGER='.'/lib/craft-log-run.mjs` and then
`cd <reviewed repo> && node "$CRAFT_LOGGER"` — the removed `:-.` hole restored
verbatim, bypassing the version pin and the loud refusal with it. Executed, it
printed PWNED with rc 0. `${CLAUDE_PLUGIN_ROOT:-}` had no absoluteness check
either. The shape predates the branch; stating the invariant while leaving two
doors open is what made it this branch's problem.
Every candidate now runs the same three checks — absolute, present, ordered
(explicit root, environment, then this engine's own installed copy) — and any
miss falls through to the same refusal. Verified by executing the emitted block
from inside a planted hostile repo for all four attacks and both legitimate
paths.
Two tests were pinning the unguarded shape as correct, so the gate was defending
the hole: both asserted the exact early-return one-liner. They assert the
property now — an explicit root is consulted, and is guarded like everything
else.
And the injection test's payload set was inert under the very defect it names:
stripping the quoting left all of `"; echo …`, `$(…)` and backticks harmless —
the first opens a quote that swallows to the next one, the other two have their
output assigned rather than printed. The payload that actually executes,
`1.0.0; echo PWNED; :`, was missing. A falsifier that cannot go red is an
assertion that cannot fail, one layer out; it is now the first payload in the
list and reds on the stripped quoting.
…e reviewed repo The comment promised more than the code gave. It called craftRoot "exactly as untrusted as the --dir this project already refuses", but the pipeline enforced only absoluteness — and an absolute path may still name the repository under review. Executed: craftRoot=<the repo>, craftRoot=<repo>/lib/.., and a symlink pointing at the repo all ran the repo's own script, rc 0. So the resolved logger is now compared against the directory the command is about to cd into and refused if it lies within it, with pwd -P on both sides. That normalization is what makes the three routes one case: a `..` climb and a symlink into the repo are indistinguishable from naming it outright. This is the constraint --dir has carried since 226adc4; the logger path had half of it. Also from the same review, and it is the sharper lesson: two assertions carried an alternation whose second branch was the PRE-FIX spelling. `engine-harness` is the only cross-engine guard on the logger command, and with `|CLAUDE_PLUGIN_ROOT:\?` in it a FULL revert of this branch left the suite green. Both dead branches removed — reverting the prelude to main's now reds 17 tests. A branch that can only be satisfied by the old shape is not a fallback, it is a blind spot, and it sat on the one guard that is supposed to notice the whole class coming back. Realm #45 updated: the route actually taken is a third one, neither of the two the question named — the engine searches for its own version in the harness's plugin cache (#48, observed rather than documented). Recorded with the reason it beats "name the mode unsupported": that mode is not unsupported, it is the one the plugin actually runs in at a consumer. The closing condition is unchanged and still unmet — it needs a live run of an INSTALLED build carrying this fix, which cannot exist before a release.
A symlinked FILE pointing into the reviewed repository still passed: `pwd -P`
resolved the DIRECTORY and the basename was re-appended unresolved, so the check
compared a path that was never the one node would open. The comment and the
realm node both claimed symlinks were covered; only symlinked directories were.
The test written for it covered three routes and stopped one short of the
fourth, which is why the gate was green.
Underneath that: this property has now been got wrong three separate times, each
in a place the previous fix did not name — a string prefix that called
`/x/store-evil` a child of `/x/store`; a check applied to CLAUDE_CONFIG_DIR
while an explicit craftRoot skipped it; and this one. Each fix was correct and
each left a sibling wrong, because the property was tested where it had just
been repaired.
So the question is named once. `containsPath(parent, child)` answers it in JS,
resolving symlinks as far as the path exists (realpathSync throws on a path not
yet created, which is ordinary for a run directory), and `insideStore` is now
one caller of it. The shell prelude answers the same question through
`craft_usable`, which every candidate goes through — absolute, present, outside
the repo — with symlink chains followed to their target and both sides
normalized. `lib/path-containment.test.mjs` holds ONE table of eight cases and
runs it against BOTH: plainly inside, a sibling sharing a prefix, a `..` climb,
a symlinked directory, a symlinked file, a chain of symlinks, an unrelated
directory, a symlinked parent. Falsified per side and per historical bug:
dropping the slash anchor reds the sibling row, dropping symlink resolution reds
the three symlink rows, in whichever implementation it is dropped.
Making the predicate per-candidate also fixed a second finding: containment was
a terminal `exit 1` on the winner, so an explicit craftRoot naming the repo
killed the whole command instead of falling through to the environment and the
installed copy. That is craft reviewing its own checkout — the mode this repo
mandates for itself — filing zero records. Now the candidate is rejected and the
next one is tried.
Two assertions were guarding nothing. `split('cd ')[1]` took the first `cd ` in
the prompt, which since the containment preamble is `$(cd <repo> && pwd -P)` in
a subshell — three lines mentioning no logger at all. Reintroducing the original
vulnerability left it green; both are anchored on the command line that actually
enters the repo now, and that mutation reds four tests.
And one mechanical trap worth recording: a line that is exactly `}` at column 0
ends an extracted inline region, so the shell function's closing brace truncated
the region and the four engines got an unbalanced copy. The checker caught it —
by compiling the region, not by comparing bytes. The brace is indented.
Seventh round, and the finding is a hole this branch introduced rather than inherited. The symlink walk stopped after sixteen hops and then carried on: the path was still a symlink, the containment comparison tested that unresolved string, nothing matched, and the candidate was accepted. A chain of twenty-one links leaving the reviewed repository and returning to it executed the repo's script with the user's privileges — demonstrated, not reasoned about. Since the explicit craftRoot candidate is tried first, it also beat a perfectly good CLAUDE_PLUGIN_ROOT. Every exit from the predicate that is not a fully resolved path outside the repo is now a refusal. The eight-case table could not see it: it pinned the three historical bugs and stopped at the boundary the new implementation itself introduced. That is the same shape one level up — coverage aimed at where the property was last repaired. There is a ninth row now, and building it taught something worth keeping: with the hops inside the repo the row does not discriminate, because a walk that gives up early still leaves a path under the repo and the comparison catches it anyway. Only a chain that leaves and returns makes full resolution the one thing standing in the way, which is how the attack was actually shown. Two tests were green for the wrong reason and one is now genuinely green: - the end-to-end "the reviewed repository never executes" passed with the symlink walk fully disabled, because `node` is not on PATH in the scrubbed env, so PWNED could not print either way. It uses `process.execPath` now and carries a positive control: the same fixture must be shown able to execute a legitimate logger, or an absence proves nothing. - `shellSaysContained` read any non-zero exit as "contained", so a prelude with a syntax error would have satisfied six rows for a reason unrelated to containment. It reads the FAILED marker now, and throws if the prelude neither resolved nor refused. And two guards are labelled belt-and-braces rather than dressed up as covered: with CRAFT_REPO empty the case pattern degenerates to `/*` and refuses every absolute path anyway, and `pwd -P` can only fail where `[ -f ]` already did. Removing either changes no observable behaviour, so no test distinguishes them — said here instead of implied by a test that would pass either way.
…he predicate Eighth round. The first finding is this branch's own shape sitting in this branch's own test file. `EVERY way of naming the logger is refused when it is not absolute` passed with the absoluteness guard DELETED — every case omitted `repo`, so it defaulted to `.` and containment resolved it to the cwd, which the test had set to the planted directory. Every refusal therefore came from containment, and the only assertion this branch has against defect #45's own class could not see that class. The cases now declare a repo that is NOT the cwd, so the relative candidates land somewhere containment has nothing to say about and only absoluteness can refuse them; they also assert that nothing in the cwd executed. Deleting the guard now reds exactly this test. Second: a repo of `/` inverted the predicate into an allow-all, on both sides. `"$CRAFT_REPO"/*` becomes `//*`, which matches no ordinary path, so every candidate read as outside; `containsPath` compared against `//` and answered the same. The root contains everything and nothing can be outside it — degenerate input, and precisely where a guard must fail closed rather than open. `repoArg` comes from the model-composed args string, the same untrusted channel as craftRoot. Both sides fixed, and the shared table has a row for it: asked of one implementation only, the wrong answer would have looked like agreement. Third: `CRAFT_LOGGER` was assigned the unresolved candidate while the predicate had validated the resolved one. Between the check and `node "$CRAFT_LOGGER"` sit the mktemp, the whole heredoc of a record that can be hundreds of kilobytes, and the cd — a window in which a symlink component of the candidate can be re-pointed into the reviewed repository. The resolved path is handed over now. Three tests compared against unresolved temp paths and started failing for that reason: on macOS /var is a symlink to /private/var, so a resolved actual against an unresolved expected reports a difference the code did not make. They realpath their fixtures now. The root row is deliberately skipped by the end-to-end execution check: with the declared repo `/`, nothing can be outside it, so the positive control that test depends on has no possible fixture. Skipping a case whose control cannot exist is honest; skipping one whose control merely fails would not be.
…ed every error Ninth round. The first finding is the branch's own pattern reproduced inside the very commit that fixed another instance of it: `d2f5232` added the resolved-vs-unresolved handover and shipped it untested. Reverting `CRAFT_LOGGER="$CRAFT_REAL"` to `"$CRAFT_TRY"` — the check-then-use window fully restored across the mktemp, the record heredoc and the cd — left all 476 tests green. Not a race to reproduce either: the difference is directly observable, so one assertion on a symlinked fixture pins it. That is how the `:-.` hole and the absoluteness hole each came back, and it was about to be how this one did. Second, and the same shape one layer down: `resolveAsFarAsItExists` caught EVERY realpathSync error while its comment named only the not-yet-created case. A symlink loop, an unreadable parent or a bad mount therefore degraded into a purely lexical answer, and `insideStore` reported a path as inside the store that resolves nowhere near it. Only ENOENT is walked past now; anything else is unresolvable, and `containsPath` returns no verdict rather than inventing one — callers read it to decide whether a path is safe to act on, and "I could not tell" must never arrive dressed as "no". Both falsified: reverting either reds exactly its own test. One input is named in the table's header rather than given a row, because the two implementations genuinely disagree on it: a candidate that does not EXIST is refused by the shell at `[ -f ]` and answered lexically by the JS side. Every row builds a real file. A row asserting an agreement that does not hold would have been worse than the note.
…left open The checkpoint builder's version derivation got a test when it was made underivable-by-forgetting; its twin on the record path did not. Replacing it with '' left all 478 tests and every gate green while the plugin-cache search stopped being emitted for `write` and `finalize` in all four engines — defect #45 walking back in through the front door of the branch that exists to close it. Every loggerPrelude test passes a literal version, so nothing observed the one wiring that takes it off the record. Three lines, red on the reverted derivation. Recorded because the shape is the one this branch keeps meeting: a fix applied to two places and tested in one.
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.
Why
Measured on a live run of the installed plugin, not reasoned about: neither
craftRootnorCLAUDE_PLUGIN_ROOTwas set in the logger agent's shell, so the prelude refused and the run filed no record at all. The refusal itself is right — resolving the logger relative to the reviewed repository would execute that repository's own script with the user's privileges, which is why the:-.fallback was removed. What was false was the assumption in front of it: the code said an installed plugin has the variable set for it, and it does not.The cost is not one lost record but zero records in that mode, and it is invisible — the store stays full of older runs, so neither the file count nor the index shows the hole. Only the report says so, and only to whoever reads it. Measurement across engine revisions is the thing that quietly stops being possible.
What changed
The engine looks for its own installed copy before giving up: under the user's own plugin cache (honouring
$CLAUDE_CONFIG_DIR), and only for the exact version it is stamped with, so a 0.18.1 engine can never log through a 0.16.0 script and file records describing a run that build never made. The version comes off the record being filed rather than a parameter of its own, so the copy the logger is found by cannot drift from the version the record claims.Along the way the logger path picked up the constraint
--dirhas carried since226adc4. Every candidate — explicitcraftRoot, the environment variable, the installed copy — now goes through one predicate: absolute, present, and outside the directory the command is about tocdinto. The containment question itself is named once,containsPathin JS withinsideStoreas one caller andcraft_usablein the shell, andlib/path-containment.test.mjsruns one table of ten cases against both.Ten review rounds, and what they found
Every round found something real, and the pattern is the finding: nine of ten were defects inside the previous round's fix, and several were holes the fix itself introduced rather than inherited.
engineRevisionandcraftCommitfrom its own build — while the record said this version. A record misdescribing which engine ran is worse than no record, because it gets counted.$CLAUDE_CONFIG_DIRreopened the original hole by another door.[ -f ]runs in the agent's cwd,noderuns after thecd— so a relative candidate resolves in the reviewed repository. A project-local config dir plus a hostile repo shipping.claude/plugins/cache/craft/craft/<version>/lib/craft-log-run.mjs(the version is public in the manifest) would have executed it.craftRootreturned early, skipping both the absoluteness check and the refusal.craftRoot=.against an untrusted repo printedPWNEDwith rc 0 — the removed:-.hole verbatim, andcraftRootarrives in the model-composed args string..., and a symlink pointing at it are now one refused case, withpwd -Pon both sides.pwd -Presolved the directory and the basename was re-appended unresolved./inverted the predicate into an allow-all on both sides (//*matches no ordinary path).Tests, and what was wrong with them
Half the rounds found the defect in a test rather than in the code, which is worth stating plainly:
repo, so it defaulted to.and the refusals came from containment rather than absoluteness;nodewas not on PATH in the scrubbed env andPWNEDcould not print either way — it usesprocess.execPathnow and carries a positive control, since an absence proves nothing until the same fixture is shown able to produce the presence;Every fix on this branch was falsified by breaking exactly its own property and watching exactly its own test go red. Where a mutation changes no observable behaviour, the guard is labelled belt-and-braces in the code rather than dressed up as covered.
Deliberately open
craftRootis the repo under review and is correctly refused, the environment variable is unset, and the search finds the cached copy of the same version. The record is then filed by that script while the branch's engine ran — telemetry attribution, not safety.[ -f ], the JS answers lexically). Named in the table's header rather than asserted as an agreement that does not hold.Gates
node --test479/479 · eslint 0 · check-workflows 0 (25 inlined regions match) · check-skills 0 · check-evals 0 — all read as raw exit codes.