Skip to content

fix: restore portable skill contracts - #8

Merged
3metaJun merged 2 commits into
mainfrom
fix/portable-skill-integrity
Sep 11, 2026
Merged

3metaJun merged 2 commits into
mainfrom
fix/portable-skill-integrity

Conversation

@3metaJun

Copy link
Copy Markdown
Owner

Problem

The Windows model checker resolved its default path to a literal ~\\.config\\mstack\\models.json, setup documented only four roles although seven are configured, and several skills still depended on Cursor or legacy worker names. Canonical skill bodies also had no reviewed baseline, so a thinned file could pass inventory checks.

Changes

  • Resolve the default model file from the platform home directory and accept ~\\... paths on Windows.
  • Document all seven model roles in /setup-mstack.
  • Replace legacy Comment Sicko, Task subagent, and built-in create-skill references with portable delegation and skill-authoring guidance.
  • Record canonical skill source and reviewed target body digests; strict upstream checks now detect missing, changed, or truncated bodies while preserving adapted skills.
  • Add regression coverage and document the new upstream integrity behavior.

Validation

  • npm test (78 passed, 1 expected platform skip)
  • npm run check-package
  • node scripts/check-upstream.mjs --source G:\\agents_temp\\mstack-pstack-full-20260911\\pstack --strict
  • git diff --check

npm publishing is intentionally deferred until the remaining fixes are complete.


Agent: GPT-6 via Codex

@3metaJun

Copy link
Copy Markdown
Owner Author

Adversarial Review (4 parallel reviewers)

Reviewed dcf8bfa via four independent adversarial passes — code correctness & path handling, test quality & validation re-run, docs/claims consistency, and manifest/digest integrity. Each re-ran the claimed validations and independently recomputed digests.

Overall verdict: APPROVE-WITH-NITS. No blockers; the mechanism works and the PR is internally consistent. The P2 items below directly undercut the two headline claims and are worth a fast-follow.

Verified sound

  • Validation claims reproduce exactly: npm test → 79 tests, 78 pass / 1 skip (the skip is the pre-existing Unix-only worktree-audit test, unrelated to this PR); npm run check-package passes.
  • All 47 target digests in profiles/upstream-manifest.json match independent recomputation — including the six SKILL.md bodies edited in this very PR (re-pinned consistently).
  • Mutation testing confirms strict detection fires on changed, truncated, missing-entry, and stale-entry bodies; CRLF-normalized hashing means Windows autocrlf checkouts won't false-positive; strict provenance (clean checkout, HEAD == pinned commit) fails closed.
  • The default-path fix works (literal ~\... bug gone); expandHome handles ~\, mixed separators, UNC passthrough, and correctly leaves ~~/~user literal.
  • The seven model roles in /setup-mstack match DEFAULT_ROLES exactly; Comment Sicko / Task subagent / worker type / create-skill are purged from shipped content (remaining hits are the upstream rename map and test guards, by design).
  • CI exercises check-upstream --strict against the pinned upstream, so source-side digests are actually enforced, not aspirational.

P2 findings

  1. Strict mode fails open when canonicalSkills is absent — scripts/check-upstream.mjs:337-339. Deleting the manifest block (or corrupting it to a non-object) yields only a console.warn and exit 0 under --strict, while the README promises strict fails when canonical bodies are "missing, differs". Aggravated by scripts/sync-upstream.mjs:459, which only carries canonicalSkills forward from the previous manifest and never generates it: a fresh --apply without a readable predecessor produces a baseline-less manifest and the strict guarantee silently evaporates. Suggest artifactProblems += 1 in that branch when strict.

  2. The ~\ fix covers only 1 of 3 scripts sharing the same helper — scripts/run-role.mjs:27-29 and scripts/smoke-harnesses.mjs:19-21 still expand only ~/. On Windows, run-role --file ~\.config\mstack\models.json resolves to a nonexistent cwd-relative path, and readModelConfig silently returns defaults — the exact silent-misconfiguration class this PR fixes, one script over.

  3. Zero automated coverage of the new canonical check block — the ~35-line block in check-upstream.mjs is exercised only target-side via skill-integrity.test.mjs; every sync-upstream.test.mjs fixture omits canonicalSkills, so CI only ever runs the warn path of the new code. The ~\ branch of expandHome itself is also untested (only the default-path regression is).

  4. Unhandled ENOENT when a manifest entry's upstream source SKILL.md is absent — scripts/check-upstream.mjs:346-352. The target side is guarded with existsSync; the source side isn't. Fails closed, but aborts the whole remaining report with a raw stack trace instead of a per-skill diagnostic.

  5. README overclaims --apply maintenance, and reflect points at a nonexistent workflow — README.md:307-309 implies the documented workflow maintains canonicalSkills, but nothing generates or re-pins those digests; every reviewed adaptation requires an undocumented hand-edit of a 64-hex hash. skills/reflect/SKILL.md:57-58 now defers to a "repository's skill-authoring workflow" with draft/test/iterate and description-optimization loops that don't exist in this repo (meta-mode/playbooks/authoring-a-skill.md is a 4-step checklist that defers to the harness).

P3 nits

  • Leftover `Task` mentions the cleanup missed: skills/meta-mode/playbooks/opening-a-pr.md:5, skills/reflect/references/tooling-reviewer.md:36, judgment-reviewer.md:23, divergent-reviewer.md:24.
  • Tautological assertion: scripts/skill-integrity.test.mjs:91-95 (notEqual(digest, digest(content + "mutation")) is always true once the preceding equality passes).
  • Non-strict runs print only canonical skill bodies: N problem(s) with no per-skill detail (check-upstream.mjs:365-368).
  • README.md:269 — "Strict mode requires" stranded alone by a missed paragraph re-wrap.
  • "active Harness delegation interface" adds a fourth synonym alongside delegation API / delegation tool / documented delegation mechanism.
  • Coverage gap (pre-existing): the 3 mattpocock/skills-derived skills (codebase-design, diagnosing-bugs, writing-for-agents) have no digest protection — the blanket "detect missing/changed/truncated bodies" claim holds for 47/50 skills.
  • --file as the last argument silently falls back to the default path (scripts/model-config.mjs:11-14, pre-existing).

Recommendation

Mergeable as-is. I'd fast-follow items 1–3: make missing baselines a strict failure, extend the ~\ expansion to run-role.mjs/smoke-harnesses.mjs, and add a mutation test covering the canonical check block.


Review method: 4 parallel adversarial subagent reviews (correctness/security, tests, docs, manifest integrity), each with independent recomputation and mutation experiments; findings cross-validated before posting.

@3metaJun
3metaJun merged commit 4358601 into main Sep 11, 2026
9 checks passed
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.

1 participant