← back to Cli Printing Press
fix(cli): printing-press P1 machine fixes (issue #333) (#335)
6b2be74d017b862ceab688d1061aa39ab59ff6c8 · 2026-04-27 01:36:29 -0700 · Trevin Chow
* fix(cli): doctor honors http_transport via flags.newClient
The doctor reachability and credentials probes previously used a fresh
stdlib http.Client, silently bypassing the spec's http_transport directive.
Against any Cloudflare/Akamai/Vercel-fronted CLI (allrecipes, recipe-goat,
future Dotdash properties), this reported "API unreachable" even when the
real commands worked correctly through Surf.
Now the doctor calls flags.newClient() and uses c.Get/c.GetWithHeaders, so
it shares the same TLS-impersonated transport every other command uses.
When the configured transport reaches but is then blocked by an interstitial
(rare but possible), looksLikeDoctorInterstitial inspects the body and names
the protection vendor (Cloudflare, Akamai, Vercel, AWS WAF, DataDome,
PerimeterX) so the diagnostic is actionable.
This reinforces -- not weakens -- Surf usage. There is no stdlib fallback;
errors propagate honestly. The CLI's transport choice is honored end-to-end.
Tests updated to match the new code shape (uses GetWithHeaders, always
declares verifyPath). Golden harness still passes.
Refs #333 (WU-1)
* fix(cli): skip auth subcommand and registration for auth.type:none
Specs declaring auth.type:none (public-data CLIs: weather, news feeds,
public score feeds, recipe sites, ESPN unauth endpoints) were shipping a
fully-wired `auth set-token / status / logout` subcommand that did
nothing useful. Agents calling `<cli> auth set-token` got a no-op; the
scorecard's auth dimension docked the CLI for "missing auth subcommand"
even though the spec said no auth was required.
Now Generator.renderAuthFiles early-returns when auth.type=="none" AND
no AuthorizationURL AND no graphql_persisted_query traffic-analysis
hint. root.go's newAuthCmd registration is gated by HasAuthCommand,
threaded through the rootData struct from the same condition.
scoreAuth detects no-auth CLIs by the absence of auth.go (post-fix, the
only path that produces no auth.go is the no-auth path) and returns 10/10
instead of penalizing the missing subcommand. Existing exceptions for
GraphQL persisted-query browser refresh and OAuth AuthorizationURL flows
are preserved -- those still emit auth.go and run the regular scoring.
Test updated: a no-auth spec with browser-chrome transport correctly has
no auth.go (stronger assertion than the previous "no refresh stub in
auth.go").
Refs #333 (WU-2)
* fix(cli): html-extract suppresses noscript subtrees + Image field
Sites like Allrecipes, Food Network, BBC Good Food, and other
Dotdash/Meredith properties wrap above-the-fold images in <noscript>
fallbacks for non-JS users. The HTML5 spec parses <noscript> content as
raw text (CDATA-like content model), so a normal walk of the anchor's
descendants leaks literal "<img src=...>" markup into the result fields.
Every HTML-scrape CLI ended up shipping a custom search-card parser to
work around this.
Now nodeTextSuppressing skips subtrees rooted at noscript / script /
style / template -- the four HTML5 elements that the parser preserves
as raw text. extractHTMLLink uses this walker for the link's name and
text fields.
htmlLink also gains a new Image field, populated by firstImageSrc which
applies the same suppression. When an anchor has both a noscript fallback
and a rendered <img>, the rendered image wins -- correct because that's
what JS-enabled users (the typical scraping target) see. Anchors with
no <img> at all leave Image empty (omitempty in the JSON tag).
Test: TestGenerateHTMLExtractionEndpoint extended with a noscript-wrapped
fallback img alongside a rendered img. Asserts:
- links[0]["name"] does not contain "<img" (suppression works)
- links[0]["image"] is the rendered URL, not the noscript fallback
- links[1]["image"] (rendered img only) populates correctly
Refs #333 (WU-3, refined per validation comment)
* fix(cli): emit dryRunOK helper + verify-friendly RunE pattern in SKILL
Hand-written transcendence commands consistently failed verify's --dry-run
probe because authors reached for cobra's `Args: cobra.MinimumNArgs(N)`
or `MarkFlagRequired(...)`. Cobra evaluates both before RunE runs, so a
dry-run guard inside RunE could not reach if those gates fired. The
polish-worker then rewrote the same 3-line pattern on every CLI -- a
wasted iteration cycle because nothing in the generator or skill told the
implementing agent the right shape.
Now the generator emits a small `dryRunOK(flags *rootFlags) bool` helper
into helpers.go. SKILL.md Phase 3's "Agent Build Checklist" gains an 8th
principle and a "Verify-friendly RunE template" subsection showing the
canonical shape:
RunE: func(cmd *cobra.Command, args []string) error {
if len(args) == 0 {
return cmd.Help()
}
if dryRunOK(flags) {
return nil
}
// ... real work ...
}
Test TestGeneratedOutput_NoMarkFlagRequired tightened to grep for the
call-site pattern (`.MarkFlagRequired("`) instead of any mention -- so
docstrings and prose discussing the symbol don't false-positive. Golden
helpers.go and dogfood.json updated to reflect the new function (one
new helper definition; dogfood's known-functions count goes from 48 to
49).
Refs #333 (WU-4)
* docs(cli): plan for printing-press P1 machine fixes (issue #333)
Adds the implementation plan covering U1-U4 (doctor transport, no-auth
generation, html noscript suppression, dryRunOK helper). Origin: retro
issue #333 with validation comment refining F4 (noscript-specific) and
F8 (regex flag-type expansion, deferred to a separate plan).
* fix(cli): address PR #335 review feedback
Resolves all 8 issues raised in self-review of #335:
looksLikeDoctorInterstitial now anchors loose markers to <title> context
so a recipe titled "Just A Moment of Pause Cookies" no longer triggers
the Cloudflare verdict. Bails early on bodies without any <title>. The
DataDome marker now also requires a context word (blocked/captcha/
challenge) alongside the vendor name, since "datadome" alone appears in
legitimate analytics scripts on sites that don't use it for blocking.
Changed source priority: data-src > data-srcset/srcset > src. Modern
lazy-loaded images put a 1x1 placeholder in src and the real URL in
data-src; the previous src-first order surfaced the placeholder for
every Pinterest/NYT Cooking-pattern site. Added firstSrcsetURL to extract
the first URL from a comma-separated srcset/data-srcset value.
Doc comment now explicitly notes that the function returns the first
matching <img> in DOM order with no quality heuristic, and acknowledges
the icon-first edge case for callers building specs against
badge-heavy layouts.
Added a comment explaining that readFileContent returns "" for both
"file does not exist" and "file is empty"; the empty-content check
works because the generator never emits an empty auth.go. Documented
the safer os.Stat alternative as a future option if a placeholder code
path emerges.
Extracted Generator.shouldEmitAuth() so renderAuthFiles and the
HasAuthCommand template-data computation share one source of truth.
Eliminates drift risk if the predicate evolves.
Moved reachStatus declaration into the errors.As arm where it's
assigned and read; outer scope no longer carries an unused variable.
Replaced remaining "--" sequences in user-facing doctor strings with
em-dashes (—) so all rendered messages match the existing template
style. Internal Go comments unchanged.
- TestGeneratedDoctor_InterstitialMarkersAreTitleAnchored asserts the
<title>-anchored Cloudflare marker, the early-bail-without-title
guard, and the DataDome context-word requirement landed in the
rendered template.
- TestGenerateHTMLExtractionEndpoint extended: the makers fixture now
includes a placeholder-src + data-src anchor (data-src priority) and
a srcset-only anchor (srcset fallback). Asserts data-src wins over a
placeholder and the first srcset URL is selected when src is absent.
Tests: full suite passes, go vet clean, golangci-lint 0 issues, golden
verify passes (no fixture changes needed for these fixes).
Refs #333, addresses review on #335.
Files touched
A docs/plans/2026-04-26-002-feat-printing-press-p1-machine-fixes-plan.mdM internal/generator/generator.goM internal/generator/generator_test.goM internal/generator/templates/doctor.go.tmplM internal/generator/templates/helpers.go.tmplM internal/generator/templates/html_extract.go.tmplM internal/generator/templates/root.go.tmplM internal/pipeline/scorecard.goM 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.go
Diff
commit 6b2be74d017b862ceab688d1061aa39ab59ff6c8
Author: Trevin Chow <trevin@trevinchow.com>
Date: Mon Apr 27 01:36:29 2026 -0700
fix(cli): printing-press P1 machine fixes (issue #333) (#335)
* fix(cli): doctor honors http_transport via flags.newClient
The doctor reachability and credentials probes previously used a fresh
stdlib http.Client, silently bypassing the spec's http_transport directive.
Against any Cloudflare/Akamai/Vercel-fronted CLI (allrecipes, recipe-goat,
future Dotdash properties), this reported "API unreachable" even when the
real commands worked correctly through Surf.
Now the doctor calls flags.newClient() and uses c.Get/c.GetWithHeaders, so
it shares the same TLS-impersonated transport every other command uses.
When the configured transport reaches but is then blocked by an interstitial
(rare but possible), looksLikeDoctorInterstitial inspects the body and names
the protection vendor (Cloudflare, Akamai, Vercel, AWS WAF, DataDome,
PerimeterX) so the diagnostic is actionable.
This reinforces -- not weakens -- Surf usage. There is no stdlib fallback;
errors propagate honestly. The CLI's transport choice is honored end-to-end.
Tests updated to match the new code shape (uses GetWithHeaders, always
declares verifyPath). Golden harness still passes.
Refs #333 (WU-1)
* fix(cli): skip auth subcommand and registration for auth.type:none
Specs declaring auth.type:none (public-data CLIs: weather, news feeds,
public score feeds, recipe sites, ESPN unauth endpoints) were shipping a
fully-wired `auth set-token / status / logout` subcommand that did
nothing useful. Agents calling `<cli> auth set-token` got a no-op; the
scorecard's auth dimension docked the CLI for "missing auth subcommand"
even though the spec said no auth was required.
Now Generator.renderAuthFiles early-returns when auth.type=="none" AND
no AuthorizationURL AND no graphql_persisted_query traffic-analysis
hint. root.go's newAuthCmd registration is gated by HasAuthCommand,
threaded through the rootData struct from the same condition.
scoreAuth detects no-auth CLIs by the absence of auth.go (post-fix, the
only path that produces no auth.go is the no-auth path) and returns 10/10
instead of penalizing the missing subcommand. Existing exceptions for
GraphQL persisted-query browser refresh and OAuth AuthorizationURL flows
are preserved -- those still emit auth.go and run the regular scoring.
Test updated: a no-auth spec with browser-chrome transport correctly has
no auth.go (stronger assertion than the previous "no refresh stub in
auth.go").
Refs #333 (WU-2)
* fix(cli): html-extract suppresses noscript subtrees + Image field
Sites like Allrecipes, Food Network, BBC Good Food, and other
Dotdash/Meredith properties wrap above-the-fold images in <noscript>
fallbacks for non-JS users. The HTML5 spec parses <noscript> content as
raw text (CDATA-like content model), so a normal walk of the anchor's
descendants leaks literal "<img src=...>" markup into the result fields.
Every HTML-scrape CLI ended up shipping a custom search-card parser to
work around this.
Now nodeTextSuppressing skips subtrees rooted at noscript / script /
style / template -- the four HTML5 elements that the parser preserves
as raw text. extractHTMLLink uses this walker for the link's name and
text fields.
htmlLink also gains a new Image field, populated by firstImageSrc which
applies the same suppression. When an anchor has both a noscript fallback
and a rendered <img>, the rendered image wins -- correct because that's
what JS-enabled users (the typical scraping target) see. Anchors with
no <img> at all leave Image empty (omitempty in the JSON tag).
Test: TestGenerateHTMLExtractionEndpoint extended with a noscript-wrapped
fallback img alongside a rendered img. Asserts:
- links[0]["name"] does not contain "<img" (suppression works)
- links[0]["image"] is the rendered URL, not the noscript fallback
- links[1]["image"] (rendered img only) populates correctly
Refs #333 (WU-3, refined per validation comment)
* fix(cli): emit dryRunOK helper + verify-friendly RunE pattern in SKILL
Hand-written transcendence commands consistently failed verify's --dry-run
probe because authors reached for cobra's `Args: cobra.MinimumNArgs(N)`
or `MarkFlagRequired(...)`. Cobra evaluates both before RunE runs, so a
dry-run guard inside RunE could not reach if those gates fired. The
polish-worker then rewrote the same 3-line pattern on every CLI -- a
wasted iteration cycle because nothing in the generator or skill told the
implementing agent the right shape.
Now the generator emits a small `dryRunOK(flags *rootFlags) bool` helper
into helpers.go. SKILL.md Phase 3's "Agent Build Checklist" gains an 8th
principle and a "Verify-friendly RunE template" subsection showing the
canonical shape:
RunE: func(cmd *cobra.Command, args []string) error {
if len(args) == 0 {
return cmd.Help()
}
if dryRunOK(flags) {
return nil
}
// ... real work ...
}
Test TestGeneratedOutput_NoMarkFlagRequired tightened to grep for the
call-site pattern (`.MarkFlagRequired("`) instead of any mention -- so
docstrings and prose discussing the symbol don't false-positive. Golden
helpers.go and dogfood.json updated to reflect the new function (one
new helper definition; dogfood's known-functions count goes from 48 to
49).
Refs #333 (WU-4)
* docs(cli): plan for printing-press P1 machine fixes (issue #333)
Adds the implementation plan covering U1-U4 (doctor transport, no-auth
generation, html noscript suppression, dryRunOK helper). Origin: retro
issue #333 with validation comment refining F4 (noscript-specific) and
F8 (regex flag-type expansion, deferred to a separate plan).
* fix(cli): address PR #335 review feedback
Resolves all 8 issues raised in self-review of #335:
looksLikeDoctorInterstitial now anchors loose markers to <title> context
so a recipe titled "Just A Moment of Pause Cookies" no longer triggers
the Cloudflare verdict. Bails early on bodies without any <title>. The
DataDome marker now also requires a context word (blocked/captcha/
challenge) alongside the vendor name, since "datadome" alone appears in
legitimate analytics scripts on sites that don't use it for blocking.
Changed source priority: data-src > data-srcset/srcset > src. Modern
lazy-loaded images put a 1x1 placeholder in src and the real URL in
data-src; the previous src-first order surfaced the placeholder for
every Pinterest/NYT Cooking-pattern site. Added firstSrcsetURL to extract
the first URL from a comma-separated srcset/data-srcset value.
Doc comment now explicitly notes that the function returns the first
matching <img> in DOM order with no quality heuristic, and acknowledges
the icon-first edge case for callers building specs against
badge-heavy layouts.
Added a comment explaining that readFileContent returns "" for both
"file does not exist" and "file is empty"; the empty-content check
works because the generator never emits an empty auth.go. Documented
the safer os.Stat alternative as a future option if a placeholder code
path emerges.
Extracted Generator.shouldEmitAuth() so renderAuthFiles and the
HasAuthCommand template-data computation share one source of truth.
Eliminates drift risk if the predicate evolves.
Moved reachStatus declaration into the errors.As arm where it's
assigned and read; outer scope no longer carries an unused variable.
Replaced remaining "--" sequences in user-facing doctor strings with
em-dashes (—) so all rendered messages match the existing template
style. Internal Go comments unchanged.
- TestGeneratedDoctor_InterstitialMarkersAreTitleAnchored asserts the
<title>-anchored Cloudflare marker, the early-bail-without-title
guard, and the DataDome context-word requirement landed in the
rendered template.
- TestGenerateHTMLExtractionEndpoint extended: the makers fixture now
includes a placeholder-src + data-src anchor (data-src priority) and
a srcset-only anchor (srcset fallback). Asserts data-src wins over a
placeholder and the first srcset URL is selected when src is absent.
Tests: full suite passes, go vet clean, golangci-lint 0 issues, golden
verify passes (no fixture changes needed for these fixes).
Refs #333, addresses review on #335.
---
...02-feat-printing-press-p1-machine-fixes-plan.md | 481 +++++++++++++++++++++
internal/generator/generator.go | 32 +-
internal/generator/generator_test.go | 126 +++++-
internal/generator/templates/doctor.go.tmpl | 209 +++++----
internal/generator/templates/helpers.go.tmpl | 22 +
internal/generator/templates/html_extract.go.tmpl | 172 +++++++-
internal/generator/templates/root.go.tmpl | 2 +
internal/pipeline/scorecard.go | 27 +-
skills/printing-press/SKILL.md | 21 +-
.../expected/generate-golden-api/dogfood.json | 2 +-
.../printing-press-golden/internal/cli/helpers.go | 22 +
11 files changed, 1009 insertions(+), 107 deletions(-)
diff --git a/docs/plans/2026-04-26-002-feat-printing-press-p1-machine-fixes-plan.md b/docs/plans/2026-04-26-002-feat-printing-press-p1-machine-fixes-plan.md
new file mode 100644
index 00000000..ff437a3b
--- /dev/null
+++ b/docs/plans/2026-04-26-002-feat-printing-press-p1-machine-fixes-plan.md
@@ -0,0 +1,481 @@
+---
+title: "feat: Printing Press P1 machine fixes (doctor transport, no-auth gen, html noscript, dryRun helper)"
+type: feat
+status: active
+date: 2026-04-26
+origin: https://github.com/mvanhorn/cli-printing-press/issues/333
+---
+
+# feat: Printing Press P1 machine fixes (doctor transport, no-auth gen, html noscript, dryRun helper)
+
+## Overview
+
+Four independent P1 work units from the allrecipes retro (issue #333). Each fixes a generator template or scorer that produced extra hand-edits during the allrecipes generation:
+
+- **U1** — generated `doctor` reachability probe currently uses stdlib `http.Client`, ignoring `http_transport` and silently failing on Cloudflare-fronted CLIs. Move it onto the same Surf-based client every other command uses.
+- **U2** — generator emits `internal/cli/auth.go` even when the spec declares `auth.type: none`, leaving a dead `auth set-token / status / logout` UI. Skip emission and registration; exempt the scorer's auth dimension.
+- **U3** — `extractHTMLLink`'s `nodeText` includes raw `<img>` markup from `<noscript>` subtrees because HTML parsers preserve raw-text-mode tag content as TextNodes. Skip `noscript`/`script`/`style`/`template` subtrees and surface the first nested image URL on a new field.
+- **U4** — hand-written transcendence commands need a `--dry-run` short-circuit + must avoid `Args: cobra.MinimumNArgs(N)` and `MarkFlagRequired(...)` for verify compatibility. Emit a `dryRunOK` helper and document the pattern in the SKILL.
+
+Reinforces (not weakens) the Surf TLS-impersonated transport everywhere it's already used.
+
+---
+
+## Problem Frame
+
+Generated CLIs fronted by Cloudflare/Akamai/Vercel-WAF (e.g., `allrecipes`, `recipe-goat`, future Dotdash sites) ship with friction the Printing Press could eliminate at generation time:
+
+1. The `doctor` command lies about connectivity because its reachability probe uses stdlib HTTP, ignoring the `http_transport: browser-chrome` directive that every other command honors. Users running `doctor` against a working CLI see "API unreachable" — a false negative that erodes trust in the doctor signal.
+
+2. Specs declaring `auth.type: none` (public-data APIs, recipe sites, public score feeds) ship with a fully-wired `auth set-token / status / logout` subcommand that does nothing useful. Worse, the scorer docks the auth dimension because the auth subcommand "isn't doing the right thing" — penalizing CLIs that correctly reflect their no-auth spec.
+
+3. The HTML-extract `mode: links` parser silently leaks raw `<img>` markup into result fields whenever sites wrap above-the-fold images in `<noscript>` (Allrecipes, Food Network, NYT, BBC Good Food — every Dotdash/Meredith property does this). The defect is genuine: `nodeText` walks all descendants, including `<noscript>` subtrees, which the html parser preserves as raw TextNodes per spec. Every HTML-scrape CLI ships with a custom search-card parser to work around this.
+
+4. Hand-written transcendence commands (the work that lands in Phase 3 of every printing-press generation) consistently fail verify's `--dry-run` probe because authors use `Args: cobra.MinimumNArgs(N)` or `MarkFlagRequired(...)`. Cobra evaluates both before RunE runs, so a `--dry-run` guard in the command body cannot reach. The polish-worker rewrites the same 3-line pattern on every CLI; the cycle is wasted because the underlying generator/skill never tells the implementing agent the right pattern.
+
+All four are repeating across the catalog. Each fix is small, well-bounded, and independent.
+
+---
+
+## Requirements Trace
+
+- R1. Generated `doctor` reachability probe uses the same HTTP client as the rest of the CLI (currently `flags.newClient()` returns a Surf-wrapped `*client.Client`). Doctor accurately reports reachability for any spec value of `http_transport`.
+- R2. Doctor distinguishes "transport is reaching but blocked by interstitial" from "transport failed entirely" — the diagnostic names the protection vendor (Cloudflare/Akamai/Vercel/AWS WAF) so the user has actionable context.
+- R3. Generator does not emit `internal/cli/auth.go` when the spec's `auth.type` is `"none"` AND no traffic-analysis browser hint applies AND no GraphQL persisted-query state exists.
+- R4. Generator does not register `newAuthCmd(&flags)` in `root.go` when auth.go was not emitted.
+- R5. Scorecard's auth dimension exempts no-auth specs from the "no auth subcommand" deduction; scoring reflects "this CLI correctly has no auth," not "this CLI is missing auth."
+- R6. HTML-extract link mode skips `<noscript>`, `<script>`, `<style>`, `<template>` subtrees when computing anchor text. Result fields contain only rendered text.
+- R7. HTML-extract `htmlLink` struct surfaces the first nested `<img src=...>` URL on a separate field (e.g., `Image`).
+- R8. Generated CLIs include a `dryRunOK(cmd, flags) bool` helper in the cmd helpers file (named so collisions with hand-written code are unlikely) that returns true when `flags.dryRun` is set, signaling the caller to short-circuit.
+- R9. The Printing Press SKILL Phase 3 build instructions explicitly document the verify-friendly RunE pattern: no `Args: cobra.MinimumNArgs(N)`, no `MarkFlagRequired(...)`; check inside RunE; fall through to `cmd.Help()` for help-only invocations; check `dryRunOK` before any IO.
+- R10. Existing CLIs (notion, github, stripe, recipe-goat, etc.) regenerate without behavioral regressions. The four template/scorer/skill changes maintain backward compatibility for specs that don't trigger the new guards.
+
+---
+
+## Scope Boundaries
+
+- Not refactoring the doctor command's overall structure or output schema. Only the HTTP probe and the credential validation client are swapped.
+- Not adding new auth flows or scorecard dimensions. Only making existing auth code paths conditional and exempting the auth dimension when no auth applies.
+- Not redesigning the HTML extraction pipeline. Only adding raw-text-mode tag suppression and one new field on `htmlLink`.
+- Not auto-rewriting hand-written commands across already-shipped CLIs. WU-4 emits a helper for new generations and documents the pattern for the agent; existing CLIs are unaffected unless they regenerate.
+- Not changing how Surf or `http_transport` is selected, configured, or passed through. The transport story is unchanged; doctor just uses it.
+
+### Deferred to Follow-Up Work
+
+- WU-5 (Insight scorer content detection), WU-6 (TypeFidelity regex flag-type expansion), WU-7 (data-pipeline scorer content detection), WU-8 (move provenance helpers to subpackage) — separate plan, not part of this set. P2 priority.
+
+---
+
+## Context & Research
+
+### Relevant Code and Patterns
+
+**WU-1 (doctor)**
+- `internal/generator/templates/doctor.go.tmpl` — current reachability probe at lines 19 (`net/http` import), 152-157 (`httpClient := &http.Client{}` + HEAD probe), 167 (GET on health endpoints), 220 (auth credentials probe). All four call sites use the stdlib client.
+- `internal/generator/templates/client.go.tmpl` — defines the client.Client wrapper and its constructor; this is the consumer side of `flags.newClient()` (the function lives in `root.go.tmpl`).
+- `internal/cli/root.go.tmpl` — `flags.newClient()` factory. Returns a `*client.Client` configured with the spec's transport (Surf when `http_transport` is `browser-chrome` or `browser-chrome-h3`).
+- The client.Client exposes `Get(path, params)` and `GetWithHeaders(path, params, headers)` — both go through Surf when configured.
+- Reference: I already executed this fix manually for the allrecipes CLI (see `~/printing-press/library/allrecipes/internal/cli/doctor.go`). The hand-edit replaced the stdlib block with `c, err := flags.newClient()` + `c.Get("/search", ...)`. The same pattern generalizes: pick a known-public path from the spec or use `/` as a fallback probe.
+- Cloudflare / Akamai / Vercel / AWS WAF interstitial detection: existing helper `looksLikeHTMLChallenge` in `internal/generator/templates/html_extract.go.tmpl:280` already lists the relevant body markers. Reuse that detector or factor it into a shared helper.
+
+**WU-2 (auth-none)**
+- `internal/generator/generator.go:1244` (`renderAuthFiles()`) — current code unconditionally renders one of three auth templates. The "always render" comment on line 1241 ("`Always render auth command - use full OAuth2 template...`") is the load-bearing assumption to break. The function already has an exception for `auth_browser.go.tmpl` when traffic-analysis hints `graphql_persisted_query`; the new exit path adds an early-return for `auth.type: none` AND no such hint.
+- `internal/generator/templates/root.go.tmpl:200` — `rootCmd.AddCommand(newAuthCmd(&flags))`. Wrap in `{{if .Auth.IsAuthenticated}}` (or equivalent already-exposed predicate).
+- `internal/spec/spec.go:Auth` struct — already has `Type` field. Existing helpers like `g.Spec.Auth.Type == "none"` are the right guard.
+- `internal/pipeline/scorecard.go:scoreAuth` (lines 295-336) — currently penalizes when `authContent` is empty (line 304: `if authContent != "" { score += 1 }` — missing auth = 0 instead of +1). Also penalizes when `os.Getenv` count is zero in config.go. The fix: when the parsed spec declares `auth.type: none`, skip the per-signal scoring and return the full 10/10 (or auto-award the "free grant" + give credit for the secure-permissions and config-load signals that DO apply to no-auth CLIs).
+- The function reads four files but never reads the spec. The cleanest fix takes the spec (or its parsed auth struct) as an additional argument so the no-auth exemption can fire deterministically.
+- Looking at the call site at `scorecard.go:114` (`sc.Steinberger.Auth = scoreAuth(outputDir)`), the surrounding context (the broader `Score` function) already has access to the spec — so we can plumb it through.
+
+**WU-3 (html-extract noscript)**
+- `internal/generator/templates/html_extract.go.tmpl:246-255` — `nodeText(n)` walks all descendants and concatenates TextNode `Data`. The defect is that html parsers (golang.org/x/net/html) preserve `<noscript>`, `<script>`, `<style>`, `<template>` content as raw TextNode children of those parents (per HTML spec — these elements have CDATA-like content models). Skip these children before the recursion descends.
+- `walkHTML` at line 217 is the recursion primitive. Either fix `walkHTML` to honor a "skip children of these tags" rule, or add a `nodeTextSkipping` variant that's local to `extractHTMLLink`.
+- `htmlLink` struct at lines 33-39 — current fields: `Rank`, `Name`, `Text`, `URL`, `Slug`. Add `Image string `json:"image,omitempty"`` and populate it from the first descendant `<img>` element's `src` attribute (skipping `<noscript><img>` because that subtree is being suppressed).
+- `attrValue(n, "src")` already exists at line 240 and is the reuse path.
+
+**WU-4 (dryRunOK helper + SKILL)**
+- `internal/generator/templates/helpers.go.tmpl` — current shared helpers file emitted into `internal/cli/helpers.go`. Add a small `dryRunOK(cmd *cobra.Command, flags *rootFlags) bool` returning `flags.dryRun` (cheap; named so the call site is self-documenting).
+- The doctor template (post-WU-1) and the generated `*_search.go` / `*_get.go` already short-circuit on `flags.dryRun` — keep their behavior; the new helper standardizes the idiom for hand-written commands.
+- `skills/printing-press/SKILL.md` — Phase 3 starts at line 1522 ("Phase 3: Build The GOAT"). The "Agent Build Checklist (per command)" section at line 1575 lists 7 principles; add an 8th: "**Verify-friendly RunE:** no `Args: cobra.MinimumNArgs(N)`, no `MarkFlagRequired(...)`; first line of RunE checks `len(args) == 0` → `cmd.Help()`, second checks `dryRunOK(cmd, flags)` → `nil`."
+- Reference for the template structure: `cmd_helpers.go` I emitted by hand for allrecipes (`~/printing-press/library/allrecipes/internal/cli/cmd_helpers.go`). The existing `helpers.go.tmpl` is a cleaner host than introducing a new `cmd_helpers.go.tmpl` file; keep the surface compact.
+
+### Institutional Learnings
+
+- `docs/retros/2026-04-13-recipe-goat-retro.md` (already shipped) flagged a related symptom: hand-written commands repeatedly fail verify because they require positional args. WU-4 closes that gap.
+- `docs/plans/2026-04-13-002-feat-cloudflare-cli-learnings-plan.md` — established the "browser-chrome transport for Dotdash/Cloudflare-fronted sites" pattern. WU-1's fix lets the doctor honor that decision instead of silently bypassing it.
+- `docs/plans/2026-04-19-004-feat-auth-doctor-plan.md` — adjacent work on the printing-press binary's `auth doctor` subcommand, not the CLI's own `doctor`. Different scope; mention here so a reviewer doesn't conflate them.
+
+### External References
+
+None required. All fixes are repo-internal generator/scorer/skill changes with established patterns.
+
+---
+
+## Key Technical Decisions
+
+- **Doctor reuses `flags.newClient()` rather than constructing a parallel Surf client.** Reuses existing transport configuration, rate limiter, cache, and request decoration. The CLI is the source of truth for "how does this CLI talk to the API"; doctor must match.
+- **Cloudflare interstitial detection is post-fetch body inspection, not transport-layer.** When Surf reaches the wall (rare but possible on flagged IPs), the response is still a 200 with the JS challenge HTML. Detect by body markers, not by status code. The `looksLikeHTMLChallenge` helper already exists in `html_extract.go.tmpl` and lists the right markers; promote it to a shared `internal/cliutil/` helper or duplicate-and-pin in the doctor template.
+- **No-auth exemption is plumbed via spec to the scorer, not detected by file absence.** The scorer currently asks "does `auth.go` exist? does config.go have `os.Getenv`?" These are file-presence proxies. The principled fix is to ask the spec: `if spec.Auth.Type == "none" → exempt`. Plumbing requires `scoreAuth` to take the spec (or auth struct) as an arg.
+- **HTML extract suppression list is fixed, not configurable.** `noscript / script / style / template` are the four HTML5 elements that browsers parse as raw text. There's no domain-specific tuning here — every site that uses noscript image fallbacks benefits, none are penalized. Don't expose as spec config.
+- **`htmlLink.Image` is populated with the first non-suppressed `<img src>`.** If the visible image is rendered at top level (no noscript wrap), it's captured. If it's only inside `<noscript>` (the suppressed subtree), `Image` stays empty — that's correct, the spec author can declare a custom html_extract path for the alt-text or fallback if they want it.
+- **`dryRunOK` is a function, not a struct method.** Lives alongside the existing helpers in `helpers.go.tmpl`. Hand-written commands import nothing extra; they just call `dryRunOK(cmd, flags)` as the second line of RunE.
+- **SKILL Phase 3 update is additive, not restructuring.** Add one bullet to the existing 7-principle checklist + a small "Verify-friendly RunE template" snippet. Don't reorganize.
+
+---
+
+## Open Questions
+
+### Resolved During Planning
+
+- *Should doctor probe a known-public path from the spec or always probe `/`?* — Use `/` as the default probe (works for any base URL). For specs that declare a `verify_path` for credential validation, reuse that path for credential probes only. Reachability stays on `/`.
+- *Should the no-auth exemption skip ALL signals or only the missing-file penalty?* — Skip the entire dimension scoring path; return 10/10 with a `note: "auth.type:none — exempted from auth scoring"` field in the JSON output. Mixed-signal scoring tempts future maintainers to add new auth signals that punish no-auth CLIs again.
+- *Can the htmlLink Image field be empty in valid output?* — Yes. Many anchor-link cards have no nested `<img>` (text-only listings). Empty is a valid signal, not an error.
+- *Does the SKILL change need any backward-compat treatment for in-flight Phase 3 work?* — No. The bullet adds a recommendation; existing implementations that don't follow it still work, they just need polish-worker to fix dry-run plumbing. WU-4 reduces polish-worker churn for new generations.
+
+### Deferred to Implementation
+
+- *Exact placement of `looksLikeHTMLChallenge` (shared cliutil vs. doctor-template-local).* — Defer; either works. The shared cliutil placement is cleaner architecturally but introduces a cross-package dependency; the local placement is duplicative but self-contained. Implementer picks based on whether the existing `internal/cliutil/` package is already imported by the doctor's surrounding code.
+- *Whether to apply the `dryRunOK` helper to the generated `*_search.go` / `*_get.go` handlers.* — Defer. Those handlers already short-circuit on `flags.dryRun`; converting them to use the helper is consistency churn that should land separately if at all.
+- *Whether `scoreAuth`'s "free grant" of +2 (currently a TODO) should be removed in this same change.* — Defer. The free grant is unrelated to no-auth handling; removing it is a separate scoring rebalancing that warrants its own plan.
+
+---
+
+## High-Level Technical Design
+
+> *This illustrates the intended approach and is directional guidance for review, not implementation specification. The implementing agent should treat it as context, not code to reproduce.*
+
+The four units modify three files in the templates and one in the scorer plus one skill markdown. Their relationships:
+
+```
+Generator templates Scorer
+───────────────────── ──────
+doctor.go.tmpl ────────────────U1 scorecard.go ──U2
+ uses flags.newClient (post-U1) scoreAuth(spec) (post-U2)
+ gates on spec.Auth.Type=="none"
+
+root.go.tmpl ────────────────U2
+ conditional newAuthCmd register
+ (gated by IsAuthenticated)
+
+helpers.go.tmpl ────────────────U4
+ emits dryRunOK helper
+
+html_extract.go.tmpl ─────────────U3
+ walkHTML or local skipping variant
+ htmlLink.Image new field
+ htmlLink.Name uses suppressed-walk
+
+generator.go ────────────────U2
+ renderAuthFiles early-return
+ on auth.type:none
+
+skills/printing-press/SKILL.md ───U4
+ Phase 3 build checklist +1 bullet
+ + Verify-friendly RunE example
+```
+
+The doctor's HTTP probe shifts from:
+
+ httpClient := &http.Client{Timeout: 5*time.Second}
+ headResp, err := httpClient.Do(headReq)
+
+to (directional sketch):
+
+ c, err := flags.newClient()
+ body, err := c.Get("/", nil)
+ if err != nil { /* transport failed entirely */ }
+ if looksLikeHTMLChallenge("", string(body)) {
+ /* transport reached, interstitial blocked us — name vendor */
+ }
+
+The auth gate is a single early-return at the top of `renderAuthFiles`:
+
+ if g.Spec.Auth.Type == "none" &&
+ !g.hasTrafficAnalysisHint("graphql_persisted_query") &&
+ g.Spec.Auth.AuthorizationURL == "" {
+ return nil // no auth.go emitted
+ }
+
+The HTML extract suppression rule, in `nodeText` or its caller:
+
+ walkHTML(n, func(child *xhtml.Node) {
+ if child.Type == xhtml.ElementNode && isRawTextTag(child.Data) {
+ return // SKIP this node and its subtree
+ }
+ if child.Type == xhtml.TextNode {
+ parts = append(parts, child.Data)
+ }
+ })
+ where isRawTextTag(name) -> name in {"noscript","script","style","template"}
+
+The dryRunOK helper:
+
+ func dryRunOK(_ *cobra.Command, flags *rootFlags) bool {
+ return flags != nil && flags.dryRun
+ }
+
+Each of these is small and self-contained. The plan separates them into four units so they can land as independent commits and so a reviewer can validate each piece in isolation.
+
+---
+
+## Implementation Units
+
+- U1. **Doctor template uses configured transport**
+
+**Goal:** Replace the stdlib `http.Client` reachability probe in `doctor.go.tmpl` with calls through `flags.newClient()` so the doctor honors `http_transport` and accurately reports connectivity for browser-chrome / browser-chrome-h3 specs. Add Cloudflare/Akamai/Vercel-WAF interstitial detection for the case where Surf reaches the wall.
+
+**Requirements:** R1, R2, R10
+
+**Dependencies:** None.
+
+**Files:**
+- Modify: `internal/generator/templates/doctor.go.tmpl`
+- Modify (likely): `internal/cliutil/` — promote `looksLikeHTMLChallenge` from `html_extract.go.tmpl` to a shared cliutil helper if not already there. May not be needed if the doctor template duplicates the markers locally; implementer's call.
+- Test: `internal/generator/doctor_test.go` (extend if exists; otherwise add a focused regen-and-assert test)
+
+**Approach:**
+- Remove stdlib `net/http` import unless still used elsewhere in the template (e.g., for credential validation; that path also moves onto `flags.newClient` per R1).
+- Replace the `httpClient := &http.Client{}` block with `c, err := flags.newClient()`. Handle error → mark API as "client init error" with the underlying error message.
+- Replace the `httpClient.Do(headReq)` HEAD probe with `body, err := c.Get("/", nil)`. The reachability question is "did the request return a non-error response," not "did it return a specific status."
+- For the credential validation block (when auth is configured): replace `httpClient.Do(authReq)` with `c.GetWithHeaders(verifyPath, nil, map[string]string{authHeaderName: authHeader})`. The Authorization header injection moves from manual `req.Header.Set` to the client's headers param.
+- After a successful fetch, inspect the body for known interstitial markers using `looksLikeHTMLChallenge`. When the body matches: report "blocked by `<vendor>` interstitial — the configured transport reached the wall. Try a different network or check the browser-chrome transport binding." Distinguish vendor by which marker matched (cloudflare / akamai / vercel / aws waf / datadome).
+- Do NOT add any stdlib fallback path. If `flags.newClient()` returns an error, propagate it. If `c.Get()` returns an error, report it. The user's transport choice is honored end-to-end.
+
+**Patterns to follow:**
+- The existing hand-edit in `~/printing-press/library/allrecipes/internal/cli/doctor.go` (post-Phase-4 of the allrecipes generation) is the working reference. Generalize it into the template.
+- `looksLikeHTMLChallenge` at `internal/generator/templates/html_extract.go.tmpl:280` lists the markers.
+- Existing client-aware code in `internal/generator/templates/sync.go.tmpl` (or any sync command) — those use `flags.newClient()` already and are the reference for error handling.
+
+**Test scenarios:**
+- Happy path: generate a CLI with `http_transport: standard`. Run doctor against a public test endpoint (mock server returning `<html><body>OK</body></html>`). Assert: `report["api"] == "reachable"`. The doctor code uses `flags.newClient()`.
+- Happy path: generate a CLI with `http_transport: browser-chrome`. Run doctor in offline test mode (the client.Client init succeeds even without network); assert that the generated doctor.go imports do NOT include `net/http` and DO use `flags.newClient()` (string match on generated output).
+- Edge case: spec without `auth.verify_path` runs the reachability probe but skips credential validation entirely; doctor reports `auth: not required` or auth is unconfigured.
+- Error path: simulate `flags.newClient()` returning an error (mock the constructor to fail). Doctor reports `api: "client init error: <msg>"` and exits non-zero only when `--fail-on error`.
+- Error path: response body contains the Cloudflare interstitial marker. Doctor reports `api: "blocked by Cloudflare 'Just a moment...' interstitial — ..."` with vendor named. Test by feeding the well-known interstitial fixture as the mock response body.
+- Negative: response body contains `<title>Cloudflare error</title>` BUT also has actual content (200 with embedded error message, not a true challenge page). Detection should NOT false-positive on this; only the actual challenge markers ("Just a moment", "challenges.cloudflare.com", etc.) trigger.
+- Regression: regenerate `notion-pp-cli` and assert that doctor still passes its existing tests (no behavior change for standard-transport CLIs).
+- Integration: regenerate `recipe-goat-pp-cli` (already uses browser-chrome) and assert doctor reports `api: "reachable (via browser-chrome transport)"` against the real recipe-goat base URL when run with internet.
+
+**Verification:**
+- `go test ./internal/generator/...` passes including new doctor template tests.
+- Manually regenerate `allrecipes` from spec; the generated `internal/cli/doctor.go` no longer imports `net/http` (or imports it only for `*http.Request` types, not `http.Client`).
+- The hand-patch I applied earlier becomes unnecessary — regenerated allrecipes doctor matches my hand-edit shape.
+
+---
+
+- U2. **Skip auth subcommand and registration when auth.type=="none"**
+
+**Goal:** When the spec declares `auth.type: none` AND no graphql_persisted_query traffic-analysis hint AND no AuthorizationURL, generator does not emit `internal/cli/auth.go` and does not register `newAuthCmd(&flags)`. Scorecard auth dimension exempts no-auth specs from "no auth subcommand" deductions.
+
+**Requirements:** R3, R4, R5, R10
+
+**Dependencies:** None.
+
+**Files:**
+- Modify: `internal/generator/generator.go` (`renderAuthFiles` function around line 1240)
+- Modify: `internal/generator/templates/root.go.tmpl` (around line 200)
+- Modify: `internal/pipeline/scorecard.go` (`scoreAuth` function around line 295; signature plumbing through callers around line 114)
+- Modify: `internal/generator/templates/auth.go.tmpl`, `auth_simple.go.tmpl`, `auth_browser.go.tmpl` — no edits required; they just don't get rendered for no-auth specs
+- Test: `internal/generator/generator_test.go` (auth rendering test); `internal/pipeline/scorecard_test.go` (auth dimension test)
+
+**Approach:**
+- In `generator.go:renderAuthFiles`, add an early return at the top:
+ - When `g.Spec.Auth.Type == "none"` AND `g.Spec.Auth.AuthorizationURL == ""` AND `!g.hasTrafficAnalysisHint("graphql_persisted_query")` → return nil immediately. No auth.go is written.
+- In `root.go.tmpl`, wrap the `rootCmd.AddCommand(newAuthCmd(&flags))` line in a Go template conditional: `{{if not .Spec.Auth.IsNone}}` (or equivalent — there may already be an `IsAuthenticated`-style predicate; check `internal/spec/spec.go` for what's exposed to templates).
+- In `scoreAuth`, change the signature to accept the parsed spec (or just the `Auth` struct). Callers at `scorecard.go:114` already have the spec in scope.
+- Add the no-auth exemption at the top of `scoreAuth`: when `spec.Auth.Type == "none"`, return 10 with a marker that downstream JSON output can surface (e.g., set a sibling `notes["auth"] = "exempted: auth.type:none"` if the score struct supports it, or accept that 10/10 alone communicates the exemption).
+- Existing no-auth signals (config permissions, has-store, etc.) are NOT used; we exit early.
+- Do not change behavior for auth.type:api_key, bearer_token, oauth, cookie, composed, session_handshake, etc.
+
+**Patterns to follow:**
+- Existing template conditionals in `root.go.tmpl` for sync/import/export commands — those gate on `.HasStore` and similar predicates. The auth gate is the same shape.
+- Existing early-return conditionals in `generator.go` (other render functions skip on guards). `renderMCPFiles` is a comparable pattern.
+- `scoreAuth`'s style: the current "auto-award 2 points so the ceiling is 10/10" pattern at line 333 is the existing pattern for "this dimension can't fully measure what we want, so we cap." Extending that with a no-auth exemption is consistent.
+
+**Test scenarios:**
+- Happy path (no-auth): generate from a spec with `auth: {type: none}`. Assert: `internal/cli/auth.go` does NOT exist in the output; `root.go` does NOT contain `newAuthCmd`; scorecard `auth: 10`.
+- Happy path (api-key auth): generate from a spec with `auth: {type: api_key, header: X-API-Key}`. Assert: `internal/cli/auth.go` exists with the simple template's contents; `root.go` contains `newAuthCmd`; scorecard auth scoring runs the existing path.
+- Edge case: spec with `auth: {type: none}` AND `traffic_analysis: {hints: ["graphql_persisted_query"]}`. Assert: auth.go IS emitted (auth_browser template) — because GraphQL persisted-queries need browser-aware refresh flows even without traditional auth. Existing exception preserved.
+- Edge case: spec with `auth: {type: none}` AND `auth.authorization_url: <url>`. Assert: auth.go IS emitted (auth.go.tmpl OAuth path) — AuthorizationURL implies OAuth even if Type is "none."
+- Error path: scoreAuth called without a spec arg (after the signature change, all callers must pass it). Compile-time guard via Go's static typing; no runtime test needed.
+- Regression: re-run scorecard on three existing CLIs of different auth shapes (notion = api_key, github = bearer, recipe-goat = api_key, slack = composed). Their auth scores must not change.
+- Negative: a spec without an `auth:` section (legacy-style spec where Auth defaults to a non-none zero value). Assert: behavior is unchanged (the early-return only fires on explicit `auth.type:none`).
+
+**Verification:**
+- `go test ./internal/generator/... ./internal/pipeline/...` passes.
+- Regenerate `allrecipes` from spec → the manual `auth.go` deletion + root.go registration removal I performed during the run becomes unnecessary; the generator does it.
+- Regenerate `notion-pp-cli` → no diff in auth-related files; scorecard auth unchanged.
+
+---
+
+- U3. **HTML extract mode:links produces clean text + separate image URL**
+
+**Goal:** `extractHTMLLink` skips `<noscript>`, `<script>`, `<style>`, `<template>` subtrees so raw `<img>` markup from JS-fallback content doesn't leak into result fields. Add `htmlLink.Image` populated from the first nested image URL.
+
+**Requirements:** R6, R7, R10
+
+**Dependencies:** None.
+
+**Files:**
+- Modify: `internal/generator/templates/html_extract.go.tmpl`:
+ - `htmlLink` struct (lines 33-39): add `Image string` field
+ - `nodeText` function (lines 246-255): add raw-text-tag suppression OR add a `nodeTextSkipping` variant used by `extractHTMLLink`
+ - `extractHTMLLink` function (around line 122): populate `Image` from first non-suppressed `<img src>`
+- Test: `internal/generator/templates/html_extract_test.go` if such a test exists (check; the template is rendered into every CLI but the logic could be tested against fixtures)
+
+**Approach:**
+- Define `var rawTextTags = map[string]struct{}{"noscript":{}, "script":{}, "style":{}, "template":{}}`.
+- Modify `nodeText` (or introduce `nodeTextNoRawText`) to skip subtrees rooted at any element in `rawTextTags`. Implementation choice: walk children manually, checking each child's `Type == ElementNode` and skipping descent when `rawTextTags[strings.ToLower(child.Data)]` matches. The current `walkHTML` is a generic recursion; the cleanest fix is a tighter walker local to text extraction, leaving `walkHTML` untouched (it's used for other things like meta-tag scanning that shouldn't suppress content).
+- In `extractHTMLLink`, after computing the suppressed text, also walk the anchor's children to find the first `<img>` element NOT inside a suppressed subtree. Use `attrValue(img, "src")` and pass through `normalizeHTMLURL` to get an absolute URL.
+- The `htmlLink.Image` field is omitted from JSON output when empty (`json:"image,omitempty"`).
+- Verify `splitRankedHTMLLinkText` and other callers of cleaned anchor text still work — they consume the cleaned string, not the raw nodes.
+- Do not add new spec configuration. The four suppressed tags are stable across HTML5; this isn't a per-spec choice.
+
+**Patterns to follow:**
+- `walkHTML` (lines 217-225) — the existing recursion. The new walker mirrors its shape but adds the suppression check.
+- `attrValue` (line 240) — reuse for `src` lookup.
+- `normalizeHTMLURL` (line 168) — already used to absolute-ize anchor `href`s; reuse for image `src` URLs.
+
+**Test scenarios:**
+- Happy path: anchor with both rendered `<img>` and `<noscript><img></noscript>` (Allrecipes pattern). Assert: `Name` contains only the visible link text (no `<img src=...>` markup); `Image` is the rendered image URL, not the noscript fallback.
+- Happy path: anchor with no `<img>` at all (text-only listing). Assert: `Name` is the visible text; `Image` is empty string (omitted from JSON).
+- Edge case: anchor wraps `<noscript>` with multiple image fallbacks. Assert: `Name` excludes all noscript content; `Image` is empty (because the first non-suppressed image doesn't exist).
+- Edge case: anchor with `<script>` injected inline (some sites do this for analytics). Assert: script content is suppressed in Name.
+- Edge case: anchor with `<style>` block (rare but valid). Assert: style content is suppressed.
+- Edge case: anchor with `<template>` (HTML5 template tag for client-side rendering). Assert: template content is suppressed.
+- Edge case: anchor wrapping a single visible image (no alt text, no surrounding text). Assert: `Name` falls back to image alt or empty; `Image` is captured.
+- Edge case: nested anchor (browsers don't render this but HTML allows it post-parsing). Behavior should be deterministic; document the expected outcome in test.
+- Regression: existing recipe-goat fixtures (cookbook search) parse with no behavioral change. `Name` and `Text` for plain anchor text remain identical.
+- Negative: HTML with malformed noscript (missing close tag). The html parser auto-closes; suppression should still work because the parser's tree structure is what we're walking.
+
+**Verification:**
+- `go test ./internal/generator/...` passes including new html_extract template tests.
+- Manually regenerate `allrecipes`; run `recipes search --q brownies --json`. Assert: each result's `Name` is clean (e.g., "S'mores Brownies 342 Ratings") with no `<img src=...>` markup.
+- Manually compare the regenerated allrecipes search output against my hand-written `internal/recipes/search.go` output. They should produce equivalent SearchResult title fields for the same input HTML.
+
+---
+
+- U4. **Verify-friendly RunE pattern for hand-written commands**
+
+**Goal:** Emit a `dryRunOK(cmd, flags) bool` helper in `helpers.go.tmpl` and document the verify-friendly RunE pattern in the Printing Press SKILL Phase 3 build instructions so hand-written transcendence commands pass verify on first run.
+
+**Requirements:** R8, R9
+
+**Dependencies:** None. (Independent of WU-1/2/3 — these change different files.)
+
+**Files:**
+- Modify: `internal/generator/templates/helpers.go.tmpl`
+- Modify: `skills/printing-press/SKILL.md` (Phase 3 — Build The GOAT, around line 1522; Agent Build Checklist around line 1575)
+- Test: `internal/generator/generator_test.go` (verify the new helper renders into output)
+
+**Approach:**
+- In `helpers.go.tmpl`, add a small helper:
+ - `dryRunOK(cmd *cobra.Command, flags *rootFlags) bool` returning `flags != nil && flags.dryRun`. Cheap; named so the call site reads naturally: `if dryRunOK(cmd, flags) { return nil }`.
+ - Place it near other generic helpers (after `replacePathParam` if still present, otherwise wherever other no-arg utility helpers live).
+- In `skills/printing-press/SKILL.md`:
+ - Add an 8th principle to the "Agent Build Checklist (per command)" list at line 1575: "**Verify-friendly RunE:** no `Args: cobra.MinimumNArgs(N)`, no `MarkFlagRequired(...)` for hand-written commands. RunE checks `len(args) == 0` first → falls through to `cmd.Help()`. Then calls `dryRunOK(cmd, flags)` → returns nil if dry-run. Then runs real logic."
+ - Add a small subsection "Verify-friendly RunE template" with a directional code sketch (framed as guidance):
+
+ RunE: func(cmd *cobra.Command, args []string) error {
+ if len(args) == 0 {
+ return cmd.Help()
+ }
+ if dryRunOK(cmd, flags) {
+ return nil
+ }
+ // ... real logic ...
+ }
+
+ - Note explicitly: cobra evaluates `Args:` and `MarkFlagRequired` BEFORE RunE runs, so a dry-run guard inside RunE cannot reach if those gates fail. For hand-written commands, do the validation inside RunE.
+- The SKILL change is additive: existing language about `Args: cobra.MinimumNArgs(N)` (which DOES appear elsewhere as a pattern for spec-derived commands) is unchanged. The new guidance specifically applies to hand-written novel-feature commands in Phase 3.
+
+**Patterns to follow:**
+- Existing helpers in `helpers.go.tmpl` — short, no-doc-comment-needed utilities (string helpers, etc.).
+- Existing SKILL.md Phase 3 section structure — bullet checklist format, then prose templates.
+
+**Test scenarios:**
+- Happy path: regenerate any CLI; assert `internal/cli/helpers.go` contains the `dryRunOK` function with the right signature.
+- Happy path: a hand-written command author follows the SKILL guidance; verify probes the command with `--dry-run`; the command returns nil and exits 0. (This is a meta-test; the actual validation happens during a future generation's Phase 4 verify pass.)
+- Negative: regenerate an existing CLI like `notion-pp-cli`; assert no behavioral change to its handlers (existing handlers don't use the new helper but build is unaffected).
+- Verify the SKILL change renders cleanly in markdown (no syntax issues).
+
+**Verification:**
+- `go test ./internal/generator/...` passes; the new helper appears in the rendered helpers.go.
+- Run a fresh `/printing-press <api>` against a small spec; the resulting `internal/cli/helpers.go` contains `dryRunOK`.
+- Read the updated SKILL.md and confirm the Phase 3 build instructions now mention the verify-friendly pattern and `dryRunOK`.
+
+---
+
+## System-Wide Impact
+
+- **Interaction graph:**
+ - WU-1: doctor → flags.newClient → client.Client → Surf transport. All four call sites in doctor template move to the wrapped client.
+ - WU-2: generator.renderAuthFiles → spec.Auth.Type → conditional render. root.go template → conditional command registration. scorecard.scoreAuth → spec arg → exemption.
+ - WU-3: html_extract.nodeText → suppression list. extractHTMLLink → first-image walk. Downstream: every generated `*_search.go` link-mode handler.
+ - WU-4: helpers.go.tmpl → new symbol. SKILL Phase 3 → guidance for hand-written code. No runtime interaction surface.
+
+- **Error propagation:**
+ - WU-1: doctor errors propagate through `report["api"]` and are surfaced in JSON output. Vendor-named blockages give actionable text. Transport errors propagate honestly (no fallback).
+ - WU-2: generator early-return on no-auth doesn't error; scorecard exemption returns 10/10 cleanly.
+ - WU-3: HTML parsing errors continue to propagate as before. Suppression doesn't introduce new error paths.
+ - WU-4: helper is pure; no error surface.
+
+- **State lifecycle risks:**
+ - None of the four units introduce new persistent state, caching, or rollout concerns. All are stateless transformations.
+
+- **API surface parity:**
+ - WU-1 changes the doctor's *implementation* but not its JSON output schema. The `report` object's keys are unchanged for the success path; new diagnostic strings appear in failure messages but the field is the same `report["api"]` string. Documented as additive.
+ - WU-2 removes `auth set-token / status / logout` subcommands from no-auth CLIs. Users running `<cli> auth set-token` on a regenerated no-auth CLI will get "unknown command" instead of a no-op. This is intentional but should be noted in the no-auth CLI's Troubleshooting.
+ - WU-3 adds `htmlLink.Image` field. Existing consumers ignore unknown fields by default (Go json decoder behavior); JSON consumers should treat it as additive.
+ - WU-4 is internal helper plus SKILL guidance; no external API change.
+
+- **Integration coverage:**
+ - WU-1 needs a regenerate-and-run integration test against a Cloudflare-fronted CLI (allrecipes, recipe-goat). Mocks alone won't prove the Surf path works end-to-end through the new doctor.
+ - WU-3 needs a regenerate-and-run check of the allrecipes search output to confirm the noscript fix produces clean fields against the live Allrecipes search page.
+
+- **Unchanged invariants:**
+ - Surf TLS impersonation behavior is unchanged. WU-1 reuses it; WU-2/3/4 don't touch transport.
+ - `http_transport: standard` CLIs continue to use the underlying http.DefaultTransport via Surf's standard mode; no regression for non-Cloudflare CLIs.
+ - Auth-needing CLIs continue to ship with full auth subcommand trees and full auth scoring.
+ - The HTML extract `mode: page` path (used by recipe-get-style commands) is unchanged; only `mode: links` text extraction is hardened.
+
+---
+
+## Risks & Dependencies
+
+| Risk | Mitigation |
+|------|------------|
+| WU-1: `flags.newClient()` may not be reachable from the doctor's PreRunE timing in some templates (e.g., when doctor has its own short-circuit before global PreRunE runs). | Verify by reading the existing doctor template's RunE. The current code already calls into `cfg, err := config.Load(flags.configPath)` early, which is comparable timing. If the constructor depends on PreRunE-set state that doctor doesn't set, document and add the missing init. |
+| WU-2: scoreAuth signature change cascades to all callers. | The function has one caller (`scorecard.go:114`). Plumbing the spec is a one-line change at the call site. Verify no test fixtures call `scoreAuth(dir)` directly without the spec; if they do, update them. |
+| WU-2: existing CLIs in the test fixtures may have implicit `auth.type:none` specs that newly trigger the exemption, changing their golden scores. | Run scorecard against every fixture spec post-change; if a no-auth fixture exists, it should now score 10 instead of whatever it scored before. Document this in the test diff. |
+| WU-3: skipping noscript may break CLIs that intentionally extract noscript content (rare but possible — e.g., extracting fallback URLs for users with JS disabled). | Audit existing CLIs that use html_extract `mode: links` (recipe-goat, hackernews?, agent-capture?) and verify none rely on noscript content. If one does, expose the suppression list as a spec config knob. |
+| WU-4: skill change is documentation; agents may not read it consistently. | Skill guidance is complementary to the helper emission. The helper exists regardless; agents who don't read the skill still get correct hand-written commands when they use the helper, and agents who do read the skill get the full pattern. The polish-worker continues as a backstop. |
+| All four: regenerating existing library CLIs with the new templates may surface unexpected diffs in `auth.go` (deleted), `doctor.go` (rewritten), `helpers.go` (new symbol). | Verify on a sample of 3 existing CLIs (notion, github, recipe-goat) before merge; document the expected diff shape in the PR description. |
+
+---
+
+## Documentation / Operational Notes
+
+- The retro at `https://files.catbox.moe/h5zu55.md` documents the original allrecipes generation that surfaced these findings. Link it from the PR description.
+- The validation comment at https://github.com/mvanhorn/cli-printing-press/issues/333#issuecomment-4325110340 contains corrected diagnoses for F4 (noscript-specific, not generic nested HTML) and F8 (regex misses flag types — out of scope for this plan; that's WU-6 in a separate plan).
+- After merge, regenerate `allrecipes`, `recipe-goat`, and one no-auth CLI (e.g., a public-data CLI in the catalog) to confirm the fixes ship cleanly.
+- Update CHANGELOG.md under the next release with: "feat(cli): doctor honors http_transport; auth subcommand skipped for no-auth specs; html-extract suppresses noscript subtrees; new dryRunOK helper for hand-written commands."
+
+---
+
+## Sources & References
+
+- **Origin issue:** [mvanhorn/cli-printing-press#333](https://github.com/mvanhorn/cli-printing-press/issues/333)
+- **Validation comment:** [issue #333 comment](https://github.com/mvanhorn/cli-printing-press/issues/333#issuecomment-4325110340)
+- **Retro document:** https://files.catbox.moe/h5zu55.md (also at `~/printing-press/manuscripts/allrecipes/20260426-230519/proofs/20260427-000900-retro-allrecipes-pp-cli.md`)
+- Related code:
+ - `internal/generator/templates/doctor.go.tmpl`
+ - `internal/generator/generator.go:renderAuthFiles`
+ - `internal/generator/templates/root.go.tmpl`
+ - `internal/generator/templates/helpers.go.tmpl`
+ - `internal/generator/templates/html_extract.go.tmpl`
+ - `internal/pipeline/scorecard.go:scoreAuth`
+ - `skills/printing-press/SKILL.md` Phase 3
+- Related plans:
+ - `docs/plans/2026-04-13-002-feat-cloudflare-cli-learnings-plan.md` (browser-chrome transport adoption)
+ - `docs/plans/2026-04-19-004-feat-auth-doctor-plan.md` (separate concern: printing-press binary's auth-doctor subcommand)
+- Related retro: `docs/retros/2026-04-13-recipe-goat-retro.md` (related symptom: hand-written command verify failures)
diff --git a/internal/generator/generator.go b/internal/generator/generator.go
index ba8ad6a7..982f97a7 100644
--- a/internal/generator/generator.go
+++ b/internal/generator/generator.go
@@ -1238,7 +1238,14 @@ func (g *Generator) renderResourceCommands(promotedResourceNames map[string]bool
}
func (g *Generator) renderAuthFiles() error {
- // Always render auth command - use full OAuth2 template when authorization URL is present,
+ // Skip auth.go entirely when the spec declares no auth surface. See
+ // shouldEmitAuth for the predicate; the matching root.go template gate
+ // (HasAuthCommand) and the scorecard's no-auth exemption (scoreAuth)
+ // must stay in sync with this same condition.
+ if !g.shouldEmitAuth() {
+ return nil
+ }
+ // Render auth command - use full OAuth2 template when authorization URL is present,
// browser cookie template for cookie-auth APIs, otherwise simple token-management template
authPath := filepath.Join("internal", "cli", "auth.go")
authTmpl := "auth_simple.go.tmpl"
@@ -1272,6 +1279,22 @@ func (g *Generator) renderAuthFiles() error {
return nil
}
+// shouldEmitAuth reports whether the generator should emit internal/cli/auth.go
+// for this spec. Auth UI is emitted when the spec describes a real auth
+// surface: a non-none auth.type, an AuthorizationURL (OAuth), or a
+// graphql_persisted_query traffic-analysis hint (browser-aware refresh).
+//
+// Public-data specs (auth.type: "none", no OAuth, no GraphQL persisted-query)
+// previously shipped a dead `auth set-token / status / logout` subcommand.
+// Now they ship without it, root.go skips the registration via
+// HasAuthCommand, and scoreAuth exempts them from the "no auth subcommand"
+// deduction. All three call sites must agree -- they call this method.
+func (g *Generator) shouldEmitAuth() bool {
+ return g.Spec.Auth.Type != "none" ||
+ g.Spec.Auth.AuthorizationURL != "" ||
+ g.hasTrafficAnalysisHint("graphql_persisted_query")
+}
+
func (g *Generator) renderMCPEntrypoint() error {
// MCP server: generate cmd/{name}-pp-mcp/ entry point and internal/mcp/ package
if g.VisionSet.MCP || true { // Always generate MCP for now
@@ -1624,6 +1647,11 @@ func (g *Generator) renderRootProjectFiles(promotedCommands []PromotedCommand, p
overflow = len(shownNovel) - maxHighlightLines
shownNovel = shownNovel[:maxHighlightLines]
}
+ // HasAuthCommand mirrors shouldEmitAuth. The template uses it to gate
+ // rootCmd.AddCommand(newAuthCmd) so the root binary does not reference an
+ // undefined symbol when auth.go was skipped.
+ hasAuthCommand := g.shouldEmitAuth()
+
rootData := struct {
*spec.APISpec
VisionSet VisionTemplateSet
@@ -1637,6 +1665,7 @@ func (g *Generator) renderRootProjectFiles(promotedCommands []PromotedCommand, p
NovelOverflowCount int
HasAsyncJobs bool
AsyncJobCount int
+ HasAuthCommand bool
}{
APISpec: g.Spec,
VisionSet: g.VisionSet,
@@ -1650,6 +1679,7 @@ func (g *Generator) renderRootProjectFiles(promotedCommands []PromotedCommand, p
NovelOverflowCount: overflow,
HasAsyncJobs: len(g.AsyncJobs) > 0,
AsyncJobCount: len(g.AsyncJobs),
+ HasAuthCommand: hasAuthCommand,
}
if err := g.renderTemplate("root.go.tmpl", filepath.Join("internal", "cli", "root.go"), rootData); err != nil {
return fmt.Errorf("rendering root: %w", err)
diff --git a/internal/generator/generator_test.go b/internal/generator/generator_test.go
index 97a5adda..7de9eed3 100644
--- a/internal/generator/generator_test.go
+++ b/internal/generator/generator_test.go
@@ -674,13 +674,22 @@ func TestGenerateHTMLExtractionEndpoint(t *testing.T) {
case "/docs/page":
_, _ = w.Write([]byte(`<html><head><title>Docs</title></head><body><a href="child">Child page</a></body></html>`))
case "/makers":
- _, _ = w.Write([]byte(`<html><head><title>Makers</title></head><body><a href="/@alice">Alice</a></body></html>`))
+ // Lazy-load fixture: anchor's <img> has a placeholder src + real
+ // data-src (Pinterest/NYT Cooking pattern). firstImageSrc must
+ // prefer data-src over src.
+ _, _ = w.Write([]byte(`<html><head><title>Makers</title></head><body><a href="/@alice"><img src="data:image/gif;base64,placeholder" data-src="/img/alice-real.jpg" alt="Alice">Alice</a><a href="/@bob"><img srcset="/img/bob-1x.jpg 1x, /img/bob-2x.jpg 2x" alt="Bob">Bob</a></body></html>`))
default:
+ // Anchor 1 wraps its image in <noscript> (Dotdash/Meredith pattern).
+ // nodeTextSuppressing must skip the noscript subtree so "<img src=...>"
+ // markup does not leak into the link's name field. firstImageSrc must
+ // also skip the noscript image and look for a rendered <img> instead --
+ // here SpeakON has both: a noscript fallback AND a rendered img after.
+ // Anchor 2 has only a rendered <img> (no noscript fallback).
_, _ = w.Write([]byte(`<html>
<head><title>Product Hunt</title><meta name="description" content="New products"></head>
<body>
- <a href="/products/speakon">1. SpeakON</a>
- <a href="/products/instant-db">2. InstantDB</a>
+ <a href="/products/speakon"><noscript><img src="/img/speakon-fallback.jpg" alt="SpeakON"></noscript><img src="/img/speakon.jpg" alt="SpeakON">1. SpeakON</a>
+ <a href="/products/instant-db"><img src="/img/instant-db.jpg" alt="InstantDB">2. InstantDB</a>
<a href="/about">About</a>
</body>
</html>`))
@@ -779,6 +788,18 @@ func TestGenerateHTMLExtractionEndpoint(t *testing.T) {
assert.Equal(t, "SpeakON", links[0]["name"])
assert.Equal(t, "speakon", links[0]["slug"])
assert.Equal(t, float64(1), links[0]["rank"])
+ // noscript suppression: the anchor wrapped its fallback <img> in <noscript>,
+ // which the HTML5 spec parses as raw text. nodeTextSuppressing must skip
+ // the noscript subtree so the link name does not leak "<img src=..." markup.
+ assert.NotContains(t, links[0]["name"], "<img", "noscript-wrapped <img> markup must not leak into the link name")
+ assert.NotContains(t, links[0]["text"], "<img", "noscript-wrapped <img> markup must not leak into the link text")
+ // Image extraction: firstImageSrc walks the same suppression-aware tree
+ // and surfaces the first non-suppressed <img src>. The rendered image
+ // should win over the noscript fallback when both are present.
+ assert.Contains(t, links[0]["image"], "speakon.jpg", "expected rendered img URL, got %v", links[0]["image"])
+ assert.NotContains(t, links[0]["image"], "speakon-fallback.jpg", "noscript fallback must not be selected when a rendered image exists")
+ // Anchor without noscript still produces a clean image URL.
+ assert.Contains(t, links[1]["image"], "instant-db.jpg")
cmd = exec.Command(binaryPath, "posts", "list", "--dry-run", "--json")
cmd.Env = append(os.Environ(), "WEBHTML_BASE_URL="+server.URL)
@@ -804,9 +825,22 @@ func TestGenerateHTMLExtractionEndpoint(t *testing.T) {
out, err = cmd.CombinedOutput()
require.NoError(t, err, string(out))
require.NoError(t, json.Unmarshal(out, &envelope), string(out))
- require.Len(t, envelope.Results, 1)
+ require.Len(t, envelope.Results, 2)
assert.Equal(t, server.URL+"/@alice", envelope.Results[0]["url"])
assert.Equal(t, "alice", envelope.Results[0]["slug"])
+ // Lazy-load priority: data-src wins over a placeholder src. The fixture
+ // embeds a base64 1x1 gif in src and the real URL in data-src. The result
+ // must be the data-src URL, NOT the placeholder.
+ assert.Contains(t, envelope.Results[0]["image"], "alice-real.jpg",
+ "data-src should win over a placeholder src; got %v", envelope.Results[0]["image"])
+ assert.NotContains(t, envelope.Results[0]["image"], "data:image/gif",
+ "placeholder src should not be selected when data-src is present")
+ // srcset fallback: when src and data-src are absent, the first srcset URL
+ // is taken. The fixture has only `srcset="/img/bob-1x.jpg 1x, ..."`, so
+ // firstSrcsetURL should extract the 1x URL.
+ assert.Equal(t, server.URL+"/@bob", envelope.Results[1]["url"])
+ assert.Contains(t, envelope.Results[1]["image"], "bob-1x.jpg",
+ "first srcset URL should be selected when src is absent; got %v", envelope.Results[1]["image"])
}
func TestGenerateStandardTransportForOfficialAPI(t *testing.T) {
@@ -2312,15 +2346,70 @@ func TestGeneratedDoctor_AuthVerifyPathProbesEndpoint(t *testing.T) {
require.NoError(t, err)
content := string(doctorGo)
- // Probe should target baseURL + verify_path, not bare baseURL
+ // Probe should target baseURL + verify_path via the configured client,
+ // not bare baseURL. The doctor uses flags.newClient() now (Surf-aware)
+ // instead of stdlib http.Client.
assert.Contains(t, content, `verifyPath := "/me?fields=id"`)
- assert.Contains(t, content, `http.NewRequest("GET", baseURL+verifyPath, nil)`)
+ assert.Contains(t, content, `c.GetWithHeaders(verifyPath`)
+ assert.NotContains(t, content, `&http.Client{`)
// When verify_path is set, 401/403 keeps the strict "invalid" verdict
assert.Contains(t, content, `"invalid (HTTP %d) — check your credentials"`)
// And does NOT emit the inconclusive fallback wording
assert.NotContains(t, content, "inconclusive (HTTP %d from base URL")
}
+func TestGeneratedDoctor_InterstitialMarkersAreTitleAnchored(t *testing.T) {
+ t.Parallel()
+
+ // looksLikeDoctorInterstitial must anchor loose Cloudflare markers to the
+ // <title> tag so a real recipe titled "Just A Moment of Pause Cookies"
+ // (or similar benign content) is not mistakenly classified as a Cloudflare
+ // challenge page. This guards against a regression where the marker list
+ // was checked against the body's lowercased prefix without title context.
+ apiSpec := &spec.APISpec{
+ Name: "interstitialdoc",
+ Version: "0.1.0",
+ BaseURL: "https://api.example.com",
+ Auth: spec.AuthConfig{Type: "none"},
+ Config: spec.ConfigSpec{
+ Format: "toml",
+ Path: "~/.config/interstitialdoc-pp-cli/config.toml",
+ },
+ Resources: map[string]spec.Resource{
+ "items": {
+ Description: "Manage items",
+ Endpoints: map[string]spec.Endpoint{
+ "list": {Method: "GET", Path: "/items", Description: "List items"},
+ },
+ },
+ },
+ }
+
+ outputDir := filepath.Join(t.TempDir(), "interstitialdoc-pp-cli")
+ gen := New(apiSpec, outputDir)
+ require.NoError(t, gen.Generate())
+
+ doctorGo, err := os.ReadFile(filepath.Join(outputDir, "internal", "cli", "doctor.go"))
+ require.NoError(t, err)
+ content := string(doctorGo)
+
+ // Cloudflare's loose "just a moment" marker must be anchored to <title>.
+ assert.Contains(t, content, `"<title>just a moment"`,
+ "Cloudflare 'just a moment' marker should be anchored to <title> to avoid false positives on benign content")
+ // And the bare unanchored variant must NOT appear by itself in the
+ // switch-case (the tightened version still contains "just a moment" as
+ // a substring of the anchored marker, so we check for the anchored form's
+ // presence rather than the bare form's absence).
+ // The <title-anchored gate at the top of the function is also required.
+ assert.Contains(t, content, `if !strings.Contains(prefix, "<title")`,
+ "interstitial detector should bail early when no <title> tag is present")
+ // DataDome marker must require an additional context word, not just the
+ // vendor name (which appears in legitimate analytics scripts on many
+ // sites that don't use DataDome for blocking).
+ assert.Contains(t, content, `strings.Contains(prefix, "datadome") && (strings.Contains(prefix, "blocked") || strings.Contains(prefix, "captcha") || strings.Contains(prefix, "challenge"))`,
+ "DataDome marker should require a context word (blocked/captcha/challenge) alongside the vendor name")
+}
+
func TestGeneratedDoctor_NoVerifyPathSoftens401(t *testing.T) {
t.Parallel()
@@ -2359,9 +2448,12 @@ func TestGeneratedDoctor_NoVerifyPathSoftens401(t *testing.T) {
require.NoError(t, err)
content := string(doctorGo)
- // Probe should hit the bare base URL (no verify_path appended)
- assert.Contains(t, content, `http.NewRequest("GET", baseURL, nil)`)
- assert.NotContains(t, content, "verifyPath := ")
+ // Without spec verify_path, the doctor probes "/" via the configured
+ // client. The client is the same Surf-aware *client.Client used by
+ // regular commands -- not a fresh stdlib http.Client.
+ assert.Contains(t, content, `verifyPath := "/"`)
+ assert.Contains(t, content, `c.GetWithHeaders(verifyPath`)
+ assert.NotContains(t, content, `&http.Client{`)
// 401/403 fallback must be the soft "inconclusive" verdict
assert.Contains(t, content, `"inconclusive (HTTP %d from base URL — set auth.verify_path in spec for a definitive probe)"`)
// And must NOT use the strict "invalid" wording
@@ -2565,9 +2657,11 @@ func TestGenerate_UserAgentOverrideGatedByBrowserTransport(t *testing.T) {
browserClient, err := os.ReadFile(filepath.Join(browserDir, "internal", "client", "client.go"))
require.NoError(t, err)
assert.NotContains(t, string(browserClient), `req.Header.Set("User-Agent"`)
- browserAuth, err := os.ReadFile(filepath.Join(browserDir, "internal", "cli", "auth.go"))
- require.NoError(t, err)
- assert.NotContains(t, string(browserAuth), "newAuthRefreshCmd")
+ // auth.go is not emitted for auth.type:none specs (see Generator.renderAuthFiles).
+ // Stronger assertion than the previous "no newAuthRefreshCmd in auth.go": there's
+ // no auth.go at all for no-auth CLIs.
+ _, err = os.Stat(filepath.Join(browserDir, "internal", "cli", "auth.go"))
+ assert.True(t, os.IsNotExist(err), "auth.go should not be emitted for auth.type:none specs")
}
func TestGenerateObjectBodyDefaultsAreParsedAsJSON(t *testing.T) {
@@ -2836,9 +2930,11 @@ func TestGeneratedOutput_NoMarkFlagRequired(t *testing.T) {
require.NoError(t, err)
content := string(data)
- // No command should use MarkFlagRequired (except import.go which is not verify-tested)
- if e.Name() != "import.go" && strings.Contains(content, "MarkFlagRequired") {
- t.Errorf("%s still contains MarkFlagRequired", e.Name())
+ // No command should call MarkFlagRequired (except import.go which is not verify-tested).
+ // Match call sites only (`.MarkFlagRequired(` with a quote arg) so that
+ // docstrings or comments mentioning the symbol don't false-positive.
+ if e.Name() != "import.go" && strings.Contains(content, `.MarkFlagRequired("`) {
+ t.Errorf("%s still calls MarkFlagRequired", e.Name())
}
// Track whether we find the RunE-based validation
diff --git a/internal/generator/templates/doctor.go.tmpl b/internal/generator/templates/doctor.go.tmpl
index 57fc96fa..9ea3a346 100644
--- a/internal/generator/templates/doctor.go.tmpl
+++ b/internal/generator/templates/doctor.go.tmpl
@@ -12,11 +12,11 @@ import (
{{- if .HasStore}}
"database/sql"
{{- end}}
+ "errors"
"fmt"
{{- if .HasStore}}
"io"
{{- end}}
- "net/http"
{{- if or .Auth.EnvVars .HasStore .Auth.RequiresBrowserSession}}
"os"
{{- end}}
@@ -27,8 +27,11 @@ import (
"path/filepath"
{{- end}}
"strings"
+{{- if .HasStore}}
"time"
+{{- end}}
+ "{{modulePath}}/internal/client"
"{{modulePath}}/internal/config"
{{- if .HasStore}}
"{{modulePath}}/internal/store"
@@ -36,6 +39,50 @@ import (
"github.com/spf13/cobra"
)
+// looksLikeDoctorInterstitial reports whether the response body matches a known
+// bot-detection challenge page (Cloudflare, Akamai, Vercel, AWS WAF, DataDome,
+// PerimeterX). Only fires on the doctor probe — used to distinguish "transport
+// reached the wall" from "transport failed entirely." Returns the vendor name
+// when matched, or empty string when no match.
+//
+// Markers are anchored to <title> or vendor-specific strings to avoid
+// false-positives on benign content. For example, a recipe titled "Just A
+// Moment of Pause Cookies" must NOT match the Cloudflare challenge marker;
+// only "<title>just a moment" (the actual interstitial title) does.
+func looksLikeDoctorInterstitial(body []byte) string {
+ if len(body) == 0 {
+ return ""
+ }
+ limit := len(body)
+ if limit > 8192 {
+ limit = 8192
+ }
+ prefix := strings.ToLower(string(body[:limit]))
+ if !strings.Contains(prefix, "<title") {
+ // Every bot interstitial we recognize sets a <title>; bodies without
+ // one are body-only API responses, not challenge pages.
+ return ""
+ }
+ switch {
+ case strings.Contains(prefix, "<title>just a moment") || // CF JS challenge
+ strings.Contains(prefix, "challenges.cloudflare.com") || // CF Turnstile
+ (strings.Contains(prefix, "attention required") && strings.Contains(prefix, "cloudflare")):
+ return "Cloudflare"
+ case strings.Contains(prefix, "akamai") && (strings.Contains(prefix, "request unsuccessful") || strings.Contains(prefix, "access denied")):
+ return "Akamai"
+ case strings.Contains(prefix, "x-vercel-mitigated") || strings.Contains(prefix, "x-vercel-challenge-token") ||
+ (strings.Contains(prefix, "vercel") && strings.Contains(prefix, "challenge")):
+ return "Vercel"
+ case strings.Contains(prefix, "request blocked") && strings.Contains(prefix, "aws waf"):
+ return "AWS WAF"
+ case strings.Contains(prefix, "datadome") && (strings.Contains(prefix, "blocked") || strings.Contains(prefix, "captcha") || strings.Contains(prefix, "challenge")):
+ return "DataDome"
+ case strings.Contains(prefix, "perimeterx") || strings.Contains(prefix, "px-captcha"):
+ return "PerimeterX"
+ }
+ return ""
+}
+
func newDoctorCmd(flags *rootFlags) *cobra.Command {
var failOn string
cmd := &cobra.Command{
@@ -147,103 +194,117 @@ func newDoctorCmd(flags *rootFlags) *cobra.Command {
}
{{- end}}
- // Check API connectivity and validate credentials
+ // Check API connectivity and validate credentials.
+ //
+ // The doctor uses the same client every other command uses --
+ // flags.newClient() returns a *client.Client wrapping whatever
+ // transport the spec declared (Surf for browser-chrome, stdlib
+ // for standard). A separate stdlib http.Client would silently
+ // bypass that choice and report false negatives against
+ // Cloudflare-fronted, Akamai-fronted, or otherwise bot-detected
+ // sites. By going through flags.newClient(), the doctor's
+ // reachability verdict matches what real commands experience.
if cfg != nil && cfg.BaseURL != "" {
- httpClient := &http.Client{Timeout: 5 * time.Second}
- baseURL := strings.TrimRight(cfg.BaseURL, "/")
-
- // Step 1: Basic reachability (unauthenticated HEAD)
- headReq, _ := http.NewRequest("HEAD", baseURL, nil)
- headResp, headErr := httpClient.Do(headReq)
- apiReachable := false
- if headErr == nil {
- headResp.Body.Close()
- apiReachable = true
- }
-
- // If HEAD failed, try GET on common health endpoints
- if !apiReachable {
- for _, p := range []string{"/health", "/healthz", "/status", "/ping", ""} {
- healthResp, healthErr := httpClient.Get(baseURL + p)
- if healthErr != nil {
- continue
+ c, clientErr := flags.newClient()
+ if clientErr != nil {
+ report["api"] = fmt.Sprintf("client init error: %s", clientErr)
+ } else {
+ // Step 1: Basic reachability via the configured transport.
+ reachBody, reachErr := c.Get("/", nil)
+ var reachAPIErr *client.APIError
+ switch {
+ case reachErr == nil:
+ // 2xx response — clearly reachable. Still inspect the
+ // body for a known interstitial; some bot walls return
+ // 200 with a JS challenge page.
+ if vendor := looksLikeDoctorInterstitial(reachBody); vendor != "" {
+ report["api"] = fmt.Sprintf("blocked by %s interstitial — the configured transport reached the wall. Try a different network, wait for the IP-level rate limit to clear, or check that the browser-chrome transport is bound correctly.", vendor)
+ } else {
+ report["api"] = "reachable"
}
- healthResp.Body.Close()
- apiReachable = true
- break
+ case errors.As(reachErr, &reachAPIErr):
+ // Non-2xx from the server. The network reached, the
+ // server responded — that's "reachable" for our
+ // purposes. Inspect the response body for a known
+ // interstitial first; otherwise note the status.
+ status := reachAPIErr.StatusCode
+ if vendor := looksLikeDoctorInterstitial([]byte(reachAPIErr.Body)); vendor != "" {
+ report["api"] = fmt.Sprintf("blocked by %s interstitial (HTTP %d) — the configured transport reached the wall.", vendor, status)
+ } else {
+ report["api"] = fmt.Sprintf("reachable (HTTP %d at /)", status)
+ }
+ default:
+ // Network-level failure: DNS, connection refused, TLS,
+ // transport init, etc. The transport itself didn't
+ // connect.
+ report["api"] = fmt.Sprintf("unreachable: %s", reachErr)
}
- }
- if !apiReachable {
- report["api"] = fmt.Sprintf("unreachable: %s", headErr)
- } else {
- report["api"] = "reachable"
- }
-
- // Step 2: Validate credentials with an authenticated request
- authHeader := cfg.AuthHeader()
- if authHeader == "" {
- // No auth configured — skip credential validation
+ // Step 2: Validate credentials with an authenticated probe.
+ authHeader := cfg.AuthHeader()
+ if authHeader == "" {
+ // No auth configured — skip credential validation
{{- if .Auth.RequiresBrowserSession}}
- } else if proofOK, proofDetail := browserSessionProofStatus(cfg, authHeader); proofOK {
- report["credentials"] = "valid"
- report["credentials_detail"] = proofDetail
- } else {
- report["credentials"] = "invalid (browser-session proof missing or stale)"
- report["credentials_detail"] = proofDetail
+ } else if proofOK, proofDetail := browserSessionProofStatus(cfg, authHeader); proofOK {
+ report["credentials"] = "valid"
+ report["credentials_detail"] = proofDetail
+ } else {
+ report["credentials"] = "invalid (browser-session proof missing or stale)"
+ report["credentials_detail"] = proofDetail
{{- else}}
- } else if !apiReachable {
- report["credentials"] = "skipped (API unreachable)"
- } else {
+ } else if reachErr != nil && !errors.As(reachErr, &reachAPIErr) {
+ report["credentials"] = "skipped (API unreachable)"
+ } else {
{{- if .Auth.VerifyPath}}
- verifyPath := "{{.Auth.VerifyPath}}"
- if !strings.HasPrefix(verifyPath, "/") {
- verifyPath = "/" + verifyPath
- }
- authReq, _ := http.NewRequest("GET", baseURL+verifyPath, nil)
+ verifyPath := "{{.Auth.VerifyPath}}"
+ if !strings.HasPrefix(verifyPath, "/") {
+ verifyPath = "/" + verifyPath
+ }
{{- else}}
- authReq, _ := http.NewRequest("GET", baseURL, nil)
+ verifyPath := "/"
{{- end}}
+ authParams := map[string]string{}
+ authHeaders := map[string]string{}
{{- if and .Auth .Auth.In (eq .Auth.In "query")}}
- q := authReq.URL.Query()
- q.Set("{{if .Auth.Header}}{{.Auth.Header}}{{else}}api_key{{end}}", authHeader)
- authReq.URL.RawQuery = q.Encode()
+ authParams["{{if .Auth.Header}}{{.Auth.Header}}{{else}}api_key{{end}}"] = authHeader
{{- else}}
- authReq.Header.Set("{{if .Auth.Header}}{{.Auth.Header}}{{else}}Authorization{{end}}", authHeader)
+ authHeaders["{{if .Auth.Header}}{{.Auth.Header}}{{else}}Authorization{{end}}"] = authHeader
{{- end}}
{{- range .RequiredHeaders}}
- authReq.Header.Set("{{.Name}}", "{{.Value}}")
+ authHeaders["{{.Name}}"] = "{{.Value}}"
{{- end}}
{{- if not .UsesBrowserManagedUserAgent}}
- authReq.Header.Set("User-Agent", "{{.Name}}-pp-cli")
+ authHeaders["User-Agent"] = "{{.Name}}-pp-cli"
{{- end}}
- authResp, authErr := httpClient.Do(authReq)
- if authErr != nil {
- report["credentials"] = "error: could not reach API"
- } else {
- authResp.Body.Close()
+ _, authErr := c.GetWithHeaders(verifyPath, authParams, authHeaders)
+ var authAPIErr *client.APIError
switch {
- case authResp.StatusCode >= 200 && authResp.StatusCode < 300:
+ case authErr == nil:
report["credentials"] = "valid"
- case authResp.StatusCode == 401 || authResp.StatusCode == 403:
+ case errors.As(authErr, &authAPIErr):
+ switch {
+ case authAPIErr.StatusCode == 401 || authAPIErr.StatusCode == 403:
{{- if .Auth.VerifyPath}}
- report["credentials"] = fmt.Sprintf("invalid (HTTP %d) — check your credentials", authResp.StatusCode)
+ report["credentials"] = fmt.Sprintf("invalid (HTTP %d) — check your credentials", authAPIErr.StatusCode)
{{- else}}
- // The probe hit the bare base URL because no auth.verify_path
- // is configured in the spec. Many APIs return 401/403 from a
- // bare versioned root regardless of token validity (the path
- // isn't routed but the gateway still demands credentials).
- // Don't claim invalid without certainty — set verify_path to
- // a known-good authenticated GET (e.g. /me, /v1/account, /user)
- // for a definitive verdict.
- report["credentials"] = fmt.Sprintf("inconclusive (HTTP %d from base URL — set auth.verify_path in spec for a definitive probe)", authResp.StatusCode)
+ // The probe hit the bare base URL because no auth.verify_path
+ // is configured in the spec. Many APIs return 401/403 from a
+ // bare versioned root regardless of token validity (the path
+ // isn't routed but the gateway still demands credentials).
+ // Don't claim invalid without certainty — set verify_path to
+ // a known-good authenticated GET (e.g. /me, /v1/account, /user)
+ // for a definitive verdict.
+ report["credentials"] = fmt.Sprintf("inconclusive (HTTP %d from base URL — set auth.verify_path in spec for a definitive probe)", authAPIErr.StatusCode)
{{- end}}
+ default:
+ // Non-auth HTTP error (404, 500, etc.) — don't blame credentials
+ report["credentials"] = fmt.Sprintf("ok (HTTP %d from %s, but auth was accepted)", authAPIErr.StatusCode, verifyPath)
+ }
default:
- // Non-auth HTTP error (404, 500, etc.) — don't blame credentials
- report["credentials"] = fmt.Sprintf("ok (HTTP %d from base URL, but auth was accepted)", authResp.StatusCode)
+ report["credentials"] = fmt.Sprintf("error: %s", authErr)
}
- }
{{- end}}
+ }
}
} else if cfg != nil && cfg.BaseURL == "" {
report["api"] = "not configured (set base_url in config file)"
diff --git a/internal/generator/templates/helpers.go.tmpl b/internal/generator/templates/helpers.go.tmpl
index 660cad72..7a8a7876 100644
--- a/internal/generator/templates/helpers.go.tmpl
+++ b/internal/generator/templates/helpers.go.tmpl
@@ -111,6 +111,28 @@ func apiErr(err error) error { return &cliError{code: 5, err: err} }
func configErr(err error) error { return &cliError{code: 10, err: err} }
func rateLimitErr(err error) error { return &cliError{code: 7, err: err} }
+// dryRunOK reports whether the command should short-circuit without doing any
+// real work because --dry-run was set. The verify pipeline probes hand-written
+// commands with --dry-run; commands that put validation in cobra's `Args:` or
+// `MarkFlagRequired` cannot reach a dry-run guard inside RunE because cobra
+// runs those checks before RunE. The verify-friendly pattern for hand-written
+// commands is:
+//
+// RunE: func(cmd *cobra.Command, args []string) error {
+// if len(args) == 0 {
+// return cmd.Help()
+// }
+// if dryRunOK(flags) {
+// return nil
+// }
+// // ... real work ...
+// }
+//
+// See SKILL.md "Phase 3: Build The GOAT" for the full pattern.
+func dryRunOK(flags *rootFlags) bool {
+ return flags != nil && flags.dryRun
+}
+
{{- if .HasDataLayer}}
// accessWarning describes an API access-denial that sync converts into a
diff --git a/internal/generator/templates/html_extract.go.tmpl b/internal/generator/templates/html_extract.go.tmpl
index 3493b7cb..f22d191c 100644
--- a/internal/generator/templates/html_extract.go.tmpl
+++ b/internal/generator/templates/html_extract.go.tmpl
@@ -31,11 +31,25 @@ type htmlExtractedPage struct {
}
type htmlLink struct {
- Rank int `json:"rank,omitempty"`
- Name string `json:"name,omitempty"`
- Text string `json:"text,omitempty"`
- URL string `json:"url,omitempty"`
- Slug string `json:"slug,omitempty"`
+ Rank int `json:"rank,omitempty"`
+ Name string `json:"name,omitempty"`
+ Text string `json:"text,omitempty"`
+ Image string `json:"image,omitempty"`
+ URL string `json:"url,omitempty"`
+ Slug string `json:"slug,omitempty"`
+}
+
+// htmlRawTextTags lists the four HTML5 elements whose content is parsed as
+// raw text (not as nested elements) per the HTML spec. Walking these as if
+// they were normal element subtrees leaks raw markup -- e.g., a `<noscript>`
+// fallback containing `<img src="...">` produces literal `<img>` text in the
+// anchor's nodeText. This map is used to skip those subtrees during link-mode
+// text extraction and image discovery.
+var htmlRawTextTags = map[string]struct{}{
+ "noscript": {},
+ "script": {},
+ "style": {},
+ "template": {},
}
func extractHTMLResponse(raw []byte, opts htmlExtractionOptions) (json.RawMessage, error) {
@@ -133,17 +147,25 @@ func extractHTMLLink(n *xhtml.Node, opts htmlExtractionOptions) (htmlLink, bool)
return htmlLink{}, false
}
- text := cleanHTMLText(nodeText(n))
+ // Use the suppression-aware walker for anchor text. Sites like Allrecipes,
+ // Food Network, and other Dotdash/Meredith properties wrap above-the-fold
+ // images in <noscript> for non-JS users; the HTML parser preserves
+ // noscript content as raw TextNode children, and a naive walk leaks
+ // "<img src=...>" markup into the result. Suppressing noscript/script/
+ // style/template subtrees in nodeTextSuppressing returns clean text.
+ text := cleanHTMLText(nodeTextSuppressing(n))
rank, name := splitRankedHTMLLinkText(text)
if name == "" {
name = text
}
+ image := normalizeHTMLURL(firstImageSrc(n, opts.BaseURL), opts.BaseURL)
return htmlLink{
- Rank: rank,
- Name: name,
- Text: text,
- URL: normalized,
- Slug: htmlLinkSlug(parsed.Path, opts.LinkPrefixes),
+ Rank: rank,
+ Name: name,
+ Text: text,
+ Image: image,
+ URL: normalized,
+ Slug: htmlLinkSlug(parsed.Path, opts.LinkPrefixes),
}, true
}
@@ -253,6 +275,134 @@ func nodeText(n *xhtml.Node) string {
return strings.Join(parts, " ")
}
+// nodeTextSuppressing walks n's descendants and concatenates TextNode content,
+// skipping subtrees rooted at noscript/script/style/template elements. The
+// HTML5 spec parses content of those four elements as raw text, so a normal
+// walk leaks their literal markup (e.g., the `<img src=...>` inside a
+// `<noscript>` fallback) into the output. Used by extractHTMLLink instead of
+// nodeText to keep result fields clean.
+func nodeTextSuppressing(n *xhtml.Node) string {
+ var parts []string
+ var walk func(*xhtml.Node)
+ walk = func(node *xhtml.Node) {
+ if node == nil {
+ return
+ }
+ if node.Type == xhtml.ElementNode {
+ if _, raw := htmlRawTextTags[strings.ToLower(node.Data)]; raw {
+ return
+ }
+ }
+ if node.Type == xhtml.TextNode {
+ parts = append(parts, node.Data)
+ }
+ for child := node.FirstChild; child != nil; child = child.NextSibling {
+ walk(child)
+ }
+ }
+ walk(n)
+ return strings.Join(parts, " ")
+}
+
+// firstImageSrc returns the URL of the first non-suppressed `<img>` inside
+// the given node's subtree, normalized against base.
+//
+// Source priority (highest first):
+//
+// 1. `data-src` — modern lazy-loading attribute; carries the real URL when
+// `src` is a placeholder (1x1 pixel, base64 transparent gif). Most
+// content-heavy sites that lazy-load (Pinterest, NYT Cooking, Food52)
+// follow this pattern, so `data-src` beats `src` when both are present.
+// 2. `data-srcset` / `srcset` — first comma-separated URL. Responsive image
+// hint lists; we take the first declared candidate for simplicity.
+// 3. `src` — final fallback for plain `<img>` tags without lazy-loading.
+//
+// Suppressed subtrees (noscript/script/style/template) are skipped, so an
+// `<img>` rendered for JS-disabled users does not get picked when an
+// above-the-fold rendered image exists.
+//
+// Limitation: returns the FIRST matching image in DOM order, with no quality
+// heuristic. Sites that put a small icon (badge, share button, profile pic)
+// before the hero image will have the icon URL surfaced. Acceptable for
+// recipe-card and product-card layouts where hero images come first;
+// document the limitation if a CLI's spec target uses an icon-first layout.
+//
+// Returns empty string when no image is found.
+func firstImageSrc(n *xhtml.Node, base string) string {
+ _ = base // base is used by the caller's normalizeHTMLURL wrapper, not here
+ if n == nil {
+ return ""
+ }
+ var found string
+ var walk func(*xhtml.Node)
+ walk = func(node *xhtml.Node) {
+ if found != "" || node == nil {
+ return
+ }
+ if node.Type == xhtml.ElementNode {
+ if _, raw := htmlRawTextTags[strings.ToLower(node.Data)]; raw {
+ return
+ }
+ if strings.EqualFold(node.Data, "img") {
+ // Priority 1: data-src (lazy-load real URL)
+ if src := strings.TrimSpace(attrValue(node, "data-src")); src != "" {
+ found = src
+ return
+ }
+ // Priority 2: data-srcset / srcset (first candidate)
+ if srcset := strings.TrimSpace(attrValue(node, "data-srcset")); srcset != "" {
+ if src := firstSrcsetURL(srcset); src != "" {
+ found = src
+ return
+ }
+ }
+ if srcset := strings.TrimSpace(attrValue(node, "srcset")); srcset != "" {
+ if src := firstSrcsetURL(srcset); src != "" {
+ found = src
+ return
+ }
+ }
+ // Priority 3: plain src (may be a placeholder, but the only
+ // signal available for non-lazy-loaded images)
+ if src := strings.TrimSpace(attrValue(node, "src")); src != "" {
+ found = src
+ return
+ }
+ }
+ }
+ for child := node.FirstChild; child != nil; child = child.NextSibling {
+ if found != "" {
+ return
+ }
+ walk(child)
+ }
+ }
+ walk(n)
+ return found
+}
+
+// firstSrcsetURL extracts the first URL from a srcset / data-srcset attribute.
+// Srcset format is comma-separated `URL DESCRIPTOR` pairs, e.g.:
+//
+// "https://x.com/a.jpg 1x, https://x.com/a@2x.jpg 2x"
+//
+// We take the first URL (first whitespace-separated token of the first
+// comma-separated entry). Returns empty string if the value is malformed.
+func firstSrcsetURL(srcset string) string {
+ if srcset == "" {
+ return ""
+ }
+ first := srcset
+ if i := strings.IndexByte(first, ','); i >= 0 {
+ first = first[:i]
+ }
+ first = strings.TrimSpace(first)
+ if i := strings.IndexAny(first, " \t"); i >= 0 {
+ first = first[:i]
+ }
+ return strings.TrimSpace(first)
+}
+
var htmlWhitespace = regexp.MustCompile(`\s+`)
var htmlRankedText = regexp.MustCompile(`^(\d+)[.)]\s+(.+)$`)
diff --git a/internal/generator/templates/root.go.tmpl b/internal/generator/templates/root.go.tmpl
index 2478c6d6..d0a62999 100644
--- a/internal/generator/templates/root.go.tmpl
+++ b/internal/generator/templates/root.go.tmpl
@@ -197,7 +197,9 @@ Run '{{.Name}}-pp-cli doctor' to verify auth and connectivity.{{backtick}},
{{- end}}
{{- end}}
rootCmd.AddCommand(newDoctorCmd(&flags))
+{{- if .HasAuthCommand}}
rootCmd.AddCommand(newAuthCmd(&flags))
+{{- end}}
rootCmd.AddCommand(newAgentContextCmd(rootCmd))
rootCmd.AddCommand(newProfileCmd(&flags))
rootCmd.AddCommand(newFeedbackCmd(&flags))
diff --git a/internal/pipeline/scorecard.go b/internal/pipeline/scorecard.go
index dd869334..a2d0c453 100644
--- a/internal/pipeline/scorecard.go
+++ b/internal/pipeline/scorecard.go
@@ -296,15 +296,34 @@ func scoreAuth(dir string) int {
configContent := readFileContent(filepath.Join(dir, "internal", "config", "config.go"))
authContent := readFileContent(filepath.Join(dir, "internal", "cli", "auth.go"))
clientContent := readFileContent(filepath.Join(dir, "internal", "client", "client.go"))
+
+ // No-auth exemption: when the generator decided this spec requires no
+ // auth surface, internal/cli/auth.go is not emitted. (Specs declaring
+ // auth.type: none with no AuthorizationURL and no graphql_persisted_query
+ // hint take this path; see Generator.shouldEmitAuth.) Penalizing such
+ // CLIs for "missing auth subcommand" or "no env vars" is wrong --
+ // they correctly reflect a no-auth spec. Award full credit and let the
+ // auth-bearing dimensions on auth-bearing CLIs speak for themselves.
+ //
+ // Note: readFileContent returns "" for both "file does not exist" and
+ // "file exists but is empty." We rely on the generator never emitting an
+ // empty auth.go -- every auth template (auth.go.tmpl, auth_simple.go.tmpl,
+ // auth_browser.go.tmpl) produces a non-empty file with at least the cobra
+ // command stub. If a future code path emits an empty auth.go as a
+ // placeholder, this exemption would fire incorrectly; the safer signal
+ // is os.Stat for file existence, but the empty-content check matches
+ // today's behavior with one fewer syscall.
+ if authContent == "" {
+ return 10
+ }
+
score := 0
// Presence: at least one env var
if strings.Count(configContent, "os.Getenv") >= 1 {
score += 2
}
- // Presence: auth file exists
- if authContent != "" {
- score += 1
- }
+ // Presence: auth file exists (we already returned above when it doesn't)
+ score += 1
// Quality: secure config file permissions (0o600 or 0600)
if strings.Contains(configContent, "0o600") || strings.Contains(configContent, "0600") || strings.Contains(configContent, "0o700") || strings.Contains(configContent, "0700") {
score += 2
diff --git a/skills/printing-press/SKILL.md b/skills/printing-press/SKILL.md
index db4c9b87..594f56e0 100644
--- a/skills/printing-press/SKILL.md
+++ b/skills/printing-press/SKILL.md
@@ -1611,7 +1611,7 @@ Priority 3 (polish):
### Agent Build Checklist (per command)
-After building each command in Priority 1 and Priority 2, verify these 7 principles are met. These map 1:1 to what Phase 4.9's agent readiness reviewer will check - apply them now so the review becomes a confirmation, not a catch-all.
+After building each command in Priority 1 and Priority 2, verify these 8 principles are met. These map 1:1 to what Phase 4.9's agent readiness reviewer will check - apply them now so the review becomes a confirmation, not a catch-all.
1. **Non-interactive**: No TTY prompts, no `bufio.Scanner(os.Stdin)`, works in CI without a terminal
2. **Structured output**: `--json` produces valid JSON, `--select` filters fields correctly
@@ -1620,6 +1620,25 @@ After building each command in Priority 1 and Priority 2, verify these 7 princip
5. **Safe retries**: Mutation commands support `--dry-run`, idempotent where possible
6. **Composability**: Exit codes are typed (0/2/3/4/5/7/10 as applicable), output pipes to `jq` cleanly
7. **Bounded responses**: `--compact` returns only high-gravity fields, list commands have `--limit`
+8. **Verify-friendly RunE**: Hand-written commands MUST NOT use `Args: cobra.MinimumNArgs(N)` or `MarkFlagRequired(...)`. Cobra evaluates both before RunE runs, so a `--dry-run` guard inside RunE cannot reach if those gates fail. Verify probes commands with `--dry-run` and expects exit 0; commands with hard arg/flag gates fail those probes. Instead: validate inside RunE, fall through to `cmd.Help()` for help-only invocations, and short-circuit on `dryRunOK(flags)` before any IO.
+
+#### Verify-friendly RunE template
+
+Use this shape for every hand-written transcendence command. The generator emits the `dryRunOK` helper into `internal/cli/helpers.go`:
+
+```go
+RunE: func(cmd *cobra.Command, args []string) error {
+ if len(args) == 0 {
+ return cmd.Help()
+ }
+ if dryRunOK(flags) {
+ return nil
+ }
+ // ... real work ...
+}
+```
+
+Why both checks: the `len(args) == 0` branch handles `<cli> mycommand --help` invocations gracefully; the `dryRunOK` branch handles verify's `<cli> mycommand <fixture> --dry-run` probes. Spec-derived commands generated by the Printing Press already follow this pattern -- this rule keeps hand-written novel-feature commands consistent with them.
### Phase 3 delegation: require feature-level acceptance
diff --git a/testdata/golden/expected/generate-golden-api/dogfood.json b/testdata/golden/expected/generate-golden-api/dogfood.json
index 941dfc83..fac528a0 100644
--- a/testdata/golden/expected/generate-golden-api/dogfood.json
+++ b/testdata/golden/expected/generate-golden-api/dogfood.json
@@ -19,7 +19,7 @@
"items": [
"wrapResultsWithFreshness"
],
- "total": 48
+ "total": 49
},
"dir": "<ARTIFACT_DIR>/generate-golden-api/printing-press-golden",
"example_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 d09a982d..1e418301 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
@@ -100,6 +100,28 @@ func apiErr(err error) error { return &cliError{code: 5, err: err} }
func configErr(err error) error { return &cliError{code: 10, err: err} }
func rateLimitErr(err error) error { return &cliError{code: 7, err: err} }
+// dryRunOK reports whether the command should short-circuit without doing any
+// real work because --dry-run was set. The verify pipeline probes hand-written
+// commands with --dry-run; commands that put validation in cobra's `Args:` or
+// `MarkFlagRequired` cannot reach a dry-run guard inside RunE because cobra
+// runs those checks before RunE. The verify-friendly pattern for hand-written
+// commands is:
+//
+// RunE: func(cmd *cobra.Command, args []string) error {
+// if len(args) == 0 {
+// return cmd.Help()
+// }
+// if dryRunOK(flags) {
+// return nil
+// }
+// // ... real work ...
+// }
+//
+// See SKILL.md "Phase 3: Build The GOAT" for the full pattern.
+func dryRunOK(flags *rootFlags) bool {
+ return flags != nil && flags.dryRun
+}
+
// accessWarning describes an API access-denial that sync converts into a
// non-fatal warning. It carries enough structured data for the sync_warning
// JSON event without parsing free-form error strings downstream.
← 8bad2efd fix(cli): score auth prefixes from config (#332)
·
back to Cli Printing Press
·
feat(cli): emit promoted-leaf paths in SKILL.md (#338) 4219e712 →