fix: honor --out-file for protect exports and scaffolds (#357)

[k0shir0]

Fixes #357

Problem
6 sites wrote directly to os.Stdout / fmt.Print, so --out-file created an empty file while the payload went to stdout:

• protect_helpers.go:35,39 yaml/json.NewEncoder(os.Stdout)
• pro_platform_helpers.go:282,289 same
• protect_analytics.go:351 fmt.Print
• protect_ulf.go:324 fmt.Print

Fix

• printExport(cliCtx, data) and printScaffold(cliCtx, v) now take cliCtx and use writerFor(cliCtx) (honours --out-file via the formatter's writer)
• protect_analytics/ulf fmt.Print -> Fprint(writerFor(cliCtx), ...)
• updated all 41 printExport and 7 printScaffold call sites
• pro_blueprints scaffold cmd now takes cliCtx

Verification

• go vet ./internal/commands clean, go build ./... clean, gofmt clean
• buffer test: printExport/printScaffold with formatter writer set to bytes.Buffer goes to buffer, not stdout
• go test -run TestBackupListResources_HonoursOutFile (same regression class) pass
• no remaining os.Stdout in export/scaffold paths (non-generated, non-completion)
• pre-existing TestUpdateCacheRoundTrip fails on main on Windows (0600 vs 0666) — not this change

[neilmartin83]

Warning

⚠️ Needs changes. Routes printExport, printScaffold and two Protect YAML exports through writerFor(cliCtx) so --out-file is honoured. Verified working end to end on the paths it touches.
Blocking: (1), (2). See the collapsed section for 2 nice-to-have suggestions.

Rating: 3/5

• Would be a 5 with the four remaining hand-written emitters in (1) routed the same way, plus one committed --out-file test and the source guard in (2).
• Coverage: silent-failure-hunter, usability-reviewer and devil-advocate have never run on this PR. The rating reflects the dimensions that were searched, not the whole diff.

Findings

⚠️ IMPORTANT (1) (correctness) — internal/commands/pro_device_actions.go:256: four more hand-written emitters still ignore --out-file

The six sites issue #357 enumerated are fixed. Four hand-written, non-generated payload emitters in the same class are not: pro comp erase --scaffold and pro md erase --scaffold (pro_device_actions.go:256, :477), and pro blueprints components configuration-profile / configuration-profile-plist (pro_blueprints.go:898, :1142). Each writes its whole payload with a bare fmt.Println, which a grep for os.Stdout does not see — so the PR body's "no remaining os.Stdout in export/scaffold paths (non-generated, non-completion)" reads as complete while these four are not. The plist command is registered on the line immediately after the one this diff changed (pro_blueprints.go:643), and its constructor at :1085 still takes no cliCtx — the exact shape newBlueprintsComponentsScaffoldCmd had before this PR. All four have cliCtx in scope or one parameter away.

Failure scenario: jamf-cli pro comp erase --scaffold --out-file body.json → body.json is 0 bytes, the JSON goes to stdout, exit 0. Reproduced at 4715dd03 in the same run where the fixed paths correctly wrote 83 and 56 bytes with stdout empty.

Suggested fix:

--- a/internal/commands/pro_device_actions.go
+++ b/internal/commands/pro_device_actions.go
 			if scaffold {
-				fmt.Println(`{
+				fmt.Fprintln(writerFor(cliCtx), `{
   "pin": "123456"
 }`)
 				return nil
 			}

Same change at pro_device_actions.go:477. In pro_blueprints.go, fmt.Println(string(component)) → fmt.Fprintln(writerFor(cliCtx), string(component)) at :898 and :1142, and give newBlueprintsComponentsConfigProfilePlistCmd a cliCtx *registry.CLIContext parameter, passed from :643.

Fixed when: all four commands write their payload to the --out-file target with stdout empty, newBlueprintsComponentsConfigProfilePlistCmd takes cliCtx, and no hand-written internal/commands payload path writes to stdout directly.

⚠️ IMPORTANT (2) (test-coverage, via test-quality-reviewer) — internal/commands/platform_testhelpers_test.go:65: no committed test, and the existing tests are structurally blind to this routing

The diff has no test file. The tests that look like coverage cannot catch a regression: newTestPlatformContext supplies Output: &captureOutput{}, which can never satisfy writerFor's *cliOutput type assertion (doctor.go:388), so writerFor always returns os.Stdout and TestCBScaffold_StaticTemplate (pro_platform_test.go:518) passes identically with or without this change. TestComputerEraseScaffold / TestMobileEraseScaffold assert only err == nil. TestBackupListResources_HonoursOutFile, cited in the PR body, is on a different path — it calls cliCtx.Output.PrintRaw, already routed before this PR, and never reaches these helpers. Both fmt.Fprint(writerFor(...)) sites have no RunE test at all. Issue #357 asked for the stdout guard alongside the fix ("Widen it here"); note the guard it names does not exist yet, because #349 is still an open issue.

Failure scenario: revert either helper's writer to os.Stdout → go test ./internal/commands/... stays green, because no test binds a real *cliOutput writer to these helpers; the defect then ships again exactly as it did through #349 and #357.

Suggested fix: reuse newTestCtx (config_subcommands_test.go:22) and captureStdout, which already exist in the package:

func TestPrintExport_RoutesToInjectedWriterNotStdout(t *testing.T) {
	for _, format := range []string{"json", "yaml"} {
		t.Run(format, func(t *testing.T) {
			old := outputFmt
			outputFmt = format
			defer func() { outputFmt = old }()
			var buf bytes.Buffer
			ctx := newTestCtx(&buf, format)
			stdout := captureStdout(t, func() {
				if err := printExport(ctx, map[string]any{"name": "widget"}); err != nil {
					t.Fatalf("printExport: %v", err)
				}
			})
			if stdout != "" {
				t.Errorf("printExport leaked to stdout: %q", stdout)
			}
			if buf.Len() == 0 {
				t.Fatal("printExport wrote nothing to the --out-file writer")
			}
		})
	}
}

Same shape for printScaffold. Then add the source sweep issue #357 asked for: refuse a bare fmt.Print* or os.Stdout write in a non-generated internal/commands payload path, allowlisting completion.go and the stderr sites.

Fixed when: a committed test fails if either helper is reverted to os.Stdout, both the json and yaml branches are covered, and a source sweep refuses a new bare stdout write under internal/commands.

This covers all findings — addressing the above gets this PR to merge-ready.

Nice-to-have suggestions (2 items)

💡 NICE-TO-HAVE (3) (code-quality) — internal/commands/pro_platform_helpers.go:277: doc comment still says "to stdout"

// printScaffold marshals the given value to stdout, respecting the -o flag. is now wrong — the destination is the formatter's writer.

Fixed when: the comment names writerFor / --out-file instead of stdout.

💡 NICE-TO-HAVE (4) (documentation) — docs/solutions/conventions/output-flag-matrix-2026-05-08.md:70: rule 2 predates writerFor and under-scopes itself

Rule 2 limits "route through the formatter, not fmt.Fprintf on os.Stdout" to "commands with a -v mode like doctor and version". This PR generalises it to every hand-written payload emitter. Naming writerFor(cliCtx) there and dropping the -v scoping is what stops the next one repeating (1).

Fixed when: rule 2 names writerFor(cliCtx) and applies to any hand-written output path.

Review coverage and scope

• Design and architecture: required cliCtx param, not a global read — see What's done well
• Correctness: writerFor fallback traced; four unfixed siblings are (1)
• Test coverage: none in diff; existing scaffold tests on the fallback branch, (2)
• Reliability: outFileHandle is an unbuffered *os.File, closed in PersistentPostRunE
• Code quality: 44 call-site edits are pure parameter threading, no behaviour change
• Documentation currency: output-flag-matrix-2026-05-08.md rule 2 is (4)
• Project rules compliance: CLAUDE.md, docs/solutions/conventions/
• [na] Security, performance, simplification, frontend, cross-repo contracts.

Diff: 26 files, +60/−58, head 4715dd03. Also read doctor.go, root.go, output/output.go, platform_testhelpers_test.go, pro_backup_test.go and all four generator templates. Built the binary and ran 9 commands through --out-file in -o json and -o yaml; go build, go vet, gofmt and go test ./internal/commands/... all clean. Lanes run: test-quality-reviewer. Never run: silent-failure, usability, devil-advocate — skipped; security, performance, scope, fidelity — inapplicable.

What's done well

✅ Threading cliCtx as a required parameter rather than reading a package-level global is the right call: a new printExport / printScaffold call site now cannot silently forget the writer, because it will not compile. That is the durable half of the fix, and it is why (1) is a sweep rather than a redesign.

✅ Choosing the existing writerFor helper over a second mechanism keeps doctor, version and these four helpers on one routing path.

Generated by pr-review v1.33.0, a Jamf Claude Code skill