From 9f6a7762746fbc48bf83f5c85bbc50cf519be91f Mon Sep 17 00:00:00 2001 From: cs-test-runner Date: Fri, 11 Sep 2026 17:05:19 +0000 Subject: [PATCH 1/2] docs(skills): document the real Vercel skill copy/review/PR workflow (#45) Adds the review workflow #44 left undocumented: discover a source read-only (demonstrated against a real repo, pstack, without selecting anything for the owner), copy an explicit selection with `sumctl skills install`, review the copy as untrusted third-party content, commit, and open an ordinary PR. Documents re-running the same explicit copy command to check for updates (idempotent when unchanged, a clean additive diff when not), backed by a real run against vercel-labs/agent-skills in a throwaway Git project. Also documents a real finding: the pinned CLI's own `update` subcommand relocates an `--agent`-scoped copy into a different agent directory instead of refreshing it in place, silently advancing skills-lock.json regardless -- so the workflow explicitly avoids it in favor of re-running the explicit copy. Docs only; no runtime behavior changes. --- docs/ACCEPTANCE.md | 4 +++ docs/features/skills.md | 1 + docs/sum-skills.md | 66 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 71 insertions(+) diff --git a/docs/ACCEPTANCE.md b/docs/ACCEPTANCE.md index c0501e0..187dd4e 100644 --- a/docs/ACCEPTANCE.md +++ b/docs/ACCEPTANCE.md @@ -90,6 +90,10 @@ Enroll `douglasjarquin/sum` itself and confirm the registration points at the in In a throwaway Git project and temporary `HOME`, run `sumctl skills install` against a local fixture with one explicitly selected skill and agent. Confirm the pinned Vercel Skills CLI copies the selected directory into the project, creates no skill symlink, and writes no user-level skill directory. Request a missing skill and confirm the command fails without reporting success, then request a `sum-*` name and confirm Sum refuses it before the upstream CLI runs. +List a real source's skills read-only (`.local/bin/skills add michael-denyer/pstack-claude --list`) and confirm nothing is copied and no selection is made; listing is inspection, not an install decision. +Copy one real skill from a real public source (`vercel-labs/agent-skills`, skill `writing-guidelines`, agent `claude-code`) into a throwaway Git project with `sumctl skills install`; confirm the resulting Git diff is exactly one project-local `SKILL.md` plus `skills-lock.json`, review it as untrusted content per `docs/sum-skills.md`, commit it, and open an ordinary PR for it. +Re-run the identical `sumctl skills install` command and confirm the diff is empty (an unchanged selection); then copy a second explicit skill into the same project and confirm the diff adds only that skill's files and a `skills-lock.json` entry, leaving the first skill's files untouched. +Run the pinned Vercel Skills CLI's own `update -p -y` directly (not through `sumctl`) against that project and confirm it does not preserve the `--agent claude-code` copy made above; do not rely on `update` for a refresh, and re-run the explicit `sumctl skills install` command instead. ## 12. Code graph per checkout with a real harness diff --git a/docs/features/skills.md b/docs/features/skills.md index 620f86e..0af4ace 100644 --- a/docs/features/skills.md +++ b/docs/features/skills.md @@ -9,3 +9,4 @@ Implementation and focused boundary coverage are in `lib/sumctl.py` and `tests/t | `skills.project-copy` | Installing explicit skill and agent names runs the pinned Vercel CLI from the Git project root with `--copy --yes`, leaving no project skill symlink or user-level skill write | automated: `tests/test_skills_cli.py`; manual: real pinned CLI fixture | CLI transcript | | `skills.explicit-selection` | Wildcards, option-looking values, non-project targets, and requested `sum-*` names are refused before the third-party CLI runs | automated: `tests/test_skills_cli.py` | offline suite | | `skills.sum-inventory` | Checking Sum itself still validates its owned names, projections, portable imports, and compatibility references | automated: `tests/test_skill_namespace.py`, `tests/test_setup.py` | offline suite | +| `skills.review-workflow` | A real project-local copy from `vercel-labs/agent-skills`, an unchanged re-review, and an additive second-skill review each produce the diff `docs/sum-skills.md` describes; the pinned CLI's own `update` relocates an `--agent`-scoped copy instead of refreshing it in place | manual: real pinned CLI against a throwaway Git project, documented in `docs/ACCEPTANCE.md` §11 | CLI transcript + Git diff | diff --git a/docs/sum-skills.md b/docs/sum-skills.md index 58edc1f..12996c6 100644 --- a/docs/sum-skills.md +++ b/docs/sum-skills.md @@ -23,3 +23,69 @@ It does not checksum or certify third-party skill content. Vercel Skills owns source parsing, discovery, copying, and its `skills-lock.json` format. The `--yes` mode can overwrite a same-named third-party destination, so inspect the target's Git diff after installation and review copied skills before use. Sum does not run a copied skill, update it automatically, select every discovered skill, or change model, MCP, permission, or global harness settings. + +## Review workflow + +Ordinary Git and PR review do the reviewing; the steps below are the same as reviewing any other +third-party dependency bump, applied to a skill copy. + +1. **List a source's skills, read-only, before choosing anything.** This does not go through + `sumctl`; run the pinned CLI directly: + + ```sh + .local/bin/skills add OWNER/REPOSITORY --list + ``` + + For example, `.local/bin/skills add michael-denyer/pstack-claude --list` lists that + repository's skills (`architect`, `tdd`, `why`, and dozens more) with their descriptions. + Listing selects nothing: which skill, if any, to adopt from a source stays the owner's + decision, made explicitly in the next step. + +2. **Copy the explicit selection** into a real Git project: + + ```sh + bin/sumctl skills install \ + --target /path/to/git-project \ + --source vercel-labs/agent-skills \ + --skill writing-guidelines \ + --agent claude-code + ``` + + A real run of this exact command against a throwaway project wrote exactly two project-local + regular files: `.claude/skills/writing-guidelines/SKILL.md` and an updated + `skills-lock.json`. Nothing was written outside the target project or to a global/user-level + location. + +3. **Inspect the Git diff before committing anything.** Treat every copied instruction, script, + resource, and stated permission as untrusted content — copying a skill is not authorization to + run it or change tool/account settings. `writing-guidelines` itself, for instance, instructs an + agent to "fetch the latest guidelines from the source URL below"; a reviewer decides whether + that outbound fetch is acceptable before the skill is ever invoked, the same as reviewing any + other third-party dependency change. + +4. **Commit and open an ordinary PR.** Human review and merge happen exactly like any other + change to the project; merging this PR is not an installation activation for Sum itself (that + is the separate release/update mechanism). + +5. **Re-run the same explicit command to propose an update, rather than the CLI's own `update`.** + A repeat of an unchanged selection reproduces byte-identical files, so `git status` reports + nothing to commit. A repeat that adds a new explicit skill produces an ordinary additive diff + scoped to that skill plus `skills-lock.json`: a real run adding a second skill to an + already-copied project left the first skill's files completely untouched and only added the + second skill's files and a new `skills-lock.json` entry. + +`bin/sumctl skills check --root` is the unrelated check in this area: it validates Sum's own +`sum-*` namespace and projections, never third-party content. Reviewing a copied third-party +skill is ordinary Git/PR review, not a Sum-run verification step. + +## Do not use the pinned CLI's own `update` + +Vercel Skills' `update`/`upgrade` subcommand is not part of this workflow, and a real run shows +why: against a project holding two skills explicitly copied with `--agent claude-code`, running +the pinned CLI's own `update -p -y` directly relocated both copies out of `.claude/skills/` into +a different `.agents/skills/` layout — dropping the originally requested agent target instead of +refreshing it in place — while `skills-lock.json`'s recorded hashes advanced regardless, with +nothing in the CLI's own output calling out the relocation. Re-run the explicit +`add`/`bin/sumctl skills install` command from step 2 above to check for or apply an update +instead; it is idempotent for an unchanged selection and reproduces the correct project-local +layout for a changed one. From 175635736639ae4dbc26963dcee57b314972a702 Mon Sep 17 00:00:00 2001 From: Douglas Jarquin <8209+douglasjarquin@users.noreply.github.com> Date: Fri, 11 Sep 2026 13:58:54 -0400 Subject: [PATCH 2/2] fix(verify): stop treating vercel-labs/agent-skills as a local path The map auditor flags backticked tokens that look like repository paths. The new skills.review-workflow row wrapped a GitHub slug in backticks and failed CI. Write the slug as plain text and add the scenario to the frozen manual inventory. --- docs/features/skills.md | 2 +- tests/test_verify.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/features/skills.md b/docs/features/skills.md index 0af4ace..43acb27 100644 --- a/docs/features/skills.md +++ b/docs/features/skills.md @@ -9,4 +9,4 @@ Implementation and focused boundary coverage are in `lib/sumctl.py` and `tests/t | `skills.project-copy` | Installing explicit skill and agent names runs the pinned Vercel CLI from the Git project root with `--copy --yes`, leaving no project skill symlink or user-level skill write | automated: `tests/test_skills_cli.py`; manual: real pinned CLI fixture | CLI transcript | | `skills.explicit-selection` | Wildcards, option-looking values, non-project targets, and requested `sum-*` names are refused before the third-party CLI runs | automated: `tests/test_skills_cli.py` | offline suite | | `skills.sum-inventory` | Checking Sum itself still validates its owned names, projections, portable imports, and compatibility references | automated: `tests/test_skill_namespace.py`, `tests/test_setup.py` | offline suite | -| `skills.review-workflow` | A real project-local copy from `vercel-labs/agent-skills`, an unchanged re-review, and an additive second-skill review each produce the diff `docs/sum-skills.md` describes; the pinned CLI's own `update` relocates an `--agent`-scoped copy instead of refreshing it in place | manual: real pinned CLI against a throwaway Git project, documented in `docs/ACCEPTANCE.md` §11 | CLI transcript + Git diff | +| `skills.review-workflow` | A real project-local copy from vercel-labs/agent-skills, an unchanged re-review, and an additive second-skill review each produce the diff `docs/sum-skills.md` describes; the pinned CLI's own `update` relocates an `--agent`-scoped copy instead of refreshing it in place | manual: real pinned CLI against a throwaway Git project, documented in `docs/ACCEPTANCE.md` §11 | CLI transcript + Git diff | diff --git a/tests/test_verify.py b/tests/test_verify.py index 589eb6b..b0c492c 100644 --- a/tests/test_verify.py +++ b/tests/test_verify.py @@ -359,7 +359,7 @@ def test_sum_repository_contract_checks_against_its_own_tasks(self): self.assertEqual(len(ids), len(set(ids))) drivers = {s["id"]: s["driver"] for s in record["scenarios"]} self.assertEqual(drivers["verify.skipped-scenario"], "automated") # Its description starts with "Manual"; only the Driver column decides. - self.assertEqual([i for i, d in drivers.items() if d == "manual"], ["live.herdr-smoke", "live.harness-canary", "verify.sum-self", "ci.hosted-verification", "root.real-harness-canary", "evidence.publish-rendered", "evidence.sum-self", "graph.real-binary", "graph.harness-mcp"]) + self.assertEqual([i for i, d in drivers.items() if d == "manual"], ["live.herdr-smoke", "live.harness-canary", "verify.sum-self", "ci.hosted-verification", "root.real-harness-canary", "evidence.publish-rendered", "evidence.sum-self", "graph.real-binary", "graph.harness-mcp", "skills.review-workflow"]) self.assertEqual(record["contract"]["entrypoint"], "mise run verify") self.assertTrue(record["artifacts"]["git_ignored"])