Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 107 additions & 0 deletions internal/generator/learn_dry_run_json_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
package generator

import (
"encoding/json"
"os/exec"
"path/filepath"
"strings"
"testing"

"github.com/mvanhorn/cli-printing-press/v4/internal/naming"
"github.com/stretchr/testify/require"
)

// learnDryRunCommands are the learn-loop commands whose --dry-run branch
// short-circuits before any store work, paired with the action name the
// emitted envelope must report.
var learnDryRunCommands = []struct {
args []string
action string
}{
{args: []string{"teach"}, action: "teach"},
{args: []string{"teach-lookup"}, action: "teach-lookup"},
{args: []string{"teach-pattern"}, action: "teach-pattern"},
{args: []string{"teach-playbook"}, action: "teach-playbook"},
{args: []string{"playbook", "amend"}, action: "playbook amend"},
{args: []string{"playbook", "list"}, action: "playbook list"},
{args: []string{"recall", "example query"}, action: "recall"},
{args: []string{"learnings", "list"}, action: "learnings list"},
{args: []string{"learnings", "stats"}, action: "learnings stats"},
{args: []string{"learnings", "candidates"}, action: "learnings candidates"},
{args: []string{"learnings", "confirm", "1"}, action: "learnings confirm"},
{args: []string{"learnings", "reject", "1"}, action: "learnings reject"},
{args: []string{"learnings", "purge"}, action: "learnings purge"},
{args: []string{"learnings", "forget", "example query"}, action: "learnings forget"},
}

// TestLearnDryRunEmitsJSONEnvelope pins that a --dry-run short-circuit stays
// machine-legible. Returning nil silently left --json callers with empty
// stdout, which the live-dogfood json_fidelity check reads as a broken
// command rather than a deliberate no-op.
func TestLearnDryRunEmitsJSONEnvelope(t *testing.T) {
t.Parallel()

apiSpec := smallReadWriteSyncableOutputSpec("learn-dry-run")
apiSpec.Learn.Enabled = true
_, binaryPath := buildGeneratedBinary(t, apiSpec)

for _, tc := range learnDryRunCommands {
t.Run(strings.Join(tc.args, " "), func(t *testing.T) {
args := append(append([]string{}, tc.args...), "--dry-run", "--json")
stdout := requireLearnDryRunOutput(t, binaryPath, args)

var envelope struct {
DryRun bool `json:"dry_run"`
Action string `json:"action"`
Would string `json:"would"`
}
require.NoError(t, json.Unmarshal([]byte(stdout), &envelope), "stdout: %q", stdout)
require.True(t, envelope.DryRun)
require.Equal(t, tc.action, envelope.Action)
require.NotEmpty(t, envelope.Would)
})
}

// The human path must report the skipped action too, as prose rather than
// the machine envelope.
t.Run("human mode", func(t *testing.T) {
stdout := requireLearnDryRunOutput(t, binaryPath, []string{"teach", "--dry-run"})
require.Contains(t, stdout, "dry-run")
require.Contains(t, stdout, "teach")
require.False(t, json.Valid([]byte(stdout)), "human mode must not emit the JSON envelope")
})
}

// TestLearnTemplatesHaveNoSilentDryRunReturns guards the emitted call sites:
// every learn-loop --dry-run guard must hand off to writeDryRun, so a new
// command cannot reintroduce the silent short-circuit.
func TestLearnTemplatesHaveNoSilentDryRunReturns(t *testing.T) {
t.Parallel()

apiSpec := smallReadWriteSyncableOutputSpec("learn-dry-run-sites")
apiSpec.Learn.Enabled = true
outputDir := filepath.Join(t.TempDir(), naming.CLI(apiSpec.Name))
require.NoError(t, New(apiSpec, outputDir).Generate())

helpers := readGeneratedFile(t, outputDir, "internal", "cli", "helpers.go")
require.Contains(t, helpers, "func writeDryRun(w io.Writer, flags *rootFlags, action string) error")
require.Contains(t, helpers, `json:"dry_run"`)

for _, name := range []string{"teach.go", "teach_playbook.go", "learnings_candidates.go", "learnings_stats.go"} {
src := readGeneratedFile(t, outputDir, "internal", "cli", name)
require.Contains(t, src, "return writeDryRun(cmd.OutOrStdout(), flags,", "%s must report its dry-run short-circuit", name)
require.NotContains(t, src, "if dryRunOK(flags) {\n\t\t\t\treturn nil\n", "%s still has a silent dry-run return", name)
}
}

func requireLearnDryRunOutput(t *testing.T, binaryPath string, args []string) string {
t.Helper()

cmd := exec.Command(binaryPath, args...)
cmd.Env = sandboxHomeEnv(t)
var stdout, stderr strings.Builder
cmd.Stdout = &stdout
cmd.Stderr = &stderr
require.NoError(t, cmd.Run(), "stdout:\n%s\nstderr:\n%s", stdout.String(), stderr.String())
return strings.TrimSpace(stdout.String())
}
21 changes: 20 additions & 1 deletion internal/generator/templates/helpers.go.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,7 @@ func detectPartialFailure(data []byte) *partialFailureReport {
// return cmd.Help()
// }
// if dryRunOK(flags) {
// return nil
// return writeDryRun(flags, "<command name>")
// }
// // ... real work ...
// }
Expand All @@ -496,6 +496,25 @@ func dryRunOK(flags *rootFlags) bool {
return flags != nil && flags.dryRun
}

type dryRunResult struct {
DryRun bool `json:"dry_run"`
Action string `json:"action"`
Would string `json:"would"`
}

// writeDryRun ends a --dry-run short-circuit by reporting the action that was
// skipped. Returning silently leaves a --json caller with empty stdout, which
// is indistinguishable from a broken command rather than a deliberate no-op.
// Callers pass cmd.OutOrStdout() so the report follows any redirected writer.
func writeDryRun(w io.Writer, flags *rootFlags, action string) error {
would := "run " + action + "; no changes made"
if flags != nil && flags.asJSON {
return json.NewEncoder(w).Encode(dryRunResult{DryRun: true, Action: action, Would: would})
}
_, err := fmt.Fprintf(w, "dry-run: would %s\n", would)
return err
}
Comment thread
npwalker marked this conversation as resolved.

// boundCtx applies the root --timeout flag to hand-written command work that
// does not go through the generated internal/client.Client. Generated endpoint
// commands already pass flags.timeout into client.New; sibling typed clients
Expand Down
8 changes: 4 additions & 4 deletions internal/generator/templates/learnings_candidates.go.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ alter command behavior or recall's verified results on their own.`,
Annotations: map[string]string{"mcp:read-only": "true"},
RunE: func(cmd *cobra.Command, args []string) error {
if dryRunOK(flags) {
return nil
return writeDryRun(cmd.OutOrStdout(), flags, "learnings candidates")
}
s, err := store.OpenWithContext(cmd.Context(), learnDBPath(dbPath))
if err != nil {
Expand Down Expand Up @@ -206,7 +206,7 @@ Usage errors (unknown id, non-open status) exit 2.`,
Annotations: map[string]string{"pp:typed-exit-codes": "0,2", "mcp:local-write": "true"},
RunE: func(cmd *cobra.Command, args []string) error {
if dryRunOK(flags) {
return nil
return writeDryRun(cmd.OutOrStdout(), flags, "learnings confirm")
}
id, err := parseCandidateID(args)
if err != nil {
Expand Down Expand Up @@ -411,7 +411,7 @@ Usage errors (unknown id, no-reject-path candidates) exit 2.`,
Annotations: map[string]string{"pp:typed-exit-codes": "0,2"},
RunE: func(cmd *cobra.Command, args []string) error {
if dryRunOK(flags) {
return nil
return writeDryRun(cmd.OutOrStdout(), flags, "learnings reject")
}
id, err := parseCandidateID(args)
if err != nil {
Expand Down Expand Up @@ -489,7 +489,7 @@ purge also clears stale candidates nobody judged.`,
Annotations: map[string]string{"mcp:hidden": "true"},
RunE: func(cmd *cobra.Command, args []string) error {
if dryRunOK(flags) {
return nil
return writeDryRun(cmd.OutOrStdout(), flags, "learnings purge")
}
s, err := store.OpenWithContext(cmd.Context(), learnDBPath(dbPath))
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion internal/generator/templates/learnings_stats.go.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ local-only: events never leave this machine.`,
Annotations: map[string]string{"mcp:read-only": "true"},
RunE: func(cmd *cobra.Command, args []string) error {
if dryRunOK(flags) {
return nil
return writeDryRun(cmd.OutOrStdout(), flags, "learnings stats")
}
s, err := store.OpenWithContext(cmd.Context(), learnDBPath(dbPath))
if err != nil {
Expand Down
12 changes: 6 additions & 6 deletions internal/generator/templates/teach.go.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ Disabling: pass --no-learn or set ` + noLearnEnvVar + `=true.`,
return nil
}
if dryRunOK(flags) {
return nil
return writeDryRun(cmd.OutOrStdout(), flags, "teach")
}
if strings.TrimSpace(query) == "" {
writeTeachErrLog(fmt.Sprintf("teach: missing --query (args=%v resources=%v)", args, resources))
Expand Down Expand Up @@ -474,7 +474,7 @@ when learnings exist.`,
return cmd.Help()
}
if dryRunOK(flags) {
return nil
return writeDryRun(cmd.OutOrStdout(), flags, "recall")
}
query := strings.Join(args, " ")
envelope := recallEnvelope{
Expand Down Expand Up @@ -697,7 +697,7 @@ func newLearningsListCmd(flags *rootFlags) *cobra.Command {
Annotations: map[string]string{"mcp:read-only": "true"},
RunE: func(cmd *cobra.Command, args []string) error {
if dryRunOK(flags) {
return nil
return writeDryRun(cmd.OutOrStdout(), flags, "learnings list")
}
if warningsOnly {
var filterIDs []string
Expand Down Expand Up @@ -806,7 +806,7 @@ Requires at least one of --resource, --action, or --all.`,
return cmd.Help()
}
if dryRunOK(flags) {
return nil
return writeDryRun(cmd.OutOrStdout(), flags, "learnings forget")
}
query := strings.Join(args, " ")
dbPath = learnDBPath(dbPath)
Expand Down Expand Up @@ -897,7 +897,7 @@ a whole family.`,
Example: ` {{.Name}}-pp-cli teach-pattern --query-template "items in {entity}" --resource-template "GROUP-{entity:category}" --resource-type "items" --entity-kind "category" --strategy substitute`,
RunE: func(cmd *cobra.Command, args []string) error {
if dryRunOK(flags) {
return nil
return writeDryRun(cmd.OutOrStdout(), flags, "teach-pattern")
}
if noLearnActive(flags) {
return nil
Expand Down Expand Up @@ -981,7 +981,7 @@ cannot be taught — they are derived from the canonical input.`,
Example: ` {{.Name}}-pp-cli teach-lookup --kind country --canonical "United States" --value USA`,
RunE: func(cmd *cobra.Command, args []string) error {
if dryRunOK(flags) {
return nil
return writeDryRun(cmd.OutOrStdout(), flags, "teach-lookup")
}
if noLearnActive(flags) {
return nil
Expand Down
6 changes: 3 additions & 3 deletions internal/generator/templates/teach_playbook.go.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ agents can record playbooks without a file on disk.`,
return nil
}
if dryRunOK(flags) {
return nil
return writeDryRun(cmd.OutOrStdout(), flags, "teach-playbook")
}
if strings.TrimSpace(query) == "" {
return usageErr(fmt.Errorf("--query is required"))
Expand Down Expand Up @@ -204,7 +204,7 @@ Disabling: pass --no-learn or set ` + noLearnEnvVar + `=true.`,
return nil
}
if dryRunOK(flags) {
return nil
return writeDryRun(cmd.OutOrStdout(), flags, "playbook amend")
}
if strings.TrimSpace(query) == "" {
writeTeachErrLog(fmt.Sprintf("playbook amend: missing --query (args=%v)", args))
Expand Down Expand Up @@ -286,7 +286,7 @@ func newPlaybookListCmd(flags *rootFlags) *cobra.Command {
Annotations: map[string]string{"mcp:read-only": "true"},
RunE: func(cmd *cobra.Command, args []string) error {
if dryRunOK(flags) {
return nil
return writeDryRun(cmd.OutOrStdout(), flags, "playbook list")
}
dbPath = learnDBPath(dbPath)
s, err := store.OpenWithContext(cmd.Context(), dbPath)
Expand Down
17 changes: 7 additions & 10 deletions skills/printing-press/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -3304,8 +3304,7 @@ func newScanFilterCmd(flags *rootFlags) *cobra.Command {
return cmd.Help()
}
if dryRunOK(flags) {
fmt.Fprintf(cmd.OutOrStdout(), "would scan up to %d pages for matching items\n", maxScanPages)
return nil
return writeDryRun(cmd.OutOrStdout(), flags, "scan")
}
if status == "" {
_ = cmd.Usage()
Expand Down Expand Up @@ -3378,7 +3377,7 @@ RunE: func(cmd *cobra.Command, args []string) error {
return cmd.Help()
}
if dryRunOK(flags) {
return nil
return writeDryRun(cmd.OutOrStdout(), flags, "<command name>")
}
if <required input missing> {
_ = cmd.Usage()
Expand All @@ -3388,7 +3387,7 @@ RunE: func(cmd *cobra.Command, args []string) error {
}
```

Why each branch exists: the `len(args) == 0 && cmd.Flags().NFlag() == 0` branch handles an interactive `<cli> mycommand` help-only invocation without treating help as an error. The `dryRunOK` branch handles verify's `<cli> mycommand <fixture> --dry-run` probes before network or filesystem IO. The required-input branch handles non-help invocations where a mode or output flag is present (`--no-input`, `--agent`, `--json`) but the required ID, query, path, or other command input is still missing. Missing required input must print usage and return `usageErr(...)` so callers get exit code 2 instead of a silent rc=0 skip.
Why each branch exists: the `len(args) == 0 && cmd.Flags().NFlag() == 0` branch handles an interactive `<cli> mycommand` help-only invocation without treating help as an error. The `dryRunOK` branch handles verify's `<cli> mycommand <fixture> --dry-run` probes before network or filesystem IO; it must end in `writeDryRun(cmd.OutOrStdout(), flags, "<command name>")` so `--dry-run --json` still produces a parseable envelope. The required-input branch handles non-help invocations where a mode or output flag is present (`--no-input`, `--agent`, `--json`) but the required ID, query, path, or other command input is still missing. Missing required input must print usage and return `usageErr(...)` so callers get exit code 2 instead of a silent rc=0 skip.

For SQLite-backed novel commands only, add this missing-mirror guard after `dryRunOK(flags)`, after any required-input `usageErr(...)` check, and after `dbPath` is resolved, but before `store.OpenWithContext`, `store.OpenReadOnly`, `sql.Open`, or other SQLite access:

Expand Down Expand Up @@ -3520,6 +3519,7 @@ The generator handles Priority 0 (data layer) and most of Priority 1 (absorbed A
- `printAutoTable(w io.Writer, items []map[string]any) error` - render JSON-like rows as the generated human table format.
- `defaultDBPath(name string) string` - resolve the local SQLite database path for `<name>`.
- `dryRunOK(flags *rootFlags) bool` - detect verify-friendly `--dry-run` short-circuits before network, store, or filesystem work.
- `writeDryRun(w io.Writer, flags *rootFlags, action string) error` - end a `--dry-run` short-circuit with a `{"dry_run":true,"action":...,"would":...}` envelope under `--json` and a prose line otherwise; pass `cmd.OutOrStdout()`. Never `return nil` silently from a dry-run branch: empty stdout under `--json` fails the live-dogfood `json_fidelity` check. Let it own the whole dry-run output — an extra prose write before it makes stdout unparseable under `--json`.
- `boundCtx(parent context.Context, flags *rootFlags) (context.Context, context.CancelFunc)` - apply root `--timeout` to hand-written commands that call sibling typed clients instead of the generated `internal/client`.
- `filterFields(data json.RawMessage, fields string) json.RawMessage` - apply `--select` to a JSON blob.
- `compactFields(data json.RawMessage) json.RawMessage` - apply `--compact` to a JSON blob.
Expand Down Expand Up @@ -3580,8 +3580,7 @@ RunE: func(cmd *cobra.Command, args []string) error {
return cmd.Help()
}
if dryRunOK(flags) {
fmt.Fprintln(cmd.OutOrStdout(), "would fetch <resource>")
return nil
return writeDryRun(cmd.OutOrStdout(), flags, "<command name>")
}
ctx, cancel := boundCtx(cmd.Context(), flags)
defer cancel()
Expand Down Expand Up @@ -3629,8 +3628,7 @@ RunE: func(cmd *cobra.Command, args []string) error {
return cmd.Help()
}
if dryRunOK(flags) {
fmt.Fprintln(cmd.OutOrStdout(), "would fetch <resource> details")
return nil
return writeDryRun(cmd.OutOrStdout(), flags, "<command name>")
}
ctx, cancel := boundCtx(cmd.Context(), flags)
defer cancel()
Expand Down Expand Up @@ -3732,8 +3730,7 @@ RunE: func(cmd *cobra.Command, args []string) error {
return cmd.Help()
}
if dryRunOK(flags) {
fmt.Fprintln(cmd.OutOrStdout(), "would query local store")
return nil
return writeDryRun(cmd.OutOrStdout(), flags, "<command name>")
}
ctx, cancel := boundCtx(cmd.Context(), flags)
defer cancel()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ func rateLimitErr(err error) error { return &cliError{code: 7, err: err} }
// return cmd.Help()
// }
// if dryRunOK(flags) {
// return nil
// return writeDryRun(flags, "<command name>")
// }
// // ... real work ...
// }
Expand All @@ -136,6 +136,25 @@ func dryRunOK(flags *rootFlags) bool {
return flags != nil && flags.dryRun
}

type dryRunResult struct {
DryRun bool `json:"dry_run"`
Action string `json:"action"`
Would string `json:"would"`
}

// writeDryRun ends a --dry-run short-circuit by reporting the action that was
// skipped. Returning silently leaves a --json caller with empty stdout, which
// is indistinguishable from a broken command rather than a deliberate no-op.
// Callers pass cmd.OutOrStdout() so the report follows any redirected writer.
func writeDryRun(w io.Writer, flags *rootFlags, action string) error {
would := "run " + action + "; no changes made"
if flags != nil && flags.asJSON {
return json.NewEncoder(w).Encode(dryRunResult{DryRun: true, Action: action, Would: would})
}
_, err := fmt.Fprintf(w, "dry-run: would %s\n", would)
return err
}

// boundCtx applies the root --timeout flag to hand-written command work that
// does not go through the generated internal/client.Client. Generated endpoint
// commands already pass flags.timeout into client.New; sibling typed clients
Expand Down
Loading
Loading