Skip to content

[US-277] feat: .claude-plugin marketplace + plugin manifests with dataset catalog guard - #386

Open
rucka wants to merge 27 commits into
mainfrom
feature/US-277-skill-marketplace-plugin-packaging
Open

[US-277] feat: .claude-plugin marketplace + plugin manifests with dataset catalog guard#386
rucka wants to merge 27 commits into
mainfrom
feature/US-277-skill-marketplace-plugin-packaging

Conversation

@rucka

@rucka rucka commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

PR Information

PR Title: [US-277] feat: .claude-plugin marketplace + plugin manifests with dataset catalog guard
Story/Epic: #277 (epic #213 — Skill marketplace + Quickstart + external KB scaffold)
Type: Feature
Priority: Medium
Labels: user story, risk:yellow

⚠️ Scope revised 2026-07-31 — read this before the older sections

The plugin no longer ships the catalog. It ships one bootstrap skill. marketplace.json's entry now sources packages/knowledge-hub/dataset/plugin, whose .claude-plugin/plugin.json declares a single authored, isolated skill — pair-capability-setup-cli — which installs pair-cli and runs it, so the knowledge base and the full skill catalog are produced by the CLI in the user's repository.

Why, in one paragraph: the whole-repository payload (source: "./") put pair's own .pair/ inside every plugin cache. A skill step that read an adoption file resolved pair's decisions as if they were the user's, with no graceful degradation — from the skill's point of view the files were present — and /pair-capability-record-decision wrote adoption files into a cache the next /plugin update erased. Both were shipped as documented "known limitations". This revision removes their cause.

Verified live, not argued (CLI v2.1.220, sandboxed CLAUDE_CONFIG_DIR, this branch):

  • claude plugin details pair@pairSkills (1) pair-capability-setup-cli, Agents (0), Hooks (0), MCP servers (0), LSP servers (0)
  • Installed payload: 12 KB (du -sh on the cache) against ~19 MB before; always-on token cost ~124
  • The cache holds .claude-plugin/ + skills/ and no .pair/ — walked to confirm
  • claude plugin validate passes at both the plugin root and the repo root, the latter resolving the entry's manifest through the new source

One layout fact the probe caught, and no repo guard would have: Claude Code expects the manifest at <plugin root>/.claude-plugin/plugin.json, not at <plugin root>/plugin.json. The first attempt at this revision had the bare form and would have shipped an uninstallable plugin.

Two later scope additions, both maintainer-directed. (1) The plugin skill became an assistant/pair-assistant, renamed before merge because the name is public: it sets pair up when missing, then drives the CLI and answers from the project's own .pair/llms.txt, discovering both surfaces at runtime instead of copying them here. Its brownfield branch asks before writing, and hands /pair-process-bootstrap the context it already gathered. (2) That handoff exposed a conformance gap in bootstrap's quick mode: the guided/quick convention defines one cascade for every decision point (explicit argument > project state > recorded decision > KB fallback), and three sections — architecture, infrastructure, observability — went straight to fallback. So on a repository with a Dockerfile, a terraform/ dir or a telemetry dependency, quick wrote the KB default with the evidence in plain sight. Fixed here on the maintainer's call rather than as its own issue; it touches dataset/.skills/process/bootstrap/** and the checklist asset, which is a different subsystem from the plugin packaging — worth knowing when reviewing.

What this invalidates. Review rounds 1–6 were conducted against the whole-repo shape. ADL Decisions 1, 3 and 5 are rewritten (2 and 4 survive); the hand-maintained 41-entry catalog and its dataset-derived guard are gone, replaced by a corpus-derived list; "pick one channel, not both" is retired, since no skill name resolves from two sources. The sections further down that predate this banner describe the old shape unless they say otherwise.

Summary

What Changed

Adds the native Claude Code install channel (D1): .claude-plugin/marketplace.json + .claude-plugin/plugin.json at the repo root, so /plugin marketplace add foomakers/pair/plugin install pair@pair installs pair's full 40-skill catalog with no pair-cli step. The CLI (pair-cli init/update) remains the tool-agnostic channel for Cursor/Windsurf/Copilot and for projects that want their own .pair/ adoption files.

  • marketplace.json: thin metadata — name, owner, one plugins[] entry (source: "./").
  • plugin.json: owns the component list — 40 explicit ./.claude/skills/pair-<flattened-name> entries (not a glob: .claude/skills/agent-browser is a third-party skill this repo consumes, and non-declaration means it is never exposed or loaded as a distributed pair skill; note the exact scope — with source: "./" the file is still copied into the cache, so this is a loading control, not a redistribution control, as the ADL's Decision 3 note states). No version pinned (falls back to commit SHA so /plugin update always fetches current). Skills only, enforced as an allowlist on all three hand-edited surfaces: plugin.json may carry metadata + skills; marketplace.json's own top level may carry marketplace metadata + owner/plugins (never forceRemoveDeletedPlugins, allowCrossMarketplaceDependenciesOn or metadata.pluginRoot); a marketplace entry may carry metadata + source/category/tags/strict and no skills — every other schema key (hooks, mcpServers, lspServers, monitors, commands, dependencies, settings, userConfig, channels, outputStyles, themes) fails the guard, as does any key the schema doesn't declare. Rule of record: the ADL's Decision 5 (the "D23/R9.3" identifiers resolve to nothing in-repo, so they are no longer cited as normative).
  • Catalog guard (packages/knowledge-hub/src/tools/claude-plugin-manifest.ts + colocated test, 301 lines): the manifest is hand-maintained by design (AC3) — the guard derives the expected catalog from packages/knowledge-hub/dataset/.skills/ through the real install transform (installedSkillDir, the same one pair update applies) and fails loudly naming the exact entry to add/remove, so a skill added/renamed/removed without a manifest update is caught by pnpm test, not discovered by a user.
  • Docs: apps/website/content/docs/integrations/claude-code.mdx gets a new "Install Channels" section (marketplace vs CLI comparison table, install/update/uninstall commands, a dedicated "Adoption Files on the Marketplace Channel" section documenting the known adoption-resolution limitation — read and write — with the caveat also carried inline in the channel table). RELEASE.md and the docs-site release-process page get a 3-item release checklist for the manifest (never run claude plugin validate --strict: it fails the deliberate no-version-pin).
  • ADL: .pair/adoption/decision-log/2026-07-28-marketplace-plugin-packaging.md records the spike findings and all five packaging decisions.

Why This Change

Fills the D1 gap: pair had no native Claude Code distribution path — only the CLI. A user in Claude Code alone had to run pair-cli init just to get the skills. The marketplace channel gives a zero-setup path while keeping the CLI channel for projects that want the KB/adoption files living in their own repo.

Story Context

User Story: As a Claude Code user, I want to install pair's skill catalog via /plugin marketplace add, so I can use pair's SDLC skills without a CLI bootstrap step.

Acceptance Criteria (story #277, verbatim):

  1. Given the foomakers/pair repo with .claude-plugin/marketplace.json present When a Claude Code user runs /plugin marketplace add foomakers/pair Then pair's skills install as a Claude Code plugin, without needing pair-cli
  2. Given the marketplace manifest When it is inspected Then it lists every currently-shipped skill from packages/knowledge-hub/dataset/.skills/, using the same names/descriptions as the dataset source (no drift between the two)
  3. Given a new skill is added to the KB, or an existing one is renamed/removed When the next release ships Then marketplace.json is updated by hand as part of that release's checklist — it is not auto-generated
  4. Given the CLI-based install path When the marketplace channel is also available Then both channels coexist without conflict — a project uses exactly one of the two (D1), never both at once

Business rules: dual channel (D1, same source of truth); no named role agents distributed (D23/R9.3 — skills only); hand-maintained, not generated.

AC → delivery mapping:

Story AC Where it is delivered How it is verified
AC1 — native install, no pair-cli .claude-plugin/marketplace.json (one entry, source: "./") + .claude-plugin/plugin.json (40 skill entries) Verified end-to-end pre-merge (round 2): claude plugin marketplace add accepts a path, so the marketplace path is testable without being on main. Sandboxed via CLAUDE_CONFIG_DIR at HEAD: marketplace add <path> → "Successfully added marketplace: pair", plugin install pair@pair → "Successfully installed", claude plugin details pair@pairSkills (40), Agents (0), Hooks (0), MCP servers (0), LSP servers (0). Plus claude plugin validate . (v2.1.220) and claude --plugin-dir <repo> from outside the repo loading all 40 skills as pair:pair-*, bare /pair-… also resolving (composition pair pair-process-reviewpair-capability-verify-quality both ACCEPTED). Residual gap: only GitHub-shorthand resolution of foomakers/pair, which genuinely resolves after merge.
AC2 — every shipped skill listed, no name/description drift plugin.json skills = 40 dataset-derived paths Deviation, deliberate: the manifest carries paths only; names/descriptions stay in each SKILL.md frontmatter (ADL Decision 3 — copying them in would create a second drift surface). Guarded instead by assertSkillCatalogInSync (expected list derived from the dataset through the real install transform) + a per-entry test pinning each declared path to its frontmatter name, with the mirror-equality guard pinning name+description byte-for-byte to the dataset. AC2 on story #277 is annotated with this pointer. For the human merge gate: AC2's wording was re-read after refinement ("provably non-drifting from the dataset source" instead of the literal "using the same names/descriptions as the dataset source") — please acknowledge that re-reading explicitly in the merge decision rather than absorbing it silently, since the AC record is the contract a later reader validates against.
AC3 — hand-maintained, release-checklist item RELEASE.md + release-process.mdx checklist; guard asserts, never generates Drift-injection tests: missing / stale / duplicate entry, dangling SKILL.md, forbidden component key — each fails naming the exact hand edit.
AC4 — the two channels coexist, exactly one per project claude-code.mdx Install Channels section (table + two callouts) Documented consequence, not detection: installing both makes the bare /pair-… form match two sources, and resolution order between project scope and the plugin is unspecified — we verified bare-name resolution only with the plugin as the sole source, so the docs claim nothing more and point at the prefixed /pair:pair-… form when certainty is needed. No automatic detection of the both-channels state exists; that would be its own story.

Classification

Review-time matrix (establishment — story #277 carries no refinement-time matrix; the same gap exists on recently merged #385/#381/#378/#377, so the systemic side warrants its own story, to be filed rather than resolved inside this PR):

Dimension Value Why
Service/domain criticality 🟡 yellow tech/risk-matrix.md declares no criticality table → KB default Medium
Change/diff risk 🟡 yellow re-assessed after the 2026-07-31 scope revision: the payload shape changed and the catalog guard was rewritten; no longer docs+manifests only — it now changes the CLI install pipeline (apps/pair-cli/src/registry/, packages/content-ops copy paths). Still yellow, not red: additive behind an option nothing sets yet, no schema/migration/infra change, and both copy paths are covered by unit tests plus a smoke scenario over the real CLI
Business impact 🟡 yellow supporting subdomain — skill distribution packaging; the KB/skill corpus itself (core) is untouched
Security relevance 🟡 yellow security-adjacent: defines what payload ships to every installed user's machine (skills-only rule, no hooks/MCP). No authn/secrets/PII code
Coupling balance 🟢 green one new leaf guard module inside knowledge-hub/src/tools, no new cross-context integration

Tier = max = 🟡 yellowrisk:yellow applied to this PR. Cost class: 🟢 green (no paid SDK, no provisioning, no LLM/API call added) — computed, not projected as a tag (Active: risk only).

Changes Made

Implementation Details

  • Spike (pre-work, ADL §Context): Claude Code's plugin/marketplace schema verified against the live docs + published schemastore schemas + claude plugin validate . on CLI v2.1.220 — findings recorded before any manifest was written.
  • Manifests: .claude-plugin/marketplace.json, .claude-plugin/plugin.json — repo-root plugin (source: "./"), so .claude/skills/ and .pair/knowledge/ both land in the plugin cache and every skill's relative KB link (../../../.pair/knowledge/...) resolves at runtime (a slimmer plugin dir would ship skills with dangling KB pointers — plugins can't reach outside their own root).
  • Catalog guard: parseMarketplaceManifest / parsePluginManifest (required-field validation per the schemastore schemas), expectedPluginSkillPaths (dataset-derived, sorted, so a new skill extends it with zero code edits), assertSkillCatalogInSync (missing/stale/duplicate detection with copy-pasteable fix), assertDeclaredSkillsResolve (no dangling SKILL.md pointer), assertSkillsOnlyDistribution (per-manifest-kind allowlist of permitted keys — unknown or newly specified schema keys fail closed), assertNoRootPluginComponents (payload-level: no root agents/, commands/, hooks/, skills/, .mcp.json — a root skills/ dir is auto-discovered on top of the manifest field and would bypass the catalog guard).
  • Docs: Install Channels comparison table + marketplace install/update/uninstall commands + adoption-resolution limitation callout (claude-code.mdx); 3-item release checklist (RELEASE.md + release-process.mdx).
  • ADL: all 5 packaging decisions + 4 rejected alternatives + consequences, in 2026-07-28-marketplace-plugin-packaging.md.

Files Changed

26 files, +1926/−24. The manifest half is what the story asked for; the CLI half is the exclude enabler added in c1692f05 (see the scope-addition section at the bottom).

  • Added — manifests + guard: .claude-plugin/marketplace.json, .claude-plugin/plugin.json, packages/knowledge-hub/src/tools/claude-plugin-manifest.ts (+ its colocated test), .pair/adoption/decision-log/2026-07-28-marketplace-plugin-packaging.md
  • Added — exclude verification: scripts/smoke-tests/scenarios/registry-exclude.sh (both copy paths + the invalid-config case, driving the real packaged CLI)
  • Modified — CLI install pipeline (exclude): apps/pair-cli/src/registry/{operations,resolver,validation}.ts (+ their tests), packages/content-ops/src/ops/SyncOptions.ts, packages/content-ops/src/ops/copy/{copy-directory,copy-directory-transforms}.ts (+ test), packages/content-ops/src/file-system/file-operations.ts (the shared isExcluded), scripts/smoke-tests/lib/utils.sh (assert_no_file)
  • Added — the bootstrap plugin: packages/knowledge-hub/dataset/plugin/.claude-plugin/plugin.json, packages/knowledge-hub/dataset/plugin/skills/pair-capability-setup-cli/SKILL.md (authored, isolated); the repo-root .claude-plugin/plugin.json is deleted — the manifest moved to the plugin root
  • Modified — docs + counts: README.md, RELEASE.md, apps/website/content/docs/contributing/release-process.mdx, apps/website/content/docs/integrations/claude-code.mdx, apps/website/content/docs/reference/configuration.mdx, apps/website/lib/docs-staleness-check.ts (+ test) — the skill count was 40 in three places and is 41; two phrasings that let it drift are now pinned by the staleness gate
  • packages/knowledge-hub/src/conformance/code-host-routing.test.ts shows in a three-dot diff but is byte-identical to origin/main — a merge-base artifact, contributing nothing to the merge

Testing

Test Coverage

  • Unit tests: new claude-plugin-manifest.test.ts (126 tests) — manifest parsing (required fields, malformed JSON, bad source shapes), catalog sync assertion (missing/stale/duplicate detection with exact messages), dangling-SKILL.md detection, allowlist drift injection over all 13 component/behaviour keys the published schemas declare, on each of the three hand-edited surfaces (plugin manifest, marketplace.json's own top level — incl. forceRemoveDeletedPlugins, allowCrossMarketplaceDependenciesOn, nested metadata.pluginRoot — and each plugins[] entry) (plus a non-schema key such as displayName, and the marketplace-entry skills case whose message spells out that it replaces plugin.json's catalog), plugin-root escape (..) rejection, and root-level component-payload absence, whose path list is derived from SCHEMA_COMPONENT_KEYS (so it covers monitors/ and anything a future schema adds) plus .mcp.json.
  • Manual validation: claude plugin validate . passes on CLI v2.1.220 (one expected "no version" warning — documented tradeoff); claude --plugin-dir <repo> loads all 40 skills from a directory outside the repo, confirmed invocable as /pair:pair-next and the bare /pair-next.

Test Results

pnpm --filter @pair/knowledge-hub test  → 644 passed / 23 test files (126 of them claude-plugin-manifest.test.ts) + link-check PASS (dataset + root .pair)
pnpm quality-gate                       → EXIT=0 at 0d25763b (ts:check + test + lint + prettier + mdlint + hygiene + docs-staleness + skills-conformance + jscpd)
pnpm --filter @pair/knowledge-hub lint  → clean
turbo run ts:check lint test --filter @pair/knowledge-hub --filter @pair/website → 9/9 successful, FULL TURBO
Manifest catalog check: ls .claude/skills (minus agent-browser) = 40; plugin.json skill entries = 40 → in sync

Quality Assurance

Review Areas

  • Business logic: verify the packaging decisions (repo-root source, hand-maintained + guarded catalog, no version pin) against the ADL's rejected-alternatives section.
  • Code structure: guard module follows the gate-tooling-in-tested-modules ADL (2026-07-13) — assertion logic lives in a tested module, never a bare script.
  • Testing: catalog guard test coverage for every drift class (missing/stale/duplicate/dangling/agents-key).

Deployment Information

No deployable package touched — this PR only changes packages/knowledge-hub (a guard module + test), root .claude-plugin/ config, apps/website docs content, and RELEASE.md. No environment/config/migration impact. .claude-plugin/ is picked up by Claude Code directly from the repo — no build/release step required for it to take effect.

Reviewer Guide

Testing the Changes

pnpm --filter @pair/knowledge-hub test
claude plugin validate .
claude --plugin-dir . # from a directory outside the repo, then /plugin list to see all 40 skills

Key Test Scenarios

  1. Add/rename/remove a skill directory under packages/knowledge-hub/dataset/.skills/ without touching .claude-plugin/plugin.jsonpnpm test fails naming the exact entry to add/remove.
  2. Add any non-allowlisted key to either manifest — hooks, mcpServers, monitors, settings, dependencies, commands, a typo, … — or create a root-level agents/, commands/, hooks/, skills/ or .mcp.json → guard fails, pointing at the ADL's Decision 5 (with source: "./" the repo IS the payload, so the root-level paths would load regardless of the manifest).
    2b. Add "skills": [...] to the marketplace.json entry → guard fails explaining that an entry-level skills REPLACES plugin.json's catalog and is invisible to assertSkillCatalogInSync. Runtime-confirmed before the fix: one such key installed a 1-skill plugin instead of 40, with claude plugin validate and every other guard still passing.
  3. Point a manifest skill entry at a directory with no SKILL.md → guard fails citing the broken path.

Dependencies & Related Work

Related PRs

Follow-up Work

Stakeholder Communication

Not applicable — internal tooling/distribution change, no customer-facing behavior change beyond the new install path documented above.


Scope addition (c1692f05) — registry-level exclude, the enabler for a single marketplace channel

Widened deliberately with the maintainer, after this PR's own limitation was talked through. This adds code, not documentation — flagged prominently because the PR's title and everything above describe manifests only.

A registry may now declare exclude: [...] — source-relative entries the copy never installs. Threaded config → RegistryConfigSyncOptions → the copy pipeline, the same route flatten/prefix already travel.

Why it belongs here

This PR documents two channels and warns "install one, never both — nothing detects this state". The agreed resolution (tracked in #392) is one channel: the marketplace ships a single setup skill that invokes pair-cli, and everything else arrives in the project with consistent paths. That skill must exist only when installed from the marketplace — otherwise the CLI writes it too, and the same skill name answers from two trees (~/.claude/plugins/cache and the project's .claude/skills/) whose resolution order this PR's own ADL records as never observed. The exclusion is what makes the single-channel shape possible, so it lands with the story that creates the need.

Measured while deciding: of pair's 42 installed skills exactly 1 has no KB reference, and it is the third-party agent-browser. So "ship the isolated skills" has an empty payload — the setup skill has to be written, not carved out.

Design points, each with a test

  • Matching is per path segment, never a string prefix: excluding process/setup must not silently drop process/setup-helper. That is the classic shape of this bug and it has its own test.
  • An excluded entry's whole subtree is skipped, so a references/ sub-doc does not survive its skill.
  • Filtered before validation and before any mkdir, so an excluded entry cannot contribute a flatten collision, a directory mapping or a link-rewrite pass — as if it were never in the source.
  • Absent ⇒ byte-identical previous behaviour, asserted, so no other registry moves.
  • buildCopyOptions reads it defensively: it is called with hand-built RegistryConfig objects too, not only with extractRegistries output — four existing tests proved that the moment a required field was assumed.
  • extractRegistries ignores a non-array value rather than trusting it.

Known limit, in the code and in #392

This prevents writing an entry, not having it. The skills registry runs behavior: overwrite, which performs no cleanup, so an excluded entry already present in a target is left untouched.

Also fixed here

The catalog guard flagged pair-process-brainstorm missing from plugin.json after #387 merged — the first time that guard has fired on a real omission. Without it the plugin would have shipped 40 skills out of 41. Catalog now at 41 entries.

Verification at c1692f05: pnpm quality-gate green, content-ops 22 copy cases, pair-cli 190 registry cases.

collectInstallableFiles was extracted to keep copyDirectoryWithTransforms under the 50-line ceiling — caught by the gate, not by the tests.

@rucka

rucka commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator Author

Verdict

risk:yellow · cost:greenCHANGES-REQUESTED — manifests, guard and tests are solid and AC1-AC3 land, but the shipped "known limitation" text misstates a real hazard (the plugin cache carries pair's own .pair/adoption/, so consuming projects silently inherit pair's PM tool/org), and the skills-only guard backs only 1 of the 3 keys the PR claims it backs.

PR: [#386] · Author: rucka · Reviewer: independent reviewer (Claude Opus 5) · Date: 2026-07-28 · Story: [US-277] · Type: feature (config + tooling + docs)

Classification matrix — per dimension
Dimension Tier Source Note
Service/domain criticality yellow KB default tech/risk-matrix.md declares no ## Criticality Table → KB default (Medium)
Change/diff risk yellow diff footprint additive, no schema/contract change, but spans root config + packages/knowledge-hub + apps/website
Business impact yellow subdomain class Integration & Process Standardization = Supporting (.pair/adoption/product/subdomain/)
Security relevance yellow manual read (/assess-security not composed) security-adjacent: the channel copies the whole repo — incl. pair's adoption + a third-party skill — onto user machines; no authn/authz/secrets/PII/untrusted parsing touched; gitleaks clean on both commits
Coupling balance green manual read (/assess-coupling not composed) new module depends only on a sibling tool module in the same package (skill-md-mirror) — balanced

Tier = max(assessed) = yellow. Cost = highest detected signal = green. Review value is a floor (D17): confirm or raise, never lower.

No refinement-time matrix existed on story #277 (and PR #386 carries no risk:* label) — see the Minor finding. This matrix is therefore established at review time, not a drift note; nothing was lowered.

Assessments

Security — Input validation

Verdict: green — the only new inputs are two repo-owned JSON manifests, parsed by parseMarketplaceManifest/parsePluginManifest with required-field + kebab-case + ./-prefix validation before use.

Details
  • .claude-plugin/*.json are repo-controlled, not user input; malformed JSON fails with a file-attributed message rather than a downstream type error.
  • One gap: requireRelativePath accepts ./../… (see Minor findings) — no exploit path (only existsSync/readFileSync under the repo root, in a test), just a less precise failure.

Security — Output handling

Verdict: green — no rendering, encoding, or serialization surface. Guard output is thrown Error messages containing repo-relative paths only.

Security — Authentication

Verdict: green — no authentication surface touched.

Security — Authorization

Verdict: yellow — no access-control code changes, but the channel changes what runs with the user's credentials: skills loaded from the plugin cache resolve pair's way-of-working.md, which names GitHub Projects and "the project is pair under the github organization foomakers". A PM-writing skill on the marketplace channel can therefore be pointed at pair's own board/org with the user's gh credentials. Documented as a limitation, but with wording that understates it — Major finding #1.

Security — Introduced vulnerabilities

Verdict: green — 0 introduced, 0 pre-existing.

Details
Severity Category File:location Introduced / pre-existing Recommendation
none found

gitleaks detect --log-opts origin/main..HEAD → no leaks (2 commits, 39 KB scanned). No new dependency, no network call, no credential handling in the diff. /pair-capability-assess-security was not composed (independent-review pass); this is a manual read of the diff against the security guidelines.

Cost

Verdict: cost:green — no billed signal in the diff.

Details
Signal Class Provider Note
paid SDK import green none
API-key env var green none
IaC / provisioning green none
cron / queue / media / LLM call green none
distribution payload green GitHub source: "./" ⇒ ~19 MB repo copy per installed version (ADL-accepted); GitHub egress, no billed service

/pair-capability-assess-cost was not composed; class derived manually from the cost-signal catalog.

Architecture (Coupling)

Verdict: green — the one new integration (claude-plugin-manifest.tsskill-md-mirror.ts) is strong-but-close: same package, same team, low volatility ⇒ balanced. Deriving the expected catalog through the real install transform instead of a second copy of the naming rule removes a drift surface rather than adding coupling.

Details
  • Integration strength: high (direct import of datasetSkillDirs / installedSkillDir). Socio-technical distance: none (same module folder). Volatility: low. Balanced per the coupling model.
  • No new cross-context integration; .claude-plugin/ is consumed by Claude Code directly, with no pair-cli coupling (D1 respected — the dataset does not carry .claude-plugin/, so consumer projects never receive pair's manifests).
  • /pair-capability-assess-coupling was not composed; verdict from a manual read.

Details

Findings by severity

Critical (must fix before merge)

  • none

Major (should fix before merge)

  • apps/website/content/docs/integrations/claude-code.mdx:37-39 (+ .pair/adoption/decision-log/2026-07-28-marketplace-plugin-packaging.md:66) — the shipped limitation text says a marketplace project "gets the guideline defaults (each skill's documented graceful-degradation path)" / "behaves as if adoption files were absent". That is not what happens: source: "./" copies the whole repo into ~/.claude/plugins/cache, so .pair/adoption/ is present — pair's own. No graceful-degradation path fires (the files exist), and skills resolve pair's decisions as if they were the consuming project's: way-of-working.md ("Github Projects is adopted … the project is pair under the github organization foomakers … use #github MCP"), tech/risk-matrix.md (Active: risk), tech/coverage-baseline.md. Verified: .claude/skills/pair-process-review/SKILL.md reads ../../../.pair/adoption/tech/tech-stack.md, and the PM-tool resolution convention reads .pair/adoption/tech/way-of-working.md. Impact: a marketplace user running /pair-capability-write-issue, /pair-process-review or /pair-capability-verify-quality can be steered at pair's own board/org and gated by pair's deltas, while the docs tell them they are on the defaults path. Fix: state the actual behaviour in both texts (pair's adoption files travel in the cache and will be read as the project's) and say plainly that PM-tool/adoption-dependent skills need the CLI channel. Structural fix tracked separately as Project-scoped adoption resolution for the Claude Code plugin (marketplace) channel #392.
  • packages/knowledge-hub/src/tools/claude-plugin-manifest.ts:275-283assertNoDistributedAgents rejects only agents, while this PR's own AC4 and both new release checklists state "no agents, hooks, or mcpServers in either manifest" and name this guard as the backstop. A future hand-edit adding hooks (shell commands that run on every installed user's machine) or mcpServers passes pnpm test silently — exactly the drift class the guard exists to catch, and the highest-consequence one. Fix: assert over the three keys (one loop, one message) and extend the drift-injection test; or rename the helper and state in the checklist that hooks/mcpServers stay manual-only.

Minor (consider)

  • apps/website/content/docs/integrations/claude-code.mdx:51-131 — everything after the new Install Channels section (Project Structure, .claude/ directory, AGENTS.md/CLAUDE.md generation, Bridge Pattern) describes the CLI channel only, unlabelled. A marketplace reader going top-to-bottom is told to expect .claude/skills/ + .pair/ in their own project and an AGENTS.md/CLAUDE.md pipeline they will not have. Add a one-line channel scope note under ## Configure Agent File.
  • packages/knowledge-hub/src/tools/claude-plugin-manifest.ts:95-103requireRelativePath accepts ./../…: it enforces the ./ prefix but not the ADL's own invariant ("a plugin cannot reach outside its own root", Decision 1). A typo like ./../.claude/skills/pair-next is accepted here and only caught downstream by the sync/resolve assertions, with a message about staleness rather than about escaping the plugin root. Reject any .. path segment.
  • packages/knowledge-hub/src/tools/claude-plugin-manifest.ts:220-230missing is computed from seen, a set populated as a side effect of the duplicate-detection filter ((seen.add(path), false)). Correct today only because the duplicate branch throws; reordering the two checks, or an early return, silently turns missing-detection into "everything missing"/"nothing missing". Build the declared set explicitly (const declaredSet = new Set(declared)) and keep the filter side-effect-free.
  • packages/knowledge-hub/src/tools/claude-plugin-manifest.test.ts:96-99it.each re-reads and re-parses PLUGIN_JSON although the enclosing describe already holds the parsed declared list in scope. Duplicate I/O + a second source of truth for the same list inside one file; use it.each(declared).
  • packages/knowledge-hub/src/tools/claude-plugin-manifest.test.ts:285expect(message).not.toContain('pair-next,') can never fail: assertDeclaredSkillsResolve renders one - <path>/SKILL.md per line and never a comma-separated list, so the intended assertion ("a resolving entry is not reported") is vacuous. Assert not.toContain('pair-next/SKILL.md') instead.
  • .pair/adoption/decision-log/2026-07-28-marketplace-plugin-packaging.md:40 — Decision 3's rationale ("agent-browser, a third-party skill this repo consumes but has no right to redistribute") over-claims what non-declaration achieves: with source: "./" the file is copied into every user's plugin cache; only loading it as a pair plugin skill is prevented. Same shape for .claude/agents/{reviewer,implementer,contract-generator}.md — they ship in the payload; D23/R9.3 holds only because Claude Code discovers plugin agents from agents/ at the plugin root (absent here, verified), not from .claude/agents/. Reword to "never exposed as a distributed pair skill/agent" so a future maintainer does not rely on non-declaration as a redistribution control.
  • PR [US-277] feat: .claude-plugin marketplace + plugin manifests with dataset catalog guard #386 (metadata) / story Skill marketplace: marketplace.json + plugin packaging for native Claude Code install #277 — no risk:* label on the PR and no classification matrix in the PR description, although .pair/adoption/tech/risk-matrix.md declares Active: risk and quality-model §3.2 treats an unclassified PR as fail-safe red. Story Skill marketplace: marketplace.json + plugin packaging for native Claude Code install #277 carries no refinement-time matrix either. Apply risk:yellow to the PR (matrix recorded in this review). Note the same gap exists on chore: post-merge fixups — assess-coupling no longer 'pending #263' + prettier format #385/[US-263] feat: coupling-balance guideline + assess-coupling capability #381/[#259] feat: verify-quality integrated with tier gate matrix (local = CI) #378/[US-352] chore: extend mirror-equality guard to per-skill SKILL.md pairs #377, so the systemic side may deserve its own story.
  • apps/website/content/docs/integrations/claude-code.mdx:14 — story AC4 ("both channels coexist without conflict — a project uses exactly one, never both") is delivered as one sentence ("never both at once, or the same skill is installed twice from two sources") and nothing detects the both-channels state. The concrete failure mode is missing: duplicate skill names make the bare /pair-… form non-unique, and every cross-skill composition in the corpus (and every doc reference) uses the bare form. State that consequence; an actual detection guard would be its own story.
  • PR description ("Story Context → Acceptance Criteria") — the four ACs listed are not story Skill marketplace: marketplace.json + plugin packaging for native Claude Code install #277's four ACs (PR AC2 = "no dangling manifest pointers" vs story AC2 = "lists every shipped skill using the same names/descriptions as the dataset"; PR AC4 = skills-only, which is story Skill marketplace: marketplace.json + plugin packaging for native Claude Code install #277's business rule, while story AC4 = dual-channel coexistence). A reviewer checking AC coverage from the PR alone validates a different set. Quote the story ACs verbatim or map PR→story explicitly.
  • .claude-plugin/plugin.json:3 / ADL Decision 1 — installing the plugin copies the entire ~19 MB repository (apps, packages, qa/, reports/, scripts/, .github/, all tool configs) into the plugin cache per version. Accepted tradeoff, analysed and justified in the ADL; recorded here for transparency only. Non-actionable — by design (ADL Decision 1: a slimmer plugin root dangles every skill's ../../../.pair/knowledge/… link).
  • Story Skill marketplace: marketplace.json + plugin packaging for native Claude Code install #277 DoD — "Manual test: /plugin marketplace add foomakers/pair in a real Claude Code session" — still unchecked, and unresolvable pre-merge: the marketplace serves .claude-plugin/ from the default branch. claude plugin validate . on CLI v2.1.220 was re-run independently in this review and passes (one expected "no version" warning). Non-actionable — resolves after merge; the human merge gate should run the real install once on main.

Questions

  • .claude-plugin/plugin.json:14-55 vs story Skill marketplace: marketplace.json + plugin packaging for native Claude Code install #277 AC2 — AC2 literally asks the manifest to list skills "using the same names/descriptions as the dataset source"; the shipped manifest lists directory paths only, with names/descriptions deliberately left in SKILL.md frontmatter (ADL Decision 3, to avoid a second drift surface). The intent (no drift) is met and better served, and the per-entry test pins each declared path to its frontmatter name — but the story record still states the literal requirement. Annotate AC2 on Skill marketplace: marketplace.json + plugin packaging for native Claude Code install #277 with the ADL deviation so story and implementation agree?
  • Cross-skill composition on the plugin channel — skill bodies invoke siblings by the bare /pair-… name, while plugin skills are exposed as pair:pair-…. The recorded manual validation covers loading and invoking a skill, not a composition (e.g. /pair-process-review/pair-capability-verify-quality, /pair-capability-classify). Since every multi-phase process depends on it, has one composition been exercised end-to-end under --plugin-dir, or should the naming caveat sit next to the adoption callout?
Positive feedback
  • The guard is the right shape. It asserts and never generates (AC3 honoured literally), derives the expected catalog from the dataset through the real install transform (installedSkillDir) instead of re-implementing the naming rule, and keeps the filesystem out of the assertions (hasSkillMd injected). A new dataset skill extends the expectation with zero code edits — verified: adding dataset/.skills/capability/zzz-fake/SKILL.md fails the sync test naming the exact entry to add, and only that test.
  • Drift classes are covered individually — missing, stale, missing+stale together, duplicate, dangling SKILL.md, agents key — each with the failure message asserted, not just "throws".
  • Deliberately not duplicating names/descriptions into the manifest is the single best decision here: it leaves exactly one drift surface (paths), already pinned by the mirror-equality guard.
  • The ADL is genuinely useful — five decisions, four rejected alternatives each with the concrete reason (symlinks on Windows, dataset-name divergence, 40-plugin install, generation pipeline), and the spike findings recorded before the manifests. Template-conformant.
  • No version pin, with the reasoning written down and pinned by a test (plugin.version undefined) so nobody "helpfully" adds one — independently confirmed: claude plugin validate . passes with exactly the one documented warning.
  • Manifests are schema-correct against the published schemastore schemas (independently fetched: marketplace.json requires name/owner/plugins, entries require name+source with pattern ^\./.*; plugin.json skills accepts a string or array of ./-relative dirs; category/tags are valid entry fields). All manifest URLs resolve (200).
Functionality & requirements (AC coverage)
  • AC1 (marketplace add/install installs the catalog) — manifests present and schema-valid; claude plugin validate . re-verified independently on CLI v2.1.220; 40 declared entries, all resolving to a real SKILL.md; end-to-end /plugin marketplace add inherently post-merge.
  • AC2 (lists every dataset skill, no drift) — met in substance and enforced by the guard: 40 tracked dataset/.skills skills = 40 declared entries; agent-browser correctly excluded; each declared path pinned to its frontmatter name. Literal "same names/descriptions" deliberately not implemented (ADL Decision 3) → open Question.
  • AC3 (hand-maintained, release-checklist backstop) — no generation anywhere; checklist added to RELEASE.md + docs site; automatic drift guard verified failing on injected drift.
  • [~] AC4 (channels coexist, one per project) — documented in the channel table only; no detection, and the real failure mode (ambiguous bare skill names breaking compositions) is unstated → Minor.
  • [~] Business rule — skills only, no named agents (D23/R9.3) — satisfied by both manifests today; the automatic backstop covers agents only → Major Collaborative Knowledge Base #2, plus the payload-level caveat → Minor.
  • Edge case "skill renamed/removed without a manifest update" — now caught automatically, an improvement over the story's own "caught only by the release checklist".
  • Error handling — every guard failure names the file, the exact entry, and the hand edit to make.
Testing & quality gates

All gates re-run independently in a detached worktree pinned to 5d1cbf37 (not taken from the PR description):

  • pnpm --filter @pair/knowledge-hub test590 passed / 23 files (incl. 72 in claude-plugin-manifest.test.ts) + check:links PASS (dataset + root .pair/knowledge)
  • pnpm --filter @pair/website test → vitest + 20 Playwright CT (incl. a11y) passed
  • turbo ts:check lint --filter @pair/knowledge-hub --filter @pair/website5/5 successful
  • prettier --check (project config) on .claude-plugin/*.json → clean; root markdownlint '*.md' → clean; pnpm hygiene:check → PASS; pnpm docs:staleness → PASS (40 skills, 8 commands in sync)
  • gitleaks detect --log-opts origin/main..HEAD → no leaks
  • claude plugin validate . → passes (1 documented warning)
  • Drift injection (mine, not the suite's): added a fake dataset skill → the AC2 sync test failed naming ./.claude/skills/pair-capability-zzz-fake; worktree restored.
  • Gates required for risk:yellow (lint + type + build + unit): all green.
  • Coverage: 282 source lines / 301 test lines, every exported function exercised on both the happy and failing path.
Adoption compliance
  • Degradation level: 2/pair-capability-verify-adoption reasoning applied manually (independent-review pass, capabilities not composed); stack resolution done by inspection.
  • No dependency added — package.json untouched anywhere in the diff; the new module imports only Node built-ins (in the test) and a sibling module. tech-stack.md needs no change.
  • Patterns match architecture.md + the two governing ADLs: gate tooling lives in a tested module, not a script (2026-07-13), and the test is colocated with the module (2026-07-08).
  • New decision recorded as an ADL (2026-07-28-marketplace-plugin-packaging.md), template-conformant (Date/Status/Category/Context/Decision/Alternatives/Consequences/Adoption Impact), correctly classified "Tooling Preference" not architectural — no ADR required, no HALT.
  • Naming: pair-<flattened-name> entries match the skill-naming taxonomy ADL and the installed root mirror byte-for-byte.
Tech debt

Surfaced, not blocking:

  1. Partial skills-only backstop (agents only) — raised as Major Collaborative Knowledge Base #2; fix in this PR keeps it off the debt list.
  2. Two of the three release-checklist lines stay manual (claude plugin validate ., skills-only) — deliberate per AC3; revisit only if drift is observed.
  3. ~19 MB plugin payload per installed version — ADL-accepted tradeoff.
  4. .pair/adoption/**.md is outside both check:links scopes (dataset + .pair/knowledge) and outside every package's prettier/markdownlint scope, so the new ADL's links are unguarded (both verified by hand here). Pre-existing, not introduced by this PR.
  5. Project-scoped adoption resolution for the plugin channel — filed as Project-scoped adoption resolution for the Claude Code plugin (marketplace) channel #392 (sub-issue of epic Skill marketplace + Quickstart + external KB scaffold #213) during this review.

No tech-debt label promotion recommended beyond #392.

Documentation
  • Docs site updated — integrations/claude-code.mdx (Install Channels table, install/update/uninstall, limitation callout) + contributing/release-process.mdx (checklist).
  • RELEASE.md carries the same 3-item checklist; wording is consistent between the two copies.
  • Decision recorded (ADL) and cross-linked from both checklists.
  • [~] Accuracy: the limitation callout and the ADL's matching sentence are wrong about the mechanism — Major Setup & Project Management Integration #1.
  • [~] Channel scoping of the rest of claude-code.mdx — Minor.
  • Module and function doc comments explain why (hand-maintained by design, single source of truth, injected filesystem), not just what.
  • Quickstart deliberately untouched (deferred to Quickstart path in bootstrap — opinionated one-command setup #278) and stated as such.
Performance & deployment
  • No hot path touched. Guard cost: one dataset tree read + 40 existsSync/readFileSync at test time (claude-plugin-manifest.test.ts runs in ~10 ms of the 3.1 s suite).
  • Deployment: nothing to build or release for .claude-plugin/ — Claude Code reads it from the default branch. Rollback = revert the two manifest files; no migration, no flag, no environment change. The only irreversible-ish effect is users who already installed the plugin, who would see it disappear from the marketplace on revert.

Reviewed independently from story #277's acceptance criteria, the PR diff/description, and the branch code at 5d1cbf37 (detached worktree, removed after the review). Gates and manifests were re-run, not taken on trust. Two Major findings ⇒ CHANGES-REQUESTED; both are small, local edits (a corrected paragraph in two files, and one loop in the guard + its test). Everything else is Minor or a Question.

rucka added a commit that referenced this pull request Jul 28, 2026
… fail-closed catalog sync

Review round 1 (PR #386):
- assertNoDistributedAgents -> assertSkillsOnlyDistribution: asserts agents, hooks
  AND mcpServers in one loop/message (hooks = shell commands on every user's
  machine, the highest-consequence key); drift-injection test covers all three
- requireRelativePath rejects any '..' segment, naming the ADL Decision 1
  invariant (a plugin cannot reach outside its own root) instead of surfacing
  later as a vague "stale entry"
- assertSkillCatalogInSync builds declaredSet explicitly; duplicate scan is now
  side-effect-free, so reordering the checks can no longer make the guard fail open
- new assertNoRootPluginComponents + real-repo test: with source "./" the repo IS
  the payload, so the payload-level skills-only guarantee is the absence of
  root-level agents/, commands/, hooks/, .mcp.json — now enforced, not assumed
- tests: it.each(declared) (no re-read/second source of truth); vacuous
  not.toContain('pair-next,') -> 'pair-next/SKILL.md'

Refs: #277
rucka added a commit that referenced this pull request Jul 28, 2026
…nnel scoping

Review round 1 (PR #386):
- claude-code.mdx + ADL: the "known limitation" said a marketplace project gets the
  guideline defaults / behaves as if adoption files were absent. Wrong: source "./"
  copies .pair/adoption/ into the plugin cache, so skills read PAIR's decisions as
  if they were the project's (pair's board/org via way-of-working, pair's risk-matrix
  and coverage deltas) under the user's credentials. Both texts now describe the real
  mechanism and state that adoption-dependent skills require the CLI channel (#392
  tracks the structural fix)
- claude-code.mdx: both-channels consequence spelled out (duplicate skill names make
  the bare /pair-... form non-unique, degrading every cross-skill composition, not
  just tidiness); channel-scope note under Configure Agent File (CLI channel only)
- claude-code.mdx + ADL: bare-vs-prefixed naming caveat, verified live on CLI v2.1.220
  under --plugin-dir with no pair skills in project/user scope: pair-process-review
  then pair-capability-verify-quality both resolve by bare name (composition works)
- ADL Decision 3: non-declaration prevents LOADING agent-browser / .claude/agents as
  a distributed pair skill or agent, not copying it; the payload-level guarantee is
  the absence of root-level agents/, commands/, hooks/, .mcp.json (now guarded)
- RELEASE.md + release-process.mdx: skills-only line covers root-level paths and
  states the guard is the automatic backstop for all of it

Refs: #277
@github-actions
github-actions Bot temporarily deployed to Website Preview July 28, 2026 20:32 Inactive
@rucka rucka added the risk:yellow Classification: medium risk tier label Jul 28, 2026
@rucka

This comment has been minimized.

@rucka

This comment has been minimized.

@github-actions
github-actions Bot temporarily deployed to Website Preview July 30, 2026 07:14 Inactive
@rucka

This comment has been minimized.

@github-actions
github-actions Bot temporarily deployed to Website Preview July 30, 2026 17:43 Inactive
@rucka

This comment has been minimized.

@rucka rucka self-assigned this Jul 30, 2026
rucka added a commit that referenced this pull request Jul 31, 2026
… fail-closed catalog sync

Review round 1 (PR #386):
- assertNoDistributedAgents -> assertSkillsOnlyDistribution: asserts agents, hooks
  AND mcpServers in one loop/message (hooks = shell commands on every user's
  machine, the highest-consequence key); drift-injection test covers all three
- requireRelativePath rejects any '..' segment, naming the ADL Decision 1
  invariant (a plugin cannot reach outside its own root) instead of surfacing
  later as a vague "stale entry"
- assertSkillCatalogInSync builds declaredSet explicitly; duplicate scan is now
  side-effect-free, so reordering the checks can no longer make the guard fail open
- new assertNoRootPluginComponents + real-repo test: with source "./" the repo IS
  the payload, so the payload-level skills-only guarantee is the absence of
  root-level agents/, commands/, hooks/, .mcp.json — now enforced, not assumed
- tests: it.each(declared) (no re-read/second source of truth); vacuous
  not.toContain('pair-next,') -> 'pair-next/SKILL.md'

Refs: #277
@rucka
rucka force-pushed the feature/US-277-skill-marketplace-plugin-packaging branch from 0d25763 to c1692f0 Compare July 31, 2026 16:02
rucka added a commit that referenced this pull request Jul 31, 2026
…nnel scoping

Review round 1 (PR #386):
- claude-code.mdx + ADL: the "known limitation" said a marketplace project gets the
  guideline defaults / behaves as if adoption files were absent. Wrong: source "./"
  copies .pair/adoption/ into the plugin cache, so skills read PAIR's decisions as
  if they were the project's (pair's board/org via way-of-working, pair's risk-matrix
  and coverage deltas) under the user's credentials. Both texts now describe the real
  mechanism and state that adoption-dependent skills require the CLI channel (#392
  tracks the structural fix)
- claude-code.mdx: both-channels consequence spelled out (duplicate skill names make
  the bare /pair-... form non-unique, degrading every cross-skill composition, not
  just tidiness); channel-scope note under Configure Agent File (CLI channel only)
- claude-code.mdx + ADL: bare-vs-prefixed naming caveat, verified live on CLI v2.1.220
  under --plugin-dir with no pair skills in project/user scope: pair-process-review
  then pair-capability-verify-quality both resolve by bare name (composition works)
- ADL Decision 3: non-declaration prevents LOADING agent-browser / .claude/agents as
  a distributed pair skill or agent, not copying it; the payload-level guarantee is
  the absence of root-level agents/, commands/, hooks/, .mcp.json (now guarded)
- RELEASE.md + release-process.mdx: skills-only line covers root-level paths and
  states the guard is the automatic backstop for all of it

Refs: #277
@github-actions
github-actions Bot temporarily deployed to Website Preview July 31, 2026 16:03 Inactive
@rucka

rucka commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator Author

In response to the first-review comment above (5109259411, marker pair:first-review #277 PR#386).

Escalating to human — round 5, new findings on a scope addition after round-4 convergence

Round 4 converged (all findings resolved) and a pair:remediation-final comment requesting APPROVED was posted at head 0d25763b. Since then the branch was rebased onto main (same commits, new hashes) and one new commit was added on top, c1692f05 — a registry-level exclude option in the KB copy pipeline (the enabler for a future marketplace-only setup skill, related to #392). A fresh independent review against this new head found 13 actionable findings (5 Major, 6 Minor, 2 Questions), all against code/docs introduced by that one commit. This flush supersedes the prior round-3 escalation-flush and the round-4 remediation-final comment — both have been minimized as outdated, since a "review clean / ready for merge" verdict cannot stand alongside a live escalation on the same PR.

Rounds so far (summary)

  • Round 1 (9b2ea234/a86d28fe): 2 Major + 9 Minor/Questions. All resolved, nothing escalated.
  • Round 2 (0c40871b/84ce01d9): 2 Major + 7 Minor + 2 Questions. All resolved, nothing escalated.
  • Round 3 (head still 84ce01d9): 6 Minor found, escalated (non-convergence on "have we enumerated every schema surface").
  • Round 4 (efad26c1/0d25763b): all 7 round-3 findings fixed with a structural fix (schema-derived lists instead of hand-enumeration); converged, remediation-final comment posted requesting APPROVED.
  • Round 5 (this flush) — new commit c1692f05 on top of the converged head introduces 13 new findings, detailed below.

Full per-finding detail for all 5 rounds lives in the working review log (see Persistence note below).

Round 5 — still-open findings (per finding: what/where)

Major

  1. apps/pair-cli/src/registry/operations.ts:87 (also packages/content-ops/src/ops/copy/copyPathOps.ts:91) — exclude is honored ONLY on the flatten/prefix (naming-transform) branch; the plain-copy branch never sees it, making it a silent no-op on 4 of 5 registries in apps/pair-cli/config.json (knowledge, adoption, github, agents). Proven against the built package: an excluded path was still written, no warning.
  2. apps/pair-cli/src/registry/validation.ts:82-83 + resolver.ts:74exclude has no config validation, unlike every sibling field. A string value (natural typo) is silently coerced to [] and the entry installs anyway; a mixed-type array reaches isExcluded and throws an unhandled TypeError mid-install.
  3. .codex/agents/{contract-generator,reviewer,implementer}.toml — three files unrelated to US-277, absent from the PR's Files Changed, swept in by this commit. contract-generator.toml is broken (points at a nonexistent .Codex/ path instead of .claude/).
  4. README.md:35,51 / claude-code.mdx:47 — skill count stated as 40; real catalog is 41 (confirmed by plugin.json, skills:conformance, docs:staleness, and a live install). README's bump this PR (38→40) missed the 41st skill; claude-code.mdx:47 also quotes a verification string that no longer reproduces.
  5. ADL 2026-07-28-marketplace-plugin-packaging.md:52-64 — the exclude mechanism is a new user-facing decision with no ADR/ADL entry; rationale/limits currently live only in the PR/issue prose.

Minor
6. configuration.mdx:39-47 Registry Fields table not updated with exclude.
7. PR body's Files Changed / Classification stale relative to the final diff (omits the registry/content-ops/.codex files; diff-risk note doesn't reflect the CLI install pipeline is now touched).
8. Three unrelated prettier-only reformats swept in, undeclared.
9. ADL Decision 3 present-tense "40" is stale (now 41).
10. claude-plugin-manifest.ts (519 lines / ~21 exports, 4 concerns) — DR-1 partial match, suggestion only, no gate violation.
11. No refinement-time classification matrix on story #277 to confirm-or-raise the review-time risk:yellow against (systemic gap, also present on recently merged sibling PRs, not yet filed as its own story).

Questions
12. AC4 ("channels coexist without conflict") delivered as documentation-only; nothing detects the both-installed state. For the human merge gate.
13. exclude ships with no consumer today (registries in config.json unchanged; consumer is #392) — should findings 1/2/5 be closed now, or the incomplete state stated explicitly in #392?

Why escalating rather than fixing silently: two judgment calls beyond mechanical fixes — (a) whether the exclude addition, being unconsumed today and enabling a separate story (#392), belongs in this PR at all vs. being split out; (b) findings 1/2/5 are "should this ship half-working" design calls, and findings 3/4 are regressions this same commit introduced into a PR that had already converged once.

Quality gates: unchanged/not re-run this round — none of the findings are known to flip a gate (config semantics, decision-record, doc counts, unrelated-file provenance).


Convention (for whoever picks this up next): any further rework or re-review on story #277 — including manual, out-of-band rounds run directly against this PR — should be appended to .pair/working/reviews/277.md, not posted as new standalone PR comments. The next orchestrated run on this story resumes from that log and, on convergence, will synthesize ONE final remediation comment and minimize the intermediate comments (this flush, and any further escalation flushes) at that point.

Persistence note: that working log is an untracked file living only in the persistent authoring worktree ../pair-worktrees/277. It must be preserved (not pruned or recreated) until this PR merges — pruning it loses the round-by-round audit trail (this flush and the first-review comment (5109259411) would still remain on the PR, and the first-review comment still prevents a duplicate first review on the next orchestrated run, but the detailed per-round record only lives in that worktree's log).

Not merging. Awaiting human decision on this escalation.

@rucka

rucka commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator Author

Remediation — round 6 (the new CLI code)

Open findings: 2, both deliberate and neither a defect — one deferred by design, one blocked on a maintainer decision. Plus one question that belongs to the merge gate. Details at the bottom.

Head: c945a920. Gate: 22 + 12 tasks green.

The five Majors

# Finding What changed
1 exclude was ignored on the non-transform copy path Real defect, and the most valuable of the six. A registry declaring exclude without flatten/prefix takes the plain path, which walks the tree itself and never consulted the option — it silently did nothing. Both paths now share one isExcluded, promoted next to isWithinPath as a path primitive. Threaded through handleDirectoryCopyperformDirectoryCopycopyDirectoryContentscopyDirHelper, and through the CLI's own buildCopyDirHelperContext; exclude/excludeRoot survive the recursive descent so entry paths resolve against the registry source at every depth.
2 exclude had no config validation, unlike include validateExcludes mirrors validateIncludes: a non-array and a non-string member are both config errors with the same message shape. Three cases added.
3 .codex/agents/*.toml did not belong here, and their paths pointed at a non-existent .Codex/workflows/ Dropped from the branch. They are a mechanical copy of the Claude agent definitions whose path strings were case-replaced along with the tool name, so they referenced a file that does not exist.
4 The skill count was corrected to 40 and the real number is 41 Fixed in all three places, and — more usefully — the two phrasings that let it drift are now pinned: SKILL_COUNT_RE covers N declared pair skills, and a new SKILL_COUNT_PROBE_RE covers the quoted Skills (N) transcript. Four new cases, including one asserting the sibling Agents (0) / Hooks (0) counts in the same transcript are never read as skill counts. runAllChecks passes against the live docs tree, so 41 is verified, not asserted.
5 The ADL had no decision for the exclude mechanism Decision 6 added: the mechanism, the single-channel motivation and its relation to #392, plus each property that could have gone wrong (segment-wise matching, filtered before validation and mkdir, one predicate shared by both paths, malformed input is a config error) and the recorded limit — exclude means never install, not uninstall, because behavior: overwrite performs no cleanup.

The Minors

  • Docs (6): configuration.mdx gains the exclude row plus the segment-wise semantics and the no-cleanup limit.
  • Description (7): Files Changed rewritten to the real 26-file shape (the CLI pipeline was missing entirely), and Change/diff risk re-assessed in place — no longer docs+manifests only. Still yellow, with the reason stated.
  • Swept formatter changes (8): the two version-check*.test.ts reformats are dropped. They are main's to fix and [US-394] fix: pre-push gate checks formatting instead of applying it (a write-mode formatter cannot fix the commits being pushed) #412 already carries them; code-host-routing.test.ts needed nothing — it is byte-identical to origin/main.
  • ADL counts (9): Decision 3 is now count-free, and the two claude plugin details transcripts are marked as observed-at-the-time, with a note that the integration page carries the same transcript pinned to the live count instead — so the two numbers may legitimately differ.
  • Verifiability (13, Question): closed by construction. A new smoke scenario (registry-exclude.sh) drives the real packaged CLI over a real source tree: the transform path, the plain path, and the invalid-config case. Writing it earned its keep twice — it caught that --config merges over the built-in config (a skills entry would have inherited the default's flatten+prefix and tested nothing), and its plain-path assertions failed until the CLI-side wiring was actually in the bundle, which makes them non-vacuous.

Still open, and why

Also removed: a stray config.json at the repo root, a leftover of a manual CLI reproduction that a git add -A of mine swept into the branch. It shadowed the CLI's own config and was never part of the story.

rucka and others added 12 commits July 31, 2026 21:45
…d list from schema

Third hand-edited surface guarded: marketplace.json's own top level (kind
'marketplace') — rejects forceRemoveDeletedPlugins, allowCrossMarketplaceDependenciesOn
and metadata.pluginRoot (nested check, since metadata stays allowed for its
descriptive fields). All three pass `claude plugin validate .` unguarded.

ROOT_PLUGIN_COMPONENT_PATHS now DERIVED from SCHEMA_COMPONENT_KEYS + '.mcp.json'
instead of hand-enumerated: the hand list lagged the schema (monitors missing —
unsandboxed background scripts, hooks' trust tier). Manifest half and payload half
of the rule now extend from one edit.

Test-comment fixes: --strict FAILS (not warns); test title no longer cites bare
D23/R9.3 (ADL Decision 5 is the rule of record). 126 tests (was 112).
Both release checklists updated to three surfaces + derived root list.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…channel in README

claude-code.mdx: "loads skills only" (declares none + no root auto-discovery dirs),
with an explicit paragraph that every repo file IS copied into the cache
(.claude/agents/, agent-browser) so non-declaration is a LOADING control; "stays
read-only" replaced with "disposable, not storage" — it contradicted the
record-decision write-loss callout 25 lines below.

README: native marketplace install path at the front door (pick one channel;
adoption-dependent skills need the CLI), 38 -> 40 skills.

ADL Decision 5: three surfaces + derived root-payload list.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…-only setup skill

Scope widened deliberately (agreed with the maintainer): the exclusion is the
enabler for #392's setup-assistant channel, so it lands where it is used rather
than as a standalone mechanism.

Test-first: four cases written and verified RED before any implementation.

A registry may now declare `exclude: [...]` — source-relative entries the copy
never installs. Threaded config -> RegistryConfig -> SyncOptions -> the copy
pipeline, matching how `flatten`/`prefix` already travel.

Why it exists: the marketplace channel will ship a setup skill that must exist
ONLY when installed from the marketplace. Without the exclusion the CLI also
writes it into the project, and the same skill name then answers from two trees
(`~/.claude/plugins/cache` and the project's `.claude/skills/`) whose resolution
order this repo's own ADL records as never observed — verified only with the
plugin as sole source. That is an execution ambiguity, not a cosmetic duplicate.

Design points, each with a test:
- Matching is PER PATH SEGMENT, never a string prefix: excluding `process/setup`
  must not silently drop `process/setup-helper`. That is the classic shape of
  this bug and it has its own test.
- An excluded entry's WHOLE SUBTREE is skipped, so a `references/` sub-doc does
  not survive its skill.
- Filtered before validation and before any `mkdir`, so an excluded entry cannot
  contribute a flatten collision, a directory mapping or a link-rewrite pass —
  as if it were never in the source.
- Absent ⇒ exact previous behaviour, asserted, so no other registry moves.
- `buildCopyOptions` reads it defensively: it is called with hand-built
  RegistryConfig objects too, not only with `extractRegistries` output — four
  existing tests proved that the moment a required field was assumed.
- `extractRegistries` ignores a non-array value rather than trusting it.

KNOWN LIMIT, in the code and in #392: this prevents WRITING an entry, not having
it. The skills registry runs `behavior: overwrite`, which performs no cleanup, so
an excluded entry already present in a target is left untouched.

`collectInstallableFiles` extracted to keep `copyDirectoryWithTransforms` under
the 50-line ceiling — caught by the gate, not by the tests.

Also: the catalog guard flagged `pair-process-brainstorm` missing from
`plugin.json` after #387 merged. Added — 41 entries. Exactly what that guard
exists for, and the first time it has fired on a real omission.

`pnpm quality-gate` green. content-ops 22 copy cases, pair-cli 190 registry.

Refs #277 · #392

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…t it drift

- README + claude-code.mdx said 40; the dataset holds 41.
- SKILL_COUNT_RE now covers 'N declared pair skills'; new SKILL_COUNT_PROBE_RE
  pins the quoted 'Skills (N)' transcript. Both were unguarded phrasings.
- Drop .codex/agents/*.toml (harness story, and their paths point at a
  non-existent .Codex/workflows/) and two prettier-only sweeps.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…cision 6

- The plain (no flatten/prefix) copy path walked the tree itself and never
  consulted exclude, so the option was a silent no-op for any registry that
  declares it without transforms. Both paths now share one isExcluded.
- validateExcludes: a non-array or non-string member is a config error, like include.
- New smoke scenario drives the real CLI over both paths + the invalid case;
  assert_no_file added to the smoke lib.
- ADL Decision 6 records the mechanism, the #392 motivation and the no-cleanup limit;
  Decision 3 is now count-free and the probe transcripts are marked as observed-then.
- configuration.mdx documents exclude, its segment-wise match and the limit.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…iling

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
config.json at the repo root was a leftover of a manual CLI reproduction, picked up
by a git add -A. It is not part of the story and shadows the CLI's own config.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… catalog

Scope revision agreed with the maintainer. The plugin no longer ships the whole
repository: marketplace.json's entry sources dataset/plugin/, whose .claude-plugin/
plugin.json declares one authored, ISOLATED skill — pair-capability-setup-cli —
which installs pair-cli and runs it, so the catalog and KB are produced by the CLI
in the user's repo.

Why: the whole-repo payload put pair's own .pair/ in every plugin cache, so a skill
read pair's adoption decisions as the project's, and record-decision wrote into a
cache the next /plugin update erased. Both were documented as known limitations.
They are now structurally absent — verified, not argued: the installed cache holds
.claude-plugin/ + skills/ only, 12K, and 'claude plugin details' reports Skills (1).

Also caught by probing: Claude Code wants the manifest at
<root>/.claude-plugin/plugin.json, not <root>/plugin.json. The first layout would
have shipped an uninstallable plugin; no repo guard would have caught it.

- Catalog guard derives the expected list from the bootstrap corpus on disk.
- New assertBootstrapSkillsValid replaces skills:conformance for that corpus,
  asserting frontmatter and the isolation invariant on LINK TARGETS (the skill
  legitimately explains the rule in prose).
- skills/ is exempt from the root-payload check at the new root, where it IS the
  declared payload; every other component key stays forbidden.
- Skills (N) transcripts are pinned to the DECLARED count, not the dataset's.
- 'Pick one channel' retired: no skill name resolves from two sources.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A 'git add -A' during conflict resolution staged operations.ts with markers still
in it. Both hunks are wanted: flattenDepth (from #411, now on main) and exclude.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@rucka
rucka force-pushed the feature/US-277-skill-marketplace-plugin-packaging branch from 81ed374 to e679012 Compare August 1, 2026 08:14
@github-actions
github-actions Bot temporarily deployed to Website Preview August 1, 2026 08:15 Inactive
@rucka

rucka commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator Author

Scope revision, round 7 — the payload changed, so this PR needs a fresh review

Open findings: 0 from round 6. But this is not a remediation report: the architecture under review changed, so rounds 1–6 were conducted against a shape this PR no longer has. Treat it as new.

Head e679012c, rebased onto main (9bc47444, which now carries #404 and #411). Gate: 22 + 12 tasks green. 26 files, +2225/−38.

What changed

The plugin no longer ships the catalog. marketplace.json stays at the repo root, but its one entry's source points at packages/knowledge-hub/dataset/plugin, which is therefore the plugin root — holding .claude-plugin/plugin.json beside the skills/ it declares, with a single authored, isolated skill: pair-capability-setup-cli. That skill installs pair-cli and runs it, so the knowledge base and the full catalog are produced by the CLI in the user's repository. The repo-root plugin.json is deleted.

The reason is not aesthetic. The whole-repo payload put pair's own .pair/ inside every plugin cache, which produced the two "known limitations" this PR previously shipped as documentation: a skill step read pair's adoption decisions as if they were the user's (no graceful degradation — from the skill's point of view the files were present), and /pair-capability-record-decision wrote adoption files into a cache the next /plugin update erased. Both are now structurally absent.

Verified by installing it, not by argument

CLI v2.1.220, sandboxed CLAUDE_CONFIG_DIR, against this branch:

  • claude plugin details pair@pairSkills (1) pair-capability-setup-cli, Agents (0), Hooks (0), MCP servers (0), LSP servers (0)
  • Installed payload 12 KB (du -sh on the cache) against ~19 MB before; always-on token cost ~124
  • The cache holds .claude-plugin/ and skills/ and no .pair/ — walked to confirm
  • claude plugin validate passes at the plugin root and at the repo root, the latter resolving the entry's manifest through the new source

A layout fact the probe caught and no repo guard would have: Claude Code expects the manifest at <plugin root>/.claude-plugin/plugin.json, not <plugin root>/plugin.json. The first attempt at this revision had the bare form and would have shipped an uninstallable plugin.

Guards rewritten

Guard Before Now
Catalog sync expected list derived from dataset/.skills through the install transform derived from the bootstrap corpus on disk; adding a distributed skill touches no manifest
Authoring validity skills:conformance + mirror-equality (both scoped to dataset/.skills) assertBootstrapSkillsValid — frontmatter, plus the isolation invariant asserted on link targets, not substrings: the skill legitimately explains the rule in prose, and a substring match would have reddened on that sentence
Root payload skills/ forbidden at the plugin root (it would auto-load undeclared skills) skills/ exempt at the new root, where it IS the declared payload and auto-discovery agrees with the manifest by construction; every other component key still forbidden
Doc counts Skills (N) pinned to the dataset count pinned to the count the manifest declares — 1 vs 41, and conflating them demanded a number wrong on both readings

What a reviewer should look at

  1. The exemption of skills/ from the root-payload check. It is the one place where a rule that existed for a good reason was relaxed. The argument is that the payload is now entirely declared, so auto-discovery and the manifest cannot disagree — find a layout where that is false.
  2. assertBootstrapSkillsValid as a replacement for two guards. It covers frontmatter and isolation. What did the two guards it replaces cover that it does not?
  3. The isolation invariant's regex. It matches markdown link targets. A skill could still reach the KB by a path in a code fence, or by telling the agent to read a file.
  4. exclude now has no consumer. Kept on the maintainer's explicit call; its original motivation (keeping the setup skill out of consuming projects) is done structurally by the authoring location instead. The ADL says so plainly. Whether shipping it is right is a merge-gate question, not a defect.
  5. The bootstrap skill's own content — it is prose an agent executes, and nothing type-checks it. The package name (@foomakers/pair-cli) was verified against the registry; @pair/pair-cli, the workspace name, 404s.

Also in this push

Measured: the installed plugin cache is 12 KB, and with a local-path source the
marketplace dir is 0 B. From a GitHub source the repo IS cloned there to read the
manifest and resolve the nested source — so the 12 KB figure is the payload, not a
claim that nothing else is fetched. 'marketplace add --sparse' exists for this
monorepo shape and is now documented in the install instructions.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions
github-actions Bot temporarily deployed to Website Preview August 1, 2026 12:18 Inactive
rucka and others added 2 commits August 1, 2026 14:27
…ir-assistant

Maintainer call: the skill should set pair up when missing AND stay useful
afterwards. Renamed pair-capability-setup-cli -> pair-assistant (the meta-skill
shape, like pair-next: it is an entry point, not a category member), decided before
merge because the name is public and changing it later breaks invocations.

Two runtime sources of truth, neither inlined:
- the CLI surface from 'pair-cli --help' / '<cmd> --help'. The repo already holds
  two copies of that surface (code + published reference, pinned by docs:staleness);
  a third inside the plugin would have no guard, and the version installed in a
  user's project may differ from the one this file was written against.
- the project's standards from its own .pair/llms.txt, read from the CWD.

The isolation rule is now stated as LINK vs READ: a relative link out of the payload
resolves inside the plugin cache (the defect this design removes) and stays banned;
a runtime read of the project's own KB is the useful half and is explicitly allowed.
The guard already matched link targets, so it needed no change — only the docstring
saying why the distinction exists.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions
github-actions Bot temporarily deployed to Website Preview August 1, 2026 12:29 Inactive
…context can be wrong

Two gaps the maintainer pointed at.

1. Running on an EXISTING project was not a distinct case. It is: the value of
   pair's adoption files there is that they describe the project as it is, so
   installing blank templates silently is the wrong default. The preflight now
   classifies greenfield vs brownfield and, for brownfield, states the fact that
   makes the choice safe (the adoption registry installs with 'add' behaviour, so
   existing decisions are never overwritten) and ASKS: install only, or install and
   adopt — the second handing off to /pair-process-bootstrap, which owns the
   checklist and arrives with the install. No answer -> install only.

2. Where the context comes from is a real concern, so the weak spots are written
   down: .pair/llms.txt has NO gate against the KB it indexes (issue #416, filed
   from #411's review), so it is treated as a table of contents — an unresolvable
   path means walk .pair/knowledge/ and report the index stale, never guess. Plus
   --help is silent about conventions, and this file can be older than both the CLI
   and the KB, which win over it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions
github-actions Bot temporarily deployed to Website Preview August 1, 2026 12:35 Inactive
… with the adoption precondition

Maintainer requirement: it must be able to run the CLI AND invoke the other skills
inside the pair installation.

- Step 6: it delegates to the catalogue by bare /pair-… name, discovering what the
  project has with 'ls .claude/skills/' rather than a list written here — the same
  anti-inlining rule the CLI flags follow. The dispatch is unambiguous by packaging,
  not by luck: this assistant exists only in the plugin, the catalogue only in the
  project, so every bare name has exactly one source.
- The precondition that makes delegation safe: right after install, .pair/adoption/
  holds what pair SHIPPED, including concrete choices of its own. Those are defaults,
  not the project's decisions. So before invoking an adoption-dependent skill, check
  whether the adoption pass was actually done and offer /pair-process-bootstrap if
  not — otherwise the skill acts on a decision the project never made, which is the
  failure this channel was redesigned to remove, arriving by another route.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions
github-actions Bot temporarily deployed to Website Preview August 1, 2026 12:41 Inactive
The plugin/marketplace format is Claude Code's own (the schemas are named
claude-code-*), while the SKILL.md format is portable — so the content travels to six
tool directories via the CLI, but this delivery channel does not. /pair-assistant
therefore reaches Claude Code users only; others install with the CLI directly, which
is coherent since they are already at a command line. A catalogue-member assistant for
the other tools was considered and deferred (maintainer call).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions
github-actions Bot temporarily deployed to Website Preview August 1, 2026 12:46 Inactive
rucka and others added 3 commits August 1, 2026 14:52
…ve misrouted users

I wrote that quick 'takes pair's defaults for things the project already decided',
and told the assistant to avoid it on an existing project. That contradicts what
bootstrap documents: both depths resolve each value through the SAME cascade
(explicit argument > project state > recorded decision > KB fallback), and the tier
table assigns 'project state' to nearly every decision — stack, test runner, AI
tooling, categorization. Quick means 'does not stop to confirm', not 'guesses'.

So on an existing project quick is now the suggestion when the user wants it done,
described honestly, with the three sections that genuinely have no project-state tier
(architecture, infrastructure, observability) named up front so the user knows which
values arrived without evidence.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… three sections

Maintainer call: land it here rather than as its own issue.

The guided/quick convention fixes one cascade for EVERY decision point — explicit
argument > project state > recorded decision > hardcoded fallback — and says the two
depths differ only in whether the resolved value is confirmed, never in where it comes
from. bootstrap's tier table honoured that for the stack, the test runner, the AI
tooling and the categorization, but sent three straight to 'fallback':

  - architecture (core pattern, style, data)
  - infrastructure
  - observability

So on a repository that already had a Dockerfile, a terraform/ dir, platform config or
an error-tracking dependency, quick wrote the KB default with the evidence in plain
sight. Those rows are now 'project state + fallback', naming what to read, and the
checklist asset says a default is for silence rather than a shortcut past looking.

The mirror guard earned its keep twice here: it caught that a same-dir link needs './'
in the root copy (the pipeline prepends it), which an identical hand-edit got wrong.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…the dataset

The two-step remedy DEVELOPMENT.md describes, in practice: prettier normalised emphasis
(*does* -> _does_) and a blank line in the dataset copies, which are inside format scope,
while their generated twins under .pair/ and .claude/ are not. Left alone, the mirror
guard would have gone red on the next run. The legitimate transform differences (./ on
same-dir links, /pair-* skill references) are untouched.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk:yellow Classification: medium risk tier

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant