feat(evaluating-skill-quality): migrate manifest checks onto JSON Schema (#758) - #1448
Merged
Conversation
…tion (re-derived) Issue #758, reframed 2026-08-29 to own delivery through implementation, not design alone. Supersedes the 2026-08-06 design from closed PR #795 (unmerged, orphaned on branch claude/gitapex-pr-758-v8ixpy). Re-derives the check-name audit against current main (~1,200 commits since PR #795): the manifest-related check-name set grew from 15 to 18 (dependency-policy-declared, external-citations-well-formed MIGRATE; external-citations-resolve RETAIN, added by issues #804/#1055). Confirms issue #804 already resolved the vendoring/portability conflict PR #795's review surfaced (spec.dependencyPolicy + spec.executionRequirements.packages, independent of spec.portability). Live-verified against the real schema and gitapex_check_skill_shape.py source.
Issue #758. Task decomposition per executing-a-branch-plan: file-ownership map, interface-dependency edges, wave assignment (executed sequentially -- Workflow tool not opted into this session), and the authorization record for the branch-plan execution gate.
Issue #758. Merged origin/main (issue #218's 3 gate proposals landed, plus a new portable-no-demonstrative-origin-repository-citation check -- non-manifest, out of scope for this migration). Re-verified live: the manifest-related 18-check set (12 MIGRATE / 6 RETAIN) is unaffected by the merge; only the non-manifest completeness list needed a count fix.
|
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: Pro Plus 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1448 +/- ##
==========================================
+ Coverage 99.63% 99.66% +0.03%
==========================================
Files 148 149 +1
Lines 25561 24555 -1006
Branches 3151 2973 -178
==========================================
- Hits 25467 24473 -994
+ Misses 94 82 -12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…on own sidecar Issue #758, task 2. Declares spec.dependencyPolicy: Declared and spec.executionRequirements.packages.pip: [pyyaml, jsonschema] on this skill's own metadata/gitapex.yaml; spec.portability stays Portable, unchanged (issue #804's resolution). Updates SKILL.md's usage line to disclose the new runtime requirement, net-zero body-line growth (still inside the disclosed 500-line ceiling). Verified live: 68/68 gitapex_check_skill_shape.py checks pass; schema validation clean (0 jsonschema errors against the edited sidecar). This step does not need to wait for task 1 (the code migration itself) -- gitapex_check_skill_shape.py's execution-requirements-well-formed and dependency-policy-declared checks already recognize these fields today.
…ema (issue #758) Replace gitapex_check_skill_shape.py's ~1,150-line hand-rolled YAML-like reader with yaml.safe_load + jsonschema.Draft202012Validator reading skill-metadata.schema.json directly. All 12 MIGRATE-classified checks now delegate their shape validation to the schema; the 6 RETAIN cross-file checks (skill-dependencies-resolve, lifecycle-deprecated-replacement-resolves, metadata-name-matches-dir, external-citations-resolve, plus the two directly-compared trivial enum checks) keep direct field access against the parsed dict. Fixed a real bug found via the migration: the experimental-stable-compatible check's schema-side not/required contradiction rule was vacuously firing when spec.lifecycle itself was the wrong type (a string, not a mapping), now guarded to only evaluate when lifecycle is actually a mapping. Ported test_gitapex_check_skill_shape.py's manifest-related tests: evidence-string assertions updated to match jsonschema's own wording, several fixtures whose evidence now reflects real YAML's own multi-document/tab-indentation/dedent syntax-error semantics rather than the hand-rolled parser's line-by-line tolerance (unreadable via manifest_unusable, disclosed, not a regression), two known-gap characterization tests flipped now that the schema's own uniqueItems and stricter type checks close them, and every _parse_manifest-internals test (docstring drift guards, differential-vs-PyYAML property tests, a hand-rolled-regex drift guard) retired with disclosure -- their own hand-rolled second copy no longer exists to drift from the schema. Ported tests/test_gitapex_skill_metadata_sidecar.py and tests/test_gitapex_check_skill_shape_properties.py the same way, including fixing test_a_path_with_no_skill_md_or_metadata_or_references_component_is_left_unchanged's own hypothesis strategy: it could generate the literal reserved words 'metadata'/'references' as a path component despite the test's own docstring intending to exclude that shape (a pre-existing, unrelated gap in the strategy's own filtering, not introduced by this migration). Removed pyproject.toml's Tier C mypy override for this module now that the hand-rolled parser code it existed for is gone; fixed the small number of new type-narrowing issues the migration's own code introduced (explicit isinstance-narrowed locals, a TypeGuard-typed helper) instead of re-suppressing them. Full suite: 6672 passed. mypy (project settings, no override): clean. ruff check/format: clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGvuA1gLCSQn98ryRWUszw
Resolves conflicts from issue #1330's shape_checks/ package split (which landed independently on main while this branch's own issue #758 migration was in progress): re-applies the JSON-Schema migration (previously written against the old gitapex_check_skill_shape.py monolith) onto the new shape_checks/manifest.py, lifecycle.py, execution_requirements.py, skill_dependencies.py, constants.py, and orchestrator.py modules instead. Adds a new shape_checks/schema.py module (schema loading, _errors_under, _join_schema_errors) as the shared home for the schema-validation infrastructure every migrated check now uses. pyproject.toml's Tier C mypy overrides for gitapex_check_skill_shape/shape_checks.skill_dependencies are retired in this same merge -- the hand-rolled parser code and the un-narrowed _valid_skill_dependency_list helper they existed for are both gone. Full repo suite verified green post-merge (7755 passed, excluding two pre-existing environment-broken hook tests unrelated to this migration); mypy and ruff clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGvuA1gLCSQn98ryRWUszw
… checks below the complexity floor _lifecycle_checks (rank C, 17) and _skill_dependency_checks (rank D, 21) exceeded the repo's cyclomatic-complexity-floor gate (xenon --max-modules B) after the issue #758 schema migration. Extracted each check's own per-CheckResult computation into a small, independently testable helper function (mirroring the pattern shape_checks/orchestrator.py's own per-section helpers already establish) -- pure structural decomposition, zero detection-logic change. Both modules now rank B or better; the repo-wide xenon gate (--exclude gitapex_check_skill_shape.py, shape_checks/execution_requirements.py, matching CI's own real invocation) passes clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGvuA1gLCSQn98ryRWUszw
…ge gap shape_checks/schema.py's own two defensive TypeError branches (_schema_dict/_schema_enum guarding against a malformed schema file) and _schema_enum's unused bare-const fallback were the 3 lines Codecov's patch-coverage check (97.95% vs the repo's 99.63% target) flagged on PR #1448's head commit. The const fallback is deleted outright -- every _schema_enum call site targets a real schema "enum" node, none a bare "const" one, so it was genuinely dead code, not a defensive guard. The two remaining TypeError branches stay (real, though unreachable via the current, CI-drift-gated schema file, fail-loud guards) and are now directly unit-tested as the pure functions they are. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGvuA1gLCSQn98ryRWUszw
tvna
marked this pull request as ready for review
August 30, 2026 17:20
tvna
marked this pull request as draft
August 30, 2026 17:21
… schema migration Independent adversarial review of PR #1448 (issue #758) found a real behavior regression: _skill_dependency_checks split schema errors on "allOf" in absolute_schema_path with no isinstance guard, so a malformed (non-mapping) spec.skillDependencies combined with portability: Portable misattributed the field's own wrong-type error to requires-portability-compatible as a false contradiction FAIL. Guard it the same way lifecycle.py's analogous experimental-stable-compatible check already does, and add a regression test covering the previously untested portability: Portable + malformed-field combination. battle-testing-a-skill's adversarial pass separately found that yaml.safe_load's except clause did not cover RecursionError/MemoryError, so a deeply-nested or hostile metadata/gitapex.yaml sidecar (e.g. a vendored skill) could crash check_shape() with a raw traceback instead of the graceful manifest-parsable FAIL every other malformed-sidecar case gets. Broaden the exception tuple and add a regression test. Also applies one style-only refactor-pass finding: flatten _external_citations_well_formed_result to early returns, matching its two sibling schema-backed result builders in the same file. Refs #758
…-pinning criterion battle-testing-a-skill's dogfooding pass on this migration found that rubric.md's Declared-dependency-policy criterion (c) -- the PEP 723 self-contained-script pattern -- is not implemented anywhere in this repository, including in the just-migrated gitapex_check_skill_shape.py, despite this skill's own sidecar now declaring dependencyPolicy: Declared for the first time repository-wide. Adopting that pattern unilaterally in this migration would be scope creep beyond issue #758's own task, and pyyaml/jsonschema are already pinned to exact resolved versions via this repository's own uv.lock (verified: jsonschema 4.26.0, pyyaml 6.0.3) -- the real reproducibility mechanism every uv run/uv sync invocation here already uses. Record the gap as a disclosed caveat rather than leaving it silently unmet. Refs #758
…w pip deps This skill's own self-review dogfood pass (rubric.md's Dependency policy section, Declared-policy sub-criterion (b)) found that declaring dependencyPolicy: Declared with executionRequirements.packages.pip: [pyyaml, jsonschema] without also naming those packages in SKILL.md's own compatibility field leaves the declaration incomplete by the skill's own rubric -- confirmed live by gitapex_scan_execution_requirements_drift.py's packages-pip-vs-compatibility heuristic, which flagged both packages before this fix and is clean after. Add the compatibility field, following the same plain-English pattern the rubric's own worked example already documents. Reflow one wrapped paragraph in the Lifecycle section by one line to hold the body under its 500-line cap (pure rewrap, no content change). Refs #758
…ts in the sidecar audit trail Replace the single ad hoc caveat entry with two audit entries (one per dogfooding pass performed for this migration -- battle-testing-a-skill and this skill's own self-review) and one deferral entry for the sub-criterion (c) PEP 723 gap, matching this sidecar's own established kind vocabulary and phrasing convention (see the issues/537 and issues/619 entries above for the same "not blocking, deferred:" shape). The self-review pass also disclosed a real isolation-contamination caveat: its dispatch context carried this repository's own CLAUDE.md via harness injection despite the exclusion instruction, so its findings are provisional per adversarial-self-audit.md -- recorded here rather than silently dropped. Refs #758
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
Migrates
gitapex_check_skill_shape.py's 12 schema-expressible manifestchecks onto the JSON Schema (issue #758, reframed 2026-08-29 to own
delivery through implementation).
Facts
docs/superpowers/specs/2026-08-29-skill-metadata-checker-schema-consolidation-design.md,superseding the closed, unmerged PR docs(skill-metadata): manifest-parser vs JSON Schema consolidation design #795. Re-derives the manifest
check-name audit against current
main(18 checks total, up from 15;12 MIGRATE / 6 RETAIN, live-verified).
docs/superpowers/plans/2026-08-29-claude-gitapex-issue-758-impl-031819.md.origin/mainthree times while this PR was open, mostrecently a 24-commit drift (issue feat(gates): fail the local preflight when the branch is behind its base #985's own gate caught it); the
manifest check-name audit and the full verification suite were
re-confirmed clean after each merge.
origin/mainindependently splitgitapex_check_skill_shape.pyinto a
shape_checks/package (issue refactor(evaluating-skill-quality): split gitapex_check_skill_shape.py, already excluded from the complexity gate at 6149 lines #1330's own PR) while thismigration was in flight -- the hand-rolled parser was relocated, not
migrated, by that split. The migration below was redone against the
new package layout, preserving every design decision from the original
monolith-based work.
Assumptions
origin/maindrift materially changes the12/6 check classification before this PR merges.
Risk / blast radius
High: touches
skills/evaluating-skill-quality/scripts/gitapex_check_skill_shape.pyand its
shape_checks/package, a heavily-relied-on deterministic gateevery skill in this repository is validated against, plus its own
~575-test suite and
pyproject.toml's mypy configuration. Mitigated by:preserving the external check-name contract exactly (all 48
CheckResultnames present before and after, none renamed or dropped -- confirmed by
the independent adversarial review below), the mandatory self-review +
battle-testing-a-skilldogfooding pass this skill's own establishedconvention requires, and live verification against every real
skills/*/sidecar before this PR leaves draft.
Rollback
git revertthis PR's merge commit(s) restores the hand-rolled parserand the pre-migration dependency profile in one step.
Verification
dependencyPolicy/executionRequirements.packagesdeclared on this checker's own sidecar;portabilityunchangeddependencyPolicy: Declared,executionRequirements.packages.pip: [pyyaml, jsonschema],portability: Portableunchanged, 70/70 shape checks passCheckResult("<name>", ...)string on both sides of the migration: all 48 present on both, none renamed or droppeduv run pytestfull suite green; live run against every realskills/*/;uv run mypy/ruff checkclean; self-review +battle-testing-a-skillpasstest_installs_the_prek_hook_for_a_real_checkout, confirmed flaky by passing in isolation, unrelated to this diff -- session-start-hook installer, untouched by this PR);mypy/ruff checkclean; dogfooding pass found and fixed 1 correctness regression and 1 robustness gap -- see## Skill audit evidencebelowChecklist
skills/*/SKILL.mdand deterministic checkerscripts -- see
## Skill audit evidencebelowevals/*/split.mdKept-edit-log entry addedskills/*/SKILL.mdStop-boundary bullet or dispatchbranch added
Skill audit evidence
This PR modifies
skills/evaluating-skill-quality/SKILL.md(one line),its
metadata/gitapex.yamlsidecar, its bundled deterministic checkerscripts (
scripts/gitapex_check_skill_shape.py,scripts/test_gitapex_check_skill_shape.py,and the
scripts/shape_checks/package), and one design doc(
docs/superpowers/specs/2026-08-29-skill-metadata-checker-schema-consolidation-design.md).Six dispatches/disclosures apply. Every confirmed finding that survived
triage was fixed in this same PR (commits
bdf33bd8,704b1adb,41e94ae7), not merely disclosed and deferred, with two exceptions namedexplicitly below.
## Factssection above; no dedicated adversarial-review round was run against its text in this PR)battle-testing-a-skill detail. Adversarial pass against
skills/evaluating-skill-quality/SKILL.mdand its bundled checker.18/20 dimensions PASS (2 N/A). Two FAILs, both squarely inside this
migration's own diff:
yaml.safe_load'sexcepttuple did not coverRecursionError/MemoryError, so a hostile or malformedmetadata/gitapex.yamlsidecar (e.g. a vendored skill) could crashcheck_shape()with a raw traceback instead of the gracefulmanifest-parsableFAIL every other malformed-sidecar case gets.Fixed: broadened the exception tuple; added regression test
test_deeply_nested_sidecar_fails_manifest_parsable_not_a_crash.runtime dependencies (
pyyaml,jsonschema) were not named inSKILL.md's owncompatibilityfield, and rubric.md's own Declareddependency-policy criterion (c), the PEP 723 self-contained-script
pattern, is not implemented anywhere in this repository. Partially
fixed, partially disclosed: added the
compatibilityfield (closesthe drift scanner's
packages-pip-vs-compatibilityheuristic finding);criterion (c) itself stays a disclosed, deferred gap in the sidecar's
own audit trail -- adopting PEP 723 unilaterally in this migration
would be scope creep beyond issue feat(skill-metadata): design and implement manifest-parser vs JSON Schema dual-source-of-truth resolution #758's own task, and
pyyaml/jsonschemaare already pinned to exact resolved versions via thisrepository's own
uv.lock(verified:jsonschema==4.26.0,pyyaml==6.0.3).evaluating-skill-quality detail. Self-review against the same
target, applying the nine-dimension rubric. Isolation caveat, disclosed
per this skill's own
adversarial-self-audit.md: the dispatch's contextcarried this repository's
CLAUDE.mdvia harness injection despite theexclusion instruction, so this verdict is provisional, not from a
verified-isolated mechanism. Shape checker: 70/70 (established fact, not
re-derived by the dispatch). No wrong-mechanism or low-cohesion finding.
The one real, actionable gap: Dependency policy sub-criterion (b) (every
declared package also named in
compatibility) failed, which underdimension 7's own gating rule blocked Mature. Fixed: added the
compatibilityfield (same fix as above); sub-criterion (c) remains thesame disclosed, deferred gap named above.
checker-script-adversarial-review detail. Independent adversarial
correctness review against every changed file in
scripts/shape_checks/and
gitapex_check_skill_shape.py, run as a fresh dispatch separate fromthe refactor pass below. Found one CONFIRMED, reproducible regression:
_skill_dependency_checkssplit schema errors on"allOf" in absolute_schema_pathwith noisinstanceguard (unlike the analogous,correctly-guarded case in
lifecycle.py), so a malformed (non-mapping)spec.skillDependenciescombined withportability: Portablemisattributed the field's own wrong-type error to
requires-portability-compatibleas a false contradiction FAIL --reproduced live against the real
check_shape()pipeline, confirmedabsent from the pre-migration code, and confirmed uncovered by the
then-575-test ported suite. Fixed: added the same
isinstance(deps, dict)guardlifecycle.pyalready carries, plusregression test
test_skill_dependencies_wrong_type_with_portable_does_not_spuriously_fail_contradiction_check.Also independently re-confirmed the YAML alias-expansion DoS surface
battle-testing-a-skillfound (same root cause, same fix). Six otherreview areas (behavior-drift/check-inventory, the vacuous-
not-guardpattern, YAML-safety, schema-error-attribution correctness, the removed
mypy overrides, and every weakened test assertion) were checked with no
further issues found.
A separate, independent refactor/simplify pass over the same diff (no
correctness claims, behavior-preserving only) found one style-only
inconsistency:
_external_citations_well_formed_resultinorchestrator.pybuilt its result through a nested if/else instead ofthe early-return convention its two sibling result-builders in the same
file already use. Applied: flattened to early returns, same behavior.
defeat-test-disclosure detail. Both new regression tests above (the
skillDependencies/Portablefalse-FAIL case and thedeeply-nested-sidecar
RecursionErrorcase) were constructedspecifically to defeat the two confirmed defects, not to exercise a
happy path -- each reproduces the exact failure mode before its
corresponding fix, per this repository's own defeat-test-disclosure
convention (issue #998).
Full sidecar audit trail:
skills/evaluating-skill-quality/metadata/gitapex.yaml'sspec.references, two newauditentries (one per dogfooding passabove) plus one
deferralentry for the sub-criterion (c) gap, allanchored to this issue.
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). See## Independent review verdictbelow.Independent review verdict
Two independent fresh dispatches were run over the full accumulated
diff, per
drafting-a-pr-to-merge's own Step 8:finding (see
## Skill audit evidenceabove), applied.requires-portability-compatiblefalse-FAIL described above), fixedand regression-tested; one disclosed, mitigated robustness gap (the
YAML
RecursionError/MemoryErrorhandling, also described above).Both findings' fixes are included in this PR's own diff (commit
bdf33bd8), re-verified against the full suite (7897/7898 passing, theone failure confirmed pre-existing/flaky and unrelated -- see the
Verification table above),
mypy, andruff check, all clean. Thisrecorded verdict is disclosure for a human reader, not a self-certifying
signal: it reflects that this session's own two independent dispatches
found the diff clean once their findings were fixed, not a guarantee no
further issue exists.
Execution log
PlanApproved-- 2026-08-29, plan mode exit; branchclaude/gitapex-issue-758-impl-031819created offorigin/main.TaskStarted-- design doc + task list committed and pushed.TaskCompleted-- migration implemented againstshape_checks/(redone once mid-flight after issue refactor(evaluating-skill-quality): split gitapex_check_skill_shape.py, already excluded from the complexity gate at 6149 lines #1330's own independent package
split), tests ported,
pyproject.toml's Tier C mypy overrides removed.TaskCompleted-- full verification suite green; live run againstevery real
skills/*/sidecar; complexity-gate decomposition(
lifecycle.py,skill_dependencies.py); Codecov patch-coverage gapclosed.
TaskCompleted-- dogfooding pass (battle-testing-a-skill +evaluating-skill-quality self-review) plus the two-layer independent
review; all confirmed findings fixed -- see
## Skill audit evidenceand
## Independent review verdictabove.Related Issue
Closes #758
Generated by Claude Code