Skip to content

Give the hook's injected-block stream a verifiable framing, and stop a sample call from writing to hooks.log (#219, #217) - #222

Merged
fdaviddpt merged 2 commits into
mainfrom
fix/219
Aug 25, 2026
Merged

Give the hook's injected-block stream a verifiable framing, and stop a sample call from writing to hooks.log (#219, #217)#222
fdaviddpt merged 2 commits into
mainfrom
fix/219

Conversation

@fdaviddpt

@fdaviddpt fdaviddpt commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Summary

Two independent fixes bundled in one lane because they touch the same two files.

#219 — the hook's injected-block stream could be forged into a fabricated match.
pre-prompt-hook.sh used to join matched vocabulary entries and refusal notices with
the literal text \n---\n# Vocabulary: (and \n---\n# JIT Context: ).
.claude/jit-context/ is attacker-controlled input, and an entry whose own body
happened to end in that exact text joined with the next block in a way byte-identical
to a genuine boundary — reproduced on scripts/jit-match.sh before this fix.

The hook now prepends one manifest line to its own output — # JIT-CTX-BLOCKS <n> <len1> <len2> ... — built entirely from awk length(), never from anything an entry
authored. jit-match.sh walks the joined text by that byte count instead of searching
it for the separator, which closes the class rather than merely detecting it: the
exact #216 reproduction (a body quoting the join text verbatim) now reads as one clean
match instead of one real match plus one "unverifiable" phantom. jit_index_verified()
(#216) is kept unchanged as a second, cheap structural guard.

pre-tool-hook.sh and pre-path-hook.sh, and the shared
jit_refusal_notice()/jit_layers_notice()/jit_config_notice() functions in
common.sh, are deliberately untouched — pre-prompt-hook.sh wraps their unchanged
return values itself, so neither the other two hooks' output shape nor any consumer of
them changes.

#217 — a sample call wrote a synthetic record into the target project's real
hooks.log.
scripts/jit-match.sh and scripts/jit-dry-run.sh
(--prompt/--tool/--path, via its single report_hook() funnel) both shell out to
the real hook to answer a diagnostic question, and every such call was writing a record
into hooks.log — the file jit-misses.sh reads to report genuine vocabulary gaps.
Both callers now set JIT_SAMPLE_CALL=1 on the hook subprocess's own environment;
common.sh treats that the same as a symlinked log path. The safety argument: it is a
plain env var only these two diagnostic scripts set, never derived from config.env,
the JSON payload, or anything else attacker/session-controlled, so a real session has no
route to the same suppression.

Both changelog fragments (changelog.d/219.fixed.md, changelog.d/217.fixed.md) carry
an explicit Compatibility note — #219 for a hand-parser of the raw hook output, #217
for a project whose hooks.log has been carrying sample-call noise since jit-dry-run.sh --prompt shipped.

Self-review

Two agents reviewed the first commit (b3fd3d2) independently: an Explore reviewer and
oss:auditor. Both came back states-findings (verified with scripts/review_return.py --framed, header count == enumerated count). Four stale-comment/doc findings from that
round were fixed in a second commit (4a05c48) — scripts/jit-match.sh's own
threat-model comment (two spots) and the .claude/jit-context/paths/00-manual/tooling.md
dogfood entry all still described the #219 fix as unmade or out of scope.

Two findings were not fixed here, on purpose:

  • scripts/jit-dry-run.sh's report_hook() has the same forging class The hook joins injected blocks with free text an entry body can legitimately contain, so no consumer can parse that stream unambiguously #219 closes
    elsewhere, unfixed.
    It greps raw hook stdout for "Vocabulary: X.md" /
    "JIT Context: X.md" text with no manifest awareness at all, so a vocabulary entry
    whose body forges that text still makes jit-dry-run.sh --prompt/--tool/--path print
    a fabricated file name as a real match — reproduced live against a tricky.md fixture
    quoting a forged # Vocabulary: evil-forged.md (matched: evilkw) block. This predates
    this branch (it is jit-dry-run.sh's pre-existing behaviour, not something this diff
    introduces or worsens) and closing it properly means porting the decode-plus-manifest
    logic jit-match.sh already carries into report_hook() — a second, comparable-sized
    diff on top of an already substantial one. Reported for filing, not fixed here.

  • Below-bar: jit_index_verified()'s reject branch and jit-match.sh's fallback
    splitter have no positive test control left in the suite
    , after the Add jit-doctor and vocabulary matching as reachable command surfaces #216 fixture was
    rewritten in this branch from assert_has "unverifiable" to assert_lacks "unverifiable" (correctly — that fixture is now a clean match). True, worth saying,
    but not currently reachable: jit-match.sh only ever gets there via a malformed
    manifest, and the real hook always emits a well-formed one (its length() is computed
    from the block's own bytes regardless of body content), so the gap is in defensive
    code with no attacker-reachable caller today. Testing it properly would need
    jit-match.sh refactored to accept injected/mocked hook output, out of scope for this
    lane.

Test plan

  • bash tests/run-all.sh — green, 0 failed (includes the two new suites,
    tests/test-block-framing.sh and tests/test-sample-call-log.sh, plus the
    rewritten section of tests/test-jit-match.sh)
  • Both new suites, plus tests/test-jit-match.sh, re-run manually under gawk and
    mawk via a PATH shim (not just the platform default awk) — all green on all three
    engines
  • shellcheck -S warning scripts/*.sh tests/*.sh — clean
  • python3 .oss/assemble_changelog.py --check — ok, both fragments valid
  • bash tests/test-line-citations.sh — clean, no line-number citations added

Closes #219
Closes #217

Verified by the maintainer

Three things, all of which the lane could not establish about its own work.

The JIT_SAMPLE_CALL safety argument, probed adversarially rather than read. The claim that carries this fix is that a real session has no route to the same suppression — a hook that can be told not to log is a hook whose log proves less, so that claim is the difference between a fix and a hole. The code comment says the variable is "never a value read out of config.env"; I built the attack instead of believing it. A fixture project whose config.env carries JIT_SAMPLE_CALL=1, driven through the real hook:

- line 1: unknown setting (only JIT_CONTEXT_*, DYNAMIC_RULES_* and DVSI_* are read)

The line is refused and named, and hooks.log was written anyway — the suppression did not take effect. The three-prefix allowlist is what stops it, and JIT_SAMPLE_CALL matches none of the three. Worth noting the refusal text goes further than it needs to and tells the reader what the finding means: if a refused line is not one you wrote, treat that file as hostile — it arrived with the repository.

Both new suites re-run against the default branch with the fix absent, in a scratch worktree detached at 3b13092:

test-block-framing.sh      PASS: 8  FAIL: 3
test-sample-call-log.sh    PASS: 2  FAIL: 4

The counts differ slightly from the report's, because the report's red was taken against this branch's own parent and main has moved since. What matters is the shape, and it is right: the assertions that pass on main are the positive controls, not gaps. a real (non-sample) hook call still writes hooks.log holds on both trees — as it must, or the suppression half would be asserting nothing. the forged text rides along as part of the one real entry's own body likewise.

The manifest is a stronger answer than the brief asked for, and that is worth saying plainly. I asked for a separator a consumer could verify rather than infer, and left the framing open. Walking the stream by a byte count computed from each block's own length() does not detect a forgery — it makes one unrepresentable, because nothing an entry body contains can change how many bytes that body is. jit_index_verified() staying in place as a cheap second check is the right call and I would have argued for it had it been removed.

What this does not close, and why the release still waits on it

scripts/jit-dry-run.sh's report_hook() still greps the raw stream for block headers with no manifest awareness, so the same forging class remains reachable there — reproduced live by this lane, against a fixture body forging evil-forged.md. The information it needs is now on the wire; that consumer simply does not read it yet.

I accepted the deferral rather than sending it back: it predates this branch, sits in a file the brief did not name, and porting the decode-plus-manifest walk is a diff of comparable size to this one. It earns its own review rather than riding along at the end of a long lane. Filed as #223, priority-high.

Consequence, stated so it is not a surprise later: forges is a release-blocking row, so this merging does not unblock a tag. It narrows the exposure from the shared surface to one shipped tool. #223 is what closes it.

Florian DAVID added 2 commits August 25, 2026 20:43
…a sample call from writing to hooks.log (#219, #217)

pre-prompt-hook.sh now prepends a byte-length manifest to additionalContext so a
consumer walks blocks by count instead of searching for a separator an entry body
can forge; jit-match.sh trusts it (falling back to the old heuristic if absent or
malformed) and jit_index_verified() stays as a secondary structural check. Separately,
jit-match.sh and jit-dry-run.sh now set JIT_SAMPLE_CALL=1 on the hook subprocess so a
diagnostic probe no longer pollutes the target project's real hooks.log, with no route
for a real session to reach the same suppression.

Co-Authored-By: Max <noreply>
scripts/jit-match.sh's own threat-model comment and its jit_index_verified() call-site
comment still described the fabrication class as unclosed and out of scope, and the
paths/00-manual/tooling.md dogfood entry still described the manifest fix itself as
filed rather than made. Found by self-review after the previous commit shipped exactly
that fix.

Co-Authored-By: Max <noreply>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant