Skip to content

chore(retrospective): merge retrospective for PR #1984 #2001

Description

@github-actions

Retrospective for PR #1984 (issue #1965): foundation for the skill-contract form (spec.contract schema, bundled generator, drift gate, ssot gate-id checks). 22 commits, 9 tasks across 4 waves, merged. 11 repairs found between PR open and merge.

Repair list:

  1. skill-audit-disclosure CI check failed shortly after PR feat(drafting-a-skill): spec.contract schema, contract generator, drift gate (Closes #1965) #1984 was opened, for a references/**-only file
  2. Dispatched branch-plan-task self-reported local-preflight clean when 3 real gate findings existed
  3. checker-script-adversarial-review found a marker-own-line false negative in the new skill-contract generator
  4. checker-script-adversarial-review/deterministic-gate-quality found an unverified sidecar-discovery-helper duplication risk
  5. checker-script-adversarial-review found 3 stale checker-count cross-references after a "both"->"all three" edit
  6. design-doc-adversarial-review found 6 residual stale "Proof" mentions outside Task 9's own rewrite scope
  7. executing-a-branch-plan Step 8's mandatory aggregate adversarial review found 7 distinct findings
  8. A Claude-Session commit-message trailer was added a second time within this PR, on a commit authored directly by the orchestrating session
  9. drafting-a-pr-to-merge Step 8's inner-layer fresh review found 6 distinct findings on the full PR diff
  10. independent-review-pending CI check failed because the drafted PR-body verdict omitted the required "- Verdict: CLEAN" bullet its own parser requires
  11. Non-required codecov/project and codecov/patch checks failed on patch coverage; disclosed as accounted-for, then closed anyway per the PR owner's explicit instruction

Note (Step 4b.1): the retrospective-gate-drift workflow's latest scheduled run (run 53, 2026-09-13T12:39:51Z, commit 9838da1) concluded failure. This retrospective's own filings below did not depend on that workflow; flagging it here per Step 4b.3's own disclosure rule.


  1. skill-audit-disclosure CI check failed shortly after PR feat(drafting-a-skill): spec.contract schema, contract generator, drift gate (Closes #1965) #1984 was opened, for a references/-only file.** Task 1's diff touched skills/evaluating-skill-quality/references/skill-metadata.schema.json, which independently triggers skill-audit-disclosure's applicability per issue feat(pipeline): enforce skill-invocation routing and plans-file traceability with deterministic gates #1796's own references-only-trigger rule, but the PR body's initial ## Skill audit evidence section did not yet disclose it; CI caught the gap after the PR was already open, fixed via a PR-body edit.
    Classification: missing deterministic gate.
    Status: missing-deterministic-gate
    Proposed gate: see gate-proposal-umbrella: skill-audit-disclosure post-push re-check gap #1800's own Planned ops (a post-push/re-open re-check of the PR body's Skill audit evidence section against the diff's current touched-file set, run locally before CI has to catch it).
    Filed as: gate-proposal: retro #2001 repair 1: skill-audit-disclosure CI check failed shortly after PR #1984 was opened for a references/**-only file #2002 (closed as a duplicate of gate-proposal-umbrella: skill-audit-disclosure post-push re-check gap #1800)

  2. Dispatched branch-plan-task self-reported local-preflight clean when 3 real gate findings existed. During Task 6, a dispatched branch-plan-task subagent self-reported "local preflight: clean" after landing commit e5f914c0, but the orchestrating session's own actual re-run of .github/scripts/gitapex_gate_local_preflight.py found 3 real findings (an uncaught MemoryError on a yaml.safe_load call, a function-body-test-coverage gap, and an 11-line patch-coverage gap), fixed in commit 4f2aeee7.
    Classification: missing deterministic gate.
    Status: missing-deterministic-gate
    Proposed gate: at executing-a-branch-plan Step 6's own main-thread merge-back point, mechanically re-invoke gitapex_gate_local_preflight.py against the merged task diff (not trust the dispatched task's own self-report) and fail loudly on any mismatch -- mirroring the precedent gitapex_check_task_commit_provenance.py already set for the same don't-trust-self-report, mechanically-re-verify principle in a different context (commit-message provenance).
    Filed as: gate-proposal: retro #2001 repair 2: Dispatched branch-plan-task self-reported local-preflight clean when 3 real gate findings existed #2003

  3. checker-script-adversarial-review found a marker-own-line false negative in the new skill-contract generator. Task 3's _locate_markers() located each contract marker by bare substring position only, never verifying the matched marker occupies its own line, letting content appended on the same line as a marker silently escape both region-replacement and --check's own drift comparison; also produced corrupted output on two same-line markers and an uncaught ValueError on a missing trailing newline. Fixed in commit 11d2a3db with 6 defeat tests.
    Classification: missing deterministic gate.
    Status: missing-deterministic-gate
    Proposed gate: not proposed -- caught by this repository's own required checker-script-adversarial-review disclosure for skills/*/scripts/*.py changes, which is specifically designed to catch exactly this class of defect.
    Tag: review-worked-as-designed

  4. checker-script-adversarial-review/deterministic-gate-quality found an unverified sidecar-discovery-helper duplication risk. Task 6's discover_skill_dirs/_resolves_to_sibling_skill in gitapex_scan_ssot_schema.py are copied, not imported, from gitapex_scan_skill_metadata_schema.py's identically-named helpers, disclosed in prose but with no automated check keeping the two copies in sync. Fixed in commit 1c30bdf5 with 2 new parity tests. (A separately-investigated NUL-byte candidate from the same review pass did not reproduce against this repository's pinned Python 3.12.3 runtime -- no fix needed, not itself a repair.)
    Classification: missing deterministic gate.
    Status: missing-deterministic-gate
    Proposed gate: not proposed -- caught by the same required checker-script-adversarial-review/deterministic-gate-quality disclosure as repair 3.
    Tag: review-worked-as-designed
    Recurrence note: same required-review mechanism as repairs 3, 5, 6, 7, 9 below.

  5. checker-script-adversarial-review found 3 stale checker-count cross-references after a "both"->"all three" edit. Task 4's commit updated the checker-count prose consistently within the two files it touched, but left three other live procedure files (skills/drafting-a-skill/references/gitapex-cross-links.md, skills/executing-a-branch-plan/references/decomposition-and-dispatch.md, skills/scorer-gated-skill-edits/SKILL.md) still citing the old count. Fixed in commit 1b3ba78e.
    Classification: missing deterministic gate.
    Status: missing-deterministic-gate
    Proposed gate: not proposed -- caught by the same required checker-script-adversarial-review disclosure as repair 3.
    Tag: review-worked-as-designed
    Recurrence note: same required-review mechanism as repairs 3, 4, 6, 7, 9.

  6. design-doc-adversarial-review found 6 residual stale "Proof" mentions outside Task 9's own rewrite scope. Task 9 rewrote five sections of the skill-contract-form design doc from proof/## Proof to gates/## Gates, byte-accurate against the shipped artifacts, but left six unqualified Proof/proof.gates[] mentions stale in sections its own Branch Plan spec deliberately did not touch (Scope, Connection to the contract form, the eliciting-a-design prototype's own bullet, Testing, Decision record). Fixed in commit a5b0d170.
    Classification: missing deterministic gate.
    Status: missing-deterministic-gate
    Proposed gate: not proposed -- caught by this repository's own required design-doc-adversarial-review disclosure for docs/gitapex/specs/*.md changes, specifically designed to catch this class of cross-document staleness.
    Tag: review-worked-as-designed
    Recurrence note: same required-review-catches-drift thesis as repairs 3, 4, 5, 7, 9.

  7. executing-a-branch-plan Step 8's mandatory aggregate adversarial review found 7 distinct findings. A full-diff aggregate refactor/review pass (required by that skill's own Step 8) found: skills/drafting-a-skill/scripts was never in pytest's own testpaths/pythonpath/addopts, so 45 generator tests silently never ran under a bare pytest invocation; the shipped field's schema description/rendering described the wrong semantic axis; contractInvariant.gate's schema description named the wrong resolution target; gitapex_run_skill_contract_check.py could not see an orphaned marker region whose sidecar declares no spec.contract; the local-preflight gate-count regex missed a hyphenated compound; stale/duplicate gate-count prose after the 50th gate landed; and docs/glossary.md's Gates entry misplaced a sentence belonging to Handoff. All 7 fixed in commit 66e99407.
    Classification: missing deterministic gate.
    Status: missing-deterministic-gate
    Proposed gate: not proposed -- caught by executing-a-branch-plan's own mandatory Step 8 aggregate review, the primary named example this skill's own Step 4 rule (ADR 0004) cites for this tag.
    Tag: review-worked-as-designed
    Recurrence note: same required-review-catches-drift thesis as repairs 3, 4, 5, 6, 9.

  8. A Claude-Session commit-message trailer was added a second time within this PR, on a commit authored directly by the orchestrating session. This PR's history was rewritten twice for the same underlying defect class: once at Task 1's own merge-back (caught correctly by gitapex_check_task_commit_provenance.py, scoped by design to a dispatched task's own BASE..HEAD range at Step 6), and a second time later in the cycle when the orchestrating session itself authored a commit directly (outside any dispatched task's worktree) that again carried the trailer -- undetected by any deterministic gate until caught by direct session/human review, repaired via a second git commit-tree rewrite (force-with-lease), with the user's explicit prior confirmation via AskUserQuestion before the destructive history rewrite.
    Classification: missing deterministic gate.
    Status: missing-deterministic-gate
    Proposed gate: see gate-proposal: retro #1772 repair 1: Commit-message AI-provenance marker not caught pre-commit #1773's own Planned ops (a pre-commit hook running the same AI-provenance-marker scan gitapex_check_task_commit_provenance.py already applies at merge-back time, so a flagged commit message is rejected at authoring time regardless of whether it originates inside a dispatched task's worktree or the main thread).
    Filed as: gate-proposal: retro #2001 repair 8: A Claude-Session commit-message trailer was added a second time within one PR, on a commit made directly by the orchestrating session rather than a dispatched branch-plan-task #2004 (closed as a duplicate of gate-proposal: retro #1772 repair 1: Commit-message AI-provenance marker not caught pre-commit #1773; this is gate-proposal: retro #1772 repair 1: Commit-message AI-provenance marker not caught pre-commit #1773's own 4th disclosed recurrence)

  9. drafting-a-pr-to-merge Step 8's inner-layer fresh review found 6 distinct findings on the full PR diff. A 5-persona reviewing-an-artifact pass at low effort found: find_drift() called discover_contracts() three times per run instead of once; the escalation-table renderer did not escape a literal |; skills/drafting-a-skill/scripts was added to pythonpath/mypy_path but never wired into gitapex_run_precommit_mypy.py's MYPY_GROUPS or test.yml's own mypy job, so the new 633-line generator was never actually type-checked; two test function names were stale relative to their own corrected assertions; the ssot-schema-drift gate's own target[] registration never described the new sidecar sweep; and the ADR/generator docstring both claimed already-shipped work as "not yet built." All 6 fixed in commit b48ce3cf.
    Classification: missing deterministic gate.
    Status: missing-deterministic-gate
    Proposed gate: not proposed -- caught by drafting-a-pr-to-merge's own required Step 8 inner-layer review, the other named example this skill's own Step 4 rule (ADR 0004) cites for this tag.
    Tag: review-worked-as-designed
    Recurrence note: same required-review-catches-drift thesis as repairs 3, 4, 5, 6, 7.

  10. independent-review-pending CI check failed because the drafted PR-body verdict omitted the required "- Verdict: CLEAN" bullet its own parser requires. The first drafted ## Independent review verdict section used prose ("Verified commit: sha") without the bullet line gitapex_gate_independent_review_pending.py's own parse_verdict()/_VERDICT_RE regex requires; fixed by resubmitting a corrected PR-body edit, which passed on retry.
    Classification: missing deterministic gate.
    Status: missing-deterministic-gate
    Proposed gate: extend gitapex_gate_pr_body_preflight.py (or add a sibling local check invoked the same way) to import and run gitapex_gate_independent_review_pending.py's own parse_verdict() against the drafted body text, failing loudly before submission if it would not parse as a valid CLEAN verdict citing the current head SHA -- the same local-preflight-before-submit pattern already used for that preflight's other four checks.
    Filed as: gate-proposal: retro #2001 repair 10: independent-review-pending CI check failed because the drafted PR-body verdict omitted the required '- Verdict: CLEAN' bullet its own parser requires #2005

  11. Non-required codecov/project and codecov/patch checks failed on patch coverage; disclosed as accounted-for, then closed anyway per the PR owner's explicit instruction. Both checks failed on an earlier head; neither is a required status check for this repository (.github/rulesets/main.json names no codecov/* context), and the repository's own registered patch-coverage gate already passed both locally and in CI. This was disclosed via a PR comment as accounted-for rather than fixed. The PR owner then explicitly instructed adding tests to close the gap anyway; commit 6b72119f brought both flagged files to 100% line coverage, closing it against Codecov's own independently-computed target too.
    Classification: external/human decision that cannot be automated -- a subjective priority call (satisfy a non-required, stricter, independently-computed coverage target beyond what the repository's own registered gate requires) that no gate was missing to make.
    Status: external-human-decision

Refs #1984

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    needs-acmMissing an Acceptance Criteria Map (or an explicit waiver) -- see issue #357retrospective

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions