mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-10-25 22:10:25 +00:00
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
This commit is contained in:
+10
-1
@@ -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()
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
|
||||
+1
-1
@@ -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 "<text>"` (CLI) or `pad_playbook` with `action: match, text: "<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 -- "<text>"` (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: "<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?"*
|
||||
|
||||
|
||||
Reference in New Issue
Block a user