Skip to content

Ship Codex skills through the scaffold lifecycle#1197

Open
mohidmakhdoomi wants to merge 21 commits into
cluesmith:mainfrom
mohidmakhdoomi:builder/pir-1196
Open

Ship Codex skills through the scaffold lifecycle#1197
mohidmakhdoomi wants to merge 21 commits into
cluesmith:mainfrom
mohidmakhdoomi:builder/pir-1196

Conversation

@mohidmakhdoomi

@mohidmakhdoomi mohidmakhdoomi commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

PIR Review: Ship Codex Skills Through the Scaffold Lifecycle

Fixes #1196

Summary

Codev now ships a Codex-native skill tree alongside its Claude skill tree and
materializes both during init, adopt, and update. Existing skill directories
remain user-owned, while recursive parity tests prevent the two provider trees
from silently drifting.

Files Changed

GitHub's canonical PR scope includes the large root .codex/skills/** addition
from the contributor commit that preceded PIR initialization; it is part of
the upstream PR because that commit is not on cluesmith/codev:main.

  • .codex/skills/** (+6,558 / -0) — self-hosted Codex mirror.
  • codev-skeleton/.codex/skills/** (+611 / -0) — seven shipped Codex skills.
  • .claude/skills/codev/SKILL.md and
    codev-skeleton/.claude/skills/codev/SKILL.md (+4 / -4) — lifecycle and
    preservation documentation mirrored into the new Codex trees above.
  • packages/codev/src/lib/scaffold.ts (+35 / -28) — provider-qualified skill
    copy results and per-skill preservation.
  • packages/codev/src/commands/{init,adopt,update}.ts (+17 / -19) — install,
    backfill, and report both provider paths.
  • packages/codev/src/__tests__/{skill-parity,scaffold,init,adopt,update}.test.ts
    (+178 / -20) — fresh install, preservation, backfill, structured output, and
    recursive parity coverage.
  • AGENTS.md and CLAUDE.md (+8 / -2) — self-hosted directory map.
  • codev/plans/1196-ship-codex-skills-in-the-skele.md,
    codev/state/pir-1196_thread.md, and porch state — protocol artifacts.
  • codev/resources/arch.md and codev/resources/lessons-learned.md — current
    materialization architecture and durable parity/preservation guidance.
  • codev/reviews/1196-ship-codex-skills-in-the-skele.md (+109 / -0) — this
    retrospective and PR body.

Per dev-gate feedback, codev-skeleton/templates/AGENTS.md and
codev-skeleton/templates/CLAUDE.md are unchanged from upstream.

Commits

Porch-generated phase and gate commits are also retained in branch history.

Test Results

  • Codev dependency/core/types/artifact/package build chain: ✓ pass
  • Targeted lifecycle and parity tests: ✓ 58 tests
  • Built-CLI integration suite: ✓ 87 tests, including CI-active fresh-init
    assertions for matching packaged Claude and Codex skills
  • Full default unit suite: ✓ 3,525 executed tests; 48 existing skips
  • Porch implement checks: ✓ build and tests
  • Built-CLI smoke test: ✓ fresh init installs both trees; adopt and update
    preserve customized Codex bytes while restoring a missing Codex skill
  • Human dev approval: ✓ approved after reviewing the worktree

The full suite was run with isolated HOME and TMPDIR because the host's
global Codex configuration and /tmp/.git otherwise alter unrelated harness
and workspace-root tests.

Architecture Updates

Updated the COLD codev/resources/arch.md Installation Architecture section.
It now records that provider-native skills are the deliberate exception to
runtime-only framework resolution, describes independent per-provider
materialization/preservation, and names the recursive parity guard. No HOT
change was needed because the existing hot facts already establish the
runtime-resolution and dual-tree rules.

Lessons Learned Updates

Added a COLD documentation lesson to
codev/resources/lessons-learned.md: when provider discovery requires physical
duplicates, compare inventories, paths, and bytes in CI through one explicit
exception allowlist, and preserve customizations at the complete-skill
directory boundary for each provider independently. This is useful reference
guidance but does not displace a HOT lesson.

Things to Look At During PR Review

  • copySkills() now returns provider-qualified relative paths. Verify init,
    adopt, and update all consume those paths consistently.
  • The init/adopt module headers now distinguish materialized integration and
    governance files from framework files that continue to resolve at runtime.
  • skipExisting protects the complete skill directory independently in each
    provider tree; it intentionally does not merge individual files.
  • PROVIDER_SPECIFIC_SKILL_EXCEPTIONS starts empty and exempts a complete
    top-level skill only when a future provider-specific implementation is
    reviewed.
  • The self-hosted root has ten skills, while the shipped skeleton has seven.
    Parity is enforced between providers within each context, not between the
    self-hosted and shipped inventories.
  • The Codex consultation reported a 27-file scope because its local-main
    baseline already contained contributor commit 25e1a000; GitHub's actual
    upstream PR scope includes that root Codex mirror.

How to Test Locally

  • View diff: VSCode sidebar → right-click builder pir-1196Review
    Diff
    .
  • Build:
    pnpm --filter @cluesmith/codev-types build && pnpm --filter @cluesmith/codev-core build && pnpm --filter @cluesmith/codev-artifact-canvas build && pnpm --filter @cluesmith/codev build
  • Targeted tests:
    pnpm --filter @cluesmith/codev exec vitest run src/__tests__/skill-parity.test.ts src/__tests__/scaffold.test.ts src/__tests__/adopt.test.ts src/__tests__/update.test.ts
  • Built-CLI integration suite:
    pnpm --filter @cluesmith/codev test:e2e:cli
  • Verify preservation: customize
    .codex/skills/arch-init/SKILL.md in a temporary adopted project, remove a
    different Codex skill, run the built codev update --agent, and confirm the
    customized bytes remain while the missing skill is restored and reported in
    newFiles.

Fixes #1196

@mohidmakhdoomi

Copy link
Copy Markdown
Contributor Author

Architect Integration Review — REQUEST CHANGES

High-risk review performed against upstream cluesmith/codev:main...mohidmakhdoomi:builder/pir-1196 (currently 60 files, +7,773/-78).

Verified

  • The scope rebuttal is substantively correct: contributor commit 25e1a000 is absent from upstream main, so the root .codex/skills/** mirror is legitimately part of this PR.
  • Commit 6c1008fe correctly fixes the stale init.ts / adopt.ts module headers.
  • Provider parity and preservation logic are sound; the targeted lifecycle/parity suite passes locally (58/58).
  • GitHub CI is fully green across unit, Tower, CLI, package-install, Ubuntu, and macOS jobs.
  • Architect CMAP: Gemini APPROVE; Codex APPROVE; Claude was unavailable due its current usage limit. My independent review found the issues below.

Required changes

  1. Fresh-init coverage is added to a test that CI explicitly excludes.

    • The new .claude/skills / .codex/skills assertions are in packages/codev/src/__tests__/init.test.ts.
    • packages/codev/vitest.config.ts excludes **/init.test.ts, so neither the reported targeted command nor the default unit job executes those assertions (the targeted run reports 4 files / 58 tests despite being passed 5 files).
    • The built-artifact suite packages/codev/src/__tests__/cli/init.e2e.test.ts runs in CI but has no skill-tree assertion.

    Add the fresh-init assertions to the built CLI integration test (ideally verify representative Claude and Codex SKILL.md files). This is the test that guards both init wiring and packaged-skeleton inclusion, and is required by issue Ship Codex skills in the skeleton and scaffold lifecycle #1196's automated fresh-install criterion.

  2. The changed user-facing codev skill still contradicts the four-tier/runtime model.
    In both root and skeleton provider copies, it says codev init creates protocols and that codev update updates/materializes protocols, roles, skills. Current init/adopt/update intentionally materialize provider skills and governance/root files while framework protocols/roles resolve from the package at runtime. Update the command summary and init/update descriptions accordingly, then mirror the bytes across Claude/Codex as required by the parity guard.

  3. Re-sync the review/PR scope wording.
    The rebuttal was valid when GitHub reported 59 files, but adding the rebuttal made the current PR 60 files. Avoid a volatile exact count or update the review and PR body to the current canonical scope.

After these are pushed, please rerun the parity/lifecycle tests plus the CLI integration suite and notify me for focused re-review. Do not merge; the pr gate remains human-only.


Architect integration review

@mohidmakhdoomi

Copy link
Copy Markdown
Contributor Author

Architect review correction

Withdraw required-change item 2 from my prior review. Do not modify any SKILL.md files in response to that review; preserve the current root/skeleton and Claude/Codex wording exactly.

Only the CI-active fresh-init coverage and current scope wording items remain active.


Architect integration review correction

@mohidmakhdoomi

Copy link
Copy Markdown
Contributor Author

Architect focused re-review — one wording fix remains

Verified commit bd6027e3:

  • The new built-CLI test correctly exercises packaged codev init, asserts both provider skill files exist, and asserts their bytes match.
  • No SKILL.md file was touched.
  • Reported targeted and CLI integration tests pass.
  • Volatile exact file counts were removed.

One small correction remains in both the review artifact and synced PR body. The new sentence is ungrammatical:

GitHub's canonical PR scope includes the large root .codex/skills/** addition came from the contributor commit...

Use, for example:

GitHub's canonical PR scope includes the large root .codex/skills/** addition from the contributor commit that preceded PIR initialization; it is part of the upstream PR because that commit is not on cluesmith/codev:main.

Please commit that review-only fix and re-sync the PR body. Do not touch SKILL.md; no test rerun is required for this prose-only correction. The human-only pr gate remains pending.


Architect focused integration re-review

@mohidmakhdoomi

Copy link
Copy Markdown
Contributor Author

Architect focused re-review — APPROVE

Verified commit f630257c and the synced PR body:

  • The scope sentence now uses the exact approved wording.
  • No SKILL.md file was touched.
  • The CI-active built-CLI fresh-init coverage from bd6027e3 remains correct.
  • The prior targeted lifecycle/parity suite passed 58/58 and the built-CLI suite passed 87/87.

No integration blockers remain. GitHub checks were retriggered by the prose-only commit and are currently running; they must remain green. The human-only pr gate remains pending, and the builder must not merge without that approval.


Architect integration review

@mohidmakhdoomi

Copy link
Copy Markdown
Contributor Author

PR is ready for review @waleedkadous @amrmelsayed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ship Codex skills in the skeleton and scaffold lifecycle

1 participant