feat(drafting-a-skill): spec.contract schema, contract generator, drift gate (Closes #1965) - #1984
Merged
Merged
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 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. Comment |
…gate Refs #1965. gitapex_gate_plans_traceability.py requires a line-start "Source ACM row:" / "Source ACM rows:" citation, not a bulleted one.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1984 +/- ##
==========================================
+ Coverage 99.57% 99.58% +0.01%
==========================================
Files 171 174 +3
Lines 29071 29808 +737
Branches 3558 3648 +90
==========================================
+ Hits 28948 29685 +737
Misses 123 123 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Add $defs/contract (precondition, goal, invariants, gates, escalation, handoff) to skill-metadata.schema.json, wired as optional spec.contract, additionalProperties: false throughout. precondition[].id and the two free kebab-case fields (onFail, escalation[].to) reuse skillNameRef's pattern inline (not a $ref -- they are not sibling-skill references); handoff.next.skill/fallback and handoff.inline[]/optional[] are real skill references and do $ref skillNameRef. Prose fields share a new $defs/contractProse (minLength 1, maxLength 200, no embedded newline, anchored with \Z rather than a bare $ to avoid the trailing-newline dispensation already documented on shapeWaiverItem.reason in this file). Required: goal.endState, goal.check, handoff.next.skill. Add one sentence to the schema's top-level description stating that spec.contract is the block a build step projects into SKILL.md; the lifecycle block's own runtime-read invariant sentence is untouched. Add fixture tests: a full valid spec.contract passes; an unknown nested key, an oversized goal.check, an embedded newline in invariants[].text, an out-of-enum gates[].plane, and a missing handoff.next.skill each fail. The existing 29-real-sidecar drift gate still passes since spec.contract stays optional.
tvna
force-pushed
the
claude/issue-1965-a2t6b5
branch
from
September 13, 2026 12:15
c74d2a7 to
bb17d7f
Compare
…(refs #1965) Adds the five docs/glossary.md entries required before the spec.contract generator's heading strings can be fixed: Goal, Gates (the owner's 2026-09-13 name for the concept, distinct from the ACM's own "Proof method" column), Escalation, Handoff (kept explicitly distinct from the existing Portable Question Handoff and Decision handoff entries), and Invariants (the resolved rename of the repository-wide "Stop boundaries" section-heading convention, recorded as a superseded-terms note per establishing-ubiquitous-language's own Maintain-step precedent, since no standalone "Stop boundaries" entry existed to mark superseded directly).
) Adds gitapex_generate_skill_contract.py and its co-located test module: reads one skill's metadata/gitapex.yaml sidecar only, renders spec.contract into the marker-delimited region of that same skill's SKILL.md (default: rewrite in place, --check: diff and exit 1 on drift). A skill with no spec.contract declared is not a target (clean exit 0). Zero or duplicated gitapex:contract:begin/end markers fail loudly. import yaml is guarded per the #1076 pattern.
…rator (refs #1965) gitapex_gate_local_preflight.py's exception-handler-gap and function-body-test-coverage gates caught two real gaps in the previous commit: yaml.safe_load's own try block did not cover RecursionError/ MemoryError (the same alias-expansion risk gitapex_check_skill_shape.py's sidecar read already guards against), fixed by widening the except clause; and every touched function lacked a top-level tests/ coverage mention this gate can recognize, since it has no fallback for this repository's pre-existing co-located test convention (the same gap gitapex_check_skill_shape.py's own check_shape()/main() already disclose) -- disclosed inline per-function with '# function-body-test-coverage: WAIVED: ...', each already exercised by the co-located test_gitapex_generate_skill_contract.py.
….json (refs #1965) Add three new drift checks to gitapex_scan_ssot_schema.py, wired into find_drift(): find_contract_gate_drift (spec.contract.invariants[].gate and spec.contract.gates[].id resolve against .gitapex/ssot.json's own gates[].id, with null invariants[].gate exempt as an explicit prose-only disclosure; a resolving gates[] entry's own plane must be one of that ssot gate's own planes[], and shipped must be true exactly when plane is a hook plane -- pretooluse/posttooluse/stop), find_contract_precondition_ duplicate_ids (precondition[].id unique within one contract), and find_contract_handoff_drift (handoff.next.skill/fallback and every handoff.inline[]/optional[] entry resolve to a real skills/*/ directory). Sidecar discovery (discover_skill_dirs/discover_contracts) and skill-name resolution (_resolves_to_sibling_skill) adapt gitapex_scan_skill_metadata_ schema.py's own equivalent helpers rather than importing them, matching this repository's established convention that every .github/scripts/*.py script reading this sidecar keeps its own copy. A sidecar that is missing, unreadable, not valid YAML, or declares no spec.contract block is silently skipped -- validating sidecar shape stays skill-metadata- schema-drift's own job. PyYAML import is guarded per the #1076 pattern. Extend tests/test_gitapex_scan_ssot_schema.py with the 7 cases from the issue's own Proposed solution text plus defensive/scoping coverage, and pin that the new checks are a clean no-op against the real repository (no skill declares spec.contract yet).
… (refs #1965) The pre-push local preflight (49 gates) failed on Task 6's own commit (e5f914c) with 3 findings, all against diff-added lines the merge-base diff includes but the commit's own test additions did not yet cover: - exception-handler-gap: discover_contracts's own yaml.safe_load call covered OSError/UnicodeDecodeError/YAMLError/RecursionError but not MemoryError, reachable the same way RecursionError already is via a hostile sidecar's YAML alias-expansion ("billion laughs") shape. Added MemoryError to the same except tuple, matching this function's own already-established silent-skip degrade-gracefully convention for every other malformed-sidecar failure mode. - function-body-test-coverage: discover_skill_dirs had no test in the same diff mentioning it by name. Added direct tests plus a companion positive case. - patch-coverage: 11 diff-added lines across discover_skill_dirs, _contract_of, discover_contracts, find_contract_gate_drift, find_contract_precondition_duplicate_ids, and find_contract_handoff_drift had no covering test execution -- each is a defensive branch for a schema-invalid/malformed-sidecar shape this scanner deliberately tolerates rather than crashing on (per this module's own graceful-degradation convention). Added one targeted test per branch: a nonexistent skills_dir, a missing sidecar file, a non-dict spec, a non-string gate/precondition id, and a non-dict handoff block. Also ported the 4 PyYAML-import-guard tests (tests/test_gitapex_scan_skill_metadata_schema.py's own missing-PyYAML coverage) onto this module's own identical top-level import guard, which the same patch-coverage run flagged as uncovered on its own diff-added lines. 139 tests pass (127 existing + 12 new); local preflight 49/49 PASS.
…'s own line (refs #1965) Adversarial review of the Task 3 skill-contract generator (required by this repository's own checker-script-adversarial-review disclosure for skills/*/scripts/*.py changes) found a real false-negative in _locate_markers(): it located each marker by bare substring position only, never verifying the matched marker actually occupies its own line. Content appended directly after the begin marker on the same line let the line-end lookup skip straight past the injected text to that line's own end, silently placing it outside the computed region -- so a hostile or merely corrupted SKILL.md could carry undetected content past both region-replacement and --check's own drift comparison (--check reported clean on a file it should have flagged). The same gap let two same-line markers compute region_start greater than region_end, which apply_region's own slice-and-concatenate would have turned into duplicated, corrupted output, and let a begin marker with no trailing newline crash with an uncaught ValueError instead of this module's own controlled GenerationError. Fixed by verifying each marker's own line, stripped, equals that marker exactly, closing all three defects with one check. Added 6 defeat tests reconstructing each defeat shape (including an end-to-end check-mode reproduction of the finding's actual real-world impact) plus 2 lightweight parity tests. 45 tests pass (40 existing plus 5 new unit tests, all pre-existing tests still green).
…gainst their sibling (refs #1965) deterministic-gate-quality review of Task 6's new contract-checking logic in gitapex_scan_ssot_schema.py flagged its documented-but- unverified duplication: discover_skill_dirs and _resolves_to_sibling_skill are copied, not imported, from gitapex_scan_skill_metadata_schema.py's own identically-named helpers (both modules' own docstrings already say so), but nothing actually kept the two copies in sync the way this same test file's own test_policy_source_format_literal_matches_schema_enum already does for a different duplicated-value pair. Adds two parity tests running both implementations against the same fixture inputs (real/missing skill directories, empty/dot/dot-dot names, path-traversal and separator-bearing names) and asserting identical output, so a future one-sided edit to either copy is caught here rather than silently diverging. The same review's other flagged concern -- an embedded NUL byte in a handoff reference reaching Path.is_file() -- was verified against this repository's own pinned Python 3.12.3 runtime (both via a live reproduction and by reading pathlib's own is_file() source, which explicitly catches ValueError as "Non-encodable path" and returns False) and does not actually raise; no code change needed for that part of the finding. 141 tests pass (139 existing plus 2 new); local preflight 49/49 PASS.
…(refs #1965) drafting-a-skill now declares its execution dependency on Task 3's skill-contract generator (skills/drafting-a-skill/scripts/ gitapex_generate_skill_contract.py) and names it by exact path at the two points a future skill migration would actually invoke it: - metadata/gitapex.yaml: spec.dependencyPolicy: Declared and spec.executionRequirements.packages.pip: [pyyaml], matching evaluating-skill-quality's own precedent for the same shape. A decision-log entry records the addition, citing issue #1964's spec.contract projection work. - SKILL.md Step 2: after spec.contract is written into a drafted skill's sidecar, render it into that skill's SKILL.md marker region with the generator (write mode, no --check), in the same edit round. - SKILL.md Step 6: the generator's --check mode joins the two pre-existing checkers in this skill's own mandatory checker sweep -- safe to run unconditionally, since a sidecar with no spec.contract key is the generator's own documented "not a target" no-op. The surrounding "both checkers" prose (the Step 6 table row, the Postcondition, the worked example) is updated to "all three" to match, avoiding a stale count now that a third checker exists. Applied this skill's own Step 6 checker sweep to the edited SKILL.md itself, per this file's self-editing exception. All three checkers pass except the pre-existing, disclosed --strict-token-budget FAIL (issue #1792, advisory-only outside --strict mode, confirmed present before this change via git stash: 8849 tokens over budget pre-edit vs. 8947 post-edit -- these two added lines cost the budget further but do not introduce a new failure class). No compatibility/prerequisites prose section exists in this SKILL.md to name PyYAML in (only the frontmatter compatibility field, scoped to disable-model-invocation semantics) -- skipped per the task's own explicit sub-step, not invented. This produces one disclosed, non-blocking warning from gitapex_scan_execution_requirements_drift.py (packages-pip-vs-compatibility, exit 0).
…er Task 4 (refs #1965) checker-script-adversarial-review of commit a47526e (Task 4: declaring and using the skill-contract generator) found the "both" to "all three" checker-count update was applied consistently within the two files that commit touched, but not swept across three other live procedure files that cite the same fact: - skills/drafting-a-skill/references/gitapex-cross-links.md's own "Deterministic-checker commands (Step 6)" section still listed only the two pre-existing commands and said "Both are read-only" -- added the third command and updated the count. - skills/executing-a-branch-plan/references/decomposition-and-dispatch.md still said "Step 6's own two checker scripts". - skills/scorer-gated-skill-edits/SKILL.md's own Step 3 still parenthesized Step 6's completion bar as "(shape and drift checkers clean)", omitting the new third checker. scorer-gated-skill-edits/SKILL.md's own Step 3 documents a mandatory cross-reference sweep specifically for this failure class (an enumerated/ordinal citation going stale elsewhere when the source of truth changes), citing a real prior incident -- this commit closes the same class of gap the review found, in the same spirit that rule exists for. No change to docs/superpowers/plans/*.md or docs/superpowers/specs/*.md: those are dated, point-in-time plan/design artifacts, not live procedure text, and are deliberately left as historical record. Verified: full local preflight 49/49 PASS.
…1965) Registers skill-contract-drift in .gitapex/ssot.json (kind script, planes ci/local, cluster skill-lifecycle) so drift between a contract-declaring skill's committed SKILL.md marker region and its sidecar's spec.contract is caught deterministically, not left as an unenforced schema shape (ADR 0005's own Confirmation section named this gate as prospective work; this lands it). The generator (skills/drafting-a-skill/scripts/ gitapex_generate_skill_contract.py, Task 3) only accepts one skill directory per invocation and, by its own portability constraint, never reads .gitapex/ssot.json or sweeps the repository itself. The new .github/scripts/gitapex_run_skill_contract_check.py wrapper is the sweep: it discovers every contract-declaring skill via gitapex_scan_ssot_schema.discover_contracts (the same discovery the sibling ssot-schema-drift gate already uses, so the two gates cannot disagree on scope) and re-invokes the generator's own --check CLI once per discovered skill as a real subprocess -- the same external-tool-per-group shape gitapex_run_precommit_mypy.py already established, so this wrapper's own mypy invocation never needs to cross-resolve into skills/drafting-a-skill/scripts. tests/test_gitapex_skill_contract_drift.py covers both layers: the generator's own --check mode against a synthetic tmp_path fixture (positive: matches a fresh regeneration, exits 0; negative: a hand-edited region drifts, exits 1) plus the wrapper's own sweep/aggregation logic (pass, failure, timeout) with discover_contracts/check_skill monkeypatched, and a real-repository sanity check confirming zero skills declare spec.contract yet (a foundation-only PR, matching test_gitapex_scan_ssot_schema.py's own equivalent pin). No new CI workflow file: this follows skill-metadata-schema-drift's own established pattern of running a full-repository-state gate as a plain pytest file inside test.yml's existing pytest step, rather than inventing a new workflow entry point for another full-sweep gate of the same shape. skills/drafting-a-skill/scripts is added to pyproject.toml's [tool.mypy] mypy_path (not pythonpath, which would require syncing gitapex_run_precommit_mypy.py's MYPY_GROUPS and test.yml's mypy job per their own drift-checked parity) so the test file's own bare import of the generator resolves under strict mypy. Registering a 50th wired gate required updating every prose count that hardcoded "49 wired gates" (CONTRIBUTING.md, .pre-commit-config.yaml, gitapex_gate_local_preflight.py, its own test) plus the now-stale "47 of the 49 wired gates carry ci" ratio (48 of 50, verified against the live registry) -- test_no_prose_count_contradicts_the_registry catches exactly this class of drift.
…1965) review-persona screening of commit 88a3e34 found two issues: - patch-coverage: gitapex_run_skill_contract_check.py had no covering tests/test_gitapex_run_skill_contract_check.py by its own stem, even though its logic is exercised by tests/test_gitapex_skill_contract_drift.py (the skill-contract-drift gate's own registered trigger file, named after the gate id, covering both scripts the gate's own script[] array names). Discloses this the same way the file's own pre-existing function-body-test-coverage: WAIVED comments already do -- as a real comment token outside the module docstring, not inside it (a WAIVED marker written inside a triple-quoted string is not a comment token tokenize recognizes, so the gate correctly ignored the first attempt). - CONTRIBUTING.md's historical pre-push-timing list gained a new "the prior 49-gate set measured roughly 24 seconds" line reusing the adjacent 48-gate entry's own figure verbatim, with no git-history evidence (checked via reflog) that a 49-gate state was ever actually timed. The surrounding list is introduced as "all are warm-run measurements, not a strict budget" -- an unverifiable reused figure contradicts that claim. Dropped the line rather than assert an unmeasured number as fact. Verified: full local preflight 50/50 PASS; targeted test run (136 tests) passes.
…d structure (refs #1965) The design doc's "The contract model", "The rendered SKILL.md", "The gates", "Vocabulary", and "Residual risks" sections still described the pre-implementation draft (proof/`## Proof`, an under-specified gate-id resolution rule, an unresolved term-collision risk) rather than what Tasks 1, 3, 5, 6, and 8 of this branch actually shipped: - The contract model: proof -> gates throughout; the yaml example now matches skill-metadata.schema.json's own `$defs/contract` shape exactly (required goal/handoff, optional precondition/invariants/ gates/escalation, gates[] as {id, plane, shipped}, downstream living under handoff rather than under gates/proof). - The rendered SKILL.md: heading list now cites the real generator (gitapex_generate_skill_contract.py) and docs/glossary.md's own Goal/ Gates/Escalation/Handoff/Invariants entries verbatim; the marker example no longer carries an inline "do not edit" comment the generator actually rejects; added the per-block rendering rules (empty-list conventions, the Escalation bullet/table threshold) read directly from the shipped _render_* functions. - The gates: rewritten to name the actual skill-contract-drift gate shape (including its sweep wrapper) and the three real ssot-schema-drift scanner functions Task 6 shipped (find_contract_gate_drift, find_contract_precondition_duplicate_ids, find_contract_handoff_drift), replacing the single generic "gate-id resolution" placeholder. - Vocabulary: records the Gates decision (2026-09-13) and the goal-stays-singular rationale, matching docs/adr/0005-spec-contract-projection.md's own Decision Drivers/ Decision Outcome, and defers to issue #1965 as the authoritative record of the naming decision rather than restating it as this document's own call. - Residual risks: marks the Proof/Gates term-collision risk resolved, citing the glossary entry and the ADR. Every other section (Scope, Connection to the contract form, First prototype, Testing, Decision record, Evidence) is left untouched -- this is a rewrite-to-match-shipped-reality pass over exactly the six sub-steps the Branch Plan's Task 9 names, not a general editing pass. Verified: all 50 wired `gitapex_gate_local_preflight.py` gates pass; the design-doc-pattern-dryrun gate (one of the gates whose own target glob covers docs/gitapex/specs/*.md) passes when run directly against this diff's added lines; the full test suite (9212 tests) passes with no new failures relative to the pre-existing baseline on this branch. Refs #1965.
…tside Task 9's own scope (refs #1965) design-doc-adversarial-review of commit 37ea0ac found the five sections Task 9 rewrote are byte-accurate against the shipped schema/ generator/scanner/glossary, but flagged a real cross-document consistency problem the rewrite left behind: six unqualified `Proof`/ `proof.gates[]` mentions survive in sections Task 9's own Branch Plan spec deliberately did not touch (Scope, Connection to the contract form, the eliciting-a-design prototype's own agreed-shape bullet, Testing, and the Decision record) -- now inconsistent with the document's own rewritten Architecture/Vocabulary sections, which explicitly name `Proof` as the superseded term and `Gates` as current. Fixed all six: Scope's own contract-element list and gate-id-check description, the "Connection to the contract form" paragraph, the eliciting-a-design prototype's own Gates line, the Testing table's gate-id row, and the Decision record's "survives as" sentence and its own adopted-shape element list (the latter now explicitly notes `Proof` was the handoff notes' own original wording, since that sentence describes a historical artifact, not current state). The three Vocabulary/Residual-risks mentions of `Proof` that Task 9 already wrote are untouched -- those are deliberate historical citations of the superseded term, not staleness. Verified: full local preflight 50/50 PASS.
…ndings Fixes 9 confirmed findings from the mandatory executing-a-branch-plan Step 8 aggregate refactor/review pass over the full accumulated diff: - skills/drafting-a-skill/scripts was never in pytest's own testpaths/ pythonpath/addopts, so its 45 generator tests silently never ran under a bare `pytest` invocation. - The `shipped` field's schema description and generator rendering described a runtime-enforcement-state axis; issue #1965's own Proposed solution 1(c) defines it as a distribution axis (ships to a consumer install vs. this repository's own dev-time tooling) instead. - contractInvariant.gate's schema description claimed a same-document cross-reference to spec.contract.gates[]; the approved Task 6 design and the scanner's own find_contract_gate_drift both resolve it against the global .gitapex/ssot.json registry, the same target gates[].id itself resolves against. - gitapex_run_skill_contract_check.py could not see a SKILL.md carrying the generator's own marker pair whose sidecar declares no spec.contract at all -- discover_contracts only ever looks at the sidecar side. Added _orphaned_marker_skills to close that gap, with positive/negative test coverage. - The local-preflight gate-count claim documentation regex only matched a bare "N excluded/wired" count, missing a hyphenated compound like "N currently-excluded". - Corrected the registered/excluded gate counts (87 total, 37 excluded) in gitapex_gate_local_preflight.py's own module docstring, and removed a duplicate/conflicting unverified-timing line its own comment block had reintroduced. - docs/glossary.md's Gates entry misplaced a sentence that actually belongs to Handoff's own downstream field; cross-referenced instead. Refs #1965
tvna
force-pushed
the
claude/issue-1965-a2t6b5
branch
from
September 13, 2026 23:19
a7878e3 to
01056de
Compare
Owner
Author
codecov/project, codecov/patch: not required, investigated, no fix neededBoth The repository's own actual coverage gate is the registered
No code change made; disclosing per this session's CI-failure-wake Generated by Claude Code |
…indings Fixes 6 confirmed findings from the fresh, 5-persona independent review of this PR's full diff (issue #1965's own foundation-task PR): - gitapex_scan_ssot_schema.py's find_drift() called discover_contracts() three times per run (once inside each of find_contract_gate_drift, find_contract_precondition_duplicate_ids, find_contract_handoff_drift), re-sweeping and re-parsing every sidecar three times over -- the exact redundant-rescan shape gitapex_scan_skill_metadata_schema.py's own module docstring already documents fixing once for its sibling _requires_graph helper. All three now take an optional contracts parameter, computed once by find_drift and shared. - gitapex_generate_skill_contract.py's escalation-table renderer did not escape a literal "|" in escalation[].when (a free contractProse field), which would corrupt the rendered Markdown table's cell boundaries. - skills/drafting-a-skill/scripts was added to [tool.mypy] mypy_path and [tool.pytest.ini_options] pythonpath in an earlier fix round, but never wired into gitapex_run_precommit_mypy.py's MYPY_GROUPS or .github/workflows/test.yml's own mypy job -- the new 633-line generator was never actually type-checked. Wiring it in surfaced one real, latent unused "type: ignore" comment, now removed. - Two test function names in test_gitapex_generate_skill_contract.py still said "already_enforcing"/"not_yet_enforcing", stale relative to the corrected rendered text ("shipped with the plugin"/"gitapex repository only") their own assertions already check. - The ssot-schema-drift gate's own .gitapex/ssot.json registration never updated its target[] to describe the new spec.contract sidecar sweep and gate-id/plane/shipped cross-registry checks its script now performs. - docs/adr/0005-spec-contract-projection.md and the generator's own module docstring both claimed the generator, its --check gate, and the ssot-scanner gate-id checks were "not yet built" -- all three in fact ship in this same PR. Two additional candidates were investigated and disclosed rather than fixed: an empty `spec.contract: {}` is treated as "not declared" by discover_contracts but as "declared, fails validation" by the generator's own load_sidecar_contract -- backstopped in practice by skill-metadata-schema-drift's own schema rejection of that shape, and an existing test pins the current discover_contracts behavior as deliberate; and a YAML alias-expansion ("billion laughs") resource- exhaustion risk in sidecar parsing, already disclosed in-code and no worse than the pre-existing sibling scanner's own identical exposure. Refs #1965
Adds error-path and adversarial-input test coverage for the 18 lines Codecov flagged as missing on this PR's diff (issue #1965): - skills/drafting-a-skill/scripts/gitapex_generate_skill_contract.py (16 lines, now 100%): non-UTF-8 sidecar, invalid YAML syntax, a non-mapping YAML root, precondition/invariants/gates fields of the wrong type, a write-mode OSError, and the missing-PyYAML import guard (mirroring gitapex_scan_execution_requirements_drift.py's own identical guard's test suite). - .github/scripts/gitapex_run_skill_contract_check.py (2 lines, now 100%): _orphaned_marker_skills own silent-skip on a non-UTF-8 SKILL.md. Both files own repository-wide patch-coverage gate already passed before this change; this closes the gap against Codecov's own, independently-computed target too. Refs #1965
Owner
Author
Independent review verdict (archived -- round ending at commit b48ce3c)
Inner layer ( Fixed:
Investigated and disclosed, not fixed:
Outer layer (GitHub-native reviewer): this repository does not have Generated by Claude Code |
tvna
marked this pull request as ready for review
September 14, 2026 03:40
This was referenced Sep 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Foundation for the skill contract form (PR1 of tracking issue #1964):
adds the
spec.contractsidecar schema block, a bundled generator thatprojects it into a marker-delimited region of a skill's
SKILL.md, aCI-enforced drift gate, gate-id resolution checks in the ssot scanner,
drafting-a-skill's dependency declaration, glossary entries for thesix contract headings, an ADR recording the projection decision, and a
rewrite of the design doc to match the finalized structure. No real
skill is migrated in this PR -- zero production targets, by design.
All 9 Branch Plan tasks complete (Task 7 disclosed as BLOCKED per
its own Stop boundary, not silently skipped -- see Facts).
Facts
planning-a-branch-from-an-issuethis session, independentlyre-verified against primary sources (issue feat(drafting-a-skill): spec.contract schema, bundled contract generator, and drift gate (skill contract form, PR1) #1965 body, issue tracking(skills): contract form for the core pipeline skills (spec.contract as source, generated SKILL.md block) #1964
body, repository file contents) rather than trusted as drafted. The
re-verification marker is recorded on issue feat(drafting-a-skill): spec.contract schema, bundled contract generator, and drift gate (skill contract form, PR1) #1965's own body.
interface-dependency edges computed before wave assignment) is
recorded in
docs/gitapex/plans/2026-09-13-issue-1965-a2t6b5.md.main, carried in by PR feat(pipeline): reduce redundant verification layers per ADR 0004 (Closes #1970) #1979(PR0, issue feat(pipeline): reduce redundant verification layers (skill contract form, PR0) #1970) rather than by this PR.
architecture-tradeoff, owner-approved,that the gate-id resolution check (Task 6) stays in
.github/scripts/gitapex_scan_ssot_schema.pyrather than moving intoany skill's bundled
scripts/, consistent with all 17 sibling.github/scripts/gitapex_scan_*.pyscanners.git commit-tree,tree content unchanged, force-with-lease pushed) to remove an
inadvertently-included
Claude-Session:trailer from three commitmessages -- CONTRIBUTING.md's PR-body trailer disclosure section states
that exception does not extend to commit messages (hard block, not a
judgment call). Caught by
gitapex_check_task_commit_provenance.pyduring Task 1's own merge-back screening. The task-list commit's SHA
changed as a result (
06b89d88->a6c8b7f0); see the Execution lognote below.
skill-audit-disclosureCI check failed because Task 1's diff touchesskills/evaluating-skill-quality/references/skill-metadata.schema.json(a
references/**file), which independently triggers this gate'sapplicability regardless of whether any
SKILL.mdchanged (issuefeat(pipeline): enforce skill-invocation routing and plans-file traceability with deterministic gates #1796's own references-only trigger). See the
## Skill audit evidencesection below.asserted
docs/glossary.mdalready had a standaloneStop boundariesentry to rename into
Invariants. No such entry exists (verified byheading grep) --
Stop boundariesis a repository-wideSKILL.mdsection-heading convention (prose usage across 100+ files), not a
prior glossary entry, matching this glossary's own existing
Dimension/Shape checkprecedent. The newInvariantsentryrecords the supersession note accordingly. Documentation-mechanics
correction, not a genuine terminology conflict -- no owner escalation
needed.
scorer-gated-skill-editsprecondition (checkable scorer,held-out split, a working
evals/scripts/gitapex_run_eval_suite.py)was confirmed firsthand this session and could NOT be met: no
ANTHROPIC_API_KEY(orHTTP_EXECUTOR_*equivalent) is configured inthis environment.
--helpon the runner resolves cleanly, but anactual live trial against
evaluating-skill-quality's own real suite(98 fixtures) fails on the first model-CLI subprocess call
(
error: model CLI exited 1, underlying cause "Not logged in -Please run /login") -- confirmed both via the runner itself and a
direct reproduction of its hermetic subprocess invocation. Per this
row's own explicit instruction, this is not worked around with a
direct edit:
skills/evaluating-skill-quality/references/rubric.mdis UNCHANGED, no commit was made for this row, and it is disclosed
here as BLOCKED rather than closed.
docs/adr/0005-spec-contract-projection.md) records theprojection decision citing Task 1's actual shipped schema shape, the
goal-stays-singular rationale, and theproof-to-Gatesrenameciting
docs/glossary.md'sGatesentry. StatusProposed.(
skills/drafting-a-skill/scripts/gitapex_generate_skill_contract.py)lives under
skills/*/scripts/, soscreening-a-low-trust-contribution'scheck 4 (hook/script changes) reports a HARD FLAG by category match
-- unconditional per that check's own rule, regardless of content.
Substantive
review-personareview of the actual code found: readsonly the one target skill's own
metadata/gitapex.yaml+SKILL.md;writes only that same
SKILL.md's marker-delimited region; zeroeval/exec/subprocess/pickleanywhere in either file;yaml.safe_loadused (not unsafeyaml.load). Two disclosed wordingjudgment calls in the code's own docstrings: the
Gatesblock'sparenthetical renders "already enforcing"/"not yet enforcing"
(matching the schema's own documented
shippedsemantics) ratherthan the Branch Plan's draft "shipped with the plugin"/"gitapex
repository only" phrasing;
handoff.inline/optionalempty arraysare suppressed (no bullet) rather than rendering
- none, matchinghow
goal.constraints's own empty case is handled elsewhere..github/scripts/gitapex_scan_ssot_schema.py's gate-id/plane/shipped/precondition-uniqueness/handoff-resolution checks) landed in
three commits:
e5f914c0(the checks themselves, 22 new tests),4f2aeee7(a same-session fixup closing 3 local-preflight findingsthe first commit's own author agent had reported clean --
exception-handler-gap,MemoryErrorunhandled on the sidecar's ownyaml.safe_load;function-body-test-coverageandpatch-coverage,12 more tests), and
1c30bdf5(2 parity tests, see below).checker-script-adversarial-review/deterministic-gate-qualityranvia
review-personaagainst Task 6's new detection logic. Itreported one candidate finding -- an embedded NUL byte in a
handoffreference potentially crashing
_resolves_to_sibling_skill'sPath.is_file()call with an uncaughtValueError-- which thissession verified against primary sources (this repository's own
pinned Python 3.12.3: a live reproduction, plus reading
pathlib.Path.is_file()'s own source, which explicitly catchesValueErroras "Non-encodable path" and returnsFalse) and founddoes NOT reproduce; no code change was needed. The same review's
other note --
discover_skill_dirs/_resolves_to_sibling_skillarecopied, not imported, from
gitapex_scan_skill_metadata_schema.py'sown identically-named helpers, disclosed in prose but with no
automated check keeping the two copies in sync -- was real and is
closed by commit
1c30bdf5's 2 new parity tests. Separately, thesame adversarial-review pass applied to Task 3's generator found a
real, distinct defect:
_locate_markers()located each contractmarker by bare substring position only, never verifying the match
actually occupies its own line, so content appended directly after a
marker on the same line was silently excluded from the computed
region -- a false negative letting
--checkreport clean on a fileit should have flagged, plus two secondary defects (corrupted output
when both markers share one line; an uncaught
ValueErrorcrashinstead of a controlled error on a missing trailing newline). Fixed
in commit
11d2a3dbwith 5 new defeat tests.drafting-a-skilldeclares and uses the generator) landed asa47526ea:spec.dependencyPolicy: Declared+spec.executionRequirements.packages.pip: [pyyaml]on the sidecar;one new line each at SKILL.md Step 2 (render after writing
spec.contract) and Step 6 (run--checkalongside the twopre-existing checkers, "both" -> "all three" updated at four sites).
The compatibility-prose sub-step in the Branch Plan's own text was
skipped, disclosed rather than silently done: this
SKILL.mdcarriesno such prose section at all (removed repository-wide under issue
refactor(drafting-a-skill): consolidate mislabeled on-demand reference files into one required file #1882).
--strict-token-budgetalready failed pre-existing (issuedrafting-a-skill's SKILL.md has never cleared --strict-token-budget since the check was introduced #1792, confirmed via
git stashre-run); disclosed, not fixed, perthis row's own explicit instruction.
checker-script-adversarial-reviewfound the commit's own internal "both"->"all three" edit left three
OTHER live procedure files citing the same fact stale -- fixed in
1b3ba78e.adversarial-coverage-mappingfound no newinjection-resistance gap.
skill-contract-driftas a CI-enforced gate) landedas
88a3e341, with a fixup831a5857. The generator only acceptsone
skill_dirat a time, so a new wrapper script(
.github/scripts/gitapex_run_skill_contract_check.py) discoversevery contract-declaring skill via
gitapex_scan_ssot_schema.discover_contracts()and subprocess-invokes the generator's
--checkper skill, followingthe established
gitapex_run_precommit_mypy.pyper-group-wrappershape. No new
.github/workflows/*.yml: followsskill-metadata-schema-drift's own established pattern of runninginside the existing
test.ymlpytest step. Landing a 50th gate broke4 files' own hardcoded "49 wired"/"47 of 49" prose, recomputed
against the live registry (48 of 50 wired gates carry
ci) and fixedin the same commit.
deterministic-gate-qualityreview found the newwrapper fails closed on every traced malformed-input path (subprocess
timeout, a generator crash, an unexpected
discover_contracts()shape); two non-blocking UX-polish notes disclosed, not fixed; one
unverifiable reused timing figure in
CONTRIBUTING.mddropped ratherthan asserted as fact, in
831a5857.37ea0ac8: everyproof/## Proofreference in the five sectionsthe Branch Plan named ->
gates/## Gates; the yaml example matches$defs/contractexactly; the rendered-SKILL.mdheading list andper-block rendering rules match Task 3's actual shipped functions;
"The gates" section matches Task 6's actual shipped checks;
Vocabulary records the 2026-09-13 Gates decision citing ADR 0005
(status
Proposed, never overclaimed asAccepted); Residual risksmarks the term-collision risk Resolved.
design-doc-adversarial-review(this repo's own required disclosure whenever a
docs/gitapex/specs/*.mdfile changes) confirmed all five rewritten sections byte-accurate
against their shipped artifacts, and separately flagged a real
cross-document consistency gap: six unqualified
Proof/proof.gates[]mentions survived in sections the Branch Plan's own Task 9 spec
deliberately did not touch (Scope, Connection to the contract form,
the
eliciting-a-designprototype's own agreed-shape bullet, Testing,Decision record) -- now stale relative to the rewritten sections.
Closed in follow-up commit
a5b0d170, leaving only the deliberatehistorical citations of the superseded
Proofterm already presentin the Vocabulary/Residual-risks sections and one new explicit
historical citation (the owner's original handoff notes used
Proof) added in that same fixup.Assumptions
pinned by golden-text tests against the glossary wording Task 8
shipped; a later reviewer disagreement on the exact rendered text is
a two-file change (generator + its tests), not a redesign.
Risk / blast radius
Touches: one schema file (
skill-metadata.schema.json, additive only),one new ADR, one new bundled script pair under
skills/drafting-a-skill/scripts/(hard-flagged by category, reviewedclean in substance, one real defect found and fixed), two new lines
each at
skills/drafting-a-skill/SKILL.mdSteps 2/6 plus its sidecar,one
.gitapex/ssot.jsonentry plus a new wrapper script and test file(also hard-flagged by category, reviewed clean, fails closed on every
traced malformed-input path), one
.github/scripts/gitapex_scan_ssot_schema.pyextension plus its testfile (also hard-flagged by category, reviewed clean),
docs/glossary.mdentries, three live procedure files' own stale-citation fixes, four
prose-count fixes after the 50th gate landed, and a full rewrite of one
design doc plus a residual-staleness follow-up (byte-accurate against
every shipped artifact per independent review).
skills/evaluating-skill-quality/references/rubric.mdis NOT touched(Task 7 blocked, see Facts). No
.github/workflows/file, no hook, andno existing
SKILL.md's frontmatterdescription:line is touched byanything in this PR -- both changed
SKILL.mdfiles (drafting-a-skill,scorer-gated-skill-edits) keep their own description unchanged,body-prose-only edits. Zero real skills declare
spec.contractyet, sothe new
skill-contract-driftgate and the new gate-id resolutionchecks are both clean no-ops against the real repository today, by
design.
Rollback
Revert the merge commit with
git revert. The schema addition isoptional (
spec.contractis not required), the generator and gate havezero production targets, and no existing file's prior behavior is
altered -- a revert removes this PR's additions cleanly with no
downstream file depending on them yet.
Verification
Acceptance Criteria Map (issue #1965's own draft, independently
re-verified this session; row 9 added during that re-verification, not
in the issue's own draft). All 9 rows now resolved (8 PASS, 1
disclosed BLOCKED per its own Stop boundary).
spec.contractis expressible in the sidecar schema$defs/contractwith the six blocks,additionalProperties: false, optional underspec; existing sidecars stay validskill-metadata.schema.json; extend its description; no change to the lifecycle invariant texttests/test_gitapex_scan_skill_metadata_schema.pypasses on all 29 existing sidecars plus fixture positive/negative cases. Result: PASS (Task 1, commitdcf32779) -- 102 tests passed, screened clean byreview-persona(checks 2-8, no flags)docs/adr/0005-*.mdin the repository's MADR-derived templatedrafting-an-adre2e3a635) --gitapex_check_adr_shape.pyPASS, screened clean byreview-persona--checkexits 1 on drift; no-target skills untouched; 0 or 2+ marker pairs failskills/drafting-a-skill/scripts/gitapex_generate_skill_contract.pyand its co-located tests--checkpass/fail; marker-count failures; no-target skip; bundled-script shape checks pass. Result: PASS (Task 3, commitsb20d4dea,8f8a18e1,11d2a3db) -- 45 tests passed, shape checker 56/56;checker-script-adversarial-reviewfound a real marker-own-line false negative, fixed with 5 defeat testsdrafting-a-skilldeclares and uses the generator--checkcallsskills/drafting-a-skill/metadata/gitapex.yamlandSKILL.mdgitapex_scan_execution_requirements_drift.py skills/drafting-a-skillpasses; skill-audit-disclosure recorded. Result: PASS (Task 4, commitsa47526ea,1b3ba78e) -- drift scanner clean,review-personafound and fixed a real stale-cross-reference gap,adversarial-coverage-mappingfound no new injection-resistance gapdrafting-a-skillalready fails--strict-token-budget(issue #1792), confirmed pre-existing; not blocked by it, per this row's own explicit instructionskill-contract-driftin.gitapex/ssot.jsonwith a pytest trigger; the invariant and its gate ship in the same changetests/test_gitapex_skill_contract_drift.pyusing a synthetic fixture skilltests/test_gitapex_scan_ssot_schema.pyaccepts the entry; new test fails on a drifted fixture, passes on a matching one. Result: PASS (Task 5, commits88a3e341,831a5857) -- no drift, 149 tests passed on the targeted run, local preflight 50/50 (new gate confirmed appearing and PASSing by name);deterministic-gate-qualityfound no blocking findings on every traced malformed-input pathinvariants[].gateandgates[].idmust match agates[].id;nullallowed for invariants; plane/shipped checked;precondition[].idunique;handoffskill names resolve.github/scripts/gitapex_scan_ssot_schema.pye5f914c0,4f2aeee7,1c30bdf5) -- 141 tests passed; one candidate finding did not reproduce (verified against primary sources), one real duplication-drift gap closed with 2 parity testsrubric.mdat the two sites throughscorer-gated-skill-editsgitapex_check_skill_shape.pyonevaluating-skill-qualitypasses; diff shows exactly two sites touched. Result: BLOCKED (Task 7) --scorer-gated-skill-edits's Precondition gate cannot be satisfied in this environment (noANTHROPIC_API_KEY; live eval-runner trial fails with "Not logged in", confirmed firsthand).rubric.mdunchanged, no commit madedocs/glossary.mdestablishing-ubiquitous-language; add glossary entriesf0d9eeb4) -- 5 entries added, no synonym collisions found, screened clean byreview-personaGateswas decided 2026-09-13;Handoffexplicitly distinguished from two existing qualified entries;Stop boundarieshad no standalone entry to renameproof/## Proof; must be rewritten togates/## Gatesand the finalized yaml shape37ea0ac8,a5b0d170) --design-doc-adversarial-reviewconfirmed every rewritten section byte-accurate against the shipped artifacts; a real residual-staleness gap in untouched sections was found and closed in the same PRChecklist
## Skill audit evidencesection discloses required verdicts/waiversevals/*/split.md-- not applicableSkill audit evidence
SKILL.mdfiles(
drafting-a-skill,scorer-gated-skill-edits) keep their ownfrontmatter
description:line unchanged (confirmed by diff), sothis WAIVED form stays legal per this gate's own issue feat(skill-audit): implement two gate extensions proposed in retrospective #422 #427 rule.
Both edits are small, additive, and non-behavioral: two new lines
naming an already-shipped, already-reviewed script by exact path
(
drafting-a-skill), and a parenthetical word-count correction withno procedural meaning change (
scorer-gated-skill-edits). Seeadversarial-coverage-mappingbelow for the security-relevance-specificcheck that WAS run.
battle-testing-a-skill above.
SKILL.mdfiles were edited directly by the executing session, notby invoking
drafting-a-skillon itself or onscorer-gated-skill-edits.drafting-a-skillis not itselfauthoring a brand-new skill here.
drafting-a-skill(Task 4: nonew injection-resistance gap from the two new generator references)
and
scorer-gated-skill-edits(the one-line cross-reference fix: apure checker-name/count correction, no new call site or argument
surface).
docs/gitapex/specs/2026-09-12-skill-contract-form-design.md(Task 9). All five rewritten sections confirmed byte-accurate
against the shipped schema/generator/scanner/glossary/ADR 0005 by
independent review; one real residual-staleness gap (six leftover
Proofmentions outside the rewrite's own scope) found and closedin the same PR (
a5b0d170)..github/scripts/gitapex_scan_ssot_schema.py,skills/drafting-a-skill/scripts/gitapex_generate_skill_contract.pyand its co-located test file, and
.github/scripts/gitapex_run_skill_contract_check.py. One candidatefinding (an embedded-NUL-byte crash path) did not reproduce against
this repository's own pinned Python 3.12.3 runtime. Two distinct,
real findings were fixed: the generator's
_locate_markers()marker-own-line false negative (
11d2a3db), and three stalechecker-count cross-references after Task 4 (
1b3ba78e).registered
.gitapex/ssot.jsongate this PR touches:.github/scripts/gitapex_scan_ssot_schema.py,.github/scripts/gitapex_run_skill_contract_check.py, andskills/drafting-a-skill/scripts/gitapex_generate_skill_contract.py's_locate_markers()-- every traced malformed-input path failsclosed; no blocking findings. Two non-blocking UX-polish notes
disclosed, not fixed.
skills/drafting-a-skill/scripts/test_gitapex_generate_skill_contract.py;14 new tests in
tests/test_gitapex_scan_ssot_schema.py;tests/test_gitapex_skill_contract_drift.pycovers the newwrapper's own timeout/failure/zero-skills/argv-shape paths.
Merge gate: independent review
This PR is also subject to the
independent-review-pendingrequiredstatus check (see
.github/workflows/independent-review-pending.yml/.github/scripts/gitapex_gate_independent_review_pending.py). It stayspending/failing until a
## Independent review verdictsection namingthis PR's current head commit is recorded in this body --
drafting-a-pr-to-merge's own Step 8 records it once its independentreview completes. There is nothing for you to do here now: do not
pre-fill this section yourself, and do not remove this note.
Independent review verdict
6b72119f87a6d20444b819dcccae652c7bd25c46Inner layer (
reviewing-an-artifact, fresh pass): the diff sincethe prior round (
b48ce3cf->6b72119f, this round's own priorverdict archived as a PR comment naming that outgoing commit) is a pure
test addition -- 169 insertions across two existing test files
(
skills/drafting-a-skill/scripts/test_gitapex_generate_skill_contract.py,tests/test_gitapex_skill_contract_drift.py), zero deletions, zeroproduction-code changes -- closing the Codecov patch-coverage gap the
prior round's own verdict already disclosed (both previously-flagged
files,
gitapex_generate_skill_contract.pyandgitapex_run_skill_contract_check.py, now at 100% line coverage).Classified
safeper this skill's own Step 1 (dominant signal is "anadded test", with no security-tier signal anywhere in the diff),
skipping the Step 2-5 persona fan-out per that step's own rule. Full
test suite (9274 tests) and local preflight (50/50 wired gates) re-run
clean after this round.
Findings carried forward from the prior round (both still unfixed, by
design -- see the archived verdict comment for full detail): the
spec.contract: {}definitional inconsistency betweendiscover_contractsandload_sidecar_contract(pending an ownercall), and the pre-existing YAML alias-expansion resource-exhaustion
exposure shared with the sibling scanner (not a regression).
Outer layer (GitHub-native reviewer): as before, this repository
has no "Claude Code Review" GitHub App installed. GitHub Copilot's
review was re-requested against the current head (
6b72119f),confirmed via a fresh PR read at 2026-09-14T01:24:26Z. Completed from
the inner layer alone per ADR 0004, same as every prior round.
Execution log
06b89d8;the task-list commit and the two commits after it were rewritten via
git commit-tree, tree content byte-identical, to remove aninadvertent
Claude-Session:commit-message trailer -- see Facts above)All 4 waves complete. Wave 1 (Tasks 1, 7, 8). Wave 2 (Tasks 2, 3,
6). Wave 3 (Tasks 4, 5). Wave 4 (Task 9). Branch execution finished;
handing off to
drafting-a-pr-to-merge's own independent-review step.Related Issue
Closes #1965
Refs #1964
Provenance note
Commit messages in this branch carry no session-attribution trailer,
per CONTRIBUTING.md's outward-artifact-preflight PR-body trailer
disclosure section (the ratified PR-body trailer exception applies only
to this PR body's own GitHub-appended "Generated by" line, issue #687;
it does not extend to commit messages).