← back to Cli Printing Press
fix(cli): add source rate-limit guardrails (#366)
174d23c8fb3ab22e6c3cd2073a7fcd7cc2ddf40c · 2026-04-28 12:01:10 -0700 · Trevin Chow
Files touched
A docs/plans/2026-04-27-005-fix-printing-press-machine-fixes-from-company-goat-retro-plan.mdM internal/cli/verify_skill_bundled.pyM internal/cli/verify_skill_test.goM internal/generator/generator.goM internal/generator/generator_test.goM internal/generator/templates/client.go.tmplA internal/generator/templates/cliutil_ratelimit.go.tmplM internal/generator/templates/cliutil_test.go.tmplM internal/pipeline/dogfood.goA internal/pipeline/internal_packages.goM internal/pipeline/reimplementation_check.goA internal/pipeline/source_client_check.goA internal/pipeline/source_client_check_test.goM scripts/verify-skill/verify_skill.pyM skills/printing-press/SKILL.mdA skills/printing-press/references/per-source-rate-limiting.mdM testdata/golden/cases/generate-golden-api/artifacts.txtM testdata/golden/expected/dogfood-verdict-matrix/fail-path-auth-dead.jsonM testdata/golden/expected/dogfood-verdict-matrix/pass.jsonM testdata/golden/expected/dogfood-verdict-matrix/warn-priority.jsonM testdata/golden/expected/generate-golden-api/dogfood.jsonM testdata/golden/expected/generate-golden-api/printing-press-golden/internal/client/client.goA testdata/golden/expected/generate-golden-api/printing-press-golden/internal/cliutil/ratelimit.go
Diff
commit 174d23c8fb3ab22e6c3cd2073a7fcd7cc2ddf40c
Author: Trevin Chow <trevin@trevinchow.com>
Date: Tue Apr 28 12:01:10 2026 -0700
fix(cli): add source rate-limit guardrails (#366)
---
...s-machine-fixes-from-company-goat-retro-plan.md | 426 +++++++++++++++++++++
internal/cli/verify_skill_bundled.py | 139 +++++++
internal/cli/verify_skill_test.go | 218 ++++++++---
internal/generator/generator.go | 1 +
internal/generator/generator_test.go | 7 +-
internal/generator/templates/client.go.tmpl | 122 +-----
.../generator/templates/cliutil_ratelimit.go.tmpl | 170 ++++++++
internal/generator/templates/cliutil_test.go.tmpl | 187 +++++++++
internal/pipeline/dogfood.go | 12 +
internal/pipeline/internal_packages.go | 30 ++
internal/pipeline/reimplementation_check.go | 23 --
internal/pipeline/source_client_check.go | 131 +++++++
internal/pipeline/source_client_check_test.go | 299 +++++++++++++++
scripts/verify-skill/verify_skill.py | 139 +++++++
skills/printing-press/SKILL.md | 4 +
.../references/per-source-rate-limiting.md | 81 ++++
.../golden/cases/generate-golden-api/artifacts.txt | 1 +
.../fail-path-auth-dead.json | 4 +
.../expected/dogfood-verdict-matrix/pass.json | 4 +
.../dogfood-verdict-matrix/warn-priority.json | 4 +
.../expected/generate-golden-api/dogfood.json | 4 +
.../internal/client/client.go | 120 +-----
.../internal/cliutil/ratelimit.go | 170 ++++++++
23 files changed, 1985 insertions(+), 311 deletions(-)
diff --git a/docs/plans/2026-04-27-005-fix-printing-press-machine-fixes-from-company-goat-retro-plan.md b/docs/plans/2026-04-27-005-fix-printing-press-machine-fixes-from-company-goat-retro-plan.md
new file mode 100644
index 00000000..89eff5d2
--- /dev/null
+++ b/docs/plans/2026-04-27-005-fix-printing-press-machine-fixes-from-company-goat-retro-plan.md
@@ -0,0 +1,426 @@
+---
+title: Printing Press machine fixes from company-goat retro
+type: fix
+status: active
+date: 2026-04-27
+origin: PR mvanhorn/printing-press-library#140 follow-up commits 1fa0332 + 9a3b72d
+---
+
+# Printing Press machine fixes from company-goat retro
+
+## Overview
+
+The `company-goat-pp-cli` PR ([library #140](https://github.com/mvanhorn/printing-press-library/pull/140)) shipped with `feat(company-goat): add company-goat`, then required two follow-up commits to be usable:
+
+- `1fa0332` — `fix(company-goat): inline --domain/--pick flag declarations + use [co] in Use strings`
+- `9a3b72d` — `fix(company-goat): handle SEC throttling and agent output` (transplanted ~228 lines of rate-limiting machinery into `internal/source/sec/sec.go`)
+
+Both fixes were systemic, not SEC-specific. Each surfaced a machine gap that would recur on every future synthetic combo CLI. This plan addresses all seven gaps found in the analysis. The work splits into four landings; the first is the highest leverage and unblocks the static checks that catch the rest.
+
+- **U1: Promote rate-limit primitives to `internal/cliutil/ratelimit.go`.** The generated `internal/client/client.go` has a private `adaptiveLimiter` that hand-written sibling clients (`internal/source/<name>/`, `internal/recipes/`, `internal/phgraphql/`, etc.) cannot reuse. Without a shared exported primitive, agents either reinvent rate-limiting per source or skip it entirely — the latter is what happened to SEC EDGAR. Promote `AdaptiveLimiter`, `RateLimitError`, `RetryAfter`, and capped exponential `Backoff` into `cliutil`. Update `client.go.tmpl` to use the public types.
+- **U2: Source-aware reimplementation/throttle check.** `internal/pipeline/reimplementation_check.go` only inspects `internal/cli/*.go` — it never opens `internal/source/<name>/*.go` (or any sibling internal package whose `*.go` makes outbound HTTP calls). Extend the check to scan sibling internal packages and warn when a file makes outbound HTTP without using `cliutil.AdaptiveLimiter` or surfacing `*cliutil.RateLimitError`. After U1 lands, this is a string-match check.
+- **U3: verify-skill follows one level of helper indirection.** `internal/cli/verify_skill_bundled.py`'s `flag_declared_in` regex matches direct `cmd.Flags().StringVar(&x, "name", ...)` calls only. A combo CLI with 14 commands sharing a `--domain`/`--pick` flag pattern can't satisfy this without inlining the same declaration 14 times. Teach the verifier to follow one-level helper indirection: when the command file calls `addXxxFlags(cmd, &t)` and the helper's body declares the flag, count it as declared on the command.
+- **U4: SKILL Phase 3 + 4 rules.** Add Principle 10 to the Agent Build Checklist (every per-source HTTP client uses `cliutil.AdaptiveLimiter` and surfaces `RateLimitError`, never empty results). Add a sub-rule to Principle 8 (positional-OR-flag commands use `[brackets]` not `<angles>`). Add a per-source row to the Phase 4 dogfood matrix guidance (with limiter exhausted, command surfaces a typed error, not empty success).
+
+U1 must land before U2 (U2's static check string-matches `cliutil.AdaptiveLimiter`, which only exists after U1). U3 and U4 are independent. Recommended sequence: U1 → U2 → U3 → U4.
+
+---
+
+## Problem Frame
+
+The two follow-up commits in PR #140 surface seven distinct machine gaps. Verified diagnoses against `internal/` source at HEAD:
+
+### Gap A — `adaptiveLimiter` is private to `internal/client/`
+
+`internal/generator/templates/client.go.tmpl:46` declares `type adaptiveLimiter struct` (lowercase, unexported). The `Wait`, `OnSuccess`, `OnRateLimit`, `Rate` methods, the `newAdaptiveLimiter` constructor, and the `retryAfter` helper (line 804) are all unexported and live in `package client`. Hand-written sibling packages cannot import them.
+
+Concrete impact in PR #140: `internal/source/sec/sec.go` had `c.HTTP.Do(req)` with no limiter, no 429 retry, no `Retry-After` parsing. SEC throttling silently produced empty Form D results — identical in shape to "this company has no filings." The fix transplanted ~228 lines duplicating the limiter into the source package.
+
+Beyond company-goat: `recipe-goat/internal/recipes/archive.go:172` returns a plain `"CDX API rate limited (HTTP 429)"` error string with no retry; `producthunt/internal/phgraphql/client.go` likewise has no shared limiter import path. Every synthetic combo CLI hits this.
+
+### Gap B — `reimplementation_check` is `internal/cli`-shaped
+
+`internal/pipeline/reimplementation_check.go:142-181` only walks `internal/cli/*.go`. The `hasSiblingInternalImport` helper (added in retro #350) at line 382-393 detects whether a `cli/*.go` file *imports* a sibling internal package — but that import is treated as a positive signal that *vindicates* the command. The check never opens the sibling package itself to verify what's there.
+
+PR #140 documents this honestly: *"the dogfood reimplementation_check heuristic only inspects internal/client and produces 7/7 false positives on this CLI."* Result: 7 commands flagged as suspicious that were actually fine, while the actual problem (SEC source missing rate-limit handling) went undetected.
+
+### Gap C — `verify` never exercises 429 / throttling
+
+`grep -n "429\|RateLimit\|Retry-After\|throttl\|rate.limit" internal/pipeline/verify.go` returns zero matches. Live verify hits the real API once and PASSes if exit code is 0, which is exactly what SEC's "I rate-limited you and you swallowed it" path produces. The published proof file recorded `verify 100%` on a CLI whose killer feature returned empty results under load.
+
+After U1 lands, this becomes a static check: "the source file calls `cliutil.AdaptiveLimiter.Wait` and treats `*cliutil.RateLimitError` as a hard failure (not a `continue` in a loop, which is the exact pattern in the original `SearchAndFetchAll`)." We fold this into U2 rather than building 429 fault injection — the static check delivers the same protection for far less complexity.
+
+### Gap D — SKILL Phase 3 has no per-source-client rule
+
+The 9-principle Agent Build Checklist (`skills/printing-press/SKILL.md:1614-1633`) covers command-shape issues only — verify-friendly RunE, side-effect convention, `--json`, exit codes, `dryRunOK`. It says nothing about the shape of the per-source HTTP client itself. The "absence-of-correctness" delegation rule (line 1663) is the closest match but misses this case: empty results that *look* correct but actually mean throttling.
+
+### Gap E — Full-dogfood test matrix has no source-level row
+
+The agent who ran the full dogfood matrix on company-goat tested every command + `--json` + error paths but didn't ask "for each named source, when its limiter exhausts, is the output distinguishable from a legitimate empty?" That row doesn't exist in the matrix because Phase 4 derives rows from the command tree, not from data sources.
+
+### Gap F — verify-skill rejects shared flag helpers
+
+`internal/cli/verify_skill_bundled.py:79` declares `FLAG_DECL_RE = re.compile(r'(Persistent)?Flags\(\)\.(StringVar|...)P?\(&[^,]+,\s*"([a-z][a-z0-9-]*)"')`. The regex requires the flag to be declared by a literal `cmd.Flags().StringVar(&..., "name", ...)` call. When 14 commands share a `--domain`/`--pick` flag and use `addTargetFlags(cmd, &t)` (whose body has the literal `cmd.Flags().StringVar(...)`), the regex finds the declaration in `helpers.go` but `flag_declared_in(cmd_files, flag)` (line 600) only sees the command's own file and reports "declared elsewhere but not on \<path\>" (line 611).
+
+Fix in PR #140 was to inline 14 declarations — busywork driven entirely by the static analyzer's limit, not by an actual CLI bug.
+
+### Gap G — `Use:` string ergonomics for "positional OR flag"
+
+`Use: "snapshot <co>"` declared `<co>` as required, but the command actually accepts `<co>` OR `--domain` (and validates one of them inside RunE). `verify-skill`'s `check_positional_args` (line 625) parses `<co>` as required and flags `<cli> snapshot --domain example.com` recipes as missing positional args. Fix in PR #140 changed Use to `[co]` (optional). Any combo CLI that resolves an entity by either positional or flag hits the same trap.
+
+---
+
+## Architecture Decisions
+
+**Promote to `cliutil`, not a new top-level package.** The existing `internal/cliutil/` package is already the canonical home for cross-source helpers (`fanout.go`, `text.go`, `verifyenv.go`, `probe.go`). Adding `ratelimit.go` is the smallest change that doesn't introduce a new package. AGENTS.md already names cliutil as "the generator-owned Go package emitted into every printed CLI." This is the natural extension.
+
+**Public types, not factory functions.** `cliutil.NewAdaptiveLimiter(rate)` returns `*cliutil.AdaptiveLimiter` (exported struct, exported methods). `cliutil.RateLimitError` is an exported struct so callers can `errors.As`. `cliutil.RetryAfter(resp)` and `cliutil.Backoff(attempt)` are package-level helpers. No interfaces — the concrete struct is the contract.
+
+**Generated `client.go` keeps the same behavior.** After U1, `client.go.tmpl` switches from `*adaptiveLimiter` to `*cliutil.AdaptiveLimiter`. The behavior is byte-equivalent: same default rates (`floor` set by `RateLimit` from spec or fallback), same `OnRateLimit` halving, same `OnSuccess` ramp-up, same `Wait` semantics. Goldens regenerate but only for import + type-name changes.
+
+**Static check, not fault injection.** Gap C originally wanted verify to inject 429s. After U1 promotes the limiter to a public symbol, the static check "does this file call `cliutil.AdaptiveLimiter.Wait`" is sufficient — it forces every per-source client onto the shared retry path. Fault injection would have required mock-server infrastructure for every API and a way to declare "this source's primary endpoint is X." Static check is one regex.
+
+**verify-skill helper indirection: AST-light, regex-heavy.** Don't add a Go AST parser to the Python verifier. Instead, when a flag isn't declared directly in the command file but is referenced in a helper call (e.g., `addXxxFlags(cmd, ...)`), look up the helper by name in any `internal/cli/*.go` file and check whether its body contains a `Flags().StringVar(...)` call for the flag. One level of indirection only — no recursive lookup.
+
+**Phase 3 SKILL rules are short.** Principle 10 is one sentence + one code example. Principle 8's `[brackets]` sub-rule is one sentence. The Phase 4 matrix row is one bullet. Long prose is the wrong shape for build checklists; agents skim them.
+
+---
+
+## U1 — Promote rate-limit primitives to `internal/cliutil/ratelimit.go`
+
+### Goal
+
+Every printed CLI ships with `internal/cliutil/ratelimit.go` exporting `AdaptiveLimiter`, `RateLimitError`, `RetryAfter`, and `Backoff`. The generated `internal/client/client.go` uses these public types. Hand-written sibling packages can `import "<modulePath>/internal/cliutil"` and use the same primitives.
+
+### Files Changed
+
+**New file:**
+- `internal/generator/templates/cliutil_ratelimit.go.tmpl`
+
+**Modified:**
+- `internal/generator/templates/client.go.tmpl` — drop private `adaptiveLimiter` struct + methods + `retryAfter` helper; switch field type to `*cliutil.AdaptiveLimiter`; update call sites; add cliutil import.
+- `internal/generator/templates/cliutil_test.go.tmpl` — add tests for new types.
+- `internal/generator/generator.go:1015-1019` — register the new template in `renderSingleFiles`.
+- `testdata/golden/cases/generate-golden-api/artifacts.txt` — add `internal/cliutil/ratelimit.go`.
+- `testdata/golden/expected/generate-golden-api/printing-press-golden/internal/cliutil/ratelimit.go` — new fixture.
+- `testdata/golden/expected/generate-golden-api/printing-press-golden/internal/client/client.go` — regenerated fixture.
+
+### Implementation Sketch
+
+`cliutil_ratelimit.go.tmpl` exports:
+
+```go
+package cliutil
+
+// AdaptiveLimiter mirrors the limiter previously private to internal/client.
+// Hand-written sibling packages (internal/source/<name>/, internal/recipes/,
+// internal/phgraphql/, etc.) MUST use this primitive instead of rolling their
+// own — the printing-press dogfood reimplementation check enforces this.
+type AdaptiveLimiter struct { /* same fields as private adaptiveLimiter */ }
+
+func NewAdaptiveLimiter(ratePerSec float64) *AdaptiveLimiter
+func (l *AdaptiveLimiter) Wait()
+func (l *AdaptiveLimiter) OnSuccess()
+func (l *AdaptiveLimiter) OnRateLimit()
+func (l *AdaptiveLimiter) Rate() float64
+
+// RateLimitError signals that the upstream returned 429 after retries were
+// exhausted. Callers MUST surface this as a hard error — not as empty results
+// — because empty-on-throttle is indistinguishable from "no data exists" and
+// silently corrupts downstream queries (company-goat retro, PR #140).
+type RateLimitError struct {
+ URL string
+ RetryAfter time.Duration
+ Body string
+}
+func (e *RateLimitError) Error() string
+
+// RetryAfter parses an HTTP Retry-After header (seconds or HTTP-date) capped
+// at 60s. Returns 5s when the header is missing or malformed.
+func RetryAfter(resp *http.Response) time.Duration
+
+// Backoff returns a deterministic exponential-with-jitter wait for retry
+// attempt N (0-indexed). Capped at 30s to keep tests bounded.
+func Backoff(attempt int) time.Duration
+```
+
+`client.go.tmpl` changes:
+- Line 36: `limiter *adaptiveLimiter` → `limiter *cliutil.AdaptiveLimiter`.
+- Lines 42-125: delete the private `adaptiveLimiter` struct, methods, and `newAdaptiveLimiter` constructor — replaced by cliutil.
+- Lines 226, 236: `newAdaptiveLimiter(rateLimit)` → `cliutil.NewAdaptiveLimiter(rateLimit)`.
+- Lines 384, 479, 492, 494, 384: `c.limiter.Wait()` → unchanged (method moved, not renamed). Same for `OnSuccess`, `OnRateLimit`, `Rate`.
+- Line 493: `retryAfter(resp)` → `cliutil.RetryAfter(resp)`.
+- Lines 802-826: delete the private `retryAfter` helper + `maxRetryWait` constant — moved to cliutil.
+- Imports: add `"<modulePath>/internal/cliutil"`.
+
+### Tests
+
+In `cliutil_test.go.tmpl`, add cases:
+1. `TestAdaptiveLimiter_RampsUpAfterSuccesses` — call `OnSuccess()` rampAfter+1 times, assert `Rate()` increased by 1.25×.
+2. `TestAdaptiveLimiter_HalvesOnRateLimit` — set rate, call `OnRateLimit()`, assert rate is halved and ceiling stored.
+3. `TestAdaptiveLimiter_FloorAtZeroPointFive` — repeatedly call `OnRateLimit()`, assert rate never drops below 0.5.
+4. `TestRateLimitError_ErrorMessage` — verify the `Error()` string includes URL and RetryAfter.
+5. `TestRetryAfter_Seconds` — `Retry-After: 10` → 10s.
+6. `TestRetryAfter_HTTPDate` — `Retry-After: <future date>` → ~delta.
+7. `TestRetryAfter_Cap` — `Retry-After: 600` → capped at 60s.
+8. `TestRetryAfter_Missing` → 5s default.
+9. `TestBackoff_DoublesPerAttempt` — `Backoff(0)`=1s, `Backoff(1)`≈2s, etc., capped at 30s.
+
+Add a generator test in `internal/generator/client_test.go` (or extend an existing test):
+- `TestClientGo_ImportsCliutil` — render `client.go.tmpl` against a stock spec; assert the output imports `<modulePath>/internal/cliutil` and uses `*cliutil.AdaptiveLimiter`.
+
+### Golden Update
+
+Run `scripts/golden.sh update` after the changes. Expected diffs:
+- `printing-press-golden/internal/cliutil/ratelimit.go` — new file (~150 lines).
+- `printing-press-golden/internal/client/client.go` — import diff, type-name diffs, ~80 fewer lines (limiter + retryAfter removed).
+- `printing-press-golden/internal/cliutil/cliutil_test.go` — new test cases (only if the test fixture is part of artifacts.txt; check first).
+
+Inspect each diff manually. Document in the final summary which goldens changed and why.
+
+### Risks
+
+- **Existing printed CLIs in `~/printing-press/library/` will regenerate to a different shape on the next run.** That's expected and desired; they currently lack the limiter and shouldn't be the source of truth for the machine.
+- **The unexported `adaptiveLimiter` symbol disappears from `client.go`.** Any code outside the generator that references it would break — `grep` confirms no such code exists in this repo.
+- **Behavior parity is structural, not byte-equivalent.** Methods are identical but field zero-values may differ if the type is renamed. Verify with the `TestAdaptiveLimiter_*` tests.
+
+### Acceptance
+
+- `go test ./...` passes.
+- `scripts/golden.sh verify` passes (or update fixtures + explain).
+- A regenerated golden CLI's `internal/client/client.go` imports `cliutil` and references `*cliutil.AdaptiveLimiter`.
+- A regenerated golden CLI's `internal/cliutil/ratelimit.go` exists and contains the four exported symbols.
+
+---
+
+## U2 — Source-aware reimplementation/throttle check
+
+### Goal
+
+`reimplementation_check` extends to inspect sibling internal packages (`internal/source/`, `internal/recipes/`, `internal/phgraphql/`, etc. — anything not in the `reservedInternalPackages` set). For each `*.go` file in those packages that makes outbound HTTP calls, emit a finding when the file does not use `cliutil.AdaptiveLimiter` (Wait/OnSuccess/OnRateLimit) AND does not surface `*cliutil.RateLimitError`.
+
+### Files Changed
+
+**Modified:**
+- `internal/pipeline/reimplementation_check.go` — extend `ReimplementationCheckResult` with a `SiblingClients` field; add `checkSiblingClients` that walks non-reserved internal packages.
+- `internal/pipeline/dogfood.go` — render the new findings in the report; thread them through verdicts.
+- `internal/pipeline/dogfood_test.go` — fixture-based tests for the new check.
+
+### Implementation Sketch
+
+In `reimplementation_check.go`, add:
+
+```go
+type SiblingClientFinding struct {
+ Package string `json:"package"`
+ File string `json:"file"`
+ Reason string `json:"reason"` // "no rate limiter" | "swallows 429"
+}
+
+// httpCallRe matches outbound HTTP calls in any package shape.
+var httpCallRe = regexp.MustCompile(
+ `\b(http\.(Get|Post|NewRequest|Do)|c\.(Do|Get|Post)|HTTPClient\.Do|HTTP\.Do)\s*\(`,
+)
+// limiterUseRe — at least one of these must be present in a sibling package
+// that makes outbound HTTP calls.
+var limiterUseRe = regexp.MustCompile(`cliutil\.(AdaptiveLimiter|NewAdaptiveLimiter)\b|\.OnRateLimit\s*\(|\.OnSuccess\s*\(`)
+// rateLimitErrorRe — either the cliutil type or a local 429-handler that returns
+// a typed error (not a continue or a swallow).
+var rateLimitErrorRe = regexp.MustCompile(`cliutil\.RateLimitError\b|RateLimitError\b`)
+
+func checkSiblingClients(cliDir string) []SiblingClientFinding {
+ // Walk internal/<pkg>/*.go for pkg not in reservedInternalPackages and
+ // not 'cli' (already covered by command-level check).
+ // For each *.go that matches httpCallRe, require limiterUseRe AND
+ // rateLimitErrorRe. Emit a finding otherwise.
+}
+```
+
+Wire into `checkReimplementation`'s caller (`dogfood.go:260`). Add a verdict row that warns when `len(SiblingClients) > 0`. Match the existing pattern at `dogfood.go:1332`.
+
+### Tests
+
+`internal/pipeline/reimplementation_check_test.go`:
+1. `TestSiblingClient_PassesWithLimiter` — fixture with HTTP call + `cliutil.AdaptiveLimiter` + `cliutil.RateLimitError` returns no findings.
+2. `TestSiblingClient_FlagsMissingLimiter` — fixture with HTTP call + no limiter → one finding with `reason: "no rate limiter"`.
+3. `TestSiblingClient_FlagsSwallow429` — fixture with HTTP call + limiter but no `RateLimitError` propagation → one finding with `reason: "swallows 429"`.
+4. `TestSiblingClient_IgnoresReservedPackages` — adds a file in `internal/store/` (which is a reserved package); assert no finding.
+5. `TestSiblingClient_NoFileNoFinding` — no sibling internal packages → no findings.
+
+### Acceptance
+
+- `go test ./internal/pipeline/...` passes.
+- Running dogfood against company-goat at HEAD before the SEC fix (recreate the broken state in a fixture) emits one finding for `internal/source/sec/sec.go: no rate limiter`.
+- Running dogfood against company-goat *after* the SEC fix (current state, with the inlined limiter) emits zero findings — the check accepts both the cliutil import path *and* the local-copy pattern as legitimate, because the local copy still uses `RateLimitError` and a Wait-shaped limiter. (Long-term, after agents migrate to cliutil, the local-copy path becomes unused.)
+
+### Risks
+
+- **False positives** on packages that do non-HTTP work but happen to have an `http.Get` in a comment or a test fixture. Filter test files (`*_test.go`) and use a tight-enough regex.
+- **False negatives** on packages with novel HTTP shapes (e.g., a package that wraps `*http.Request` in another struct). Acceptable — the check is a heuristic, like the existing reimplementation check. Document the regex; new shapes can be added.
+- **Existing CLIs without limiters in their sibling packages** will start generating warnings. That is the goal — not a risk. Document in the U2 commit so users running `printing-press dogfood` against a 6-month-old CLI understand what changed.
+
+---
+
+## U3 — verify-skill follows one-level helper indirection
+
+### Goal
+
+`flag_declared_in(cmd_files, flag)` returns true when the flag is declared either directly in `cmd_files` OR by a helper called from `cmd_files` whose body declares it. One level only — no recursive resolution.
+
+### Files Changed
+
+**Modified:**
+- `internal/cli/verify_skill_bundled.py` (and the byte-identical `scripts/verify-skill/verify_skill.py`).
+- `internal/cli/verify_skill_test.go` — add test cases.
+
+### Implementation Sketch
+
+After the existing `flag_declared_in(cmd_files, flag)` call in `check_flag_commands` (line 600), add a fallback that:
+
+1. Scans `cmd_files` for calls matching `(\w+)\(\s*(?:cmd|cmd\b)`. Captures helper names invoked with `cmd` as first arg.
+2. For each helper name, finds the helper's body in any `internal/cli/*.go` file (defined as `func <name>(`).
+3. Runs `FLAG_DECL_RE` over the helper's body. If the flag matches, count it.
+
+Pseudocode:
+
+```python
+def flag_declared_via_helper(cli_dir, cmd_files, flag_name):
+ helper_call_re = re.compile(r'\b([a-zA-Z_]\w*)\s*\(\s*cmd\b')
+ helper_names = set()
+ for f in cmd_files:
+ for m in helper_call_re.finditer(f.read_text()):
+ helper_names.add(m.group(1))
+ if not helper_names:
+ return False
+ for go_file in (cli_dir / "internal/cli").glob("*.go"):
+ text = go_file.read_text()
+ for name in helper_names:
+ if not re.search(rf'\bfunc\s+{re.escape(name)}\s*\(', text):
+ continue
+ # Extract a generous window after the func declaration.
+ # We don't need to brace-match precisely; FLAG_DECL_RE on a
+ # 2KB window is enough to catch every realistic helper.
+ for m in re.finditer(rf'func\s+{re.escape(name)}\s*\([^)]*\)\s*\{{', text):
+ window = text[m.end():m.end()+2000]
+ for fm in FLAG_DECL_RE.finditer(window):
+ if fm.group(3) == flag_name:
+ return True
+ return False
+```
+
+Wire it in `check_flag_commands`:
+
+```python
+if cmd_files and flag_declared_in(cmd_files, flag):
+ continue
+if persistent_flag_declared(cli_dir, flag):
+ continue
+if flag_declared_via_helper(cli_dir, cmd_files, flag): # NEW
+ continue
+# ... existing finding emission
+```
+
+Sync the bundled and canonical scripts (the byte-identical-hash test in `internal/cli/release_test.go` will fail otherwise).
+
+### Tests
+
+In `internal/cli/verify_skill_test.go`:
+1. `TestVerifySkill_FlagDeclaredViaHelper_OneLevel` — fixture with `addTargetFlags(cmd, &t)` in command file and the helper body in `helpers.go` with `cmd.Flags().StringVar(&t.domain, "domain", ...)`. Assert no finding for `--domain`.
+2. `TestVerifySkill_FlagNotDeclaredAnywhere` — neither in command file nor helper. Assert one finding.
+3. `TestVerifySkill_HelperWithoutCmdArg` — helper called as `addThing(other)` (no `cmd`); should not be considered. Assert appropriate behavior (probably one finding if the flag is otherwise undeclared).
+
+### Acceptance
+
+- `go test ./internal/cli/...` passes.
+- The bundled and canonical script hashes match (`TestVerifySkillScriptInSync` passes).
+- Re-running `verify-skill` on company-goat *before* the inlining fix produces zero `flag-commands` findings for `--domain` and `--pick`.
+
+### Risks
+
+- **False positives** if a helper's body declares the flag for an unrelated command (e.g., `addRootFlags(cmd)` declaring `--root-only` but verify-skill thinks it applies to a leaf). The window-based approach can't tell. Mitigation: log the helper name in the verdict so a reviewer can confirm; keep the severity at `error` so the agent investigates.
+- **Window cutoff** — 2KB is enough for every helper observed in printed CLIs, but a contrived helper longer than that could escape detection. Acceptable.
+
+---
+
+## U4 — SKILL Phase 3 + Phase 4 rules
+
+### Goal
+
+Add three small rules to `skills/printing-press/SKILL.md`:
+
+1. **Principle 10** in the Agent Build Checklist: every per-source HTTP client uses `cliutil.AdaptiveLimiter` and surfaces `cliutil.RateLimitError`, never empty results.
+2. **Principle 8 sub-rule**: positional-OR-flag commands MUST use `[brackets]` not `<angles>`.
+3. **Phase 4 dogfood matrix row**: per source, verify that limiter exhaustion produces a typed error, not empty success.
+
+### Files Changed
+
+**Modified:**
+- `skills/printing-press/SKILL.md` — three small additions in the Agent Build Checklist (around line 1614) and the Phase 4 dogfood matrix guidance.
+
+### Implementation Sketch
+
+Add Principle 10 after Principle 9 (line 1633), short and direct:
+
+```
+10. **Per-source rate limiting**: any hand-written client in `internal/source/<name>/`,
+ `internal/recipes/`, or any other sibling internal package that makes outbound HTTP
+ calls MUST use `cliutil.AdaptiveLimiter` and return `*cliutil.RateLimitError` from
+ its public methods when 429 retries are exhausted. Returning empty results on
+ throttle is a silent corruption — downstream queries can't tell "the source has no
+ data" from "the source rate-limited us." Reference: company-goat retro, PR #140.
+```
+
+Add a sub-bullet under Principle 8 (line 1623), short:
+
+> Commands that accept either a positional `<x>` OR a flag `--y` as alternatives MUST declare `Use: "<cmd> [x]"` (square brackets, not angle brackets) and validate "exactly one of x or --y" inside RunE. Required positionals declared with angle brackets break verify-skill recipes that use the flag-only form.
+
+Add to the Phase 4 dogfood-matrix guidance (around line 1751 — `verify-skill` discussion). Rather than burying it there, add a new short subsection after "Full dogfood means everything":
+
+> **Per-source row for combo CLIs.** When a synthetic combo CLI lists N named data sources (`internal/source/<name>/`, `internal/recipes/`, etc.), the dogfood test matrix MUST add one row per source: with the limiter at floor rate and 429s injected (or the upstream genuinely throttling), assert that the user-facing command surfaces a typed error referencing the source — not empty JSON / `0 results`. A passing row says: "the CLI distinguishes 'no data' from 'we got rate-limited' for this source." Dogfood without this row passed company-goat with SEC EDGAR silently broken; PR #140 caught it only after publish.
+
+### Tests
+
+`skills/` doesn't have unit tests, but `internal/generator/skill_test.go` does render-time tests on the SKILL template. None of the changes here are template-driven (they're prose in the human-authored SKILL), so no Go test is required. Add a SKILL.md format check via `go vet` or markdown linter only if one already exists (it doesn't — confirmed by repo grep).
+
+### Acceptance
+
+- `git diff skills/printing-press/SKILL.md` shows three small additions in the right sections.
+- A future run on a synthetic combo CLI surfaces all three rules during Phase 3 review.
+
+### Risks
+
+- **SKILL bloat** — these add ~25 lines total. The existing checklist is already long; agents skim it. Mitigation: keep each rule to 1-3 sentences with an explicit reference back to the company-goat retro for context.
+
+---
+
+## Sequencing & Dependencies
+
+```
+U1 (cliutil/ratelimit) ───┬─→ U2 (source-aware check) ───→ ship
+ │
+ └─→ U4 (SKILL rules ref cliutil)
+
+U3 (verify-skill helper) ──────────────────────────────→ ship (independent)
+```
+
+- U2's static check string-matches `cliutil.AdaptiveLimiter` — so U1 must merge first.
+- U4's Principle 10 names `cliutil.AdaptiveLimiter` — same constraint.
+- U3 has no dependency on the others; can land first or last.
+
+Recommended PR order: U1 → U2 → U3 → U4. Each PR follows commit-style `fix(cli):` or `fix(skills):` per AGENTS.md.
+
+## Out of Scope
+
+- **Verify mode that injects 429s.** Originally Gap C; folded into U2 as a static check. Real fault injection requires per-API mock servers and a way to declare "this source's primary endpoint is X" in the spec. Out of scope for this plan; revisit if static-check false-negatives become a problem.
+- **Fixing existing published CLIs in `~/printing-press/library/`.** This plan touches the machine; published CLIs regenerate on the next run. Manual backports are user-driven, not machine-driven.
+- **Renaming `internal/source/` to a generator-recognized convention.** Today, sibling internal packages can be named anything (`source/`, `recipes/`, `phgraphql/`, `atom/`). U2's check is regex-based and works for all of them, so no rename is needed.
+- **Promoting more helpers to cliutil.** Only rate-limiting primitives are promoted in this plan. `cleanText`, `fanout`, `verifyenv`, `probe`, `freshness` are already there. If future retros find more shared shapes (e.g., HTML extraction), promote them in their own plans.
+
+## Acceptance — Plan-level
+
+- `go test ./...` passes after all four landings.
+- `scripts/golden.sh verify` passes (with U1's expected fixture updates).
+- A regenerated golden CLI shows `internal/cliutil/ratelimit.go` and a `client.go` that imports it.
+- Running dogfood on a synthetic CLI fixture without limiters in `internal/source/<x>/` emits a `SiblingClientFinding`.
+- `verify-skill` on a CLI with `addTargetFlags(cmd, &t)` indirection does not emit `flag-commands` findings.
+- SKILL.md contains Principles 8-sub, 10, and the per-source matrix row.
diff --git a/internal/cli/verify_skill_bundled.py b/internal/cli/verify_skill_bundled.py
index 915ee4c7..5835a43b 100755
--- a/internal/cli/verify_skill_bundled.py
+++ b/internal/cli/verify_skill_bundled.py
@@ -438,6 +438,143 @@ def flag_declared_in(files: Iterable[Path], flag_name: str) -> bool:
return False
+# Matches a function call whose first positional arg is `cmd` (or `cmd.Flags()`).
+# Captures the function name so the verifier can look up its body.
+HELPER_CALL_RE = re.compile(
+ r"\b([a-zA-Z_]\w*)\s*\(\s*cmd(?:\b|\.Flags\(\))"
+)
+
+# Cobra/stdlib methods that take cmd as first arg but never declare flags.
+_HELPER_CALL_IGNORE = frozenset({
+ "AddCommand", "Run", "Execute", "Help", "Usage",
+ "Print", "Printf", "Println",
+})
+
+
+def go_block_body(text: str, open_brace: int) -> str:
+ """Return the body for the Go block whose `{` starts at open_brace.
+
+ This is intentionally a small scanner rather than a Go parser. It handles
+ nested braces and skips comments/strings so adjacent helper functions do
+ not leak into the matched helper body.
+ """
+ if open_brace < 0 or open_brace >= len(text) or text[open_brace] != "{":
+ return ""
+
+ depth = 0
+ i = open_brace
+ state = "code"
+ while i < len(text):
+ ch = text[i]
+ nxt = text[i + 1] if i + 1 < len(text) else ""
+
+ if state == "line_comment":
+ if ch == "\n":
+ state = "code"
+ i += 1
+ continue
+ if state == "block_comment":
+ if ch == "*" and nxt == "/":
+ state = "code"
+ i += 2
+ else:
+ i += 1
+ continue
+ if state == "double_string":
+ if ch == "\\":
+ i += 2
+ continue
+ if ch == '"':
+ state = "code"
+ i += 1
+ continue
+ if state == "raw_string":
+ if ch == "`":
+ state = "code"
+ i += 1
+ continue
+ if state == "rune":
+ if ch == "\\":
+ i += 2
+ continue
+ if ch == "'":
+ state = "code"
+ i += 1
+ continue
+
+ if ch == "/" and nxt == "/":
+ state = "line_comment"
+ i += 2
+ continue
+ if ch == "/" and nxt == "*":
+ state = "block_comment"
+ i += 2
+ continue
+ if ch == '"':
+ state = "double_string"
+ i += 1
+ continue
+ if ch == "`":
+ state = "raw_string"
+ i += 1
+ continue
+ if ch == "'":
+ state = "rune"
+ i += 1
+ continue
+
+ if ch == "{":
+ depth += 1
+ elif ch == "}":
+ depth -= 1
+ if depth == 0:
+ return text[open_brace + 1:i]
+ i += 1
+
+ return ""
+
+
+def flag_declared_via_helper(cli_dir: Path, cmd_files: Iterable[Path], flag_name: str) -> bool:
+ """Return True if any helper called from cmd_files (with cmd as first arg)
+ declares flag_name in its body. One level of indirection only — no recursive
+ resolution."""
+ helper_names: set[str] = set()
+ for f in cmd_files:
+ try:
+ text = f.read_text()
+ except Exception:
+ continue
+ for m in HELPER_CALL_RE.finditer(text):
+ name = m.group(1)
+ if name not in _HELPER_CALL_IGNORE:
+ helper_names.add(name)
+ if not helper_names:
+ return False
+
+ src = cli_dir / "internal/cli"
+ if not src.exists():
+ return False
+
+ func_re = re.compile(
+ r"func\s+("
+ + "|".join(re.escape(n) for n in helper_names)
+ + r")\s*\([^)]*\)\s*(?:\w+\s*)?\{"
+ )
+ for go_file in src.glob("*.go"):
+ if go_file.name.endswith("_test.go"):
+ continue
+ try:
+ text = go_file.read_text()
+ except Exception:
+ continue
+ for m in func_re.finditer(text):
+ body = go_block_body(text, m.end() - 1)
+ for fm in FLAG_DECL_RE.finditer(body):
+ if fm.group(3) == flag_name:
+ return True
+ return False
+
+
def persistent_flag_declared(cli_dir: Path, flag_name: str) -> bool:
src = cli_dir / "internal/cli"
if not src.exists():
@@ -601,6 +738,8 @@ def check_flag_commands(cli_dir: Path, skill: Path, cli_binary: str, report: Rep
continue
if persistent_flag_declared(cli_dir, flag):
continue
+ if cmd_files and flag_declared_via_helper(cli_dir, cmd_files, flag):
+ continue
path_str = " ".join(cmd_path)
if flag_declared_in(all_files, flag):
report.findings.append(
diff --git a/internal/cli/verify_skill_test.go b/internal/cli/verify_skill_test.go
index 25966649..5c7de6f5 100644
--- a/internal/cli/verify_skill_test.go
+++ b/internal/cli/verify_skill_test.go
@@ -22,10 +22,19 @@ func TestVerifySkill_DetectsWrongFlagOnCommand(t *testing.T) {
bin := buildPrintingPressBinary(t)
dir := t.TempDir()
- require.NoError(t, os.MkdirAll(filepath.Join(dir, "internal", "cli"), 0o755))
+ skill := `---
+name: pp-fixture
+description: "fixture"
+---
+
+# Fixture
- // Minimal source: search (without --max-time) and tonight (with --max-time)
- require.NoError(t, os.WriteFile(filepath.Join(dir, "internal", "cli", "search.go"), []byte(`package cli
+` + "```bash" + `
+fixture-pp-cli search "chicken" --max-time 30m
+` + "```" + `
+`
+ writeVerifySkillFixture(t, dir, map[string]string{
+ "search.go": `package cli
import "github.com/spf13/cobra"
func newSearchCmd() *cobra.Command {
var limit int
@@ -33,8 +42,8 @@ func newSearchCmd() *cobra.Command {
cmd.Flags().IntVar(&limit, "limit", 10, "Max results")
return cmd
}
-`), 0o644))
- require.NoError(t, os.WriteFile(filepath.Join(dir, "internal", "cli", "tonight.go"), []byte(`package cli
+`,
+ "tonight.go": `package cli
import (
"github.com/spf13/cobra"
"time"
@@ -45,22 +54,8 @@ func newTonightCmd() *cobra.Command {
cmd.Flags().DurationVar(&maxTime, "max-time", 0, "Max total time")
return cmd
}
-`), 0o644))
-
- // SKILL claims search --max-time (the bug).
- skill := `---
-name: pp-fixture
-description: "fixture"
----
-
-# Fixture
-
-` + "```bash" + `
-fixture-pp-cli search "chicken" --max-time 30m
-` + "```" + `
-`
- require.NoError(t, os.WriteFile(filepath.Join(dir, "SKILL.md"), []byte(skill), 0o644))
- require.NoError(t, os.WriteFile(filepath.Join(dir, ".printing-press.json"), []byte(`{"cli_name":"fixture-pp-cli"}`), 0o644))
+`,
+ }, skill)
out, err := exec.Command(bin, "verify-skill", "--dir", dir).CombinedOutput()
require.Error(t, err, "verifier must exit non-zero for a SKILL with an undeclared flag")
@@ -88,12 +83,10 @@ func TestVerifySkill_NoFalsePositiveOnSharedLeafName(t *testing.T) {
bin := buildPrintingPressBinary(t)
dir := t.TempDir()
- require.NoError(t, os.MkdirAll(filepath.Join(dir, "internal", "cli"), 0o755))
- // root.go wires both save and profile into rootCmd. The verifier
- // must follow rootCmd.AddCommand calls to know that cmd_path=['save']
- // resolves to save_cmd.go (not profile.go's profile-save subcommand).
- require.NoError(t, os.WriteFile(filepath.Join(dir, "internal", "cli", "root.go"), []byte(`package cli
+ skill := "---\nname: pp-fixture\n---\n\n# Fixture\n\n```bash\nfixture-pp-cli save https://example.com --tags foo,bar\n```\n"
+ writeVerifySkillFixture(t, dir, map[string]string{
+ "root.go": `package cli
import "github.com/spf13/cobra"
func Execute() error {
rootCmd := &cobra.Command{Use: "fixture-pp-cli"}
@@ -101,12 +94,8 @@ func Execute() error {
rootCmd.AddCommand(newProfileCmd())
return rootCmd.Execute()
}
-`), 0o644))
-
- // Top-level save: declares --tags. Use string is intentionally
- // short so the legacy specificity heuristic would lose the tie-break
- // against profile.go's longer Use string.
- require.NoError(t, os.WriteFile(filepath.Join(dir, "internal", "cli", "save_cmd.go"), []byte(`package cli
+`,
+ "save_cmd.go": `package cli
import "github.com/spf13/cobra"
func newSaveCmd() *cobra.Command {
var tags string
@@ -114,12 +103,8 @@ func newSaveCmd() *cobra.Command {
cmd.Flags().StringVar(&tags, "tags", "", "Comma-separated tags")
return cmd
}
-`), 0o644))
-
- // profile save: declares its own flags. Use string is more specific
- // (more positionals + variadic), which would win the legacy
- // tie-break and drop save_cmd.go from the flag check.
- require.NoError(t, os.WriteFile(filepath.Join(dir, "internal", "cli", "profile.go"), []byte(`package cli
+`,
+ "profile.go": `package cli
import "github.com/spf13/cobra"
func newProfileCmd() *cobra.Command {
cmd := &cobra.Command{Use: "profile"}
@@ -132,14 +117,8 @@ func newProfileSaveCmd() *cobra.Command {
cmd.Flags().StringVar(&label, "label", "", "Profile label")
return cmd
}
-`), 0o644))
-
- // SKILL uses --tags on the top-level save command. The graph walk
- // must resolve to save_cmd.go (which declares --tags) and not
- // profile.go (which declares --label).
- skill := "---\nname: pp-fixture\n---\n\n# Fixture\n\n```bash\nfixture-pp-cli save https://example.com --tags foo,bar\n```\n"
- require.NoError(t, os.WriteFile(filepath.Join(dir, "SKILL.md"), []byte(skill), 0o644))
- require.NoError(t, os.WriteFile(filepath.Join(dir, ".printing-press.json"), []byte(`{"cli_name":"fixture-pp-cli"}`), 0o644))
+`,
+ }, skill)
out, err := exec.Command(bin, "verify-skill", "--dir", dir).CombinedOutput()
require.NoError(t, err, "verifier must NOT raise findings for valid shared-leaf usage; got: %s", string(out))
@@ -154,8 +133,10 @@ func TestVerifySkill_PassesWhenSkillMatches(t *testing.T) {
bin := buildPrintingPressBinary(t)
dir := t.TempDir()
- require.NoError(t, os.MkdirAll(filepath.Join(dir, "internal", "cli"), 0o755))
- require.NoError(t, os.WriteFile(filepath.Join(dir, "internal", "cli", "search.go"), []byte(`package cli
+
+ skill := "---\nname: pp-fixture\n---\n\n# Fixture\n\n```bash\nfixture-pp-cli search \"chicken\" --limit 5\n```\n"
+ writeVerifySkillFixture(t, dir, map[string]string{
+ "search.go": `package cli
import "github.com/spf13/cobra"
func newSearchCmd() *cobra.Command {
var limit int
@@ -163,16 +144,138 @@ func newSearchCmd() *cobra.Command {
cmd.Flags().IntVar(&limit, "limit", 10, "Max results")
return cmd
}
-`), 0o644))
- skill := "---\nname: pp-fixture\n---\n\n# Fixture\n\n```bash\nfixture-pp-cli search \"chicken\" --limit 5\n```\n"
- require.NoError(t, os.WriteFile(filepath.Join(dir, "SKILL.md"), []byte(skill), 0o644))
- require.NoError(t, os.WriteFile(filepath.Join(dir, ".printing-press.json"), []byte(`{"cli_name":"fixture-pp-cli"}`), 0o644))
+`,
+ }, skill)
out, err := exec.Command(bin, "verify-skill", "--dir", dir).CombinedOutput()
require.NoError(t, err, "clean SKILL should exit 0, got: %s", string(out))
require.Contains(t, string(out), "All checks passed")
}
+// TestVerifySkill_FlagDeclaredViaHelper asserts the verifier accepts a flag
+// declared one level deep through a shared helper invoked with cmd as first
+// arg, e.g. addTargetFlags(cmd, &t) whose body declares the flag.
+func TestVerifySkill_FlagDeclaredViaHelper(t *testing.T) {
+ bin := buildPrintingPressBinary(t)
+ dir := t.TempDir()
+
+ skill := "---\nname: pp-fixture\n---\n\n# Fixture\n\n```bash\nfixture-pp-cli snapshot --domain example.com\n```\n"
+ writeVerifySkillFixture(t, dir, map[string]string{
+ "snapshot.go": `package cli
+import "github.com/spf13/cobra"
+type targetFlags struct{ domain, pick string }
+func newSnapshotCmd() *cobra.Command {
+ var t targetFlags
+ cmd := &cobra.Command{Use: "snapshot [co]"}
+ addTargetFlags(cmd, &t)
+ return cmd
+}
+`,
+ "helpers.go": `package cli
+import "github.com/spf13/cobra"
+func addTargetFlags(cmd *cobra.Command, t *targetFlags) {
+ cmd.Flags().StringVar(&t.domain, "domain", "", "Domain")
+ cmd.Flags().StringVar(&t.pick, "pick", "", "Pick which source")
+}
+`,
+ "root.go": `package cli
+import "github.com/spf13/cobra"
+func Execute() error {
+ rootCmd := &cobra.Command{Use: "fixture-pp-cli"}
+ rootCmd.AddCommand(newSnapshotCmd())
+ return rootCmd.Execute()
+}
+`,
+ }, skill)
+
+ out, err := exec.Command(bin, "verify-skill", "--dir", dir).CombinedOutput()
+ require.NoError(t, err, "verifier must accept flags declared via one level of helper indirection; got: %s", string(out))
+ require.Contains(t, string(out), "All checks passed")
+}
+
+// TestVerifySkill_FlagHelperDoesNotScanAdjacentFunctions confirms helper
+// matching is limited to the called helper's body. A later helper in the same
+// file must not make addTargetFlags look like it declares --pick.
+func TestVerifySkill_FlagHelperDoesNotScanAdjacentFunctions(t *testing.T) {
+ bin := buildPrintingPressBinary(t)
+ dir := t.TempDir()
+
+ skill := "---\nname: pp-fixture\n---\n\n# Fixture\n\n```bash\nfixture-pp-cli snapshot --pick sec\n```\n"
+ writeVerifySkillFixture(t, dir, map[string]string{
+ "snapshot.go": `package cli
+import "github.com/spf13/cobra"
+func newSnapshotCmd() *cobra.Command {
+ cmd := &cobra.Command{Use: "snapshot [co]"}
+ addTargetFlags(cmd)
+ return cmd
+}
+`,
+ "helpers.go": `package cli
+import "github.com/spf13/cobra"
+func addTargetFlags(cmd *cobra.Command) {
+ var domain string
+ cmd.Flags().StringVar(&domain, "domain", "", "Domain")
+}
+
+func addPickFlags(cmd *cobra.Command) {
+ var pick string
+ cmd.Flags().StringVar(&pick, "pick", "", "Pick which source")
+}
+`,
+ "root.go": `package cli
+import "github.com/spf13/cobra"
+func Execute() error {
+ rootCmd := &cobra.Command{Use: "fixture-pp-cli"}
+ rootCmd.AddCommand(newSnapshotCmd())
+ return rootCmd.Execute()
+}
+`,
+ }, skill)
+
+ out, err := exec.Command(bin, "verify-skill", "--dir", dir).CombinedOutput()
+ require.Error(t, err, "verifier must not treat adjacent helper declarations as part of addTargetFlags")
+ require.Contains(t, string(out), "--pick")
+}
+
+// TestVerifySkill_FlagNotDeclaredAnywhere confirms the helper-indirection
+// fallback does not cover for a flag that genuinely isn't declared.
+func TestVerifySkill_FlagNotDeclaredAnywhere(t *testing.T) {
+ bin := buildPrintingPressBinary(t)
+ dir := t.TempDir()
+
+ // SKILL claims --pick which is not declared anywhere.
+ skill := "---\nname: pp-fixture\n---\n\n# Fixture\n\n```bash\nfixture-pp-cli snapshot --pick sec\n```\n"
+ writeVerifySkillFixture(t, dir, map[string]string{
+ "snapshot.go": `package cli
+import "github.com/spf13/cobra"
+func newSnapshotCmd() *cobra.Command {
+ cmd := &cobra.Command{Use: "snapshot [co]"}
+ addTargetFlags(cmd)
+ return cmd
+}
+`,
+ "helpers.go": `package cli
+import "github.com/spf13/cobra"
+func addTargetFlags(cmd *cobra.Command) {
+ var x string
+ cmd.Flags().StringVar(&x, "domain", "", "Domain")
+}
+`,
+ "root.go": `package cli
+import "github.com/spf13/cobra"
+func Execute() error {
+ rootCmd := &cobra.Command{Use: "fixture-pp-cli"}
+ rootCmd.AddCommand(newSnapshotCmd())
+ return rootCmd.Execute()
+}
+`,
+ }, skill)
+
+ out, err := exec.Command(bin, "verify-skill", "--dir", dir).CombinedOutput()
+ require.Error(t, err, "undeclared flag must still produce a finding")
+ require.Contains(t, string(out), "--pick")
+}
+
// TestVerifySkill_RejectsMissingInputs confirms usage errors (code 2).
func TestVerifySkill_RejectsMissingInputs(t *testing.T) {
t.Parallel()
@@ -190,6 +293,17 @@ func TestVerifySkill_RejectsMissingInputs(t *testing.T) {
require.True(t, strings.Contains(string(out), "no SKILL.md") || strings.Contains(string(out), "no internal/cli"))
}
+func writeVerifySkillFixture(t *testing.T, dir string, files map[string]string, skill string) {
+ t.Helper()
+ cliDir := filepath.Join(dir, "internal", "cli")
+ require.NoError(t, os.MkdirAll(cliDir, 0o755))
+ for name, content := range files {
+ require.NoError(t, os.WriteFile(filepath.Join(cliDir, name), []byte(content), 0o644))
+ }
+ require.NoError(t, os.WriteFile(filepath.Join(dir, "SKILL.md"), []byte(skill), 0o644))
+ require.NoError(t, os.WriteFile(filepath.Join(dir, ".printing-press.json"), []byte(`{"cli_name":"fixture-pp-cli"}`), 0o644))
+}
+
// buildPrintingPressBinary compiles the printing-press binary into a test
// tempdir and returns its path. Built once per test because each test's
// TempDir is fresh; Go's test cache ensures the compile is fast.
diff --git a/internal/generator/generator.go b/internal/generator/generator.go
index f0405299..cb577194 100644
--- a/internal/generator/generator.go
+++ b/internal/generator/generator.go
@@ -1014,6 +1014,7 @@ func (g *Generator) renderSingleFiles() error {
"cliutil_fanout.go.tmpl": filepath.Join("internal", "cliutil", "fanout.go"),
"cliutil_text.go.tmpl": filepath.Join("internal", "cliutil", "text.go"),
"cliutil_probe.go.tmpl": filepath.Join("internal", "cliutil", "probe.go"),
+ "cliutil_ratelimit.go.tmpl": filepath.Join("internal", "cliutil", "ratelimit.go"),
"cliutil_verifyenv.go.tmpl": filepath.Join("internal", "cliutil", "verifyenv.go"),
"cliutil_test.go.tmpl": filepath.Join("internal", "cliutil", "cliutil_test.go"),
"types.go.tmpl": filepath.Join("internal", "types", "types.go"),
diff --git a/internal/generator/generator_test.go b/internal/generator/generator_test.go
index 40cbe0da..d6e2023a 100644
--- a/internal/generator/generator_test.go
+++ b/internal/generator/generator_test.go
@@ -50,6 +50,7 @@ func TestGenerateProjectsCompile(t *testing.T) {
"internal/cliutil/fanout.go",
"internal/cliutil/text.go",
"internal/cliutil/probe.go",
+ "internal/cliutil/ratelimit.go",
"internal/cliutil/verifyenv.go",
"internal/cliutil/cliutil_test.go",
"internal/client/client.go",
@@ -65,9 +66,9 @@ func TestGenerateProjectsCompile(t *testing.T) {
// Bump it AND add to mustInclude above when adding always-emitted
// templates. Per-spec dynamic files (per-resource command files,
// generated tests) account for the difference between fixtures.
- {name: "stytch", specPath: filepath.Join("..", "..", "testdata", "stytch.yaml"), expectedFiles: 46},
- {name: "clerk", specPath: filepath.Join("..", "..", "testdata", "clerk.yaml"), expectedFiles: 51},
- {name: "loops", specPath: filepath.Join("..", "..", "testdata", "loops.yaml"), expectedFiles: 48},
+ {name: "stytch", specPath: filepath.Join("..", "..", "testdata", "stytch.yaml"), expectedFiles: 47},
+ {name: "clerk", specPath: filepath.Join("..", "..", "testdata", "clerk.yaml"), expectedFiles: 52},
+ {name: "loops", specPath: filepath.Join("..", "..", "testdata", "loops.yaml"), expectedFiles: 49},
}
for _, tt := range tests {
diff --git a/internal/generator/templates/client.go.tmpl b/internal/generator/templates/client.go.tmpl
index 0554ccf8..65441183 100644
--- a/internal/generator/templates/client.go.tmpl
+++ b/internal/generator/templates/client.go.tmpl
@@ -15,14 +15,13 @@ import (
"net/url"
"os"
"path/filepath"
- "strconv"
"strings"
- "sync"
"time"
{{- if .UsesBrowserHTTPTransport}}
"github.com/enetx/surf"
{{ end}}
+ "{{modulePath}}/internal/cliutil"
"{{modulePath}}/internal/config"
)
@@ -33,97 +32,12 @@ type Client struct {
DryRun bool
NoCache bool
cacheDir string
- limiter *adaptiveLimiter
+ limiter *cliutil.AdaptiveLimiter
{{- if eq .Auth.Type "session_handshake"}}
Session *SessionManager
{{- end}}
}
-// adaptiveLimiter provides proactive rate limiting with adaptive ceiling discovery.
-// Starts at a conservative floor rate and ramps up after consecutive successes.
-// On 429, halves the rate and records a ceiling. Per-session only — not persisted.
-// Thread-safe: all methods are guarded by a mutex for concurrent sync workers.
-type adaptiveLimiter struct {
- mu sync.Mutex
- rate float64 // current requests per second
- floor float64 // starting/minimum rate
- ceiling float64 // discovered ceiling (0 = unknown)
- successes int // consecutive successes since last 429
- rampAfter int // successes needed before increasing rate
- lastRequest time.Time // zero-value on init — first call skips wait (intentional)
-}
-
-func newAdaptiveLimiter(ratePerSec float64) *adaptiveLimiter {
- if ratePerSec <= 0 {
- return nil
- }
- return &adaptiveLimiter{
- rate: ratePerSec,
- floor: ratePerSec,
- rampAfter: 10,
- }
-}
-
-// Wait blocks until the rate limiter allows the next request.
-func (l *adaptiveLimiter) Wait() {
- if l == nil {
- return
- }
- l.mu.Lock()
- delay := time.Duration(float64(time.Second) / l.rate)
- elapsed := time.Since(l.lastRequest)
- l.mu.Unlock()
- if elapsed < delay {
- time.Sleep(delay - elapsed)
- }
- l.mu.Lock()
- l.lastRequest = time.Now()
- l.mu.Unlock()
-}
-
-// OnSuccess records a successful request and ramps up the rate after enough consecutive successes.
-func (l *adaptiveLimiter) OnSuccess() {
- if l == nil {
- return
- }
- l.mu.Lock()
- defer l.mu.Unlock()
- l.successes++
- if l.successes >= l.rampAfter {
- newRate := l.rate * 1.25
- if l.ceiling > 0 && newRate > l.ceiling*0.9 {
- newRate = l.ceiling * 0.9
- }
- l.rate = newRate
- l.successes = 0
- }
-}
-
-// OnRateLimit records a 429 response — halves the rate and discovers the ceiling.
-func (l *adaptiveLimiter) OnRateLimit() {
- if l == nil {
- return
- }
- l.mu.Lock()
- defer l.mu.Unlock()
- l.ceiling = l.rate
- l.rate = l.rate / 2
- if l.rate < 0.5 {
- l.rate = 0.5 // absolute minimum: 1 request per 2 seconds
- }
- l.successes = 0
-}
-
-// Rate returns the current rate in requests per second. Returns 0 if the limiter is nil.
-func (l *adaptiveLimiter) Rate() float64 {
- if l == nil {
- return 0
- }
- l.mu.Lock()
- defer l.mu.Unlock()
- return l.rate
-}
-
{{if eq .ClientPattern "proxy-envelope"}}
// proxyEnvelope is the JSON wrapper sent to the proxy endpoint.
type proxyEnvelope struct {
@@ -223,7 +137,7 @@ func New(cfg *config.Config, timeout time.Duration, rateLimit float64) *Client {
Config: cfg,
HTTPClient: httpClient,
cacheDir: cacheDir,
- limiter: newAdaptiveLimiter(rateLimit),
+ limiter: cliutil.NewAdaptiveLimiter(rateLimit),
Session: sess,
}
{{- else}}
@@ -233,7 +147,7 @@ func New(cfg *config.Config, timeout time.Duration, rateLimit float64) *Client {
Config: cfg,
HTTPClient: httpClient,
cacheDir: cacheDir,
- limiter: newAdaptiveLimiter(rateLimit),
+ limiter: cliutil.NewAdaptiveLimiter(rateLimit),
}
{{- end}}
}
@@ -490,7 +404,7 @@ func (c *Client) do(method, path string, params map[string]string, body any, hea
// Rate limited - adjust adaptive limiter and retry
if resp.StatusCode == 429 && attempt < maxRetries {
c.limiter.OnRateLimit()
- wait := retryAfter(resp)
+ wait := cliutil.RetryAfter(resp)
fmt.Fprintf(os.Stderr, "rate limited, waiting %s (attempt %d/%d, rate adjusted to %.1f req/s)\n", wait, attempt+1, maxRetries, c.limiter.Rate())
time.Sleep(wait)
lastErr = apiErr
@@ -799,32 +713,6 @@ func (c *Client) refreshAccessToken() error {
return nil
}
-const maxRetryWait = 60 * time.Second
-
-func retryAfter(resp *http.Response) time.Duration {
- header := resp.Header.Get("Retry-After")
- if header == "" {
- return 5 * time.Second
- }
- if seconds, err := strconv.Atoi(header); err == nil {
- d := time.Duration(seconds) * time.Second
- if d > maxRetryWait {
- return maxRetryWait
- }
- return d
- }
- if t, err := http.ParseTime(header); err == nil {
- wait := time.Until(t)
- if wait > maxRetryWait {
- return maxRetryWait
- }
- if wait > 0 {
- return wait
- }
- }
- return 5 * time.Second
-}
-
// sanitizeJSONResponse strips known JSONP/XSSI prefixes and UTF-8 BOM from
// response bodies so that downstream JSON parsing succeeds. For clean JSON
// responses these checks are no-ops.
diff --git a/internal/generator/templates/cliutil_ratelimit.go.tmpl b/internal/generator/templates/cliutil_ratelimit.go.tmpl
new file mode 100644
index 00000000..e6de7d15
--- /dev/null
+++ b/internal/generator/templates/cliutil_ratelimit.go.tmpl
@@ -0,0 +1,170 @@
+// Copyright {{currentYear}} {{.Owner}}. Licensed under Apache-2.0. See LICENSE.
+// Generated by CLI Printing Press (https://github.com/mvanhorn/cli-printing-press). DO NOT EDIT.
+
+package cliutil
+
+import (
+ "fmt"
+ "math"
+ "net/http"
+ "strconv"
+ "strings"
+ "sync"
+ "time"
+)
+
+// AdaptiveLimiter paces outbound requests with adaptive ceiling discovery.
+// Starts at a floor rate, ramps up after consecutive successes, halves on 429
+// and records a ceiling. Per-session only — not persisted. Methods are safe
+// to call on a nil receiver.
+type AdaptiveLimiter struct {
+ mu sync.Mutex
+ rate float64
+ floor float64
+ ceiling float64
+ successes int
+ rampAfter int
+ lastRequest time.Time // zero-value: first Wait() returns immediately
+}
+
+// NewAdaptiveLimiter returns a limiter starting at ratePerSec, or nil when
+// rate-limiting should be disabled. Methods on the nil limiter no-op.
+func NewAdaptiveLimiter(ratePerSec float64) *AdaptiveLimiter {
+ if ratePerSec <= 0 {
+ return nil
+ }
+ return &AdaptiveLimiter{
+ rate: ratePerSec,
+ floor: ratePerSec,
+ rampAfter: 10,
+ }
+}
+
+func (l *AdaptiveLimiter) Wait() {
+ if l == nil {
+ return
+ }
+ l.mu.Lock()
+ delay := time.Duration(float64(time.Second) / l.rate)
+ elapsed := time.Since(l.lastRequest)
+ l.mu.Unlock()
+ if elapsed < delay {
+ time.Sleep(delay - elapsed)
+ }
+ l.mu.Lock()
+ l.lastRequest = time.Now()
+ l.mu.Unlock()
+}
+
+func (l *AdaptiveLimiter) OnSuccess() {
+ if l == nil {
+ return
+ }
+ l.mu.Lock()
+ defer l.mu.Unlock()
+ l.successes++
+ if l.successes >= l.rampAfter {
+ newRate := l.rate * 1.25
+ if l.ceiling > 0 && newRate > l.ceiling*0.9 {
+ newRate = l.ceiling * 0.9
+ }
+ l.rate = newRate
+ l.successes = 0
+ }
+}
+
+func (l *AdaptiveLimiter) OnRateLimit() {
+ if l == nil {
+ return
+ }
+ l.mu.Lock()
+ defer l.mu.Unlock()
+ l.ceiling = l.rate
+ l.rate = l.rate / 2
+ if l.rate < 0.5 {
+ l.rate = 0.5
+ }
+ l.successes = 0
+}
+
+func (l *AdaptiveLimiter) Rate() float64 {
+ if l == nil {
+ return 0
+ }
+ l.mu.Lock()
+ defer l.mu.Unlock()
+ return l.rate
+}
+
+// RateLimitError signals an upstream returned 429 after retries were
+// exhausted. Callers must surface this as a hard error rather than empty
+// results — empty-on-throttle is indistinguishable from "no data exists"
+// and silently corrupts downstream queries.
+type RateLimitError struct {
+ URL string
+ RetryAfter time.Duration
+ Body string
+}
+
+func (e *RateLimitError) Error() string {
+ msg := fmt.Sprintf("rate limited: HTTP 429 for %s", e.URL)
+ if e.RetryAfter > 0 {
+ msg += fmt.Sprintf("; retry after %s", e.RetryAfter)
+ }
+ if body := strings.TrimSpace(e.Body); body != "" {
+ msg += ": " + body
+ }
+ return msg
+}
+
+// MaxRetryWait caps the wait derived from a Retry-After header so a buggy
+// or hostile upstream cannot pin a CLI for hours.
+const MaxRetryWait = 60 * time.Second
+
+// RetryAfter parses an HTTP Retry-After header (RFC 7231: delta-seconds or
+// HTTP-date), capped at MaxRetryWait. Returns 5s when missing or unparseable.
+func RetryAfter(resp *http.Response) time.Duration {
+ if resp == nil {
+ return 5 * time.Second
+ }
+ header := strings.TrimSpace(resp.Header.Get("Retry-After"))
+ if header == "" {
+ return 5 * time.Second
+ }
+ if seconds, err := strconv.Atoi(header); err == nil {
+ d := time.Duration(seconds) * time.Second
+ if d > MaxRetryWait {
+ return MaxRetryWait
+ }
+ if d <= 0 {
+ return 5 * time.Second
+ }
+ return d
+ }
+ if t, err := http.ParseTime(header); err == nil {
+ wait := time.Until(t)
+ if wait > MaxRetryWait {
+ return MaxRetryWait
+ }
+ if wait > 0 {
+ return wait
+ }
+ }
+ return 5 * time.Second
+}
+
+// MaxBackoff caps Backoff so tests stay bounded. Callers needing jitter
+// add their own; the bare exponential keeps the contract deterministic.
+const MaxBackoff = 30 * time.Second
+
+// Backoff returns 2^attempt seconds capped at MaxBackoff.
+func Backoff(attempt int) time.Duration {
+ if attempt < 0 {
+ attempt = 0
+ }
+ wait := time.Duration(math.Pow(2, float64(attempt))) * time.Second
+ if wait > MaxBackoff {
+ return MaxBackoff
+ }
+ return wait
+}
diff --git a/internal/generator/templates/cliutil_test.go.tmpl b/internal/generator/templates/cliutil_test.go.tmpl
index 91b78c45..a2d3d253 100644
--- a/internal/generator/templates/cliutil_test.go.tmpl
+++ b/internal/generator/templates/cliutil_test.go.tmpl
@@ -626,3 +626,190 @@ func TestProbeReachable_SendsRangeHeader(t *testing.T) {
t.Errorf("Range header: want %q, got %q", "bytes=0-1023", receivedRange)
}
}
+
+// ---- AdaptiveLimiter / RateLimitError / RetryAfter / Backoff ----
+
+func TestAdaptiveLimiter_NewNilOnNonPositive(t *testing.T) {
+ if NewAdaptiveLimiter(0) != nil {
+ t.Fatal("NewAdaptiveLimiter(0) should return nil")
+ }
+ if NewAdaptiveLimiter(-1) != nil {
+ t.Fatal("NewAdaptiveLimiter(-1) should return nil")
+ }
+}
+
+func TestAdaptiveLimiter_NilSafeMethods(t *testing.T) {
+ var l *AdaptiveLimiter
+ l.Wait()
+ l.OnSuccess()
+ l.OnRateLimit()
+ if got := l.Rate(); got != 0 {
+ t.Errorf("nil limiter Rate() = %v, want 0", got)
+ }
+}
+
+func TestAdaptiveLimiter_RampsUpAfterSuccesses(t *testing.T) {
+ l := NewAdaptiveLimiter(2.0)
+ startRate := l.Rate()
+ for i := 0; i < l.rampAfter; i++ {
+ l.OnSuccess()
+ }
+ if got := l.Rate(); got <= startRate {
+ t.Errorf("Rate() after rampAfter successes = %v, want > %v", got, startRate)
+ }
+}
+
+func TestAdaptiveLimiter_HalvesOnRateLimit(t *testing.T) {
+ l := NewAdaptiveLimiter(8.0)
+ startRate := l.Rate()
+ l.OnRateLimit()
+ got := l.Rate()
+ if got != startRate/2 {
+ t.Errorf("Rate() after OnRateLimit = %v, want %v", got, startRate/2)
+ }
+}
+
+func TestAdaptiveLimiter_FloorsAtHalfRPS(t *testing.T) {
+ l := NewAdaptiveLimiter(2.0)
+ for i := 0; i < 10; i++ {
+ l.OnRateLimit()
+ }
+ if got := l.Rate(); got < 0.5 {
+ t.Errorf("Rate() after many OnRateLimit = %v, want >= 0.5", got)
+ }
+}
+
+func TestAdaptiveLimiter_WaitEnforcesPacing(t *testing.T) {
+ l := NewAdaptiveLimiter(10.0)
+ l.Wait()
+ start := time.Now()
+ l.Wait()
+ elapsed := time.Since(start)
+ if elapsed < 80*time.Millisecond {
+ t.Errorf("second Wait() took %v, want >= 80ms", elapsed)
+ }
+}
+
+func TestRateLimitError_ErrorMessage(t *testing.T) {
+ cases := []struct {
+ name string
+ err *RateLimitError
+ want string
+ }{
+ {
+ name: "with retry-after and body",
+ err: &RateLimitError{URL: "https://api.example.com/x", RetryAfter: 5 * time.Second, Body: "slow down"},
+ want: "rate limited: HTTP 429 for https://api.example.com/x; retry after 5s: slow down",
+ },
+ {
+ name: "with retry-after no body",
+ err: &RateLimitError{URL: "https://api.example.com/x", RetryAfter: 2 * time.Second},
+ want: "rate limited: HTTP 429 for https://api.example.com/x; retry after 2s",
+ },
+ {
+ name: "no retry-after with body",
+ err: &RateLimitError{URL: "https://api.example.com/x", Body: "later"},
+ want: "rate limited: HTTP 429 for https://api.example.com/x: later",
+ },
+ {
+ name: "no retry-after no body",
+ err: &RateLimitError{URL: "https://api.example.com/x"},
+ want: "rate limited: HTTP 429 for https://api.example.com/x",
+ },
+ }
+ for _, tc := range cases {
+ t.Run(tc.name, func(t *testing.T) {
+ if got := tc.err.Error(); got != tc.want {
+ t.Errorf("Error() = %q, want %q", got, tc.want)
+ }
+ })
+ }
+}
+
+func TestRateLimitError_ErrorsAs(t *testing.T) {
+ var err error = &RateLimitError{URL: "https://x", RetryAfter: time.Second}
+ var target *RateLimitError
+ if !errors.As(err, &target) {
+ t.Fatal("errors.As should match *RateLimitError")
+ }
+ if target.URL != "https://x" {
+ t.Errorf("target.URL = %q, want %q", target.URL, "https://x")
+ }
+}
+
+func TestRetryAfter_Seconds(t *testing.T) {
+ resp := &http.Response{Header: http.Header{}}
+ resp.Header.Set("Retry-After", "10")
+ if got := RetryAfter(resp); got != 10*time.Second {
+ t.Errorf("RetryAfter(10) = %v, want 10s", got)
+ }
+}
+
+func TestRetryAfter_HTTPDate(t *testing.T) {
+ future := time.Now().Add(7 * time.Second).UTC().Format(http.TimeFormat)
+ resp := &http.Response{Header: http.Header{}}
+ resp.Header.Set("Retry-After", future)
+ got := RetryAfter(resp)
+ if got < 5*time.Second || got > 8*time.Second {
+ t.Errorf("RetryAfter(http-date 7s ahead) = %v, want ~7s", got)
+ }
+}
+
+func TestRetryAfter_Cap(t *testing.T) {
+ resp := &http.Response{Header: http.Header{}}
+ resp.Header.Set("Retry-After", "600")
+ if got := RetryAfter(resp); got != MaxRetryWait {
+ t.Errorf("RetryAfter(600) = %v, want capped at %v", got, MaxRetryWait)
+ }
+}
+
+func TestRetryAfter_Missing(t *testing.T) {
+ resp := &http.Response{Header: http.Header{}}
+ if got := RetryAfter(resp); got != 5*time.Second {
+ t.Errorf("RetryAfter(missing) = %v, want 5s default", got)
+ }
+}
+
+func TestRetryAfter_MalformedFallsBackToDefault(t *testing.T) {
+ resp := &http.Response{Header: http.Header{}}
+ resp.Header.Set("Retry-After", "not-a-number")
+ if got := RetryAfter(resp); got != 5*time.Second {
+ t.Errorf("RetryAfter(garbage) = %v, want 5s default", got)
+ }
+}
+
+func TestRetryAfter_NilResp(t *testing.T) {
+ if got := RetryAfter(nil); got != 5*time.Second {
+ t.Errorf("RetryAfter(nil) = %v, want 5s default", got)
+ }
+}
+
+func TestBackoff_DoublesPerAttempt(t *testing.T) {
+ cases := []struct {
+ attempt int
+ want time.Duration
+ }{
+ {0, 1 * time.Second},
+ {1, 2 * time.Second},
+ {2, 4 * time.Second},
+ {3, 8 * time.Second},
+ {4, 16 * time.Second},
+ }
+ for _, tc := range cases {
+ if got := Backoff(tc.attempt); got != tc.want {
+ t.Errorf("Backoff(%d) = %v, want %v", tc.attempt, got, tc.want)
+ }
+ }
+}
+
+func TestBackoff_CapsAtMax(t *testing.T) {
+ if got := Backoff(20); got != MaxBackoff {
+ t.Errorf("Backoff(20) = %v, want capped at %v", got, MaxBackoff)
+ }
+}
+
+func TestBackoff_NegativeAttemptClampsToZero(t *testing.T) {
+ if got := Backoff(-3); got != 1*time.Second {
+ t.Errorf("Backoff(-3) = %v, want 1s (clamped to 0)", got)
+ }
+}
diff --git a/internal/pipeline/dogfood.go b/internal/pipeline/dogfood.go
index 40e01351..8983794d 100644
--- a/internal/pipeline/dogfood.go
+++ b/internal/pipeline/dogfood.go
@@ -33,6 +33,7 @@ type DogfoodReport struct {
WiringCheck WiringCheckResult `json:"wiring_check"`
NovelFeaturesCheck NovelFeaturesCheckResult `json:"novel_features_check"`
ReimplementationCheck ReimplementationCheckResult `json:"reimplementation_check"`
+ SourceClientCheck SourceClientCheckResult `json:"source_client_check"`
TestPresence TestPresenceResult `json:"test_presence"`
NamingCheck NamingCheckResult `json:"naming_check"`
Issues []string `json:"issues"`
@@ -258,6 +259,7 @@ func RunDogfood(dir, specPath string, opts ...DogfoodOption) (*DogfoodReport, er
report.WiringCheck = checkWiring(dir)
report.NovelFeaturesCheck = checkNovelFeatures(dir, cfg.researchDir)
report.ReimplementationCheck = checkReimplementation(dir, cfg.researchDir)
+ report.SourceClientCheck = checkSourceClients(dir)
report.TestPresence = checkTestPresence(dir)
report.NamingCheck = checkNamingConsistency(dir)
report.Issues = collectDogfoodIssues(report, spec != nil)
@@ -1330,6 +1332,7 @@ var dogfoodVerdictRules = []dogfoodVerdictRule{
{"WARN", func(r *DogfoodReport, _ bool) bool { return len(r.NovelFeaturesCheck.Missing) > 0 }},
// Surface hand-rolled responses without hard-blocking early iteration.
{"WARN", func(r *DogfoodReport, _ bool) bool { return len(r.ReimplementationCheck.Suspicious) > 0 }},
+ {"WARN", func(r *DogfoodReport, _ bool) bool { return len(r.SourceClientCheck.Findings) > 0 }},
}
func deriveDogfoodVerdict(report *DogfoodReport, hasSpec bool) string {
@@ -1403,6 +1406,15 @@ func collectDogfoodIssues(report *DogfoodReport, hasSpec bool) []string {
report.ReimplementationCheck.Checked,
strings.Join(parts, "; ")))
}
+ if len(report.SourceClientCheck.Findings) > 0 {
+ parts := make([]string, 0, len(report.SourceClientCheck.Findings))
+ for _, f := range report.SourceClientCheck.Findings {
+ parts = append(parts, fmt.Sprintf("%s — %s", f.File, f.Reason))
+ }
+ issues = append(issues, fmt.Sprintf("%d source client file(s) without rate-limit handling: %s",
+ len(report.SourceClientCheck.Findings),
+ strings.Join(parts, "; ")))
+ }
if len(report.TestPresence.MissingTests) > 0 {
issues = append(issues, fmt.Sprintf("pure-logic packages with no tests: %s",
strings.Join(report.TestPresence.MissingTests, ", ")))
diff --git a/internal/pipeline/internal_packages.go b/internal/pipeline/internal_packages.go
new file mode 100644
index 00000000..b39e25f2
--- /dev/null
+++ b/internal/pipeline/internal_packages.go
@@ -0,0 +1,30 @@
+package pipeline
+
+import "regexp"
+
+// reservedInternalPackages names the internal packages the generator emits
+// unconditionally. Files under any of these are not agent-authored, so dogfood
+// checks that look for hand-rolled API behavior must skip them.
+var reservedInternalPackages = map[string]bool{
+ "client": true,
+ "store": true,
+ "cliutil": true,
+ "cache": true,
+ "config": true,
+ "mcp": true,
+ "types": true,
+ "share": true,
+ "deliver": true,
+ "profile": true,
+ "feedback": true,
+ "graphql": true,
+}
+
+// outboundHTTPCallRe matches every outbound HTTP request shape that appears in
+// generated and agent-authored Go code. Centralized so reimplementation_check
+// (per-command) and source_client_check (per-sibling-package) cannot diverge.
+var outboundHTTPCallRe = regexp.MustCompile(
+ `\bhttp\.(?:Get|Post|NewRequest(?:WithContext)?|Do)\s*\(|` +
+ `\b\w+\.HTTPClient\.Do\s*\(|` +
+ `\b\w+\.HTTP\.Do\s*\(|` +
+ `\bc\.(?:Do|Get|Post)\s*\(`)
diff --git a/internal/pipeline/reimplementation_check.go b/internal/pipeline/reimplementation_check.go
index c62efdd1..dce797f9 100644
--- a/internal/pipeline/reimplementation_check.go
+++ b/internal/pipeline/reimplementation_check.go
@@ -352,29 +352,6 @@ func hasClientSignal(content string) bool {
hasSiblingInternalImport(content)
}
-// reservedInternalPackages is the set of internal package names the
-// generator emits unconditionally. An import of any of these is NOT a
-// signal of a hand-built API client — it just means the file uses a
-// generator-emitted helper. Anything else under `internal/<name>` is
-// presumed to be agent-authored API client code (e.g. `internal/algolia`
-// for a CLI that fronts both Firebase and Algolia).
-//
-// Surfaced by hackernews retro #350 finding F4.
-var reservedInternalPackages = map[string]bool{
- "client": true,
- "store": true,
- "cliutil": true,
- "cache": true,
- "config": true,
- "mcp": true,
- "types": true,
- "share": true,
- "deliver": true,
- "profile": true,
- "feedback": true,
- "graphql": true,
-}
-
// hasSiblingInternalImport reports whether the file imports a non-reserved
// `internal/<name>` package — the signal for a hand-built secondary
// client. The regex matches all internal imports; we filter the
diff --git a/internal/pipeline/source_client_check.go b/internal/pipeline/source_client_check.go
new file mode 100644
index 00000000..bb464eca
--- /dev/null
+++ b/internal/pipeline/source_client_check.go
@@ -0,0 +1,131 @@
+package pipeline
+
+import (
+ "os"
+ "path/filepath"
+ "regexp"
+ "strings"
+)
+
+// SourceClientCheckResult flags hand-written sibling internal packages that
+// make outbound HTTP calls without using a rate limiter or surfacing a typed
+// 429 error. Empty-on-throttle is indistinguishable from "no data exists" and
+// silently corrupts downstream queries.
+type SourceClientCheckResult struct {
+ Checked int `json:"checked"`
+ Findings []SourceClientFinding `json:"findings,omitempty"`
+ Skipped bool `json:"skipped,omitempty"`
+}
+
+type SourceClientFinding struct {
+ // Package is the path under internal/ containing the offending file
+ // (e.g. "source/sec"), so a finding self-locates without needing to
+ // reconcile the Package and File fields.
+ Package string `json:"package"`
+ File string `json:"file"`
+ Reason string `json:"reason"`
+}
+
+// limiterUseRe matches an invocation or construction of any limiter — the
+// cliutil type, the constructor, or any of the lifecycle methods. Restricted
+// to actual call sites so a struct field or unrelated identifier ending in
+// "Limiter" does not satisfy the check.
+var limiterUseRe = regexp.MustCompile(
+ `cliutil\.(?:AdaptiveLimiter|NewAdaptiveLimiter)\b|` +
+ `\.OnRateLimit\s*\(\s*\)|` +
+ `\.OnSuccess\s*\(\s*\)|` +
+ `\.Wait\s*\(\s*\)`)
+
+// rateLimitErrorRe matches the typed-error contract: cliutil.RateLimitError,
+// any locally-named RateLimitError type, or an explicit 429 status check
+// whose then-branch the caller can route to a typed error path.
+var rateLimitErrorRe = regexp.MustCompile(
+ `cliutil\.RateLimitError\b|` +
+ `\bRateLimitError\b|` +
+ `StatusCode\s*==\s*(?:http\.StatusTooManyRequests|429)`)
+
+// sourceCheckExtraSkip is the set of packages source_client_check skips that
+// are not in reservedInternalPackages. internal/cli has its own treatment in
+// reimplementation_check, so source_client_check leaves it alone.
+var sourceCheckExtraSkip = map[string]bool{"cli": true}
+
+// checkSourceClients walks internal/<pkg>/*.go for every <pkg> not covered by
+// other dogfood checks and not generator-emitted. For each file that makes
+// outbound HTTP calls, it requires both a limiter signal and a typed-error
+// signal; missing either produces a finding.
+func checkSourceClients(cliDir string) SourceClientCheckResult {
+ internalDir := filepath.Join(cliDir, "internal")
+ entries, err := os.ReadDir(internalDir)
+ if err != nil {
+ return SourceClientCheckResult{Skipped: true}
+ }
+
+ result := SourceClientCheckResult{}
+ for _, pkgEntry := range entries {
+ if !pkgEntry.IsDir() {
+ continue
+ }
+ pkgName := pkgEntry.Name()
+ if reservedInternalPackages[pkgName] || sourceCheckExtraSkip[pkgName] {
+ continue
+ }
+ walkSourcePackage(cliDir, filepath.Join(internalDir, pkgName), &result)
+ }
+
+ if result.Checked == 0 && len(result.Findings) == 0 {
+ result.Skipped = true
+ }
+ return result
+}
+
+func walkSourcePackage(cliDir, pkgDir string, result *SourceClientCheckResult) {
+ _ = filepath.WalkDir(pkgDir, func(path string, d os.DirEntry, walkErr error) error {
+ if walkErr != nil {
+ return nil
+ }
+ if d.IsDir() {
+ name := d.Name()
+ if name == "testdata" || name == "vendor" || strings.HasPrefix(name, ".") {
+ return filepath.SkipDir
+ }
+ return nil
+ }
+ if !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") {
+ return nil
+ }
+
+ data, readErr := os.ReadFile(path)
+ if readErr != nil {
+ return nil
+ }
+ content := string(data)
+ result.Checked++
+
+ if !outboundHTTPCallRe.MatchString(content) {
+ return nil
+ }
+
+ hasLimiter := limiterUseRe.MatchString(content)
+ hasTypedError := rateLimitErrorRe.MatchString(content)
+ if hasLimiter && hasTypedError {
+ return nil
+ }
+
+ rel, _ := filepath.Rel(cliDir, path)
+ pkgRel, _ := filepath.Rel(filepath.Join(cliDir, "internal"), filepath.Dir(path))
+ finding := SourceClientFinding{
+ Package: filepath.ToSlash(pkgRel),
+ File: filepath.ToSlash(rel),
+ }
+ switch {
+ case !hasLimiter && !hasTypedError:
+ finding.Reason = "outbound HTTP without rate limiter or typed 429 handling"
+ case !hasLimiter:
+ finding.Reason = "outbound HTTP without rate limiter (cliutil.AdaptiveLimiter or equivalent)"
+ default:
+ finding.Reason = "outbound HTTP without typed RateLimitError or 429 status check (swallows throttling as empty results)"
+ }
+ result.Findings = append(result.Findings, finding)
+ return nil
+ })
+}
diff --git a/internal/pipeline/source_client_check_test.go b/internal/pipeline/source_client_check_test.go
new file mode 100644
index 00000000..a0437590
--- /dev/null
+++ b/internal/pipeline/source_client_check_test.go
@@ -0,0 +1,299 @@
+package pipeline
+
+import (
+ "os"
+ "path/filepath"
+ "strings"
+ "testing"
+)
+
+// writeInternal drops content at <cliDir>/internal/<relPath>, creating parents.
+// relPath is the path under internal/ (e.g. "source/sec/sec.go").
+func writeInternal(t *testing.T, cliDir, relPath, content string) {
+ t.Helper()
+ full := filepath.Join(cliDir, "internal", relPath)
+ if err := os.MkdirAll(filepath.Dir(full), 0o755); err != nil {
+ t.Fatalf("mkdir %s: %v", filepath.Dir(full), err)
+ }
+ if err := os.WriteFile(full, []byte(content), 0o644); err != nil {
+ t.Fatalf("write %s: %v", relPath, err)
+ }
+}
+
+func TestCheckSourceClients_NoSiblingPackages(t *testing.T) {
+ cliDir := t.TempDir()
+ writeInternal(t, cliDir, "client/client.go", "package client\n")
+ writeInternal(t, cliDir, "store/store.go", "package store\n")
+
+ got := checkSourceClients(cliDir)
+ if !got.Skipped {
+ t.Errorf("Skipped = false, want true (no sibling packages)")
+ }
+ if len(got.Findings) != 0 {
+ t.Errorf("Findings = %v, want none", got.Findings)
+ }
+}
+
+func TestCheckSourceClients_NoInternalDir(t *testing.T) {
+ cliDir := t.TempDir()
+ got := checkSourceClients(cliDir)
+ if !got.Skipped {
+ t.Errorf("Skipped = false, want true (no internal/ at all)")
+ }
+}
+
+func TestCheckSourceClients_Findings(t *testing.T) {
+ const cliutilLimiterClient = `package sec
+
+import (
+ "net/http"
+ "example.com/foo/internal/cliutil"
+)
+
+type Client struct {
+ HTTP *http.Client
+ limiter *cliutil.AdaptiveLimiter
+}
+
+func (c *Client) Fetch(url string) error {
+ c.limiter.Wait()
+ req, _ := http.NewRequest("GET", url, nil)
+ resp, err := c.HTTP.Do(req)
+ if err != nil { return err }
+ if resp.StatusCode == http.StatusTooManyRequests {
+ c.limiter.OnRateLimit()
+ return &cliutil.RateLimitError{URL: url}
+ }
+ c.limiter.OnSuccess()
+ return nil
+}
+`
+ const noLimiter = `package sec
+
+import "net/http"
+
+type Client struct{ HTTP *http.Client }
+
+func (c *Client) Fetch(url string) error {
+ req, _ := http.NewRequest("GET", url, nil)
+ resp, err := c.HTTP.Do(req)
+ if err != nil { return err }
+ if resp.StatusCode == 429 { return nil }
+ return nil
+}
+`
+ const limiterButNo429 = `package sec
+
+import (
+ "net/http"
+)
+
+type secLimiter struct{}
+func (*secLimiter) Wait() {}
+func (*secLimiter) OnSuccess() {}
+func (*secLimiter) OnRateLimit() {}
+
+type Client struct {
+ HTTP *http.Client
+ limiter *secLimiter
+}
+
+func (c *Client) Fetch(url string) error {
+ c.limiter.Wait()
+ req, _ := http.NewRequest("GET", url, nil)
+ _, err := c.HTTP.Do(req)
+ if err != nil { return err }
+ c.limiter.OnSuccess()
+ return nil
+}
+`
+ const bareHTTP = `package recipes
+
+import "net/http"
+
+func Fetch(url string) error {
+ resp, err := http.Get(url)
+ if err != nil { return err }
+ _ = resp
+ return nil
+}
+`
+ const localCopiesPass = `package sec
+
+import (
+ "net/http"
+)
+
+type adaptiveLimiter struct{}
+func (*adaptiveLimiter) Wait() {}
+func (*adaptiveLimiter) OnSuccess() {}
+func (*adaptiveLimiter) OnRateLimit() {}
+
+type RateLimitError struct{ URL string }
+func (e *RateLimitError) Error() string { return "rate limited: " + e.URL }
+
+type Client struct {
+ HTTP *http.Client
+ limiter *adaptiveLimiter
+}
+
+func (c *Client) Fetch(url string) error {
+ c.limiter.Wait()
+ req, _ := http.NewRequestWithContext(nil, "GET", url, nil)
+ resp, err := c.HTTP.Do(req)
+ if err != nil { return err }
+ if resp.StatusCode == http.StatusTooManyRequests {
+ c.limiter.OnRateLimit()
+ return &RateLimitError{URL: url}
+ }
+ c.limiter.OnSuccess()
+ return nil
+}
+`
+ const reservedHTTP = `package store
+
+import "net/http"
+
+func Sync(url string) error {
+ resp, err := http.Get(url)
+ if err != nil { return err }
+ _ = resp
+ return nil
+}
+`
+ const testFileWithHTTP = `package sec
+
+import (
+ "net/http"
+ "testing"
+)
+
+func TestFoo(t *testing.T) {
+ resp, err := http.Get("http://x")
+ if err != nil { t.Fatal(err) }
+ _ = resp
+}
+`
+ const compliantSec = `package sec
+
+import (
+ "net/http"
+ "example.com/foo/internal/cliutil"
+)
+
+func Fetch(url string, l *cliutil.AdaptiveLimiter) error {
+ l.Wait()
+ resp, err := http.Get(url)
+ if err != nil { return err }
+ if resp.StatusCode == http.StatusTooManyRequests {
+ return &cliutil.RateLimitError{URL: url}
+ }
+ return nil
+}
+`
+ const ignoredFixtureFile = `package fixture
+
+import "net/http"
+func Bad(url string) error { _, err := http.Get(url); return err }
+`
+ const limiterFieldOnlyName = `package sec
+
+import "net/http"
+
+type fooLimiter struct{}
+
+type Client struct{ HTTP *http.Client; flagLimiter fooLimiter }
+
+func (c *Client) Fetch(url string) error {
+ resp, err := http.Get(url)
+ if err != nil { return err }
+ _ = resp
+ return nil
+}
+`
+
+ cases := []struct {
+ name string
+ files map[string]string
+ wantFindings int
+ wantReasonHas string
+ wantPackage string // checked when non-empty against Findings[0]
+ wantCheckedPos bool // assert Checked > 0
+ }{
+ {
+ name: "passes with cliutil limiter and error",
+ files: map[string]string{"source/sec/sec.go": cliutilLimiterClient},
+ wantCheckedPos: true,
+ },
+ {
+ name: "flags missing limiter",
+ files: map[string]string{"source/sec/sec.go": noLimiter},
+ wantFindings: 1,
+ wantReasonHas: "rate limiter",
+ wantPackage: "source/sec",
+ },
+ {
+ name: "flags swallowed 429",
+ files: map[string]string{"source/sec/sec.go": limiterButNo429},
+ wantFindings: 1,
+ wantReasonHas: "RateLimitError",
+ },
+ {
+ name: "flags both missing",
+ files: map[string]string{"recipes/archive.go": bareHTTP},
+ wantFindings: 1,
+ wantReasonHas: "without rate limiter or typed 429",
+ },
+ {
+ name: "locally-defined limiter and RateLimitError both pass",
+ files: map[string]string{"source/sec/sec.go": localCopiesPass},
+ },
+ {
+ name: "ignores reserved packages",
+ files: map[string]string{
+ "store/store.go": reservedHTTP,
+ "source/dummy/dummy.go": "package dummy\n",
+ },
+ },
+ {
+ name: "ignores _test.go files",
+ files: map[string]string{"source/sec/sec_test.go": testFileWithHTTP},
+ },
+ {
+ name: "ignores testdata and vendor",
+ files: map[string]string{
+ "source/sec/sec.go": compliantSec,
+ "source/sec/testdata/fixture.go": ignoredFixtureFile,
+ "source/sec/vendor/x/x.go": ignoredFixtureFile,
+ },
+ },
+ {
+ name: "identifier ending in Limiter alone is not enough",
+ files: map[string]string{"source/sec/sec.go": limiterFieldOnlyName},
+ wantFindings: 1,
+ wantReasonHas: "rate limiter",
+ },
+ }
+
+ for _, tc := range cases {
+ t.Run(tc.name, func(t *testing.T) {
+ cliDir := t.TempDir()
+ for path, content := range tc.files {
+ writeInternal(t, cliDir, path, content)
+ }
+ got := checkSourceClients(cliDir)
+ if len(got.Findings) != tc.wantFindings {
+ t.Fatalf("Findings = %d, want %d: %+v", len(got.Findings), tc.wantFindings, got.Findings)
+ }
+ if tc.wantReasonHas != "" && !strings.Contains(got.Findings[0].Reason, tc.wantReasonHas) {
+ t.Errorf("Reason = %q, want substring %q", got.Findings[0].Reason, tc.wantReasonHas)
+ }
+ if tc.wantPackage != "" && got.Findings[0].Package != tc.wantPackage {
+ t.Errorf("Package = %q, want %q", got.Findings[0].Package, tc.wantPackage)
+ }
+ if tc.wantCheckedPos && got.Checked == 0 {
+ t.Errorf("Checked = 0, want > 0")
+ }
+ })
+ }
+}
diff --git a/scripts/verify-skill/verify_skill.py b/scripts/verify-skill/verify_skill.py
index 915ee4c7..5835a43b 100755
--- a/scripts/verify-skill/verify_skill.py
+++ b/scripts/verify-skill/verify_skill.py
@@ -438,6 +438,143 @@ def flag_declared_in(files: Iterable[Path], flag_name: str) -> bool:
return False
+# Matches a function call whose first positional arg is `cmd` (or `cmd.Flags()`).
+# Captures the function name so the verifier can look up its body.
+HELPER_CALL_RE = re.compile(
+ r"\b([a-zA-Z_]\w*)\s*\(\s*cmd(?:\b|\.Flags\(\))"
+)
+
+# Cobra/stdlib methods that take cmd as first arg but never declare flags.
+_HELPER_CALL_IGNORE = frozenset({
+ "AddCommand", "Run", "Execute", "Help", "Usage",
+ "Print", "Printf", "Println",
+})
+
+
+def go_block_body(text: str, open_brace: int) -> str:
+ """Return the body for the Go block whose `{` starts at open_brace.
+
+ This is intentionally a small scanner rather than a Go parser. It handles
+ nested braces and skips comments/strings so adjacent helper functions do
+ not leak into the matched helper body.
+ """
+ if open_brace < 0 or open_brace >= len(text) or text[open_brace] != "{":
+ return ""
+
+ depth = 0
+ i = open_brace
+ state = "code"
+ while i < len(text):
+ ch = text[i]
+ nxt = text[i + 1] if i + 1 < len(text) else ""
+
+ if state == "line_comment":
+ if ch == "\n":
+ state = "code"
+ i += 1
+ continue
+ if state == "block_comment":
+ if ch == "*" and nxt == "/":
+ state = "code"
+ i += 2
+ else:
+ i += 1
+ continue
+ if state == "double_string":
+ if ch == "\\":
+ i += 2
+ continue
+ if ch == '"':
+ state = "code"
+ i += 1
+ continue
+ if state == "raw_string":
+ if ch == "`":
+ state = "code"
+ i += 1
+ continue
+ if state == "rune":
+ if ch == "\\":
+ i += 2
+ continue
+ if ch == "'":
+ state = "code"
+ i += 1
+ continue
+
+ if ch == "/" and nxt == "/":
+ state = "line_comment"
+ i += 2
+ continue
+ if ch == "/" and nxt == "*":
+ state = "block_comment"
+ i += 2
+ continue
+ if ch == '"':
+ state = "double_string"
+ i += 1
+ continue
+ if ch == "`":
+ state = "raw_string"
+ i += 1
+ continue
+ if ch == "'":
+ state = "rune"
+ i += 1
+ continue
+
+ if ch == "{":
+ depth += 1
+ elif ch == "}":
+ depth -= 1
+ if depth == 0:
+ return text[open_brace + 1:i]
+ i += 1
+
+ return ""
+
+
+def flag_declared_via_helper(cli_dir: Path, cmd_files: Iterable[Path], flag_name: str) -> bool:
+ """Return True if any helper called from cmd_files (with cmd as first arg)
+ declares flag_name in its body. One level of indirection only — no recursive
+ resolution."""
+ helper_names: set[str] = set()
+ for f in cmd_files:
+ try:
+ text = f.read_text()
+ except Exception:
+ continue
+ for m in HELPER_CALL_RE.finditer(text):
+ name = m.group(1)
+ if name not in _HELPER_CALL_IGNORE:
+ helper_names.add(name)
+ if not helper_names:
+ return False
+
+ src = cli_dir / "internal/cli"
+ if not src.exists():
+ return False
+
+ func_re = re.compile(
+ r"func\s+("
+ + "|".join(re.escape(n) for n in helper_names)
+ + r")\s*\([^)]*\)\s*(?:\w+\s*)?\{"
+ )
+ for go_file in src.glob("*.go"):
+ if go_file.name.endswith("_test.go"):
+ continue
+ try:
+ text = go_file.read_text()
+ except Exception:
+ continue
+ for m in func_re.finditer(text):
+ body = go_block_body(text, m.end() - 1)
+ for fm in FLAG_DECL_RE.finditer(body):
+ if fm.group(3) == flag_name:
+ return True
+ return False
+
+
def persistent_flag_declared(cli_dir: Path, flag_name: str) -> bool:
src = cli_dir / "internal/cli"
if not src.exists():
@@ -601,6 +738,8 @@ def check_flag_commands(cli_dir: Path, skill: Path, cli_binary: str, report: Rep
continue
if persistent_flag_declared(cli_dir, flag):
continue
+ if cmd_files and flag_declared_via_helper(cli_dir, cmd_files, flag):
+ continue
path_str = " ".join(cmd_path)
if flag_declared_in(all_files, flag):
report.findings.append(
diff --git a/skills/printing-press/SKILL.md b/skills/printing-press/SKILL.md
index 71bc221f..a94648f0 100644
--- a/skills/printing-press/SKILL.md
+++ b/skills/printing-press/SKILL.md
@@ -1681,6 +1681,7 @@ After building each command in Priority 1 and Priority 2, verify these 9 princip
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.
+ - **Use string for "positional OR flag" commands**: when a command accepts a positional `<x>` OR a flag `--y` as alternatives (e.g., `snapshot <co>` or `snapshot --domain example.com`), declare `Use: "<cmd> [x]"` with **square brackets** (optional), not `<x>` (required). Validate "exactly one of x or --y" inside RunE. Required positionals declared with angle brackets break verify-skill recipes that use the flag-only form.
9. **Side-effect commands stay quiet under verify**: Any hand-written command that performs a visible side effect (opens a browser tab, sends a notification, plays audio, dials out to an OS handler) MUST follow both halves of the convention:
- **Print by default; opt in to the action.** The default behavior prints what would happen (`would launch: <url>`); a flag like `--launch` / `--send` / `--play` is required to actually do it. food52's `open` command is the reference shape — see `internal/cli/open.go` after retro #337.
- **Short-circuit when `cliutil.IsVerifyEnv()` returns true.** The Printing Press verifier sets `PRINTING_PRESS_VERIFY=1` in every mock-mode subprocess; commands that ignore it can spam the user's environment during a verify pass even with the print-by-default flag pattern. The helper is generated into every CLI's `internal/cliutil/verifyenv.go`. Pattern:
@@ -1691,6 +1692,7 @@ After building each command in Priority 1 and Priority 2, verify these 9 princip
}
```
This is defense-in-depth: the verifier also runs a heuristic side-effect classifier, but it can miss commands whose `--help` text and source don't match the heuristics. The env-var check is the floor.
+10. **Per-source rate limiting**: any hand-written client in a sibling internal package (`internal/source/<name>/`, `internal/recipes/`, `internal/phgraphql/`, etc. — anything not generator-emitted) that makes outbound HTTP calls MUST use `cliutil.AdaptiveLimiter` and surface `*cliutil.RateLimitError` when 429 retries are exhausted. Empty-on-throttle is indistinguishable from "no data exists" and silently corrupts downstream queries. Read [references/per-source-rate-limiting.md](references/per-source-rate-limiting.md) when authoring a sibling client. Enforced at generation time by dogfood's `source_client_check`.
#### Verify-friendly RunE template
@@ -1847,6 +1849,8 @@ Ship threshold (the umbrella's verdict is the canonical signal — all of these
**Behavioral correctness is part of the ship threshold, not just structural quality.** A Grade A scorecard with a broken flagship feature (e.g., `goat "brownies"` returning a chili recipe) does NOT pass the ship threshold. Run a sample invocation of every novel-feature command before declaring shipcheck complete.
+**Per-source row for combo CLIs (synthetic spec, multiple data sources).** For every named source in a combo CLI (`internal/source/<name>/`, `internal/recipes/`, `internal/phgraphql/`, etc.) the dogfood test matrix MUST add one row per source: with the source's limiter exhausted (or the upstream genuinely throttling), assert that the user-facing command surfaces a typed `*cliutil.RateLimitError` referencing the source — not empty JSON / `0 results`. A passing row says: "the CLI distinguishes 'no data' from 'we got rate-limited' for this source." The matrix-builder derives rows from the command tree by default; for combo CLIs, also derive rows from the source list. `source_client_check` catches the static signal that throttling is silently swallowed; only the runtime row proves the user-visible behavior.
+
Maximum 2 shipcheck loops by default.
Write:
diff --git a/skills/printing-press/references/per-source-rate-limiting.md b/skills/printing-press/references/per-source-rate-limiting.md
new file mode 100644
index 00000000..06ccbaea
--- /dev/null
+++ b/skills/printing-press/references/per-source-rate-limiting.md
@@ -0,0 +1,81 @@
+# Per-source rate limiting
+
+Hand-written clients in sibling internal packages (`internal/source/<name>/`,
+`internal/recipes/`, `internal/phgraphql/`, etc.) MUST use
+`cliutil.AdaptiveLimiter` and surface `*cliutil.RateLimitError` from public
+methods when 429 retries are exhausted. Returning empty results on throttle is
+silent corruption: downstream queries cannot tell "the source has no data" from
+"the source rate-limited us."
+
+## Reference shape
+
+```go
+import (
+ "context"
+ "errors"
+ "net/http"
+
+ "<modulePath>/internal/cliutil"
+)
+
+type Client struct {
+ HTTP *http.Client
+ limiter *cliutil.AdaptiveLimiter
+}
+
+func New() *Client {
+ return &Client{
+ HTTP: &http.Client{},
+ limiter: cliutil.NewAdaptiveLimiter(2.0),
+ }
+}
+
+func (c *Client) Fetch(ctx context.Context, url string) ([]byte, error) {
+ c.limiter.Wait()
+ req, err := http.NewRequestWithContext(ctx, "GET", url, nil)
+ if err != nil {
+ return nil, err
+ }
+ resp, err := c.HTTP.Do(req)
+ if err != nil {
+ return nil, err
+ }
+ defer resp.Body.Close()
+ if resp.StatusCode == http.StatusTooManyRequests {
+ c.limiter.OnRateLimit()
+ return nil, &cliutil.RateLimitError{
+ URL: url,
+ RetryAfter: cliutil.RetryAfter(resp),
+ }
+ }
+ c.limiter.OnSuccess()
+ // ... read + return body ...
+ return nil, nil
+}
+```
+
+## Higher-level fanout commands
+
+Aggregations that fan out N filings/items MUST `errors.As(err, &rateErr)` and
+propagate the failure rather than `continue`-ing past it:
+
+```go
+for _, item := range items {
+ detail, err := c.FetchDetail(ctx, item.ID)
+ if err != nil {
+ var rateErr *cliutil.RateLimitError
+ if errors.As(err, &rateErr) {
+ return nil, err
+ }
+ continue // other errors are still skippable
+ }
+ out = append(out, detail)
+}
+```
+
+## Enforcement
+
+`printing-press dogfood` runs `source_client_check`, which scans every
+`internal/<pkg>/*.go` file outside the generator-emitted set. A file that
+makes outbound HTTP calls but lacks a limiter signal or a typed-error signal
+produces a finding.
diff --git a/testdata/golden/cases/generate-golden-api/artifacts.txt b/testdata/golden/cases/generate-golden-api/artifacts.txt
index 18e834f6..cde14981 100644
--- a/testdata/golden/cases/generate-golden-api/artifacts.txt
+++ b/testdata/golden/cases/generate-golden-api/artifacts.txt
@@ -12,6 +12,7 @@ printing-press-golden/internal/cli/projects_tasks_list-project.go
printing-press-golden/internal/cli/projects_tasks_update-project.go
printing-press-golden/internal/cli/promoted_public.go
printing-press-golden/internal/cliutil/text.go
+printing-press-golden/internal/cliutil/ratelimit.go
printing-press-golden/internal/config/config.go
printing-press-golden/internal/mcp/tools.go
printing-press-golden/cmd/printing-press-golden-pp-mcp/main.go
diff --git a/testdata/golden/expected/dogfood-verdict-matrix/fail-path-auth-dead.json b/testdata/golden/expected/dogfood-verdict-matrix/fail-path-auth-dead.json
index f3916e01..34fa2406 100644
--- a/testdata/golden/expected/dogfood-verdict-matrix/fail-path-auth-dead.json
+++ b/testdata/golden/expected/dogfood-verdict-matrix/fail-path-auth-dead.json
@@ -62,6 +62,10 @@
"exempted_via_store": 0,
"skipped": true
},
+ "source_client_check": {
+ "checked": 0,
+ "skipped": true
+ },
"spec_path": "<ARTIFACT_DIR>/dogfood-verdict-matrix/fixtures/fail-path-auth-dead/spec.yaml",
"test_presence": {
"checked": 0
diff --git a/testdata/golden/expected/dogfood-verdict-matrix/pass.json b/testdata/golden/expected/dogfood-verdict-matrix/pass.json
index 73d5c9e2..29599792 100644
--- a/testdata/golden/expected/dogfood-verdict-matrix/pass.json
+++ b/testdata/golden/expected/dogfood-verdict-matrix/pass.json
@@ -50,6 +50,10 @@
"exempted_via_store": 0,
"skipped": true
},
+ "source_client_check": {
+ "checked": 0,
+ "skipped": true
+ },
"test_presence": {
"checked": 0
},
diff --git a/testdata/golden/expected/dogfood-verdict-matrix/warn-priority.json b/testdata/golden/expected/dogfood-verdict-matrix/warn-priority.json
index fdfb0d38..e616e283 100644
--- a/testdata/golden/expected/dogfood-verdict-matrix/warn-priority.json
+++ b/testdata/golden/expected/dogfood-verdict-matrix/warn-priority.json
@@ -56,6 +56,10 @@
"exempted_via_store": 0,
"skipped": true
},
+ "source_client_check": {
+ "checked": 0,
+ "skipped": true
+ },
"test_presence": {
"checked": 0
},
diff --git a/testdata/golden/expected/generate-golden-api/dogfood.json b/testdata/golden/expected/generate-golden-api/dogfood.json
index 68688ab2..6b243dc6 100644
--- a/testdata/golden/expected/generate-golden-api/dogfood.json
+++ b/testdata/golden/expected/generate-golden-api/dogfood.json
@@ -53,6 +53,10 @@
"exempted_via_store": 0,
"skipped": true
},
+ "source_client_check": {
+ "checked": 0,
+ "skipped": true
+ },
"test_presence": {
"checked": 0
},
diff --git a/testdata/golden/expected/generate-golden-api/printing-press-golden/internal/client/client.go b/testdata/golden/expected/generate-golden-api/printing-press-golden/internal/client/client.go
index b7c1059c..460f5171 100644
--- a/testdata/golden/expected/generate-golden-api/printing-press-golden/internal/client/client.go
+++ b/testdata/golden/expected/generate-golden-api/printing-press-golden/internal/client/client.go
@@ -15,10 +15,9 @@ import (
"net/url"
"os"
"path/filepath"
- "strconv"
"strings"
- "sync"
"time"
+ "printing-press-golden-pp-cli/internal/cliutil"
"printing-press-golden-pp-cli/internal/config"
)
@@ -29,92 +28,7 @@ type Client struct {
DryRun bool
NoCache bool
cacheDir string
- limiter *adaptiveLimiter
-}
-
-// adaptiveLimiter provides proactive rate limiting with adaptive ceiling discovery.
-// Starts at a conservative floor rate and ramps up after consecutive successes.
-// On 429, halves the rate and records a ceiling. Per-session only — not persisted.
-// Thread-safe: all methods are guarded by a mutex for concurrent sync workers.
-type adaptiveLimiter struct {
- mu sync.Mutex
- rate float64 // current requests per second
- floor float64 // starting/minimum rate
- ceiling float64 // discovered ceiling (0 = unknown)
- successes int // consecutive successes since last 429
- rampAfter int // successes needed before increasing rate
- lastRequest time.Time // zero-value on init — first call skips wait (intentional)
-}
-
-func newAdaptiveLimiter(ratePerSec float64) *adaptiveLimiter {
- if ratePerSec <= 0 {
- return nil
- }
- return &adaptiveLimiter{
- rate: ratePerSec,
- floor: ratePerSec,
- rampAfter: 10,
- }
-}
-
-// Wait blocks until the rate limiter allows the next request.
-func (l *adaptiveLimiter) Wait() {
- if l == nil {
- return
- }
- l.mu.Lock()
- delay := time.Duration(float64(time.Second) / l.rate)
- elapsed := time.Since(l.lastRequest)
- l.mu.Unlock()
- if elapsed < delay {
- time.Sleep(delay - elapsed)
- }
- l.mu.Lock()
- l.lastRequest = time.Now()
- l.mu.Unlock()
-}
-
-// OnSuccess records a successful request and ramps up the rate after enough consecutive successes.
-func (l *adaptiveLimiter) OnSuccess() {
- if l == nil {
- return
- }
- l.mu.Lock()
- defer l.mu.Unlock()
- l.successes++
- if l.successes >= l.rampAfter {
- newRate := l.rate * 1.25
- if l.ceiling > 0 && newRate > l.ceiling*0.9 {
- newRate = l.ceiling * 0.9
- }
- l.rate = newRate
- l.successes = 0
- }
-}
-
-// OnRateLimit records a 429 response — halves the rate and discovers the ceiling.
-func (l *adaptiveLimiter) OnRateLimit() {
- if l == nil {
- return
- }
- l.mu.Lock()
- defer l.mu.Unlock()
- l.ceiling = l.rate
- l.rate = l.rate / 2
- if l.rate < 0.5 {
- l.rate = 0.5 // absolute minimum: 1 request per 2 seconds
- }
- l.successes = 0
-}
-
-// Rate returns the current rate in requests per second. Returns 0 if the limiter is nil.
-func (l *adaptiveLimiter) Rate() float64 {
- if l == nil {
- return 0
- }
- l.mu.Lock()
- defer l.mu.Unlock()
- return l.rate
+ limiter *cliutil.AdaptiveLimiter
}
@@ -144,7 +58,7 @@ func New(cfg *config.Config, timeout time.Duration, rateLimit float64) *Client {
Config: cfg,
HTTPClient: httpClient,
cacheDir: cacheDir,
- limiter: newAdaptiveLimiter(rateLimit),
+ limiter: cliutil.NewAdaptiveLimiter(rateLimit),
}
}
@@ -332,7 +246,7 @@ func (c *Client) do(method, path string, params map[string]string, body any, hea
// Rate limited - adjust adaptive limiter and retry
if resp.StatusCode == 429 && attempt < maxRetries {
c.limiter.OnRateLimit()
- wait := retryAfter(resp)
+ wait := cliutil.RetryAfter(resp)
fmt.Fprintf(os.Stderr, "rate limited, waiting %s (attempt %d/%d, rate adjusted to %.1f req/s)\n", wait, attempt+1, maxRetries, c.limiter.Rate())
time.Sleep(wait)
lastErr = apiErr
@@ -464,32 +378,6 @@ func (c *Client) refreshAccessToken() error {
return nil
}
-const maxRetryWait = 60 * time.Second
-
-func retryAfter(resp *http.Response) time.Duration {
- header := resp.Header.Get("Retry-After")
- if header == "" {
- return 5 * time.Second
- }
- if seconds, err := strconv.Atoi(header); err == nil {
- d := time.Duration(seconds) * time.Second
- if d > maxRetryWait {
- return maxRetryWait
- }
- return d
- }
- if t, err := http.ParseTime(header); err == nil {
- wait := time.Until(t)
- if wait > maxRetryWait {
- return maxRetryWait
- }
- if wait > 0 {
- return wait
- }
- }
- return 5 * time.Second
-}
-
// sanitizeJSONResponse strips known JSONP/XSSI prefixes and UTF-8 BOM from
// response bodies so that downstream JSON parsing succeeds. For clean JSON
// responses these checks are no-ops.
diff --git a/testdata/golden/expected/generate-golden-api/printing-press-golden/internal/cliutil/ratelimit.go b/testdata/golden/expected/generate-golden-api/printing-press-golden/internal/cliutil/ratelimit.go
new file mode 100644
index 00000000..3f7cbf58
--- /dev/null
+++ b/testdata/golden/expected/generate-golden-api/printing-press-golden/internal/cliutil/ratelimit.go
@@ -0,0 +1,170 @@
+// Copyright 2026 printing-press-golden. Licensed under Apache-2.0. See LICENSE.
+// Generated by CLI Printing Press (https://github.com/mvanhorn/cli-printing-press). DO NOT EDIT.
+
+package cliutil
+
+import (
+ "fmt"
+ "math"
+ "net/http"
+ "strconv"
+ "strings"
+ "sync"
+ "time"
+)
+
+// AdaptiveLimiter paces outbound requests with adaptive ceiling discovery.
+// Starts at a floor rate, ramps up after consecutive successes, halves on 429
+// and records a ceiling. Per-session only — not persisted. Methods are safe
+// to call on a nil receiver.
+type AdaptiveLimiter struct {
+ mu sync.Mutex
+ rate float64
+ floor float64
+ ceiling float64
+ successes int
+ rampAfter int
+ lastRequest time.Time // zero-value: first Wait() returns immediately
+}
+
+// NewAdaptiveLimiter returns a limiter starting at ratePerSec, or nil when
+// rate-limiting should be disabled. Methods on the nil limiter no-op.
+func NewAdaptiveLimiter(ratePerSec float64) *AdaptiveLimiter {
+ if ratePerSec <= 0 {
+ return nil
+ }
+ return &AdaptiveLimiter{
+ rate: ratePerSec,
+ floor: ratePerSec,
+ rampAfter: 10,
+ }
+}
+
+func (l *AdaptiveLimiter) Wait() {
+ if l == nil {
+ return
+ }
+ l.mu.Lock()
+ delay := time.Duration(float64(time.Second) / l.rate)
+ elapsed := time.Since(l.lastRequest)
+ l.mu.Unlock()
+ if elapsed < delay {
+ time.Sleep(delay - elapsed)
+ }
+ l.mu.Lock()
+ l.lastRequest = time.Now()
+ l.mu.Unlock()
+}
+
+func (l *AdaptiveLimiter) OnSuccess() {
+ if l == nil {
+ return
+ }
+ l.mu.Lock()
+ defer l.mu.Unlock()
+ l.successes++
+ if l.successes >= l.rampAfter {
+ newRate := l.rate * 1.25
+ if l.ceiling > 0 && newRate > l.ceiling*0.9 {
+ newRate = l.ceiling * 0.9
+ }
+ l.rate = newRate
+ l.successes = 0
+ }
+}
+
+func (l *AdaptiveLimiter) OnRateLimit() {
+ if l == nil {
+ return
+ }
+ l.mu.Lock()
+ defer l.mu.Unlock()
+ l.ceiling = l.rate
+ l.rate = l.rate / 2
+ if l.rate < 0.5 {
+ l.rate = 0.5
+ }
+ l.successes = 0
+}
+
+func (l *AdaptiveLimiter) Rate() float64 {
+ if l == nil {
+ return 0
+ }
+ l.mu.Lock()
+ defer l.mu.Unlock()
+ return l.rate
+}
+
+// RateLimitError signals an upstream returned 429 after retries were
+// exhausted. Callers must surface this as a hard error rather than empty
+// results — empty-on-throttle is indistinguishable from "no data exists"
+// and silently corrupts downstream queries.
+type RateLimitError struct {
+ URL string
+ RetryAfter time.Duration
+ Body string
+}
+
+func (e *RateLimitError) Error() string {
+ msg := fmt.Sprintf("rate limited: HTTP 429 for %s", e.URL)
+ if e.RetryAfter > 0 {
+ msg += fmt.Sprintf("; retry after %s", e.RetryAfter)
+ }
+ if body := strings.TrimSpace(e.Body); body != "" {
+ msg += ": " + body
+ }
+ return msg
+}
+
+// MaxRetryWait caps the wait derived from a Retry-After header so a buggy
+// or hostile upstream cannot pin a CLI for hours.
+const MaxRetryWait = 60 * time.Second
+
+// RetryAfter parses an HTTP Retry-After header (RFC 7231: delta-seconds or
+// HTTP-date), capped at MaxRetryWait. Returns 5s when missing or unparseable.
+func RetryAfter(resp *http.Response) time.Duration {
+ if resp == nil {
+ return 5 * time.Second
+ }
+ header := strings.TrimSpace(resp.Header.Get("Retry-After"))
+ if header == "" {
+ return 5 * time.Second
+ }
+ if seconds, err := strconv.Atoi(header); err == nil {
+ d := time.Duration(seconds) * time.Second
+ if d > MaxRetryWait {
+ return MaxRetryWait
+ }
+ if d <= 0 {
+ return 5 * time.Second
+ }
+ return d
+ }
+ if t, err := http.ParseTime(header); err == nil {
+ wait := time.Until(t)
+ if wait > MaxRetryWait {
+ return MaxRetryWait
+ }
+ if wait > 0 {
+ return wait
+ }
+ }
+ return 5 * time.Second
+}
+
+// MaxBackoff caps Backoff so tests stay bounded. Callers needing jitter
+// add their own; the bare exponential keeps the contract deterministic.
+const MaxBackoff = 30 * time.Second
+
+// Backoff returns 2^attempt seconds capped at MaxBackoff.
+func Backoff(attempt int) time.Duration {
+ if attempt < 0 {
+ attempt = 0
+ }
+ wait := time.Duration(math.Pow(2, float64(attempt))) * time.Second
+ if wait > MaxBackoff {
+ return MaxBackoff
+ }
+ return wait
+}
← 5777d267 chore(cli): drop dead manifest_url + manifest_checksum from
·
back to Cli Printing Press
·
docs(cli): plan MCP tool surface mirrors Cobra tree, with mc 4a2caae0 →