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

[tvna]

Summary

hooks/check-pr-duplicate-issue.sh denies (exit 2) instead of
silently allowing (exit 0) when tool_name in the PreToolUse payload is
a non-string JSON value (array/object/number/bool) -- the same
type-confusion class PR #1213/#1217 already closed in six sibling hooks.

Facts

• Live-reproduced against main before this fix: an array-wrapped
  tool_name let a create_pull_request call skip this hook's own
  open-PR-duplicate-citation check entirely (a payload with a
  resolving-style citation in its body field, piped into
  hooks/check-pr-duplicate-issue.sh, exited 0 instead of the expected
  2 -- see issue hooks/check-pr-duplicate-issue.sh: fails open on a non-string tool_name (same class as #1208, #1217) #1315's own Facts section for the exact reproduction
  command).
• The sibling tool_input-shape guard at line 99 is already present and
  unaffected; only the tool_name-shape guard was missing.
• Fix mirrors hooks/check-pr-issue-acm-disclosure.sh's own
  already-proven (.tool_name == null) or (.tool_name | type == "string") guard (PR fix(hooks): fail closed on missing/malformed jq in four PreToolUse gates #1213/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), verbatim.

Assumptions

None beyond the issue's own stated scope.

Risk / blast radius

Low. Purely additive fail-closed guard on one PreToolUse hook; no
existing allow-path behavior changes (only a previously-silent-allow path
for a malformed tool_name now denies).

Rollback

git revert this PR's merge commit; the hook reverts to its prior
(vulnerable) behavior.

Acceptance Criteria Map

(from issue #1315, re-verified unchanged this session -- see the
re-verification marker on the issue's own body)

Criterion	Interpretation	Planned ops	Proof method	Residual risk
A non-string tool_name must deny (exit 2), not silently fall through as "not our tool"	This hook fails closed on a tool_name of the wrong JSON type the same way PR #1213 made four other hooks do, and PR #1314 made check-pr-issue-acm-disclosure.sh/check-pr-title-convention.sh do	Add (.tool_name == null) or (.tool_name | type == "string") immediately before this hook's own tool_name=$(...) extraction (line 84)	printf '%s' '{"tool_name":["X"],...}' | bash hooks/check-pr-duplicate-issue.sh; echo $? returns 2, not 0; a new pytest case asserts this directly	Low -- the fix is a proven, already-shipped predicate (PR #1213), not a new mechanism
Regression coverage exists	This hook's existing test suite (hooks/test_gitapex_check_pr_duplicate_issue_shell.py) covered no non-string tool_name case	Add a parametrized (array/object/number/bool) test, mirroring PR #1213's test_denied_when_tool_name_is_not_a_string and PR #1314's equivalent additions to the two origin hooks	uv run --frozen pytest hooks/test_gitapex_check_pr_duplicate_issue_shell.py -v passes with the new cases included	None beyond this fix's own scope
The shared jq type-confusion matrix test (hooks/test_gitapex_jq_type_confusion_matrix.py, issue #1312) should also cover this hook	#1312's own matrix file registered six hooks; this is a seventh sharing the identical guard shape it was built to test	Add a GuardedField entry for check-pr-duplicate-issue.sh's tool_name field to GUARDED_FIELDS	uv run --frozen pytest hooks/test_gitapex_jq_type_confusion_matrix.py -q passes with the new entry included	Low -- purely additive to an already-proven test harness

Verification

All three rows above proved, post-implementation (commit b9729820;
step-8 review rounds landed comment polish in 3a33863b and two
registry-consistency fixes in 452a94fa, no behavior change to the
guard itself):

• Row 1: live repro re-run post-fix -- printf '%s' '{"tool_name":["mcp__github__create_pull_request"],"tool_input":{"owner":"tvna","repo":"gitapex","title":"t","body":"Refs #1"}}' | GH_TOKEN= GITHUB_TOKEN= bash hooks/check-pr-duplicate-issue.sh; echo $? -> 2 (deny), was 0 pre-fix.
• Row 2: uv run --frozen pytest hooks/test_gitapex_check_pr_duplicate_issue_shell.py -v -> 24 passed (new test_denied_when_tool_name_is_not_a_string[array/object/number/bool] cases confirmed Red pre-fix, Green post-fix).
• Row 3: uv run --frozen pytest hooks/test_gitapex_jq_type_confusion_matrix.py -q -> 135 passed (new pr-duplicate-issue:tool_name and pr-duplicate-issue:tool_input entries included).
• Full repo suite: uv run --frozen pytest hooks -q -> 533 passed, no regressions (confirmed independently in both step-8 review rounds).
• Lint/type-check: ruff check, ruff format --check, mypy --strict all pass on every touched Python file; bash -n confirms the modified hook's shell syntax; .github/scripts/gitapex_scan_ssot_schema.py reports no ssot.json drift.

Skill audit evidence

• deterministic-gate-quality: RAN -- this diff modifies a registered
  deterministic gate (hooks/check-pr-duplicate-issue.sh, per
  gitapex_detect_changed_gate_scripts.py). Read against
  skills/evaluating-deterministic-gate-quality/references/dimensions.md
  dimension 15 (fail-closed on malformed input) specifically: the new
  guard is the exact, already-proven (.tool_name == null) or (.tool_name | type == "string") predicate from PR fix(hooks): fail closed on missing/malformed jq in four PreToolUse gates #1213/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 (no new detection
  logic invented), and the new hook-local pytest case plus the shared
  hooks/test_gitapex_jq_type_confusion_matrix.py registration both
  confirm it fails closed (exit 2) for every non-string/non-null matrix
  value (array/object/number/bool/zero/empty-string), not only the
  happy-path string case.
• defeat-test-disclosure: RAN -- the new test_denied_when_tool_name_is_not_a_string
  parametrized test (array/object/number/bool) was written test-first: it
  fails against the pre-fix hook (confirmed live, all 4 cases red before
  the guard was added) and passes only once the guard lands, specifically
  targeting the new detection logic's boundary (a non-string, non-null
  tool_name) rather than only exercising its happy path. Two independent
  step-8 review rounds additionally mutation-tested the guard directly
  (temporarily disabled it, confirmed the tests fail for the right
  reason, restored it) and live-tested the full 8-value jq
  falsy/type-confusion matrix plus adjacent hostile inputs against the
  shipped guard; no fail-open found.

Checklist

• Tests pass locally
• Docs updated if behavior changed (n/a -- hook behavior change is
  self-documenting via its own inline comments, no separate doc)
• Issue number cited in every commit
• If this PR adds/modifies a skills/*/SKILL.md, a
  docs/superpowers/specs/*.md design doc, a security-relevant
  skill, or a deterministic checker script
  (skills/*/scripts/*.py, evals/scripts/*.py,
  .github/scripts/*.py), a ## Skill audit evidence section
  discloses the required verdicts/waivers (this PR modifies a
  registered deterministic gate, hooks/check-pr-duplicate-issue.sh
  -- see ## Skill audit evidence above)
• If this PR adds a new Kept-edit-log entry to any evals/*/split.md
  (n/a)
• If this PR adds or increases a skills/*/SKILL.md's Stop-boundary
  bullets or named dispatch branches (n/a)

Merge gate: independent review

This PR is also subject to the independent-review-pending required
status check (see .github/workflows/independent-review-pending.yml /
.github/scripts/gitapex_gate_independent_review_pending.py). See the
## Independent review verdict section below.

Independent review verdict

• Verdict: CLEAN
• Verified commit: 452a94f

Outer layer: GitHub Copilot review requested via request_copilot_review
(comment-only -- Copilot's review carries no pass/fail signal of its own,
a materially weaker guarantee than Anthropic's Claude Code Review App
would give; that App is not confirmed installed/configured for this
repository, so this is disclosed rather than treated as equivalent).

Inner layer: two independent fresh-context adversarial-review dispatches
ran against this diff (single dispatch per round -- the diff is small
enough that category fan-out was not needed), each explicitly framed as
holding no assumption the diff is correct:

• Round 1 (against commit 3a33863): found two Low-severity findings
  (.gitapex/ssot.json's gate rule text not updated for the new guard;
  the shared jq type-confusion matrix's own "authoritative registry"
  claim left this hook's existing tool_input guard unregistered). Both
  independently verified against the actual code, fixed, and re-tested
  green (commit 452a94fa).
• Round 2 (against the full diff at commit 452a94fa): CLEAN. Live
  mutation-tested both the new tool_name guard and the newly-registered
  tool_input matrix entry (each guard temporarily disabled, confirmed
  the corresponding tests fail for the right reason, then restored and
  re-confirmed green); ran the full hooks/ suite (533 passed), ruff,
  ruff format --check, mypy, and .github/scripts/gitapex_scan_ssot_schema.py
  (no drift). No finding survived independent verification at or above
  the 0.7 confidence bar.

Execution log

PlanApproved{run_id: 333ae4b9}
TaskStarted{run_id: 333ae4b9, task_id: task-1}
TaskCompleted{run_id: 333ae4b9, task_id: task-1, commit_sha: b9729820}
TaskCompleted{run_id: 333ae4b9, task_id: step-8-refactor-review, commit_sha: 3a33863b}
TaskCompleted{run_id: 333ae4b9, task_id: step-8-review-fixes, commit_sha: 452a94fa}

Related Issue

Closes #1315

[coderabbitai]

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: 03fc5fdc-9d62-4ef6-875d-1b80aeca5635

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.55%. Comparing base (6dd908f) to head (452a94f).
⚠️ Report is 15 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #1363    +/-   ##
========================================
  Coverage   99.54%   99.55%            
========================================
  Files         123      124     +1     
  Lines       21912    22019   +107     
  Branches     2682     2699    +17     
========================================
+ Hits        21813    21920   +107     
  Misses         99       99            

☔ 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.

[tvna]

The contract-discipline-drift check is failing, but it is not this PR's failure:

• The job's own logs show it invoking .github/scripts/gitapex_scan_contract_discipline_drift.py, which does not exist anywhere in this repository at HEAD (confirmed via a repo-wide search, both in the working tree and via git show/git log across main). The job fails with python3: can't open file '.../gitapex_scan_contract_discipline_drift.py': [Errno 2] No such file or directory -- a hard, deterministic failure unrelated to any diff content.
• This PR's own diff never touches .github/scripts/, any workflow file, or either of the two paths the job diffs (skills/evaluating-skill-quality/references/rubric.md, skills/drafting-a-skill/references/contract-structure.md).
• No .github/workflows/*.yml in this repository defines a job named contract-discipline-drift at all (searched by job name and by the missing script's filename) -- the workflow that runs it is not part of this repository's own checkout, so I have no visibility into it or the ability to fix it from here.
• No fix for this exists anywhere I can find (no sibling PR, no revert candidate) since the referenced script was apparently never added to this repository in the first place.

This will fail identically for any PR/commit in this repository until either the missing script is added or the (external) workflow definition is corrected -- out of scope for issue #1315's own tool_name guard fix. Not re-running it, since the failure is deterministic (a missing file), not a flake.

---

Generated by Claude Code