← back to Cli Printing Press
feat(cli,skills): polish ledger per-item enforcement + AGENTS.md guidance (#389)
0320269d9e4211aaa3c3996e41f6407d0fd8a2bf · 2026-04-29 10:30:31 -0700 · Trevin Chow
* feat(cli,skills): polish ledger per-item enforcement + AGENTS.md ledger pattern guidance
Adds four enforcement primitives to the tools-polish ledger so a polish
run cannot complete via boilerplate. The cal-com test (Apr 2026)
accepted 246 thin-mcp-description findings with rationales like
"systemic to OpenAPI specs" — the binary's "zero pending" gate was
satisfied while the agent had pattern-matched across the entire list
with one excuse. Ports the per-item discipline from
/simplify-and-refactor-code-isomorphically (Isomorphism Card filled
before each edit) and /library-updater (per-package log + crash-recovery
checkpoint) into the polish ledger.
Binary changes (internal/cli/tools_audit.go):
- Pre-decision fields on ToolsAuditFinding (spec_source_material,
target_description, gap_analysis), required when accepting
thin-mcp-description / empty-mcp-description findings
- Duplicate-rationale gate: clusters accepted notes by normalized
text; >5 in one cluster surfaces as incomplete
- Scorecard-delta gate: ScorecardBefore captured on first run, sticky
across re-runs; if MCPDescriptionQuality didn't move and any
thin-mcp accepts exist, run is incomplete
- Resume protocol: PolishProgress.LastProcessedFindingID + next:
hint in the render so post-compaction re-invocation picks up
mid-walk
- ScoreMCPDescriptionQuality exported from pipeline so the audit
binary can compute current vs. before for the delta gate
Skill prose (skills/printing-press-polish/):
- SKILL.md Priority 6 now states the stricter accept contract for
MCP-description findings and the four-gate completion criteria
- references/tools-polish.md rewrites "Marking a finding accepted"
with the pre-decision fields contract, adds a "Completion gates"
section, updates the verification checklist to require zero
pending plus zero gate failures
AGENTS.md:
- Restructures "Deterministic Inventory + Agent-Marked Ledger"
section with the four enforcement primitives so future skills
using this pattern get them by default. Explicitly cites
simplify-and-refactor and library-updater as the sources of
truth for the per-item discipline.
Manual verification on ~/printing-press/library/cal-com confirms all
three gates fire as designed against the existing 204-finding
bulk-accept ledger; the resume hint surfaces the next pending finding.
35 new unit tests cover normalization, clustering, scorecard-delta
edge cases, and resume protocol checkpoint behavior.
Closes #383
* refactor(cli): simplify polish ledger code + add code/comment hygiene to AGENTS.md
Cleanup pass over the polish ledger enforcement code added in 860efd4e,
plus a new "Code & Comment Hygiene" section in AGENTS.md codifying the
write-time defaults and review-time patterns the cleanup keeps applying.
Code changes:
- Read tools-manifest.json once per audit run instead of three times.
Adds pipeline.ScoreMCPDescriptionQualityForManifest for callers that
already have the parsed manifest; the existing dir-based wrapper
delegates to it.
- Hoist finding kinds (empty-short, thin-short, missing-read-only,
empty-mcp-description, thin-mcp-description) to typed constants;
collapse single-case switch in requiresPreDecisionFields to ||.
- Extract isPending(f) and firstPending(findings, lo, hi) so the
pending-or-incomplete predicate has one source of truth. Fixes a
render-counter bug where accepted-but-incomplete entries counted as
accepted in the summary line — they now correctly count as pending
and surface the run's incompleteness more visibly.
- Delete duplicate truncateRationale; reuse the existing truncate from
mcp_audit.go after making it UTF-8-safe via rune slicing.
- Single-source hasGateFailure/gateFailureCount.
- Trim narrating comments: speculative future-proofing on
ScorecardSnapshot, historical incident references in
requiresPreDecisionFields, "four gates" misnomer (it's three plus a
resume hint), and a brittle reference link inside the
ToolsAuditFinding struct comment that depended on a skill doc path
that may be reorganized.
AGENTS.md:
- Adds "Code & Comment Hygiene" section between "Skill Authoring:
Reference File Pattern" and "Deterministic Inventory + Agent-Marked
Ledger." Splits guidance honestly into write-time defaults (rules
the agent can apply from a single instance) and review-time patterns
(rules that need a diff view). The split prevents prescribing rules
the agent can't follow at write time, e.g., "extract a helper at
N=2 call sites" when only one site is being authored.
All 2300 tests pass; manual verification on cal-com confirms the three
gates plus the resume hint render correctly with the bug-fixed pending
count.
* docs(cli): tighten AGENTS.md terminology and local/public library split
Eight clarity improvements surfaced while reviewing the doc against
recent work:
Glossary additions and clarifications:
- Default disambiguation conventions for the four overloaded terms
(library, publish, manifest, catalog) — body prose can now use the
bare term and readers know what it means.
- Glossary entries for `manifest.json` (Claude plugin manifest) and
`tools-manifest.json` (MCP tool catalog), each cross-referencing
the others. Three different `manifest.json` files exist in a
printed CLI directory and only one was glossaried.
- `catalog` entry retitled "catalog (embedded)" with explicit
contrast against the public library's category-organized catalog.
- Manuscript glossary entry now cross-references the local library
to make "working copy vs immutable archive" relationship explicit.
Section restructure:
- Renamed "`~/printing-press/` Layout" to "Local Artifacts
(`~/printing-press/`)" so the section heading flags it as the
local half of a local-vs-public dichotomy.
- New "Public Library" section after Local Artifacts, covering the
public library repo, the local→public flow via the publish skill,
and the two flavors of local-vs-public divergence (expected:
publish-rewritten files like `go.mod`; unexpected: forgotten
pushes).
The local-vs-public divergence concept was previously implicit —
mentioned via the polish divergence check and the publish glossary
entries but never structurally explained.
* docs(cli): trim AGENTS.md to 321 lines (from 430)
Five cuts focused on extractions and compression of sections that don't
need to be loaded on every interaction:
Extracted to dedicated reference docs:
- Skill Workflow Parity, Skill Authoring, Skill Frontmatter
→ docs/SKILLS.md. AGENTS.md keeps a one-paragraph "Skill Authoring"
pointer that names the doc and what's in it. Skill content only
matters when editing skills.
- Deterministic Inventory + Agent-Marked Ledger → docs/PATTERNS.md.
AGENTS.md keeps a one-paragraph "Patterns" pointer naming the
pattern and its four enforcement primitives. Pattern detail only
matters when designing a workflow that uses the shape.
Compressed inline:
- Machine vs Printed CLI: paragraph-form rules collapsed to a tight
bullet list; Anti-Reimplementation subsection trimmed to its three
reject categories and three carve-outs without the surrounding
rationale prose.
- Agent-Native Surface: the three skip rules and store-population
carveout retained; "Adding a new framework command" and "Why
agent-facing != user-facing" subsections collapsed into the
default-expose principle they restated.
- Versioning + Release Process merged into "Versioning & Release"
with the automated flow at the top and the version-file list below.
Result: AGENTS.md drops from 430 to 321 lines while preserving every
operational rule. The extracted content lives next to the existing
docs/PIPELINE.md so the agent loads it only when it matters.
Files touched
M AGENTS.mdA docs/PATTERNS.mdA docs/SKILLS.mdM internal/cli/mcp_audit.goM internal/cli/tools_audit.goA internal/cli/tools_audit_test.goM internal/pipeline/scorecard.goM skills/printing-press-polish/SKILL.mdM skills/printing-press-polish/references/tools-polish.md
Diff
commit 0320269d9e4211aaa3c3996e41f6407d0fd8a2bf
Author: Trevin Chow <trevin@trevinchow.com>
Date: Wed Apr 29 10:30:31 2026 -0700
feat(cli,skills): polish ledger per-item enforcement + AGENTS.md guidance (#389)
* feat(cli,skills): polish ledger per-item enforcement + AGENTS.md ledger pattern guidance
Adds four enforcement primitives to the tools-polish ledger so a polish
run cannot complete via boilerplate. The cal-com test (Apr 2026)
accepted 246 thin-mcp-description findings with rationales like
"systemic to OpenAPI specs" — the binary's "zero pending" gate was
satisfied while the agent had pattern-matched across the entire list
with one excuse. Ports the per-item discipline from
/simplify-and-refactor-code-isomorphically (Isomorphism Card filled
before each edit) and /library-updater (per-package log + crash-recovery
checkpoint) into the polish ledger.
Binary changes (internal/cli/tools_audit.go):
- Pre-decision fields on ToolsAuditFinding (spec_source_material,
target_description, gap_analysis), required when accepting
thin-mcp-description / empty-mcp-description findings
- Duplicate-rationale gate: clusters accepted notes by normalized
text; >5 in one cluster surfaces as incomplete
- Scorecard-delta gate: ScorecardBefore captured on first run, sticky
across re-runs; if MCPDescriptionQuality didn't move and any
thin-mcp accepts exist, run is incomplete
- Resume protocol: PolishProgress.LastProcessedFindingID + next:
hint in the render so post-compaction re-invocation picks up
mid-walk
- ScoreMCPDescriptionQuality exported from pipeline so the audit
binary can compute current vs. before for the delta gate
Skill prose (skills/printing-press-polish/):
- SKILL.md Priority 6 now states the stricter accept contract for
MCP-description findings and the four-gate completion criteria
- references/tools-polish.md rewrites "Marking a finding accepted"
with the pre-decision fields contract, adds a "Completion gates"
section, updates the verification checklist to require zero
pending plus zero gate failures
AGENTS.md:
- Restructures "Deterministic Inventory + Agent-Marked Ledger"
section with the four enforcement primitives so future skills
using this pattern get them by default. Explicitly cites
simplify-and-refactor and library-updater as the sources of
truth for the per-item discipline.
Manual verification on ~/printing-press/library/cal-com confirms all
three gates fire as designed against the existing 204-finding
bulk-accept ledger; the resume hint surfaces the next pending finding.
35 new unit tests cover normalization, clustering, scorecard-delta
edge cases, and resume protocol checkpoint behavior.
Closes #383
* refactor(cli): simplify polish ledger code + add code/comment hygiene to AGENTS.md
Cleanup pass over the polish ledger enforcement code added in 860efd4e,
plus a new "Code & Comment Hygiene" section in AGENTS.md codifying the
write-time defaults and review-time patterns the cleanup keeps applying.
Code changes:
- Read tools-manifest.json once per audit run instead of three times.
Adds pipeline.ScoreMCPDescriptionQualityForManifest for callers that
already have the parsed manifest; the existing dir-based wrapper
delegates to it.
- Hoist finding kinds (empty-short, thin-short, missing-read-only,
empty-mcp-description, thin-mcp-description) to typed constants;
collapse single-case switch in requiresPreDecisionFields to ||.
- Extract isPending(f) and firstPending(findings, lo, hi) so the
pending-or-incomplete predicate has one source of truth. Fixes a
render-counter bug where accepted-but-incomplete entries counted as
accepted in the summary line — they now correctly count as pending
and surface the run's incompleteness more visibly.
- Delete duplicate truncateRationale; reuse the existing truncate from
mcp_audit.go after making it UTF-8-safe via rune slicing.
- Single-source hasGateFailure/gateFailureCount.
- Trim narrating comments: speculative future-proofing on
ScorecardSnapshot, historical incident references in
requiresPreDecisionFields, "four gates" misnomer (it's three plus a
resume hint), and a brittle reference link inside the
ToolsAuditFinding struct comment that depended on a skill doc path
that may be reorganized.
AGENTS.md:
- Adds "Code & Comment Hygiene" section between "Skill Authoring:
Reference File Pattern" and "Deterministic Inventory + Agent-Marked
Ledger." Splits guidance honestly into write-time defaults (rules
the agent can apply from a single instance) and review-time patterns
(rules that need a diff view). The split prevents prescribing rules
the agent can't follow at write time, e.g., "extract a helper at
N=2 call sites" when only one site is being authored.
All 2300 tests pass; manual verification on cal-com confirms the three
gates plus the resume hint render correctly with the bug-fixed pending
count.
* docs(cli): tighten AGENTS.md terminology and local/public library split
Eight clarity improvements surfaced while reviewing the doc against
recent work:
Glossary additions and clarifications:
- Default disambiguation conventions for the four overloaded terms
(library, publish, manifest, catalog) — body prose can now use the
bare term and readers know what it means.
- Glossary entries for `manifest.json` (Claude plugin manifest) and
`tools-manifest.json` (MCP tool catalog), each cross-referencing
the others. Three different `manifest.json` files exist in a
printed CLI directory and only one was glossaried.
- `catalog` entry retitled "catalog (embedded)" with explicit
contrast against the public library's category-organized catalog.
- Manuscript glossary entry now cross-references the local library
to make "working copy vs immutable archive" relationship explicit.
Section restructure:
- Renamed "`~/printing-press/` Layout" to "Local Artifacts
(`~/printing-press/`)" so the section heading flags it as the
local half of a local-vs-public dichotomy.
- New "Public Library" section after Local Artifacts, covering the
public library repo, the local→public flow via the publish skill,
and the two flavors of local-vs-public divergence (expected:
publish-rewritten files like `go.mod`; unexpected: forgotten
pushes).
The local-vs-public divergence concept was previously implicit —
mentioned via the polish divergence check and the publish glossary
entries but never structurally explained.
* docs(cli): trim AGENTS.md to 321 lines (from 430)
Five cuts focused on extractions and compression of sections that don't
need to be loaded on every interaction:
Extracted to dedicated reference docs:
- Skill Workflow Parity, Skill Authoring, Skill Frontmatter
→ docs/SKILLS.md. AGENTS.md keeps a one-paragraph "Skill Authoring"
pointer that names the doc and what's in it. Skill content only
matters when editing skills.
- Deterministic Inventory + Agent-Marked Ledger → docs/PATTERNS.md.
AGENTS.md keeps a one-paragraph "Patterns" pointer naming the
pattern and its four enforcement primitives. Pattern detail only
matters when designing a workflow that uses the shape.
Compressed inline:
- Machine vs Printed CLI: paragraph-form rules collapsed to a tight
bullet list; Anti-Reimplementation subsection trimmed to its three
reject categories and three carve-outs without the surrounding
rationale prose.
- Agent-Native Surface: the three skip rules and store-population
carveout retained; "Adding a new framework command" and "Why
agent-facing != user-facing" subsections collapsed into the
default-expose principle they restated.
- Versioning + Release Process merged into "Versioning & Release"
with the automated flow at the top and the version-file list below.
Result: AGENTS.md drops from 430 to 321 lines while preserving every
operational rule. The extracted content lives next to the existing
docs/PIPELINE.md so the agent loads it only when it matters.
---
AGENTS.md | 226 +++++-----
docs/PATTERNS.md | 43 ++
docs/SKILLS.md | 56 +++
internal/cli/mcp_audit.go | 10 +-
internal/cli/tools_audit.go | 426 +++++++++++++++++--
internal/cli/tools_audit_test.go | 461 +++++++++++++++++++++
internal/pipeline/scorecard.go | 48 ++-
skills/printing-press-polish/SKILL.md | 3 +-
.../references/tools-polish.md | 63 ++-
9 files changed, 1134 insertions(+), 202 deletions(-)
diff --git a/AGENTS.md b/AGENTS.md
index 473fbf48..9468fc14 100644
--- a/AGENTS.md
+++ b/AGENTS.md
@@ -1,88 +1,67 @@
# CLI Printing Press - Development Conventions
-## Machine vs Printed CLI — What Are You Optimizing?
+## Machine vs Printed CLI
-This repo contains **the machine** (generator, templates, binary, skills) that produces **printed CLIs**. When fixing bugs or adding features, always ask: is this a machine change or a printed CLI change?
+This repo is **the machine** (generator, templates, binary, skills) that produces **printed CLIs**. When fixing a bug or adding a feature, ask: machine change or printed-CLI change?
-- **Machine changes** (generator, templates, parser, skills) affect every future CLI. They must be generalized — think about how the fix applies across different APIs, spec formats, and auth patterns, not just the CLI you're looking at right now.
-- **Printed CLI changes** (code in `~/printing-press/library/<cli>/`) fix one specific CLI. These are fine for targeted improvements but don't compound.
+- **Machine changes** (generator, templates, parser, skills) affect every future CLI; they must generalize across APIs, spec formats, and auth patterns.
+- **Printed-CLI changes** (`~/printing-press/library/<api-slug>/`) fix one CLI; they don't compound.
-**Default to machine changes.** If a problem shows up in a printed CLI, the first question is: should the generator have gotten this right? If yes, fix the machine so every future CLI benefits. Only fix the printed CLI directly when the issue is genuinely specific to that one API.
+Rules:
-**Never change the machine for one CLI's edge case** unless explicitly told to. If a fix only helps Pagliacci but would be wrong for Stripe, it doesn't belong in the generator. Add a conditional with a clear guard, or leave it as a printed-CLI-level fix.
+- **Default to machine changes.** If a problem appears in a printed CLI, ask first whether the generator should have gotten this right. Only fix the printed CLI directly when the issue is genuinely API-specific.
+- **Don't change the machine for one CLI's edge case.** A fix that helps one API but breaks another doesn't belong in the generator — guard it with a clear conditional or leave it as a printed-CLI fix.
+- **Don't hardcode API/site names in reusable artifacts.** Skills, templates, generator code, prompts, shared docs must use placeholders (`<api>`, `<site>`, "the target site") unless the text is explicitly an example or test fixture. Concrete names belong in `Example:` paragraphs, never in operational instructions.
+- **Update dependent verifiers in the same change.** A new generator capability that affects scoring requires a scorer update; one that changes the MCP surface requires an audit update. Forgetting either half ships a CLI whose advertised contract diverges from what's emitted.
-**Do not hardcode one API/site into reusable machine artifacts.** Skills, templates, generator code, prompts, and shared docs must use placeholders or generic phrasing (`<api>`, `<site>`, "the target site") unless the text is explicitly labeled as an example or test fixture. A Product Hunt, Pagliacci, Stripe, etc. name in reusable guidance is usually a bug: it leaks one investigation into every future printed CLI. If a concrete example is useful, put it in an "Example:" paragraph and keep the operational instruction generic.
+When iterating on a printed CLI to discover issues, label findings as systemic (retro candidate) vs specific (printed-CLI fix). The retro → plan → implement loop feeds discoveries back into the machine.
-**When iterating on a printed CLI to discover issues**, note which problems are systemic (retro findings) vs specific. The retro → plan → implement loop exists to feed discoveries from individual CLIs back into the machine.
+### Anti-reimplementation
-**When adding a capability that affects scoring**, update the scorer in the same change. The goal is not to inflate scores — it's to ensure the scorer accurately reflects the capability. If you add composed cookie auth but the scorer only recognizes Bearer/Basic, it will penalize a correctly-implemented CLI. Fix the scorer to recognize the new pattern, not to give it a free pass.
+A printed CLI wraps an API; it does not replace one. Novel-feature commands must call the real endpoint or read from the local store populated by sync. Reimplementations are worse than the API they pretend to replace.
-### Anti-Reimplementation
-
-A printed CLI wraps an API. It does not replace the API. Novel-feature commands must call the real endpoint, or read from the local SQLite store populated by sync. Anything in between is a reimplementation, and reimplementations are worse than the API they pretend to replace.
-
-Concretely, the generator and review loop reject:
+Reject:
- Hand-rolled response builders that return constants, hardcoded JSON, or struct literals shaped like an API payload
- Endpoint stubs that return `"OK"` or a canned success message without calling the client
- Aggregations computed in-process when the API has an aggregation endpoint
- Enum mappings and reference data synthesized locally when the API returns them
-Three carve-outs are legitimate:
+Carve-outs:
-- Commands that read from the generated `internal/store` package to join or query sync'd data (the `stale`, `bottleneck`, `health`, `reconcile` family). These are local-data commands, not fake API calls.
-- Commands that cache an API response in the store after calling it. Presence of both a client call and a store call is fine.
-- Commands whose data is the curated content itself — substitution tables, holiday lists, currency metadata, conversion factors. The data IS the feature; calling an API or hitting the store would be wrong. Opt in by adding the directive `// pp:novel-static-reference` anywhere in the command's source file (typically near the package-level data declaration). The reimplementation check exempts the command on the same footing as the store/client carve-outs.
+- Commands that read from `internal/store` (the `stale`, `bottleneck`, `health`, `reconcile` family) — local-data, not fake API calls
+- Commands that cache an API response in the store after calling it — both a client call and a store call is fine
+- Commands whose data is the curated content itself (substitution tables, holiday lists, currency metadata) — opt in via `// pp:novel-static-reference` directive in the command's source file
-The rule is enforced in two places. The absorb manifest has a Kill Check (see `skills/printing-press/references/absorb-scoring.md`) that rejects reimplementation candidates before they enter the feature list. Dogfood runs `reimplementation_check` over every built novel-feature command and flags any handler file that shows neither a client call nor a store access (and lacks the static-reference opt-out).
+Enforced by the absorb manifest's Kill Check (`skills/printing-press/references/absorb-scoring.md`) and dogfood's `reimplementation_check`, which flags handler files showing neither a client call nor a store access (without the static-reference opt-out).
## Agent-Native Surface
-Every printed CLI exposes two surfaces: the **CLI surface** that humans drive with shell commands, and the **MCP surface** that agents call as tools. Per the agent-native parity principle, any action a user can take should be reachable by an agent — but the surfaces are not identical. The CLI carries operator/human ergonomics that don't belong in an agent's tool catalog.
-
-### What belongs on the MCP surface
+Every printed CLI exposes two surfaces: a CLI surface for humans and an MCP surface for agents. Any action a user can take should be reachable by an agent, but the surfaces are not identical — operator ergonomics belong on the human-facing CLI, not in an agent's tool catalog.
-The runtime walker in `internal/mcp/cobratree/` mirrors the Cobra tree at server start. Default: every user-facing command becomes an MCP tool. The walker filters via three rules, in order:
+### Default: expose; skip rules are exceptions
-1. **Endpoint mirrors keep typed schemas.** A Cobra command annotated `cmd.Annotations["pp:endpoint"] = "<resource>.<endpoint>"` is registered as a typed MCP tool by the existing template (one per spec endpoint, schema derived from spec params). The walker skips these so they aren't shell-out duplicates.
-2. **Framework commands are excluded by name.** The `frameworkCommands` set in `cobratree/classify.go.tmpl` lists generator-emitted CLI commands the walker must skip. Two cases qualify:
- - A typed MCP tool already covers the capability (the typed schema is strictly better than a shell-out): `sql`, `search`, `about`/`agent-context` (covered by typed `context`), `api` (covered by typed endpoint tools).
- - The command is non-functional via MCP — interactive setup, shell ergonomics, trivial introspection, local-only state: `auth`, `completion`, `doctor`, `version`, `feedback`, `profile`, `which`, `help`.
-3. **Per-command opt-out via annotation.** Domain commands that should not be agent tools — interactive setup wizards, debug commands, anything that needs human-in-the-loop input — set `cmd.Annotations["mcp:hidden"] = "true"` at construction time.
+The runtime walker in `internal/mcp/cobratree/` mirrors the Cobra tree at server start, registering every user-facing command as an MCP tool. It skips a command only if one of these applies:
-**Critical: store-population commands stay exposed.** `sync`, `stale`, `orphans`, `reconcile`, `load`, `export`, `import`, `workflow`, `analytics` are generator-emitted but they have real agent value, so they MUST be reachable as MCP tools. The walker registers them as shell-out tools by default (no entry in the framework set means "the runtime walker exposes it").
+1. **Endpoint mirrors keep typed schemas.** Commands annotated `cmd.Annotations["pp:endpoint"] = "<resource>.<endpoint>"` are already registered as typed tools elsewhere; the walker skips to avoid duplicates.
+2. **Framework commands.** Listed by name in `cobratree/classify.go.tmpl`'s `frameworkCommands` set. Two reasons qualify a command for that set: a typed equivalent is strictly better (`sql`, `search`, `context`), or the command is non-functional via MCP (interactive setup, version reporting, local-only state — `auth`, `completion`, `doctor`, `version`, `feedback`, `profile`, `which`, `help`).
+3. **Per-command opt-out.** `cmd.Annotations["mcp:hidden"] = "true"` for domain commands that need human-in-the-loop input.
-Excluding `sync` while exposing the typed `sql` tool is a **broken contract** — `sql` returns empty results until something populates the store, and the only thing that does is `sync`. The same logic applies to `search` (FTS5 over the store): without `sync`, it's inert. Earlier framework-set drafts incorrectly excluded all of these as "operator commands"; the agent surfaced the bug by trying to query an empty database. The corrected rule, encoded above, is "framework-skip is for things with a better typed equivalent OR no agent value at all" — store-population doesn't fit either case.
+**Store-population commands stay exposed.** `sync`, `stale`, `orphans`, `reconcile`, `load`, `export`, `import`, `workflow`, `analytics` look like operator commands but have real agent value — `sql` and `search` return empty until `sync` populates the store. Hiding either side breaks the contract.
-A novel domain command that maps cleanly to a single agent action gets exposed automatically. The author does not need to declare it anywhere.
+**When in doubt, leave it exposed.** Hiding a command that should be exposed silently breaks contracts; exposing one that should be hidden just adds a low-value tool. The default flips toward exposure because agents must be able to do anything users can.
### Tool safety annotations
-The MCP spec includes per-tool annotations (`readOnlyHint`, `destructiveHint`, `idempotentHint`, `openWorldHint`) that hosts like Claude Desktop use to classify tool safety and decide when to ask for user permission. Without annotations the host defaults conservatively to "could write or delete," demanding permission per call.
-
-The generator emits annotations automatically based on what it knows:
-
-- **Spec endpoint mirrors:** annotated from the HTTP method. `GET` → `readOnlyHint: true` + `openWorldHint: true`. `DELETE` → `destructiveHint: true` + `openWorldHint: true`. `POST`/`PUT`/`PATCH` get `openWorldHint: true` (writes, not destructive). All endpoint-mirror tools also carry `openWorldHint: true` since they call external APIs.
-- **Built-in MCP tools:** `context`, `sql`, `search` all carry `readOnlyHint: true` (no `openWorldHint` — they read local domain context, the local DB, or the local FTS index, not external APIs).
-- **Runtime-walker shell-out tools:** the walker can't infer semantics from a Cobra command alone, so they ship without annotations by default. Authors can opt a command into the read-only hint via `cmd.Annotations["mcp:read-only"] = "true"` (parallel to `mcp:hidden`). Use this on novel CLI commands that don't mutate external state — read-only API queries, local cache lookups, lightweight derivations.
-
-Default openness: missing annotations don't break anything; they just produce more permission prompts. Adding the wrong annotation (e.g., claiming `readOnlyHint: true` on a tool that mutates state) is the failure mode to avoid — the host trusts the claim and stops asking.
-
-### Adding a new framework command
-
-When you add a new generator-emitted top-level command, the default is **expose it as an MCP tool** — no action needed in the walker. The runtime walker registers any user-facing Cobra command as a shell-out tool automatically.
-
-Only update `frameworkCommands` in `internal/generator/templates/cobratree/classify.go.tmpl` when the new command meets one of the two skip criteria above (typed equivalent already exists, or non-functional via MCP). Adding to `frameworkCommands` is a structural choice to *hide* a capability from agents — make it deliberately, not by reflex.
-
-Skipping a command that should be exposed silently breaks contracts (the `sync`/`sql` example above). Exposing a command that should be hidden adds a low-value tool to the catalog but doesn't break anything. **When in doubt, leave it out of the framework set.**
-
-This is the same shape as the "When adding a capability that affects scoring" rule a few sections up: a new generator capability must update the dependent verifier or surface in the same change. Forgetting either half ships a CLI whose advertised contract diverges from what's actually emitted.
+MCP hosts use `readOnlyHint` / `destructiveHint` / `idempotentHint` / `openWorldHint` to decide when to ask the user for permission. Missing annotations default to "could write or delete" — every call prompts.
-### Why agent-facing != user-facing
+The generator emits annotations automatically:
-Diagnostics (`doctor`, `version`), interactive setup (`auth login`), local-only operations (`profile save`, `feedback`), and shell ergonomics (`completion`, `which`) all belong on the human-facing CLI. Exposing them as MCP tools doesn't help agents — it pollutes the tool catalog with tools that either fail without TTY input, return information the agent doesn't need (e.g., the CLI's own version), or duplicate first-class MCP tools (`agent-context` would shadow the typed `context` MCP tool). Curated absence is a feature.
+- **Endpoint mirrors:** from HTTP method. `GET` → read-only + open-world. `DELETE` → destructive + open-world. `POST`/`PUT`/`PATCH` → open-world (writes, not destructive).
+- **Built-in tools:** `context`, `sql`, `search` are read-only (no open-world; they read local data).
+- **Shell-out tools (runtime walker):** no annotations by default — the walker can't infer from a Cobra command alone. Opt into read-only with `cmd.Annotations["mcp:read-only"] = "true"` for novel commands that don't mutate external state (read-only API queries, cache lookups, derivations).
-The default flips toward exposure for one reason: agents must be able to do anything users can. So the rules are written as exceptions to a permissive default, not as an allowlist. When in doubt, leave it exposed — undeclared MCP gaps were the bug class that drove this whole architecture (see `docs/plans/2026-04-28-001-feat-mcp-cobra-tree-mirror-plan.md` for the audit that surfaced ESPN missing 18 commands and company-goat missing 7).
+Wrong annotations are worse than missing ones: the host trusts the claim and stops asking. A `readOnlyHint: true` on a mutating tool is a real bug; a missing annotation is just a permission prompt.
## Build, Test & Lint
@@ -142,6 +121,8 @@ Golden verification does not replace `go test ./...`, `go vet ./...`, `golangci-
- `skills/` - Claude Code skill definitions
- `testdata/` - Test fixtures (internal + OpenAPI specs)
- `docs/PIPELINE.md` - Portable contract for the 9-phase generation pipeline (preflight through ship). Phase names and ordering are authoritative in `internal/pipeline/state.go`; per-phase intent is authoritative in `internal/pipeline/seeds.go`. Update `docs/PIPELINE.md` in the same PR whenever those files change
+- `docs/SKILLS.md` - Skill authoring conventions: workflow parity, reference-file pattern, frontmatter fields
+- `docs/PATTERNS.md` - Cross-cutting design patterns (deterministic-inventory ledger, etc.)
## Glossary
@@ -153,6 +134,13 @@ Key terms used throughout this repo. Several have overloaded meanings — the gl
**Subsystem names are fine alongside the Printing Press name.** When skills produce diagnostic output (retro findings, issue tables, work units), use component names — generator, scorer, skills, binary — to tell developers *where* to fix something. "Fix the Printing Press" is useless as an action item; "fix the scorer — it penalizes cookie auth" is actionable. The Printing Press is the system; the subsystems are how you navigate within it.
+**Default disambiguation conventions.** Several terms below are overloaded; when body prose uses one without qualifier, default to the local form:
+
+- "library" → local library (`~/printing-press/library/<api-slug>/`). The public library is always called out explicitly: "public library" or "public library repo."
+- "publish" → in body prose, prefer "the publish step" (pipeline) or "publish to the public library" (skill workflow) when context isn't already established.
+- "manifest" → `tools-manifest.json` (the MCP tool catalog). The other manifests (`manifest.json` for plugin metadata, `.printing-press.json` for provenance) are always called by full name.
+- "catalog" → the embedded `catalog/` in this repo. The public library's category-organized catalog of finished CLIs is "public library catalog."
+
| Canonical term | Meaning |
|----------------|---------|
| **the printing press** / **the machine** | This repo's generator system — the Go binary, templates, skills, and catalog that together produce CLIs. |
@@ -163,7 +151,7 @@ Key terms used throughout this repo. Several have overloaded meanings — the gl
| **brief** | The output of the machine's research phase (Phase 1) — a condensed doc covering API identity, competitors, data layer, and product thesis. Stored in `manuscripts/<api>/<run>/research/`. Drives all downstream decisions. |
| **browser-sniff** | Browser-driven API discovery. The user captures live traffic via browser automation (browser-use, agent-browser) or DevTools as a HAR; the `browser-sniff` subcommand analyzes the HAR and produces an OpenAPI-compatible spec. Produces a `discovery/` manuscript with `browser-sniff-report.md`, HAR captures, and `browser-sniff-unique-paths.txt`. Use when no official spec exists or to supplement one with endpoints the docs miss. |
| **crowd-sniff** | Discovery technique that scrapes npm, PyPI, and GitHub for unofficial API clients to learn undocumented endpoints, auth patterns, and rate limits. Produces a `discovery/` manuscript with `crowd-sniff-report.md`. Complementary to browser-sniff — community-sourced vs. browser-captured. Used when no official spec exists or to supplement one. |
-| **manuscript** | The full archive of a generation run. Contains three subdirectories: `research/` (briefs, spec analysis), `proofs/` (dogfood, verify, scorecard results), and optionally `discovery/` (browser-sniff and crowd-sniff artifacts). Stored at `~/printing-press/manuscripts/<api-slug>/<run-id>/`. |
+| **manuscript** | The full archive of a generation run. Contains three subdirectories: `research/` (briefs, spec analysis), `proofs/` (dogfood, verify, scorecard results), and optionally `discovery/` (browser-sniff and crowd-sniff artifacts). Stored at `~/printing-press/manuscripts/<api-slug>/<run-id>/`. The local library is the working copy of the latest successful run for a given API; manuscripts are immutable archives across runs — same `<api-slug>` keys, separate top-level directories. |
| **emboss** | A second-pass improvement cycle for an already-printed CLI. Audits baseline, re-researches, identifies top improvements, rebuilds, re-verifies, reports delta. Subcommand: `printing-press emboss <api>`. Still active — not deprecated. |
| **polish** | Targeted fix-up of a printed CLI (distinct from emboss's full cycle). Skill: `/printing-press-polish`. The retro improves the machine; polish improves the printed CLI. |
| **retro** / **retrospective** | Post-generation analysis of *the machine itself* — not the printed CLI. Identifies systemic improvements to templates, the Go binary, skill instructions, or catalog. Output goes to `docs/retros/` and `manuscripts/<api>/<run>/proofs/`. |
@@ -186,8 +174,10 @@ Key terms used throughout this repo. Several have overloaded meanings — the gl
| **public library repo** | The GitHub repo [`mvanhorn/printing-press-library`](https://github.com/mvanhorn/printing-press-library) — public catalog of finished CLIs organized by category. `/printing-press-publish` pushes here. |
| **publish (pipeline)** | The pipeline step that moves a working CLI into the local library and writes the `.printing-press.json` provenance manifest. |
| **publish (to public library repo)** | The skill-driven workflow (`/printing-press-publish`) that packages a local library CLI and creates a PR in the public library repo. |
-| **provenance** / **`.printing-press.json`** | Manifest written to each published CLI's root. Contains generation metadata: spec URL, checksum, run ID, printing-press version, timestamp. `api_name` is the canonical API identity; `cli_name` is the executable name. Makes the directory self-describing. |
-| **catalog** | Embedded YAML entries in `catalog/` describing available APIs (name, spec URL, category, tier). Baked into the binary at build time via `catalog.FS`. |
+| **provenance** / **`.printing-press.json`** | Manifest written to each published CLI's root. Contains generation metadata: spec URL, checksum, run ID, printing-press version, timestamp. `api_name` is the canonical API identity; `cli_name` is the executable name. Makes the directory self-describing. Distinct from `manifest.json` (plugin metadata) and `tools-manifest.json` (MCP tool catalog). |
+| **`manifest.json`** | Claude plugin manifest at the printed CLI root. Carries `display_name`, `description`, `homepage`, version, and other plugin-host fields. Read by Claude Desktop and other MCP-aware hosts when installing the CLI as a plugin. Distinct from `tools-manifest.json` (the MCP tool catalog) and `.printing-press.json` (provenance). |
+| **`tools-manifest.json`** | MCP tool catalog at the printed CLI root. For each tool, carries name, description, parameters, and auth metadata. The MCP server reads it at runtime to register typed tools with full schemas; the audit and scorecard pipelines consume it. "The manifest" without qualifier means this file. Distinct from `manifest.json` (plugin metadata) and `.printing-press.json` (provenance). |
+| **catalog** (embedded) | Embedded YAML entries in `catalog/` describing available APIs (name, spec URL, category, tier). Baked into the binary at build time via `catalog.FS`. Distinct from the **public library catalog**, which is the category-organized index of finished CLIs in the public library repo. |
| **tier** | Catalog classification: `official` (vendor-maintained spec) or `community` (unofficial/reverse-engineered). Affects risk expectations. |
| **runstate** | Mutable per-workspace state at `~/printing-press/.runstate/<scope>/`. Tracks current run and sync cursors. Distinct from manuscripts, which are immutable archives. |
@@ -219,25 +209,20 @@ Every commit and PR title must include one of these scopes. The `PR Title` actio
**PR titles must follow the same format.** GitHub's "Squash and merge" uses the PR title as the squash commit message, so release-please reads PR titles on main. The `PR Title` GitHub Action (`.github/workflows/pr-title.yml`) enforces this — PRs with invalid titles cannot merge.
-## Versioning
+## Versioning & Release
+
+Releases are fully automated by release-please + goreleaser; no manual steps. The flow:
+
+1. Merge PRs to main with conventional-commit titles.
+2. release-please opens (and updates) a release PR with the accumulated changelog.
+3. When ready to ship, merge the release PR. release-please bumps all version files, creates a git tag, opens a GitHub release; goreleaser builds and attaches cross-platform binaries.
**Never manually edit version numbers.** Three files carry the version and release-please keeps them in sync:
- `.claude-plugin/plugin.json` → `version`
- `.claude-plugin/marketplace.json` → `plugins[0].version`
-- `internal/version/version.go` → `var Version` (annotated with `x-release-please-version`)
-
-`TestVersionConsistencyAcrossFiles` in `internal/cli/release_test.go` will fail if versions drift.
-
-## Release Process
+- `internal/version/version.go` → `var Version` (annotated `x-release-please-version`)
-Releases are fully automated. No manual steps required.
-
-1. **Merge PRs to main** with conventional commit messages / PR titles
-2. **release-please opens a release PR** accumulating all changes since the last release, with a generated changelog
-3. **Merge the release PR** when ready to cut a release
-4. **Automated:** release-please bumps all three version files, creates a git tag, and creates a GitHub release
-5. **Automated:** goreleaser builds cross-platform binaries (linux/darwin/windows × amd64/arm64) and attaches them to the release
-6. **Users update** via `go install ...@latest` (picks up the new tag) or download binaries from the release
+`TestVersionConsistencyAcrossFiles` in `internal/cli/release_test.go` fails if they drift.
## Adding Catalog Entries
@@ -259,11 +244,11 @@ Run `go test ./...` before considering your work done.
Generated CLIs must pass 7 gates: go mod tidy, go vet, go build, binary build, --help, version, doctor.
-## `~/printing-press/` Layout
+## Local Artifacts (`~/printing-press/`)
Generated artifacts live under the user's home directory, not in this repo.
-- `library/<api-slug>/` — Published CLIs (e.g., `notion`). Directory is keyed by API slug, not CLI name. The binary inside is still `<api-slug>-pp-cli`.
+- `library/<api-slug>/` — Local library: printed CLIs the generator has produced (e.g., `notion`). Directory is keyed by API slug, not CLI name. The binary inside is still `<api-slug>-pp-cli`. This is the working copy; the public library is the published-and-curated counterpart (see "Public Library" below).
- `manuscripts/<api-slug>/` — Archived research and verification proofs, keyed by API slug (e.g., `notion`), not CLI name. One API can have multiple runs.
- `.runstate/<scope>/` — Mutable per-workspace state (current run, sync cursors). Scoped by repo basename + hash.
@@ -271,6 +256,19 @@ The API slug is derived by the generator from the spec title (`cleanSpecName`),
The `-pp-` infix exists to avoid colliding with official CLIs. The binary `notion-pp-cli` can coexist with whatever `notion-cli` Notion ships themselves. The library directory is just `notion/` — the `-pp-cli` suffix only appears on binary names, not directory names.
+## Public Library
+
+The public library is the GitHub repo [`mvanhorn/printing-press-library`](https://github.com/mvanhorn/printing-press-library) — a curated, category-organized catalog of finished printed CLIs. Users install printed CLIs from here; this is where a CLI goes when it's ready to ship.
+
+**Local → public flow.** A successfully generated printed CLI lives in the local library. The `/printing-press-publish` skill packages a local CLI and opens a PR against the public library repo. Merging that PR is what moves the CLI from "works on this machine" to "users can install it."
+
+**Local-vs-public divergence.** The local library and public library can drift in two ways:
+
+- **Expected divergence.** Some files are intentionally rewritten by the publish step — most notably `go.mod`'s module path. The polish skill's divergence check exempts these.
+- **Unexpected divergence.** Local edits since the last publish — polish in progress, manual fixes, mcp-sync regen — that haven't been pushed. The polish skill's divergence check surfaces these so you can decide to either republish or discard the local changes.
+
+Treat the public library as the durable artifact and the local library as the working copy. When users hit a bug, they're hitting the public library's version, not whatever's currently in `~/printing-press/library/`.
+
## Internal Skills
`.claude/skills/` contains internal skills for developing the printing press itself (e.g., `printing-press-retro`). These load automatically when Claude Code is started from inside this repo — no setup needed.
@@ -283,73 +281,41 @@ If you're running Claude Code from a different directory and need these skills a
This copies the internal skills to `~/.claude/skills/`.
-## Skill Workflow Parity
-
-When a machine change alters what an agent should do, what a command now guarantees, or where source-of-truth data lives, update the relevant `SKILL.md` in the same change. Do not leave the skill as a stale manual workaround for behavior the machine now owns.
-
-Check `skills/printing-press/SKILL.md` especially when touching generator, dogfood, verify, scorecard, publish, lock/promote, manuscript/runstate, or README/SKILL rendering behavior. If a machine step becomes deterministic, the skill should say the command owns it and reserve agentic review for the remaining semantic judgment. If a command's output, gate, phase order, or failure mode changes, update the phase instructions, reviewer prompt contracts, and fix-order guidance that mention it.
-
-Decide responsibility explicitly:
-
-- **Machine capability:** deterministic transformations, schema sync, provenance fields, generated sections with structured inputs, mechanical validation, artifact copying, score calculations, and anything where the correct output can be derived from repo files or command output without judgment. Implement it in Go/templates/tests; SKILL.md should describe the guarantee, not ask the agent to perform it manually.
-- **SKILL.md / agent capability:** judgment calls, product tradeoffs, semantic honesty, whether prose overpromises, whether output is plausible, whether a feature is worth building, or workflows that require user/API context not available to the binary. Keep these as clear agent instructions and reviewer prompt contracts.
-- **Both:** the machine should produce or verify the deterministic substrate, then SKILL.md should direct the agent to inspect the remaining semantic layer. Example pattern: dogfood syncs README/SKILL feature blocks from `novel_features_built`; the skill tells the agent to audit surrounding prose, recipes, trigger phrases, and examples for indirect claims.
-
-For any SKILL.md update, search for the old concept across the skill file, not just the paragraph closest to the code change. Agentic review prompts often duplicate workflow assumptions from earlier phase instructions.
-
-## Skill Authoring: Reference File Pattern
-
-Skills use a `references/` directory for content that is only needed during specific phases or conditions. The SKILL.md stays lean with inline pointers (`Read [references/foo.md](...) when X`), and the agent loads the reference file only when the condition is met.
-
-**Why this matters:** SKILL.md content is loaded into the context window for every tool call in the session. A 2,000-line skill burns tokens on every phase — even phases that don't need most of the content. Extracting conditional sections (e.g., browser capture flows only needed when browser-sniffing, codex templates only needed in codex mode) into reference files reduces baseline context by 30-40%.
-
-**What stays inline:** Cardinal rules, decision matrices, phase structure, user-facing prompts — anything the agent needs at all times or to decide whether to load more.
-
-**What gets extracted:** Implementation details for conditional paths: capture tool CLI commands, delegation templates, scoring frameworks, report templates. These are loaded on-demand when the agent reaches the relevant phase gate.
-
-## Deterministic Inventory + Agent-Marked Ledger
-
-When a workflow has a checklist where detection is mechanical but each item needs per-item judgment, split the work between a binary-emitted inventory and an agent-maintained ledger. The binary owns "what's there"; the agent owns "what to do about each item." A persistent file holds both, so the work survives context flushes and the audit trail surfaces the agent's reasoning.
-
-The canonical example is `printing-press tools-audit` + `skills/printing-press/references/tools-polish.md`. The binary parses every Cobra command and emits findings (empty Short, thin Short, missing read-only annotation). The agent walks each finding, fixes most, and marks the rest `accepted` with a one-sentence rationale. The ledger persists at `<cli-dir>/.printing-press-tools-polish.json` for 24 hours.
-
-Reach for this pattern when the work has the **detect mechanically + decide per-item + persist rationale** shape. The trigger isn't a numeric item count — a 15-item list with three accept decisions across two sessions benefits, while a 200-item batch update where every item has the same fix does not. Skip it when one pass is enough, when every item has the same fix, when detection itself requires judgment, or when a `TodoWrite` task list with rationale in the description carries the whole workflow.
-
-**Structure:**
-
-1. **Binary writes the inventory.** A subcommand emits a structured snapshot file (`.<topic>-ledger.json` or similar) on every run. Each entry has stable identity fields (file, line, kind, key) and may carry agent-written `status` and `note` fields (`omitempty` so the bare audit output stays clean).
-2. **Agent annotates the ledger.** When the agent decides to keep an item as-is, it edits the ledger to set `status: "accepted"` and writes a `note`. Code fixes are *not* marked manually — the next run re-detects and the finding disappears automatically.
-3. **Re-runs preserve agent state.** The binary reads the previous ledger before writing the new one. Findings whose identity key matches inherit `status` and `note`. Findings present last run but absent now read as "resolved" in the delta line. New findings start fresh as pending.
-4. **Staleness, not history.** Ledgers age out (e.g., 24h) and are deleted. They're working state, not artifacts to preserve in version control. Add the ledger filename to the relevant repo's `.gitignore` if the cli-dir lives inside one.
-5. **Verification asks for zero pending, not zero findings.** "Done" means every finding is either fixed (auto-removed) or explicitly accepted with a note — not that the count is zero. Reviewers can see accepts in the ledger and judge whether each rationale holds.
-
-Compared to the alternatives: pure `TodoWrite` state is invisible to the binary and dies with the session; pure binary recompute can't track accept decisions and re-flags them every run; multi-file artifacts (cards/, ledger.md, rejections.md per the `simplify-and-refactor` skill) are heavier than warranted when each item is small and self-contained.
-
-## Skill Frontmatter: `context: fork` and `user-invocable`
-
-Two skill frontmatter fields shape how a skill participates in larger workflows. Both default to permissive behavior (shared context, user-invocable). Set them explicitly when the skill plays a non-default role.
+## Skill Authoring
-### `context: fork`
+When a machine change alters what an agent should do or what a command guarantees, update the relevant `SKILL.md` in the same change — don't leave the skill as a stale manual workaround for behavior the machine now owns.
-Default: skills run in the caller's context. The skill sees the full parent conversation; the parent sees the skill's tool calls and output interleaved with its own work.
+Detail in [`docs/SKILLS.md`](docs/SKILLS.md): workflow parity (when machine changes require skill changes), the reference-file pattern (extracting conditional content from SKILL.md), and the `context: fork` / `user-invocable` frontmatter fields.
-`context: fork` gives the skill its own context window. Two consequences pull in opposite directions:
+## Code & Comment Hygiene
-- **Benefit:** the skill starts with a fresh, dedicated context — its full window is available for its own work (multi-step loops, sub-agent transcripts, large reads) rather than competing with whatever the parent has already accumulated.
-- **Cost:** the skill can't see anything from the parent's conversation. Everything it needs must come through `args`, be readable from disk, or be hardcoded.
+### Write-time defaults
-The decision rule is whether the skill is **self-contained** given its declared inputs. If args plus the filesystem cover everything the skill needs (e.g., `printing-press-polish` takes a CLI dir and reads the rest from the repo and manuscripts; `printing-press-output-review` takes a CLI dir and runs `scorecard --live-check` to gather data), `context: fork` is a clear win. If the skill needs prior tool output, conversation history, or anything else the parent has accumulated, don't fork — the skill won't have access to it and you'll end up plumbing context through args anyway.
+- **No speculative future-proofing in comments.** "Structured to absorb additional dimensions if future X needs them" — write the future struct when the future arrives. Today's reader can't act on a comment about hypothetical needs.
+- **No dates, incidents, or ticket numbers in code comments.** Belongs in the PR description and commit message, not the code. Comments stay forever; incidents fade.
+- **Code comments must be self-contained.** Don't make them load-bearing on in-repo skill prose, plans, or reference files that could be reorganized. RFCs, vendor API docs, and language specs are durable; in-repo prose is not. If you find yourself wanting to link, keep enough context inline that the code reads correctly when the link breaks.
+- **Don't restate the field or function name in its comment.** `MCPDescriptionQuality int` does not need `// the score for MCP description quality`. Document WHY (hidden constraints, subtle invariants), not WHAT (the name already says it).
+- **Categorical strings → typed const at introduction.** When adding an event kind, finding type, status name, or any string that names a category, declare the const in the same commit even with one call site. The compiler catches typos at every future site, and the const adds two lines today.
+- **Single-case switch with default fallthrough → `||`.** If every branch returns the same thing, `switch x { case A, B: return true } return false` is just `return x == A || x == B`. Switch shape implies cases will diverge; if they won't, write the `||`.
+- **Parse command inputs once at the entry point.** In a `RunE`, read files / manifests / configs at the entry and pass parsed results into helpers. Don't re-read "for clarity" — the cost compounds when helpers cross-call.
+- **UTF-8 safe string truncation.** `s[:n] + "…"` cuts mid-rune on multibyte input. Use rune slicing or an existing truncate helper from the same package.
-### `user-invocable`
+### Pre-commit: scan the diff
-Default: `true` — the skill is discoverable as a slash command (`/skill-name`) and routes from trigger phrases in the description. Setting `user-invocable: false` makes it internal-only: only Claude can invoke it (typically via the Skill tool from another skill).
+- Near-identical loops or functions that should share a helper
+- A compound predicate (e.g., `f.Status != accepted || (requiresX(f.Kind) && missingX(f))`) inlined at 3+ sites that should be a named function
+- Parallel `hasX() bool` / `xCount() int` that drifted apart — derive one from the other
+- The same string literal repeated across sites where the categorical-const rule above would have applied — the const is cheap to add retroactively if missed at write-time
-Set `user-invocable: false` when the skill has no standalone meaning for a user. A user typing `/internal-skill` would get half a workflow with no input gate, no follow-up offer, no completion verdict. The actionable wrappers are the parent skills.
+## Editing AGENTS.md
-In this family, every printing-press skill is user-invocable except `printing-press-output-review`, which runs only as a sub-step inside Phase 4.85 (main skill) and the polish diagnostic loop.
+The "Code & Comment Hygiene" rules apply to this file too. Specifically:
-### Internal-only sub-skill pattern
+- **No dates, incidents, or ticket numbers in rules.** Justification belongs in the PR introducing the rule, not embedded in it.
+- **Don't defend the doc's structure inside the doc.** "We split this honestly because…" doesn't help future readers — write the rule, trust them.
+- **Make rules applicable at the moment they fire.** Write-time rules in a write-time section, diff-review rules in a review section. A rule the agent can't apply at the relevant moment is worse than no rule.
+- **Examples should be generic or anti-pattern-shaped, not lifted from the specific incident that prompted the rule.**
-When a workflow step has multiple parents and no standalone user meaning, extract it into a `user-invocable: false` skill that both parents invoke via the Skill tool. Single source of truth for the prompt, gate logic, and any reference docs. The framework dispatches it; nobody has to find and read sibling SKILL.md prose at runtime.
+## Patterns
-The two fields compose. `context: fork` + `user-invocable: false` is the combo for self-contained internal sub-skills. `context: fork` alone (default user-invocable) is for user-facing skills with their own multi-step workflow that don't need parent context. Default frontmatter is for terse helper skills, or any skill that genuinely needs to see the parent's conversation.
+Cross-cutting design patterns are documented in [`docs/PATTERNS.md`](docs/PATTERNS.md). Notably **Deterministic Inventory + Agent-Marked Ledger** — the shape used by `printing-press tools-audit` for workflows that combine mechanical detection with per-item agent judgment, with four enforcement primitives (pre-decision fields, duplicate-rationale rejection, numeric end-state gate, resume protocol) for cases where bulk-accept is a realistic failure mode.
diff --git a/docs/PATTERNS.md b/docs/PATTERNS.md
new file mode 100644
index 00000000..88f40aa5
--- /dev/null
+++ b/docs/PATTERNS.md
@@ -0,0 +1,43 @@
+# Patterns
+
+Cross-cutting design patterns the printing press uses, with the rules for applying them. Loaded on-demand when designing or extending a workflow that needs one.
+
+## Deterministic Inventory + Agent-Marked Ledger
+
+When a workflow has a checklist where detection is mechanical but each item needs per-item judgment, split the work between a binary-emitted inventory and an agent-maintained ledger. The binary owns "what's there"; the agent owns "what to do about each item." A persistent file holds both, so the work survives context flushes and the audit trail surfaces the agent's reasoning.
+
+The canonical example is `printing-press tools-audit` + `skills/printing-press-polish/references/tools-polish.md`. The binary parses every Cobra command and the runtime tools manifest, emits findings (empty Short, thin Short, missing read-only annotation, thin/empty MCP description). The agent walks each finding, fixes most, and marks the rest `accepted` with a one-sentence rationale plus pre-decision fields where the gate requires them. The ledger persists at `<cli-dir>/.printing-press-tools-polish.json` for 24 hours.
+
+Reach for this pattern when the work has the **detect mechanically + decide per-item + persist rationale** shape. The trigger isn't a numeric item count — a 15-item list with three accept decisions across two sessions benefits, while a 200-item batch update where every item has the same fix does not. Skip it when one pass is enough, when every item has the same fix, when detection itself requires judgment, or when a `TodoWrite` task list with rationale in the description carries the whole workflow.
+
+### Structure
+
+1. **Binary writes the inventory.** A subcommand emits a structured snapshot file (`.<topic>-ledger.json` or similar) on every run. Each entry has stable identity fields (file, line, kind, key) and may carry agent-written `status` and `note` fields (`omitempty` so the bare audit output stays clean).
+2. **Agent annotates the ledger.** When the agent decides to keep an item as-is, it edits the ledger to set `status: "accepted"` and writes a `note`. Code fixes are *not* marked manually — the next run re-detects and the finding disappears automatically.
+3. **Re-runs preserve agent state.** The binary reads the previous ledger before writing the new one. Findings whose identity key matches inherit `status` and `note`. Findings present last run but absent now read as "resolved" in the delta line. New findings start fresh as pending.
+4. **Staleness, not history.** Ledgers age out (e.g., 24h) and are deleted. They're working state, not artifacts to preserve in version control. Add the ledger filename to the relevant repo's `.gitignore` if the cli-dir lives inside one.
+5. **Verification asks for zero pending plus zero gate failures, not zero findings.** "Done" means every finding is either fixed (auto-removed) or explicitly accepted with a note that satisfies the enforcement primitives below. Reviewers can see accepts in the ledger and judge whether each rationale holds.
+
+### Enforcement primitives (when bulk-accept is the failure mode)
+
+The five-point structure above gives you a ledger. It does not, on its own, force the agent to deliberate per item — `/simplify-and-refactor-code-isomorphically` (Isomorphism Card filled before each edit) and `/library-updater` (per-package log + crash-recovery checkpoint) layer additional checks on top of the same shape. Apply the primitives below when the workload is large enough that bulk-accept is a realistic failure mode (>50 findings of similar kind, >1 review round expected, or anywhere the agent might reach for "this is systemic" as a punt).
+
+1. **Pre-decision fields per item, filled before the verdict.** For finding kinds where bulk-accept is the failure mode, add required fields the agent must populate before `status: "accepted"` counts. The fields force per-item reasoning — naming the specific source material, the target output, and the gap between them is much harder to fake than a one-line `note`. The binary refuses runs where any accepted entry of that kind has empty pre-decision fields.
+
+ Concrete example from tools-audit: `thin-mcp-description` accepts require `spec_source_material` (what the OpenAPI spec actually provides for this endpoint), `target_description` (what a 10/10 description would say), and `gap_analysis` (why the generator can't produce target from source today). The third field is load-bearing — it forces the agent to decide between "file as a generator improvement" and "write an override," instead of a generic "specs are thin" punt.
+
+2. **Reject identical rationales above a threshold.** Cluster accepted entries by normalized `note` text (lowercase, collapsed whitespace). If any cluster exceeds N entries (5 in tools-audit), the run is incomplete. Differentiated rationales survive; bulk paste-the-same-thing-everywhere does not.
+
+ The threshold is a hedge, not an absolute: 3-4 accepts sharing a rationale is normal noise; 50 sharing one is a punt. Set the threshold low enough to catch the punt without false-positiving on natural overlap.
+
+3. **Numeric end-state gate tied to a scorer dimension.** Capture the relevant score *before* work begins (sticky in the ledger across runs). On each subsequent run, recompute the current score. If the agent accepted findings that *should* have driven the dimension up but didn't, the run is incomplete. Either the accepts were unwarranted (overrides would have lifted the score) or the dimension is mis-scored (rare; surface to retro).
+
+ Concrete example from tools-audit: `scorecard_before.mcp_description_quality` is captured on the first run. If subsequent runs accept any `thin-mcp-description` findings without lifting `MCPDescriptionQuality`, the run is incomplete. The agent owes either an override or a generator-improvement filing — accept-and-walk-away is no longer a complete state.
+
+4. **Resume protocol with explicit progress field.** The 24h staleness rule covers across-runs cleanup but not within-run context flushes. Add `progress.last_processed_finding_id` to the ledger header; the agent updates it after each decision. The binary's render surfaces the next-pending finding as a `next:` line so a re-invocation after compaction picks up where the agent left off rather than re-scanning from the head.
+
+ The progress field is a soft hint, not a gate — when absent, the binary derives the next-pending finding from `status` + pre-decision-fields state. Setting the field is the explicit checkpoint the agent updates as they walk.
+
+### Comparison to alternatives
+
+Pure `TodoWrite` state is invisible to the binary and dies with the session; pure binary recompute can't track accept decisions and re-flags them every run; multi-file artifacts (cards/, ledger.md, rejections.md per the `simplify-and-refactor` skill) are heavier than warranted when each item is small and self-contained. The single-JSON ledger plus the four enforcement primitives is the minimum that survives both context flushes and bulk-accept patterns.
diff --git a/docs/SKILLS.md b/docs/SKILLS.md
new file mode 100644
index 00000000..0f8cecc1
--- /dev/null
+++ b/docs/SKILLS.md
@@ -0,0 +1,56 @@
+# Skill Authoring
+
+Conventions for the skills shipped from this repo (under `skills/`) and any internal skills under `.claude/skills/`. Loaded on-demand when working on skill content; not needed on every interaction.
+
+## Workflow Parity
+
+When a machine change alters what an agent should do, what a command now guarantees, or where source-of-truth data lives, update the relevant `SKILL.md` in the same change. Don't leave the skill as a stale manual workaround for behavior the machine now owns.
+
+Check `skills/printing-press/SKILL.md` especially when touching generator, dogfood, verify, scorecard, publish, lock/promote, manuscript/runstate, or README/SKILL rendering behavior. If a machine step becomes deterministic, the skill should say the command owns it and reserve agentic review for the remaining semantic judgment. If a command's output, gate, phase order, or failure mode changes, update the phase instructions, reviewer prompt contracts, and fix-order guidance that mention it.
+
+Decide responsibility explicitly:
+
+- **Machine capability:** deterministic transformations, schema sync, provenance fields, generated sections with structured inputs, mechanical validation, artifact copying, score calculations, and anything where the correct output can be derived from repo files or command output without judgment. Implement it in Go/templates/tests; SKILL.md should describe the guarantee, not ask the agent to perform it manually.
+- **SKILL.md / agent capability:** judgment calls, product tradeoffs, semantic honesty, whether prose overpromises, whether output is plausible, whether a feature is worth building, or workflows that require user/API context not available to the binary. Keep these as clear agent instructions and reviewer prompt contracts.
+- **Both:** the machine should produce or verify the deterministic substrate, then SKILL.md should direct the agent to inspect the remaining semantic layer. Example pattern: dogfood syncs README/SKILL feature blocks from `novel_features_built`; the skill tells the agent to audit surrounding prose, recipes, trigger phrases, and examples for indirect claims.
+
+For any SKILL.md update, search for the old concept across the skill file, not just the paragraph closest to the code change. Agentic review prompts often duplicate workflow assumptions from earlier phase instructions.
+
+## Reference File Pattern
+
+Skills use a `references/` directory for content that is only needed during specific phases or conditions. The SKILL.md stays lean with inline pointers (`Read [references/foo.md](...) when X`), and the agent loads the reference file only when the condition is met.
+
+**Why this matters:** SKILL.md content is loaded into the context window for every tool call in the session. A 2,000-line skill burns tokens on every phase — even phases that don't need most of the content. Extracting conditional sections (e.g., browser capture flows only needed when browser-sniffing, codex templates only needed in codex mode) into reference files reduces baseline context by 30-40%.
+
+**What stays inline:** Cardinal rules, decision matrices, phase structure, user-facing prompts — anything the agent needs at all times or to decide whether to load more.
+
+**What gets extracted:** Implementation details for conditional paths: capture tool CLI commands, delegation templates, scoring frameworks, report templates. These are loaded on-demand when the agent reaches the relevant phase gate.
+
+## Frontmatter: `context: fork` and `user-invocable`
+
+Two skill frontmatter fields shape how a skill participates in larger workflows. Both default to permissive behavior (shared context, user-invocable). Set them explicitly when the skill plays a non-default role.
+
+### `context: fork`
+
+Default: skills run in the caller's context. The skill sees the full parent conversation; the parent sees the skill's tool calls and output interleaved with its own work.
+
+`context: fork` gives the skill its own context window. Two consequences pull in opposite directions:
+
+- **Benefit:** the skill starts with a fresh, dedicated context — its full window is available for its own work (multi-step loops, sub-agent transcripts, large reads) rather than competing with whatever the parent has already accumulated.
+- **Cost:** the skill can't see anything from the parent's conversation. Everything it needs must come through `args`, be readable from disk, or be hardcoded.
+
+The decision rule is whether the skill is **self-contained** given its declared inputs. If args plus the filesystem cover everything the skill needs (e.g., `printing-press-polish` takes a CLI dir and reads the rest from the repo and manuscripts; `printing-press-output-review` takes a CLI dir and runs `scorecard --live-check` to gather data), `context: fork` is a clear win. If the skill needs prior tool output, conversation history, or anything else the parent has accumulated, don't fork — the skill won't have access to it and you'll end up plumbing context through args anyway.
+
+### `user-invocable`
+
+Default: `true` — the skill is discoverable as a slash command (`/skill-name`) and routes from trigger phrases in the description. Setting `user-invocable: false` makes it internal-only: only Claude can invoke it (typically via the Skill tool from another skill).
+
+Set `user-invocable: false` when the skill has no standalone meaning for a user. A user typing `/internal-skill` would get half a workflow with no input gate, no follow-up offer, no completion verdict. The actionable wrappers are the parent skills.
+
+In this family, every printing-press skill is user-invocable except `printing-press-output-review`, which runs only as a sub-step inside Phase 4.85 (main skill) and the polish diagnostic loop.
+
+### Internal-only sub-skill pattern
+
+When a workflow step has multiple parents and no standalone user meaning, extract it into a `user-invocable: false` skill that both parents invoke via the Skill tool. Single source of truth for the prompt, gate logic, and any reference docs. The framework dispatches it; nobody has to find and read sibling SKILL.md prose at runtime.
+
+The two fields compose. `context: fork` + `user-invocable: false` is the combo for self-contained internal sub-skills. `context: fork` alone (default user-invocable) is for user-facing skills with their own multi-step workflow that don't need parent context. Default frontmatter is for terse helper skills, or any skill that genuinely needs to see the parent's conversation.
diff --git a/internal/cli/mcp_audit.go b/internal/cli/mcp_audit.go
index 482ce56c..ef19a9d1 100644
--- a/internal/cli/mcp_audit.go
+++ b/internal/cli/mcp_audit.go
@@ -214,12 +214,16 @@ func renderMCPAuditTable(w interface{ Write(p []byte) (int, error) }, findings [
}
}
+// truncate cuts s to at most n display characters, appending "…"
+// when shortened. Walks runes (not bytes) to keep multibyte
+// characters intact at the boundary.
func truncate(s string, n int) string {
- if len(s) <= n {
+ runes := []rune(s)
+ if len(runes) <= n {
return s
}
if n <= 1 {
- return s[:n]
+ return string(runes[:n])
}
- return s[:n-1] + "…"
+ return string(runes[:n-1]) + "…"
}
diff --git a/internal/cli/tools_audit.go b/internal/cli/tools_audit.go
index 3f2f1b47..268c755b 100644
--- a/internal/cli/tools_audit.go
+++ b/internal/cli/tools_audit.go
@@ -23,6 +23,24 @@ const (
statusAccepted = "accepted"
suspiciousMaxLen = 30
suspiciousMinWord = 4
+
+ // Finding kinds. These strings appear in the JSON output, the
+ // ledger, and in agent-readable error messages, so changing a
+ // value is a backwards-incompatible ledger format change.
+ kindEmptyShort = "empty-short"
+ kindThinShort = "thin-short"
+ kindMissingReadOnly = "missing-read-only"
+ kindEmptyMCPDesc = "empty-mcp-description"
+ kindThinMCPDesc = "thin-mcp-description"
+
+ // duplicateRationaleThreshold caps how many accepted findings may
+ // share the same normalized note before the run is flagged as
+ // incomplete. Hedge against bulk-accept patterns ("systemic to
+ // OpenAPI specs" stamped on N findings without per-item
+ // deliberation). Five identical rationales is suspicious; ten is
+ // almost certainly a punt. See AGENTS.md "Deterministic Inventory
+ // + Agent-Marked Ledger".
+ duplicateRationaleThreshold = 5
)
// MCP description thresholds (pipeline.MCPDescMinLen, MCPDescMinWords,
@@ -77,7 +95,13 @@ Exit 0 regardless of findings (diagnostic, not gating).`,
Args: cobra.ExactArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
cliDir := args[0]
- findings, err := runToolsAudit(cliDir)
+ // Parse tools-manifest.json once and share it across the
+ // audit, snapshot capture, and scorecard-delta gate. Each
+ // of those previously called pipeline.ReadToolsManifest
+ // independently — three reads on the first run.
+ manifest, _ := pipeline.ReadToolsManifest(cliDir)
+
+ findings, err := runToolsAudit(cliDir, manifest)
if err != nil {
return err
}
@@ -91,10 +115,11 @@ Exit 0 regardless of findings (diagnostic, not gating).`,
previous := readPreviousLedger(cliDir)
delta := reconcileWithLedger(previous, findings)
- if err := writeLedger(cliDir, findings); err != nil {
+ if err := writeLedger(cliDir, manifest, findings, previous); err != nil {
fmt.Fprintf(cmd.ErrOrStderr(), "warning: writing ledger %s: %v\n", filepath.Join(cliDir, ledgerFilename), err)
}
- renderToolsAuditTable(cmd.OutOrStdout(), findings, delta)
+ completion := evaluateCompletion(manifest, findings, previous)
+ renderToolsAuditTable(cmd.OutOrStdout(), findings, delta, completion)
return nil
},
}
@@ -128,6 +153,14 @@ type ToolsAuditFinding struct {
Evidence string `json:"evidence"` // the offending text
Status string `json:"status,omitempty"` // "" (== pending) or "accepted"; agent writes
Note string `json:"note,omitempty"` // agent-written rationale for an accept decision
+
+ // Pre-decision fields. The agent fills these BEFORE flipping
+ // Status to "accepted" on kinds where requiresPreDecisionFields
+ // is true; the gate forces per-item deliberation instead of
+ // pattern-matching with one rationale across many findings.
+ SpecSourceMaterial string `json:"spec_source_material,omitempty"`
+ TargetDescription string `json:"target_description,omitempty"`
+ GapAnalysis string `json:"gap_analysis,omitempty"`
}
// readShapedNames is the heuristic for "this command name suggests a
@@ -154,12 +187,12 @@ var readShapedNames = map[string]struct{}{
// <cliDir>/internal/cli/*.go (shell-out tools) and the runtime tools
// manifest at <cliDir>/tools-manifest.json (typed endpoint tools).
// Findings are sorted by file then line for stable output.
-func runToolsAudit(cliDir string) ([]ToolsAuditFinding, error) {
+func runToolsAudit(cliDir string, manifest *pipeline.ToolsManifest) ([]ToolsAuditFinding, error) {
cobraFindings, err := auditCobraSource(cliDir)
if err != nil {
return nil, err
}
- manifestFindings := auditMCPManifest(cliDir)
+ manifestFindings := auditMCPManifest(manifest)
findings := append(cobraFindings, manifestFindings...)
sort.Slice(findings, func(i, j int) bool {
if findings[i].File != findings[j].File {
@@ -210,15 +243,13 @@ func auditCobraSource(cliDir string) ([]ToolsAuditFinding, error) {
return findings, nil
}
-// auditMCPManifest reads tools-manifest.json and flags MCP tool
-// descriptions that fall below the agent-grade bar. The manifest is
-// the source of truth for typed endpoint tools' descriptions; for
-// shell-out tools, descriptions come from the Cobra Short and the
-// auditCommandFields path covers them. Returns nil silently if the
-// manifest is missing (older CLIs predate it) or malformed.
-func auditMCPManifest(cliDir string) []ToolsAuditFinding {
- m, err := pipeline.ReadToolsManifest(cliDir)
- if err != nil {
+// auditMCPManifest flags MCP tool descriptions that fall below the
+// agent-grade bar. The manifest is the source of truth for typed
+// endpoint tools' descriptions; for shell-out tools, descriptions
+// come from the Cobra Short and the auditCommandFields path covers
+// them. Nil manifest (missing or malformed file) emits no findings.
+func auditMCPManifest(m *pipeline.ToolsManifest) []ToolsAuditFinding {
+ if m == nil {
return nil
}
var findings []ToolsAuditFinding
@@ -229,12 +260,12 @@ func auditMCPManifest(cliDir string) []ToolsAuditFinding {
switch {
case t.Description == "":
findings = append(findings, ToolsAuditFinding{
- Kind: "empty-mcp-description", Command: t.Name,
+ Kind: kindEmptyMCPDesc, Command: t.Name,
File: pipeline.ToolsManifestFilename, Evidence: "(empty)",
})
case pipeline.IsThinMCPDescription(t.Description):
findings = append(findings, ToolsAuditFinding{
- Kind: "thin-mcp-description", Command: t.Name,
+ Kind: kindThinMCPDesc, Command: t.Name,
File: pipeline.ToolsManifestFilename, Evidence: t.Description,
})
}
@@ -350,12 +381,12 @@ func auditCommandFields(file string, line int, f commandFields) []ToolsAuditFind
switch {
case f.short == "":
out = append(out, ToolsAuditFinding{
- Kind: "empty-short", Command: name, File: file, Line: line,
+ Kind: kindEmptyShort, Command: name, File: file, Line: line,
Evidence: "(empty)",
})
case suspiciousShort(f.short):
out = append(out, ToolsAuditFinding{
- Kind: "thin-short", Command: name, File: file, Line: line,
+ Kind: kindThinShort, Command: name, File: file, Line: line,
Evidence: f.short,
})
}
@@ -365,7 +396,7 @@ func auditCommandFields(file string, line int, f commandFields) []ToolsAuditFind
// method; framework commands don't register at all).
if isShellOut && !f.hasReadOnly && readShapedName(name) {
out = append(out, ToolsAuditFinding{
- Kind: "missing-read-only", Command: name, File: file, Line: line,
+ Kind: kindMissingReadOnly, Command: name, File: file, Line: line,
Evidence: "name matches read heuristic; no mcp:read-only annotation",
})
}
@@ -405,50 +436,333 @@ func readShapedName(name string) bool {
return ok
}
-func renderToolsAuditTable(w io.Writer, findings []ToolsAuditFinding, delta ledgerDelta) {
+// completionStatus carries the three load-bearing gates plus a
+// resume hint. The gates distinguish a complete polish run from one
+// that "looks done" via count-based pending alone. See AGENTS.md
+// "Deterministic Inventory + Agent-Marked Ledger" for the rationale.
+type completionStatus struct {
+ IncompleteAccepts []ToolsAuditFinding // accepted but pre-decision fields missing
+ DuplicateRationaleGroups []rationaleGroup // accepts that share a normalized note
+ ScorecardDeltaIssue *scorecardDeltaIssue // accepted MCP-desc findings without score lift
+ NextPending *ToolsAuditFinding // resume hint
+}
+
+// rationaleGroup is one cluster of accepted findings whose normalized
+// notes match. The threshold is duplicateRationaleThreshold; groups
+// below it are not surfaced (the agent gets some natural overlap).
+type rationaleGroup struct {
+ Rationale string // the normalized note text
+ Findings []ToolsAuditFinding // accepted findings sharing this rationale
+}
+
+// scorecardDeltaIssue describes the scorecard-delta gate firing: the
+// run accepted N thin-mcp-description findings but MCPDescriptionQuality
+// did not improve. Either the accepts were unwarranted (the dimension
+// would have lifted with overrides) or the dimension is mis-scored.
+// Surface both numbers so the agent can debug.
+type scorecardDeltaIssue struct {
+ Before int
+ After int
+ AcceptedThinMCP int
+}
+
+// evaluateCompletion runs the three gates plus the resume hint
+// against the current findings. The completionStatus carries gate
+// failures (IncompleteAccepts, DuplicateRationaleGroups,
+// ScorecardDeltaIssue) and a NextPending hint; gate-failure fields
+// being empty + no pending entries means the run is complete.
+func evaluateCompletion(manifest *pipeline.ToolsManifest, findings []ToolsAuditFinding, previous *ToolsAuditLedger) completionStatus {
+ var c completionStatus
+ for _, f := range findings {
+ if f.Status != statusAccepted {
+ continue
+ }
+ if requiresPreDecisionFields(f.Kind) && missingPreDecisionFields(f) {
+ c.IncompleteAccepts = append(c.IncompleteAccepts, f)
+ }
+ }
+ c.DuplicateRationaleGroups = detectDuplicateRationales(findings, duplicateRationaleThreshold)
+ c.ScorecardDeltaIssue = checkScorecardDelta(manifest, findings, previous)
+ c.NextPending = nextPendingFinding(findings, previous)
+ return c
+}
+
+// requiresPreDecisionFields gates which finding kinds force the
+// SpecSourceMaterial/TargetDescription/GapAnalysis trio when
+// accepted. The Cobra-side kinds have legitimate uniform accept
+// patterns (DO-NOT-EDIT generated files) that would be noise to
+// gate; the MCP-description kinds are the ones bulk-accept with a
+// generic rationale historically masked.
+func requiresPreDecisionFields(kind string) bool {
+ return kind == kindThinMCPDesc || kind == kindEmptyMCPDesc
+}
+
+func missingPreDecisionFields(f ToolsAuditFinding) bool {
+ return strings.TrimSpace(f.SpecSourceMaterial) == "" ||
+ strings.TrimSpace(f.TargetDescription) == "" ||
+ strings.TrimSpace(f.GapAnalysis) == ""
+}
+
+// detectDuplicateRationales finds clusters of accepted findings whose
+// normalized notes match. Normalization is lowercase + collapsed
+// whitespace so "Systemic to OpenAPI specs" and "systemic to openapi
+// specs" cluster. Groups under the threshold are dropped — some
+// natural overlap is fine; we're guarding against the punt pattern,
+// not against any duplication.
+func detectDuplicateRationales(findings []ToolsAuditFinding, threshold int) []rationaleGroup {
+ if threshold <= 0 {
+ return nil
+ }
+ clusters := make(map[string][]ToolsAuditFinding)
+ for _, f := range findings {
+ if f.Status != statusAccepted {
+ continue
+ }
+ key := normalizeRationale(f.Note)
+ if key == "" {
+ continue
+ }
+ clusters[key] = append(clusters[key], f)
+ }
+ var groups []rationaleGroup
+ for key, fs := range clusters {
+ if len(fs) > threshold {
+ groups = append(groups, rationaleGroup{Rationale: key, Findings: fs})
+ }
+ }
+ sort.Slice(groups, func(i, j int) bool {
+ return len(groups[i].Findings) > len(groups[j].Findings)
+ })
+ return groups
+}
+
+func normalizeRationale(s string) string {
+ return strings.ToLower(strings.Join(strings.Fields(s), " "))
+}
+
+// checkScorecardDelta compares ScorecardBefore (captured at run start)
+// against the current MCPDescriptionQuality score. Fires only when
+// there are accepted thin-mcp-description findings — accepting other
+// kinds doesn't entail score movement, so skipping the gate for those
+// avoids false positives.
+//
+// Returns nil when ScorecardBefore is absent (older ledger, or CLI
+// with no manifest) or when there are no accepted thin-mcp findings.
+// Both conditions mean the gate has nothing to enforce.
+func checkScorecardDelta(manifest *pipeline.ToolsManifest, findings []ToolsAuditFinding, previous *ToolsAuditLedger) *scorecardDeltaIssue {
+ if previous == nil || previous.ScorecardBefore == nil {
+ return nil
+ }
+ var acceptedThinMCP int
+ for _, f := range findings {
+ if f.Status == statusAccepted && f.Kind == kindThinMCPDesc {
+ acceptedThinMCP++
+ }
+ }
+ if acceptedThinMCP == 0 {
+ return nil
+ }
+ current, scored := pipeline.ScoreMCPDescriptionQualityForManifest(manifest)
+ if !scored {
+ return nil
+ }
+ if current > previous.ScorecardBefore.MCPDescriptionQuality {
+ return nil
+ }
+ return &scorecardDeltaIssue{
+ Before: previous.ScorecardBefore.MCPDescriptionQuality,
+ After: current,
+ AcceptedThinMCP: acceptedThinMCP,
+ }
+}
+
+// isPending reports whether a finding still needs the agent's
+// attention. Either status is unset, or status is "accepted" on a
+// kind that requires pre-decision fields and the fields aren't
+// populated — an accepted-but-incomplete entry, which the gates
+// surface and which counts as pending for the summary line.
+func isPending(f ToolsAuditFinding) bool {
+ if f.Status != statusAccepted {
+ return true
+ }
+ return requiresPreDecisionFields(f.Kind) && missingPreDecisionFields(f)
+}
+
+// nextPendingFinding returns the resume hint: the first finding that
+// isPending. The previous ledger's Progress.LastProcessedFindingID is
+// read as a soft hint — when present, the search starts after that
+// finding so a re-invocation resumes mid-walk; if exhausted or
+// missing, falls back to a head-to-tail scan.
+func nextPendingFinding(findings []ToolsAuditFinding, previous *ToolsAuditLedger) *ToolsAuditFinding {
+ startIdx := 0
+ if previous != nil && previous.Progress != nil && previous.Progress.LastProcessedFindingID != "" {
+ for i, f := range findings {
+ if findingKey(f) == previous.Progress.LastProcessedFindingID {
+ startIdx = i + 1
+ break
+ }
+ }
+ }
+ if f := firstPending(findings, startIdx, len(findings)); f != nil {
+ return f
+ }
+ return firstPending(findings, 0, startIdx)
+}
+
+// firstPending returns the first isPending finding in findings[lo:hi]
+// or nil. Extracted so nextPendingFinding's forward + fallback scans
+// share the same predicate.
+func firstPending(findings []ToolsAuditFinding, lo, hi int) *ToolsAuditFinding {
+ if lo < 0 {
+ lo = 0
+ }
+ if hi > len(findings) {
+ hi = len(findings)
+ }
+ for i := lo; i < hi; i++ {
+ if isPending(findings[i]) {
+ return &findings[i]
+ }
+ }
+ return nil
+}
+
+func renderToolsAuditTable(w io.Writer, findings []ToolsAuditFinding, delta ledgerDelta, completion completionStatus) {
var pending, accepted int
for _, f := range findings {
- if f.Status == statusAccepted {
- accepted++
- } else {
+ if isPending(f) {
pending++
+ } else {
+ accepted++
}
}
- if pending == 0 {
+ gateFired := completion.hasGateFailure()
+
+ switch {
+ case pending == 0 && !gateFired:
if accepted > 0 {
fmt.Fprintf(w, "tools-audit: no pending findings (%d accepted)\n", accepted)
} else {
fmt.Fprintln(w, "tools-audit: no findings")
}
- if delta.hasPrevious && len(delta.resolved) > 0 {
- fmt.Fprintf(w, "since last run: %d resolved, 0 new\n", len(delta.resolved))
+ case pending == 0 && gateFired:
+ fmt.Fprintf(w, "tools-audit: incomplete (%d accepted, %d gate failure(s))\n", accepted, completion.gateFailureCount())
+ default:
+ fmt.Fprintf(w, "tools-audit: %d pending finding(s)", pending)
+ if accepted > 0 {
+ fmt.Fprintf(w, " (%d accepted)", accepted)
}
- return
- }
- fmt.Fprintf(w, "tools-audit: %d pending finding(s)", pending)
- if accepted > 0 {
- fmt.Fprintf(w, " (%d accepted)", accepted)
+ if gateFired {
+ fmt.Fprintf(w, ", %d gate failure(s)", completion.gateFailureCount())
+ }
+ fmt.Fprintln(w)
}
- fmt.Fprintln(w)
+
if delta.hasPrevious {
fmt.Fprintf(w, "since last run: %d resolved, %d new\n", len(delta.resolved), len(delta.added))
}
- fmt.Fprintln(w)
- fmt.Fprintf(w, "%-20s %-15s %-30s %s\n", "KIND", "COMMAND", "FILE:LINE", "EVIDENCE")
- for _, f := range findings {
- if f.Status == statusAccepted {
- continue
+
+ renderCompletionGates(w, completion)
+
+ if pending > 0 {
+ fmt.Fprintln(w)
+ fmt.Fprintf(w, "%-20s %-15s %-30s %s\n", "KIND", "COMMAND", "FILE:LINE", "EVIDENCE")
+ for _, f := range findings {
+ if !isPending(f) {
+ continue
+ }
+ loc := fmt.Sprintf("%s:%d", f.File, f.Line)
+ fmt.Fprintf(w, "%-20s %-15s %-30s %s\n", f.Kind, f.Command, loc, f.Evidence)
}
+ }
+
+ if completion.NextPending != nil {
+ f := completion.NextPending
loc := fmt.Sprintf("%s:%d", f.File, f.Line)
- fmt.Fprintf(w, "%-20s %-15s %-30s %s\n", f.Kind, f.Command, loc, f.Evidence)
+ fmt.Fprintln(w)
+ fmt.Fprintf(w, "next: %s on %s (%s)\n", f.Kind, f.Command, loc)
+ }
+}
+
+// gateFailureCount returns the number of gates surfacing an issue the
+// agent has to act on. NextPending is a resume hint, not a gate, and
+// is excluded.
+func (c completionStatus) gateFailureCount() int {
+ n := 0
+ if len(c.IncompleteAccepts) > 0 {
+ n++
+ }
+ if len(c.DuplicateRationaleGroups) > 0 {
+ n++
+ }
+ if c.ScorecardDeltaIssue != nil {
+ n++
+ }
+ return n
+}
+
+func (c completionStatus) hasGateFailure() bool {
+ return c.gateFailureCount() > 0
+}
+
+// renderCompletionGates surfaces each fired gate with the specific
+// reason and the entries the agent should revisit. Silent when no
+// gate fired.
+func renderCompletionGates(w io.Writer, c completionStatus) {
+ if !c.hasGateFailure() {
+ return
+ }
+ fmt.Fprintln(w)
+ fmt.Fprintln(w, "incomplete: the run is not done yet")
+ if len(c.IncompleteAccepts) > 0 {
+ fmt.Fprintf(w, " - %d accepted finding(s) missing pre-decision fields (spec_source_material, target_description, gap_analysis):\n", len(c.IncompleteAccepts))
+ for _, f := range c.IncompleteAccepts {
+ fmt.Fprintf(w, " %s on %s (%s:%d)\n", f.Kind, f.Command, f.File, f.Line)
+ }
+ }
+ for _, g := range c.DuplicateRationaleGroups {
+ fmt.Fprintf(w, " - %d accepted finding(s) share rationale %q — differentiate per item or rewrite:\n", len(g.Findings), truncate(g.Rationale, 60))
+ for _, f := range g.Findings {
+ fmt.Fprintf(w, " %s on %s (%s:%d)\n", f.Kind, f.Command, f.File, f.Line)
+ }
+ }
+ if d := c.ScorecardDeltaIssue; d != nil {
+ fmt.Fprintf(w, " - MCPDescriptionQuality unchanged (%d/10 → %d/10) but %d thin-mcp-description finding(s) accepted; either write overrides or accepts are unwarranted\n", d.Before, d.After, d.AcceptedThinMCP)
}
}
// ToolsAuditLedger is the on-disk snapshot of the last audit run.
+//
+// ScorecardBefore is captured on the first audit run that finds no
+// existing ledger and preserved across subsequent runs. It anchors the
+// scorecard-delta gate: a polish run that "completes" with accepted
+// thin-mcp-description findings but no movement in
+// MCPDescriptionQuality is incomplete by definition.
+//
+// Progress is an optional checkpoint the polish skill writes after
+// deliberating on each finding. A re-invocation after a context flush
+// reads it to resume mid-walk. When absent, the resume hint is the
+// first pending finding in scan order.
type ToolsAuditLedger struct {
- Timestamp time.Time `json:"timestamp"`
- CLIDir string `json:"cli_dir"`
- Findings []ToolsAuditFinding `json:"findings"`
+ Timestamp time.Time `json:"timestamp"`
+ CLIDir string `json:"cli_dir"`
+ Findings []ToolsAuditFinding `json:"findings"`
+ ScorecardBefore *ScorecardSnapshot `json:"scorecard_before,omitempty"`
+ Progress *PolishProgress `json:"progress,omitempty"`
+}
+
+// ScorecardSnapshot captures the scorecard dimensions the polish
+// ledger gates on, taken at run start.
+type ScorecardSnapshot struct {
+ MCPDescriptionQuality int `json:"mcp_description_quality"`
+ Captured time.Time `json:"captured"`
+}
+
+// PolishProgress is the agent-written checkpoint for resume after
+// context loss. The binary derives a fallback resume hint from the
+// finding list when this is absent, so the field is optional.
+type PolishProgress struct {
+ LastProcessedFindingID string `json:"last_processed_finding_id,omitempty"`
}
type ledgerDelta struct {
@@ -481,12 +795,23 @@ func readPreviousLedger(cliDir string) *ToolsAuditLedger {
return &l
}
-func writeLedger(cliDir string, findings []ToolsAuditFinding) error {
+func writeLedger(cliDir string, manifest *pipeline.ToolsManifest, findings []ToolsAuditFinding, previous *ToolsAuditLedger) error {
ledger := ToolsAuditLedger{
Timestamp: time.Now().UTC(),
CLIDir: cliDir,
Findings: findings,
}
+ // ScorecardBefore is sticky: captured on the first run that has no
+ // existing ledger, preserved on every subsequent run. Anchoring the
+ // scorecard-delta gate, so re-running must not rebase the baseline.
+ if previous != nil && previous.ScorecardBefore != nil {
+ ledger.ScorecardBefore = previous.ScorecardBefore
+ } else {
+ ledger.ScorecardBefore = captureScorecardSnapshot(manifest)
+ }
+ if previous != nil && previous.Progress != nil {
+ ledger.Progress = previous.Progress
+ }
data, err := json.MarshalIndent(ledger, "", " ")
if err != nil {
return fmt.Errorf("encoding ledger: %w", err)
@@ -495,6 +820,20 @@ func writeLedger(cliDir string, findings []ToolsAuditFinding) error {
return os.WriteFile(filepath.Join(cliDir, ledgerFilename), data, 0644)
}
+// captureScorecardSnapshot reads the dimensions the ledger gates on.
+// Returns nil when no dimension can be scored (e.g., no manifest) so
+// the gate has nothing to enforce.
+func captureScorecardSnapshot(manifest *pipeline.ToolsManifest) *ScorecardSnapshot {
+ score, scored := pipeline.ScoreMCPDescriptionQualityForManifest(manifest)
+ if !scored {
+ return nil
+ }
+ return &ScorecardSnapshot{
+ MCPDescriptionQuality: score,
+ Captured: time.Now().UTC(),
+ }
+}
+
// reconcileWithLedger carries Status/Note from the previous ledger
// onto matching current findings (so accept decisions survive re-runs)
// and computes the resolved/added delta in a single pass. Identity is
@@ -516,6 +855,9 @@ func reconcileWithLedger(previous *ToolsAuditLedger, current []ToolsAuditFinding
if old, ok := prev[k]; ok {
current[i].Status = old.Status
current[i].Note = old.Note
+ current[i].SpecSourceMaterial = old.SpecSourceMaterial
+ current[i].TargetDescription = old.TargetDescription
+ current[i].GapAnalysis = old.GapAnalysis
} else {
delta.added = append(delta.added, current[i])
}
diff --git a/internal/cli/tools_audit_test.go b/internal/cli/tools_audit_test.go
new file mode 100644
index 00000000..ff66ed9d
--- /dev/null
+++ b/internal/cli/tools_audit_test.go
@@ -0,0 +1,461 @@
+package cli
+
+import (
+ "bytes"
+ "strings"
+ "testing"
+ "time"
+
+ "github.com/mvanhorn/cli-printing-press/v2/internal/pipeline"
+)
+
+func TestRequiresPreDecisionFields(t *testing.T) {
+ tests := []struct {
+ kind string
+ want bool
+ }{
+ {"thin-mcp-description", true},
+ {"empty-mcp-description", true},
+ {"thin-short", false},
+ {"empty-short", false},
+ {"missing-read-only", false},
+ {"unknown", false},
+ {"", false},
+ }
+ for _, tc := range tests {
+ if got := requiresPreDecisionFields(tc.kind); got != tc.want {
+ t.Errorf("requiresPreDecisionFields(%q) = %v, want %v", tc.kind, got, tc.want)
+ }
+ }
+}
+
+func TestMissingPreDecisionFields(t *testing.T) {
+ full := ToolsAuditFinding{
+ SpecSourceMaterial: "summary + 3 params",
+ TargetDescription: "Create a tag with name + color",
+ GapAnalysis: "generator can compose this from spec",
+ }
+ tests := []struct {
+ name string
+ f ToolsAuditFinding
+ want bool
+ }{
+ {"all populated", full, false},
+ {"missing spec", ToolsAuditFinding{TargetDescription: "x", GapAnalysis: "y"}, true},
+ {"missing target", ToolsAuditFinding{SpecSourceMaterial: "x", GapAnalysis: "y"}, true},
+ {"missing gap", ToolsAuditFinding{SpecSourceMaterial: "x", TargetDescription: "y"}, true},
+ {"all empty", ToolsAuditFinding{}, true},
+ {"whitespace only", ToolsAuditFinding{SpecSourceMaterial: " ", TargetDescription: "\t", GapAnalysis: "\n"}, true},
+ }
+ for _, tc := range tests {
+ t.Run(tc.name, func(t *testing.T) {
+ if got := missingPreDecisionFields(tc.f); got != tc.want {
+ t.Errorf("missingPreDecisionFields() = %v, want %v", got, tc.want)
+ }
+ })
+ }
+}
+
+func TestNormalizeRationale(t *testing.T) {
+ tests := []struct {
+ in, want string
+ }{
+ {"Systemic to OpenAPI specs", "systemic to openapi specs"},
+ {"systemic to openapi specs", "systemic to openapi specs"},
+ {"\tSystemic\nto\topenapi specs ", "systemic to openapi specs"},
+ {"", ""},
+ {" ", ""},
+ }
+ for _, tc := range tests {
+ if got := normalizeRationale(tc.in); got != tc.want {
+ t.Errorf("normalizeRationale(%q) = %q, want %q", tc.in, got, tc.want)
+ }
+ }
+}
+
+func TestDetectDuplicateRationales(t *testing.T) {
+ mk := func(cmd, status, note string) ToolsAuditFinding {
+ return ToolsAuditFinding{
+ Kind: "thin-mcp-description", Command: cmd, File: "tools-manifest.json",
+ Status: status, Note: note,
+ }
+ }
+
+ t.Run("no accepts returns nil", func(t *testing.T) {
+ findings := []ToolsAuditFinding{
+ mk("a", "", ""), mk("b", "", ""),
+ }
+ if got := detectDuplicateRationales(findings, 5); got != nil {
+ t.Errorf("got %v, want nil", got)
+ }
+ })
+
+ t.Run("under threshold returns nil", func(t *testing.T) {
+ var findings []ToolsAuditFinding
+ for i := range 5 {
+ findings = append(findings, mk("c"+string(rune('0'+i)), statusAccepted, "same rationale"))
+ }
+ if got := detectDuplicateRationales(findings, 5); got != nil {
+ t.Errorf("got %v, want nil — exactly threshold should not fire", got)
+ }
+ })
+
+ t.Run("over threshold surfaces group", func(t *testing.T) {
+ var findings []ToolsAuditFinding
+ for i := range 6 {
+ findings = append(findings, mk("c"+string(rune('0'+i)), statusAccepted, "Systemic to OpenAPI specs"))
+ }
+ groups := detectDuplicateRationales(findings, 5)
+ if len(groups) != 1 {
+ t.Fatalf("got %d groups, want 1", len(groups))
+ }
+ if groups[0].Rationale != "systemic to openapi specs" {
+ t.Errorf("got rationale %q, want normalized form", groups[0].Rationale)
+ }
+ if len(groups[0].Findings) != 6 {
+ t.Errorf("got %d findings in group, want 6", len(groups[0].Findings))
+ }
+ })
+
+ t.Run("normalization clusters case and whitespace variants", func(t *testing.T) {
+ findings := []ToolsAuditFinding{
+ mk("a", statusAccepted, "Systemic to OpenAPI specs"),
+ mk("b", statusAccepted, "systemic to openapi specs"),
+ mk("c", statusAccepted, "SYSTEMIC TO OPENAPI SPECS"),
+ mk("d", statusAccepted, " Systemic to OpenAPI specs "),
+ mk("e", statusAccepted, "systemic to openapi specs"),
+ mk("f", statusAccepted, "Systemic\tto\tOpenAPI\tspecs"),
+ }
+ groups := detectDuplicateRationales(findings, 5)
+ if len(groups) != 1 || len(groups[0].Findings) != 6 {
+ t.Errorf("expected 1 cluster of 6, got %+v", groups)
+ }
+ })
+
+ t.Run("empty notes are ignored", func(t *testing.T) {
+ var findings []ToolsAuditFinding
+ for i := range 10 {
+ findings = append(findings, mk("c"+string(rune('0'+i)), statusAccepted, ""))
+ }
+ if got := detectDuplicateRationales(findings, 5); got != nil {
+ t.Errorf("got %v, want nil — empty notes should not cluster", got)
+ }
+ })
+
+ t.Run("multiple groups sorted by size descending", func(t *testing.T) {
+ var findings []ToolsAuditFinding
+ for i := range 6 {
+ findings = append(findings, mk("a"+string(rune('0'+i)), statusAccepted, "rationale one"))
+ }
+ for i := range 8 {
+ findings = append(findings, mk("b"+string(rune('0'+i)), statusAccepted, "rationale two"))
+ }
+ groups := detectDuplicateRationales(findings, 5)
+ if len(groups) != 2 {
+ t.Fatalf("got %d groups, want 2", len(groups))
+ }
+ if len(groups[0].Findings) != 8 {
+ t.Errorf("first group should be larger; got %d", len(groups[0].Findings))
+ }
+ })
+}
+
+func TestNextPendingFinding(t *testing.T) {
+ mk := func(cmd, kind, status string, hasFields bool) ToolsAuditFinding {
+ f := ToolsAuditFinding{
+ Kind: kind, Command: cmd, File: "tools-manifest.json",
+ Line: 0, Evidence: "evidence-" + cmd, Status: status,
+ }
+ if hasFields {
+ f.SpecSourceMaterial = "x"
+ f.TargetDescription = "y"
+ f.GapAnalysis = "z"
+ }
+ return f
+ }
+
+ t.Run("returns first pending in scan order", func(t *testing.T) {
+ findings := []ToolsAuditFinding{
+ mk("a", "thin-mcp-description", statusAccepted, true),
+ mk("b", "thin-mcp-description", "", false),
+ mk("c", "thin-mcp-description", "", false),
+ }
+ got := nextPendingFinding(findings, nil)
+ if got == nil || got.Command != "b" {
+ t.Errorf("got %+v, want command b", got)
+ }
+ })
+
+ t.Run("returns nil when fully accepted with fields", func(t *testing.T) {
+ findings := []ToolsAuditFinding{
+ mk("a", "thin-mcp-description", statusAccepted, true),
+ mk("b", "thin-mcp-description", statusAccepted, true),
+ }
+ if got := nextPendingFinding(findings, nil); got != nil {
+ t.Errorf("got %+v, want nil", got)
+ }
+ })
+
+ t.Run("treats accepted-without-fields as pending", func(t *testing.T) {
+ findings := []ToolsAuditFinding{
+ mk("a", "thin-mcp-description", statusAccepted, true),
+ mk("b", "thin-mcp-description", statusAccepted, false), // accepted without fields
+ mk("c", "thin-mcp-description", "", false),
+ }
+ got := nextPendingFinding(findings, nil)
+ if got == nil || got.Command != "b" {
+ t.Errorf("got %+v, want command b (accepted without fields counts as pending)", got)
+ }
+ })
+
+ t.Run("non-mcp accepted without fields is not pending", func(t *testing.T) {
+ findings := []ToolsAuditFinding{
+ mk("a", "thin-short", statusAccepted, false), // accepted without fields, not gated
+ mk("b", "thin-short", "", false),
+ }
+ got := nextPendingFinding(findings, nil)
+ if got == nil || got.Command != "b" {
+ t.Errorf("got %+v, want command b (a is not gated since thin-short)", got)
+ }
+ })
+
+ t.Run("progress checkpoint resumes after last processed", func(t *testing.T) {
+ findings := []ToolsAuditFinding{
+ mk("a", "thin-mcp-description", "", false),
+ mk("b", "thin-mcp-description", statusAccepted, true),
+ mk("c", "thin-mcp-description", "", false),
+ }
+ previous := &ToolsAuditLedger{
+ Progress: &PolishProgress{LastProcessedFindingID: findingKey(findings[1])},
+ }
+ got := nextPendingFinding(findings, previous)
+ if got == nil || got.Command != "c" {
+ t.Errorf("got %+v, want command c (skipped a because checkpoint was at b)", got)
+ }
+ })
+
+ t.Run("progress past everything falls back to head scan", func(t *testing.T) {
+ findings := []ToolsAuditFinding{
+ mk("a", "thin-mcp-description", "", false),
+ mk("b", "thin-mcp-description", statusAccepted, true),
+ }
+ previous := &ToolsAuditLedger{
+ Progress: &PolishProgress{LastProcessedFindingID: findingKey(findings[1])},
+ }
+ got := nextPendingFinding(findings, previous)
+ if got == nil || got.Command != "a" {
+ t.Errorf("got %+v, want command a (fallback to head when checkpoint exhausted forward path)", got)
+ }
+ })
+
+ t.Run("missing checkpoint key falls back to head scan", func(t *testing.T) {
+ findings := []ToolsAuditFinding{
+ mk("a", "thin-mcp-description", "", false),
+ }
+ previous := &ToolsAuditLedger{
+ Progress: &PolishProgress{LastProcessedFindingID: "nonexistent:0:thin-mcp-description:gone:gone"},
+ }
+ got := nextPendingFinding(findings, previous)
+ if got == nil || got.Command != "a" {
+ t.Errorf("got %+v, want command a (stale checkpoint shouldn't skip pending)", got)
+ }
+ })
+}
+
+func TestCheckScorecardDelta(t *testing.T) {
+ mkLedger := func(before int, hasSnap bool) *ToolsAuditLedger {
+ l := &ToolsAuditLedger{}
+ if hasSnap {
+ l.ScorecardBefore = &ScorecardSnapshot{
+ MCPDescriptionQuality: before,
+ Captured: time.Now(),
+ }
+ }
+ return l
+ }
+ mkFinding := func(kind, status string) ToolsAuditFinding {
+ return ToolsAuditFinding{Kind: kind, Status: status, File: "tools-manifest.json"}
+ }
+
+ // One thin + one rich = 50% thin = score 0 per the curve.
+ thinManifest := &pipeline.ToolsManifest{Tools: []pipeline.ManifestTool{
+ {Name: "a", Description: "Create x"},
+ {Name: "b", Description: "this description is long enough to be over the threshold for the test"},
+ }}
+ // All-rich manifest scores 10/10.
+ richManifest := &pipeline.ToolsManifest{Tools: []pipeline.ManifestTool{
+ {Name: "a", Description: "this is a sufficiently long and rich description for the test that exceeds threshold"},
+ }}
+
+ t.Run("nil previous returns nil", func(t *testing.T) {
+ got := checkScorecardDelta(thinManifest, []ToolsAuditFinding{mkFinding("thin-mcp-description", statusAccepted)}, nil)
+ if got != nil {
+ t.Errorf("got %+v, want nil", got)
+ }
+ })
+
+ t.Run("previous without snapshot returns nil", func(t *testing.T) {
+ got := checkScorecardDelta(thinManifest, []ToolsAuditFinding{mkFinding("thin-mcp-description", statusAccepted)}, mkLedger(0, false))
+ if got != nil {
+ t.Errorf("got %+v, want nil", got)
+ }
+ })
+
+ t.Run("no thin-mcp accepts returns nil", func(t *testing.T) {
+ got := checkScorecardDelta(thinManifest, []ToolsAuditFinding{mkFinding("thin-short", statusAccepted)}, mkLedger(3, true))
+ if got != nil {
+ t.Errorf("got %+v, want nil", got)
+ }
+ })
+
+ t.Run("score lifted returns nil", func(t *testing.T) {
+ got := checkScorecardDelta(richManifest, []ToolsAuditFinding{mkFinding("thin-mcp-description", statusAccepted)}, mkLedger(0, true))
+ if got != nil {
+ t.Errorf("got %+v, want nil (score lifted)", got)
+ }
+ })
+
+ t.Run("no lift with thin accepts fires", func(t *testing.T) {
+ before := mkLedger(0, true)
+ got := checkScorecardDelta(thinManifest, []ToolsAuditFinding{
+ mkFinding("thin-mcp-description", statusAccepted),
+ mkFinding("thin-mcp-description", statusAccepted),
+ }, before)
+ if got == nil {
+ t.Fatal("got nil, want issue")
+ }
+ if got.AcceptedThinMCP != 2 {
+ t.Errorf("AcceptedThinMCP = %d, want 2", got.AcceptedThinMCP)
+ }
+ if got.Before != 0 || got.After != 0 {
+ t.Errorf("got before=%d after=%d, want 0/0", got.Before, got.After)
+ }
+ })
+
+ t.Run("nil manifest returns nil", func(t *testing.T) {
+ got := checkScorecardDelta(nil, []ToolsAuditFinding{mkFinding("thin-mcp-description", statusAccepted)}, mkLedger(0, true))
+ if got != nil {
+ t.Errorf("got %+v, want nil (nil manifest, scorer unscored)", got)
+ }
+ })
+}
+
+func TestRenderCompletionGatesIncludesAllReasons(t *testing.T) {
+ c := completionStatus{
+ IncompleteAccepts: []ToolsAuditFinding{
+ {Kind: "thin-mcp-description", Command: "tags_create", File: "tools-manifest.json", Line: 0},
+ },
+ DuplicateRationaleGroups: []rationaleGroup{
+ {Rationale: "systemic to openapi specs", Findings: []ToolsAuditFinding{
+ {Kind: "thin-mcp-description", Command: "a", File: "tools-manifest.json"},
+ {Kind: "thin-mcp-description", Command: "b", File: "tools-manifest.json"},
+ }},
+ },
+ ScorecardDeltaIssue: &scorecardDeltaIssue{Before: 3, After: 3, AcceptedThinMCP: 5},
+ }
+ var buf bytes.Buffer
+ renderCompletionGates(&buf, c)
+ out := buf.String()
+ for _, want := range []string{
+ "incomplete: the run is not done yet",
+ "missing pre-decision fields",
+ "tags_create",
+ "share rationale",
+ "systemic to openapi specs",
+ "MCPDescriptionQuality unchanged",
+ "3/10 → 3/10",
+ "5 thin-mcp-description finding(s) accepted",
+ } {
+ if !strings.Contains(out, want) {
+ t.Errorf("output missing %q\n--- output ---\n%s", want, out)
+ }
+ }
+}
+
+func TestRenderCompletionGatesSilentWhenClean(t *testing.T) {
+ var buf bytes.Buffer
+ renderCompletionGates(&buf, completionStatus{})
+ if buf.Len() != 0 {
+ t.Errorf("expected silent output, got %q", buf.String())
+ }
+}
+
+func TestEvaluateCompletionAggregatesGates(t *testing.T) {
+ // Manifest with one thin description -> score = 0 (50% thin -> default tier).
+ manifest := &pipeline.ToolsManifest{Tools: []pipeline.ManifestTool{
+ {Name: "a", Description: "Create x"},
+ {Name: "b", Description: "this is a sufficiently long description for the test bench"},
+ }}
+ previous := &ToolsAuditLedger{
+ ScorecardBefore: &ScorecardSnapshot{MCPDescriptionQuality: 0, Captured: time.Now()},
+ }
+ // Build findings: one accepted thin-mcp without pre-decision fields
+ // (fires gate 1), six thin-mcp accepts sharing one rationale (fires
+ // gate 2), and the same thin-mcp accepts naturally fire gate 3
+ // (no score lift while accepting thin-mcp).
+ var findings []ToolsAuditFinding
+ for i := range 7 {
+ f := ToolsAuditFinding{
+ Kind: "thin-mcp-description", Command: "c" + string(rune('0'+i)),
+ File: "tools-manifest.json", Line: 0, Evidence: "thin",
+ Status: statusAccepted, Note: "Systemic to OpenAPI specs",
+ }
+ if i > 0 {
+ // Fill fields on all but the first to isolate gate 1
+ f.SpecSourceMaterial = "summary only"
+ f.TargetDescription = "a richer description"
+ f.GapAnalysis = "generator could compose more from spec"
+ }
+ findings = append(findings, f)
+ }
+
+ c := evaluateCompletion(manifest, findings, previous)
+
+ if len(c.IncompleteAccepts) != 1 {
+ t.Errorf("IncompleteAccepts = %d, want 1", len(c.IncompleteAccepts))
+ }
+ if len(c.DuplicateRationaleGroups) != 1 {
+ t.Errorf("DuplicateRationaleGroups = %d, want 1", len(c.DuplicateRationaleGroups))
+ } else if len(c.DuplicateRationaleGroups[0].Findings) != 7 {
+ t.Errorf("group size = %d, want 7", len(c.DuplicateRationaleGroups[0].Findings))
+ }
+ if c.ScorecardDeltaIssue == nil {
+ t.Error("ScorecardDeltaIssue = nil, want issue (no lift + accepts)")
+ } else if c.ScorecardDeltaIssue.AcceptedThinMCP != 7 {
+ t.Errorf("AcceptedThinMCP = %d, want 7", c.ScorecardDeltaIssue.AcceptedThinMCP)
+ }
+ // NextPending should point at the one with missing fields (c0).
+ if c.NextPending == nil || c.NextPending.Command != "c0" {
+ t.Errorf("NextPending = %+v, want c0", c.NextPending)
+ }
+
+ if !c.hasGateFailure() {
+ t.Error("hasGateFailure = false, want true")
+ }
+ if got := c.gateFailureCount(); got != 3 {
+ t.Errorf("gateFailureCount = %d, want 3", got)
+ }
+}
+
+func TestTruncate(t *testing.T) {
+ tests := []struct {
+ name string
+ in string
+ n int
+ want string
+ }{
+ {"under limit", "short", 10, "short"},
+ {"at limit", "exactly10!", 10, "exactly10!"},
+ {"truncated ascii", "a long string", 5, "a lo…"},
+ {"n=1 returns first rune", "abcdef", 1, "a"},
+ {"n=0 returns empty", "abc", 0, ""},
+ {"multibyte safe at boundary", "héllo wörld", 8, "héllo w…"},
+ }
+ for _, tc := range tests {
+ t.Run(tc.name, func(t *testing.T) {
+ if got := truncate(tc.in, tc.n); got != tc.want {
+ t.Errorf("truncate(%q, %d) = %q, want %q", tc.in, tc.n, got, tc.want)
+ }
+ })
+ }
+}
diff --git a/internal/pipeline/scorecard.go b/internal/pipeline/scorecard.go
index f159a675..32c2427f 100644
--- a/internal/pipeline/scorecard.go
+++ b/internal/pipeline/scorecard.go
@@ -698,24 +698,13 @@ func IsThinMCPDescription(desc string) bool {
return len(d) < MCPDescMinLen && len(strings.Fields(d)) < MCPDescMinWords
}
-// scoreMCPDescriptionQuality measures how many of a CLI's typed MCP
-// tools carry agent-grade descriptions vs. terse spec-derived
-// summaries. Reads tools-manifest.json (the source of truth for typed
-// endpoint tools' descriptions at runtime) and counts entries whose
-// description trips IsThinMCPDescription — the same predicate
-// `printing-press tools-audit` uses for thin-mcp-description findings.
-//
-// Unscored when no manifest exists (legacy CLIs predating the
-// manifest schema) or when the manifest has no tools.
-//
-// Score curve favors low thin-percentage steeply; the goal is to
-// reward CLIs that have done the work to provide rich descriptions
-// rather than to give partial credit to ones that haven't. CLIs whose
-// descriptions are 50%+ thin score 0 — there's no signal to credit
-// when most of the surface is unusable to an agent.
-func scoreMCPDescriptionQuality(dir string) (score int, scored bool) {
- m, err := ReadToolsManifest(dir)
- if err != nil || len(m.Tools) == 0 {
+// ScoreMCPDescriptionQualityForManifest scores an already-parsed
+// manifest. Callers that read tools-manifest.json for other reasons
+// in the same code path (e.g., printing-press tools-audit, which
+// emits findings from the manifest) call this variant to avoid
+// re-parsing.
+func ScoreMCPDescriptionQualityForManifest(m *ToolsManifest) (score int, scored bool) {
+ if m == nil || len(m.Tools) == 0 {
return 0, false
}
thin := 0
@@ -741,6 +730,29 @@ func scoreMCPDescriptionQuality(dir string) (score int, scored bool) {
}
}
+// scoreMCPDescriptionQuality measures how many of a CLI's typed MCP
+// tools carry agent-grade descriptions vs. terse spec-derived
+// summaries. Reads tools-manifest.json (the source of truth for typed
+// endpoint tools' descriptions at runtime) and counts entries whose
+// description trips IsThinMCPDescription — the same predicate
+// `printing-press tools-audit` uses for thin-mcp-description findings.
+//
+// Unscored when no manifest exists (legacy CLIs predating the
+// manifest schema) or when the manifest has no tools.
+//
+// Score curve favors low thin-percentage steeply; the goal is to
+// reward CLIs that have done the work to provide rich descriptions
+// rather than to give partial credit to ones that haven't. CLIs whose
+// descriptions are 50%+ thin score 0 — there's no signal to credit
+// when most of the surface is unusable to an agent.
+func scoreMCPDescriptionQuality(dir string) (score int, scored bool) {
+ m, err := ReadToolsManifest(dir)
+ if err != nil {
+ return 0, false
+ }
+ return ScoreMCPDescriptionQualityForManifest(m)
+}
+
func scoreLocalCache(dir string) int {
clientContent := readFileContent(filepath.Join(dir, "internal", "client", "client.go"))
score := 0
diff --git a/skills/printing-press-polish/SKILL.md b/skills/printing-press-polish/SKILL.md
index 6bc1cc3d..9870c948 100644
--- a/skills/printing-press-polish/SKILL.md
+++ b/skills/printing-press-polish/SKILL.md
@@ -372,8 +372,9 @@ Stop and:
1. Run `printing-press tools-audit "$CLI_DIR" --json` to surface mechanical findings (empty Short, thin Short, missing `mcp:read-only` on read-shaped command names).
2. You must read `references/tools-polish.md` and follow its instructions to address the findings AND run a judgment pass over every command — regardless of whether the audit flagged it. The audit catches mechanical issues; description quality and borderline classification (read-only vs. local-write) always require agent reasoning. You must not skip this.
+3. **Accepting MCP-description findings carries a stricter contract.** `thin-mcp-description` and `empty-mcp-description` accepts require three pre-decision fields (`spec_source_material`, `target_description`, `gap_analysis`) populated per finding. The binary rejects bulk accepts (>5 findings sharing one rationale) and runs that "complete" without lifting MCPDescriptionQuality. Fix via override or generator improvement is the expected path; accept is rare. See `references/tools-polish.md` "Marking a finding accepted" for the full contract.
-Proceed to "After all fixes" only when audit findings are resolved AND every command's description has been evaluated against the playbook's agent-grade criteria.
+Proceed to "After all fixes" only when the audit's summary line reads `no pending findings` with no `incomplete:` block — every gate (pre-decision fields, duplicate rationale, scorecard delta) passes.
### After all fixes
diff --git a/skills/printing-press-polish/references/tools-polish.md b/skills/printing-press-polish/references/tools-polish.md
index 96b21d28..fc917d55 100644
--- a/skills/printing-press-polish/references/tools-polish.md
+++ b/skills/printing-press-polish/references/tools-polish.md
@@ -261,14 +261,16 @@ The sync regenerates `tools-manifest.json` and `internal/mcp/tools.go` with the
## Ledger and resumability
-`tools-audit` writes `<cli-dir>/.printing-press-tools-polish.json` after every run. It contains the timestamp, cli-dir, and one entry per finding.
+`tools-audit` writes `<cli-dir>/.printing-press-tools-polish.json` after every run. It contains the timestamp, cli-dir, one entry per finding, a `scorecard_before` snapshot captured on the first run, and an optional `progress` checkpoint.
1. **Delta computation.** On a second run within 24 hours, the audit prints `since last run: N resolved, M new`. Stale ledgers (>24h) are deleted automatically.
-2. **Resumability.** If your context window flushes mid-polish, re-run `tools-audit <cli-dir>`. Findings you've fixed have disappeared from the new scan; findings you accepted are still recorded with status. You pick up where you left off.
-3. **Audit trail of accept decisions.** When you decide a finding is fine as-is, you mark the entry `accepted` and write a one-sentence rationale. The next run filters it out of the pending table.
+2. **Resumability.** If your context window flushes mid-polish, re-run `tools-audit <cli-dir>`. Findings you've fixed have disappeared from the new scan; findings you accepted are still recorded with status. The render's `next:` line at the bottom names the next finding you owe a decision on. Update `progress.last_processed_finding_id` to the finding key you've just decided so a future re-run resumes after it instead of re-scanning from the head.
+3. **Audit trail of accept decisions.** When you decide a finding is fine as-is, you mark the entry `accepted` with a rationale. For `thin-mcp-description` and `empty-mcp-description` you must also fill the three pre-decision fields (see below). The next run filters the accepted entry out of the pending table only when the gate fields are populated; otherwise the binary surfaces it as an incomplete accept.
### Marking a finding accepted
+#### Cobra-side findings (`empty-short`, `thin-short`, `missing-read-only`)
+
When the table shows a finding that's actually fine, edit the ledger entry directly:
```json
@@ -283,22 +285,67 @@ When the table shows a finding that's actually fine, edit the ledger entry direc
}
```
-After marking, re-run `tools-audit <cli-dir>`. The pending table excludes the accepted entry; the header shows `(N accepted)` to confirm.
+The pending table excludes the accepted entry on the next run; the header shows `(N accepted)` to confirm.
+
+**Don't accept `empty-short` or `missing-read-only`.** They have no judgment call: empty descriptions always need text, and a read command always wants the annotation.
+
+#### MCP-description findings (`thin-mcp-description`, `empty-mcp-description`)
+
+These findings carry a stricter accept contract. The historical failure mode is bulk-accept: walking 200+ thin-description findings and stamping all of them with one rationale ("systemic to OpenAPI specs", "doesn't compound") instead of deliberating per item. The binary now refuses to count an accept as complete unless three pre-decision fields are populated:
+
+```json
+{
+ "kind": "thin-mcp-description",
+ "command": "tags_create",
+ "file": "tools-manifest.json",
+ "line": 0,
+ "evidence": "Create a new tag",
+ "status": "accepted",
+ "note": "Spec summary is the only source; an override would inflate every CLI without compounding",
-**Don't accept `empty-short`, `empty-mcp-description`, or `missing-read-only` findings.** They have no judgment call: empty descriptions always need text, and a read command always wants the annotation.
+ "spec_source_material": "summary='Create a new tag'; one required body param 'name' (string); response is the tag object with id, name, slug",
+ "target_description": "Create a new tag in the workspace. Required: name. Returns the tag's id and slug. Tags must exist before they can be assigned to links.",
+ "gap_analysis": "Generator could compose target from the spec's request and response schemas; today it only emits the bare summary. Override is the right path until #384 lifts the baseline."
+}
+```
-**Acceptance is rare for `thin-mcp-description`.** A typed endpoint tool whose description is so brief it tripped the heuristic almost certainly leaves the agent guessing. Accept only when the operation is genuinely trivial enough that more words don't help.
+Each field has a job:
+
+- **`spec_source_material`** — the honest answer to "what does the spec actually give us for this endpoint?" Quote from the source. Don't generalize ("specs are thin") — name the specific summary, parameters, request body, and response schema you read in `<cli-dir>/spec.json` or `spec.yaml`.
+- **`target_description`** — what a 10/10 description for this tool would say given the source material. Even when accepting the current state, naming the target makes the gap visible.
+- **`gap_analysis`** — why the generator can't produce `target_description` from `spec_source_material` today. This is the load-bearing field: it forces you to reason about whether the gap is genuinely systemic (file as a generator improvement, e.g., #384) or per-finding (write an override).
+
+If you can't fill all three honestly, you don't yet understand the finding well enough to accept it. Write the override instead, or do the spec reading required to fill the fields.
+
+**Forbidden accept patterns:**
+
+- "Systemic to OpenAPI specs", "doesn't compound", "out of scope for polish", or any rationale you'd stamp identically across many findings. The binary clusters accepts by normalized note text and refuses runs where any cluster exceeds 5 entries.
+- Accept-without-reading-spec. The pre-decision fields require the spec source material — guess and the gap analysis will read like generic prose.
+- Accept-and-move-on without writing the corresponding generator-improvement task. If you're filing the same `gap_analysis` repeatedly, that's evidence the generator should produce the target instead; surface it in the polish summary as a retro candidate.
+
+**Acceptance should be rare here.** The expected steady state is "fix it via override" or "fix it via generator improvement," not "accept it." See the override path under `thin-mcp-description` above.
### Don't manually mark findings as fixed
-If you fixed a finding via code change, do nothing in the ledger. The next audit re-scans the source and the manifest; finding gone → finding gone from the table → delta line shows `1 resolved`. Never set `status: "fixed"` by hand.
+If you fixed a finding via code change or override, do nothing in the ledger. The next audit re-scans the source and the manifest; finding gone → finding gone from the table → delta line shows `1 resolved`. Never set `status: "fixed"` by hand.
+
+### Completion gates the binary enforces
+
+The audit's "complete" signal is no longer just "zero pending." Four gates must pass:
+
+1. **Pre-decision fields gate.** Every accepted `thin-mcp-description` / `empty-mcp-description` entry has `spec_source_material`, `target_description`, and `gap_analysis` populated. Missing fields surface as `incomplete: N accepted finding(s) missing pre-decision fields` with each entry's `kind`/`command`/`file:line`.
+2. **Duplicate-rationale gate.** No more than 5 accepted findings share the same normalized `note` text. The audit clusters notes by lowercase + collapsed whitespace, so paraphrasing without changing meaning doesn't work — differentiate per item or rewrite both. Surfaces as `incomplete: N accepted finding(s) share rationale "..."`.
+3. **Scorecard-delta gate.** When `MCPDescriptionQuality` does not move from `scorecard_before` to current AND the run accepted any thin-mcp-description findings, the run is incomplete. Either the accepts were unwarranted (you owed an override that would have lifted the score) or the dimension is mis-scored (rare, surface to retro). Surfaces as `incomplete: MCPDescriptionQuality unchanged (X/10 → X/10) but N thin-mcp-description finding(s) accepted`.
+4. **Resume hint.** The render's `next:` line at the bottom names the first pending finding (status unset, or accepted-without-fields). Update `progress.last_processed_finding_id` after each decision so a re-run after compaction picks up where you left off.
+
+A run is complete only when the summary line reads `tools-audit: no pending findings (N accepted)` with no `incomplete:` block printed.
## Verification checklist
After applying fixes, before declaring the polish complete:
- [ ] `go build ./...` clean (annotations don't break compilation)
-- [ ] `printing-press tools-audit <cli-dir>` shows zero pending findings — every finding is either fixed (auto-removed) or explicitly accepted with a `note`
+- [ ] `printing-press tools-audit <cli-dir>` summary line reads `no pending findings`. No `incomplete:` block — every gate (pre-decision fields, duplicate rationale, scorecard delta) passes.
- [ ] `printing-press dogfood --dir <cli-dir>` reports `MCP Surface: PASS`
- [ ] If `mcp-descriptions.json` was edited, `printing-press mcp-sync <cli-dir>` was run to apply the overrides into `tools-manifest.json` and `internal/mcp/tools.go`. Re-run `tools-audit` after the sync to confirm thin-mcp-description findings disappeared.
- [ ] If commands were renamed or had their annotations restructured, smoke-test the binary by inspecting `--help` output for the affected commands
← e651d01f feat(cli): MCP description overrides + tools-audit scoring +
·
back to Cli Printing Press
·
fix(cli,skills): manifest display_name preservation + diverg 401bda54 →