Skip to content

feat: expand portable skills into mstack - #1

Merged
3metaJun merged 10 commits into
mainfrom
feat/mstack
Sep 10, 2026
Merged

3metaJun merged 10 commits into
mainfrom
feat/mstack

Conversation

@3metaJun

Copy link
Copy Markdown
Owner

The repository previously exposed a smaller portable skill set with a local-only installer. This expands it into mstack: one 50-skill bundle for Codex, Claude Code, OpenCode, and pi, with optional agents, the adapted workflow guide, meta-mode tools, and the source-only Benny automation pack.

The installer now supports named local and SSH environments, per-Harness adapters, atomic staging, locking, backup and rollback. This also adds model-role execution, Harness smoke checks, context auditing and reconciliation, and pinned upstream synchronization with checkout provenance, path containment, stale-file handling, and transformed-baseline manifests.

Verification

  • npm test on Windows: 50 skills validated, 47/47 tests passed.
  • npm test on WSL Debian: 50 skills validated, 47/47 tests passed; bash -n tools/meta-mode/worktree-audit.sh passed.
  • npm run check-upstream -- --source G:\agents_temp\pstack-read\pstack --strict: 47 pstack skills and all configured artifacts matched pinned commit 7366ac1.
  • npm pack --dry-run --json: 199 files, no audit, key, or PEM files.
  • Google Free SSH install and --replace: skill and artifact installation, backup, local environment isolation, lock/stage cleanup, and remote cleanup verified.
  • Windows C: junction targeting G: --replace: installation and same-volume backup verified, then cleaned.
  • Claude Code 2.1.267 live smoke with claude-haiku-4-5-20251001: native Skill invocation of meta-mode succeeded.

Agent: GPT-6 via Codex

Comment thread skills/meta-mode/playbooks/babysit.md Outdated
Comment thread skills/meta-mode/playbooks/babysit.md Outdated
Comment thread tools/meta-mode/watch-pr/github.ts
Comment thread THIRD_PARTY_NOTICES.md
Comment thread scripts/install.mjs Outdated
Comment thread scripts/install.mjs Outdated
Comment thread scripts/install.mjs Outdated
Comment thread tools/meta-mode/worktree-audit.sh Outdated
Comment thread scripts/audit-context.mjs Outdated
Comment thread skills/meta-mode/playbooks/multi-phase-plan.md Outdated
Comment thread agents/comment-reviewer.md Outdated
@3metaJun

Copy link
Copy Markdown
Owner Author

Adversarial review — 4 parallel reviewers (security · correctness/test-gaps · provenance & license · docs/consistency)

Verdict: fix-before-merge. No security hole found — no command injection or path traversal in the installer/remote path, the secrets scan is clean, and most of the PR description's claims verified under attack. But this review found one watcher hang, two classes of broken shipped instructions, a license-text gap, and several installer data-safety gaps. Detailed findings are in the inline comments; summary below.

What checked out clean (verified, not assumed)

  • Path containment (sync path) is real. profilePath rejects absolute/../UNC/drive-relative paths, resolveInside adds resolve + realpath checks; adversarial input simulation didn't get through. The gap is installer-side (inline comment on install.mjs).
  • Hygiene. No secrets, tokens, PEM blocks, or personal paths anywhere in tracked files; no author-local path leakage; npm pack --dry-run reproduces the claimed exactly-199 files with no audit/key/PEM files.
  • Counts are honest. 50 skills = 47 pstack (2 renames documented) + 3 mattpocock; 23 principle skills; 23 playbooks; profiles/skills.json matches the tree exactly; all 151 relative markdown links resolve; pinned commit 7366ac1 is consistent across notices/upstreams/manifest.
  • Tests are substantive. npm test reproduced 47/47 locally (Windows). They use real subprocesses, injected failures, and exact-output assertions — the problem is coverage, not assertion strength (next section).

Top findings (details inline)

  1. [P1] umstack — the pstack→mstack rename corrupted "upstack" in 5 spots across babysit.md and bugbot-triage.md.
  2. [P1] meta-mode playbooks command tool paths that exist in no layout (scripts/watch-pr/…, scripts/orch/orch.ts, scripts/worktree-audit.sh, mstack/skills/meta-mode/scripts/check-plan.mjs) — actual location is tools/meta-mode/…. These are the "run it directly" steps of Babysit/Shipping/Orchestrate/Worktree-cleanup.
  3. [P1] watch-pr orderStack infinite-loops on a cyclic head/base stack (two open PRs with swapped branches) → hang + GBs of allocation in an unattended watcher.
  4. [P1] THIRD_PARTY_NOTICES.md asserts MIT but omits the upstream copyright + permission notices MIT requires (Lauren Tan 2026; Matt Pocock 2026) — one-commit fix.
  5. [P2] Installer: empty-string environment target silently installs to the user's global skills dir (confirmed via --dry-run).
  6. [P2] Installer: unguarded rollback loop masks the original error and can strand --replace originals in backups (remote-install.mjs does this correctly — mirror it).
  7. [P2] Installer copies symlinks verbatim into live harness dirs; nothing anywhere rejects them.
  8. [P2] Artifact path containment enforced only by validate.mjs (CI), not by the installer — a tampered profiles/artifacts.json can target above the harness root.
  9. [P2] worktree-audit.sh uses BSD-only stat -f / date -r; on Linux/WSL every audit silently buckets everything as safe (only bash -n runs in CI).
  10. [P2] audit-context.mjs crashes with raw ENOENT on a dangling symlink (reconcile-context handles the same input gracefully).
  11. [P2] git show origin/main:mstack/… in autopilot playbooks presumes an undocumented vendored-subtree layout.
  12. [P3] agents/comment-reviewer.md frontmatter name doesn't match filename/sibling.

Notable (no good inline anchor)

  • CI runs only npm test. The meta-mode bun suite (~45 cases, including the one that would catch finding 3) and typecheck never run in CI; check-upstream isn't wired in either. A source-less mode that hashes current files against upstream-manifest.json would make provenance guarded instead of asserted — currently it's advisory and requires the author's upstream checkout.
  • Case-insensitive target deduplication only on win32 — APFS is case-insensitive too (install-paths.mjs:10); two case-variant targets can make --replace back up and overwrite the just-installed copy, order-dependently.
  • Remote locks are unowned empty dirs (no pid/timestamp, no unlock command, cleanup failures swallowed); local stale locks likewise have no liveness check — a killed installer bricks the target until manual deletion. remote-install also concatenates stdout+stderr before parsing the stage path, so an sshd MOTD aborts legitimate installs.
  • Backups accumulate unboundedly inside the live skills root (no retention policy).
  • validate.mjs's private-marker scan walks node_modules → false leak failures after any local install; its canaries (C:\Users\<digits>) miss other username shapes.
  • MSTACK_ENVIRONMENTS_FILE resolves through two different resolvers (install.mjs rejects relative, environment-lib accepts cwd-relative) — same variable, two semantics.
  • store.ts: crash between inbox drain rename/mkdir bricks push with a misleading "not initialized" error and orphans .inbox-drain-*; lock takeover can double-take after a dead holder; sync-upstream --apply writes non-atomically, which trains users toward --force.
  • tools/meta-mode devDeps use latest ranges, undermining the (appropriately) committed bun.lock.
  • Trivia: duplicate .audit/ in .gitignore; 2.3 MB of guide JPGs dominate the npm tarball; MSTACK_PSTACK_SOURCE env var mixes the old product name into the new CLI surface.

PR-description claim audit

  • "atomic staging, locking, backup and rollback" — substantiated (O_EXCL locks, staged rename, conflict re-check under lock), modulo findings 6/8 and the lock ergonomics above.
  • "path containment" — true for sync/check-upstream; only CI-enforced for installer artifacts (finding 8).
  • "pinned upstream synchronization with checkout provenance, transformed-baseline manifests" — real: hash + clean-tree pinning, per-artifact sha256s; re-verified 49/51 manifest entries byte-identical to the tree (2 documented compareContent:false divergences). But unguarded over time (no CI enforcement).
  • Windows C:→G: junction --replace verification — not found in the repo's tests; the only junction test is same-root in context.test.mjs. If it was done manually, consider landing it as a test.
  • "npm pack: 199 files, no audit/key/PEM" — reproduced exactly.

The design underneath is sound — findings 1–12 are mostly mechanical (renames, path repointing, guarded rollback, a CI job, two license paragraphs). Happy to re-review the fixes.

@3metaJun
3metaJun merged commit d0bf5f0 into main Sep 10, 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