Skip to content

fix(cli): emit JSON dry-run envelope from learn-loop commands - #3832

Open
npwalker wants to merge 2 commits into
mvanhorn:mainfrom
npwalker:fix/learn-dry-run-json
Open

fix(cli): emit JSON dry-run envelope from learn-loop commands#3832
npwalker wants to merge 2 commits into
mvanhorn:mainfrom
npwalker:fix/learn-dry-run-json

Conversation

@npwalker

@npwalker npwalker commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Intent

Every learn-loop command short-circuited --dry-run with a bare return nil, so teach --dry-run --json — and teach-lookup, teach-pattern, playbook amend, and the rest — wrote nothing at all to stdout. Live dogfood's json_fidelity check reads empty stdout as a broken command, so the same four rows failed in every printed CLI that ships the loop.

Issue: Closes #3828

Approach

Add one emitted helper next to dryRunOK rather than open-coding an envelope at 14 call sites:

func writeDryRun(w io.Writer, flags *rootFlags, action string) error

Under --json it encodes {"dry_run":true,"action":"teach","would":"run teach; no changes made"}; otherwise it prints dry-run: would run teach; no changes made. Human mode previously printed nothing for these commands, so this is added text, not changed text.

Callers pass cmd.OutOrStdout() (matching printJSONFiltered) so the report follows a redirected writer. All 14 learn-loop dry-run guards (teach, teach-lookup, teach-pattern, teach-playbook, playbook amend, playbook list, recall, learnings list/forget/stats/candidates/confirm/reject/purge) now return writeDryRun(cmd.OutOrStdout(), flags, "<command name>"). The action strings are command names only — no domain vocabulary — so the loop stays domain-neutral.

The hand-written-command guidance in skills/printing-press/SKILL.md moves to the same shape (canonical RunE snippet, the three worked examples, and the emitted-helper list), so novel commands do not reintroduce the silent branch and hit the same json_fidelity failure.

Scope

Primary area: internal/generator/templates/learn** plus helpers.go.tmpl, and the printing-press skill.

Why this belongs in this repo: the silent dry-run branch is emitted by the machine into every learn-enabled printed CLI, so fixing it in one printed CLI would not compound. The helper is generic (action string), carries no API-specific text, and is available to hand-written novel commands in any print.

Risk

  • Commands that previously printed nothing under --dry-run now print one line in human mode and one JSON object in machine mode. Any downstream consumer asserting empty stdout for a learn-loop dry-run would see a diff — none exist in this repo, and empty stdout was the bug.
  • writeDryRun is emitted into non-learn CLIs too, where nothing calls it. That matches the existing posture of dryRunOK and boundCtx, which are already emitted unused in a plain print for hand-written commands to pick up; the dogfood dead_functions check still reports dead: 0.
  • No MCP surface, auth, publish, verifier, or release behavior changes.

Output Contract

Templates and generated files change, so the goldens moved. Goldens were updated intentionally; the entire diff is the new behavior and nothing else:

  • .../internal/cli/helpers.go (3 cases): adds the dryRunResult type + writeDryRun func, and updates the dryRunOK doc-comment example.
  • generate-learn-loop-api/.../teach.go, learnings_candidates.go, learnings_stats.go: each return nil inside a dryRunOK guard becomes return writeDryRun(flags, "<command name>"). No other lines move.
  • generate-golden-api/dogfood.json: dead_functions.total 82 -> 83 (the new helper); dead stays 0.

Generated-output evidence: a new test builds a learn-enabled CLI and executes the real binary, asserting the decoded envelope for all 14 commands plus the human-mode prose, rather than asserting on template text.

Verification

  • go test ./... -timeout 15m — generator package green (ok ... 446s). One unrelated flake, TestDiscoverPathParamProbes_WalksNestedTree in internal/pipeline, tripped a timing assertion under parallel load and passes on isolated re-run; it is untouched by this change.

  • scripts/golden.sh verify — green after the intentional update (32/32).

  • scripts/verify-generator-output.sh — passed for the 10 default cases, plus an explicit run of generate-learn-loop-api (generate + tidy + build of the emitted module).

  • bash scripts/verify-learn-purity.sh — 52 learn templates passed.

  • Review round 2 (commit 33fe11f3, both Greptile P1s): writeDryRun moved to an io.Writer parameter, and the skill's worked examples no longer print prose ahead of the envelope. Re-ran golden update+verify (32/32), verify-learn-purity.sh, verify-generator-output.sh generate-learn-loop-api, the decoy-home field proof, the new tests, and golangci-lint run ./internal/generator/.

  • golangci-lint run ./internal/generator/ — 0 issues.

  • Field proof: generated a learn-loop CLI into a decoy HOME/USERPROFILE/XDG_* profile, built it, and ran each command with --dry-run --json and --dry-run; every command returned exit 0 with the expected envelope / prose.

  • New internal/generator/learn_dry_run_json_test.go:

    • TestLearnDryRunEmitsJSONEnvelope — builds the generated binary, runs all 14 learn-loop commands with --dry-run --json, decodes stdout and asserts dry_run, action, would; a final subtest asserts human mode prints prose and is not valid JSON.
    • TestLearnTemplatesHaveNoSilentDryRunReturns — asserts the emitted helpers.go defines writeDryRun with the dry_run tag and that no learn CLI file retains a silent dryRunOK -> return nil branch, so a new command cannot regress it.
  • Generator/template change: verified generated output, including emitted-code assertions or compiled generated CLI output

  • Generator/template change: covered the affected fallback or variant shape, not only happy-path fixtures

  • Generator/template change: checked emitted definitions and call sites for matching gates

On the last two: every one of the 14 emitted call sites is exercised by the test (write commands, read commands, and the mcp:hidden / typed-exit-code variants), and the non-learn fork is covered by the golden helpers.go cases proving the helper is emitted and the CLIs still generate and build without it being called.

AI / Automation Disclosure

  • No AI or automation was used
  • Human-reviewed: AI or automation was used, and a human reviewed the work for intent, fit, and obvious issues before submission
  • AI-reviewed only: an AI agent reviewed the work, but no human reviewed it before submission
  • Fully automated: generated and submitted without human review for this specific change

🤖 Generated with Claude Code

https://claude.ai/code/session_01BRGzpiy4LuvcAF5QG6dYJc

Every learn-loop command short-circuited --dry-run with a bare `return
nil`, so `teach --dry-run --json` (and teach-lookup, teach-pattern,
playbook amend, and the rest) wrote nothing at all to stdout. Live
dogfood's json_fidelity check reads empty stdout as a broken command, so
the same four rows failed in every printed CLI that ships the loop.

Add a `writeDryRun(flags, action)` helper alongside `dryRunOK` in the
emitted helpers and route all 14 learn-loop dry-run guards through it. In
--json mode it writes `{"dry_run":true,"action":...,"would":...}`; in
human mode it prints a prose line instead of exiting silently. The action
strings are command names only, so the loop stays domain-neutral.

The hand-written-command guidance in the printing-press SKILL.md moves to
the same shape, so novel commands do not reintroduce the silent branch.

Refs mvanhorn#3828
Closes mvanhorn#3828

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BRGzpiy4LuvcAF5QG6dYJc
@mergify

mergify Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Merge Protections

🔴 2 of 2 protections blocking · waiting on 🙋 you

Protection Waiting on
🔴 require-ready-label-and-ci 🙋 you
🔴 🚦 Auto-queue 🙋 you

🔴 require-ready-label-and-ci

Waiting for any of

  • label = ready-to-merge
  • title ~= ^chore\(main\): release
This rule is failing.
  • any of:
    • label = ready-to-merge
    • all of:
      • head = release-please--branches--main
      • title ~= ^chore\(main\): release
  • #changes-requested-reviews-by = 0
  • #review-threads-unresolved = 0
  • check-success = build-and-test
  • check-success = generated-test
  • check-success = go-lint
  • check-success = golden
  • check-success = pr-title
  • check-success = test
  • any of:
    • -files ~= ^(\.github/workflows/|\.github/scripts/|scripts/|\.github/CODEOWNERS$)
    • approved-reviews-by = mvanhorn
    • approved-reviews-by = tmchow
    • author = mvanhorn
    • author = tmchow
  • any of:
    • check-success = Greptile Review
    • check-neutral = Greptile Review
    • check-skipped = Greptile Review
    • head ~= ^mergify/merge-queue/
    • label = queued
    • all of:
      • head = release-please--branches--main
      • title ~= ^chore\(main\): release

🔴 🚦 Auto-queue

Waiting for

  • label = ready-to-merge
This rule is failing.

When all merge protections are satisfied and these conditions match, this pull request will be queued automatically.

  • label = ready-to-merge

Comment thread internal/generator/templates/helpers.go.tmpl
Comment thread skills/printing-press/SKILL.md Outdated
@greptile-apps

greptile-apps Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR makes learn-loop dry runs emit a structured JSON envelope or human-readable status line.

  • Routes all learn-loop dry-run branches through a writer-aware output helper.
  • Updates generated goldens and hand-written command guidance.
  • Adds integration coverage that builds and executes a generated learn-enabled CLI.

Confidence Score: 4/5

The PR appears safe to merge, with one non-blocking inconsistency in its copyable helper documentation.

The runtime dry-run paths now use the redirected Cobra writer and the skill examples avoid mixed output, but the adjacent emitted helper example no longer matches the helper signature.

Files Needing Attention: internal/generator/templates/helpers.go.tmpl

Important Files Changed

Filename Overview
internal/generator/templates/helpers.go.tmpl Adds the writer-aware dry-run envelope helper used by generated commands.
internal/generator/learn_dry_run_json_test.go Builds a generated CLI and validates JSON and human dry-run output across all learn-loop commands.
skills/printing-press/SKILL.md Updates canonical hand-written command guidance to delegate dry-run output entirely to the helper.
internal/generator/templates/teach.go.tmpl Routes teach, recall, learning-list, forget, pattern, and lookup dry-run branches through the new helper.
internal/generator/templates/learnings_candidates.go.tmpl Routes candidate list, confirmation, rejection, and purge dry runs through the new helper.

Reviews (2): Last reviewed commit: "fix(cli): route dry-run envelope through..." | Re-trigger Greptile

Address review on the dry-run envelope: writeDryRun wrote straight to
os.Stdout, so a command with a redirected Cobra writer (or an in-process
capture) lost the report. Take an io.Writer and pass cmd.OutOrStdout()
from all 14 learn-loop call sites, matching printJSONFiltered.

The skill's worked examples also printed their own prose line before
calling writeDryRun, which would interleave with the envelope and make
stdout unparseable under --json. writeDryRun now owns the whole dry-run
output in those snippets.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BRGzpiy4LuvcAF5QG6dYJc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

learn-loop templates: teach/teach-lookup/teach-pattern/playbook amend print nothing under --dry-run --json, failing json_fidelity in every printed CLI

1 participant