Conversation
…ires: to degrade a tools rule that depends on an absent binary (#170, #203) #170: a whitespace-only advisory entry file read back as non-empty and injected a header with nothing under it -- content == "" was the guard for "nothing to say", and a file of blank lines is "\n", not "". jit_inject_text() now reports the entry is empty instead of staying silent, and pre-tool-hook.sh own #165 header-bound check is fixed to read the raw entry body rather than the transformed content. #203: a tools rule with mode: block (or require:/forbid:) had no way to say "and if this binary is not installed" -- filed against a downstream repo whose generated supertool-required.md blocked every file operation for a user without supertool installed. requires: <binary> is probed with command -v in bash, once, before the hook one awk process starts (awk here never shells out), and degrades a refusal to advisory when the named binary is not on PATH, naming what is missing in the injected text. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MNPrdageDpVsLFP8eL9VSj
…reader, and a changelog line citation Independent review (Explore + oss:auditor) found two real issues in the diff just committed for #170/#203: - scripts/jit-dry-run.sh's pre-existing `IFS=$'\''\t'\'' read -r r_tool ... r_forbid _rest` row reader suffers the identical bash tab-collapse bug jit_missing_requires() was written to avoid: a tools row with both require and forbid empty, followed by a non-empty requires: column (the exact shape #203 introduces and the README's own example uses), had the requires: value land in $r_require instead of $_rest, producing a false "this row can refuse" ADVISORY notice for an ordinary `remind` row. Fixed by pre-translating tabs to a single control byte before the read, same as jit_missing_requires() -- driven to STX (0x02) rather than SOH (0x01), because bash reserves 0x01 and 0x7f internally (CTLESC/CTLNUL) and a field delimited by either of those is silently not split at all, which would have been a worse and differently-invisible failure than the one being fixed. - changelog.d/170.fixed.md cited a line range (`test-inject-mode.sh:637-643`), which this repository own convention refuses (tests/test-line-citations.sh) for the reason the convention gives: it rots on the next unrelated insertion above it. Replaced with a greppable name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MNPrdageDpVsLFP8eL9VSj
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
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.
Closes #170
Closes #203
#170
An advisory (non-refusing)
toolsrule whose entry file holds only whitespace used to inject a header with nothing under it: the silence guard tested for the literal empty string, and a file of blank lines reads back as"\n", which is not"".jit_inject_text()inscripts/common.shnow reports[jit] The entry file has no text to inject.instead of returning the whitespace body verbatim, scoped to the specific case of a no-frontmatter (forced full-mode) entry whose body is non-empty-but-whitespace-only. A truly empty entry (a single blank line, reading back as the literal empty string) keeps resolving to""and stays silent -- that behaviour is existing, tested and deliberate, and the fix does not touch it.The #165 header-bound check in
pre-tool-hook.sh(which decides whether an advisory row's header is exempt from the summary-mode byte budget) had to move from testingcontentto testing the raw entrybody, becausecontentis no longer whitespace on the path this fix touches -- readingcontentthere would have wrongly exempted a report-only row from that budget.tests/test-inject-mode.sh'swsrequire.mdsection previously asserted the old bare-header behaviour and named it "pre-existing behaviour of an advisory rule and not what this change is about" -- a prior lane (#165) scoped this out on purpose, filing it separately as #170. This PR retires that scoping deliberately, in its own commit hunk.#203
A
toolsrule withmode: block(orrequire:/forbid:) had no way to say "and if this binary is not installed, do not block" -- filed by an external downstream maintainer against a generatedsupertool-required.md(not part of this repo; that half is filed upstream atDigital-Process-Tools/claude-oss#524) that blocked every file operation for anyone who installed this plugin without supertool.New frontmatter field
requires: <binary>, probed withcommand -vin bash, once, before the hook's single awk process starts -- the awk programs in this codebase never shell out (nosystem()anywhere, on purpose, since they parse untrusted JSON and untrusted index text), so the presence check could not live inside the row loop.scripts/common.shgainedjit_missing_requires();scripts/pre-tool-hook.shreads a new 7th TSV column and degrades amode: block/require:/forbid:refusal to advisory when the named binary is not on PATH, naming what is missing in the injected text.scripts/rebuild-tsv.shwrites the column;scripts/jit-dry-run.sh's staleness check was updated to match.Below-bar finding, recorded rather than fixed (from this PR's own self-review): the pre-existing containment gate at
pre-tool-hook.sh:429(if (!can_refuse) continue) reusescan_refuse, which is nowwould_refuse && !requires_missing. A row that is BOTH file-name-malformed ANDrequires_missingnever reaches the new degrade-note code, so the caller gets the pre-existing file-containment refusal notice but not also the requires:-degrade explanation. This is reachable only through a hand-edited or corrupted index row carrying two independent defects at once, and the caller is not left with silence -- the generic file-containment refusal still fires. Fixing it means deciding whether the #140 containment-continue should testwould_refuseinstead ofcan_refuse, which is a real question about that invariant's own scope and belongs to whoever owns it rather than a same-lane patch.Review
Self-review via Explore (no findings) and oss:auditor (3 findings) against the first commit. Two were fixed in a follow-up commit:
scripts/jit-dry-run.sh's own pre-existing tab-collapse bug in its row reader, exposed by #203's new column shape (falseADVISORY "this row can refuse"on an ordinaryremindrow); and a line-number citation in the #170 changelog fragment that this repo's owntests/test-line-citations.shrefuses. The third is the below-bar item above.Test plan
bash tests/run-all.sh-- 10/10 suites clean except the pre-existing, unrelatedtest-awk-locale-pins.shfailure (fails identically onorigin/main, confirmed by stashing this diff and rerunning)shellcheck -S warning scripts/*.sh tests/*.sh-- cleanpython3 .oss/assemble_changelog.py --check-- both fragments passtests/test-requires-field.sh, 19/19, drives every combination of mode:block / require: / forbid: / requires: presence-absence with paired controls