fix(hooks): add jq type-confusion matrix test, close two origin-hook gaps by tvna · Pull Request #1314 · tvna/gitapex

tvna · 2026-08-25T09:54:51Z

Summary

Adds hooks/test_gitapex_jq_type_confusion_matrix.py, a shared parametrized test running the full jq falsy/type-confusion value matrix against every (.field == null) or (.field | type == "X")-shaped guard clause across the six hooks issue #1218 names, per #1237's own retrospective proposal. Re-verifying #1312's own drafted Acceptance Criteria Map against the real hooks found two of those six (check-pr-issue-acm-disclosure.sh, check-pr-title-convention.sh) still fail closed on only 6 of the 8 matrix values -- independently tracked as #1216/#1217 -- so this PR also applies both issues' own already-specified, already-proven fix in the same change, rather than shipping a matrix test that documents a live gap instead of closing it.

Facts

Issue chore(retrospective): merge retrospective for PR #1213 #1237 (retrospective for PR fix(hooks): fail closed on missing/malformed jq in four PreToolUse gates #1213) proposed "a parametrized property test enumerating jq's full falsy/type-confusion matrix (absent, null, false, true, 0, "", array, object) against every (.field == null) or (.field | type == "X") guard in hooks/*.sh, run in CI" as the systemic gate that would have caught PR fix(hooks): fail closed on missing/malformed jq in four PreToolUse gates #1213's four separate adversarial-review repairs in one pass. Issue hooks/*.sh: add CI-enforced jq falsy/type-confusion matrix test for guard clauses #1312 requests exactly that.
Re-verifying hooks/*.sh: add CI-enforced jq falsy/type-confusion matrix test for guard clauses #1312's own drafted ACM (per planning-a-branch-from-an-issue's Step 5) against the live hook source, rather than trusting the draft: Row 1's Proof method requires the new test to "confirm it passes now" against current main. Live reproduction this session found that untrue for two of the six hooks Row 1 itself names:
- tool_name: 0 and tool_name: ["mcp__github__create_pull_request"] both silently fall through as "not our tool" (exit 0) on both check-pr-issue-acm-disclosure.sh and check-pr-title-convention.sh -- neither hook has a tool_name-type guard at all.
- tool_input: false crashes both hooks with jq's own exit 5 ("Cannot index/check boolean ..."), past deny(), non-blocking per Claude Code's own PreToolUse contract -- both hooks still carry the pre-PR#1213 predicate (.tool_input // {}) | type == "object", which treats JSON false the same as null.
- Both gaps are independently tracked as issues hooks/: check-pr-issue-acm-disclosure.sh and check-pr-title-convention.sh fail open on tool_input: false (same class as #1208) #1216 (tool_input: false) and hooks/: check-pr-issue-acm-disclosure.sh and check-pr-title-convention.sh fail open on a non-string tool_name (same class as #1208) #1217 (non-string tool_name), filed 2026-08-18, each already carrying a complete ACM proposing the identical fix PR fix(hooks): fail closed on missing/malformed jq in four PreToolUse gates #1213 used on the other four hooks.
Re-verification marker and full correction note posted to issue hooks/*.sh: add CI-enforced jq falsy/type-confusion matrix test for guard clauses #1312's own body (see its "Re-verification note" section): hooks/*.sh: add CI-enforced jq falsy/type-confusion matrix test for guard clauses #1312
.gitapex/ssot.json's pr-issue-acm-disclosure and pr-title-convention gate entries had no rule text disclosing any fail-closed jq guard, unlike the four already-fixed hooks' own entries -- updated to match, per PR fix(hooks): fail closed on missing/malformed jq in four PreToolUse gates #1213's own retrospective precedent (repair 9) of keeping registered rule text current with the guards a hook actually enforces.

Assumptions

None load-bearing: this PR's scope decision (fixing hooks/: check-pr-issue-acm-disclosure.sh and check-pr-title-convention.sh fail open on tool_input: false (same class as #1208) #1216/hooks/: check-pr-issue-acm-disclosure.sh and check-pr-title-convention.sh fail open on a non-string tool_name (same class as #1208) #1217 alongside hooks/*.sh: add CI-enforced jq falsy/type-confusion matrix test for guard clauses #1312 rather than narrowing hooks/*.sh: add CI-enforced jq falsy/type-confusion matrix test for guard clauses #1312's own scope) follows directly from Row 1's own stated Proof method plus live reproduction, not from an inferred preference.

Skill audit evidence

deterministic-gate-quality: RAN -- evaluating-deterministic-gate-quality run (isolated subagent dispatch, no CLAUDE.md in context) against both hooks/check-pr-issue-acm-disclosure.sh and hooks/check-pr-title-convention.sh, read against skills/evaluating-deterministic-gate-quality/references/dimensions.md including dimension 15 (fail-closed default on malformed input). Verdict: both hooks well-formed and well-placed. The review live-tested the actual guard behavior (not just read the diff) against synthetic payloads on both the fixed and pre-fix script bodies: pre-fix, tool_name:0/array exited 0 (silent allow) and tool_input:false crashed with jq's own exit 5 on both hooks; post-fix, both cases correctly deny (exit 2) and the valid-call happy path still exits 0/2 as expected. One named minor gap (dimension 12): the (.field == null) or (.field | type == "X") predicate is hand-duplicated across six hook files, mitigated (not eliminated) by the new shared hooks/test_gitapex_jq_type_confusion_matrix.py regression test rather than a single-source predicate -- issue hooks/: extract the guard prologue into a shared sourced helper (duplication caused #1216 and #1217) #1218 tracks the eventual shared-helper consolidation.
defeat-test-disclosure: RAN -- test_denied_when_tool_name_is_not_a_string (added to both hooks' own test files) and the false case added to test_denied_when_tool_input_is_not_an_object (and the new hooks/test_gitapex_jq_type_confusion_matrix.py) were each written and confirmed failing against the pre-fix hook bodies before the fix landed (TDD red: 12 failures), specifically to defeat the old guards rather than exercise their happy path -- not merely happy-path coverage.

Acceptance Criteria Map

Criterion	Interpretation	Planned ops	Proof method	Residual risk
Every currently-known `(.field == null) or (.field	type == "X")`guard clause in`hooks/*.sh` is covered by the full 8-value matrix, run in CI	Per #1237: `tool_name`-type and `tool_input`-shape guards across all six hooks issue #1218 names, plus the `.tool_input.command`/`.tool_input.file_path` leaf guards	Added `hooks/test_gitapex_jq_type_confusion_matrix.py` (a shared canonical value matrix + per-hook guarded-field registry, run via subprocess against each real hook script); no new CI job needed -- `hooks` is already on `pyproject.toml`'s `testpaths`	`uv run --frozen pytest hooks/test_gitapex_jq_type_confusion_matrix.py -q`: TDD red confirmed first (12 failures, all against the two origin hooks' `tool_name`/`tool_input` guards) before the fix below; green after the fix (96 passed)
The two origin hooks (`check-pr-issue-acm-disclosure.sh`, `check-pr-title-convention.sh`) must actually fail closed across the full matrix, not just today's already-fixed four	Apply the exact predicate PR #1213 already proved correct: `(.tool_name == null) or (.tool_name	type == "string")`before`tool_name`extraction,`(.tool_input == null) or (.tool_input	type == "object")`replacing the falsy-gap-prone`(.tool_input // {}) \| type == "object"`	Edited both `.sh` files; added matching `pytest.mark.parametrize` regression cases to each hook's own existing `test_gitapex_check_*.py` file (closes #1216, #1217 per those issues' own stated Acceptance Criteria); refreshed both hooks' `.gitapex/ssot.json` `rule` text
The gate must extend to a new guard clause added later, not just today's known set	Full automatic coverage requires either source-parsing (out of scope, per #1312's own Non-goals) or issue #1218's shared-helper consolidation	None required by this PR	Not applicable -- scope boundary, not a build	Until #1218 lands, a sixth or later hook that hand-rolls its own guard clause instead of reusing a shared helper would not automatically gain matrix coverage from this gate alone -- an accepted, disclosed gap, not a silent one

Risk / blast radius

The two hook fixes are the exact, already-proven predicate PR fix(hooks): fail closed on missing/malformed jq in four PreToolUse gates #1213 shipped on four other hooks -- same construction, same failure mode closed, low risk per hooks/: check-pr-issue-acm-disclosure.sh and check-pr-title-convention.sh fail open on tool_input: false (same class as #1208) #1216/hooks/: check-pr-issue-acm-disclosure.sh and check-pr-title-convention.sh fail open on a non-string tool_name (same class as #1208) #1217's own residual-risk assessment.
check-pr-issue-acm-disclosure.sh and check-pr-title-convention.sh gate every mcp__github__create_pull_request/update_pull_request call; a regression here would block PR creation/updates repository-wide. Covered by the full existing per-hook test suites (both still 100% passing) plus the new matrix test.
One unrelated, pre-existing failure observed in this sandbox's full-suite run: tests/test_gitapex_scan_harden_checkout_pin_drift.py::test_repository_workflows_are_drift_free, caused by this container's own shallow git clone (git rev-parse --is-shallow-repository returns true; the gate correctly refuses to resolve a shallow-boundary commit as authoritative). Real CI checks out with fetch-depth: '0' (full history) per .github/actions/harden-checkout, so this does not reproduce there. Not touched by this diff.

Rollback

Revert this commit; both hook .sh files and their test suites return to the pre-PR-#1213-parity state (four hooks fixed, two not), and the new matrix test file is removed. No schema/data migration involved.

Verification

uv run --frozen pytest hooks/test_gitapex_jq_type_confusion_matrix.py hooks/test_gitapex_check_pr_issue_acm_disclosure_shell.py hooks/test_gitapex_check_pr_title_convention.py -q --no-cov -> 143 passed
uv run --frozen pytest hooks/ -q --no-cov -> 383 passed
uv run --frozen pytest --cov-report=xml --cov-report=json (full suite, mirrors CI's test.yml) -> 5619 passed, 1 failed (pre-existing, environment-caused shallow-clone issue documented above, unrelated to this diff)
uv run --frozen python3 .github/scripts/gitapex_scan_ssot_schema.py -> "No ssot.json drift found."
bash -n hooks/check-pr-issue-acm-disclosure.sh && bash -n hooks/check-pr-title-convention.sh -> syntax OK
uv run --frozen ruff check / ruff format --check / mypy / betterleaks (pre-commit hooks) -> all passed on commit

Checklist

Tests pass locally
Docs updated if behavior changed -- N/A, no user-facing docs describe these hooks' internal guard predicates beyond .gitapex/ssot.json's own rule text, which is updated
Issue number cited in every commit
Skill audit evidence -- required because this diff changes registered deterministic gates (.gitapex/ssot.json, hooks/check-pr-issue-acm-disclosure.sh, hooks/check-pr-title-convention.sh); see the ## Skill audit evidence section above
Transfer-check disclosure -- N/A, no evals/*/split.md Kept-edit-log entry added
Skill branch/fixture coverage -- N/A, no SKILL.md Stop-boundary bullets or dispatch branches added

Related Issue

Closes #1312, Closes #1216, Closes #1217

…gaps Adds hooks/test_gitapex_jq_type_confusion_matrix.py: a shared, parametrized test running the full jq falsy/type-confusion value matrix (absent, null, false, true, 0, "", array, object) against every `(.field == null) or (.field | type == "X")`-shaped guard clause across the six hooks issue #1218 names, per #1237's own retrospective proposal for PR #1213. Re-verifying issue #1312's own drafted ACM against the real hooks found its Proof method ("passes now" against current main) did not hold: two of the six named hooks, check-pr-issue-acm-disclosure.sh and check-pr-title-convention.sh, still lack a tool_name-type guard entirely and still carry the pre-PR#1213 tool_input-shape predicate that fails to reject `tool_input: false` -- live-confirmed, and independently tracked as issues #1216/#1217. Both are fixed here with the exact predicate PR #1213 already proved correct, so the new matrix test genuinely passes across all six hooks rather than documenting a live gap. .gitapex/ssot.json's rule text for both hooks' gates is updated to disclose the new guards, matching the four already-fixed hooks' own convention. Closes #1312, Closes #1216, Closes #1217

coderabbitai · 2026-08-25T09:55:00Z

Important

🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: c4404970-7a12-4d9f-adb4-50bb66d296db

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

codecov · 2026-08-25T09:57:52Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.51%. Comparing base (b37b8c4) to head (8052ea0).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1314   +/-   ##
=======================================
  Coverage   99.51%   99.51%           
=======================================
  Files         117      118    +1     
  Lines       20839    20936   +97     
  Branches     2455     2460    +5     
=======================================
+ Hits        20738    20835   +97     
  Misses        101      101

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

hooks/test_gitapex_jq_type_confusion_matrix.py carried two lines unreachable by its own current guarded-field registry: an unused __repr__ (dead once explicit pytest ids= are passed) and a branch in bad_shape_phrase that only fires for a string-type guard's null/absent case denying with exit 2, which none of the six registered hooks ever do. Removes the dead __repr__ and collapses the branch into a non-branching bad_shape_phrases helper checked without regard to guard type, restoring 100% coverage on the new file instead of adding an unreachable branch to chase the coverage gate. Refs #1312, Refs #1216, Refs #1217

apm install (SessionStart-provisioned per #690) wired the superpowers and clairvoyance plugins' own hooks into the repo-committed settings.json; this reflects that already-established, expected drift, unrelated to this branch's own jq type-confusion matrix work. Refs #690

Adversarial convention-adherence review of PR #1314's own diff (this skill's step 8 inner layer) found three real divergences from the four already-fixed sibling hooks' own established pattern: - Both new tool_name-guard comments said "instead of failing closed" where all four siblings say "rather than failing closed" verbatim. - check-pr-title-convention.sh's freshly-added tool_input-guard comment skipped the general non-object-crash framing paragraph every sibling that introduces this predicate fresh includes before layering in the false-specific refinement. - test_denied_when_tool_name_is_not_a_string in both hook test files hand-rolled its own subprocess call instead of widening the file's own run() helper's tool_name type to object and reusing it, unlike three of four sibling test files that do exactly that. Also independently re-verified (against the actual sibling test files, not just the review's own claim) that the sibling tool_input-shape tests hand-roll rather than reuse run() -- that shape is the established convention there, not a gap, so the two hooks' own equivalent tests are left hand-rolled with a comment explaining why. Refs #1312, Refs #1216, Refs #1217

tvna · 2026-08-25T10:29:14Z

Two-layer independent review (drafting-a-pr-to-merge Step 8)

Recorded against head 8052ea0 (mergeable_state: clean, all 17 checks green as of this comment).

Outer layer (GitHub-native reviewer): No Claude Code Review App confirmed installed on this repository. Requested a GitHub Copilot review (request_copilot_review) at 2026-08-25T10:20Z; no review has posted as of this comment (2026-08-25T10:28Z). Disclosing the gap rather than treating it as passed -- monitoring continues per Step 10, and any Copilot feedback that lands afterward will be addressed like any other review comment.

Inner layer (always-runs adversarial fan-out): four category-focused passes dispatched against the diff, each with independent verification before anything below is reported as a finding.

Correctness: no bugs found. Both new jq predicates live-tested against the full falsy/type-confusion matrix on both hooks; guard ordering and precedence confirmed correct. One low-confidence, non-blocking documentation nit on .gitapex/ssot.json wording was raised and dismissed after checking it: the phrasing is byte-for-byte the same convention already used in the four already-merged sibling hooks' own entries, so it is not a defect introduced by this diff.
Regression/blast-radius: no regression found in the two changed hooks against any legitimate call shape (tested directly: plain string tool_name, create/update calls, tool_input omitted/{}/title-only/body-only, nested draft fields). One real, independently-verified finding outside this PR's own scope: a seventh hook, hooks/check-pr-duplicate-issue.sh, shares the identical tool_name-type-confusion gap (live-confirmed: an array-wrapped tool_name lets a duplicate-citing PR through, exit 0) but was never in issue hooks/: extract the guard prologue into a shared sourced helper (duplication caused #1216 and #1217) #1218's six-hook enumeration and isn't touched by hooks/: check-pr-issue-acm-disclosure.sh and check-pr-title-convention.sh fail open on tool_input: false (same class as #1208) #1216/hooks/: check-pr-issue-acm-disclosure.sh and check-pr-title-convention.sh fail open on a non-string tool_name (same class as #1208) #1217. Filed as a new, separate issue (hooks/check-pr-duplicate-issue.sh: fails open on a non-string tool_name (same class as #1208, #1217) #1315) with its own live-reproduced ACM, rather than expanding this PR's scope past what hooks/*.sh: add CI-enforced jq falsy/type-confusion matrix test for guard clauses #1312/hooks/: check-pr-issue-acm-disclosure.sh and check-pr-title-convention.sh fail open on tool_input: false (same class as #1208) #1216/hooks/: check-pr-issue-acm-disclosure.sh and check-pr-title-convention.sh fail open on a non-string tool_name (same class as #1208) #1217 already justify.
Reuse/simplification: two points raised. (1) Issue hooks/: extract the guard prologue into a shared sourced helper (duplication caused #1216 and #1217) #1218's shared-helper proposal is relevant background, but hooks/: check-pr-issue-acm-disclosure.sh and check-pr-title-convention.sh fail open on tool_input: false (same class as #1208) #1216/hooks/: check-pr-issue-acm-disclosure.sh and check-pr-title-convention.sh fail open on a non-string tool_name (same class as #1208) #1217's own Constraints explicitly mandate reusing PR fix(hooks): fail closed on missing/malformed jq in four PreToolUse gates #1213's proven hand-patch predicate rather than blocking on hooks/: extract the guard prologue into a shared sourced helper (duplication caused #1216 and #1217) #1218's refactor -- this PR follows the issues' own already-approved plan, not a new decision. (2) The new per-hook test_denied_when_tool_name_is_not_a_string/test_denied_when_tool_input_is_not_an_object tests do overlap with the new shared matrix file's own coverage of these two hooks, but hooks/: check-pr-issue-acm-disclosure.sh and check-pr-title-convention.sh fail open on tool_input: false (same class as #1208) #1216/hooks/: check-pr-issue-acm-disclosure.sh and check-pr-title-convention.sh fail open on a non-string tool_name (same class as #1208) #1217's own Acceptance Criteria explicitly require these per-hook tests to close those issues on their own terms -- kept intentionally, not an oversight. Two minor GuardedField/_set_field design-generality nits in the new matrix file were noted as optional, non-blocking polish.
Convention-adherence: three real divergences from the four already-fixed sibling hooks' own established pattern, all fixed in commit 8052ea0: two comment-wording mismatches ("instead of" vs the established "rather than" failing closed), one missing crash-framing paragraph in a freshly-added comment, and two new tests hand-rolling a subprocess call instead of reusing/widening the file's own run() helper the way three of four siblings do. Independently re-verified against the actual sibling test files (not just the review's own claim) that the tool_input-shape tests are correctly left hand-rolled -- that is genuinely the established convention there, not a gap the review missed.

All inner-layer findings are either fixed (convention-adherence, 3/3), filed separately with justification for not expanding this PR's scope (regression, 1 finding -> issue #1315), or considered and correctly left as-is against the closing issues' own explicit constraints (reuse, 2 points). No outstanding blocking finding against this diff.

Generated by Claude Code

tvna deployed to ruleset-verify August 25, 2026 09:54 — with GitHub Actions Active

tvna deployed to ruleset-verify August 25, 2026 10:06 — with GitHub Actions Active

tvna deployed to ruleset-verify August 25, 2026 10:08 — with GitHub Actions Active

tvna mentioned this pull request Aug 25, 2026

hooks/check-pr-duplicate-issue.sh: fails open on a non-string tool_name (same class as #1208, #1217) #1315

Closed

tvna deployed to ruleset-verify August 25, 2026 10:25 — with GitHub Actions Active

tvna marked this pull request as draft August 25, 2026 10:29

tvna marked this pull request as ready for review August 25, 2026 10:34

tvna merged commit 79476b5 into main Aug 25, 2026
20 of 21 checks passed

tvna deleted the claude/pr-1312-prep-61czj9 branch August 25, 2026 10:34

github-actions Bot mentioned this pull request Aug 25, 2026

chore(retrospective): merge retrospective for PR #1314 #1317

Open

This was referenced Aug 25, 2026

fix(hooks): allow uv add/remove; pin apm install/uninstall as allowed #1323

Merged

chore(retrospective): merge retrospective for PR #1325 #1338

Open

fix(hooks): deny non-string tool_name in check-pr-duplicate-issue.sh (#1315) #1363

Merged

Navigation Menu

fix(hooks): add jq type-confusion matrix test, close two origin-hook gaps - #1314

fix(hooks): add jq type-confusion matrix test, close two origin-hook gaps#1314
tvna merged 4 commits into
mainfrom
claude/pr-1312-prep-61czj9

tvna commented Aug 25, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Aug 25, 2026 •

edited

Loading

Uh oh!

codecov Bot commented Aug 25, 2026 •

edited

Loading

Uh oh!

tvna commented Aug 25, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

tvna commented Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Facts

Assumptions

Skill audit evidence

Acceptance Criteria Map

Risk / blast radius

Rollback

Verification

Checklist

Related Issue

Uh oh!

coderabbitai Bot commented Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tvna commented Aug 25, 2026

Two-layer independent review (drafting-a-pr-to-merge Step 8)

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tvna commented Aug 25, 2026 •

edited

Loading

coderabbitai Bot commented Aug 25, 2026 •

edited

Loading

codecov Bot commented Aug 25, 2026 •

edited

Loading