From d1171557ef42669ef70f76aae23a62c896356b72 Mon Sep 17 00:00:00 2001 From: xarmian Date: Tue, 22 Sep 2026 12:48:04 +0000 Subject: [PATCH] fix(cli,skill): pad playbook match needs -- before dash-led text (TASK-3120) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- cmd/pad/cmd_playbook.go | 11 ++++- cmd/pad/cmd_playbook_match_test.go | 67 ++++++++++++++++++++++++++++++ skills/pad/SKILL.md | 2 +- 3 files changed, 78 insertions(+), 2 deletions(-) diff --git a/cmd/pad/cmd_playbook.go b/cmd/pad/cmd_playbook.go index 9bc730c6d..8c22ba2b3 100644 --- a/cmd/pad/cmd_playbook.go +++ b/cmd/pad/cmd_playbook.go @@ -249,7 +249,16 @@ of them. Read-only and side-effect-free — nothing is stored or enqueued. Refuses with 404 (decision_provider_unavailable) when no provider is configured for this instance — callers should fall back to slug/trigger -routing rather than treating that as a hard failure.`, +routing rather than treating that as a hard failure. + +Text is user speech and may start with "-" (a dash-led sentence, or literal +text like "-ship it"); the flag parser reads a leading "-" as a flag, so put +"--" before the text to stop that (BUG-3142 tracks this for the CLI/MCP +stdio surface generally — this command's own help just tells you the +workaround): + +Example: + pad playbook match -- "-ship it"`, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { client, _ := getClient() diff --git a/cmd/pad/cmd_playbook_match_test.go b/cmd/pad/cmd_playbook_match_test.go index e7452981e..18080cac3 100644 --- a/cmd/pad/cmd_playbook_match_test.go +++ b/cmd/pad/cmd_playbook_match_test.go @@ -118,3 +118,70 @@ func TestPlaybookMatchCommand_NoneAndJSON(t *testing.T) { t.Errorf("json choice = %v, want none", raw["choice"]) } } + +// TestPlaybookMatchCommand_LeadingDashNeedsDoubleDash pins the codex-review +// finding (BUG-3142's class, this command's own instance): text starting +// with "-" is parsed as a flag by cobra's default flag scanner, both here +// and over stdio MCP (which shells out to this same command) — so text like +// "-ship it" must be preceded by "--" to reach the positional arg intact. +// BOTH directions are asserted: without "--" the command must FAIL (a +// silently-mistreated dash-led argument would be worse than a clear error), +// and with "--" the exact text — dash included — must reach MatchPlaybook. +func TestPlaybookMatchCommand_LeadingDashNeedsDoubleDash(t *testing.T) { + var lastText string + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/workspaces/ws/playbooks/match", func(w http.ResponseWriter, r *http.Request) { + var body map[string]any + _ = json.NewDecoder(r.Body).Decode(&body) + lastText, _ = body["text"].(string) + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{ + "choice": "none", "confidence": 0.5, "probabilities": map[string]float64{"none": 0.5}, + "options": []map[string]any{}, + }) + }) + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + setTempHomeMain(t) + t.Setenv("PAD_URL", srv.URL) + t.Setenv("PAD_TOKEN", "pad_testtoken") + + origWS, origFormat := workspaceFlag, formatFlag + t.Cleanup(func() { workspaceFlag, formatFlag = origWS, origFormat }) + workspaceFlag, formatFlag = "ws", "json" + + const dashText = "-ship it" + + // WITHOUT "--": cobra reads "-ship" as an unrecognized flag and must + // refuse, not silently swallow or mangle the text. + lastText = "" + cmd := playbookMatchCmd() + cmd.SetArgs([]string{dashText}) + var execErr error + _ = captureStderr(t, func() { + _ = captureStdout(t, func() { + execErr = cmd.Execute() + }) + }) + if execErr == nil { + t.Error("playbook match with a leading-dash arg and no -- succeeded; want a flag-parse error") + } + if lastText != "" { + t.Errorf("server received text=%q without --; want no request to have been sent at all", lastText) + } + + // WITH "--": the dash-led text reaches MatchPlaybook byte-for-byte. + lastText = "" + cmd = playbookMatchCmd() + cmd.SetArgs([]string{"--", dashText}) + _ = captureStderr(t, func() { + _ = captureStdout(t, func() { + if err := cmd.Execute(); err != nil { + t.Fatalf("playbook match -- %q: %v", dashText, err) + } + }) + }) + if lastText != dashText { + t.Errorf("server received text=%q, want %q (the dash preserved)", lastText, dashText) + } +} diff --git a/skills/pad/SKILL.md b/skills/pad/SKILL.md index 84771d108..d48bf5f35 100644 --- a/skills/pad/SKILL.md +++ b/skills/pad/SKILL.md @@ -93,7 +93,7 @@ If the first token isn't a known slug, fall through to the natural-language rout > *"Sounds like the release playbook (PLAYB-1160). Want me to run it? It expects a `version` argument (semver, e.g. `0.5.0`). What version are you cutting?"* -**Matching by typed decision (rung 3, PLAN-3114 / TASK-3120).** Only reach for this when the user is plainly asking for a procedure or workflow to run ("do the thing we always do for X", "is there a playbook for this?") AND rungs 1-2 above found nothing — never call it on ordinary conversation, since every call spends real provider money. Call `pad playbook match ""` (CLI) or `pad_playbook` with `action: match, text: ""` (MCP), passing the user's own words. A 404 (no provider configured) or any other error falls back SILENTLY to today's behavior — don't mention the attempt, just continue as if this rung didn't exist. A `choice` of `"none"` means the same thing: say nothing about it and move on. Any match the provider DOES return — whatever its confidence — is an OFFER, never an auto-run, exactly like the trigger rung above: +**Matching by typed decision (rung 3, PLAN-3114 / TASK-3120).** Only reach for this when the user is plainly asking for a procedure or workflow to run ("do the thing we always do for X", "is there a playbook for this?") AND rungs 1-2 above found nothing — never call it on ordinary conversation, since every call spends real provider money. Call `pad playbook match -- ""` (CLI — the `--` matters: user speech may start with `-`, e.g. "-ship it", which the flag parser otherwise reads as a flag; BUG-3142) or `pad_playbook` with `action: match, text: ""` (MCP, unaffected — it's a structured param, not a CLI token), passing the user's own words. A 404 (no provider configured) or any other error falls back SILENTLY to today's behavior — don't mention the attempt, just continue as if this rung didn't exist. A `choice` of `"none"` means the same thing: say nothing about it and move on. Any match the provider DOES return — whatever its confidence — is an OFFER, never an auto-run, exactly like the trigger rung above: > *"Sounds like PLAYB-5 (Ship a release) — want me to run it?"*