docs(source-control): scope exec-bit-check's content-determinism claim to the A/C arms - #2167
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Claude finished @kyle-sexton's task in 1m 2s —— View job Security review complete
FindingsNo security issues found. This PR is scoped entirely to documentation and test-fixture prose, confirmed by reading every changed hunk:
No GitHub Actions workflow files, permission grants, secrets handling, or dependency/action pins are touched by this PR, so the dangerous-pattern categories in scope for this review (injection, auth/access-control, token handling, |
|
Claude finished @kyle-sexton's task in 3m 2s —— View job Reviewing PR #2167
|
|
Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
|
Claude finished @kyle-sexton's task in 49s —— View job Security review complete
FindingsNo security issues found. This PR is scoped entirely to documentation and test-fixture prose, confirmed by reading every changed file at HEAD (
No GitHub Actions workflow files, permission grants, secrets/token handling, or dependency/action pins are touched, so the dangerous-pattern categories in scope for this lane (injection, auth/access-control, Note: this environment's Bash tool required approval for every invocation and none resolved, so I reviewed the changed files directly via the Read tool against the checked-out |
|
Claude finished @kyle-sexton's task in 3m 6s —— View job Reviewing PR #2167
|
Conflicts and how they were resolved: * block-no-verify.sh -- main's #2122/#2135 NUL-byte work rewrote the same comment this change rewrote. Both survive: the comment now says that rc 1 from hook::jq_fields means ONLY an unparsable payload, because a missing jq can no longer reach that line, and keeps main's note that the remaining allow-on-unparsable path is not what the NUL check covers. * every CHANGELOG.md and the two conflicting plugin.json files -- main's #2135 landed the same 16-plugin lib bump this change needs, so main's side was taken wholesale and this change's entry and bump were re-applied on top. Every carrying plugin is therefore strictly above what main now carries. * guardrails takes a MINOR bump (0.24.1 -> 0.25.0), not a patch: it now denies calls it previously allowed. * source-control goes to 0.51.7 rather than 0.51.6. Main's #2135 took 0.51.5, and 0.51.6 is reserved for PR #2167, which touches the same plugin and should merge first. lib/hook-utils.sh auto-merged (main's NUL handling plus this change's posture block); the 16 plugin copies were re-synced from it afterwards rather than trusted to the merge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…m to the A/C arms The rename arm's `100755`-source gate makes its answer depend on `diff.renames`, and it splits on git's DEFAULT setting: `git mv` of a `100644` shebang file reads as `D`+`A` under `diff.renames=false` and IS reported, while the same index and the same HEAD read as `R100` under `diff.renames=true` and are NOT. is real and pinned by `repo19`: a deliberately non-executable sourced library or template must not be flipped to `100755` because someone moved it. Dropping the gate buys config-agreement by shipping that false positive to every consumer; making the `A` branch match buys it by reporting less, risking silence on genuinely new files. What changes is the prose. Content-determinism is now stated as a property of the `A` and `C` classes only -- at the script header, at the candidate-set gate, in `--help`, in `reference/exec-bit.md`, and next to `repo19` -- never of the whole tool. New case group 19b pins both halves of the disagreement on one fixture repo, with HEAD and the index asserted identical across the two runs, so the decision is executable rather than only written down. It kills both rejected policies under mutation: dropping the rename gate fails the `renames=true` half, and making the `A` branch skip a rename-as-add fails the `renames=false` half. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
b038eae to
541a84c
Compare
|
Warning Automated review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
Re-run the job to retry the review. A new push re-triggers this lane only if the caller's |
|
Warning Automated security review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
The check is green on purpose, and it is not evidence. It certifies that a security pass ran, and this one did not complete — but the cause is outside this PR's control, so merging is deliberately left unblocked rather than locking every merge for the length of the outage. Nothing was reviewed at this head. Where this check is required, it is satisfied without that evidence; a human should review security-sensitive changes here before merging. Re-run the job to retry the review; a new push also retries it only if the caller's Re-running does NOT help for every class:
|
…peration guards (#2178) ## What `hook::require_jq` was `command -v jq && return 0`, else a once-per-session notice and **`exit 0`** — the whole hook skipped, the tool call proceeds. Measured against `origin/main`, with the jq-present column as the discrimination control: ``` jq PRESENT jq HIDDEN dangerous push DENY ALLOW <-- the guard was skipped entirely safe command ALLOW ALLOW ``` The same two scripts fail **closed** on the other input they cannot parse: above `MAX_COMMAND_LEN` (16384) a command is treated as obfuscation and blocked unread. Two opposite postures toward "I cannot read this input" in one file — so an author who could not fit a dangerous command under the ceiling could simply be on a machine without `jq`. ## The disposition: fail CLOSED, scoped to the irreversible-operation guards After (same harness, same machine): ``` block-dangerous-git.sh jq PRESENT jq HIDDEN dangerous push DENY DENY safe command ALLOW DENY block-no-verify.sh jq PRESENT jq HIDDEN commit --no-verify DENY DENY safe command ALLOW DENY posture control (unchanged) block-convention-violation.sh jq PRESENT=ALLOW jq HIDDEN=ALLOW block-hook-bypass.sh jq PRESENT=ALLOW jq HIDDEN=ALLOW block-noncanonical-commit.sh jq PRESENT=ALLOW jq HIDDEN=ALLOW ``` **The (safe, jq HIDDEN) = DENY cell is a real cost, not an oversight.** These guards run on every Bash/PowerShell tool call; without `jq` they cannot read the command at all, so they cannot tell a dangerous one from a safe one and deny both. On a `jq`-less machine every matched tool call is blocked until `jq` is installed or the guard's kill switch is set. That is the hard dependency option 2 named. Option 3 (a `jq`-free substring pre-check) was rejected and is not implemented. The kill switch is still a real exit: `hook::check_enabled` runs *before* the gate, so `block_dangerous_git_enabled=false` bypasses the guard on a `jq`-less machine. Asserted. ## Which hooks are in the class — mechanical, not a taste judgement The criterion is **internal consistency**: a hook is fail-closed iff it *already* fails closed on another unparsable-input condition. Today that is a `MAX_COMMAND_LEN` ceiling, and **repo-wide that is exactly two files** — `block-dangerous-git.sh` and `block-no-verify.sh`, the two the issue names. That is not a coincidence: intra-script contradiction is what #2146 reports, and a script with no length-ceiling posture has no contradiction to resolve. **Considered and deliberately excluded**, so this is not a silent scoping choice: | Hook | Why not | | --- | --- | | `block-hook-bypass` | exits 2 and carries the same "the only supported deliberate bypass is the kill switch" sentence — but it guards a **file write** (`cat > path`), trivially reversible, and has no length ceiling | | `block-noncanonical-commit` | guards a message *shape*; a mangled message is recoverable by `--amend` | | `secret-pattern-detection`, `hardcoded-path-check`, `block-convention-violation` | all guard reversible file writes | | other-plugin blocking `require_jq` callers (`context-guard/zone-gate`, `source-control/pr-*-gate`, `autonomy/lane-stop-gate`) | checked repo-wide; **none** carries a length ceiling | Severity is a slope; "already fails closed elsewhere in the same script" is a line. `require-jq-posture.test.sh` pins the membership both ways, so a hook that grows a ceiling and keeps the fail-open gate fails, and so does a hook that adopts the blocking gate without one. ## Helper design: a sibling function, not a parameter `hook::require_jq_blocking` alongside the unchanged `hook::require_jq`. **Why not a flag on the existing function:** a parameter's *omitted* value has to default to something, and the safe-looking default (fail open, today's behaviour) means a guard that should fail closed but whose flag someone forgot fails open **silently** — which is the exact defect this PR fixes, reintroduced at the API. Two names make the posture greppable, make the fail-closed path impossible to reach by accident, and make omission a visible choice. **Why not branch at the call sites:** the issue's own acceptance says the reasoning belongs at the helper, and a call-site branch leaves the decision point still unexplained. (It also would not have avoided the 16 plugin bumps: `sync-hook-utils.sh --check-bump` is content-based, so even a comment-only lib edit requires them.) **The reasoning is at the helper.** One `TWO POSTURES, AND WHY THERE ARE TWO` block sits above both functions — why fail-open is the default, why a minority must not be, the membership criterion, the exclusions, the disclosed cost, and why two functions rather than a flag. The call-site comments now say "this asserts the behaviour; that explains it", and the posture test asserts the block is actually there. ## The control that FAILS against current `main` `require-jq-posture.test.sh` was run **unchanged against `origin/main`'s guardrails plugin** (`git archive origin/main plugins/guardrails`, hashes verified equal to `origin/main`'s blobs): ``` FAIL: block-dangerous-git.sh defines MAX_COMMAND_LEN but does not call hook::require_jq_blocking FAIL: block-no-verify.sh defines MAX_COMMAND_LEN but does not call hook::require_jq_blocking FAIL: hook-utils.sh's posture block mentions 'TWO POSTURES' FAIL: jq HIDDEN, dangerous push: DENY ... : expected 'DENY', got 'ALLOW' FAIL: jq HIDDEN, commit --no-verify: DENY ... : expected 'DENY', got 'ALLOW' ... PASS=21 FAIL=15 SUITE EXIT: 1 ``` with the four cells against `main` reproducing the issue's table exactly (`DENY/ALLOW` over `ALLOW/ALLOW`). Against this branch: **PASS=36 FAIL=0**. ## How `jq` was hidden — and how that measurement was kept honest A `BASH_ENV` file defines a `command` shell function that reports `jq` absent and forwards every other lookup to the real builtin, plus a `jq` function that fails like a missing binary. **`PATH` is untouched.** Stripping `PATH` directories also removes `git`, which these guards invoke, and a guard that cannot find `git` produces the same ALLOW for an entirely unrelated reason. The suite prints a precondition line measured **inside** the hidden environment and refuses to read a verdict until it holds: ``` PRECONDITION (measured inside the jq-hidden environment): jq=hidden git=visible bash=visible path-to-jq=intact ``` `path-to-jq=intact` is `builtin command -v jq` still resolving — proof the **lookup** was hidden and the tool was not removed. Every `jq` probe in `hook-utils.sh` is a `command -v jq` (verified: 7 sites, all of that form), so the override reaches all of them. A second check sources the real `hook-utils.sh` inside the hidden environment and asserts the gate's own predicate sees no `jq`, plus the inverse without the override. **The harness caught itself once.** The first `origin/main` run archived only `plugins/guardrails/hooks`, so `block-dangerous-git.sh` could not source its bundled PowerShell classifier from `<plugin-root>/lib` and exited early — producing `ALLOW` in **all four** cells, including `jq PRESENT / dangerous push`. The jq-present discrimination control is what flagged it as a broken harness rather than a measured result. Fixed by archiving the whole plugin. This is the failure mode the issue says invalidated three prior attempts. ## Proof the fixture reached the path under test Under `jq` hidden, the denial is asserted to be the **new** path and not some unrelated failure: - names `jq` as the missing prerequisite; - carries the documented install route `https://jqlang.org/download/`; - names the guard's own kill switch (`block_dangerous_git_enabled` / `block_no_verify_enabled`); - is **not** the fail-open skip notice (`hook skipped for this session` asserted absent). And the advisory control asserts the inverse — that the fail-closed denial text is absent from every advisory hook's stderr. ## Blast radius `sync-hook-utils.sh --check-bump` requires every carrying plugin to bump when the shared lib changes, so all 16 are bumped with a CHANGELOG entry (the precedent set by b20e70a / #2147). The 15 non-guardrails entries state honestly that the lib gained a fail-closed sibling with **no behaviour change in that plugin** — nothing outside `guardrails` calls it. `guardrails` takes a **minor** bump (`0.24.1` → **`0.25.0`**), not a patch: it now denies calls it previously allowed. `origin/main` moved under this branch mid-flight — #2135 landed the *same* 16-plugin lib bump for its NUL-byte fix, so every version collided. Resolved by taking main's side of every CHANGELOG and manifest wholesale and re-applying this change's entry and bump on top, so main's entries survive intact and every plugin here is strictly above what main now carries. `block-no-verify.sh` conflicted textually on the very comment both changes rewrote; both survive (see the merge commit message). `require-jq-notice-isolation.test.sh` needed one adjustment: its discovery matched `hook::require_jq` as a substring and so would have swept in `hook::require_jq_blocking`, whose callers have no notice key to collide. The match is now anchored, and the test's own subject is documented as not applying to the blocking gate. ## Verification run on this branch | Check | Result | | --- | --- | | `require-jq-posture.test.sh` (new) | PASS=36 FAIL=0 | | same suite vs `origin/main` | PASS=21 **FAIL=15**, exit 1 | | `require-jq-notice-isolation.test.sh` | PASS=2 FAIL=0 | | `block-no-verify.test.sh` | see CI | | `block-dangerous-git.test.sh` | see CI | | `scripts/sync-hook-utils.sh --check` / `--check-bump origin/main` | pass | | `scripts/check-changelog-parity.sh --check` / `--check-bump origin/main` | pass | | `scripts/check-silent-skips.sh` | pass | | `scripts/check-shell-portability.sh --paths <changed>` | pass | | `shellcheck -x -S warning <changed>` | clean | ## Merge-order note This PR and #2167 **both bump `plugins/source-control`** (this one because it carries the shared `hook-utils.sh`). Main is at `0.51.5`; #2167 now claims `0.51.6` and this PR claims **`0.51.7`**, so it stays strictly greater either way. **Merge #2167 first** — it is one plugin and cheaper to redo. If this one lands first instead, #2167 must re-bump to `0.51.8`. Closes #2146 ## Related - #2124 / #2147 — a separate live bypass in the same guard, and the precedent for a 16-plugin lib bump - #2145 — the contract-line inaccuracy in the same file - #1938 — the stranded post-merge review-findings sweep - #2167 — the other PR in this pair; collides with this one on the `source-control` version bump --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
… per_page=100 on every paginated read (#2262) Closes #2244 ## Summary - **D6 verify-commit-pushed** (`reference/review-discipline.md` and `skills/pull-request/SKILL.md` — the SKILL.md site *is* present on current `main` at line 181, contrary to the issue's grep against older `c1b4c629`): replaces the branch-tip read `commits?sha=<branch>&per_page=1` + `--jq '.[0].sha'` with the single-resource presence read `repos/{owner}/{repo}/commits/<fix-sha>`. The tip form asks a presence question but answers a tip question — any push after the fix (follow-up commit, rebase, sibling lane) makes it report the fix missing while present: a false negative on a control gate, and a positional index on a list, which `readiness.md` rule 2 forbids six lines above D7. The single-resource form echoes the SHA on exit 0 when present and fails HTTP 422 (`No commit found for SHA`) when absent — index-free, identity-bound, cannot be satisfied by the wrong record. - **#2246 (source-control rows only)**: adds `per_page=100` to the six remaining `--paginate` sites in this plugin: `merge.md:14-16`, `SKILL.md:166-168` (C1–C3), `monitor.md:195`, `fetch-all-pr-comments.sh:141`, `telemetry-upsert.md:39`. Not truncation defects — `--paginate` alone is complete — but non-conformant with rule 1 as `readiness.md:55` publishes it, at 3.3x the request cost. - **Deliberately unchanged**: `babysit_gh.py:441` and `request_review.py:186` from the #2246 re-sweep are false positives — the `per_page=100` sits in the endpoint URL on the line adjacent to the `--paginate` flag the line-based sweep matched (`request_review.py:185`; every `fetch_paginated_api` caller URL at `babysit_gh.py:459/478/523/691`). The other-plugin rows (claude-ops, work-items) belong to the parallel lane, not this PR. - Version 0.51.11 + CHANGELOG section (renumbered above the 0.51.10 that #2167 landed on main mid-flight). ## Test plan - Live verification of the replacement form in both directions on this repo: `commits/33f0df51…` → echoes the SHA, exit 0; `commits/0000…0000` → HTTP 422, exit 1. - `plugins/source-control/scripts/fetch-all-pr-comments.test.sh` — all 23 checks pass (the three endpoints the script pages carry no existing query string, so `?per_page=100` composes safely). - `scripts/check-changelog-parity.sh --check` and `--check-bump origin/main` — both pass. - `markdownlint-cli2` on all six changed markdown files — 0 issues. - Conflict-marker sweep (all four forms) — clean. ## Related - #2244 (closes) - #2246 (source-control rows; claude-ops/work-items rows remain for the parallel lane) - #2163 / #2238 / #2239 — prior instances of the same defect class Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
What
exec-bit-check.sh's rename arm reports a newly-staged100644shebang file or not depending onthe consumer's
diff.renamessetting, and it splits on git's default:Index and HEAD are identical across those two runs. Only the config differs.
The disposition: KEEP the gate. No behaviour change.
repo19pins a real false positive thesrc_mode == "100755"gate prevents — a deliberatelynon-executable sourced library or template must not be flipped to
100755because someone moved it.The two alternatives were weighed and rejected:
diff.renamesrepo19false positive to every consumerAbranch skip a rename-as-addSo this is a documentation change, and #2141 closes as a decision recorded — not as a code fix.
What actually changed
Content-determinism is now stated as a property of the
AandCclasses only, never of thewhole tool. Every surface was checked, not just the expected one:
exec-bit-check.shheaderWHAT IT DOES NOT CLAIMblockR --arm)THE ACCEPTED TRADE (#2141)at the gate--help/usage()reference/exec-bit.mdexec-bit-check.test.shatrepo19plugins/source-control/CHANGELOG.md0.51.5entry records the decisionSKILL.md0.49.3/0.51.4#2140had already scoped the universal claim to the copy arm in the test file's group 21b. Theresidual was in the script header, the gate comment,
--help, andexec-bit.md.New case group 19b — the decision, executable
One fixture repo, run twice with nothing changing but the
diff.renameskey, asserting thedeliberate disagreement:
HEAD and index trees are asserted equal to each other across the runs. Without that, the case
would prove only that two different repositories differ.
Disclosed inversion of the issue's stated acceptance. #2141 asked for "a case asserting the
diff.renames=falseanddiff.renames=trueanswers agree". Under "keep the gate" they do notagree, so this case asserts the opposite — deliberate disagreement — and says so in its own comment.
Naming that here rather than leaving it to be discovered.
The control, stated honestly
No behaviour test can fail against unmodified
main, because the disposition is no behaviourchange. That is stated plainly rather than shipped as a green test implying otherwise.
The available substitute is mutation: the case group is run against the two rejected policies,
implemented as patched copies of the script. Real output:
Each mutant is killed in the predicted direction, on a different half, and mutant 2 still reports
zz-dis-extra.sh— so it is a faithful policy-3 mutant, not a blanket skip.Proof the fixture reached the path under test
It did not, at first — and that is worth stating.
Group 19b passed standalone and skipped silently inside the full suite. Cause:
mkrepoincrements
REPO_SEQinside a command substitution, so the increment never reaches the caller andevery fixture in this suite is handed the same repository path. Each fixture's own unqualified
git commitclears the previous one's staged set, which is why it has gone unnoticed — but filenames persist, and my
lib.shcollided withrepo19's, sogit mvreturnedfatal: destination existsinside the>/dev/null 2>&1subshell.Fixed by prefixing this group's paths
dis-, and theskip_casenow reports what it saw ratherthan only that it gave up — which is what surfaced this. Final full-suite run: 76 cases, 0
failures, with all six 19b cases running (
[60]–[65]), not skipped.Follow-up worth filing separately (not fixed here, to keep this change documentation-scope): the
REPO_SEQbug means the suite's ~24 fixtures share one repository rather than being isolated. Thatis a latent hazard for any future fixture, and possibly weakens existing ones.
Merge-order note
This PR and the #2146 PR both bump
plugins/source-control(#2146 touches the sharedlib/hook-utils.sh, which--check-bumprequires every carrying plugin to bump). Whichever mergessecond needs a fresh
origin/mainmerge plus a re-bump of that plugin's version and CHANGELOG.Closes #2141
Related
the property, and 19b is its deliberate inverse
R*/C*gate was introducedsource-controlversion bump