← back to Cli Printing Press
fix(cli): machine improvements from hackernews retro #350 (#352)
0ed2fbd809bf93c386022647f583fe6a12abd761 · 2026-04-27 15:09:47 -0700 · Trevin Chow
* fix(cli): trim phantom freshness paths in rendered docs
freshnessCommandPaths() unconditionally emitted "<resource> list/get/search"
variants for every syncable resource regardless of whether those subcommands
actually existed on the resource. README and SKILL.md rendered these
verbatim under "Covered command paths," misleading users and agents
about the command tree.
Now the rendered slice consults endpoint existence per resource:
- Promoted single-endpoint resources emit only "<cli> <resource>" — Cobra
resolves it directly to the leaf command.
- Multi-endpoint resources emit the bare form plus one entry per real
endpoint name from spec.Resources[name].Endpoints.
The runtime fallback map in auto_refresh.go keeps its no-op variants —
those help Cobra arg resolution at runtime and are harmless. Only the
slice rendered into user-facing docs is trimmed.
Surfaced by hackernews retro #350 finding F1.
Includes the parent plan document covering all 8 retro work units.
* fix(cli): reject ASCII control bytes in rendered SKILL/README
Extend validateRenderedArtifact (which already runs on README.md and
SKILL.md) to scan the rendered output for ASCII control bytes outside
the small set markdown legitimately uses (tab, LF, CR). On rejection,
surface the file path, byte offset, and a hint pointing at the most
likely cause: a JSON-parsed field containing an unescaped backslash
sequence like "\b" or "\f" that the JSON parser turned into a real
control byte.
Backstory: an agent author writes `"command": "...\bGo\b..."` in
research.json's narrative.recipes intending the regex literal
`\bGo\b`. encoding/json correctly parses `\b` as backspace (0x08),
the template engine writes those bytes straight into SKILL.md, and
the result renders as nothing in most viewers — silent corruption.
Targeted check on `recipes[].command` alone would only catch this
one path; a general control-byte scan catches every future class of
escape mistake.
Also adds a SKILL.md instruction in Phase 1.5e directing agents to
double-escape backslashes in regex literals inside JSON string fields,
so the scanner is the safety net for the rare case the instruction
gets missed rather than the primary defense.
Surfaced by hackernews retro #350 finding F2.
* fix(cli): delete dead wrapResultsWithFreshness helper
Generated CLIs ship a wrapResultsWithFreshness helper in
internal/cli/helpers.go that no template ever calls. Spec-driven
commands use wrapWithProvenance directly; novel hand-built commands
that wanted freshness in their envelope haven't reached for it
either across five months and two dozen library CLIs. Polish-worker
removes it on every run.
Decision: delete from the template rather than promote it to a
canonical pattern via SKILL Phase 3 documentation. Keeping a helper
no agent uses just to mask the dead-code finding traded clarity for
nothing.
Golden fixtures updated to reflect:
- dead_functions count: 1 -> 0
- dead_code score: 4 -> 5
- scorecard total: 81 -> 82
- helpers.go: 13 lines fewer
Surfaced by hackernews retro #350 finding F7. Decision and rationale
documented in the parent plan
docs/plans/2026-04-27-004-fix-printing-press-machine-improvements-from-hackernews-retro-plan.md.
* fix(cli): truncate list responses client-side when no paginator declared
When a spec declares a `limit` param on a list endpoint but no
Pagination block, the generator now emits a truncateJSONArray(data,
flagLimit) call after the API response returns. APIs like Firebase,
file-backed JSON dumps, and RSS-style endpoints accept ?limit=N
without honoring it; the user-facing --limit flag was silently broken
on every CLI generated against such APIs.
Three pieces:
1. internal/generator/generator.go: new endpointNeedsClientLimit gate
(GET method + non-positional `limit` param + no Pagination block →
true). New HelperFlags.HasClientLimit flag set during
computeHelperFlags, and a matching template func registered on the
FuncMap.
2. internal/generator/templates/helpers.go.tmpl: truncateJSONArray
helper, gated on HasClientLimit so CLIs that don't need it don't
ship dead code.
3. internal/generator/templates/command_endpoint.go.tmpl: the
per-endpoint command emitter calls truncateJSONArray after the
response error check when the gate fires.
Truncation is idempotent — when the API already returned <= N items
the function is a no-op. Paginated APIs are unaffected because the
gate skips them.
Surfaced by hackernews retro #350 finding F6 — six commands had to be
hand-patched in the printed CLI before this lands. Subsequent regens
of those CLIs will get the truncation from the generator.
* fix(cli): warn when sync consumes items but stores zero rows
UpsertBatch now returns (stored, error). The generated sync code uses
the stored count to drive totalCount and progressCount instead of
len(items) — so sync_complete reports the honest stored count, not
the consumed count.
When a non-empty page yields zero stored rows (every item failed ID
extraction), sync emits:
- humanFriendly: stderr warning naming the resource and likely cause
(scalar item shape rather than objects with extractable IDs)
- structured: a sync_anomaly event with consumed/stored/reason fields
Without this, syncs against APIs like Firebase /topstories.json (which
returns [int, int, int] arrays of bare IDs) silently completed with
"500 synced" while writing zero rows — the local store was empty and
downstream commands relying on it returned empty results with no clue
why. Hackernews retro #350 surfaced the bug; controversial returned []
after a "successful" sync until the regen author traced it.
Touches three sync templates (REST sync.go, dependent-resource sync,
graphql_sync.go) plus the generated store_upsert_batch_test that
exercises the new signature. The spec format is unchanged — this is
purely an observability fix; ID-list hydration via a `response_format`
spec field stays out of scope per the retro audit.
Surfaced by hackernews retro #350 finding F5 (warn-only half).
* fix(cli): reimplementation_check recognizes sibling internal packages
A novel-feature command that imports a hand-built secondary API
client (e.g. internal/algolia for a CLI fronting both a primary and
a secondary API) was tripping the reimplementation false positive
because the pre-existing regex set only knew the canonical patterns:
flags.newClient, http.Get/Post, c.Get/c.Post, and `internal/client`
imports. Hand-built helper packages had no signal.
Now hasClientSignal also returns true when the file imports any
non-reserved package under `internal/<name>`. Reserved names match
the generator-emitted set (client, store, cliutil, cache, config,
mcp, types, share, deliver, profile, feedback, graphql); anything
else is presumed agent-authored API client code.
Go's RE2 has no negative lookahead, so the regex matches all
internal/<name> imports and a small whitelist filter drops the
reserved set in code.
False positives from this signal — a non-client utility package
mistakenly recognized as a client — are strictly less bad than the
false negatives we get without it (a real Algolia client flagged
as reimplementation). Multi-source CLIs like hackernews
(Firebase + Algolia) were producing 5+ noise findings on every
dogfood; they now produce zero.
Surfaced by hackernews retro #350 finding F4.
* fix(cli): skip dogfood path-validity for internal-yaml specs
Dogfood was reporting "Path Validity: 0/0 valid (FAIL)" for every
internal-yaml CLI even when paths were perfectly valid. The check is
written for OpenAPI path patterns; internal-yaml expresses paths in
its own shape, so the regex-based matcher consistently produced
"0/0 valid" against valid CLIs. Scorecard's parallel path-validity
check already excludes internal-yaml from scoring as unscored, so
dogfood and scorecard contradicted each other.
Now internalSpecToDogfoodSpec sets a new IsInternalYAML flag; the
path-validity branch in RunDogfood treats internal-yaml the same way
synthetic specs are already treated — record SKIPPED with a clear
"internal-yaml spec: paths validated at parse time" detail rather
than running the OpenAPI-shaped check.
The existing TestRESTSpec_DogfoodStillChecksPaths is updated to
assert the broader skip behavior. OpenAPI specs continue to run the
check (existing dogfood_test.go fixture exercises that path).
Surfaced by hackernews retro #350 finding F8.
* fix(skills): tighten retro skill's machine-vs-CLI gate
Phase 3 question 5 ("Blast radius and fallback cost") had Step A
through Step D, but the steps left blast-radius assessment open-
ended. Agents could write "API subclass: ~20% of catalog" without
naming concrete APIs and ship a P2 finding on optimism alone. Three
prior retros raised the same multi-base-URL spec change at P2 without
the cost-benefit math ever justifying implementation — the gate was
the wrong shape to catch the pattern.
Three new steps:
Step B (revised): name three concrete APIs that would benefit. If
only two can be named, the finding caps at P3 with a subclass tag.
Step C (new): counter-check question. "If I implemented this fix,
would it actively hurt any API that doesn't have this pattern?" If
yes, the fix needs a guard before being P1/P2.
Step D (new): recurrence-cost check. Search prior retros under
~/printing-press/manuscripts. If the same finding has been raised in
2+ prior retros without being implemented, downgrade to P3 with a
"raised N times" annotation or reframe into a smaller incremental
fix.
Cardinal rule "Bias toward fixing" updated to add the counterweight:
"only when the fix would help APIs you can name. The retro is a
triage tool, not a wishlist."
Surfaced by hackernews retro #350 finding F9 (the meta-finding from
the audit pass after the user pushed back: "carefully consider whether
all the suggested retro items are indeed good to improve in the machine
vs them being specific for this CLI"). Applies to new retros only —
this plan does not retroactively re-classify findings under existing
manuscripts.
Files touched
A docs/plans/2026-04-27-004-fix-printing-press-machine-improvements-from-hackernews-retro-plan.mdA internal/generator/freshness_paths_test.goM internal/generator/generator.goA internal/generator/limit_truncation_test.goA internal/generator/sanitizer_test.goM internal/generator/templates/auto_refresh.go.tmplM internal/generator/templates/command_endpoint.go.tmplM internal/generator/templates/graphql_sync.go.tmplM internal/generator/templates/helpers.go.tmplM internal/generator/templates/store.go.tmplM internal/generator/templates/store_upsert_batch_test.go.tmplM internal/generator/templates/sync.go.tmplM internal/pipeline/dogfood.goM internal/pipeline/reimplementation_check.goA internal/pipeline/sibling_client_test.goM internal/pipeline/spec_detect.goM internal/pipeline/synthetic_spec_test.goM skills/printing-press-retro/SKILL.mdM skills/printing-press/SKILL.mdM testdata/golden/expected/generate-golden-api/dogfood.jsonM testdata/golden/expected/generate-golden-api/printing-press-golden/internal/cli/helpers.goM testdata/golden/expected/generate-golden-api/scorecard.json
Diff
commit 0ed2fbd809bf93c386022647f583fe6a12abd761
Author: Trevin Chow <trevin@trevinchow.com>
Date: Mon Apr 27 15:09:47 2026 -0700
fix(cli): machine improvements from hackernews retro #350 (#352)
* fix(cli): trim phantom freshness paths in rendered docs
freshnessCommandPaths() unconditionally emitted "<resource> list/get/search"
variants for every syncable resource regardless of whether those subcommands
actually existed on the resource. README and SKILL.md rendered these
verbatim under "Covered command paths," misleading users and agents
about the command tree.
Now the rendered slice consults endpoint existence per resource:
- Promoted single-endpoint resources emit only "<cli> <resource>" — Cobra
resolves it directly to the leaf command.
- Multi-endpoint resources emit the bare form plus one entry per real
endpoint name from spec.Resources[name].Endpoints.
The runtime fallback map in auto_refresh.go keeps its no-op variants —
those help Cobra arg resolution at runtime and are harmless. Only the
slice rendered into user-facing docs is trimmed.
Surfaced by hackernews retro #350 finding F1.
Includes the parent plan document covering all 8 retro work units.
* fix(cli): reject ASCII control bytes in rendered SKILL/README
Extend validateRenderedArtifact (which already runs on README.md and
SKILL.md) to scan the rendered output for ASCII control bytes outside
the small set markdown legitimately uses (tab, LF, CR). On rejection,
surface the file path, byte offset, and a hint pointing at the most
likely cause: a JSON-parsed field containing an unescaped backslash
sequence like "\b" or "\f" that the JSON parser turned into a real
control byte.
Backstory: an agent author writes `"command": "...\bGo\b..."` in
research.json's narrative.recipes intending the regex literal
`\bGo\b`. encoding/json correctly parses `\b` as backspace (0x08),
the template engine writes those bytes straight into SKILL.md, and
the result renders as nothing in most viewers — silent corruption.
Targeted check on `recipes[].command` alone would only catch this
one path; a general control-byte scan catches every future class of
escape mistake.
Also adds a SKILL.md instruction in Phase 1.5e directing agents to
double-escape backslashes in regex literals inside JSON string fields,
so the scanner is the safety net for the rare case the instruction
gets missed rather than the primary defense.
Surfaced by hackernews retro #350 finding F2.
* fix(cli): delete dead wrapResultsWithFreshness helper
Generated CLIs ship a wrapResultsWithFreshness helper in
internal/cli/helpers.go that no template ever calls. Spec-driven
commands use wrapWithProvenance directly; novel hand-built commands
that wanted freshness in their envelope haven't reached for it
either across five months and two dozen library CLIs. Polish-worker
removes it on every run.
Decision: delete from the template rather than promote it to a
canonical pattern via SKILL Phase 3 documentation. Keeping a helper
no agent uses just to mask the dead-code finding traded clarity for
nothing.
Golden fixtures updated to reflect:
- dead_functions count: 1 -> 0
- dead_code score: 4 -> 5
- scorecard total: 81 -> 82
- helpers.go: 13 lines fewer
Surfaced by hackernews retro #350 finding F7. Decision and rationale
documented in the parent plan
docs/plans/2026-04-27-004-fix-printing-press-machine-improvements-from-hackernews-retro-plan.md.
* fix(cli): truncate list responses client-side when no paginator declared
When a spec declares a `limit` param on a list endpoint but no
Pagination block, the generator now emits a truncateJSONArray(data,
flagLimit) call after the API response returns. APIs like Firebase,
file-backed JSON dumps, and RSS-style endpoints accept ?limit=N
without honoring it; the user-facing --limit flag was silently broken
on every CLI generated against such APIs.
Three pieces:
1. internal/generator/generator.go: new endpointNeedsClientLimit gate
(GET method + non-positional `limit` param + no Pagination block →
true). New HelperFlags.HasClientLimit flag set during
computeHelperFlags, and a matching template func registered on the
FuncMap.
2. internal/generator/templates/helpers.go.tmpl: truncateJSONArray
helper, gated on HasClientLimit so CLIs that don't need it don't
ship dead code.
3. internal/generator/templates/command_endpoint.go.tmpl: the
per-endpoint command emitter calls truncateJSONArray after the
response error check when the gate fires.
Truncation is idempotent — when the API already returned <= N items
the function is a no-op. Paginated APIs are unaffected because the
gate skips them.
Surfaced by hackernews retro #350 finding F6 — six commands had to be
hand-patched in the printed CLI before this lands. Subsequent regens
of those CLIs will get the truncation from the generator.
* fix(cli): warn when sync consumes items but stores zero rows
UpsertBatch now returns (stored, error). The generated sync code uses
the stored count to drive totalCount and progressCount instead of
len(items) — so sync_complete reports the honest stored count, not
the consumed count.
When a non-empty page yields zero stored rows (every item failed ID
extraction), sync emits:
- humanFriendly: stderr warning naming the resource and likely cause
(scalar item shape rather than objects with extractable IDs)
- structured: a sync_anomaly event with consumed/stored/reason fields
Without this, syncs against APIs like Firebase /topstories.json (which
returns [int, int, int] arrays of bare IDs) silently completed with
"500 synced" while writing zero rows — the local store was empty and
downstream commands relying on it returned empty results with no clue
why. Hackernews retro #350 surfaced the bug; controversial returned []
after a "successful" sync until the regen author traced it.
Touches three sync templates (REST sync.go, dependent-resource sync,
graphql_sync.go) plus the generated store_upsert_batch_test that
exercises the new signature. The spec format is unchanged — this is
purely an observability fix; ID-list hydration via a `response_format`
spec field stays out of scope per the retro audit.
Surfaced by hackernews retro #350 finding F5 (warn-only half).
* fix(cli): reimplementation_check recognizes sibling internal packages
A novel-feature command that imports a hand-built secondary API
client (e.g. internal/algolia for a CLI fronting both a primary and
a secondary API) was tripping the reimplementation false positive
because the pre-existing regex set only knew the canonical patterns:
flags.newClient, http.Get/Post, c.Get/c.Post, and `internal/client`
imports. Hand-built helper packages had no signal.
Now hasClientSignal also returns true when the file imports any
non-reserved package under `internal/<name>`. Reserved names match
the generator-emitted set (client, store, cliutil, cache, config,
mcp, types, share, deliver, profile, feedback, graphql); anything
else is presumed agent-authored API client code.
Go's RE2 has no negative lookahead, so the regex matches all
internal/<name> imports and a small whitelist filter drops the
reserved set in code.
False positives from this signal — a non-client utility package
mistakenly recognized as a client — are strictly less bad than the
false negatives we get without it (a real Algolia client flagged
as reimplementation). Multi-source CLIs like hackernews
(Firebase + Algolia) were producing 5+ noise findings on every
dogfood; they now produce zero.
Surfaced by hackernews retro #350 finding F4.
* fix(cli): skip dogfood path-validity for internal-yaml specs
Dogfood was reporting "Path Validity: 0/0 valid (FAIL)" for every
internal-yaml CLI even when paths were perfectly valid. The check is
written for OpenAPI path patterns; internal-yaml expresses paths in
its own shape, so the regex-based matcher consistently produced
"0/0 valid" against valid CLIs. Scorecard's parallel path-validity
check already excludes internal-yaml from scoring as unscored, so
dogfood and scorecard contradicted each other.
Now internalSpecToDogfoodSpec sets a new IsInternalYAML flag; the
path-validity branch in RunDogfood treats internal-yaml the same way
synthetic specs are already treated — record SKIPPED with a clear
"internal-yaml spec: paths validated at parse time" detail rather
than running the OpenAPI-shaped check.
The existing TestRESTSpec_DogfoodStillChecksPaths is updated to
assert the broader skip behavior. OpenAPI specs continue to run the
check (existing dogfood_test.go fixture exercises that path).
Surfaced by hackernews retro #350 finding F8.
* fix(skills): tighten retro skill's machine-vs-CLI gate
Phase 3 question 5 ("Blast radius and fallback cost") had Step A
through Step D, but the steps left blast-radius assessment open-
ended. Agents could write "API subclass: ~20% of catalog" without
naming concrete APIs and ship a P2 finding on optimism alone. Three
prior retros raised the same multi-base-URL spec change at P2 without
the cost-benefit math ever justifying implementation — the gate was
the wrong shape to catch the pattern.
Three new steps:
Step B (revised): name three concrete APIs that would benefit. If
only two can be named, the finding caps at P3 with a subclass tag.
Step C (new): counter-check question. "If I implemented this fix,
would it actively hurt any API that doesn't have this pattern?" If
yes, the fix needs a guard before being P1/P2.
Step D (new): recurrence-cost check. Search prior retros under
~/printing-press/manuscripts. If the same finding has been raised in
2+ prior retros without being implemented, downgrade to P3 with a
"raised N times" annotation or reframe into a smaller incremental
fix.
Cardinal rule "Bias toward fixing" updated to add the counterweight:
"only when the fix would help APIs you can name. The retro is a
triage tool, not a wishlist."
Surfaced by hackernews retro #350 finding F9 (the meta-finding from
the audit pass after the user pushed back: "carefully consider whether
all the suggested retro items are indeed good to improve in the machine
vs them being specific for this CLI"). Applies to new retros only —
this plan does not retroactively re-classify findings under existing
manuscripts.
---
...hine-improvements-from-hackernews-retro-plan.md | 454 +++++++++++++++++++++
internal/generator/freshness_paths_test.go | 168 ++++++++
internal/generator/generator.go | 141 ++++++-
internal/generator/limit_truncation_test.go | 98 +++++
internal/generator/sanitizer_test.go | 113 +++++
internal/generator/templates/auto_refresh.go.tmpl | 4 +-
.../generator/templates/command_endpoint.go.tmpl | 9 +
internal/generator/templates/graphql_sync.go.tmpl | 16 +-
internal/generator/templates/helpers.go.tmpl | 40 +-
internal/generator/templates/store.go.tmpl | 29 +-
.../templates/store_upsert_batch_test.go.tmpl | 4 +-
internal/generator/templates/sync.go.tmpl | 32 +-
internal/pipeline/dogfood.go | 19 +
internal/pipeline/reimplementation_check.go | 65 ++-
internal/pipeline/sibling_client_test.go | 182 +++++++++
internal/pipeline/spec_detect.go | 15 +-
internal/pipeline/synthetic_spec_test.go | 20 +-
skills/printing-press-retro/SKILL.md | 38 +-
skills/printing-press/SKILL.md | 2 +-
.../expected/generate-golden-api/dogfood.json | 8 +-
.../printing-press-golden/internal/cli/helpers.go | 12 -
.../expected/generate-golden-api/scorecard.json | 6 +-
22 files changed, 1392 insertions(+), 83 deletions(-)
diff --git a/docs/plans/2026-04-27-004-fix-printing-press-machine-improvements-from-hackernews-retro-plan.md b/docs/plans/2026-04-27-004-fix-printing-press-machine-improvements-from-hackernews-retro-plan.md
new file mode 100644
index 00000000..52379550
--- /dev/null
+++ b/docs/plans/2026-04-27-004-fix-printing-press-machine-improvements-from-hackernews-retro-plan.md
@@ -0,0 +1,454 @@
+---
+title: 'Printing Press machine improvements from hackernews retro #350'
+type: fix
+status: active
+date: 2026-04-27
+origin: ~/printing-press/manuscripts/hackernews/20260427-120911/proofs/20260427-130958-retro-hackernews-pp-cli.md
+---
+
+# Printing Press machine improvements from hackernews retro #350
+
+## Overview
+
+Implement the eight in-scope work units from the hackernews retro ([issue #350](https://github.com/mvanhorn/cli-printing-press/issues/350)). The retro raised nine findings about the Printing Press; F3 (multi-base-URL spec change) was demoted to deferred and stays out of this plan. The remaining work spans three subsystems — generator templates, scoring tools, and skill instructions — and is mostly small mechanical fixes plus one meta-improvement to the retro skill itself.
+
+The unifying thread: **stop emitting machine-internal data into user-facing surfaces, and stop classifying narrow findings as broad ones.** Phantom freshness paths render machine fallbacks into README/SKILL. Backspace bytes leak from JSON-parsed regex into rendered markdown. Silent zero-row sync logs report "500 synced" when nothing was stored. Each is a leak between layers that has a direct fix.
+
+---
+
+## Problem Frame
+
+Hackernews regen on printing-press v2.3.9 produced a Grade A CLI but surfaced eight systemic Printing Press issues during the retro. Some affect every cache-enabled CLI generation (phantom freshness paths, dead `wrapResultsWithFreshness` helper). Some affect API subclasses (no-paginator APIs ignore `?limit=N`, sync of bare-ID arrays writes zero rows silently). Some are scoring tool false positives (reimplementation_check missing secondary clients, dogfood path-validity FAIL on internal-yaml). One — F9 — is a meta-finding about the retro skill itself producing over-broad findings, which was the catalyst for the audit pass that reshaped this plan.
+
+The fixes are independent and can land in any order. Most are small enough to ship as individual commits.
+
+---
+
+## Requirements Trace
+
+- R1. Trim `FreshnessCommands` rendered into README/SKILL to only paths whose subcommands actually exist (origin: F1).
+- R2. Detect control bytes in rendered SKILL/README output and reject the render with file:offset and source field name. Add skill instruction warning agents to double-escape backslashes in `narrative.recipes[].command` and similar fields (origin: F2).
+- R3. Reimplementation_check recognizes imports of sibling `internal/<name>` packages as legitimate API access (origin: F4).
+- R4. Sync emits a stderr warning and a structured event with `stored: 0` when it consumes items but writes zero rows (origin: F5).
+- R5. When the spec profiler detects no paginator on a list endpoint, generated commands truncate the response client-side via `truncateJSONArray` after the API call returns (origin: F6).
+- R6. Dogfood reports `Path Validity: SKIPPED (internal-yaml spec)` instead of `0/0 valid (FAIL)` when the spec source is internal YAML (origin: F8).
+- R7. Decide the canonical pattern for `wrapResultsWithFreshness` — either delete it from the template, or add SKILL Phase 3 instruction directing agents to use it (origin: F7).
+- R8. Tighten the retro skill's blast-radius gate: require three concrete cross-API examples before classifying a finding as P1/P2; add a counter-check question and a recurrence-cost check (origin: F9).
+
+**Origin trace:** the origin document is the retro itself (no requirements doc precedes it), so this section omits Actors / Key Flows / Acceptance Examples — the origin's WUs map 1:1 to this plan's units below.
+
+---
+
+## Scope Boundaries
+
+- WU-3 from the retro (add `enrichment_apis:` section to spec format) is **out of scope**. The retro audit moved it to deferred P3 because the same finding has been raised in three retros without justifying the implementation cost. Re-evaluate when combo CLIs exceed ~40% of catalog.
+- The hydration spec field for `response_format: id_list` is **out of scope** (originally retro F5b, now Skipped). Subclass too narrow (~3% of APIs). Hand-build hydration in printed CLIs that need it.
+- This plan does not retroactively re-classify findings in existing retros under `~/printing-press/manuscripts/`. WU-9's stricter gate applies to **new** retros only.
+- This plan does not touch the printed `hackernews` CLI in the library. Any printed-CLI-specific fixes were applied during the regen session; this plan is exclusively about the Printing Press.
+
+### Deferred to Follow-Up Work
+
+- **WU-3 (`enrichment_apis` spec format)** — separate plan when justification threshold is met. The current workaround (hand-build a 150-LOC client per multi-source CLI) is acceptable.
+
+---
+
+## Context & Research
+
+### Relevant Code and Patterns
+
+- `internal/generator/generator.go::freshnessCommandPaths()` (lines 636-664) — current source of phantom paths. Iterates `g.profile.SyncableResources` and unconditionally adds `prefix`, `prefix + " list"`, `prefix + " get"`, `prefix + " search"`. The same struct (`g.profile.SyncableResources[].Endpoints`) already tracks which endpoints actually exist — that's the data freshness paths should consult.
+- `internal/generator/generator.go::Generator.generate()` (line 973-974) — maps `skill.md.tmpl` → `SKILL.md` and `readme.md.tmpl` → `README.md`. This is the natural injection point for a render-time control-byte sanitizer.
+- `internal/pipeline/reimplementation_check.go::clientCallRe` (around line 100) — regex set: `flags\.newClient`, `http\.(Get|Post|NewRequest|Do)`, `c\.(Do|Get|Post)`. The fix extends this with sibling-internal-package import detection.
+- `internal/pipeline/reimplementation_check.go::hasStoreSignal()` (around line 200) — existing pattern for "look at file content for an import line + call site". The same shape works for client signal: detect imports matching `"<module>/internal/<sibling-name>"` where sibling-name ≠ `client` and ≠ `store`.
+- `internal/generator/templates/sync.go.tmpl::syncResource` (line 352, 406) — `db.UpsertBatch(resource, items)` is called, then `sync_complete` event is emitted with `total: %d` where `%d` is `totalCount`. `totalCount` increments by `len(items)` after each successful UpsertBatch — but UpsertBatch can silently no-op for items whose ID extraction fails. This is the gap.
+- `internal/pipeline/dogfood.go::Run()` (lines 205-230) — already has a `spec.IsSynthetic()` branch that records `Skipped: true` for synthetic specs. Internal-yaml specs without `kind: synthetic` fall through to `checkPaths(...)` which runs but reports oddly when the spec has no recognized paths.
+- `internal/generator/templates/helpers.go.tmpl` (line 1263) — defines `wrapResultsWithFreshness`. No call sites in any generated template.
+- `skills/printing-press-retro/SKILL.md::Phase 3` — current blast-radius checklist (Step A "Cross-API stress test", Step B "Estimate frequency", Step C "Assess fallback cost", Step D "Make the tradeoff"). Steps B and C are open-ended — agents handwave concrete evidence.
+
+### Institutional Learnings
+
+- `docs/retros/2026-04-13-recipe-goat-retro.md` — case-history motivation for the agentic Phase 4.85 output review. Same shape as F9: a previously-acceptable bias toward "ship it" produced false negatives that an agent layer caught. The fix in that case was structural (new phase). The fix here is procedural (stricter gate question).
+- `docs/retros/` (movie-goat retro F8, hackernews v1.3.3 retro F3) — both raised the spec-multi-base-URL finding before. Recurrence without implementation IS evidence the cost-benefit hasn't justified — F9 codifies recognizing this pattern.
+
+### External References
+
+None. All work is internal to this repo; no external API or framework decisions.
+
+---
+
+## Key Technical Decisions
+
+- **Phantom-paths fix (R1) trims the rendered slice, not the runtime fallback map.** The map in generated `auto_refresh.go` keeps its `<resource> list/get/search` variants because Cobra's argument resolution can land on any of them at runtime — the no-op fallback is harmless. Only the `.FreshnessCommands` slice rendered into user-facing docs needs trimming.
+- **Render-time sanitizer (R2) checks ALL rendered output, not just recipe fields.** A targeted check on `narrative.recipes[].command` would only catch this specific case. A general "no control bytes 0x00-0x08, 0x0B-0x0C, 0x0E-0x1F" check on all rendered SKILL/README/manifest output catches every future class of escape mistake. Tab (0x09), newline (0x0A), and carriage return (0x0D) are explicitly allowed.
+- **Sibling-package detection (R3) is broad, not narrow.** Any import of `<module>/internal/<x>` where `<x>` is not `client` or `store` counts as client signal. Worst case: a non-client internal helper package is mistakenly recognized as a client. That's still a strictly-less-bad outcome than today's false positive (real Algolia client miscategorized as reimplementation).
+- **Sync warn (R4) compares stored-vs-consumed.** Either thread a return value through `UpsertBatch` reporting rows-actually-written, or run a `SELECT count(*)` before/after. The first is more precise; the second is simpler and works without changing the upsert API. Choice deferred to implementation — both meet the acceptance criteria.
+- **Limit truncation (R5) defaults to ON when no paginator detected.** This is the safer default than ON when explicitly opted-in: every API without a paginator silently breaks `--limit` today; turning truncation on by default matches user expectations and is harmless when the API already returns ≤ limit rows.
+- **WU-8 (R7) decision: delete the helper.** Five months of generated CLIs and zero call sites. Polish-worker removes it on every run. Adding a SKILL instruction to use it would burden the hand-build path without clear benefit when `wrapWithProvenance` already covers the use case from spec-driven commands. Removing it from the template eliminates a dead-code finding that polish-worker has been masking.
+- **Retro skill gate (R8) is procedural, not enforced.** The SKILL.md changes add three explicit gate questions to Phase 3 question 5. The skill cannot mechanically force three concrete API names — but a stricter prompt produces measurably more rigorous findings. Same approach as the existing "Cross-API stress test" prose.
+
+---
+
+## Open Questions
+
+### Resolved During Planning
+
+- **Should WU-1 also remove the runtime fallback variants from `auto_refresh.go`?** No. Keep the map's no-op variants for runtime resolution flexibility; trim only the rendered slice. (See Key Technical Decisions above.)
+- **Should the render-time sanitizer be opt-in per template or applied globally?** Globally, on every rendered text file (SKILL.md, README.md, anything else). Opt-in defeats the purpose.
+- **Do we keep `wrapResultsWithFreshness` and document it, or delete?** Delete. (See Key Technical Decisions above.)
+
+### Deferred to Implementation
+
+- **Exact mechanism for sync's stored-vs-consumed comparison.** Either thread row count back from UpsertBatch, or do a count query bracketed around the UpsertBatch call. Implementer picks based on whether modifying UpsertBatch's return type ripples beyond `sync.go.tmpl`.
+- **Profiler signature for "no paginator detected".** Need to confirm the profiler exposes a single check or whether the generator inspects `endpoint.Pagination == nil` directly. Implementer reads the existing pagination-aware emit path and follows the same convention.
+- **Names of the three concrete APIs the retro gate forces an agent to list.** N/A — the gate requires the AGENT to name three; the skill instructions don't prescribe which.
+
+---
+
+## Implementation Units
+
+- U1. **Trim phantom freshness paths from rendered docs**
+
+**Goal:** README and SKILL list only freshness paths whose subcommands actually exist for each resource.
+
+**Requirements:** R1.
+
+**Dependencies:** None.
+
+**Files:**
+- Modify: `internal/generator/generator.go` — refactor `freshnessCommandPaths()` to consult endpoint existence
+- Modify: `internal/generator/generator_test.go` — extend existing test fixtures
+- Test: `internal/generator/generator_test.go` (existing file)
+
+**Approach:**
+- Iterate `g.profile.SyncableResources` as today, but for each resource only emit `prefix + " " + endpointName` for endpoints actually declared on that resource. Always emit the bare `prefix` (resource-as-command).
+- Leave the `auto_refresh.go.tmpl` runtime map untouched — it continues emitting its no-op fallback variants because those help Cobra arg resolution.
+- Existing `Cache.Commands` block (custom command-path coverage from spec) continues unchanged.
+
+**Patterns to follow:**
+- The endpoint iteration shape at `generator.go::458` and `498` already walks `r.Endpoints` for emission; freshness paths should mirror it.
+
+**Test scenarios:**
+- Happy path: a resource with endpoints `top`, `new`, `best`, `get` (HN's stories) emits `["<cli> stories", "<cli> stories top", "<cli> stories new", "<cli> stories best", "<cli> stories get"]` in `.FreshnessCommands`.
+- Happy path: a resource with a single endpoint `list` emits `["<cli> ask", "<cli> ask list"]` (the bare command stays because Cobra resolves `<cli> ask` to the promoted single endpoint).
+- Edge case: a resource with zero endpoints (shouldn't exist in valid specs, but guard) emits only `["<cli> <resource>"]`.
+- Negative: `<cli> ask get`, `<cli> ask search`, `<cli> stories list`, `<cli> stories search` do NOT appear unless those endpoints actually exist.
+- Integration: a generated SKILL.md and README.md from the existing `golden-api.yaml` fixture render only real paths after the change. Verify via `scripts/golden.sh verify` — fixture update may be needed and should be part of the same commit.
+
+**Verification:**
+- All real freshness paths still resolve to actual subcommands in a generated CLI.
+- `scripts/golden.sh verify` passes (after intentional fixture update for the changed paths).
+- `internal/generator/auto_refresh.go.tmpl` is unchanged — runtime map still has no-op variants.
+
+---
+
+- U2. **Render-time control-byte sanitizer + skill warning**
+
+**Goal:** Generator rejects any rendered SKILL.md/README.md/etc. that contains ASCII control bytes (0x00-0x08, 0x0B-0x0C, 0x0E-0x1F). Skill instructions for `narrative.recipes[].command` and similar fields warn agents to double-escape backslashes for embedded regex.
+
+**Requirements:** R2.
+
+**Dependencies:** None.
+
+**Files:**
+- Modify: `internal/generator/generator.go` — add post-render sanitization check around the `render(...)` call site (near line 970-980)
+- Modify: `skills/printing-press/SKILL.md` — Phase 1.5e research.json instructions, narrative.recipes section, add the double-escape warning
+- Test: `internal/generator/sanitizer_test.go` (new file)
+
+**Approach:**
+- After each template render, before writing the file to disk, scan the byte stream for disallowed control chars. Allow `\t` (0x09), `\n` (0x0A), `\r` (0x0D). Reject everything else in the 0x00-0x1F range.
+- On rejection, return an error naming the file, the byte offset, and a hint about which source field is most likely responsible (best-effort: scan recent template variables that could carry user input).
+- In SKILL.md research.json instructions, add one-liner under the `narrative.recipes[].command` description: "Regex literals in `recipes[].command`, `troubleshoots[].fix`, and `quickstart[].command` must double-escape backslashes (`\\b` not `\b`) so JSON parses to literal `\b` rather than backspace."
+
+**Patterns to follow:**
+- `internal/generator/generator.go` already has post-render passes for some files (e.g., gofmt on `.go` outputs). Add the sanitizer in the same pass shape.
+
+**Test scenarios:**
+- Happy path: rendered SKILL.md with normal text passes the sanitizer unchanged.
+- Edge case: rendered SKILL.md containing tab/newline/CR passes (these are allowed).
+- Error path: rendered SKILL.md containing a backspace byte (0x08) is rejected; error message names the file and the byte offset.
+- Error path: rendered output containing form-feed (0x0C), bell (0x07), or other control chars is rejected.
+- Integration: generate a CLI with a research.json containing `"command": "...\\bGo\\b..."` (2 backslashes — JSON parses as backspace). Generation fails with the sanitizer's error, not silently shipping mojibake.
+- Integration: generate a CLI with a research.json containing `"command": "...\\\\bGo\\\\b..."` (4 backslashes — JSON parses as `\bGo\b`). Renders correctly.
+
+**Verification:**
+- Existing CLIs in `~/printing-press/library/` regenerate cleanly (no false-positive sanitizer rejections).
+- A deliberately-broken research.json fails generation with a clear error pointing at the file and offset.
+- SKILL.md research.json instructions surface the double-escape rule before agents author recipes.
+
+---
+
+- U3. **Reimplementation_check recognizes sibling internal packages**
+
+**Goal:** Hand-built API client packages in `internal/<name>/` (where `<name>` is not `client` or `store`) are recognized as legitimate API access; commands that import and call into them no longer trip the reimplementation false positive.
+
+**Requirements:** R3.
+
+**Dependencies:** None.
+
+**Files:**
+- Modify: `internal/pipeline/reimplementation_check.go` — add sibling-internal-import detection alongside existing `clientCallRe`
+- Test: `internal/pipeline/reimplementation_check_test.go` — extend existing table-driven tests
+
+**Approach:**
+- Add a regex `siblingInternalImportRe` matching `"[^"]*/internal/(?!client\b|store\b|cliutil\b|cache\b|config\b|mcp\b|types\b)[a-z][a-z0-9]*"` — any internal package not in the generator's reserved set.
+- In `classifyReimplementation`, set `hasClient = true` when the file content matches `siblingInternalImportRe`. This treats "imports a non-reserved sibling internal package" as evidence of legitimate API access.
+- Reserved package names are the ones the generator emits unconditionally. List them in a const so the test verifies the list is consistent.
+
+**Patterns to follow:**
+- The existing `hasStoreSignal` function (around line 200) shows the pattern: regex match on file content → boolean signal → drives the classification result.
+
+**Test scenarios:**
+- Happy path: a command file importing `"hackernews-pp-cli/internal/algolia"` and calling any method on a typed value does NOT trip reimplementation. `hasClient = true` is set by the new regex.
+- Happy path: a command file importing `"food52-pp-cli/internal/recipescraper"` (hypothetical) does NOT trip reimplementation.
+- Negative: a command file importing only `"<module>/internal/store"` does NOT match the sibling-internal regex (store is reserved), but is still exempted by the existing store carve-out — net behavior unchanged.
+- Negative: a command file importing only stdlib and returning a hardcoded JSON literal still trips reimplementation (`hasClient = false`, `hasTrivialBody = true`).
+- Negative: a command file importing nothing from `<module>/internal/` and not calling any of the canonical client patterns still trips reimplementation.
+- Edge case: a command file importing `<module>/internal/cliutil` does NOT trip the new signal (cliutil is reserved). The existing cliutil-aware logic governs.
+
+**Verification:**
+- The hackernews CLI (which imports `internal/algolia`) regenerates with 0 reimplementation findings instead of 5.
+- Existing CLIs without secondary clients keep their current scoring (no regressions).
+- `internal/pipeline/reimplementation_check_test.go` table grows by ≥3 cases covering the new signal.
+
+---
+
+- U4. **Sync warns when consumed-vs-stored counts diverge**
+
+**Goal:** Sync emits an honest count of stored rows. When `UpsertBatch` consumed N items but stored zero (e.g., bare-ID arrays from Firebase-shaped APIs), users see a clear warning and the structured event reports `stored: 0`.
+
+**Requirements:** R4.
+
+**Dependencies:** None.
+
+**Files:**
+- Modify: `internal/generator/templates/sync.go.tmpl` — replace the `totalCount += len(items)` post-UpsertBatch with a stored-count read
+- Modify: `internal/generator/templates/store.go.tmpl::UpsertBatch` — return the count of rows actually written
+- Test: `internal/generator/templates_test.go` (existing) or new `internal/generator/sync_template_test.go`
+
+**Approach:**
+- Change `UpsertBatch(resourceType string, items []json.RawMessage) error` to `UpsertBatch(resourceType string, items []json.RawMessage) (int, error)` returning the count of rows actually upserted. Bump the rows-written counter inside the inner upsert helper that already runs per-item.
+- In sync.go.tmpl: after `stored, err := db.UpsertBatch(...)`, set `totalCount += stored` (not `+= len(items)`). When `len(items) > 0 && stored == 0`, emit:
+ - Stderr warning: `<resource> returned scalar items; consumed N, stored 0. The store will be empty for this resource. Likely cause: API returns ID lists rather than objects.`
+ - Structured event: `{"event":"sync_anomaly","resource":"%s","consumed":%d,"stored":0,"reason":"all_items_failed_id_extraction"}`
+- Existing structured `sync_complete` event still emits but now with the honest `stored` count.
+
+**Patterns to follow:**
+- The existing `sync_warning` event shape (around line 280 in sync.go.tmpl) for stderr messaging during sync.
+
+**Test scenarios:**
+- Happy path: a sync receives `[{id:1, ...}, {id:2, ...}]` → both upserted, `stored: 2`, no warning, `sync_complete` shows `total: 2`.
+- Edge case: an empty array `[]` → `stored: 0`, no warning (consumed was also 0).
+- Error path: a sync receives `[1, 2, 3]` (bare IDs) → `stored: 0`, warning emitted, `sync_anomaly` event emitted, `sync_complete` shows `total: 0`.
+- Error path: a sync receives `[{id:1}, "garbage"]` → `stored: 1`, no warning (some rows landed). Mixed shapes are not flagged.
+- Integration: regenerate hackernews and run `sync` against /topstories.json. Stderr now shows the warning, JSON events show `stored: 0`, log no longer claims "500 synced".
+
+**Verification:**
+- Hackernews's sync log changes from `"500 synced"` to a clear "0 stored, 500 consumed" message with the warning.
+- CLIs whose sync writes object arrays show zero behavior change.
+- `golden.sh verify` passes after the template change (golden may need update if any fixture exercises sync; check first).
+
+---
+
+- U5. **Client-side limit truncation when no paginator detected**
+
+**Goal:** Generated list commands honor `--limit N` even against APIs that ignore the `?limit=` query param, by truncating the response client-side after the API call returns.
+
+**Requirements:** R5.
+
+**Dependencies:** None.
+
+**Files:**
+- Modify: `internal/generator/templates/` — list-endpoint command templates (the ones currently emitting `params["limit"] = ...`); the same templates that produced `stories_top.go`-style files
+- Modify: `internal/generator/templates/helpers.go.tmpl` — add `truncateJSONArray` helper to the generated CLI's helpers package
+- Test: existing template tests; add a fixture spec with no pagination to exercise the truncation path
+
+**Approach:**
+- Detect "no paginator" in the generator: an endpoint where `endpoint.Pagination == nil` AND no recognized paginator query param (look at existing pagination detection at `generator.go::684` for the established convention).
+- When the endpoint has no paginator AND the command accepts a `--limit` flag, emit `data = truncateJSONArray(data, flagLimit)` after `resolveRead` returns.
+- Generate `truncateJSONArray` into the printed CLI's helpers package (it's already present in hackernews's hand-written `limit_helper.go` — promote the same shape into the template).
+- For endpoints WITH a paginator, emit nothing new — the API already honors the limit.
+
+**Patterns to follow:**
+- Hackernews's hand-written `internal/cli/limit_helper.go::truncateJSONArray` is the canonical implementation. Same logic, generated into every CLI.
+- Existing `extractPageItems` in generated sync.go shows the JSON array detection pattern.
+
+**Test scenarios:**
+- Happy path: a spec with `endpoints.list { params: [{name: limit, ...}] }` and no `pagination:` block emits a command that calls `truncateJSONArray(data, flagLimit)` after `resolveRead`.
+- Happy path: `<cli> resource list --limit 3 --json | jq '.results | length'` returns 3 even when the API returns 500 items.
+- Edge case: `--limit 0` (default) → truncateJSONArray is a no-op (returns input unchanged).
+- Negative: a spec with explicit `pagination: {limit_param: per_page}` emits no client-side truncation (defers to the API's paginator).
+- Negative: a spec with cursor-based pagination (`pagination: {cursor_param: page_token}`) emits no client-side truncation.
+- Integration: regenerate one cache-enabled CLI with a paginated API (e.g., a Stripe-style spec) and verify the generated list command does NOT call truncateJSONArray.
+- Integration: regenerate hackernews and verify the 6 commands previously hand-patched (`stories_top.go` etc.) now have generator-emitted `truncateJSONArray` instead of needing the printed-CLI helper.
+
+**Verification:**
+- Hackernews's `internal/cli/limit_helper.go` becomes redundant with generator output (printed CLI can drop the hand-built file on its next regen).
+- A no-paginator API correctly honors `--limit` end-to-end.
+- A paginated API behaves identically to today (no double-truncation).
+
+---
+
+- U6. **Dogfood path-validity reports SKIPPED for internal-yaml specs**
+
+**Goal:** Dogfood emits a clear `Path Validity: SKIPPED (internal-yaml spec)` line instead of the contradictory `0/0 valid (FAIL)` users see today on internal-yaml CLIs.
+
+**Requirements:** R6.
+
+**Dependencies:** None.
+
+**Files:**
+- Modify: `internal/pipeline/dogfood.go` — extend the existing `IsSynthetic()` skip branch to cover all internal-yaml specs, OR add a parallel branch
+- Test: `internal/pipeline/dogfood_test.go` — add a case for non-synthetic internal-yaml
+
+**Approach:**
+- Today's code (around line 213) sets `Skipped: true` only when `spec.IsSynthetic()`. Extend this to also skip when the spec source is internal-yaml (regardless of synthetic flag), since path-validity assumes OpenAPI-style path patterns that internal-yaml expresses differently.
+- Detail message: `"internal-yaml spec — paths validated at parse time"` (per acceptance criteria).
+- Verify the report's overall verdict logic doesn't FAIL on a SKIPPED path-validity check — should already be the case since synthetic specs don't fail today.
+
+**Patterns to follow:**
+- The existing `spec.IsSynthetic()` branch at `dogfood.go:212-218` — same shape, broader condition.
+
+**Test scenarios:**
+- Happy path: dogfood on an internal-yaml CLI without `kind: synthetic` reports `Path Validity: SKIPPED (internal-yaml spec — paths validated at parse time)`.
+- Happy path: dogfood on a synthetic internal-yaml CLI keeps the existing skip message (don't regress that case).
+- Happy path: dogfood on an OpenAPI CLI keeps the `N/M valid` count (don't regress the common case).
+- Negative: overall dogfood verdict on an internal-yaml CLI with all other checks passing is PASS or WARN, not FAIL, attributable to path-validity (the existing scorecard already excludes this dimension; verify alignment).
+
+**Verification:**
+- Hackernews dogfood report no longer shows `0/0 valid (FAIL)`. Shows `SKIPPED` instead.
+- Scorecard's existing internal-yaml exclusion logic is unchanged (no double-skip).
+- An OpenAPI CLI's dogfood is byte-for-byte identical pre/post change.
+
+---
+
+- U7. **Delete dead `wrapResultsWithFreshness` helper**
+
+**Goal:** Remove the unused `wrapResultsWithFreshness` helper from the generator template so future CLIs don't ship dead code that polish-worker has to scrub.
+
+**Requirements:** R7.
+
+**Dependencies:** None.
+
+**Files:**
+- Modify: `internal/generator/templates/helpers.go.tmpl` — remove `wrapResultsWithFreshness` function
+- Modify: `internal/generator/templates/auto_refresh.go.tmpl` — remove the comment referring to `wrapResultsWithFreshness` at line 150
+- Test: `internal/generator/generator_test.go` — verify no test asserts on its presence (likely none, but check)
+
+**Approach:**
+- Decision rationale (Key Technical Decisions above): zero call sites in five months of generated CLIs, polish-worker removes it on every run, `wrapWithProvenance` already covers the use case from spec-driven commands. Delete and move on.
+- If implementation discovers any unexpected call site (it shouldn't), pause and reassess — could indicate the helper was load-bearing for an obscure path.
+
+**Patterns to follow:**
+- N/A — pure deletion.
+
+**Test scenarios:**
+- Happy path: a regenerated CLI compiles cleanly without the helper.
+- Happy path: dogfood does NOT report a dead-helper finding for `wrapResultsWithFreshness` after this change (since the helper no longer exists).
+- Negative: existing tests still pass (no test should be referencing the helper).
+
+**Verification:**
+- `grep -r wrapResultsWithFreshness internal/generator/` returns zero results.
+- A fresh `printing-press generate` run produces a CLI whose `internal/cli/helpers.go` does not contain the helper.
+- Polish-worker, on the next polish pass for any CLI, no longer reports the dead-helper finding.
+
+**Test expectation: none — pure deletion of an unused symbol. The verification is that nothing breaks; no new behavior to test.**
+
+---
+
+- U8. **Tighten retro skill's machine-vs-CLI gate**
+
+**Goal:** Future retros require concrete cross-API evidence before classifying findings as P1/P2. The skill instructions add three explicit gates: name three concrete APIs, counter-check question, recurrence-cost check.
+
+**Requirements:** R8.
+
+**Dependencies:** None.
+
+**Files:**
+- Modify: `skills/printing-press-retro/SKILL.md` — Phase 3 question 5 ("Blast radius and fallback cost") and the cardinal rules at the top
+
+**Approach:**
+- In Phase 3 question 5, replace Step B ("Estimate frequency") with a stricter form requiring three concrete API names. Add bullet wording like:
+
+ > **Step B (revised): Name three concrete APIs that would benefit.** List three specific APIs by name (e.g., "Stripe, Notion, GitHub") that would benefit from this fix beyond the one that surfaced it. If you can only name two — or one plus handwaving "many APIs in theory" — the finding is capped at P3 with a `subclass:<name>` annotation. Concrete evidence is the burden of proof, not optimism.
+
+- After Step D, add a new Step E:
+
+ > **Step E: Counter-check question.** Ask explicitly: "If I implemented this fix, would it actively hurt any API that doesn't have this pattern?" If yes, the fix needs a guard or condition before being P1/P2 — not a default change.
+
+- Add a new Step F:
+
+ > **Step F: Recurrence-cost check.** Search prior retros under `~/printing-press/manuscripts/*/proofs/*-retro-*.md` for the same finding. If the same finding has been raised in 2+ prior retros without being implemented, the prior cost-benefit math has been "no" twice. Don't re-raise it at the same priority — either move to P3 with a "raised N times, still not justified" annotation, or reframe the finding into a smaller incremental fix.
+
+- In the cardinal rules at the top of SKILL.md, add a counterweight to "Bias toward fixing":
+
+ > **Bias toward fixing only when the fix would help APIs you can name.** "20% of catalog" without naming three concrete APIs is not evidence — it's optimism. The retro is a triage tool, not a wishlist.
+
+**Patterns to follow:**
+- The existing prose-style rule wording in `skills/printing-press-retro/SKILL.md::Cardinal Rules`. Match the tone of the existing five rules.
+
+**Test scenarios:**
+- Happy path: an agent retro-ing a CLI tries to classify a finding as P2 with frequency "API subclass: ~20% of catalog". The Step B gate forces them to name three concrete APIs. If they can only name two, the finding lands as Skipped or P3.
+- Happy path: a finding with clear cross-API evidence (e.g., F1's "every cache-enabled CLI") sails through the gate unchanged.
+- Edge case: a finding raised previously (2+ prior retros) cannot be re-raised at P2; agent must reframe or downgrade.
+- Negative: existing well-supported findings in the retro template aren't disrupted.
+
+**Verification:**
+- The next retro run after this change produces a findings list where every P1/P2 finding names three concrete APIs in its "Frequency" or "Cross-API check" field.
+- Findings that fail the gate appear in Skipped with a `subclass:<name>` tag rather than ghosted into the P-buckets.
+- A re-retroed CLI doesn't re-raise the same finding at the same priority without a recurrence-cost annotation.
+
+**Test expectation: none — skill instruction changes are not unit-testable. The verification is observational on the next retro session.**
+
+---
+
+## System-Wide Impact
+
+- **Interaction graph:** U1 (freshness paths) and U7 (helper deletion) both touch generator templates that affect every CLI generation. U2 (sanitizer) intercepts every render. U3 (reimplementation_check) runs on every dogfood. U4 (sync warn) modifies behavior in every printed CLI's sync command. U5 (limit truncation) modifies generation for every list-endpoint command. U6 (path-validity) affects every dogfood report. U8 (retro skill) affects every retro session.
+- **Error propagation:** U2 introduces a new error class (render-time control-byte rejection) that aborts generation. Generators exit non-zero with a clear file:offset:reason message. No silent swallowing.
+- **State lifecycle risks:** None. No persistent state changes.
+- **API surface parity:** U4 changes `UpsertBatch`'s return signature (`error` → `(int, error)`). All call sites in the generated `sync.go` need updating in the same template change. No external callers to `UpsertBatch` exist (it's per-CLI-generated code). Verify by grep across `internal/generator/templates/`.
+- **Integration coverage:** U1 + U2 + U5 all need to be exercised against the existing `golden-api.yaml` fixture. Run `scripts/golden.sh verify` after each unit; expect intentional fixture updates for U1 and U5.
+- **Unchanged invariants:**
+ - Cobra runtime command resolution is unchanged. The `auto_refresh.go` runtime map keeps its no-op fallback variants.
+ - Existing scorecard scoring rules are unchanged. U3 only changes the boolean signal feeding into reimplementation classification; the score weights stay the same.
+ - The synthetic-spec carve-out in dogfood (U6) is unchanged — internal-yaml is added as a peer condition, not a replacement.
+
+---
+
+## Risks & Dependencies
+
+| Risk | Mitigation |
+|------|------------|
+| U2 sanitizer false-positives on legitimate template output (rare control bytes in spec content) | Test against every CLI in `~/printing-press/library/` during implementation. Allow `\t`, `\n`, `\r` explicitly. If a real output legitimately needs a control byte, error message names it precisely so the spec author can fix the input. |
+| U4's `UpsertBatch` signature change ripples to other template files | Grep all uses across `internal/generator/templates/*.tmpl` before changing the signature. Update every caller in the same commit. Pre-existing CLIs in the library don't matter (each CLI's generated code is internally consistent). |
+| U5 inadvertently double-truncates on APIs that appear unpaginated but secretly honor limit | The truncation is idempotent — if the API returns `limit` items already, `truncateJSONArray` is a no-op. False positives are harmless. False negatives (paginated API mistakenly considered unpaginated) would result in client-side truncation when not strictly needed; still correct, just slightly less efficient. |
+| U8 retro-skill changes don't actually change agent behavior because instructions are advisory | Mitigate by including concrete-language requirements ("name three", "search prior retros") rather than vague guidance. Calibrate by retroing the next 2-3 CLIs and observing whether the rigor improved. If not, escalate to a structural fix (e.g., a script that grep-searches prior retros for duplicate findings). |
+| Golden fixtures need updating in U1 and U5; mistaking those updates for genuine drift | Run `scripts/golden.sh verify` BEFORE making the code change, then again AFTER. Compare diffs to confirm only the expected fixtures changed. Document the expected diff in the commit message. Only after manual diff inspection, run `scripts/golden.sh update`. |
+
+---
+
+## Documentation / Operational Notes
+
+- **AGENTS.md guidance check:** AGENTS.md says "When you change code, check for a `_test.go` file in the same package." Each unit lists the corresponding test file.
+- **Pre-commit / pre-push hooks:** repo has lefthook hooks (`go fmt -w`, `golangci-lint`). Each unit's implementation should pass these locally before committing. AGENTS.md warns against `gofmt -w .` from repo root (rewrites golden fixtures); use `go fmt ./...` per package patterns.
+- **Skill workflow parity (AGENTS.md):** U2 changes generator behavior in a way that affects what the skill should warn about. Update `skills/printing-press/SKILL.md` Phase 1.5e in the same change as U2's generator code.
+- **Commit hygiene:** Use the repo's commit-style convention from AGENTS.md. Each unit lands as its own commit with a `fix(cli):` or `fix(skills):` scope. Multi-subsystem units (U2 touches both `internal/generator/` and `skills/`) use `fix(cli):` since the generator change is the load-bearing half. U8 is `fix(skills):` since it's skill-only.
+- **Release impact:** None of these changes are user-visible API breaks. They're behavior fixes and bug-elimination. Standard release-please flow applies.
+
+---
+
+## Sources & References
+
+- **Origin document:** `~/printing-press/manuscripts/hackernews/20260427-120911/proofs/20260427-130958-retro-hackernews-pp-cli.md`
+- **GitHub issue:** [mvanhorn/cli-printing-press#350](https://github.com/mvanhorn/cli-printing-press/issues/350)
+- **Related code:**
+ - `internal/generator/generator.go::freshnessCommandPaths` (U1)
+ - `internal/generator/generator.go::generate` rendering pipeline (U2)
+ - `internal/pipeline/reimplementation_check.go` (U3)
+ - `internal/generator/templates/sync.go.tmpl` and `store.go.tmpl` (U4)
+ - `internal/generator/templates/` list-endpoint command templates (U5)
+ - `internal/pipeline/dogfood.go` (U6)
+ - `internal/generator/templates/helpers.go.tmpl` (U7)
+ - `skills/printing-press-retro/SKILL.md` (U8)
+- **Related published artifact:** [Hackernews regen PR #139](https://github.com/mvanhorn/printing-press-library/pull/139) — the printed CLI whose generation surfaced these findings.
+- **Prior recurring findings:** `docs/retros/2026-04-13-recipe-goat-retro.md`, movie-goat retro F8, hackernews v1.3.3 retro F3 (all surfaced the multi-base-URL spec gap that U8's recurrence-cost check is designed to catch).
diff --git a/internal/generator/freshness_paths_test.go b/internal/generator/freshness_paths_test.go
new file mode 100644
index 00000000..bbaa07e9
--- /dev/null
+++ b/internal/generator/freshness_paths_test.go
@@ -0,0 +1,168 @@
+package generator
+
+import (
+ "sort"
+ "testing"
+
+ "github.com/mvanhorn/cli-printing-press/v2/internal/profiler"
+ "github.com/mvanhorn/cli-printing-press/v2/internal/spec"
+ "github.com/stretchr/testify/assert"
+)
+
+// TestFreshnessCommandPaths_PromotedResourceEmitsBareOnly verifies that a
+// resource with a single endpoint (which the generator promotes to the
+// resource-level command) emits only the bare `<cli> <resource>` form in
+// the rendered freshness paths. Phantom variants like `<cli> <resource> list`
+// must NOT appear because the user can't invoke them — Cobra doesn't
+// register `list` as a subcommand for promoted resources.
+//
+// Regression guard for retro #350 finding F1: prior behavior emitted
+// `<resource> list / get / search` for every syncable resource regardless
+// of whether those subcommands actually exist.
+func TestFreshnessCommandPaths_PromotedResourceEmitsBareOnly(t *testing.T) {
+ t.Parallel()
+
+ apiSpec := minimalSpec("hn")
+ apiSpec.Cache.Enabled = true
+ // `ask` has a single endpoint named `list` — the generator will promote
+ // it. Freshness paths should emit only the bare `hn-pp-cli ask`.
+ apiSpec.Resources["ask"] = spec.Resource{
+ Description: "Ask HN",
+ Endpoints: map[string]spec.Endpoint{
+ "list": {Method: "GET", Path: "/askstories.json"},
+ },
+ }
+
+ g := &Generator{
+ Spec: apiSpec,
+ profile: &profiler.APIProfile{
+ SyncableResources: []profiler.SyncableResource{
+ {Name: "ask", Path: "/askstories.json"},
+ },
+ },
+ PromotedResourceNames: map[string]bool{"ask": true},
+ PromotedEndpointNames: map[string]string{"ask": "list"},
+ }
+
+ got := g.freshnessCommandPaths()
+ assert.Equal(t, []string{"hn-pp-cli ask"}, got,
+ "promoted single-endpoint resource should emit only the bare path")
+}
+
+// TestFreshnessCommandPaths_MultiEndpointResourceEmitsRealEndpointsOnly
+// verifies that a multi-endpoint resource emits the bare path plus one
+// entry per real endpoint name — and nothing else. The hardcoded
+// `list / get / search` variants from the prior implementation must NOT
+// appear unless those endpoint names actually exist on the resource.
+func TestFreshnessCommandPaths_MultiEndpointResourceEmitsRealEndpointsOnly(t *testing.T) {
+ t.Parallel()
+
+ apiSpec := minimalSpec("hn")
+ apiSpec.Cache.Enabled = true
+ // `stories` has 4 endpoints: top, new, best, get. None of them are
+ // `list` or `search`. The freshness paths must reflect that exactly.
+ apiSpec.Resources["stories"] = spec.Resource{
+ Description: "Stories",
+ Endpoints: map[string]spec.Endpoint{
+ "top": {Method: "GET", Path: "/topstories.json"},
+ "new": {Method: "GET", Path: "/newstories.json"},
+ "best": {Method: "GET", Path: "/beststories.json"},
+ "get": {Method: "GET", Path: "/item/{itemId}.json"},
+ },
+ }
+
+ g := &Generator{
+ Spec: apiSpec,
+ profile: &profiler.APIProfile{
+ SyncableResources: []profiler.SyncableResource{
+ {Name: "stories", Path: "/topstories.json"},
+ },
+ },
+ PromotedResourceNames: map[string]bool{},
+ }
+
+ got := g.freshnessCommandPaths()
+ sort.Strings(got)
+
+ want := []string{
+ "hn-pp-cli stories",
+ "hn-pp-cli stories best",
+ "hn-pp-cli stories get",
+ "hn-pp-cli stories new",
+ "hn-pp-cli stories top",
+ }
+ assert.Equal(t, want, got, "should emit only real endpoint names")
+
+ // Negative assertions: the prior phantom variants must NOT be present.
+ for _, phantom := range []string{
+ "hn-pp-cli stories list",
+ "hn-pp-cli stories search",
+ } {
+ assert.NotContains(t, got, phantom,
+ "phantom path %q must not appear — endpoint does not exist on the resource", phantom)
+ }
+}
+
+// TestFreshnessCommandPaths_CacheCommandsAdded verifies that explicit
+// custom command paths declared in spec.Cache.Commands are still emitted
+// alongside the syncable-resource paths.
+func TestFreshnessCommandPaths_CacheCommandsAdded(t *testing.T) {
+ t.Parallel()
+
+ apiSpec := minimalSpec("hn")
+ apiSpec.Cache.Enabled = true
+ apiSpec.Cache.Commands = []spec.CacheCommand{
+ {Name: "controversial", Resources: []string{"stories"}},
+ }
+ apiSpec.Resources["stories"] = spec.Resource{
+ Description: "Stories",
+ Endpoints: map[string]spec.Endpoint{
+ "top": {Method: "GET", Path: "/topstories.json"},
+ },
+ }
+
+ g := &Generator{
+ Spec: apiSpec,
+ profile: &profiler.APIProfile{
+ SyncableResources: []profiler.SyncableResource{
+ {Name: "stories", Path: "/topstories.json"},
+ },
+ },
+ PromotedResourceNames: map[string]bool{"stories": true},
+ PromotedEndpointNames: map[string]string{"stories": "top"},
+ }
+
+ got := g.freshnessCommandPaths()
+ sort.Strings(got)
+
+ assert.Contains(t, got, "hn-pp-cli stories",
+ "bare resource path should be present")
+ assert.Contains(t, got, "hn-pp-cli controversial",
+ "explicit Cache.Commands entry should be emitted")
+}
+
+// TestFreshnessCommandPaths_DisabledReturnsNil verifies the early return
+// when cache is disabled.
+func TestFreshnessCommandPaths_DisabledReturnsNil(t *testing.T) {
+ t.Parallel()
+
+ apiSpec := minimalSpec("hn")
+ apiSpec.Cache.Enabled = false
+
+ g := &Generator{Spec: apiSpec, profile: &profiler.APIProfile{}}
+ assert.Nil(t, g.freshnessCommandPaths(),
+ "should return nil when cache is disabled")
+}
+
+// TestFreshnessCommandPaths_NoProfileReturnsNil verifies the early return
+// when no profile has been computed (defensive).
+func TestFreshnessCommandPaths_NoProfileReturnsNil(t *testing.T) {
+ t.Parallel()
+
+ apiSpec := minimalSpec("hn")
+ apiSpec.Cache.Enabled = true
+
+ g := &Generator{Spec: apiSpec, profile: nil}
+ assert.Nil(t, g.freshnessCommandPaths(),
+ "should return nil when profile is missing")
+}
diff --git a/internal/generator/generator.go b/internal/generator/generator.go
index a9496516..627a0e74 100644
--- a/internal/generator/generator.go
+++ b/internal/generator/generator.go
@@ -260,11 +260,20 @@ func New(s *spec.APISpec, outputDir string) *Generator {
},
"jsonStringParam": isJSONStringParam,
"jsonEnumSuggestion": jsonEnumSuggestion,
- "envName": naming.EnvPrefix,
- "safeName": safeSQLName,
- "isBackfillColumn": isStoreBackfillColumn,
- "hasBackfillColumns": hasStoreBackfillColumns,
- "backfillDecl": storeBackfillDecl,
+ // endpointNeedsClientLimit reports whether a list endpoint needs
+ // client-side truncation. True when the endpoint has a `limit`-named
+ // param AND no Pagination block — the spec author asked for a
+ // limit flag, but didn't declare a server-side paginator. Many
+ // APIs (Firebase, file-backed JSON dumps, RSS feeds) accept a
+ // `?limit=N` query param without honoring it; truncating client-
+ // side means the user-facing --limit flag works regardless.
+ // Surfaced by hackernews retro #350 finding F6.
+ "endpointNeedsClientLimit": endpointNeedsClientLimit,
+ "envName": naming.EnvPrefix,
+ "safeName": safeSQLName,
+ "isBackfillColumn": isStoreBackfillColumn,
+ "hasBackfillColumns": hasStoreBackfillColumns,
+ "backfillDecl": storeBackfillDecl,
"safeNameSuffix": func(name, suffix string) string {
return safeSQLName(name + suffix)
},
@@ -489,6 +498,7 @@ type HelperFlags struct {
HasPathParams bool // spec has path parameters → emit replacePathParam
HasMultiPositional bool // spec has endpoints with 2+ positional params → emit usageErr
HasDataLayer bool // CLI has a local store (sync/search) → emit provenance helpers
+ HasClientLimit bool // at least one endpoint needs client-side limit truncation → emit truncateJSONArray
}
// computeHelperFlags scans the spec's resources to determine which helpers are needed.
@@ -499,6 +509,9 @@ func computeHelperFlags(s *spec.APISpec) HelperFlags {
if strings.EqualFold(e.Method, "DELETE") {
flags.HasDelete = true
}
+ if endpointNeedsClientLimit(e) {
+ flags.HasClientLimit = true
+ }
positionalCount := 0
for _, p := range e.Params {
if p.Positional {
@@ -515,6 +528,9 @@ func computeHelperFlags(s *spec.APISpec) HelperFlags {
if strings.EqualFold(e.Method, "DELETE") {
flags.HasDelete = true
}
+ if endpointNeedsClientLimit(e) {
+ flags.HasClientLimit = true
+ }
positionalCount := 0
for _, p := range e.Params {
if p.Positional {
@@ -633,6 +649,19 @@ func (g *Generator) readmeData() *readmeTemplateData {
}
}
+// freshnessCommandPaths returns the rendered slice of "covered command paths"
+// surfaced in user-facing docs (README.md and SKILL.md) for the freshness
+// section. The slice contains only paths whose subcommands actually exist in
+// the generated CLI — promoted single-endpoint resources emit only the bare
+// `<cli> <resource>` form, multi-endpoint resources emit the bare form plus
+// one entry per real endpoint name.
+//
+// The runtime fallback map in `internal/cli/auto_refresh.go` (rendered by
+// auto_refresh.go.tmpl) keeps its `<resource> list/get/search` no-op
+// variants because Cobra's argument resolution can land on any of them at
+// runtime — having the map accept those forms keeps freshness lookups
+// loose. Only the slice rendered into docs needs trimming, so users and
+// agents don't see phantom subcommands they can't actually invoke.
func (g *Generator) freshnessCommandPaths() []string {
if !g.Spec.Cache.Enabled || g.profile == nil {
return nil
@@ -646,15 +675,35 @@ func (g *Generator) freshnessCommandPaths() []string {
seen[path] = struct{}{}
paths = append(paths, path)
}
+ cliName := naming.CLI(g.Spec.Name)
for _, resource := range g.profile.SyncableResources {
- prefix := naming.CLI(g.Spec.Name) + " " + resource.Name
+ prefix := cliName + " " + resource.Name
+ // Always emit the bare `<cli> <resource>` form. For promoted
+ // single-endpoint resources Cobra resolves this to the leaf
+ // command; for multi-endpoint resources it resolves to the
+ // parent help. Both are real, reachable paths.
add(prefix)
- add(prefix + " list")
- add(prefix + " get")
- add(prefix + " search")
+
+ // Promoted resources have only one underlying endpoint and it
+ // is wired directly to the bare command — emitting endpoint
+ // names would create phantom paths users can't invoke.
+ if g.PromotedResourceNames[resource.Name] {
+ continue
+ }
+
+ // For multi-endpoint resources, emit one entry per real endpoint
+ // name. The endpoint map key matches the generated subcommand
+ // name (e.g., a `top` endpoint becomes `<cli> stories top`).
+ specResource, ok := g.Spec.Resources[resource.Name]
+ if !ok {
+ continue
+ }
+ for endpointName := range specResource.Endpoints {
+ add(prefix + " " + endpointName)
+ }
}
for _, command := range g.Spec.Cache.Commands {
- add(naming.CLI(g.Spec.Name) + " " + command.Name)
+ add(cliName + " " + command.Name)
}
sort.Strings(paths)
return paths
@@ -1826,6 +1875,44 @@ func validateRenderedArtifact(outPath, content string) error {
return fmt.Errorf("%s contains unsubstituted placeholder %q", outPath, marker)
}
}
+ if err := scanForControlBytes(outPath, content); err != nil {
+ return err
+ }
+ return nil
+}
+
+// scanForControlBytes rejects any rendered markdown that contains ASCII
+// control bytes outside the small set legitimately used in text:
+// 0x09 (tab), 0x0A (LF), 0x0D (CR). Everything else in 0x00-0x1F is
+// rejected with the file path, byte offset, and a hint about the most
+// likely cause.
+//
+// Why: research.json values flow through Go's encoding/json which honors
+// JSON escape sequences like "\b" (0x08 backspace), "\f" (0x0C form
+// feed), etc. When an agent author writes a regex literal as
+// `"command": "...\bGo\b..."` (intending the literal characters
+// `\` `b` `G` `o` `\` `b`) the JSON parser yields a string containing
+// real backspace bytes, and the template engine writes those bytes
+// straight into SKILL.md / README.md. The result renders as nothing in
+// most viewers — silent corruption.
+//
+// The fix runs at render time (not at JSON parse time) so it catches
+// every future class of escape mistake, not just the regex case that
+// surfaced it. A targeted check on `narrative.recipes[].command`
+// alone would only catch this one path.
+//
+// Surfaced by hackernews retro #350 finding F2.
+func scanForControlBytes(outPath, content string) error {
+ for i := 0; i < len(content); i++ {
+ b := content[i]
+ // Tab (0x09), LF (0x0A), CR (0x0D) are allowed in markdown.
+ // Everything else in 0x00-0x1F is forbidden.
+ if b > 0x1F || b == 0x09 || b == 0x0A || b == 0x0D {
+ continue
+ }
+ hint := "likely cause: a JSON-parsed field (e.g. narrative.recipes[].command) contained \"\\b\", \"\\f\", or another JSON escape that became a control byte. Double-escape backslashes in regex literals: \"\\\\b\" not \"\\b\"."
+ return fmt.Errorf("%s contains forbidden control byte 0x%02X at offset %d. %s", outPath, b, i, hint)
+ }
return nil
}
@@ -2060,6 +2147,40 @@ type jsonFlagSuggestion struct {
Values []string
}
+// endpointNeedsClientLimit reports whether a list endpoint needs
+// client-side response truncation. True when:
+// - method is GET (only read endpoints need truncation)
+// - the endpoint has a non-positional `limit` param (the user-facing
+// --limit flag exists)
+// - no Pagination block is declared (the spec author hasn't told us
+// the API actually paginates)
+//
+// When all three conditions hold, the generator emits a
+// truncateJSONArray call after the API response returns so --limit N
+// is honored even when the API ignores ?limit=N. APIs like Firebase
+// and various file-backed JSON endpoints accept the query param
+// without applying it server-side; the truncation is harmless when
+// the API DID return only N items already (idempotent).
+//
+// Surfaced by hackernews retro #350 finding F6.
+func endpointNeedsClientLimit(endpoint spec.Endpoint) bool {
+ if !strings.EqualFold(strings.TrimSpace(endpoint.Method), "GET") {
+ return false
+ }
+ if endpoint.Pagination != nil {
+ return false
+ }
+ for _, p := range endpoint.Params {
+ if p.Positional || p.PathParam {
+ continue
+ }
+ if strings.EqualFold(strings.TrimSpace(p.Name), "limit") {
+ return true
+ }
+ }
+ return false
+}
+
func isJSONStringParam(p spec.Param) bool {
if p.Type != "string" {
return false
diff --git a/internal/generator/limit_truncation_test.go b/internal/generator/limit_truncation_test.go
new file mode 100644
index 00000000..2da0f282
--- /dev/null
+++ b/internal/generator/limit_truncation_test.go
@@ -0,0 +1,98 @@
+package generator
+
+import (
+ "testing"
+
+ "github.com/mvanhorn/cli-printing-press/v2/internal/spec"
+ "github.com/stretchr/testify/assert"
+)
+
+// TestEndpointNeedsClientLimit verifies the gate that controls when
+// generated GET commands emit a `truncateJSONArray(data, flagLimit)` call
+// after the API response returns. This is the core decision behind
+// hackernews retro #350 finding F6 — APIs that accept ?limit=N without
+// honoring it (Firebase, file dumps, RSS) silently broke --limit until
+// the generator started truncating client-side.
+func TestEndpointNeedsClientLimit(t *testing.T) {
+ t.Parallel()
+
+ mkParams := func(p ...spec.Param) []spec.Param { return p }
+ limitParam := spec.Param{Name: "limit", Type: "int"}
+ otherParam := spec.Param{Name: "page", Type: "int"}
+ positionalLimit := spec.Param{Name: "limit", Type: "int", Positional: true}
+ pathLimit := spec.Param{Name: "limit", Type: "int", PathParam: true}
+
+ cases := []struct {
+ name string
+ endpoint spec.Endpoint
+ want bool
+ }{
+ {
+ name: "GET with limit and no pagination → truncate",
+ endpoint: spec.Endpoint{Method: "GET", Params: mkParams(limitParam)},
+ want: true,
+ },
+ {
+ name: "GET with limit and pagination block → defer to API",
+ endpoint: spec.Endpoint{Method: "GET", Params: mkParams(limitParam), Pagination: &spec.Pagination{}},
+ want: false,
+ },
+ {
+ name: "GET with no limit param → no truncate",
+ endpoint: spec.Endpoint{Method: "GET", Params: mkParams(otherParam)},
+ want: false,
+ },
+ {
+ name: "GET with no params → no truncate",
+ endpoint: spec.Endpoint{Method: "GET"},
+ want: false,
+ },
+ {
+ name: "POST with limit param → no truncate (mutation, not list)",
+ endpoint: spec.Endpoint{Method: "POST", Params: mkParams(limitParam)},
+ want: false,
+ },
+ {
+ name: "PUT with limit param → no truncate",
+ endpoint: spec.Endpoint{Method: "PUT", Params: mkParams(limitParam)},
+ want: false,
+ },
+ {
+ name: "DELETE with limit param → no truncate",
+ endpoint: spec.Endpoint{Method: "DELETE", Params: mkParams(limitParam)},
+ want: false,
+ },
+ {
+ name: "GET with positional 'limit' → no truncate (not a flag)",
+ endpoint: spec.Endpoint{Method: "GET", Params: mkParams(positionalLimit)},
+ want: false,
+ },
+ {
+ name: "GET with path-param 'limit' → no truncate",
+ endpoint: spec.Endpoint{Method: "GET", Params: mkParams(pathLimit)},
+ want: false,
+ },
+ {
+ name: "GET with mixed params including limit → truncate",
+ endpoint: spec.Endpoint{Method: "GET", Params: mkParams(otherParam, limitParam)},
+ want: true,
+ },
+ {
+ name: "lowercase method get → truncate (case-insensitive method check)",
+ endpoint: spec.Endpoint{Method: "get", Params: mkParams(limitParam)},
+ want: true,
+ },
+ {
+ name: "uppercase param Limit → truncate (case-insensitive param-name check)",
+ endpoint: spec.Endpoint{Method: "GET", Params: mkParams(spec.Param{Name: "Limit", Type: "int"})},
+ want: true,
+ },
+ }
+
+ for _, tc := range cases {
+ t.Run(tc.name, func(t *testing.T) {
+ got := endpointNeedsClientLimit(tc.endpoint)
+ assert.Equal(t, tc.want, got)
+ })
+ }
+}
diff --git a/internal/generator/sanitizer_test.go b/internal/generator/sanitizer_test.go
new file mode 100644
index 00000000..c051774c
--- /dev/null
+++ b/internal/generator/sanitizer_test.go
@@ -0,0 +1,113 @@
+package generator
+
+import (
+ "strings"
+ "testing"
+
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+)
+
+// TestScanForControlBytes_AllowsNormalText verifies that ordinary markdown
+// content with no control bytes passes the scan unchanged.
+func TestScanForControlBytes_AllowsNormalText(t *testing.T) {
+ t.Parallel()
+
+ cases := []struct {
+ name string
+ content string
+ }{
+ {"plain prose", "# Hello\n\nThis is a normal SKILL.md."},
+ {"with tab", "| col1\tcol2 |\n"},
+ {"with CRLF", "line1\r\nline2"},
+ {"unicode", "—curly quotes — em dash — ✓"},
+ {"backticks and pipes", "`hackernews-pp-cli stories top` | `--limit N` |"},
+ {"empty", ""},
+ }
+ for _, tc := range cases {
+ t.Run(tc.name, func(t *testing.T) {
+ require.NoError(t, scanForControlBytes("SKILL.md", tc.content))
+ })
+ }
+}
+
+// TestScanForControlBytes_RejectsBackspace is the regression guard for
+// the exact bug surfaced by hackernews retro #350: a recipe's regex
+// `\bGo\b` parsed by JSON as backspace bytes leaked into the rendered
+// SKILL.md. The scanner must reject this with a clear error pointing
+// at the offset and explaining the likely cause.
+func TestScanForControlBytes_RejectsBackspace(t *testing.T) {
+ t.Parallel()
+
+ content := "Recipe: `hackernews-pp-cli hiring '(remote).*\bGo\b'`"
+ err := scanForControlBytes("SKILL.md", content)
+ require.Error(t, err)
+ assert.Contains(t, err.Error(), "SKILL.md")
+ assert.Contains(t, err.Error(), "0x08")
+ // The offset of the first \b in the string above (after "remote).*"):
+ expectedOffset := strings.Index(content, "\b")
+ require.GreaterOrEqual(t, expectedOffset, 0)
+ assert.Contains(t, err.Error(), "JSON escape",
+ "error should hint at the JSON-escape root cause")
+ assert.Contains(t, err.Error(), "Double-escape backslashes",
+ "error should give the actionable fix")
+}
+
+// TestScanForControlBytes_RejectsAllForbiddenBytes verifies every byte
+// in 0x00-0x1F (except 0x09, 0x0A, 0x0D) triggers the rejection.
+func TestScanForControlBytes_RejectsAllForbiddenBytes(t *testing.T) {
+ t.Parallel()
+
+ for b := 0; b <= 0x1F; b++ {
+ if b == 0x09 || b == 0x0A || b == 0x0D {
+ continue // explicitly allowed
+ }
+ content := "before" + string(rune(b)) + "after"
+ err := scanForControlBytes("SKILL.md", content)
+ assert.Error(t, err, "byte 0x%02X should be rejected", b)
+ }
+}
+
+// TestScanForControlBytes_AllowsAllowedControlBytes verifies the
+// explicit allowlist (tab, newline, CR) passes the scan.
+func TestScanForControlBytes_AllowsAllowedControlBytes(t *testing.T) {
+ t.Parallel()
+
+ for _, b := range []byte{0x09, 0x0A, 0x0D} {
+ content := "before" + string(rune(b)) + "after"
+ require.NoError(t, scanForControlBytes("SKILL.md", content),
+ "byte 0x%02X should be allowed", b)
+ }
+}
+
+// TestScanForControlBytes_OnlyMarkdown verifies the scanner is wired
+// only into README.md / SKILL.md via validateRenderedArtifact — other
+// rendered files (Go source, JSON manifests) are not subject to this
+// check because they have their own validation paths.
+func TestScanForControlBytes_OnlyMarkdown(t *testing.T) {
+ t.Parallel()
+
+ contentWithBackspace := "before\bafter"
+ // validateRenderedArtifact dispatches by filename — non-markdown
+ // files should pass without scanning.
+ require.NoError(t, validateRenderedArtifact("internal/cli/root.go", contentWithBackspace),
+ "Go source files are out of scope")
+ require.NoError(t, validateRenderedArtifact("tools-manifest.json", contentWithBackspace),
+ "JSON manifests are out of scope")
+ // Markdown files trigger the scan.
+ require.Error(t, validateRenderedArtifact("README.md", contentWithBackspace))
+ require.Error(t, validateRenderedArtifact("SKILL.md", contentWithBackspace))
+}
+
+// TestScanForControlBytes_OffsetIsAccurate verifies the error message
+// names the byte offset of the first forbidden byte, not the last.
+// Implementers fixing the input need to find the right field.
+func TestScanForControlBytes_OffsetIsAccurate(t *testing.T) {
+ t.Parallel()
+
+ content := "0123456789\babcdef\bghi"
+ err := scanForControlBytes("SKILL.md", content)
+ require.Error(t, err)
+ assert.Contains(t, err.Error(), "offset 10",
+ "should report the offset of the first \\b, not subsequent ones")
+}
diff --git a/internal/generator/templates/auto_refresh.go.tmpl b/internal/generator/templates/auto_refresh.go.tmpl
index fff303a9..7d262bca 100644
--- a/internal/generator/templates/auto_refresh.go.tmpl
+++ b/internal/generator/templates/auto_refresh.go.tmpl
@@ -146,8 +146,8 @@ func autoRefreshIfStale(ctx context.Context, flags *rootFlags, resources []strin
// ensureFreshForResources lets hand-authored commands participate in the same
// freshness hook as generated resource commands. Custom commands should call
-// this before reading from the store, then use wrapWithProvenance or
-// wrapResultsWithFreshness for JSON output if they want meta.freshness.
+// this before reading from the store, then use wrapWithProvenance for JSON
+// output if they want meta.freshness.
func ensureFreshForResources(ctx context.Context, flags *rootFlags, resources ...string) cliutil.FreshnessMeta {
meta := autoRefreshIfStale(ctx, flags, resources)
flags.freshnessMeta = meta
diff --git a/internal/generator/templates/command_endpoint.go.tmpl b/internal/generator/templates/command_endpoint.go.tmpl
index 723b6c97..fa8a1bc8 100644
--- a/internal/generator/templates/command_endpoint.go.tmpl
+++ b/internal/generator/templates/command_endpoint.go.tmpl
@@ -345,6 +345,15 @@ func new{{camel .FuncPrefix}}{{camel .EndpointName}}Cmd(flags *rootFlags) *cobra
return classifyAPIError(err)
}
{{- end}}
+{{- if endpointNeedsClientLimit .Endpoint}}
+ // The API doesn't declare a paginator but accepts a limit
+ // query param. Some APIs (Firebase, file-backed JSON dumps,
+ // RSS feeds) silently ignore ?limit=N and return the full
+ // collection — truncate client-side so --limit N is
+ // honored regardless. Idempotent when the API already
+ // returned <= N items.
+ data = truncateJSONArray(data, flagLimit)
+{{- end}}
{{- if .Endpoint.UsesHTMLResponse}}
if !flags.dryRun {
diff --git a/internal/generator/templates/graphql_sync.go.tmpl b/internal/generator/templates/graphql_sync.go.tmpl
index e5d799b9..61b0e5d4 100644
--- a/internal/generator/templates/graphql_sync.go.tmpl
+++ b/internal/generator/templates/graphql_sync.go.tmpl
@@ -316,15 +316,25 @@ func syncResource(c *client.Client, db *store.Store, resource, sinceTS string, f
break
}
- // Batch upsert all items from this page
- if err := db.UpsertBatch(resource, conn.Nodes); err != nil {
+ // Batch upsert all items from this page. Track stored separately
+ // from consumed so we can warn when ID extraction fails for the
+ // whole batch.
+ stored, err := db.UpsertBatch(resource, conn.Nodes)
+ if err != nil {
if !humanFriendly {
fmt.Fprintf(os.Stderr, `{"event":"sync_error","resource":"%s","error":"%s"}`+"\n", resource, strings.ReplaceAll(err.Error(), `"`, `\"`))
}
return syncResult{Resource: resource, Count: totalCount, Err: fmt.Errorf("upserting batch for %s: %w", resource, err), Duration: time.Since(started)}
}
+ if len(conn.Nodes) > 0 && stored == 0 {
+ if humanFriendly {
+ fmt.Fprintf(os.Stderr, "warning: %s returned %d items but stored 0 — the local store will be empty for this resource. Likely cause: scalar item shape rather than objects with extractable IDs.\n", resource, len(conn.Nodes))
+ } else {
+ fmt.Fprintf(os.Stderr, `{"event":"sync_anomaly","resource":"%s","consumed":%d,"stored":0,"reason":"all_items_failed_id_extraction"}`+"\n", resource, len(conn.Nodes))
+ }
+ }
- totalCount += len(conn.Nodes)
+ totalCount += stored
// Progress reporting
if humanFriendly {
diff --git a/internal/generator/templates/helpers.go.tmpl b/internal/generator/templates/helpers.go.tmpl
index e99c2937..d9577bb2 100644
--- a/internal/generator/templates/helpers.go.tmpl
+++ b/internal/generator/templates/helpers.go.tmpl
@@ -1260,17 +1260,37 @@ func wrapWithProvenance(data json.RawMessage, prov DataProvenance) (json.RawMess
return json.Marshal(envelope)
}
-// wrapResultsWithFreshness gives hand-authored commands a small opt-in helper
-// for the generated provenance envelope without forcing arbitrary custom JSON
-// outputs to change shape.
-func wrapResultsWithFreshness(data json.RawMessage, flags *rootFlags) (json.RawMessage, error) {
- prov := DataProvenance{}
- if flags != nil {
- prov.Source = flags.dataSource
- prov.Freshness = flags.freshnessMeta
- }
- return wrapWithProvenance(data, prov)
+{{- if .HasClientLimit}}
+
+// truncateJSONArray returns a JSON array containing at most n elements
+// from the input. When n <= 0, when the input isn't a JSON array, or
+// when the array is already at-or-below the limit, the input is
+// returned unchanged.
+//
+// Used by list-endpoint commands whose API ignores the ?limit=N query
+// param (e.g. Firebase-style endpoints that return the full collection
+// regardless of the param). The truncation is idempotent — calling it
+// when the API already honored the limit is a no-op. The generator
+// only emits the call when the spec declares a `limit` param without a
+// Pagination block, so paginated APIs are unaffected.
+func truncateJSONArray(data json.RawMessage, n int) json.RawMessage {
+ if n <= 0 {
+ return data
+ }
+ var arr []json.RawMessage
+ if err := json.Unmarshal(data, &arr); err != nil {
+ return data
+ }
+ if len(arr) <= n {
+ return data
+ }
+ out, err := json.Marshal(arr[:n])
+ if err != nil {
+ return data
+ }
+ return out
}
+{{- end}}
// defaultDBPath returns the canonical path for the local SQLite database.
func defaultDBPath(name string) string {
diff --git a/internal/generator/templates/store.go.tmpl b/internal/generator/templates/store.go.tmpl
index 4d739bf1..d9f94013 100644
--- a/internal/generator/templates/store.go.tmpl
+++ b/internal/generator/templates/store.go.tmpl
@@ -507,25 +507,34 @@ func (s *Store) Upsert{{pascal .Name}}(data json.RawMessage) error {
{{- end}}
{{- end}}
-// UpsertBatch inserts or replaces multiple records in a single transaction.
-// This is 10-100x faster than individual Upsert calls for bulk operations.
+// UpsertBatch inserts or replaces multiple records in a single transaction
+// and returns the count of items actually stored. This is 10-100x faster
+// than individual Upsert calls for bulk operations.
+//
+// The returned count lets sync log the honest stored-row count rather
+// than the consumed-item count. When the API returns scalar IDs rather
+// than objects (Firebase /topstories.json, GitHub user-repo lists, etc.)
+// every item fails ID extraction and stored=0 even when consumed=N —
+// callers can detect this divergence and warn loudly instead of
+// silently reporting "N synced".
//
// For resource types that have a domain-specific typed table, the per-item
// generic insert is followed by a dispatch to the matching upsert<Pascal>Tx
// inside the same transaction. Without that dispatch, paginated syncs would
// only populate the generic resources table — typed tables (and indexed
// columns like parent_id added by dependent-resource sync) would stay empty.
-func (s *Store) UpsertBatch(resourceType string, items []json.RawMessage) error {
+func (s *Store) UpsertBatch(resourceType string, items []json.RawMessage) (int, error) {
tx, err := s.db.Begin()
if err != nil {
- return fmt.Errorf("starting batch transaction: %w", err)
+ return 0, fmt.Errorf("starting batch transaction: %w", err)
}
defer tx.Rollback()
- var skippedCount int
+ var stored, skippedCount int
for _, item := range items {
var obj map[string]any
if err := json.Unmarshal(item, &obj); err != nil {
+ skippedCount++
continue
}
// Try common primary key field names in priority order.
@@ -545,7 +554,7 @@ func (s *Store) UpsertBatch(resourceType string, items []json.RawMessage) error
}
if err := s.upsertGenericResourceTx(tx, resourceType, id, item); err != nil {
- return fmt.Errorf("upserting %s/%s: %w", resourceType, id, err)
+ return 0, fmt.Errorf("upserting %s/%s: %w", resourceType, id, err)
}
switch resourceType {
@@ -553,11 +562,12 @@ func (s *Store) UpsertBatch(resourceType string, items []json.RawMessage) error
{{- if and (gt (len .Columns) 3) (ne .Name "sync_state") (hasDomainUpsert .Name)}}
case "{{.Name}}":
if err := s.upsert{{pascal .Name}}Tx(tx, id, obj, item); err != nil {
- return fmt.Errorf("typed upsert for %s/%s: %w", resourceType, id, err)
+ return 0, fmt.Errorf("typed upsert for %s/%s: %w", resourceType, id, err)
}
{{- end}}
{{- end}}
}
+ stored++
}
// Warn when most items in a batch lack an extractable ID — this likely
@@ -566,7 +576,10 @@ func (s *Store) UpsertBatch(resourceType string, items []json.RawMessage) error
fmt.Fprintf(os.Stderr, "warning: %d/%d %s items skipped (no extractable ID field found)\n", skippedCount, len(items), resourceType)
}
- return tx.Commit()
+ if err := tx.Commit(); err != nil {
+ return 0, err
+ }
+ return stored, nil
}
{{- range .Tables}}
diff --git a/internal/generator/templates/store_upsert_batch_test.go.tmpl b/internal/generator/templates/store_upsert_batch_test.go.tmpl
index 5de1787f..f900dd9e 100644
--- a/internal/generator/templates/store_upsert_batch_test.go.tmpl
+++ b/internal/generator/templates/store_upsert_batch_test.go.tmpl
@@ -41,7 +41,7 @@ func TestUpsertBatch_Populates{{pascal .Name}}Table(t *testing.T) {
json.RawMessage(`{"id": "test-002"}`),
json.RawMessage(`{"id": "test-003"}`),
}
- if err := s.UpsertBatch("{{.Name}}", items); err != nil {
+ if _, err := s.UpsertBatch("{{.Name}}", items); err != nil {
t.Fatalf("UpsertBatch: %v", err)
}
@@ -82,7 +82,7 @@ func TestUpsertBatch_Sets{{pascal .Name}}ParentID(t *testing.T) {
json.RawMessage(`{"id": "child-002", "parent_id": "parent-A"}`),
json.RawMessage(`{"id": "child-003", "parent_id": "parent-B"}`),
}
- if err := s.UpsertBatch("{{.Name}}", items); err != nil {
+ if _, err := s.UpsertBatch("{{.Name}}", items); err != nil {
t.Fatalf("UpsertBatch: %v", err)
}
diff --git a/internal/generator/templates/sync.go.tmpl b/internal/generator/templates/sync.go.tmpl
index e237a912..e6304dcc 100644
--- a/internal/generator/templates/sync.go.tmpl
+++ b/internal/generator/templates/sync.go.tmpl
@@ -348,16 +348,35 @@ func syncResource(c interface {
break
}
- // Batch upsert all items from this page
- if err := db.UpsertBatch(resource, items); err != nil {
+ // Batch upsert all items from this page. UpsertBatch returns the
+ // count of rows actually stored — items whose primary-key field
+ // can't be extracted (e.g. APIs that return scalar IDs rather
+ // than objects) are silently skipped. Tracking stored separately
+ // from consumed lets us emit a clear sync_anomaly event when an
+ // API silently drops everything.
+ stored, err := db.UpsertBatch(resource, items)
+ if err != nil {
if !humanFriendly {
fmt.Fprintf(os.Stderr, `{"event":"sync_error","resource":"%s","error":"%s"}`+"\n", resource, strings.ReplaceAll(err.Error(), `"`, `\"`))
}
return syncResult{Resource: resource, Count: totalCount, Err: fmt.Errorf("upserting batch for %s: %w", resource, err), Duration: time.Since(started)}
}
- totalCount += len(items)
- atomic.AddInt64(&progressCount, int64(len(items)))
+ // When a non-empty page yielded zero stored rows, the API
+ // returned items in a shape we couldn't extract IDs from —
+ // likely scalar IDs (Firebase /topstories.json, GitHub user-
+ // repo lists) where the spec author should declare a hydration
+ // pattern, or an unrecognized primary-key field name.
+ if len(items) > 0 && stored == 0 {
+ if humanFriendly {
+ fmt.Fprintf(os.Stderr, "warning: %s returned %d items but stored 0 — the local store will be empty for this resource. Likely cause: scalar item shape rather than objects with extractable IDs.\n", resource, len(items))
+ } else {
+ fmt.Fprintf(os.Stderr, `{"event":"sync_anomaly","resource":"%s","consumed":%d,"stored":0,"reason":"all_items_failed_id_extraction"}`+"\n", resource, len(items))
+ }
+ }
+
+ totalCount += stored
+ atomic.AddInt64(&progressCount, int64(stored))
// Progress reporting (include rate limit info when active)
currentRate := c.RateLimit()
@@ -729,14 +748,15 @@ func syncDependentResource(c interface {
}
}
- if err := db.UpsertBatch(dep.Name, items); err != nil {
+ stored, err := db.UpsertBatch(dep.Name, items)
+ if err != nil {
if humanFriendly {
fmt.Fprintf(os.Stderr, "\n %s: upsert error for parent %s: %v\n", dep.Name, parentID, err)
}
break
}
- totalCount += len(items)
+ totalCount += stored
pagesFetched++
if maxPages > 0 && pagesFetched >= maxPages {
diff --git a/internal/pipeline/dogfood.go b/internal/pipeline/dogfood.go
index 6ca6f5ec..40e01351 100644
--- a/internal/pipeline/dogfood.go
+++ b/internal/pipeline/dogfood.go
@@ -182,6 +182,14 @@ type openAPISpec struct {
// only for internal-format specs; OpenAPI specs leave this nil and
// fall through to the generic `canonicalargs` registry.
ParamDefaults map[string]string
+ // IsInternalYAML is true when this spec was loaded from the
+ // printing-press internal YAML format rather than OpenAPI. Internal
+ // YAML expresses paths in its own shape — the OpenAPI-derived path-
+ // validity check produces noisy false positives against it (often
+ // "0/0 valid (FAIL)" while the scorecard's parallel check correctly
+ // records the dimension as unscored). Surfaced by hackernews retro
+ // #350 finding F8.
+ IsInternalYAML bool
}
func (s *openAPISpec) IsSynthetic() bool {
@@ -216,6 +224,17 @@ func RunDogfood(dir, specPath string, opts ...DogfoodOption) (*DogfoodReport, er
Skipped: true,
Detail: "synthetic spec: path validity not applicable",
}
+ } else if spec.IsInternalYAML {
+ // Internal YAML specs declare paths in their own shape. The
+ // OpenAPI-derived path-matching here produces "0/0 valid
+ // (FAIL)" against perfectly valid CLIs. The scorecard's
+ // parallel path-validity check already records the dimension
+ // as unscored for internal YAML; align dogfood with that
+ // rather than report a contradictory FAIL.
+ report.PathCheck = PathCheckResult{
+ Skipped: true,
+ Detail: "internal-yaml spec: paths validated at parse time",
+ }
} else {
report.PathCheck = checkPaths(dir, spec.Paths)
}
diff --git a/internal/pipeline/reimplementation_check.go b/internal/pipeline/reimplementation_check.go
index 19708b9b..c62efdd1 100644
--- a/internal/pipeline/reimplementation_check.go
+++ b/internal/pipeline/reimplementation_check.go
@@ -90,6 +90,27 @@ var (
// that build their own raw http.Request also land here.
clientCallRe = regexp.MustCompile(`\b(flags\.newClient\s*\(|http\.(Get|Post|NewRequest|Do)\s*\(|c\.Do\s*\(|c\.Get\s*\(|c\.Post\s*\()`)
+ // siblingInternalImportRe catches any import of a package under
+ // `internal/<name>`. Go's RE2 has no negative lookahead, so the
+ // regex captures all matches and the surrounding code filters out
+ // the generator-reserved set (see hasSiblingInternalImport).
+ //
+ // Why we care: any package alongside the generated `client`,
+ // `store`, `cliutil`, etc. is almost certainly a hand-built API
+ // client (think `internal/algolia` for a CLI that fronts both a
+ // primary and a secondary API). Calls into such packages are
+ // legitimate API access; the pre-existing regex set didn't
+ // recognize them, so dogfood was producing false-positive
+ // reimplementation findings on every multi-source CLI.
+ //
+ // False positives from this signal (a non-client utility package
+ // mistakenly recognized as a client) are strictly less bad than
+ // the false negatives we get without it (a real Algolia client
+ // flagged as reimplementation).
+ //
+ // Surfaced by hackernews retro #350 finding F4.
+ siblingInternalImportRe = regexp.MustCompile(`"[^"]*/internal/([a-z][a-z0-9_]*)"`)
+
// trivialBodyRe catches the classic empty-stub shape used when an
// agent wires a Cobra command but never implements it:
//
@@ -326,7 +347,49 @@ func callsStoreHelper(content string, helpers map[string]bool) bool {
}
func hasClientSignal(content string) bool {
- return clientImportRe.MatchString(content) || clientCallRe.MatchString(content)
+ return clientImportRe.MatchString(content) ||
+ clientCallRe.MatchString(content) ||
+ hasSiblingInternalImport(content)
+}
+
+// reservedInternalPackages is the set of internal package names the
+// generator emits unconditionally. An import of any of these is NOT a
+// signal of a hand-built API client — it just means the file uses a
+// generator-emitted helper. Anything else under `internal/<name>` is
+// presumed to be agent-authored API client code (e.g. `internal/algolia`
+// for a CLI that fronts both Firebase and Algolia).
+//
+// Surfaced by hackernews retro #350 finding F4.
+var reservedInternalPackages = map[string]bool{
+ "client": true,
+ "store": true,
+ "cliutil": true,
+ "cache": true,
+ "config": true,
+ "mcp": true,
+ "types": true,
+ "share": true,
+ "deliver": true,
+ "profile": true,
+ "feedback": true,
+ "graphql": true,
+}
+
+// hasSiblingInternalImport reports whether the file imports a non-reserved
+// `internal/<name>` package — the signal for a hand-built secondary
+// client. The regex matches all internal imports; we filter the
+// reserved set in code because Go's RE2 has no negative lookahead.
+func hasSiblingInternalImport(content string) bool {
+ matches := siblingInternalImportRe.FindAllStringSubmatch(content, -1)
+ for _, m := range matches {
+ if len(m) < 2 {
+ continue
+ }
+ if !reservedInternalPackages[m[1]] {
+ return true
+ }
+ }
+ return false
}
func lastPathSegment(path string) string {
diff --git a/internal/pipeline/sibling_client_test.go b/internal/pipeline/sibling_client_test.go
new file mode 100644
index 00000000..69ea9b37
--- /dev/null
+++ b/internal/pipeline/sibling_client_test.go
@@ -0,0 +1,182 @@
+package pipeline
+
+import (
+ "strings"
+ "testing"
+)
+
+// TestHasSiblingInternalImport_RecognizesAlgolia is the regression guard
+// for hackernews retro #350 finding F4. A novel-feature command that
+// imports a hand-built sibling internal package (e.g. internal/algolia)
+// and calls into it must not trip the reimplementation false positive.
+func TestHasSiblingInternalImport_RecognizesAlgolia(t *testing.T) {
+ t.Parallel()
+
+ content := `package cli
+
+import (
+ "hackernews-pp-cli/internal/algolia"
+ "github.com/spf13/cobra"
+)
+
+func newPulseCmd(flags *rootFlags) *cobra.Command {
+ return &cobra.Command{
+ Use: "pulse",
+ RunE: func(cmd *cobra.Command, args []string) error {
+ ac := algolia.New(flags.timeout)
+ _, err := ac.Search(args[0], algolia.SearchOpts{})
+ return err
+ },
+ }
+}
+`
+ if !hasSiblingInternalImport(content) {
+ t.Fatalf("expected hasSiblingInternalImport to recognize internal/algolia")
+ }
+ if !hasClientSignal(content) {
+ t.Fatalf("expected hasClientSignal to be true for algolia-importing command")
+ }
+}
+
+// TestHasSiblingInternalImport_IgnoresReservedPackages verifies that
+// imports of generator-emitted packages (client, store, cliutil, etc.)
+// do NOT trip the new sibling-import signal. Those have their own
+// signals (clientImportRe, storeImportRe) and shouldn't double-count.
+func TestHasSiblingInternalImport_IgnoresReservedPackages(t *testing.T) {
+ t.Parallel()
+
+ for _, pkg := range []string{
+ "client", "store", "cliutil", "cache", "config",
+ "mcp", "types", "share", "deliver", "profile", "feedback",
+ "graphql",
+ } {
+ content := `package cli
+
+import (
+ "my-cli/internal/` + pkg + `"
+ "github.com/spf13/cobra"
+)
+
+func newCmd() *cobra.Command { return nil }
+`
+ if hasSiblingInternalImport(content) {
+ t.Errorf("reserved package %q should not trip sibling-import signal", pkg)
+ }
+ }
+}
+
+// TestHasSiblingInternalImport_RecognizesArbitraryNames verifies the
+// signal fires for any non-reserved internal package name. Future CLIs
+// might use internal/omdb, internal/recipescraper, etc. — the check is
+// permissive about names because the alternative (maintaining a
+// whitelist of known client-shaped names) doesn't scale.
+func TestHasSiblingInternalImport_RecognizesArbitraryNames(t *testing.T) {
+ t.Parallel()
+
+ for _, pkg := range []string{
+ "algolia", "omdb", "recipescraper", "tmdb",
+ "mybank", "fcm", "snake_case_ok", "x9z",
+ } {
+ content := `package cli
+
+import "my-cli/internal/` + pkg + `"
+`
+ if !hasSiblingInternalImport(content) {
+ t.Errorf("non-reserved package %q should trip sibling-import signal", pkg)
+ }
+ }
+}
+
+// TestHasSiblingInternalImport_NoInternalImport verifies that files
+// with no first-party internal/<name> import return false. We don't
+// guard against third-party `<vendor>/.../internal/<x>` paths because
+// Go's compiler enforces the internal rule — those paths can only
+// appear in modules under the same module-path prefix, and never in
+// the printed CLI's command files.
+func TestHasSiblingInternalImport_NoInternalImport(t *testing.T) {
+ t.Parallel()
+
+ cases := []string{
+ `package cli`,
+ `package cli
+
+import "github.com/spf13/cobra"
+`,
+ `package cli
+
+import (
+ "strings"
+ "encoding/json"
+ "github.com/spf13/cobra"
+)
+`,
+ }
+ for i, content := range cases {
+ if hasSiblingInternalImport(content) {
+ t.Errorf("case %d should not trip sibling-import signal", i)
+ }
+ }
+}
+
+// TestHasClientSignal_PreservesExistingSignals verifies the OR-with-new-
+// regex didn't accidentally weaken the canonical client patterns.
+func TestHasClientSignal_PreservesExistingSignals(t *testing.T) {
+ t.Parallel()
+
+ cases := []struct {
+ name string
+ content string
+ }{
+ {
+ name: "flags.newClient",
+ content: `c, err := flags.newClient()`,
+ },
+ {
+ name: "internal/client import",
+ content: `import "my-cli/internal/client"`,
+ },
+ {
+ name: "http.Get",
+ content: `resp, err := http.Get("https://example.com")`,
+ },
+ {
+ name: "c.Get receiver call",
+ content: `data, err := c.Get("/path", nil)`,
+ },
+ }
+ for _, tc := range cases {
+ t.Run(tc.name, func(t *testing.T) {
+ if !hasClientSignal(tc.content) {
+ t.Errorf("hasClientSignal should still match %q after adding sibling-import detection", tc.content)
+ }
+ })
+ }
+}
+
+// TestHasSiblingInternalImport_TrivialBodyStillTripsReimplementation
+// verifies the negative case: a command file that imports nothing from
+// internal/<name> AND has a trivial body still trips reimplementation.
+// The new sibling-import signal is additive — it doesn't loosen the
+// trivial-body catch.
+func TestHasSiblingInternalImport_TrivialBodyStillTripsReimplementation(t *testing.T) {
+ t.Parallel()
+
+ content := strings.TrimSpace(`
+package cli
+
+import "github.com/spf13/cobra"
+
+func newStubCmd() *cobra.Command {
+ return &cobra.Command{
+ Use: "stub",
+ RunE: func(cmd *cobra.Command, args []string) error { return nil },
+ }
+}
+`)
+ if hasClientSignal(content) {
+ t.Fatalf("trivial-body command should NOT have a client signal")
+ }
+ if !trivialBodyRe.MatchString(content) {
+ t.Fatalf("trivial-body regex should still match the canonical empty stub")
+ }
+}
diff --git a/internal/pipeline/spec_detect.go b/internal/pipeline/spec_detect.go
index ee046027..821eaa23 100644
--- a/internal/pipeline/spec_detect.go
+++ b/internal/pipeline/spec_detect.go
@@ -38,14 +38,17 @@ func isInternalYAMLSpec(data []byte) bool {
}
// internalSpecToDogfoodSpec converts a parsed internal YAML APISpec into the
-// openAPISpec struct used by dogfood/verify.
+// openAPISpec struct used by dogfood/verify. Sets IsInternalYAML so
+// downstream checks (notably path-validity) can skip rules that only
+// make sense for OpenAPI-derived path patterns.
func internalSpecToDogfoodSpec(s *apispec.APISpec) *openAPISpec {
return &openAPISpec{
- Paths: collectInternalSpecPaths(s),
- Auth: s.Auth,
- Kind: s.Kind,
- HTTPTransport: s.EffectiveHTTPTransport(),
- ParamDefaults: collectInternalSpecParamDefaults(s),
+ Paths: collectInternalSpecPaths(s),
+ Auth: s.Auth,
+ Kind: s.Kind,
+ HTTPTransport: s.EffectiveHTTPTransport(),
+ ParamDefaults: collectInternalSpecParamDefaults(s),
+ IsInternalYAML: true,
}
}
diff --git a/internal/pipeline/synthetic_spec_test.go b/internal/pipeline/synthetic_spec_test.go
index bb91b04a..5632df5f 100644
--- a/internal/pipeline/synthetic_spec_test.go
+++ b/internal/pipeline/synthetic_spec_test.go
@@ -67,9 +67,14 @@ resources:
require.Empty(t, report.PathCheck.Invalid)
}
-// TestRESTSpec_DogfoodStillChecksPaths ensures the default (non-synthetic)
-// behavior is unchanged: specs without `kind: synthetic` still get the strict
-// path-validity check. Guards against over-broad application of the skip.
+// TestRESTSpec_DogfoodStillChecksPaths originally guarded against over-broad
+// application of the synthetic-spec path-validity skip. After hackernews retro
+// #350 finding F8, internal-yaml specs (synthetic OR plain REST) skip the
+// path-validity check by default — the OpenAPI-derived path matcher produces
+// "0/0 valid (FAIL)" against perfectly valid internal-yaml CLIs while the
+// scorecard's parallel check correctly records the dimension as unscored. This
+// test now asserts the broader skip behavior; OpenAPI specs (a separate
+// fixture flow) still run the check.
func TestRESTSpec_DogfoodStillChecksPaths(t *testing.T) {
t.Parallel()
@@ -112,9 +117,12 @@ resources:
report, err := RunDogfood(dir, specPath)
require.NoError(t, err)
- // Default rest spec should run the path check and find the /widgets path.
- require.Greater(t, report.PathCheck.Tested, 0,
- "rest spec should run the path check (Tested > 0)")
+ // Internal-yaml specs (synthetic or plain REST) skip the OpenAPI-shaped
+ // path check. Confirms the broader skip from retro #350 finding F8.
+ require.True(t, report.PathCheck.Skipped,
+ "internal-yaml rest spec should skip path-validity")
+ require.Contains(t, report.PathCheck.Detail, "internal-yaml",
+ "detail should explain the skip reason")
}
// TestSyntheticSpec_ScorecardMarksPathValidityUnscored verifies scorecard
diff --git a/skills/printing-press-retro/SKILL.md b/skills/printing-press-retro/SKILL.md
index d08b0443..00c884bb 100644
--- a/skills/printing-press-retro/SKILL.md
+++ b/skills/printing-press-retro/SKILL.md
@@ -319,14 +319,34 @@ affect this specific API and wouldn't recur.
**Step A: Cross-API stress test.** Test across API shapes (standard REST, proxy-envelope,
RPC-style) and input methods (OpenAPI, crowd-sniffed, HAR-sniffed, no spec).
-**Step B: Estimate frequency.** Every API / Most APIs / API subclass (name it) / This API only.
-
-**Step C: Assess fallback cost.** How reliably will Claude catch and fix this across every
+**Step B: Name three concrete APIs that would benefit.** List three specific APIs by name
+(e.g., "Stripe, Notion, GitHub") that would benefit from this fix beyond the one that
+surfaced it. If you can only name two — or one plus handwaving "many APIs in theory" — the
+finding is capped at **P3 with a `subclass:<name>` annotation**, or moves to Skipped.
+Concrete cross-API evidence is the burden of proof for P1/P2; "20% of catalog" without
+naming three is optimism, not evidence.
+
+**Step C: Counter-check question.** Ask explicitly: "If I implemented this fix, would it
+actively hurt any API that doesn't have this pattern?" If yes, the fix needs a guard or
+condition before being P1/P2 — not a default change. Example: turning on client-side
+`?limit=N` truncation by default would hurt APIs that need server-side pagination for
+correctness; it stays P2 only because it's gated on profiler-detected absence of a
+paginator. Without that guard the same finding is unsafe to land.
+
+**Step D: Recurrence-cost check.** Search prior retros under
+`~/printing-press/manuscripts/*/proofs/*-retro-*.md` for the same finding. If the same
+finding has been raised in 2+ prior retros without being implemented, the prior cost-
+benefit math has been "no" twice. Don't re-raise it at the same priority — either move
+to P3 with a "raised N times, still not justified" annotation, or reframe the finding
+into a smaller incremental fix that addresses part of the friction. Recurrence at the
+same priority is a triage failure, not stronger evidence.
+
+**Step E: Assess fallback cost.** How reliably will Claude catch and fix this across every
future API? A "simple" edit Claude forgets 30% of the time means 30% ship with the defect.
-**Step D: Make the tradeoff.** Default is **fix it in the Printing Press**. The burden of
+**Step F: Make the tradeoff.** Default is **fix it in the Printing Press**. The burden of
proof is on *not* fixing. Skip only when the behavior is unlikely to recur across 50
-different APIs.
+different APIs AND Step B couldn't name three concrete APIs.
When the finding applies to an API subclass, include: Condition (when to activate),
Guard (when to skip), Frequency estimate.
@@ -613,8 +633,12 @@ Printing Press repo and cannot act on the findings directly.
- Prefer automatic fixes (templates, binary) over instructional fixes (skill).
- For recurring friction, always answer "inherent or fixable?" honestly.
- Be honest about what went well. Protecting good patterns matters.
-- **Bias toward fixing.** When in doubt, fix it — scope narrowly with conditional
- logic if needed.
+- **Bias toward fixing only when the fix would help APIs you can name.**
+ When in doubt, fix it — but only when Phase 3 Step B gave you three concrete
+ cross-API examples. "20% of catalog" without named APIs is optimism, not
+ evidence. The retro is a triage tool, not a wishlist; an issue overloaded
+ with subclass findings shipped at P2 wastes maintainer attention. Scope
+ narrowly with conditional logic when a real cross-API pattern is in play.
- **Look for broader patterns.** Before skipping, consider whether this is the first
sighting of a behavior you'd encounter again.
- When a fix applies to an API subclass, include the condition AND the guard.
diff --git a/skills/printing-press/SKILL.md b/skills/printing-press/SKILL.md
index e383d4ad..ad545989 100644
--- a/skills/printing-press/SKILL.md
+++ b/skills/printing-press/SKILL.md
@@ -1210,7 +1210,7 @@ For each tool, fill in what you know from the research. Stars and command_count
5. `quickstart` is a 3–6 step flow using REAL arguments (symbols, IDs, resource names an agent can actually pass). Each step's `comment` explains *why* it runs. This replaces the generic "resource list" first-command fallback.
6. `troubleshoots` captures API-specific failure modes (rate-limit mitigation, cookie expiry, paginated quirks). Each `fix` must be actionable — a command or a concrete setting change.
7. `when_to_use` is SKILL-only narrative. 2–4 sentences describing the kinds of agent tasks this CLI is the right choice for. Not rendered in README.
-8. `recipes` are 3–5 worked examples rendered in SKILL.md. Each has a title, a real command, and a one-line explanation. Prefer recipes that exercise novel features. **At least one recipe must pair `--agent` with `--select`** — using dotted paths (e.g. `--select events.shortName,events.competitions.competitors.team.displayName`) when the response is deeply nested. APIs like ESPN, HubSpot, and Linear return tens of KB per call; without a `--select` recipe, agents burn context parsing verbose payloads. Pick a command known to return a large or deeply nested response and show the narrowing pattern.
+8. `recipes` are 3–5 worked examples rendered in SKILL.md. Each has a title, a real command, and a one-line explanation. Prefer recipes that exercise novel features. **At least one recipe must pair `--agent` with `--select`** — using dotted paths (e.g. `--select events.shortName,events.competitions.competitors.team.displayName`) when the response is deeply nested. APIs like ESPN, HubSpot, and Linear return tens of KB per call; without a `--select` recipe, agents burn context parsing verbose payloads. Pick a command known to return a large or deeply nested response and show the narrowing pattern. **Regex literals must double-escape backslashes** — write `\\b` not `\b` (and `\\t`, `\\f`, etc.) inside any `command`, `fix`, or other JSON string field. JSON parses `\b` as backspace (0x08), `\f` as form feed (0x0C), and so on, which then leak into the rendered SKILL.md as control bytes that render as nothing in most viewers. The generator's render-time scanner rejects these with a clear offset; double-escape from the start to avoid the error.
9. `trigger_phrases` are natural-language phrases a user might say that should invoke this CLI's skill. Include 3–5 domain-specific phrases (e.g. for a finance CLI: "quote AAPL", "check my portfolio", "options for TSLA") and 2 generic phrases ("use <api-name>", "run <api-name>"). Domain verbs vary — don't just template "use X" variants.
10. All `narrative` fields are optional. Omit fields you can't populate honestly rather than emit filler. The generator falls back to generic content gracefully.
11. **Avoid hardcoded counts in narrative copy when the count tracks a runtime list.** A number embedded in `headline` or `value_prop` ("across N trusted sources", "from N retailers", "queries N vendors") propagates into root.go's Short/Long, the README, the SKILL, the MCP tools description, and `which.go` — every output surface that reads the narrative. When the underlying registry grows or shrinks, the count goes stale across all of those surfaces simultaneously, and a single-line edit to add a source requires hunting down ~10 hardcoded copies. Prefer plural-without-count phrasing ("across the major sources", "from a curated set of retailers") or describe the breadth qualitatively ("dozens of vendors") rather than committing to a specific integer. If a count is load-bearing for the value prop, keep the brief's narrative count-free and have the printed-CLI's README/SKILL author write the count once into a single hand-edited paragraph after generation — accepting that it will need a manual update whenever the registry changes.
diff --git a/testdata/golden/expected/generate-golden-api/dogfood.json b/testdata/golden/expected/generate-golden-api/dogfood.json
index fac528a0..c44370b5 100644
--- a/testdata/golden/expected/generate-golden-api/dogfood.json
+++ b/testdata/golden/expected/generate-golden-api/dogfood.json
@@ -15,11 +15,8 @@
"total": 17
},
"dead_functions": {
- "dead": 1,
- "items": [
- "wrapResultsWithFreshness"
- ],
- "total": 49
+ "dead": 0,
+ "total": 48
},
"dir": "<ARTIFACT_DIR>/generate-golden-api/printing-press-golden",
"example_check": {
@@ -30,7 +27,6 @@
"with_examples": 0
},
"issues": [
- "1 dead helper functions found",
"example check skipped: could not build CLI binary: exit status 1"
],
"naming_check": {
diff --git a/testdata/golden/expected/generate-golden-api/printing-press-golden/internal/cli/helpers.go b/testdata/golden/expected/generate-golden-api/printing-press-golden/internal/cli/helpers.go
index 04d5620b..3db13fff 100644
--- a/testdata/golden/expected/generate-golden-api/printing-press-golden/internal/cli/helpers.go
+++ b/testdata/golden/expected/generate-golden-api/printing-press-golden/internal/cli/helpers.go
@@ -1147,18 +1147,6 @@ func wrapWithProvenance(data json.RawMessage, prov DataProvenance) (json.RawMess
return json.Marshal(envelope)
}
-// wrapResultsWithFreshness gives hand-authored commands a small opt-in helper
-// for the generated provenance envelope without forcing arbitrary custom JSON
-// outputs to change shape.
-func wrapResultsWithFreshness(data json.RawMessage, flags *rootFlags) (json.RawMessage, error) {
- prov := DataProvenance{}
- if flags != nil {
- prov.Source = flags.dataSource
- prov.Freshness = flags.freshnessMeta
- }
- return wrapWithProvenance(data, prov)
-}
-
// defaultDBPath returns the canonical path for the local SQLite database.
func defaultDBPath(name string) string {
home, _ := os.UserHomeDir()
diff --git a/testdata/golden/expected/generate-golden-api/scorecard.json b/testdata/golden/expected/generate-golden-api/scorecard.json
index 2a63d525..a2e3ad8c 100644
--- a/testdata/golden/expected/generate-golden-api/scorecard.json
+++ b/testdata/golden/expected/generate-golden-api/scorecard.json
@@ -15,7 +15,7 @@
"breadth": 4,
"cache_freshness": 5,
"data_pipeline_integrity": 7,
- "dead_code": 4,
+ "dead_code": 5,
"doctor": 10,
"error_handling": 10,
"insight": 8,
@@ -28,11 +28,11 @@
"mcp_tool_design": 0,
"output_modes": 10,
"path_validity": 0,
- "percentage": 81,
+ "percentage": 82,
"readme": 8,
"sync_correctness": 10,
"terminal_ux": 8,
- "total": 81,
+ "total": 82,
"type_fidelity": 3,
"vision": 7,
"workflows": 10
← 47fe3393 fix(publish): gate packaging on skill verification (#348)
·
back to Cli Printing Press
·
test(cli): simplify shipcheck test setup (#354) 4a6e5c7f →