Files
pad/internal/cmdhelp/example_validation.go
T
xarmian 6fdd8816f3 feat(playbook): match free text to an active playbook via the decision provider (TASK-3120) (#1429)
* feat(server): POST /playbooks/match — typed-decision Choice over active playbooks (TASK-3120)

PLAN-3114 unit 5. A synchronous, read-only endpoint: given free text, asks
the workspace's typed-decision provider which active (non-draft,
non-deprecated) playbook it best matches, with a reserved "none" option so
unrelated text gets an honest answer instead of a forced pick — and, as a
side effect, keeps a workspace with exactly one active playbook askable
(decision.Question.Validate needs >=2 Choice options).

- decision.Runner gains a nil-safe Provider() accessor: this endpoint needs
  a plain synchronous Ask, not the owed-jobs pipeline Evaluate drives.
- No provider configured -> 404 decision_provider_unavailable (callers fall
  back to slug/trigger routing). A provider error or an out-of-set answer
  the provider was never offered -> 502 decision_provider_error, distinct
  from the 404 case so a caller can tell "try again" from "not configured".
  Zero active playbooks -> 200 with a stated reason, no provider call.
  Over the provider's 255-option ceiling (including "none") -> refused
  rather than silently dropping playbooks and answering from an
  incomplete set.
- listPlaybookItems is factored out of handleListPlaybooks so list and
  match share one visibility-scoped item source and can't drift on which
  playbooks a caller can see.
- 30s timeout on the synchronous provider call, tighter than the
  provider's own worst-case retry budget (~330s) since an HTTP caller
  blocks on this one directly, unlike the async decision tick.
- content_state_population_test.go's BUG-3033 sweep gets a new ruling: the
  PlaybookSummary call here feeds the provider prompt only, never the
  response, so there's no reader-facing text to mark.

Claude-Session: https://claude.ai/code/session_01ByMjAhvdWgbAWWeGE9jwWd

* feat(cli): pad playbook match "<text>" (TASK-3120)

CLI + client-method half of PLAN-3114 unit 5 — POST /playbooks/match.
Table output prints the matched ref/title (or "none"), confidence, model,
and a probabilities table sorted descending; --format json passes the
response through unmodified. Read-only, side-effect-free.

Claude-Session: https://claude.ai/code/session_01ByMjAhvdWgbAWWeGE9jwWd

* docs(skill): route to playbook match as rung 3, offer-only (TASK-3120)

PLAN-3114 unit 5's skill half. Exact invocation-slug match and trigger
match stay rungs 1-2; match is consulted only when the user is plainly
asking for a procedure/workflow and both of those missed — never on
ordinary conversation, since every call spends provider money. Any match
returned is offered, never auto-run, at any confidence: TASK-3137's replay
of this same provider's attention question set found no validated
threshold, so nothing here invents one for this question either. A 404
(no provider configured), a "none" choice, or any other error falls back
silently to today's slug/trigger-only behavior.

Claude-Session: https://claude.ai/code/session_01ByMjAhvdWgbAWWeGE9jwWd

* chore(mcp): de-indent padItemToolDescription's continuation lines (TASK-3120)

Whitespace-only — git diff -w on this file is empty. The description's
continuation lines carried 18 leading spaces purely for visual alignment
under each action's "  name — " bullet in the Go source; that padding is
emitted verbatim into the tools/list JSON, where it conveys nothing an
LLM reading the tool description gets any value from.

De-indented by exactly 14 spaces on every line that had >= 18 (all 116 of
them, uniformly, since the file carries only three indentation levels:
0, 2, 18 — no deeper nesting to preserve a relative offset for).
Continuations now land at 4 (bullet's 2 + 2), so a reader can still tell
which action a line belongs to; a 1-space collapse would have inverted
that (continuation LESS indented than its own bullet).

tools/list: 55290 -> 53666 bytes (-1624). initialize: unchanged at 24530
(it carries no tool descriptions). Frees headroom against the 55296
budget for the pad_playbook.match action landing in the next commit —
budget constants themselves are untouched, per standing instruction.

Claude-Session: https://claude.ai/code/session_01ByMjAhvdWgbAWWeGE9jwWd

* feat(mcp): pad_playbook.match action, ToolSurfaceVersion 0.39 -> 0.41 (TASK-3120)

PLAN-3114 unit 5's MCP half, landing on top of the whitespace-only
catalog_item.go trim (previous commit) so this stays within the
tools/list budget with room to spare.

- catalog_playbook.go: new `text` param, new `match` action
  (passThrough — the CLI's positional arg is named "text", matching
  this tool's param, so no custom shaping is needed) plus a
  description bullet in padPlaybookToolDescription.
- tool_surface.go: `match` marked read-only (nothing stored, no job
  enqueued).
- dispatch_http_routes.go: `playbook match` routeSpec (POST, flat
  {text} body) so the remote /mcp transport can dispatch it in-process.
- version.go: ToolSurfaceVersion 0.39 -> 0.41. 0.40 is reserved for
  PR #1427 (the 0.32/0.33 precedent — a gap is cheaper than a
  collision). 0.41's own entry: pure ADDITIVE bump, same disposition
  as 0.13/0.28/0.11 — nothing existing moved.
- instructions.md, README.md, CLAUDE.md: version headings + the
  pad_playbook bullet/table-row/changelog-sentence updated per the
  drift tests (TestInstructionsMDCoversEveryCatalogAction,
  TestReadmeTableCoversEveryCatalogAction, the two version-match
  tests). instructions.md's addition is intentionally terse ("`match`
  finds a playbook from text.") — it's embedded in the `initialize`
  handshake payload, which has its own 24576-byte budget and had only
  46 bytes of headroom; a fuller explanation lives in the tools/list
  param/action docs instead, which have budget to spare after the trim.
- catalog_readonly_test.go, dispatch_http_parity_test.go: new
  `pad_playbook.match` entries in three hand-built fixture maps
  (expected cmdPath ×2, a hand-rolled cmdhelp stub, and the shared
  parity-drive fixture input) so the existing bijection/parity/dispatch
  guards actually exercise the new action instead of skipping it.

Bytes measured (go test ./internal/mcp -run Budget -v):
  tools/list:  55290 (baseline) -> 53666 (after the trim commit) ->
               54805 (after this commit) — 491 bytes of headroom left
               against the 55296 budget.
  initialize:  24530 (baseline, unchanged by the trim) -> 24574 (after
               this commit) — 2 bytes of headroom against the 24576
               budget. Flagged in the implementer report: this is now
               as tight as tools/list was before the trim, and nothing
               in this unit addresses it.

Claude-Session: https://claude.ai/code/session_01ByMjAhvdWgbAWWeGE9jwWd

* fix(server,decision): don't leak provider errors, classify oversized requests, fix none's wording (TASK-3120)

Lead's shallow-pass findings on cd7a41ed, applied after the MCP commit.

F1 (disclosure): the provider-error path answered with err.Error()
verbatim. typesafe.go's client surfaces the provider's raw 4xx body in
that string (APIError.Error()), which is the INSTANCE's own account
state with typesafe.ai — key validity, quota, billing wording — not
anything about the caller's request. This endpoint requires only
viewer access, so any workspace member could read it. Now logged
server-side in full (slog, workspace + err) and answered with a fixed
message plus a coarse details.reason ("timeout" for a context
deadline, "upstream" otherwise) via a new writeDecisionProviderError
helper. Test constructs a fake error containing a secret-looking
string and asserts it never reaches the response body.

F2 (misclassification): the request budget is 32000 tokens
(~80000 chars). At a couple hundred active playbooks with realistic
summaries, the Choice question's options alone can exceed it before
any state is even added — or leave no room for the input text. Both
were plain fmt.Errorf with no sentinel, so they fell into the generic
"provider failed" 502 path, telling a caller "try again" for a
request that will never fit no matter how many times it's retried.
decision/typesafe.go gains ErrRequestTooLarge (wrapped with %w onto
both pre-send refusals; existing message text unchanged, just
appended-to) alongside the existing ErrMaxTokensExceeded (the
provider's own after-the-fact refusal of a request fitState let
through) — both now map to a distinct 400 match_request_too_large.
New decision-package unit test builds a realistic 254-option Choice
(title + near-cap summary + trigger + slug per option, mirroring what
playbook-match could plausibly send) and confirms fitState refuses it
before the fake HTTP handler is ever called; server-side table test
covers both sentinels through the fake provider.

F3 (nit): the "none" option's description read "...listed above",
but Options is an unordered map — nothing puts them "above" it.
Reworded to "...any of the other options' procedures"; the pinned
exact-prompt test updated to match.

Also (lead's separate MCP-round instruction): decision.go's
maxChoiceOptions is now exported as MaxChoiceOptions, and the
server's over-limit refusal reads that constant instead of a copied
`255` literal, so the two ceilings cannot drift apart.

go test ./internal/decision/... ./internal/mcp/... ./internal/server/...
./cmd/pad/... ./internal/cli/... — all green. golangci-lint run ./...
— 0 issues. Full `go test ./...` run separately; final line in the
report.

Claude-Session: https://claude.ai/code/session_01ByMjAhvdWgbAWWeGE9jwWd

* chore(mcp): cut pad_playbook.match's tools/list text to the approved +292 variant (TASK-3120)

The lead's shape-(a) approval was for the measured "+292 documented"
prototype, not the fuller wording ae38353c actually shipped
(+1139 bytes) — the trim commit exists to pay for the approved
action text, not to fund arbitrary new prose. Cut the `text` param to
one sentence (the 404-fallback advice lives once, in the action
bullet, not repeated in the param) and the `match` action bullet down
to four lines at consistent bullet+2 continuation indentation,
matching the rest of padPlaybookToolDescription's style (the previous
wording had drifted to 20-space continuation on its last two lines).

tools/list: 53666 (post-trim baseline) -> 54023 (+357, close to the
~300 target — wording differs from the original prototype's exact
text, so an exact match isn't expected). initialize: unchanged at
24574 (this file's bullet is tools/list-only; instructions.md is a
separate file, untouched here).

Claude-Session: https://claude.ai/code/session_01ByMjAhvdWgbAWWeGE9jwWd

* fix(server): don't overclaim writeDecisionProviderError's timeout reason (TASK-3120)

The doc comment said "timeout" covered a cancelled caller request too,
but context.Canceled is a distinct error from context.DeadlineExceeded
and errors.Is(err, context.DeadlineExceeded) does not match it — that
case actually falls through to "upstream", contradicting what the
comment claimed. Narrowed the comment to deadline-only (the preferred
fix per review): a cancelled caller's own request also never reads
this response body, so the distinction was moot as well as wrong.
Code was already correct; only the comment overclaimed.

Claude-Session: https://claude.ai/code/session_01ByMjAhvdWgbAWWeGE9jwWd

* fix(cli,skill): pad playbook match needs -- before dash-led text (TASK-3120)

Codex round 2 finding on f31d5d64. Text starting with "-" (a dash-led
sentence, or literal text like "-ship it") is parsed as a flag by
cobra's default scanner, on both the CLI and stdio MCP (which shells
out to this same command). Pre-existing and class-wide — `pad item
search "-x"` fails the same way — so the general stdio fix across
every command is filed as BUG-3142 rather than done here; BuildCLIArgs
is untouched.

This command's own help now shows the workaround: an Example section
with `pad playbook match -- "-ship it"`. skills/pad/SKILL.md's rung-3
routing entry calls it the same way, with a one-line reason, and notes
the MCP form is unaffected (a structured `text` param, not a CLI
token).

New CLI test asserts BOTH directions: `playbook match "-ship it"`
(no --) fails at cobra's flag parser and never reaches the server;
`playbook match -- "-ship it"` succeeds and the server receives the
text with its leading dash intact. Ran both legs — see the test.

Claude-Session: https://claude.ai/code/session_01ByMjAhvdWgbAWWeGE9jwWd

* fix(skill): local stdio MCP is not exempt from the leading-dash flag parse (TASK-3120)

Codex round 4 finding. The rung-3 routing entry claimed the MCP form of
match was "unaffected" by BUG-3142, but that's only true for remote
/mcp, which keeps `text` a structured param. Local stdio MCP shells
out to `pad playbook match`, turning `text` into a CLI positional via
BuildCLIArgs (dispatch.go:435) — the same flag parser, the same bug.
Corrected: remote /mcp is safe as-is; local stdio MCP still hits it and
should prefer the CLI's `--` form when the text starts with `-`.

Claude-Session: https://claude.ai/code/session_01ByMjAhvdWgbAWWeGE9jwWd

* fix(cmdhelp): the example validator's flag scan must stop at "--" (TASK-3120)

CI red on #1429 (a2a5a853), Go/Go-PG/Nix all failing the same way:
TestRealPadTree_ExampleDriftValidator flagged the new `pad playbook
match -- "-ship it"` example's own dash-led TEXT ARGUMENT as an
unknown flag `--ship it`. Cobra stops flag parsing at a lone "--" —
that's the whole point of the token — but validateExample's scan
treated "--" as just another flag-shaped token: TrimLeft reduced it
to an empty name, which fell into the "skip, keep scanning" branch
instead of stopping the scan. Everything after "--" kept getting
flag-checked, so the terminated example's own positional text was
misread as a flag.

Fixed by checking `t == "--"` BEFORE the dash-stripping and breaking
the scan there, per review. A lone "-" (the stdin convention, e.g.
`pad item import -`) is a different token — it still reaches the
dash-stripping, still reduces to an empty name, and still just
`continue`s past — so it does not terminate the scan.

Three new unit tests in internal/cmdhelp cover all three legs: (a) a
dash-led positional after "--" produces no finding, (b) an unknown
flag BEFORE "--" is still caught — the break cannot hide drift ahead
of the terminator, and (c) a lone "-" does not stop the scan (an
unknown flag after it is still caught). Manually re-ran (a) with the
break removed to confirm it goes red without the fix, then restored
the real fix — ran, not just described: FAIL "a dash-led positional
after -- should not be reported as an unknown flag" without the break,
PASS with it.

The playbook match example itself is unchanged.

Claude-Session: https://claude.ai/code/session_01ByMjAhvdWgbAWWeGE9jwWd
2026-09-22 11:28:57 -04:00

230 lines
7.7 KiB
Go

package cmdhelp
import (
"fmt"
"strings"
"github.com/spf13/cobra"
)
// ValidateExamples is the contractual mechanism by which cmdhelp v0.1
// prevents documentation drift (spec §6, §11 Q5). For every example in
// every command of `doc`:
//
// 1. Tokenize the `cmd` string with a shell-aware splitter.
// 2. Resolve the non-flag tokens against `root`'s live cobra command
// tree. Failure means the example references a command path that
// doesn't exist (typo, deleted command, drift).
// 3. Verify every `--flag` token resolves to a flag on the resolved
// command — local, persistent, or inherited from any parent.
//
// Returns a slice of error strings, one per finding, with the offending
// command path + example index for fast triage. Empty slice means clean.
//
// This is intentionally a function not a test so cmd/pad's own test
// suite (which has access to the real cobra tree) can call it.
// Callers must pass `root` so flag-tree walks can include persistent
// flags from the binary root.
func ValidateExamples(doc *Document, root *cobra.Command) []string {
if doc == nil || root == nil {
return nil
}
var findings []string
for path, cmd := range doc.Commands {
for i, ex := range cmd.Examples {
if ferrs := validateExample(path, i, ex, doc, root); len(ferrs) > 0 {
findings = append(findings, ferrs...)
}
}
}
return findings
}
func validateExample(path string, idx int, ex Example, doc *Document, root *cobra.Command) []string {
tokens, err := shellSplit(ex.Cmd)
if err != nil {
return []string{fmt.Sprintf("%s example[%d]: cannot tokenize %q: %v", path, idx, ex.Cmd, err)}
}
if len(tokens) == 0 {
return []string{fmt.Sprintf("%s example[%d]: empty cmd string", path, idx)}
}
// Token 0 should be the binary's name. Examples that begin with
// something else (e.g. `cat ~/foo.json | jq`) are documentation
// snippets showing related output, not pad invocations the
// validator can check. Skip them rather than fail — the drift
// contract is for pad-invocation drift specifically.
binary := root.Name()
if tokens[0] != binary {
return nil
}
// Pass the full post-binary token stream to cobra's Find. Cobra
// knows each command's flag set and skips flag/value pairs while
// matching subcommand names — so examples that interleave flags
// with subcommands (e.g. `pad --workspace foo item create task`)
// resolve to `item create`, not the root.
target, _, ferr := root.Find(tokens[1:])
if ferr != nil || target == nil {
return []string{fmt.Sprintf("%s example[%d]: command path doesn't resolve: %s", path, idx, ex.Cmd)}
}
// Validate every --flag/-f against target's flag tree.
// Note: flags can appear before the subcommand path on cobra
// (e.g. `pad --workspace foo item create ...`), so scan all tokens.
var findings []string
skipNext := false
for _, t := range tokens[1:] {
if skipNext {
skipNext = false
continue
}
// Cobra treats a lone "--" as the flag terminator: every token
// after it is a positional argument, never a flag, even one that
// LOOKS like an unknown flag (e.g. `-- "-ship it"`). The scan must
// stop here too, before the dash-stripping below — continuing past
// it, as this used to, misread the terminated example's own
// leading-dash text as an unrecognized flag. A lone "-" (the stdin
// convention) is a different token and must NOT stop the scan; it
// falls through to the empty-name check below instead.
if t == "--" {
break
}
if !strings.HasPrefix(t, "-") {
continue
}
// Strip leading dashes and any =value suffix.
name := strings.TrimLeft(t, "-")
if eq := strings.IndexByte(name, '='); eq >= 0 {
name = name[:eq]
}
if name == "" {
continue // lone "-" (stdin convention), not a flag
}
// Walk target up to root, accept the flag if any level has it.
// Boolean flags don't consume the next token; non-bool flags
// do — but we only need the name check for drift detection,
// so don't bother with the value-consumption walk except to
// note that the "next token" might be a value rather than
// another flag (no special handling needed here).
if !flagExists(target, name) {
// Try negate-flag form: --no-<rest>.
if strings.HasPrefix(name, "no-") {
stripped := strings.TrimPrefix(name, "no-")
if flagExists(target, stripped) {
continue
}
}
findings = append(findings, fmt.Sprintf("%s example[%d]: unknown flag --%s in %q", path, idx, name, ex.Cmd))
}
}
return findings
}
// flagExists reports whether `name` is a known flag on cmd or any of
// its ancestors (covers persistent / inherited flags). Both long and
// short forms are checked — pflag's Lookup treats single-character
// names as shorthand and ShorthandLookup as the lookup for them.
func flagExists(cmd *cobra.Command, name string) bool {
for c := cmd; c != nil; c = c.Parent() {
if c.Flags().Lookup(name) != nil {
return true
}
if c.PersistentFlags().Lookup(name) != nil {
return true
}
if len(name) == 1 {
if c.Flags().ShorthandLookup(name) != nil {
return true
}
if c.PersistentFlags().ShorthandLookup(name) != nil {
return true
}
}
}
return false
}
// shellSplit tokenizes a command line in a POSIX-ish way: whitespace
// separates tokens, double-quotes group, single-quotes group (no
// expansion inside), backslash escapes the next rune outside single
// quotes. Stops at the first unquoted pipeline boundary (`|`, `;`,
// `&`, `>`, `<`) so the validator only checks the first command in a
// pipeline. Not a full shell — no $vars, no globbing.
//
// Sufficient for cmdhelp `examples[].cmd` strings, which are expected
// to be runnable invocations of one CLI command (possibly piped to
// another tool — that other tool is the user's shell, not pad's).
func shellSplit(s string) ([]string, error) {
var tokens []string
var cur strings.Builder
var inDQuote, inSQuote, escape bool
flush := func() {
if cur.Len() > 0 {
tokens = append(tokens, cur.String())
cur.Reset()
}
}
for _, r := range s {
switch {
case escape:
cur.WriteRune(r)
escape = false
case r == '\\' && !inSQuote:
escape = true
case r == '"' && !inSQuote:
inDQuote = !inDQuote
case r == '\'' && !inDQuote:
inSQuote = !inSQuote
case (r == ' ' || r == '\t') && !inDQuote && !inSQuote:
flush()
case (r == '|' || r == ';' || r == '&' || r == '>' || r == '<') && !inDQuote && !inSQuote:
// Pipeline boundary — return what we have so far. The rest
// of the string belongs to a different command (or the
// shell), which is out of scope for cmdhelp validation.
flush()
return tokens, nil
default:
cur.WriteRune(r)
}
}
if inDQuote || inSQuote {
return nil, fmt.Errorf("unterminated quote")
}
if escape {
return nil, fmt.Errorf("trailing backslash")
}
flush()
return tokens, nil
}
// ValidateBoolArity asserts that every bool-typed flag in doc obeys
// spec §5.3: bool flags MUST be presence switches. They MUST NOT
// appear in `--flag=value` form anywhere in their command's examples
// (those should be declared as enum-typed instead).
//
// Returns one finding per violation. Empty slice means clean.
func ValidateBoolArity(doc *Document) []string {
if doc == nil {
return nil
}
var findings []string
for path, cmd := range doc.Commands {
for fname, f := range cmd.Flags {
if f.Type != "bool" {
continue
}
needle := "--" + fname + "="
for i, ex := range cmd.Examples {
if strings.Contains(ex.Cmd, needle) {
findings = append(findings,
fmt.Sprintf("%s example[%d]: bool flag --%s appears in valued form (--%s=...) — spec §5.3 requires bool flags to be presence-only; declare as enum: [\"true\",\"false\"] for valued booleans",
path, i, fname, fname))
}
}
}
}
return findings
}