fix(plugin): the hooks need no jq and no tac (#4107)
CI & Build / Python lint (push) Successful in 2s
CI & Build / Plugin hooks (push) Successful in 9s
CI & Build / integration (push) Successful in 49s
CI & Build / TypeScript typecheck (push) Successful in 57s
CI & Build / Python tests (push) Failing after 1m7s
CI & Build / Build & push image (push) Skipped
CI & Build / Python lint (push) Successful in 2s
CI & Build / Plugin hooks (push) Successful in 9s
CI & Build / integration (push) Successful in 49s
CI & Build / TypeScript typecheck (push) Successful in 57s
CI & Build / Python tests (push) Failing after 1m7s
CI & Build / Build & push image (push) Skipped
Every hook opened `command -v jq >/dev/null 2>&1 || exit 0`, so on a machine without jq the operator got no session context, no rules, no prior art and no process sync — and not one word saying why, because `exit 0` is indistinguishable from "ran fine, nothing to say". jq is absent by default on macOS, on the Debian/Ubuntu slim images, on Alpine and in most CI containers. That is not a prerequisite to document; it is the plugin handing its own packaging problem to whoever installs it. `tac` was worse: GNU-only, so the prior-art hook's enclosing-definition arm did nothing at all on every Mac, silently, from the day it shipped. It is not replaced but removed — scribe_defs judges each line independently, so extracting forward and taking `tail -1` is the same answer as reversing and taking the head, and it drops the early-exit `head` that #4042 was filed for. No server contract changed, so a lagging plugin cache keeps working. scribe_json.awk JSON -> IDX<TAB>PATH<TAB>VALUE. Two modes: `whole` for an event or a response body, `lines` for a transcript, where an unparseable record is dropped and the rest still read — the `map(try fromjson catch empty)` the jq program opened with. Arrays also report their LENGTH at `[#]`, which is what keeps "zero notes" distinct from "no answer" (#2932). scribe_turn.awk the turn-bounding program, replacing the thirty lines of jq in the Stop hook. scribe_defs.sh scribe_json_flat / _pick / _list / _len / _list_minus read, scribe_json_out writes the envelope (five copies of one shape, gone), scribe_urlenc replaces `jq -sRr '@uri'`. Percent-encoding goes through `od -tu1` rather than an awk character loop on purpose: awk's idea of a character follows the locale, so gawk reads an accented letter as one and mawk as two, and an encoder built on substr() would emit a different URL depending on which awk is installed. Encoding is defined on bytes. Verified byte-identical to `jq -sRr '@uri'`. Measured, not assumed. The per-event path costs 8ms against jq's 3ms. The transcript path was 70x slower until two fixes: the Stop hook now finds where the turn starts with a fixed-string grep before parsing (a needle carrying unescaped quotes cannot occur inside a JSON string, so it matches only at a record's top level — checked against a full JSON parse of a 27MB transcript: 152 prompt records, 152 matches, no misses, no extras), and the parser reads each token out of a 1024-byte window instead of copying the rest of the buffer per token, which was quadratic in line length on the 400KB tool results a transcript carries. Differential-tested against the jq program it replaces over 724 windows cut from three real transcripts — 724 identical, 0 mismatched, 45 of them exercising a real task close and a real reply. That sweep is what caught `scribe_turn.awk` never setting FS, which truncated every multi-word reply at its first space and was invisible to a test whose replies were all empty. check_plugin.py's `jq -R` lint becomes a guard against either binary coming back, and three smoke checks lose their `shutil.which("jq")` skip. jq is not in `ci-python` either, so those three announced a skip on every CI run and had never once run there: removing the dependency from the product also closed a permanent hole in its verification. They pass now across all ten hooks. tests/test_hook_json_reader.py is a differential against Python's `json` over nested objects, arrays, unicode, escapes, control characters, empty cases and a value longer than the token window, plus the envelope, the encoder and the turn analyzer. 139 cases. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy
This commit is contained in:
+42
-29
@@ -28,18 +28,23 @@ forgetting to run it is still possible. What changed is that forgetting is now
|
||||
LOUD — a red lane on the batch that forgot, instead of a silent no-op found
|
||||
weeks later when somebody says "I don't think it updated" (#2220).
|
||||
|
||||
shellcheck and jq are NOT in `ci-python` (verified against CI-runner's Dockerfile
|
||||
and scripts/install-common.sh, not from memory — rule #37). CI installs both
|
||||
per-job, which is what CI-runner's own docs/process.md prescribes for a dep with
|
||||
a single consumer: "If only one project needs the dep, prefer that project
|
||||
shellcheck is NOT in `ci-python` (verified against CI-runner's Dockerfile and
|
||||
scripts/install-common.sh, not from memory — rule #37). CI installs it per-job,
|
||||
which is what CI-runner's own docs/process.md prescribes for a dep with a
|
||||
single consumer: "If only one project needs the dep, prefer that project
|
||||
installing it per-job in their workflow — at least until a second consumer
|
||||
arrives." Promotion into the image is filed as an issue there rather than
|
||||
assumed here.
|
||||
|
||||
Both are optional at runtime: without shellcheck the lint step is SKIPPED and
|
||||
says so, and without jq the smoke test is skipped. A skipped check announces
|
||||
itself loudly, because a check that quietly no-ops is the failure mode this
|
||||
whole file exists to prevent.
|
||||
It is optional at runtime: without shellcheck the lint step is SKIPPED and says
|
||||
so. A skipped check announces itself loudly, because a check that quietly
|
||||
no-ops is the failure mode this whole file exists to prevent.
|
||||
|
||||
jq USED TO BE IN THAT SAME SENTENCE, and the smoke tests below skipped
|
||||
themselves without it. Since jq is not in `ci-python` either, that meant three
|
||||
of them announced a skip on every CI run and had never actually run there. The
|
||||
hooks need no jq now (#4107), so those checks run unconditionally — removing a
|
||||
dependency from the product removed a permanent hole in its verification.
|
||||
|
||||
Usage:
|
||||
python3 scripts/check_plugin.py # all checks
|
||||
@@ -197,16 +202,24 @@ PATTERNS: list[tuple[re.Pattern, str, str]] = [
|
||||
"hook then does nothing, silently.",
|
||||
),
|
||||
(
|
||||
# -R without -s: reads input line by line, so a multi-line payload is
|
||||
# encoded per line and joined with raw newlines. The class is a-r + t-z
|
||||
# (i.e. every letter EXCEPT `s`) so `-rR` is caught and `-sRr` is not —
|
||||
# an earlier a-q spelling silently excluded `r` and missed the real
|
||||
# defect, which is exactly the flag combination that shipped.
|
||||
re.compile(r"jq\s+-(?:[a-rt-zA-Z]*R[a-rt-zA-Z]*)\s"),
|
||||
"line-oriented jq -R",
|
||||
"jq -R reads input LINE BY LINE. Encoding a multi-line payload that way "
|
||||
"produces separate encoded lines joined by raw newlines — an invalid "
|
||||
"URL. Use -s (slurp) as well, e.g. `jq -sRr '@uri'`.",
|
||||
# NEITHER OF THESE MAY COME BACK (#4107). This replaced a narrower rule
|
||||
# about `jq -R` being line-oriented, which is now moot in the only way
|
||||
# that rule could become moot: there is no jq left to pass flags to.
|
||||
#
|
||||
# The wider rule is the one worth having. jq is absent by default on
|
||||
# macOS, on the Debian/Ubuntu slim images, on Alpine and in most CI
|
||||
# containers; `tac` is GNU-only and absent on macOS. Every hook used to
|
||||
# guard itself with `command -v jq || exit 0`, so a machine without it
|
||||
# got no context, no rules, no prior art and no process sync, silently —
|
||||
# `exit 0` is indistinguishable from "nothing to say". A hook may use
|
||||
# only what POSIX guarantees; scribe_defs.sh has the readers.
|
||||
re.compile(r"(?<![\w./-])(jq|tac)(?![\w./-])"),
|
||||
"jq or tac in a hook",
|
||||
"A hook may depend only on what POSIX guarantees — jq is not installed "
|
||||
"by default anywhere the plugin is likely to land, and tac is GNU-only. "
|
||||
"Read JSON with scribe_json_flat / _pick / _list / _len, write it with "
|
||||
"scribe_json_out, encode with scribe_urlenc; to take the LAST match of "
|
||||
"a filter, pipe to `tail -1` instead of reversing with tac (#4107).",
|
||||
),
|
||||
(
|
||||
# `$(producer | head -N) || var=""` — the fallback OUTSIDE the
|
||||
@@ -398,13 +411,13 @@ def _run_hook(script: Path, event: str, env_extra: dict[str, str]) -> subprocess
|
||||
|
||||
|
||||
def check_fail_open() -> None:
|
||||
if not shutil.which("jq"):
|
||||
# Without jq every hook bails at its first line, so this would pass
|
||||
# while exercising nothing. Say so rather than bank a green tick.
|
||||
skip("jq not installed — the hooks would exit at line 1, so this "
|
||||
"check would pass without testing anything")
|
||||
return
|
||||
|
||||
# No jq gate any more (#4107). This check used to skip itself when jq was
|
||||
# missing, because without it every hook bailed at line 1 and the check
|
||||
# would have passed while exercising nothing. jq is NOT in `ci-python`, so
|
||||
# what that actually meant is that this smoke test announced a skip on
|
||||
# every CI run and never once ran there. The hooks now need only POSIX
|
||||
# tools, so it runs everywhere — which is the point of the change it is
|
||||
# testing.
|
||||
scenarios = [
|
||||
("unconfigured", {}),
|
||||
# Connection refused immediately — exercises the unreachable-instance
|
||||
@@ -467,8 +480,8 @@ def check_local_prior_art_needs_no_instance() -> None:
|
||||
nothing to say, and speaking when there is — both with no instance at all.
|
||||
"""
|
||||
script = HOOKS_DIR / "scribe_prior_art.sh"
|
||||
if not script.is_file() or not shutil.which("jq"):
|
||||
skip("prior-art local arm: hook or jq missing")
|
||||
if not script.is_file():
|
||||
skip("prior-art local arm: hook missing")
|
||||
return
|
||||
|
||||
# A definition this repo really does contain, written into a DIFFERENT file
|
||||
@@ -510,8 +523,8 @@ def check_session_context_reports_its_version() -> None:
|
||||
would be missing exactly when it is wanted.
|
||||
"""
|
||||
script = HOOKS_DIR / "scribe_session_context.sh"
|
||||
if not script.is_file() or not shutil.which("jq"):
|
||||
skip("version marker: hook or jq missing")
|
||||
if not script.is_file():
|
||||
skip("version marker: hook missing")
|
||||
return
|
||||
|
||||
manifest_v = manifest_version()
|
||||
|
||||
Reference in New Issue
Block a user