chore(retrospective): merge retrospective for PR #1213

[github-actions]

Summary

Retrospective for PR #1213 ("fix(hooks): fail closed on missing/malformed jq in four PreToolUse gates", which addressed #1208), merged 2026-08-19T22:18:37Z by the repository owner. Twelve repairs occurred between PR open (2026-08-18T15:08:11Z) and merge, across ten commits: one Codecov patch-coverage fix, one CodeRabbit review round, two independent adversarial-review rounds (bundling four distinct bug fixes across a single commit plus a fifth in a following commit), one code-reuse finding from this skill's own two-layer review, a self-directed registry-linkage audit (prompted by a direct question, not CI or review) that found and fixed a missing .gitapex/ssot.json entry plus stale rule text on three others, a merge conflict from a concurrent independent session fixing the identical registry gap, a follow-up completeness audit disclosing a gap in the drift gate's own rule text, and a final merge inheriting two pre-existing main-branch CI fixes unrelated to this PR's own diff.

Repairs

1. [Codecov patch-coverage: unreachable skip branch] The template-overwrite test suite's own pytest.skip fallback (for when no real PR template exists in the checkout) was never exercised in this repository's own CI, since a template always exists here -- Codecov's patch-coverage gate flagged the line as uncovered and failed the check. Fixed in commit ad67a28 by replacing the checkout scan with a self-contained tmp_path fixture that creates its own template file, removing the unreachable branch entirely rather than covering it artificially.
   Classification: missing deterministic gate.
   Status: missing-deterministic-gate
   Proposed gate: a pre-push (or pre-commit) stage running pytest --cov scoped to new/changed test files only, mirroring Codecov's own patch-coverage gate locally and fast, so an unreachable branch in a new test is caught before it ever reaches CI.

2. [CodeRabbit: tool_input false-falsy gap] The tool_input-shape guard these hooks had just gained, (.tool_input // {}) | type == "object", accepted the JSON literal false the same as null or an absent key (jq's // treats both as falsy) -- a tool_input: false payload slipped past the guard and crashed the next field-extraction line instead of denying. Live-confirmed, then fixed in commit de7e6bd by tightening the predicate to (.tool_input == null) or (.tool_input | type == "object"), verified against the full value matrix.
   Classification: missing deterministic gate.
   Status: missing-deterministic-gate
   Proposed gate: see repair 5's proposed gate below -- the same general type-confusion matrix test would have caught this instance too.

3. [Adversarial round: warn()'s ARG_MAX-vulnerable jq construction] check-bash-safety.sh's warn() (the non-blocking counterpart to the already-hardened deny()) still built its JSON via jq -n --arg, the exact construction deny() had already been hardened away from. Its only call site interpolates the provenance scanner's report over the entire outgoing push, large enough to realistically hit the limit -- live-confirmed a crash (exit 126) on a 3MB message. Fixed in commit 0b0a3af (bundled with repairs 4-6 below in the same push) with the same jq -Rs piped-stdin construction already proven in deny().
   Classification: missing deterministic gate.
   Status: missing-deterministic-gate
   Proposed gate: a consistency check (e.g. a shape-checker rule) flagging any hooks/*.sh file where deny() uses the safe jq -Rs piped-stdin form but a sibling function (warn() or equivalent) in the same file still uses jq -n --arg, so a hardening applied to one does not silently stop short of the other.

4. [Adversarial round: test suite narrower than its own docstring claims] The same dispatch found the tool_input tests never actually included the "string" case their own docstrings claimed to cover, and no hook had a regression test proving tool_input: null/absent still correctly allows the call through. Both added in commit 0b0a3af.
   Classification: missing deterministic gate.
   Status: missing-deterministic-gate
   Proposed gate: same as repair 5 below -- a single parametrized fixture enumerating the full value matrix (including the allow-through baseline) per guard, rather than individual cases added reactively per review round.

5. [Adversarial round, most severe: non-string tool_name bypass] jq -r never errors on a non-string .tool_name (e.g. an array) -- it pretty-prints the JSON value across multiple lines, which then never matches the plain-string re-check every one of these hooks' own defense-in-depth relies on, silently falling through as "not our tool" instead of failing closed. Most severe on check-merge-pull-request-block.sh: an array-wrapped tool_name let a real merge_pull_request call straight through this repository's own categorical "no override" deny -- the exact bypass class issue hooks/: four more hooks fail open on missing/malformed jq (same class as gitapex#436) #1208 exists to close, via a different field than the one it named. Live-confirmed across all four hooks, fixed in commit 0b0a3af with (.tool_name == null) or (.tool_name | type == "string").
   Classification: missing deterministic gate.
   Status: missing-deterministic-gate
   Proposed gate: 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. This exact bug class needed four separate adversarial rounds (repairs 2, 5, 7, plus this one) to fully stamp out across four files -- a systematic matrix test, ideally written once against issue hooks/: extract the guard prologue into a shared sourced helper (duplication caused #1216 and #1217) #1218's own already-proposed shared sourced guard-prologue helper rather than duplicated per file, would have caught all four in one pass instead of four.

6. [Adversarial round: unguarded mktemp crash] Two unguarded var=$(mktemp) calls in check-pr-skill-audit-disclosure.sh's tier-1/tier-2 logic crashed the whole script under set -e on an unwritable/full TMPDIR, instead of the intended degrade-to-tier-2-then-CI fallback every other tier-1-incomplete path in that hook already takes. Live-confirmed, fixed in commit 0b0a3af to fall through with a warning instead.
   Classification: missing deterministic gate.
   Status: missing-deterministic-gate
   Proposed gate: a static check flagging any $(mktemp ...) call in hooks/*.sh not immediately guarded by a fallback, since set -e alone converts an unguarded mktemp failure into a hard crash rather than the graceful degrade these hooks otherwise implement consistently.

7. [Adversarial round: leaf-field type confusion one level deeper] The same jq-pretty-print gap as repair 5, found one level deeper: check-bash-safety.sh's .tool_input.command and check-template-overwrite.sh's .tool_input.file_path, both feeding a blacklist-style danger-pattern match rather than a positive-match-required-to-pass check, so an array/object value slipped through as "no match" instead of failing closed. Live-confirmed with an array-wrapped ["gh","pr","merge","1"] command and an array-wrapped file_path targeting the real, on-disk PR template. Fixed in commit 3236ee7.
   Classification: missing deterministic gate.
   Status: missing-deterministic-gate
   Proposed gate: same as repair 5 above.

8. [Two-layer review, inner layer: hand-built test instead of reusing existing helper] Three test_denied_when_tool_name_is_not_a_string tests hand-built their own ~15-line raw subprocess.run block instead of reusing each file's own pre-existing run() helper, which already parameterized tool_name. Fixed in commit 2a83666 by widening each run()'s tool_name parameter from str to object and calling it directly.
   Classification: unclear agent instruction -- no standing instruction says to check for and reuse an existing parametrizable test helper already present in the same file before hand-writing a new one; a code-reuse judgment call review caught, not something a duplication-detection gate could flag reliably without false positives.
   Status: unclear-agent-instruction

9. [Self-directed audit: missing ssot.json registry entry] Prompted directly (not by CI or review) after the two-layer review, a registry-linkage audit found check-merge-pull-request-block.sh -- the hook carrying this PR's most severe fix (repair 5) -- had no .gitapex/ssot.json gates[] entry at all, and the three already-registered gates this PR touches carried rule text predating every fail-closed guard added across repairs 2-7. .github/scripts/gitapex_scan_ssot_schema.py's own eight drift checks only validate that a registered entry points to a real file, never the reverse -- which is why CI never caught the missing entry; it was still covered by skill-audit-disclosure's own separate naming-convention backstop. Fixed in commit a3cd021: added the missing entry and refreshed the three stale rule texts.
   Classification: missing deterministic gate.
   Status: missing-deterministic-gate
   Proposed gate: implement issue .gitapex/ssot.json: drift gate has no reverse-direction check (unregistered gate-shaped file passes silently) #1227's own already-filed proposal -- extend gitapex_scan_ssot_schema.py with a ninth, reverse-direction check that walks the same naming-convention regex gitapex_detect_changed_gate_scripts.py already applies for a different purpose, and fails when a matching gate-shaped file has no gates[] entry.

10. [Merge conflict: concurrent independent session fixed the identical gap] Merging origin/main to pick up unrelated upstream work produced a genuine conflict in .gitapex/ssot.json: a separate, concurrent PR (refactor(hooks): consolidate hooks.json description into ssot.json registry #1223, issue refactor(hooks): consolidate hooks.json description into ssot.json gate registry #1222) had independently added an equivalent merge-pull-request-block entry at a different array position with different rule/local_exclusion text. Resolved in commit ce558d5 by dropping this PR's own duplicate entry (which would otherwise have violated find_duplicate_ids) and merging the fail-closed-mechanics sentence from this PR's own draft into main's existing entry's rule field, preserving both sessions' information rather than picking one side. Documented via a PR comment per this skill's own mandatory conflict-disclosure rule.
    Classification: external/human decision -- ordinary concurrent-development timing (two independent sessions closing the identical registry gap in parallel) is not a pattern any deterministic gate or clearer instruction could have prevented.
    Status: external-human-decision

11. [Completeness audit: drift gate's own rule text didn't disclose its own gap] A follow-up completeness audit (prompted directly, after the merge above) found the ssot-schema-drift gate's own rule text accurately described what it currently checks but never disclosed the reverse-direction gap that let repair 9 happen -- the same gap issue .gitapex/ssot.json: drift gate has no reverse-direction check (unregistered gate-shaped file passes silently) #1227 now tracks. Fixed in commit 20c5bfa by adding a one-sentence disclosure citing .gitapex/ssot.json: drift gate has no reverse-direction check (unregistered gate-shaped file passes silently) #1227.
    Classification: unclear agent instruction -- no standing instruction says that filing an issue describing a gate's own known limitation must also update that gate's own ssot.json rule text in the same breath to disclose it inline; a self-disclosure-completeness judgment call, not something a script could enforce without a speculative, over-fit gate.
    Status: unclear-agent-instruction

12. [Final merge: inherited two pre-existing main-branch CI fixes] Responding to a direct report that CI was red, both flagged checks (pytest's prose-count-vs-registry drift, ruleset-scan's live-vs-declared ruleset drift) were root-caused via isolated git worktree testing against origin/main's own tip, independent of this PR's diff -- confirming both were pre-existing main-branch bugs, already fixed upstream by unrelated work that had landed on main in the meantime. Recovered in commit 2e266b6 by merging origin/main in (a clean merge, no conflict markers); both checks confirmed success via a live get_check_runs call against the merge commit afterward, not assumed from static reasoning.
    Classification: external/human decision -- ordinary concurrent-development timing (unrelated work fixing pre-existing bugs on main while this PR was in flight), the same pattern issue chore(retrospective): merge retrospective for PR #1234 #1236's own repair 5 already modeled for the immediately preceding PR.
    Status: external-human-decision

Carried-forward gate

Issue #1226 (retrospective for PR #1223) first surfaced this backlog and proposed a periodic (not per-PR) batch-triage pass over retrospective-labelled issues that fail the two-signal resolution check, since doing this per-PR does not scale once the backlog reaches this size. Issue #1235 (retrospective for PR #1233) re-ran the check and found 291 of 294 candidates unresolved, still only #187/#285/#536 resolved. Issue #1236 (retrospective for PR #1234, the immediately preceding cycle) re-ran it again with the same result.

Re-running the identical two-signal check this cycle (295 prior candidates: the full retrospective-labelled backlog minus this cycle's own stub #1237, now including #1236 itself as a prior candidate for the first time) against the full, unshallowed history of origin/main post-merge (2e266b6): still only 3 resolved (#187, #285, #536, unchanged across at least 4 consecutive cycles now), 292 unresolved -- exactly one more than #1236's own 291, fully explained by #1236 itself now counting as a prior candidate instead of the current cycle's own stub. This PR's own commits cite #1208, #1216, #1217, #1218, and #1227 -- none among the backlog numbers. No implementing commit for #1226's own proposed periodic-triage mechanism exists on origin/main as of this cycle either. Following #1226/#1235/#1236's own established precedent, the complete number list is not repeated a fourth consecutive time here -- see #1226's own body for the original full enumeration.

Status: carried-forward
Proposed gate: (restated from #1226, unimplemented across at least 4 consecutive cycles now including this one) a periodic (not per-PR) batch-triage pass over retrospective-labelled issues that fail the two-signal resolution check -- reading each one's actual proposed gate, and for each: confirm it is still relevant, close it explicitly if superseded/no-longer-applicable, or convert it into real follow-on implementation work if still worth building.

Refs

Refs #1213, #1208.

[github-actions]

This issue's body does not disclose an Acceptance Criteria Map (ACM) or an explicit waiver. Per skills/drafting-an-acm-issue/SKILL.md (see issue #357), add either:

• an ACM table (criterion -> interpretation -> planned ops -> proof method -> residual risk), or
• a waiver line: ACM: not-applicable (chore|docs|tracking|defect): <reason>

This is a post-hoc, non-blocking flag (issue #414) -- it does not prevent this issue from being worked on, and applies regardless of who or what opened it.