forked from Neptune/mirror.kickstart.nvim
Release v1.29.0 — Modular Skill Architecture #55
Mike Bros
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "release/1.29.0"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Release v1.29.0 — Modular Skill Architecture
Changes
Architecture & Research (OP#2199)
Ingestion Adapter Layer (OP#2200)
Conductor Refactor (OP#2201)
Phase Migration (OP#2202)
Transition Events (OP#2203)
Documentation (OP#2204)
Workflow Goto (OP#2313)
Gravity Sync Improvements
PR Review Followup
Infrastructure
Checklist
References
Version: v1.29.0 — Modular Skill Architecture (Gravity PM)
Epics: OP#2199, OP#2200, OP#2201, OP#2202, OP#2203, OP#2204, OP#2313
All three validation suites pass after migrating phase skill frontmatter: validate-gravity-manifest.py: PASSED (0 errors, 76 warnings) - 79 SKILL.md files checked - All phase resolution OK - All domain dependency resolution OK - Warnings are non-blocking (domain/tool skills pending migration) test-conductor-routing.py: PASSED (31 tests, 0 failures) - Entry command matching OK for all phases - Smart entry routing OK - Phase sequence ordering contiguous 1-5 - Standalone skills correctly outside lifecycle - Transition event construction OK - Domain dependency resolution OK test-ingestion-skill.py: PASSED (103 tests, 0 failures) - Manifest loaded (schema v1) - All phase + domain dependency resolution OK - 26 subphases validated across 5 phases - 60 domain skills validated - Ingestion override detection OK Closes OP#2224 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>PR Review
Verdict: Acceptable
This review was conducted from a fresh context against the current branch state (23 files, 4198 lines changed). First review cycle.
Findings
Test Results
test-conductor-routing.py: 31/31 passedtest-ingestion-skill.py: 103/103 passedvalidate-gravity-manifest.py: 0 errors, 76 warnings (all pre-existing domain skill metadata)Smoke Tests
Human Review Items
2 items flagged for human review — see items marked with HUMAN REVIEW below.
Key Findings
parse_frontmatter()crashes on malformed input —validate-gravity-manifest.py:60validate-gravity-manifest.py:207phase_seq/atomicfields —gr-pr-review/SKILL.md,gr-pr-followup/SKILL.mdvalidate-gravity-manifest.py:145phase_seq/atomic—gr-release-finish/SKILL.md,gr-release-refresh/SKILL.mdscripts/validate-gravity-manifest.py
[IMPORTANT] Line 60:
parse_frontmatter()does not handle missing closing---delimitercontent.index("---", 3)will raise an unhandledValueErrorif a SKILL.md file starts with---but has no closing delimiter. The sibling scripttest-ingestion-skill.py:56-59correctly wraps this in a try/except block.Suggested fix:
[IMPORTANT] Lines 207-219: Orphaned skill detection produces false positives for structural skills
The exclusion list only skips
domain/prefixes andWriting. Structural skills that are not entry points —gr-gravity(the conductor itself),gt-ingest-claude(the default ingestion adapter) — are flagged as orphaned. So are allgt-*tool skills (gt-journal, gt-npm-release, gt-openproject, gt-testing, gt-wiki, gt-worktrunk, gt-writing-quality) that are invoked independently of the manifest.This produces 9 false-positive warnings that dilute the signal from actual orphans. Consider adding a
tool_skillsexclusion (e.g., skipgt-*prefixes since tool skills are not manifest-managed), or addgr-gravityandgt-ingest-claudeexplicitly since they are referenced by the manifest's runtime behavior even though they aren't listed as entries.[MINOR] Line 145: Misleading log message
ok(f"{rel}: frontmatter valid ({checked} fields checked)")— the variablecheckedcounts SKILL.md files validated so far, not fields. The message implies per-file field counting.Suggested fix: Change to
({checked} files checked)or remove the count from the per-file message and only report it in the summary.scripts/test-conductor-routing.py, scripts/test-ingestion-skill.py
[MINOR] Inconsistent file permissions
test-ingestion-skill.pyhas execute permissions (-rwxrwxr-x) whiletest-conductor-routing.pyandvalidate-gravity-manifest.pydo not (-rw-rw-r--). All three have shebangs. Either all should be executable or none should (and rely onpython3 script.pyinvocation).configs/claude/skills/gr-pr-review/SKILL.md, configs/claude/skills/gr-pr-followup/SKILL.md
[IMPORTANT] Standalone skills declare
category: phasebut are missing required phase fieldsPer
references/skill-metadata-standard.md, phase skills requirephase_seq(lifecycle position) andatomic(re-entry behavior). Bothgr-pr-reviewandgr-pr-followupdeclarecategory: phasebut have neither field. They are listed understandalone:in the manifest, notphases:.Options:
category: standaloneclassification (extending the taxonomy from 3 to 4 types)category: phasebut document thatphase_seqandatomicare only required for lifecycle phases (not standalone)category: toolsince they operate outside the lifecycleconfigs/claude/skills/gr-release-finish/SKILL.md, configs/claude/skills/gr-release-refresh/SKILL.md
[MINOR] Subphase override skills also declare
category: phasewithoutphase_seq/atomicSame pattern as the standalone skills above. These are subphase
skill_pathoverrides referenced from the release phase, not top-level phases. Currently doesn't break anything since the validator doesn't enforce category-specific required fields, but creates inconsistency with the documented schema.HUMAN REVIEW Items
HUMAN REVIEW: The
gravity-phases.yamlmanifest covers lifecycle phases and standalone skills but does not include tool skills (gt-*). Skills likegt-ingest-claude,gt-journal,gt-testing,gt-worktrunk,gt-wikietc. are referenced by phases (viadomain_dependenciesoringestion) but are not discoverable through the manifest. If the long-term vision is for the manifest to be the single source of truth for all Gravity skills, this is a gap worth addressing — possibly via atools:section in the manifest. If tool skills are intentionally outside the manifest scope, documenting this boundary inphase-manifest-schema.mdwould clarify the design.HUMAN REVIEW: This is a large PR (4198 lines, 23 files) introducing foundational architecture — phase manifest, ingestion adapter, conductor refactor, frontmatter migration for all phase skills, transition event format, and 3 validation scripts. The changes are coherent and well-tested (134 passing tests). The breadth is appropriate for a version-scoped architectural initiative, but the volume means edge cases in the documentation changes may have been missed. A focused read of the new reference docs (
transition-events.md,ingestion-interface.md,phase-manifest-schema.md) by a human familiar with the Gravity vision would add confidence.PR Review Followup
Responding to review posted on 2026-03-16 (review comment).
All 6 findings have been resolved on this branch.
Triage Summary
Fixes Applied
[IMPORTANT]
parse_frontmatter()crashes on malformed input — fixed in7fc7ff2ValueErrorinvalidate-gravity-manifest.py:60, matching the pattern intest-ingestion-skill.py[IMPORTANT] Orphaned skill detection false positives — fixed in
7fc7ff2gt-*tool skills andgr-gravityconductor invalidate-gravity-manifest.py:207-219[IMPORTANT] Standalone skills missing required
phase_seq/atomic— fixed in7fc7ff2skill-metadata-standard.mdto clarify thatphase_seqandatomicare required only for lifecycle phases (inphases:section), not for standalone phases or subphase overrides[MINOR] Misleading log message — fixed in
7fc7ff2({checked} fields checked)to justfrontmatter validinvalidate-gravity-manifest.py:145[MINOR] Inconsistent file permissions — fixed in
7fc7ff2test-conductor-routing.pyandvalidate-gravity-manifest.pyexecutable (matchingtest-ingestion-skill.py)[MINOR] Subphase override skills missing
phase_seq/atomic— fixed in7fc7ff2Human Review Items Addressed
phase-manifest-schema.mdclarifying that tool skills (gt-*) and domain skills (domain/*) are intentionally outside the manifest scope — they are discoverable by directory convention, not manifest registration.Test Results
validate-gravity-manifest.py: 0 errors, 67 warnings (down from 76)test-conductor-routing.py: 31/31 passedtest-ingestion-skill.py: 103/103 passedThis PR should now be ready for merge.
@ai wrote in #55 (comment):
Agreed let's fix this now by adding a tools section in the manifest. The idea is that a manifest can be a portable, changeable workflow and it should be the source of truth.
@ai wrote in #55 (comment):
Upon review I don't see anything glaringly wrong here. I will need to test with the latest to know if this is going to work long term though.
PR Review Followup
Responding to human review feedback from @mike (comment).
Feedback Addressed
1. Add
tools:section to manifest — fixed inb141a80The manifest is now the single source of truth for all Gravity skills. Added a
tools:section togravity-phases.yamlregistering all 8gt-*tool skills:Supporting changes:
phase-manifest-schema.md: Added Tool Skill Definition section with field referencegr-gravity/SKILL.md: Updated manifest loading to parsetoolssectionvalidate-gravity-manifest.py: Verifies tool skill paths, includes tools in orphan detection (removed blanketgt-*exclusion)test-conductor-routing.py: Added Test 8 validating all tools resolve with required fields2. Large PR edge cases — acknowledged, no action needed.
Test Results
test-conductor-routing.py: 56/56 passed (was 31 — added 25 tool tests)test-ingestion-skill.py: 103/103 passedvalidate-gravity-manifest.py: 0 errors, 67 warnings (all pre-existing domain skill metadata)PR Review
Verdict: Acceptable
This review was conducted from a fresh context against the current branch state (29 files, ~5000 lines changed). Updated review — previous reviews posted on 2026-03-16. Focuses on findings not previously identified in the prior 4 comments (1 review + 2 followups + 1 human feedback). This is review cycle 2.
Findings
Test Results
test-conductor-routing.py: 72/72 passed (was 56 — added goto routing + execution simulation tests)test-ingestion-skill.py: 103/103 passedvalidate-gravity-manifest.py: 0 errors, 67 warnings (all pre-existing domain skill metadata — nocategoryfield)Smoke Tests
Human Review Items
1 item flagged for human review — see below.
Key Findings
create_float_window—init.lua:45-67_diffput()and_diffget()—merge.lua:141-242merge.lua:706-710kkey on conflict files —picker.lua:345lua/custom/gravity/init.lua
[MINOR] Lines 45-46, 67, 71:
create_float_windowuses deprecatednvim_buf_set_optionandnvim_win_set_optionAPIsThe rest of the codebase (merge.lua, picker.lua) consistently uses the modern
vim.bo[buf]andvim.wo[win]syntax. This function still uses the deprecated form:Suggested fix:
Also applies to the spinner animation in
M.test()(lines 442-444).lua/custom/gravity/merge.lua
[MINOR] Lines 141-242:
_diffput()and_diffget()share ~40 lines of near-identical logicBoth functions follow the same pattern: ensure cursor is in the right buffer, perform the diff operation, double-deferred hunk rescan with
vim.schedule, navigate to next hunk or handle 0-hunk state (disable folds), and update winbar. The only differences are:_diffput()temporarily makessource_bufmodifiablediffputvsdiffget)A shared helper like
_apply_diff_action(action_cmd, opts)would eliminate the duplication and make the double-defer pattern a single point of maintenance.[MINOR] Lines 706-710: Shell command constructed via
string.formatwithentry.nameinterpolationWhile
entry.namecomes from the local manifest (low risk), using a list form forjobstartis more robust against accidental shell metacharacters:lua/custom/gravity/picker.lua
[NITPICK] Line 345: The
kkey handler's else-branch message is misleading for conflict filesWhen pressing
kon aconflictfile, the handler falls through to:But for conflict files, Enter opens the merge view (it does not accept the repo version). A more accurate message:
This is consistent with the
akey handler at line 369 which already uses this phrasing:'Conflicts need merge view — press Enter'.Previously Identified
6 findings from the prior review cycle were all fixed in commit
7fc7ff2. The tools section was added inb141a80per human review feedback. No deferred items exist — all prior findings are resolved.HUMAN REVIEW Items
HUMAN REVIEW: The goto workflow feature (7 commits) introduces the
gotofield on phases/subphases in the manifest, validation in both test scripts, and a simulation function. Currently only postflight→release uses a real goto (transition type). Thedelegatetype andon_error/alwaysconditions are defined in schema and validated but have no runtime consumer yet — they exist as documented extension points. This is architecturally sound for v1.29.0, but the gap between what the schema allows and what the conductor actually handles should be noted. When delegate gotos are implemented, the conductor's goto handling will need to match the simulation logic intest-conductor-routing.py:114-169. Verify this aligns with the intended rollout plan.PR Review Followup
Responding to review posted on 2026-03-16 (review comment).
Review verdict was Acceptable — no immediate fixes required.
Triage Summary
Deferred to v1.30.0
The following items have been tracked for the next version:
nvim_buf_set_option/nvim_win_set_optionincreate_float_window—init.lua:45-67_diffput()and_diffget()—merge.lua:141-242merge.lua:706-710kkey on conflict files —picker.lua:345Human Review Items (Unchanged)
These items require human judgment and were not auto-triaged:
delegatetype andon_error/alwaysconditions in schema but has no runtime consumer yet. Verify this aligns with the intended rollout plan for v1.30.0 call/return.Test Results
This PR is ready for merge.
PR Review Followup (Amended)
Amending the previous followup (comment). All 4 findings originally deferred to v1.30.0 were pulled back into v1.29.0 and fixed on this branch.
Triage Summary (Revised)
Fixes Applied
[MINOR] Deprecated
nvim_buf_set_option/nvim_win_set_optionincreate_float_window— fixed inb483f99[MINOR] Code duplication between
_diffput()and_diffget()— fixed in39762f0[MINOR] Shell command string interpolation in headless agent launch — fixed in
39762f0[NITPICK] Misleading help text for
kkey on conflict files — fixed in39762f0Human Review Items (Unchanged)
delegatetype andon_error/alwaysconditions in schema but has no runtime consumer yet. Verify this aligns with the intended rollout plan for v1.30.0 call/return.Test Results
validate-gravity-manifest.py: 0 errors, 67 warnings (pre-existing)test-conductor-routing.py: 72/72 passedtest-ingestion-skill.py: 103/103 passedRelease Refreshed
Draft release and PR body updated to include OP#2358-2361 in the release notes.
This PR is ready for merge.