fix: feedback review follow-ups from #95

[Fermionic-Lyu]

Addresses the three non-blocking suggestions from the #95 review:

1. --json validation errors are now JSON. Previously a bad/missing flag unwound through guard→die as a plaintext stderr line while every success/transport path returned JSON. Now --json gets {status:"error", submitted:false, error} on stdout with the same self-teaching enum message; exit code stays 1 (validation is fixable by the agent).
2. --file guards: reject files >256KB before reading (detail caps at 4000 chars — anything huge is a wrong path or an archive) and reject binary content (NUL check) instead of redact/truncating garbage.
3. redact.ts: comment documenting the accepted IPv4 over-scrub asymmetry (4-part version strings can be scrubbed; over-scrub is recoverable noise, under-scrub is a leak). Same comment mirrored to the ingest's copy in InsForge/insta-feedback.

npm run typecheck && npm test green (308).

🤖 Generated with Claude Code

---

Summary by cubic

Unifies JSON-mode validation output in the feedback command and hardens --file handling to prevent garbage input. Old: --json validation errors were plaintext on stderr; new: a JSON object {status:'error', submitted:false, error} is printed to stdout with exit code 1. Also rejects files >256KB and binary content before reading; redact.ts only adds a comment about accepted IPv4 over-scrub.

• In --json mode, validation errors no longer throw; the command prints the JSON error on stdout, sets process.exitCode = 1, and returns. Non-JSON mode behavior is unchanged (throws on bad flags).
• --file is size-checked before read and rejected if binary (NUL check). Error messages name the path and reason.

Migration

• If automation previously parsed stderr for --json validation errors, switch to parsing stdout JSON and check status:'error' and submitted:false with exit code 1.
• Ensure files passed via --file are text and under 256KB; trim or sample logs before sending.

^{Written for commit f7b2af7. Summary will update on new commits.}

[jwfing]

Review: fix — feedback review follow-ups from #95

Summary: A tight, well-tested follow-up that fixes the three non-blocking suggestions from #95 (JSON-channel validation errors, --file size/binary guards, and a documenting comment on the IPv4 over-scrub) — no Critical issues, safe to merge.

Requirements context

No /docs/superpowers/ (or docs/specs/) spec matched this PR — assessing against the PR description and the parent #95 review intent alone. The PR is scoped precisely to the three named follow-ups and adds nothing beyond them, which is correct for a review-follow-up change.

Findings

Critical

(none)

Suggestion

• Functionality / defense-in-depth — src/commands/feedback.ts:143-148. The size guard relies on statSync(opts.file).size, which reports 0 for character/FIFO special files and follows symlinks. A --file pointed at e.g. /dev/zero (or a symlink to one) passes the MAX_FILE_BYTES check and then readFileSync(..., 'utf8') reads unbounded, hanging/OOMing the CLI. This is pre-existing (before this PR readFileSync ran with no guard at all, so the PR is a net improvement and not a regression), but since this change is specifically the place that adds a size ceiling, a !statSync(...).isFile() reject would close the one path the byte-count guard can't. Non-blocking — an agent or human aiming --file at a device node on their own box is a remote scenario.

Information

• Software engineering — test/feedback.test.ts:140-142. Nice catch adding afterEach(() => { process.exitCode = 0 }): the new --json validation test intentionally sets process.exitCode = 1, and without the reset a green run would still exit the vitest process non-zero. Calling this out as a positive, not a defect.
• Security — src/commands/feedback.ts:148. Binary detection keys on a NUL byte only, so a non-NUL binary with invalid UTF-8 would be read (as U+FFFD replacement chars) and sent. Acceptable — the 256 KB ceiling and the 4000-char detail truncation bound the blast radius, and NUL-sniffing is the standard cheap heuristic. No action needed.
• Functionality — src/commands/feedback.ts:149-152. The msg.startsWith('--file') ? msg : ... re-wrap correctly avoids the --file <path>: --file <path> is … double-prefix for the guards' own messages while still prefixing statSync/readFileSync errors (ENOENT, EISDIR). Verified against the guard→onError→die path in src/index.ts:31-38 — the non---json branch still rethrows and reaches die as plaintext stderr exactly as before, so existing behavior is preserved.

Dimension coverage

• Software engineering: Good. New tests exercise the changed behavior directly (text/oversized/binary --file, and --json validation → JSON-on-stdout + exit 1). Style, comments, and throw-plain-Error convention match the surrounding file.
• Functionality: All three follow-ups land correctly. --json validation errors now emit {status:"error", submitted:false, error} on stdout with process.exitCode = 1 (uniform with the success/transport shapes) instead of guard()'s plaintext stderr; the deliberate exit-1 (fixable flags) vs exit-0 (unfixable backend) asymmetry is preserved and documented.
• Security: No new secrets logged or returned; the size/binary guards reduce the chance of shipping a garbage/binary blob. --file reads a local path the caller already controls; content is still redacted before send. See the special-file Suggestion above.
• Performance: Bounded — statSync + a ≤256 KB readFileSync, no hot path, no new network/DB work.

Verdict

approved (informational — zero Critical findings; the GitHub green-check remains a separate human action). One non-blocking Suggestion and a few Information notes.

[jwfing]

LGTM - approved.