human diagnostic reports (migrate dry-run, diff, lint, suggest) now color their labels the way compilers do

[Kiran01bm]

Summary

The human diagnostic reports (migrate dry-run, diff, lint, suggest) now color their labels the way compilers do, and the demo GIFs are re-rendered with a theme that separates typed input from tool output.

Why

The reports already use the compiler-diagnostic grammar, but rendered monochrome the severity labels don't jump out the way they do in rustc/clang output — a refusal and a note read with the same weight. In the animated demos, prompt, typed command, and output all rendered in the same white, so the eye had to parse the layout to tell input from result.

What

• A shared --color=auto|always|never flag on migrate, diff, lint, and suggest. auto colors only when stdout is a terminal and respects NO_COLOR and TERM=dumb; the JSON reports and diff --sql are never colored.
• Severity labels take conventional compiler colors (error red, warning yellow, note cyan, help green); structural labels (statement N:, plan:, docs:, name:line:column:, summaries) are bold. Stripping the escape codes reproduces the plain layout byte for byte — color never moves a character.
• The four demo tapes set the Dracula theme and a colored prompt (directory cyan, branch magenta, typed input bright white, output reset via a DEBUG trap); all GIFs re-rendered.

Before                                 After
┌────────────────────────────────┐    ┌────────────────────────────────┐
│ statement 1:                   │    │ statement 1:            (bold) │
│   ALTER TABLE ...              │    │   ALTER TABLE ...              │
│                                │    │                                │
│ warning[safer-idiom]:          │    │ warning[safer-idiom]: (yellow) │
│   ...                          │    │   ...                          │
│                                │    │                                │
│ help[...]:                     │    │ help[...]:             (green) │
│   ...                          │    │   ...                          │
│                                │    │                                │
│ (all one weight, one color)    │    │ JSON / --sql: unchanged, plain │
└────────────────────────────────┘    └────────────────────────────────┘

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[aparajon]

🤖 Adversarial correctness review, requested by @aparajon and performed by his agent. Reviewed at head 744f389, with every output mode run against a live PostgreSQL 16 and every rendering compared byte for byte after stripping escapes, so the parity claims below are measured rather than read off the diff.

Verdict: the layout invariant holds everywhere I could attack it, the machine contracts stay plain, and this is safe to land. The design decision that makes it safe is that the palette only ever wraps a label that already occupies its own line — no styled string is ever measured, wrapped, or padded — and the parity tests pin that as an equality rather than a spot-check. Nothing here blocks; the findings are a missing changelog entry and two nits.

Findings

1. --color=auto is automation-observable, and there is no CHANGELOG entry for it. The Unreleased section documents every sibling change in this series — diff exiting 2, the --sql move, the lint/suggest grammar change, the help:/note: reservation — under a preamble that says the emphasis is "anything that changes what automation observes". This qualifies, and not in a theoretical way: I ran pg-sprite lint c.sql with no flags under script -q /dev/null and got four SGR sequences in the report; the same command with stdout on a pipe is plain. So a caller that allocates a pty — docker run -t, script, an expect harness, a CI runner with tty allocation, a Go supervisor using creack/pty for interleaved logs — now receives escapes in the human report where it previously received plain text, and any regex or errorformat consumer sitting there breaks. The reason this deserves an entry rather than a shrug is the second half: the change also ships the escape hatch (--color=never, NO_COLOR, TERM=dumb), and a caller who hits this in a pipeline needs to be told the hatch exists in the one file that is supposed to tell them.

2. (nit) The recorded prompt still bakes in a local path — and it is now cyan. export PS1='…~/kiran01bm/github/pg-sprite…' appears in all four tapes. I raised this as an optional nit on #42 and it was reasonable to defer then; it is worth doing now for two reasons that did not apply before. This PR rewrites that exact line in all four tapes and re-renders all four GIFs, so the change costs nothing extra — the alternative is a fifth binary blob per GIF later. And the styling makes it more prominent, not less: the directory segment is the first colored thing the eye lands on in every frame. A neutral ~/pg-sprite reads as the project's demo rather than one contributor's screen.

3. (nit) The tape-writing notes do not record the new convention. docs/demos/README.md has a "Tape-writing notes" section that captures exactly this class of non-obvious knowledge (backtick strings, the Sleep before Show, the hidden prompt). The color setup is squarely in that class — Set Theme "Dracula", the unset NO_COLOR && export TERM=xterm-256color defense against the rendering machine's environment, the escaped-\[…\] PS1, and especially the trap 'printf "\033[0m"' DEBUG that keeps the input style from bleeding into output. Whoever writes tape five will not reinvent that from the diff, and a fifth GIF that looks different from the other four is the visible cost.

Action items

1. (Finding 1) Add an Unreleased CHANGELOG entry for --color: that the human reports now color their labels when stdout is a terminal, that --color=never / NO_COLOR / TERM=dumb force plain, and that the JSON reports and diff --sql are never colored regardless.
2. (Finding 2) Neutralize the recorded prompt to ~/pg-sprite in all four tapes and re-render in this PR.
3. (optional) (Finding 3) Add a tape-writing note covering the theme, the environment reset, and the DEBUG trap.
4. (optional) Mention --color / NO_COLOR once in the README's flag prose — it is now the front door for four commands and the only place it is written down is a docs/cli-output-examples.md preamble.
5. (optional) Table-drive TestLintJSONStaysPlainUnderColorAlways over suggest --json, diff --json, diff --sql and migrate --dry-run --json. All five hold today (measured), and the signatures already make the leak hard — writeJSON, writePlanText and Verdict.JSON take no palette — so this is defense in depth on a claim the PR body makes for all five, not a hole.

Verified (tried to break, couldn't)

The byte-parity invariant survived every attack I could construct: at the CLI level, --color=never and --color=always | sed 's/\x1b\[[0-9;]*m//g' are cmp-identical for lint, suggest, diff, migrate --dry-run and the migrate verdict, against a real database, with 9–13 escape sequences present in the colored form, so the comparison is not vacuous; the invariant holds structurally too, since the palette wraps only labels that entry/entryStyled place alone on a line while wrapWords operates on the unstyled message, and the verdict's column alignment spaces sit outside the styled span. The machine contracts stay plain under --color=always on all five surfaces I could reach: lint --json, suggest --json, diff --json, diff --sql, and migrate --dry-run --json — zero escape bytes each. Exit codes are untouched by the palette (dry-run refusal 2, verdict refusal 2, diff 0 on a converged table, lint 0 on warnings-only) and match between color modes. --color=always correctly overrides NO_COLOR — the no-color.org spec makes an explicit user flag the winner — while auto honors both NO_COLOR (non-empty, so NO_COLOR= re-enables, as the code comment claims) and TERM=dumb, and an invalid value is rejected by Kong's enum rather than silently falling back. isTerminal's fstat hand-roll is right for the shipped platforms: .goreleaser.yaml builds linux and darwin only, so the Windows VT-enablement problem that would justify pulling in an isatty dependency does not exist here; the only false positive is a character device like /dev/null, which no one parses. Embedding OutputFlags ahead of the positional Path does not disturb Kong's argument binding — pg-sprite lint changes.sql still resolves positionally. The verdict.String duplication is genuinely locked rather than nominally: the parity test asserts v.String()+"\n" equals the plain rendering in both directions across four fixtures that between them exercise every human-rendered field — table, statement, attempts, detail, safer, forced, failed-step, both ExecutedSQL branches, and all three outcomes plus the unknown fallback — so a field added to one renderer and not the other fails. The smoke test is immune by construction: demo/tour.sh asserts only on --json fields and exit codes, never on text. No test function was removed and no assertion weakened — every test change in the diff is a signature threading a palette{}. go build ./..., go vet, and the full ./internal/cli/..., ./pkg/verdict/..., ./pkg/lint/..., ./pkg/suggest/..., ./pkg/plan/... suites pass locally at head, CI is green across all 12 checks, and the four re-rendered GIFs grew about 4% (873 KB → 1.06 MB total), which is well inside the budget the demos README sets out. Worth noting too that all four action items from #42 landed — demos: build db-up has its prerequisites, the "pinned by renderer tests" claim is now stated accurately, and lint.tape's geometry was raised for the longer report. The one claim I could not check first-hand is how the Dracula palette actually looks in the rendered frames: vhs is not installed in this environment, so I reviewed the tape sources and the escape sequences the binary emits rather than the GIFs.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Second pass, same head (744f389), through the two lenses @aparajon asks pg-sprite changes to be judged on: how easily an outside team adopts this, and the seam an orchestrator embedding the engine consumes. Correctness findings are in the comment above; nothing here blocks.

Lens 1 — OSS adoption

This is a bigger adoption change than "it's prettier", and the framing in the PR body undersells it. The compiler-diagnostic grammar is this project's most distinctive choice — it is why a reader who has never heard of pg-sprite already knows how to read its output. But a grammar that renders monochrome only claims the resemblance; a grammar that renders in rustc's own colors demonstrates it, in the first second, before any prose is read. Choosing the conventional assignments rather than inventing a palette is the whole trick: error red, warning yellow, note cyan, help green is a vocabulary the audience already learned somewhere else, so the learning cost of the tool's most information-dense surface drops to zero. That is a real lever, and it is the kind that compounds with the GIF-led README from #44/#45 rather than competing with it.

The auto triad is a trust signal, and worth more than it costs. Respecting NO_COLOR, honoring TERM=dumb, detecting a non-terminal stdout, and keeping always/never as explicit overrides is a small pile of code that a surprising number of otherwise-good tools get wrong — and getting it wrong is the sort of thing an evaluator notices in the first ten minutes, usually as escape garbage in a log file. Getting it right is invisible when it works, which is exactly why it is worth saying out loud: an adopter piping into a file, into less, or into a CI log gets clean text without knowing a flag exists.

Where the adoption surface is thinner than the code: the README never mentions it. The README is the front door and it now leads with four colored GIFs. --color appears exactly once in the repo's prose, in a docs/cli-output-examples.md preamble that a reader reaches only after deciding to care about the JSON contracts. The reader who most needs the sentence is the one whose terminal shows plain output where the GIF showed color — NO_COLOR exported in a dotfile, a TERM the detection distrusts, a pager in the way — and who has no way to tell a deliberate design from a broken install. One clause next to the existing --sql and --json prose closes that.

Still the demo I would most want, picking the thread up from #39 and #42: contention. Four tapes now, none of them showing pg-sprite meeting a held lock and backing off rather than joining the queue. Every prospective adopter has personally been burned by a schema change that queued behind a lock and took the application down with it, and that is the one thing they cannot get from a competitor's README or talk themselves out of. It is also the demo that benefits most from this PR, because a bounded lock_timeout giving up is precisely a warning: or error: moment — the color does the narrative work that prose would otherwise have to.

Lens 2 — the seam an orchestrator consumes

The factoring is right, and it is the part I would most want kept true. The palette lives entirely in internal/cli; verdict.String() stays the plain rendering in pkg/verdict, and pkg/plan, pkg/lint and pkg/suggest gained nothing. A Go caller embedding the engine — the case an orchestrator actually hits — sees no new field, no new dependency, and no styled string. Color is a front-door concern and this PR treats it as one. Worth stating explicitly because the tempting shortcut in a change like this is to put the styling on the report type where every consumer inherits it, and that door stays closed here.

The one seam-adjacent thing to keep an eye on is that the verdict now has two renderers. writeVerdictText reproduces verdict.String()'s layout in a different package, and the parity test is the only thing holding them together. Today that lock is complete — I enumerated Verdict's fields against the four test fixtures and every human-rendered one is exercised, so a field added to either renderer alone fails the test. But the lock is fixture-driven, not structural: a new field rendered by only one of the two, in a shape none of the four fixtures happens to set, passes. Cheap way to make it structural instead of vigilant: one fixture that populates every field of Verdict at once, added in the same change as any new field. (Cause is already rendered by neither and carried only in JSON — probably deliberate, since Detail carries the human text, but it is the existing example of a field the parity test cannot see.)

And the property to preserve as the CLI grows: the machine surfaces are plain because of their signatures, not because of a convention. writeJSON, writePlanText and Verdict.JSON take no palette, so a future command cannot accidentally color a contract — it would have to change a signature to do it. That is a much better guarantee than a rule in a doc, and it is worth knowing it is what is doing the work, so that a later refactor that threads a palette "for consistency" is recognized as removing a safety property rather than tidying up.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Approving on @aparajon's behalf after the adversarial correctness review above (no blocking findings — the byte-parity invariant and the plain machine contracts both verified against a live database). This stamp was left by Claude Code (claude-opus-5).

[Kiran01bm]

Review response from Kiran's (@Kiran01bm) code review assessment agent (Amp / Claude Opus 4.5)

Summary: All actionable findings from both passes are addressed in a follow-up commit on this branch — the required CHANGELOG entry plus every optional except the prompt neutralization (deliberate, rejected below); the contention demo remains tracked as an internal follow-up.

#	Finding	Status	Explanation
1	--color=auto is pty-observable with no CHANGELOG entry	fixed	Unreleased entry added: the pty case, all three escape hatches (--color=never / NO_COLOR / TERM=dumb), and that --json / diff --sql / exit codes are never colored.
2	Tape-writing notes don't record the color conventions	fixed	Two notes added to docs/demos/README.md: the theme + hidden NO_COLOR/TERM environment defense, and the \[…\] prompt escaping + DEBUG trap.
3	README never mentions --color / NO_COLOR	fixed	One clause in the "What it looks like" preamble, where the GIFs lead — colored on a terminal, --color=never / NO_COLOR forces plain, machine outputs never colored.
4	JSON-plain test covers only lint --json	fixed	TestMachineOutputsStayPlainUnderColorAlways table-drives all five surfaces: lint/suggest at command level under --color=always, diff --json/--sql and dry-run --json through the exact renderers the commands dispatch to.
L1	Contention demo (raised on #39, #42, and here)	deferred	Tracked as an internal follow-up (F8): a tape showing a bounded lock_timeout giving up rather than queueing behind a held lock.
L2	Two-renderer parity lock is fixture-driven, not structural	fixed	Went one step further than suggested: an all-fields Verdict fixture joins the parity loop, and a reflection guard fails the test if a future field is added without extending the fixture — the lock cannot decay silently.
L2	Palette confined to internal/cli; machine surfaces plain by signature	no action	Verified-correct observations; finding 5's test now pins the signature property against a refactor that threads a palette "for consistency".