chore(retrospective): merge retrospective for PR #1741 #1748

Summary

Retrospective for PR #1741 ("feat(gates): add consolidated local PR-body preflight check"), merged 2026-09-04. Fifteen repairs occurred between PR open and merge, across the initial feature commit, three rounds of self-initiated adversarial review, and two genuine CI failures.

Repairs found this cycle:

1. Coverage-combine floor gap on initial implementation
2. Missing dependency-precondition probe before wrapping a gate with a registered precondition
3. Duplicated hook verdict computation
4. Undisclosed transitive dependency in a new registry entry
5. run_all_checks aborted the whole report on one sub-check's own exception
6. Stacked-PR merge-base miscalculation, recurrence of an already-fixed bug class
7. Fail-open on hook-script crash, inverting an established fail-closed policy
8. Missing -- argv separator, second recurrence of issue gate-proposal-umbrella: environment/repo-state preconditions #1566's own bug class
9. Subprocess timeout budget never checked against the wrapping hook's own harness timeout
10. Bash unguarded array expansion, recurrence of an already-guarded pattern
11. _isolated exception scope narrower than the helper its own docstring compared itself to
12. _run's new timeout parameter froze a module constant at function-definition time
13. Inaccurate "two sub-checks report SKIPPED" claim repeated in four places
14. Stacked-PR test fixture did not actually reproduce the risk it claimed to test
15. Dead code left behind after removing its only call site

Repairs

1. [Coverage-combine floor gap on initial implementation] CI's coverage-combine job flagged gitapex_gate_pr_body_preflight.py below the repository's 90 percent coverage floor right after the initial feature commit (1a1421e5). Fixed by 77c0c4a8, adding direct tests for the missing-sibling-script error paths, the check-diff entry point, invalid UTF-8 handling, and a sub-check timeout.
   Classification: missing deterministic gate.
   Status: missing-deterministic-gate
   Proposed gate: extend the local pre-push preflight (or the git pre-push hook) with a lightweight coverage-floor check scoped to files the diff actually touches, mirroring coverage-combine's own threshold.
   Filed as: gate-proposal: retro #1748 repair 1: Coverage-combine floor gap on initial implementation #1750

2. [Missing dependency-precondition probe before wrapping a gate with a registered precondition] The initial check_skill_audit_disclosure sub-check invoked gitapex_gate_skill_audit_disclosure.py's own --check-diff mode with no probe for its pydantic import dependency first, risking a misleading generic FAIL if pydantic were not importable -- the identical risk hooks/check-pr-skill-audit-disclosure.sh's own tier-1 path had already fixed once (issue gate-proposal-umbrella: environment/repo-state preconditions #1566). Found by a self-initiated adversarial review (round 1), fixed in e734ae12.
   Classification: missing deterministic gate.
   Status: missing-deterministic-gate
   Proposed gate: a structural check flagging a new PreToolUse hook or gate script invoking another gate script whose registry entry declares a Python-package precondition, with no corresponding precondition probe of its own.
   Filed as: gate-proposal: retro #1748 repair 2: Missing dependency-precondition probe before wrapping a gate with a registered precondition #1751

3. [Duplicated hook verdict computation] The initial hook (hooks/check-pr-body-preflight.sh) unconditionally re-ran the skill-audit-disclosure sub-check on every call, duplicating the verdict hooks/check-pr-skill-audit-disclosure.sh already computes independently on the identical two matchers. Found by round 1, fixed via a --skip flag.
   Classification: unclear agent instruction -- recognizing that two independently-authored hooks wired to the same matcher pair would end up computing the identical verdict is a cross-file architectural judgment call, not a pattern a deterministic linter or static check could realistically flag; nothing short of the review itself catches this class of overlap.
   Status: unclear-agent-instruction

4. [Undisclosed transitive dependency in a new registry entry] The new pr-body-preflight gate's own .gitapex/ssot.json entry did not declare its preconditions.requires_python_packages for the transitive pydantic dependency its skill-audit-disclosure sub-check pulls in. Found by round 1, fixed in e734ae12.
   Classification: missing deterministic gate.
   Status: missing-deterministic-gate
   Proposed gate: a registry-completeness check cross-referencing each gate entry's own subprocess-invoked scripts against their own declared Python-package preconditions.
   Filed as: gate-proposal: retro #1748 repair 4: Undisclosed transitive dependency in a new registry entry #1752

5. [run_all_checks aborted the whole report on one sub-check's own exception] run_all_checks's own docstring claimed it deliberately runs all sub-checks even after an earlier one fails, but an unhandled exception from one sub-check's own setup actually aborted the entire aggregate run, silently losing every other sub-check's own result -- the opposite of the module's stated design. Found by a self-initiated adversarial review (round 2), fixed in 9db88527 via a new _isolated per-check wrapper.
   Classification: unclear agent instruction -- the initial implementation's own behavior contradicted its own docstring's claim; no gate can verify that running code actually matches its own prose description, only a careful read (which is what the review supplied).
   Status: unclear-agent-instruction

6. [Stacked-PR merge-base miscalculation, recurrence of an already-fixed bug class] hooks/check-pr-body-preflight.sh fell back to the repository's default branch when update_pull_request supplied no explicit base, dragging a stacked PR's own parent-branch changes into --check-diff's scope -- the exact false-deny risk class hooks/check-pr-skill-audit-disclosure.sh's own base_is_explicit guard already exists to prevent, in the sibling hook this new one was modeled after. Found by round 2, fixed via the same base_is_explicit pattern.
   Classification: missing deterministic gate.
   Status: missing-deterministic-gate
   Proposed gate: a sibling-pattern-drift check flagging a new hook that resolves a default-branch fallback without tracking whether that fallback occurred.
   Filed as: gate-proposal: retro #1748 repair 6: Stacked-PR merge-base miscalculation, recurrence of an already-fixed bug class #1753
   Recurrence note: shares a thesis with repairs 7, 8, 9, and 10 -- a new hook modeled after hooks/check-pr-skill-audit-disclosure.sh silently dropped several of that sibling's own already-established safety patterns during the copy.

7. [Fail-open on hook-script crash, inverting an established fail-closed policy] The initial hook failed OPEN (warned, allowed) when the wrapped preflight script itself crashed without a recognized FAIL line, inverting hooks/check-pr-skill-audit-disclosure.sh's own explicit fail-closed policy for the identical situation, deliberately established in PR fix(hooks): fail closed on missing/malformed jq in four PreToolUse gates #1213. Found by round 2, fixed to deny.
   Classification: missing deterministic gate.
   Status: missing-deterministic-gate
   Proposed gate: same sibling-pattern-drift check family as repair 6 -- flag a new hook whose own "check script exited unrecognizably" branch does not deny, when a sibling hook already establishes a documented fail-closed precedent.
   Filed as: gate-proposal: retro #1748 repair 7: Fail-open on hook-script crash, inverting an established fail-closed policy #1754
   Recurrence note: shares a thesis with repairs 6, 8, 9, and 10 (see repair 6).

8. [Missing -- argv separator, second recurrence of issue gate-proposal-umbrella: environment/repo-state preconditions #1566's own bug class] _missing_packages_report called gitapex_check_python_precondition.py without the -- argv separator hooks/check-pr-skill-audit-disclosure.sh's own tier-1 call already carries for the identical script, for the identical reason (issue gate-proposal-umbrella: environment/repo-state preconditions #1566). Found independently by three reviewers in round 2 (correctness, blast-radius, and security axes each raised it), fixed with a defeat test.
   Classification: missing deterministic gate.
   Status: missing-deterministic-gate
   Proposed gate: same sibling-pattern-drift check family as repairs 6-7 and 9-10 -- flag a new call site invoking the precondition checker with a dynamic argument list omitting the required -- separator.
   Filed as: gate-proposal: retro #1748 repair 8: Missing -- argv separator, second recurrence of issue #1566's own bug class #1755
   Recurrence note: shares a thesis with repairs 6, 7, 9, and 10 (see repair 6); this specific bug class had already recurred once before this PR (issue gate-proposal-umbrella: environment/repo-state preconditions #1566), making this its second confirmed recurrence.

9. [Subprocess timeout budget never checked against the wrapping hook's own harness timeout] The per-subprocess timeout (initially 120s, then an under-corrected 8s) was never checked against hooks/check-pr-body-preflight.sh's own wired 30s PreToolUse harness timeout, even though the hook can invoke up to four such subprocesses sequentially in one call. Found across rounds 2 and 3, fixed by lowering to 5s and empirically verifying the margin against this repository's own largest tracked Markdown files.
   Classification: missing deterministic gate.
   Status: missing-deterministic-gate
   Proposed gate: a static check computing, for each wired PreToolUse hook, the worst-case sum of its own script's sequential subprocess-call timeouts against the hook's own declared harness timeout.
   Filed as: gate-proposal: retro #1748 repair 9: Subprocess timeout budget never checked against the wrapping hook's own harness timeout #1756
   Recurrence note: shares a thesis with repairs 6, 7, 8, and 10 (see repair 6), though this one is against the new hook's own internal budget rather than a sibling hook's established pattern.

10. [Bash unguarded array expansion, recurrence of an already-guarded pattern] hooks/check-pr-body-preflight.sh's own --check-diff array was expanded unconditionally even when empty, which raises "unbound variable" under set -u on bash older than 4.4 (e.g. macOS's shipped bash 3.2) -- hooks/check-pr-skill-audit-disclosure.sh already guards its own comparable optional array with an explicit length check. Found by round 2, fixed via a length-guarded build.
    Classification: missing deterministic gate.
    Status: missing-deterministic-gate
    Proposed gate: a repository-specific scan over hooks/*.sh for an unguarded array expansion with no preceding length guard.
    Filed as: gate-proposal: retro #1748 repair 10: Bash unguarded array expansion, recurrence of an already-guarded pattern #1757
    Recurrence note: shares a thesis with repairs 6, 7, 8, and 9 (see repair 6).

11. [_isolated exception scope narrower than the helper its own docstring compared itself to] The new _isolated per-check wrapper (round 2's own fix for repair 5) caught only two exception types, narrower than gitapex_gate_local_preflight.py's own run_check its docstring explicitly compared itself to -- an uncaught OSError (e.g. git itself missing from PATH) still crashed the whole aggregate run, plus a duplicate exception-to-result conversion existed alongside it. Found by a third self-initiated adversarial review (round 3, scrutinizing round 2's own fix), fixed by widening the scope to match and deduplicating into one shared helper.
    Classification: unclear agent instruction -- verifying that a new helper's own exception scope actually matches the scope its own docstring claims to mirror is a careful-reading task, not a pattern a generic linter catches; this was caught only by dedicating a fresh review round specifically to the previous round's own fix.
    Status: unclear-agent-instruction

12. [_run's new timeout parameter froze a module constant at function-definition time] Adding a timeout parameter to _run as a plain default-argument value silently froze that module constant's value at function-definition time -- a classic Python late-binding pitfall -- breaking every test (and every real call) relying on monkeypatching the constant afterward. Self-found while implementing round 3's own fixes (not flagged by a review agent), live-confirmed via _run.__defaults__ staying stale post-monkeypatch, fixed with a call-time-resolved sentinel.
    Classification: unclear agent instruction -- a well-known but easy-to-miss Python default-argument-evaluation-timing pitfall; a documented warning in this repository's own code-quality-principles reference (used specifically because this repository's own test suite leans heavily on monkeypatching module-level constants) would have been a more direct fix than a new gate.
    Status: unclear-agent-instruction

13. [Inaccurate "two sub-checks report SKIPPED" claim repeated in four places] Four separate places (this module's own docstring, its --check-diff argparse help text, the hook's own header comment and runtime Notice message, and CONTRIBUTING.md) claimed that omitting --check-diff skips two sub-checks; in fact only skill-audit-disclosure is skipped, provenance-disclosure always runs in a body-only mode. Found by round 3, corrected in all four places.
    Classification: unclear agent instruction -- an inaccurate claim about the code's own behavior, copy-pasted across four locations without independently re-verifying it against the actual sub-check dispatch logic each time.
    Status: unclear-agent-instruction
    Recurrence note: shares a thesis with repair 14 -- a claim (in prose or in a test fixture) not independently re-verified against the actual runtime behavior it describes.

14. [Stacked-PR test fixture did not actually reproduce the risk it claimed to test] The regression test for repair 6's own fix originally published the parent branch's change to origin/main before forking the stacked branch, which meant the wrong-ancestor and right-ancestor merge-base computations produced the identical diff scope regardless of whether the fix was present -- the test would have passed even without the fix. Found by round 3, corrected by removing the premature publish step and adding a direct assertion that the wrong-ancestor diff genuinely includes the parent's own change.
    Classification: unclear agent instruction -- confirming that a regression test actually fails without its own fix requires deliberately reasoning through the fixture's own git history shape; the existing defeat-test-disclosure convention only requires disclosing that a defeat test exists, not independently proving it defeats.
    Status: unclear-agent-instruction
    Recurrence note: shares a thesis with repair 13 (see repair 13).

15. [Dead code left behind after removing its only call site] A test-fixture edit (round 3's own fix for repair 14) removed the only call site of a helper function (_publish) but left the now-dead function itself in place, leaving 4 uncovered lines that failed the codecov/patch CI check. Fixed in 8d6b1fdb by deleting the dead function.
    Classification: missing deterministic gate.
    Status: missing-deterministic-gate
    Proposed gate: a dead-code check for a module-private top-level function or class with zero remaining call sites in its own file, run as part of the local pre-push preflight.
    Filed as: gate-proposal: retro #1748 repair 15: Dead code left behind after removing its only call site #1758

Notes

Nine repairs (1, 2, 4, 6, 7, 8, 9, 10, 15) were classified as missing-deterministic-gate and filed as their own standalone issues (#1750-#1758), each carrying its own Acceptance Criteria Map -- building each proposed gate is that issue's own follow-on work, per merge-retrospective's Stop boundary. Six repairs (3, 5, 11, 12, 13, 14) were classified as unclear-agent-instruction; their Classification line's own rationale is the record, and no standalone issue is filed for them.

The recurring thesis across repairs 6-10 (all five filed as standalone issues) is worth calling out on its own: this PR's new hook was explicitly modeled after hooks/check-pr-skill-audit-disclosure.sh, yet silently dropped five of that sibling's own already-established safety patterns during the copy (a stacked-PR base-tracking guard, a fail-closed crash policy, an argv -- separator, a timeout budget check, and a bash array-expansion guard) -- three of which (the base-tracking guard, the -- separator, and the fail-closed policy) are recurrences of bug classes this repository has already paid down once before, in the sibling hook itself or in issue #1566. A "does a new hook modeled after an existing one actually carry that model's own established safety patterns forward" check, if it existed as one consolidated structural gate rather than five separate proposals, might close all five at once -- left as an observation for whoever picks up #1750-#1758, not acted on here.

Refs #1741