diff --git a/AGENTS.md b/AGENTS.md index f9b13e1..559a56c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -122,6 +122,7 @@ The full structure, with a file-level breakdown, is in | [docs/content/docs/architecture/_index.md](./docs/content/docs/architecture/_index.md) | Architecture section index | | [docs/content/docs/architecture/overview.md](./docs/content/docs/architecture/overview.md) | Package structure, CLI tree, data flow | | [docs/content/docs/architecture/error-handling.md](./docs/content/docs/architecture/error-handling.md) | Sentinel errors, the wrapping rule, and `error_kind` contract | +| [docs/content/docs/architecture/output.md](./docs/content/docs/architecture/output.md) | `output.Writer`, pretty vs NDJSON, TTY detection, exit codes | `docs/design.md` is the *plan*; `docs/content/docs/architecture/` describes what has been built. As subsystems land, their behaviour moves from the former into the latter. diff --git a/docs/content/docs/architecture/_index.md b/docs/content/docs/architecture/_index.md index c8f7f94..a9bcb37 100644 --- a/docs/content/docs/architecture/_index.md +++ b/docs/content/docs/architecture/_index.md @@ -10,10 +10,11 @@ reconciliation algorithm, milestones, and open questions — lives in [design.md](https://github.com/specsnl/labelsync/blob/main/docs/design.md). As each subsystem lands, its behaviour moves from the design plan into this section. -| Page | Covers | -|---------------------------------------|-----------------------------------------------------------------------------| -| [Overview](./overview.md) | Package structure, CLI command tree, the reconciliation data flow | -| [Error Handling](./error-handling.md) | Sentinel errors, the `%w` wrapping rule, and the `error_kind` JSON contract | +| Page | Covers | +|---------------------------------------|-----------------------------------------------------------------------------------| +| [Overview](./overview.md) | Package structure, CLI command tree, the reconciliation data flow | +| [Error Handling](./error-handling.md) | Sentinel errors, the `%w` wrapping rule, and the `error_kind` JSON contract | +| [Output & Exit Codes](./output.md) | `output.Writer`, pretty vs NDJSON, TTY detection, the `slog` boundary, exit codes | Pages are added here as the subsystems they document are implemented — the palette allocator, the -planner, the GitHub client and its rate limiting, and the output system each get their own page. +planner, and the GitHub client with its rate limiting each get their own page. diff --git a/docs/content/docs/architecture/output.md b/docs/content/docs/architecture/output.md new file mode 100644 index 0000000..5236845 --- /dev/null +++ b/docs/content/docs/architecture/output.md @@ -0,0 +1,491 @@ +--- +title: Output & Exit Codes +weight: 3 +--- + +Everything a user is meant to read goes through `output.Writer`. Everything only a maintainer is +meant to read goes through `log/slog`. The two never mix, and the split is settled in +`internal/util/output` so that no later package has to decide it again. + +Process exit codes live next door, in `internal/util/exit`. + +The conventions here are Unix ones, not house style; the reasons are what make them worth following +rather than memorising. The [review checklist](#review-checklist) at the end is the short version. + +## The one rule + +**stdout is the product. stderr is the story of making it.** + +- **stdout** — the thing a user would pipe into another command or redirect to a file. The rendered + diff, the NDJSON action stream, a resolved group listing. If `> out.txt` should capture it, it is + stdout. +- **stderr** — everything else. Warnings, errors, progress, prompts, spinners, debug logs. + +This is not a stylistic preference: it is the only thing that makes +`labelsync groups --output=json | jq '.[] | .repo'` work. One narration line on stdout corrupts the +pipe. And because diagnostics are on stderr, the user still sees them on the terminal *while* piping +stdout elsewhere — which is also why `2>/dev/null` stays meaningful. + +Ask of every new line of output: **would someone want this in the file if they redirected stdout?** +If no, it is stderr. + +## The boundary + +| Channel | Stream | Audience | On a normal run | +|-----------------|----------------|-----------------------|-----------------------------------| +| `output.Writer` | stdout, stderr | The person running it | Everything they were meant to see | +| `log/slog` | stderr | Someone debugging | **Silent** — nothing at all | +| `fmt.Println` | — | — | **Never** | + +`fmt.Println` is never used for reporting. It cannot be captured in a test, cannot switch to JSON, +and has no idea which stream it belongs on. Three defects, no upside. + +### Choosing between them + +| You want to… | Use | Lands on | +|-----------------------------------------------|------------------------------|----------| +| Emit the result the command exists to produce | `output.Table(w, rows, ...)` | stdout | +| Tell the user what the code is doing | `w.Info(...)` | stderr | +| Report a recoverable problem (skipped repo) | `w.Warn(...)` | stderr | +| Report a failure | `w.Error(...)` | stderr | +| Report a failure carrying a sentinel | `w.WriteErr(err)` | stderr | +| Tell a maintainer what the code is doing | `slog.Debug(...)` | stderr | + +`WriteErr` over `Error("%v", err)` whenever you hold an `error`: it runs `labelsync.KindOf` and adds +`error_kind` to the JSON object. `Error` with a formatted string cannot. + +## `output.Writer` + +```go +type Writer interface { + Info(format string, args ...any) // stderr + Warn(format string, args ...any) // stderr + Error(format string, args ...any) // stderr + WriteErr(err error) // stderr + WriteTable(t TableData) // stdout +} +``` + +`WriteTable` is the only method on stdout, and the asymmetry is the point: **`Writer` has exactly one +product channel, and everything else narrates.** `Info` is progress — "resolving 3 groups", +"applying 12 actions" — and progress is not what `> out.txt` is for. + +In JSON mode it is not only a stream-choice question. Narration on stdout would interleave objects +that have none of the fields the data objects have: + +```json +{"level":"info","message":"resolving 3 groups"} +{"group":"websites","repositories":12,"source":"org: specsnl"} +``` + +Valid NDJSON, and `jq -r .group` still yields a `null` for the first line. With `Info` on stderr the +stdout stream is uniformly typed and `jq` never sees a record it cannot type — which +`TestJSONWriter_StdoutIsUniformlyTyped` asserts directly, by requiring every stdout line to carry +the data key and no `level`. + +`specs-cli` puts `Info` on stdout and has commands depending on that. labelsync diverges +deliberately: nothing here called `Info` when the decision was made, so there was no contract to +break, and inheriting the defect to stay symmetrical is the wrong trade. If a command ever needs a +*result* line that is not a table, it gets a new product-level method — `Info` does not move back. + +Two implementations back the `--output` flag: + +| Implementation | `--output` | Rendering | +|----------------|------------|-----------------------------------------------------| +| `PrettyWriter` | `pretty` | lipgloss-styled text, colour-degraded to the stream | +| `JSONWriter` | `json` | NDJSON — one self-contained object per line | + +Both take their streams as constructor arguments, so a test captures output by passing +`bytes.Buffer`s rather than by intercepting the process. + +### Tables are typed rows, not strings + +Commands do not call `WriteTable` directly. They call `output.Table`, which takes the rows as they +already exist and a description of how to display them: + +```go +type GroupRow struct { + Name string `json:"group"` + Repositories int `json:"repositories"` + Source string `json:"source"` +} + +output.Table(app.Out, groups, + output.Col("Group", func(g GroupRow) string { return g.Name }), + output.Col("Repositories", func(g GroupRow) string { return strconv.Itoa(g.Repositories) }), + output.Col("Source", func(g GroupRow) string { return g.Source }), +) +``` + +**The two audiences get different projections of the same row.** The pretty table comes from the +columns. The JSON comes from marshalling the row itself, so its keys and its types are the struct's +own — `{"repositories":12}`, a number a consumer can filter on, not the `"12"` a shared +string-table would have forced. Those `json` tags are a public contract in the same way +`error_kind` is: added to, never renamed. + +That split is also what lets a column be formatted or computed without the record paying for it. A +size is an `int64` in the record and `1.2 MiB` in the table; a timestamp is RFC 3339 in the record +and `3 days ago` in the table. Under a shared `[][]string` the choice was to have one or the other. + +`Table` is a generic function rather than a method because Go does not allow type parameters on +methods. It prepares a `TableData` — headers, cells, and the source records — and that constructor +is what guarantees every row has exactly one cell per header. The old +`Table(headers []string, rows [][]string)` could not: a ragged or reordered row was a runtime +surprise, and the writer had to defend against short rows on every call. + +### Writes are best-effort + +`Writer` has no error channel. Reporting a failed write would itself need a working stream, so +output calls discard their error. This is why `.golangci.yml` excludes `fmt.Fprintln` and +`lipgloss.Fprintln` from `errcheck`, and why `writeJSONLine` discards the encoder's error with an +explicit `_ =`. + +## Colour and TTY detection + +Handled for you — do not re-implement it per call site. + +`NewPrettyWriter` wraps each stream once, at construction, in a `colorprofile.Writer`. That writer +asks the stream what it can render — truecolor, 256-colour, 16-colour, or nothing at all — and +downsamples or strips the escape sequences to match. `NO_COLOR`, `CLICOLOR`, and `CLICOLOR_FORCE` +are honoured. + +The consequence that matters: a run piped into a CI log gets the same text with the escapes +removed, not a wall of `\x1b[`. Styling degrades; it does not have to be switched off by hand. + +Detection happens once rather than per line, and the environment is a constructor argument rather +than an ambient read, so the golden tests are the same on a developer's terminal and in CI. Two +consequences worth internalising: + +- **Anything written through the writer is safe.** Add a styled renderer later and it inherits the + detection automatically. +- **Anything written around it is not.** `fmt.Fprintln(os.Stdout, someStyledString)` sends raw ANSI + into a redirected file. Wrapping the stream — rather than styling call-by-call — is what makes the + safe path the default one. + +### What `IsTTY` is still for + +Colour is handled, so it is tempting to conclude the helper has no remaining job. It has two, and +neither is about styling. + +Progress UI — spinners, progress bars, the rate-limit countdown — is stderr like every other +diagnostic, and is *additionally* drawn only when the stream is a terminal. This is the standard +shape: npm, cargo, docker, and git all do it. The animation relies on `\r` and cursor moves that are +meaningless in a redirected file, so into a pipe or CI log it degrades to periodic plain lines, or +to nothing at all. + +- the **rate-limit countdown** — `\r` on a terminal, periodic log lines into a pipe, because control + characters in a CI log are unreadable; +- the **prune prompt** — never shown to a pipe. A CI job blocked on an invisible prompt hangs until + someone cancels it. That is `ErrInteractiveRequired`. + +Note which stream each one asks about. The countdown asks about **stderr**, where it draws. The +prompt asks about **stdin**, because the hang it prevents is a read with nobody to answer it — a job +with a terminal attached to stderr and its stdin closed must still refuse to prompt. Two questions, +two streams; check the one the decision actually depends on. + +```go +func IsTTY(stream any) bool +``` + +`IsTTY` therefore takes `any` and type-asserts for `Fd() uintptr`, rather than taking an +`io.Writer`. The writer signature made `IsTTY(os.Stdin)` compile only by accident — `*os.File` +happens to have a `Write` method — and read as though stdin were out of scope. Anything without a +file descriptor, a `bytes.Buffer` in a test included, is not a terminal. + +## NDJSON + +`JSONWriter` emits exactly one complete JSON object per line. Not one array: an array could only be +closed once the last row was known, and a run killed halfway would leave a truncated document +behind. One object per line means a consumer can parse the stream as it arrives, and whatever was +written before the kill is still valid. + +stdout carries the data, one object per row, every line the same shape: + +```json +{"group":"websites","repositories":12,"source":"org: specsnl"} +{"group":"platform","repositories":3,"source":"repos:"} +``` + +stderr carries the narration, every line carrying a `level`: + +```json +{"level":"info","message":"resolving 3 groups"} +{"level":"warn","message":"skipping specsnl/old-thing: archived"} +{"error_kind":"repo_inaccessible","level":"error","message":"repository is inaccessible: specsnl/old-thing"} +``` + +`WriteErr` adds `error_kind` when the error wraps a known sentinel — see +[Error Handling](./error-handling.md). The message is prose and may be reworded; `error_kind` is the +contract. + +Row keys come from the row struct's `json` tags — see +[Tables are typed rows](#tables-are-typed-rows-not-strings). Nothing derives them from a column +heading, so rewording a heading cannot disturb a `jq` filter and there is no normalisation step to +reason about. + +HTML escaping is off. Label names legitimately contain `&` and `<`, and rendering them as `&` +helps nobody reading a log. + +## Table rendering + +Two renderers, because the diff and the list commands want different things: + +| Function | Shape | Used by | +|-----------------|---------------------------------------|----------------------------------------------------| +| `RenderColumns` | Aligned columns, no header, no border | The pretty diff | +| `RenderTable` | Bordered table with a header row | `PrettyWriter.WriteTable` — `groups`, `cache info` | + +The diff is a list, not a table: there is nothing to put in a header row, and a box around it would +fight the per-repository grouping. But the columns still have to line up down the page or the +changes are unreadable. + +```text ++ create type: bug #d73a4a "Something isn't working" +~ recolour wontfix #d73a4a → #16a3c4 (displaced by "type: bug") += ok priority: high +``` + +Column widths are measured in **display width**, not bytes. A label name may contain an emoji, and +`len()` would over-pad the column by several characters. + +## Debug logging + +```go +func SetupLogger(stderr io.Writer, format Format, debug bool) *slog.LevelVar +``` + +`SetupLogger` installs the process-wide default `slog` logger on stderr and returns the `LevelVar` +gating it. Without `--debug` the level is `output.LevelSilent` — above every level `slog` defines, +so nothing is emitted at all. A silent default is the point: without it, warn- and error-level +diagnostics would leak onto stderr and interleave with the real output, and the two channels stop +being distinguishable. + +The trap that follows from that: **`slog.Warn` and `slog.Error` are invisible.** They are not +"quieter", they are gone. If a user needs to know, it is `w.Warn` / `w.Error`. `slog` levels above +Debug exist for the `--debug` reader only. + +```go +slog.Debug("read labels", "repo", repo, "count", len(labels), "etag_hit", hit) // good +slog.Warn("skipping " + repo) // wrong — user-facing, and silent +w.Warn("skipping %s: archived", repo) // right +``` + +The level is returned rather than baked in because Cobra parses persistent flags after the command +tree is built: construct the logger once, then `level.Set(slog.LevelDebug)` from `PersistentPreRunE` +once `--debug` is known. + +Records follow the output format, so `--debug --output=json` gives a stderr stream that is JSON all +the way down. Stderr and never stdout, for the same reason warnings go there. + +## Wiring it in Cobra + +The command tree does not exist yet — it lands with `internal/cmd` — but the wiring it has to use is +part of this contract, because getting it wrong is what makes the rest untestable. + +### Take the writers from the command, not the process + +```go +// in PersistentPreRunE, where the flags are parsed +switch outputFormat { +case string(output.FormatJSON): + app.Out = output.NewJSONWriter(cmd.OutOrStdout(), cmd.ErrOrStderr()) +default: + app.Out = output.NewPrettyWriter(cmd.OutOrStdout(), cmd.ErrOrStderr(), nil) +} + +app.LogLevel = output.SetupLogger(cmd.ErrOrStderr(), format, debug) +``` + +`cmd.OutOrStdout()` resolves to `os.Stdout` in production, so nothing is lost — but in a test +`cmd.SetOut(buf)` / `cmd.SetErr(buf)` captures everything with no global-state hacking. + +`NewDefaultPrettyWriter`, `NewDefaultJSONWriter`, and `SetupDefaultLogger` hardcode `os.Stdout` / +`os.Stderr`. They exist for a pre-flag-parse fallback and for one-off tools. **Do not reach for them +from a command** — that is how output silently escapes a test's buffers. + +This applies to `slog` too. Pass `cmd.ErrOrStderr()` to `SetupLogger`, not `os.Stderr`, or `--debug` +output becomes the one thing a command test cannot see. + +Read piped input through `cmd.InOrStdin()` for the same reason, so +`cat labels.yml | labelsync ...` is testable the same way as the output side. + +### Silence Cobra and handle the error once + +```go +root := &cobra.Command{ + SilenceUsage: true, // usage on a runtime failure is noise + SilenceErrors: true, // main prints it once, so it reliably lands on stderr +} +``` + +On a returned error, current Cobra already writes to stderr — the error line via `ErrOrStderr()`, +and the usage block via `OutOrStderr()`, which itself *falls back to stderr* when `cmd.SetOut` is +never called. So the stream is not the problem. Two other things are, and both flags earn their +place: + +- **`SilenceUsage`** — without it, *every* returned error appends the full usage block after the + message. Usage helps with a bad flag, not with "repository is inaccessible"; on a runtime failure + it is noise that buries the one line that matters. +- **`SilenceErrors`** — without it Cobra prints its own bare `Error: ` *in addition* to + whatever `main` prints, so the failure shows up twice and Cobra's copy carries no `error_kind`. + Owning the single print in `main` is what lets a JSON run put `error_kind` on its final line. + +## Exit codes + +`internal/util/exit`. The idea is `terraform plan -detailed-exitcode`: without a code that means +"succeeded, and found work", a CI dry-run can only ever pass, which makes it useless as a check. + +| Constant | Code | Bit | Meaning | +|----------------|------|-----|--------------------------------------------------------------------------------| +| `exit.OK` | `0` | — | In sync — no changes needed, or every action applied and no repository skipped | +| `exit.Error` | `1` | — | The run failed. Nothing about the live state can be inferred | +| `exit.Drift` | `2` | `1` | Completed without writing and found pending actions — `sync --dry-run` | +| `exit.Skipped` | `4` | `2` | Applied successfully, but one or more repositories could not be reached | + +### Outcomes combine; failure does not + +A single run can satisfy more than one outcome — a `--dry-run` that finds pending actions *and* +cannot reach a repository is both drift and skipped. Rather than rank them, the outcome codes are +**disjoint bits and OR together**: that run exits `6`. The next outcome added takes `8`. + +```go +code := exit.OK +if dryRun && len(plan.Actions) > 0 { + code |= exit.Drift +} +if len(result.Skipped) > 0 { + code |= exit.Skipped +} +``` + +`exit.Error` is deliberately **not** a bit in that space. It is the classic Unix generic failure and +it is exclusive: when the run failed, the live state is unknown, so "failed *and* drifted" is not a +statement labelsync can honestly make. A failure returns `1` and nothing else. That is also what +keeps every combination meaningful — with `Error` in the mask, `3` would have to mean "failed and +drifted", which is precisely the claim the failure invalidates. + +The cost is real and worth naming: `[ $? -eq 2 ]` is no longer how a caller tests for drift once a +second outcome exists. Callers test bits. + +```sh +labelsync sync --dry-run; rc=$? +(( rc == 1 )) && exit 1 # the run itself failed +(( rc & 2 )) && echo "labels have drifted" +(( rc & 4 )) && echo "some repositories were unreachable" +``` + +`if labelsync sync; then` is unaffected — zero still means clean, non-zero still means look closer. + +The numbers are a public contract: pipelines branch on them, so bits may be **added**, never +reassigned, and `exit.Error` stays `1` forever. `exit_test.go` spells each value out literally, and +asserts the outcome codes are single non-overlapping bits, so either mistake fails there rather than +in someone's pipeline. + +### Getting a code out of a command + +Codes travel to `main` on the error, because `RunE` returns an `error` and nothing else. That is +what `exit.Err` is for: + +```go +type Err struct { + Code Code + Err error // nil when the code reports an outcome rather than a failure +} + +func (e *Err) Unwrap() error { return e.Err } + +// Of returns OK for nil, the carried code for an *Err, and Error for anything else. +func Of(err error) Code +``` + +A command returns `&exit.Err{Code: code}` when `code != exit.OK`, and a plain wrapped sentinel +otherwise — an ordinary failure needs no carrier, because `Of` already maps it to `exit.Error`. A +carrier with a wrapped failure but no code still reports `Error`: a zero exit on a failed run is the +one wrong answer a pipeline cannot detect. + +`Unwrap` matters as much as `Code`. A carrier that hid the error it wraps would strip `error_kind` +from exactly the failures that carry a code. + +`main` unwraps once, prints only if there is something to print, and exits: + +```go +func main() { + app := cmd.NewApp() + + err := cmd.ExecuteContext(ctx, app) + + var ex *exit.Err + if err != nil && !(errors.As(err, &ex) && ex.Err == nil) { + app.Out.WriteErr(err) + } + + os.Exit(int(exit.Of(err))) +} +``` + +A nil `Err` field means silent, and that guard is the whole reason the type exists: exit code `2` on +a drifting dry run must not also print an error line, because the drift *was* the successful result +and the diff has already been rendered on stdout. + +Routing the error through `Writer` rather than `fmt.Fprintln(os.Stderr, err)` is what gives JSON runs +an `error_kind` on their final line. + +`os.Exit` belongs in `main` and nowhere else. It skips deferred cleanup, so called from inside a +command it leaks temp files, unreleased locks, and unflushed writers. Return the error; let `main` +decide. + +## Testing + +Golden files under `internal/util/output/testdata/`, regenerated with: + +```sh +task dc:run:go-builder -- go test ./internal/util/output/ -update +``` + +The pretty rendering has **two** goldens per stream: one produced with an empty environment, where +every escape sequence is stripped, and one with `CLICOLOR_FORCE=1` where they are not. The pair is +the actual claim — that the plain rendering is colour *degrading cleanly*, rather than styles never +having been applied. + +The JSON tests assert the NDJSON property directly: every line of both streams parses on its own as +a complete object. + +## Review checklist + +- [ ] Would a user want this in the file after `> out.txt`? If not, it is stderr. +- [ ] No `fmt.Println` / `fmt.Printf` outside tests. +- [ ] Writers and the logger come from `cmd.OutOrStdout()` / `cmd.ErrOrStderr()`, not `os.*`. +- [ ] Nothing user-facing goes through `slog`; nothing debug-only goes through `Writer`. +- [ ] `WriteErr(err)` rather than `Error("%v", err)` whenever an `error` is in hand. +- [ ] No `os.Exit` outside `main`. +- [ ] Nothing styled is written around the writer. +- [ ] Tables go through `output.Table` with a row struct whose `json` tags carry the machine + contract — never a hand-built `TableData`, and never a struct field pre-formatted into a + string just to make the table read well. +- [ ] Any new prompt is guarded by a TTY check **on stdin**. +- [ ] Outcome codes are OR'd, never assigned over each other; `exit.Error` is not OR'd with anything. +- [ ] A non-zero code that is not a failure returns `&exit.Err{Code: ...}` with no wrapped error, so + nothing is printed. + +## Pitfalls, with receipts + +Real defects in `specs-cli`, kept here because they are the failure modes these rules exist to +prevent, not hypotheticals. + +**Styling written around the wrapped writer.** `HumanWriter.Table` uses `fmt.Fprintln` on the raw +stream while `RenderTable` applies `Bold` and `ANSIColor(240)`. Escape codes land in redirected files +and `NO_COLOR` is ignored — the sibling methods are safe only because they happen to use +`lipgloss.Fprintln`. Wrapping the stream once removes the chance to get this wrong. See +[specsnl/specs-cli#109](https://github.com/specsnl/specs-cli/issues/109) for a related `Table` issue. + +**A default log level that depends on nobody using it.** `specs-cli` defaults to `slog.LevelInfo`, so +any `slog.Info` anywhere prints on an ordinary run. Nothing leaks today only because the codebase +happens to log at Debug — an invariant nobody wrote down. `LevelSilent` makes the guarantee +structural. + +**A handler chosen by the wrong condition.** `specs-cli` installs the JSON slog handler only when +`--debug` **and** `--output=json` are both set, so `--output=json` alone leaves text records on +stderr. Handler format should follow the output format, independently of verbosity. + +**`slog` pointed at `os.Stderr` instead of the command's writer.** `cmd.SetErr(buf)` then cannot +capture `--debug` output, which is precisely the testability the accessors exist to provide. diff --git a/docs/content/docs/architecture/overview.md b/docs/content/docs/architecture/overview.md index 8eb8775..d518311 100644 --- a/docs/content/docs/architecture/overview.md +++ b/docs/content/docs/architecture/overview.md @@ -30,16 +30,18 @@ labelsync/ ├── apply/ # executes a Plan, prune prompts └── util/ ├── exit/ # exit codes - ├── output/ # lipgloss logger + table renderer + ├── output/ # Writer (pretty + NDJSON), table renderers, slog setup └── validate/ # shared validators ``` ### Implemented so far -| Package | Status | Notes | -|----------------------|---------|----------------------------------------------------------------| -| `internal/labelsync` | landed | XDG config/cache paths, config file names, sentinels, `KindOf` | -| everything else | planned | See the milestone table in the design plan | +| Package | Status | Notes | +|------------------------|---------|----------------------------------------------------------------| +| `internal/labelsync` | landed | XDG config/cache paths, config file names, sentinels, `KindOf` | +| `internal/util/exit` | landed | The four exit codes — see [Output & Exit Codes](./output.md) | +| `internal/util/output` | landed | `Writer`, pretty + NDJSON, TTY detection, `slog` wiring | +| everything else | planned | See the milestone table in the design plan | ### Why `plan` and `palette` are isolated @@ -74,12 +76,14 @@ labelsync [--config ] ### Exit codes -| Code | Meaning | -|------|--------------------------------------------------------------------| -| `0` | In sync — no changes needed, or applied successfully with no drift | -| `1` | Error (config invalid, auth failure, unrecoverable API error) | -| `2` | Drift detected — `--dry-run` found pending actions | -| `3` | Applied successfully, but one or more repositories were skipped | +| Code | Constant | Meaning | +|------|----------------|--------------------------------------------------------------------| +| `0` | `exit.OK` | In sync — no changes needed, or applied successfully with no drift | +| `1` | `exit.Error` | Error (config invalid, auth failure, unrecoverable API error) | +| `2` | `exit.Drift` | Drift detected — `--dry-run` found pending actions | +| `3` | `exit.Skipped` | Applied successfully, but one or more repositories were skipped | + +Defined in `internal/util/exit`; the rationale is in [Output & Exit Codes](./output.md#exit-codes). ## Configuration diff --git a/docs/design.md b/docs/design.md index 4e5da6d..f8db0ae 100644 --- a/docs/design.md +++ b/docs/design.md @@ -581,6 +581,9 @@ before running a prune. ### Exit codes +> **Landed** as `internal/util/exit` — see +> [Output & Exit Codes](./content/docs/architecture/output.md#exit-codes). + Borrowed from `terraform plan -detailed-exitcode`. Without this, a CI dry-run can only ever pass, which makes it useless as a check. @@ -589,7 +592,11 @@ which makes it useless as a check. | `0` | In sync — no changes needed / applied successfully with no drift | | `1` | Error (config invalid, auth failure, unrecoverable API error) | | `2` | Drift detected — `--dry-run` found pending actions | -| `3` | Applied successfully, but one or more repositories were skipped | +| `4` | Applied successfully, but one or more repositories were skipped | + +The outcome codes are disjoint bits and combine: a dry run that finds drift *and* cannot reach a +repository exits `6`. `1` stays exclusive — a failed run cannot also report on a live state it never +established. ### Non-interactive guard @@ -707,6 +714,9 @@ the build rather than silently escaping `KindOf` and rendering an empty `error_k ## Output +> **Landed.** `internal/util/output` is built; what it actually does is documented in +> [Output & Exit Codes](./content/docs/architecture/output.md). + All user-facing output goes through `output.Writer`, with `pretty` and `json` implementations selected by `--output`. `log/slog` is a **debug-only diagnostic channel on stderr**, silent on a normal run, and never used for user-facing reporting. @@ -852,19 +862,21 @@ asserts byte-identical output. Colour churn on re-run is the most likely subtle ## Dependencies -| Package | Purpose | Shared with specs-cli | -|--------------------------------------|---------------------------------------------|--------------------------------------------------| -| `github.com/spf13/cobra` | CLI command tree | yes | -| `gopkg.in/yaml.v3` | Config parsing | yes | -| `charm.land/huh/v2` | Interactive prune selection (`MultiSelect`) | yes | -| `charm.land/lipgloss/v2` | Output styling, diff table, countdown | yes | -| `github.com/adrg/xdg` | Config + cache directory resolution | yes | -| `github.com/danwakefield/fnmatch` | Repo include/exclude globs | yes | -| `golang.org/x/sync` | `errgroup` bounded parallel reads | yes | -| `log/slog` | Debug logging (stdlib) | yes | -| `github.com/google/go-github/v76` | GitHub REST client | **new** | -| `github.com/cli/go-gh/v2` | Token resolution only | **new** | -| `github.com/lucasb-eyer/go-colorful` | CIELAB + CIEDE2000 | **new (direct)** — already indirect via lipgloss | +| Package | Purpose | Shared with specs-cli | +|-----------------------------------------|-----------------------------------------------|--------------------------------------------------| +| `github.com/spf13/cobra` | CLI command tree | yes | +| `gopkg.in/yaml.v3` | Config parsing | yes | +| `charm.land/huh/v2` | Interactive prune selection (`MultiSelect`) | yes | +| `charm.land/lipgloss/v2` | Output styling, diff table, countdown | yes | +| `github.com/adrg/xdg` | Config + cache directory resolution | yes | +| `github.com/danwakefield/fnmatch` | Repo include/exclude globs | yes | +| `golang.org/x/sync` | `errgroup` bounded parallel reads | yes | +| `log/slog` | Debug logging (stdlib) | yes | +| `github.com/google/go-github/v76` | GitHub REST client | **new** | +| `github.com/cli/go-gh/v2` | Token resolution only | **new** | +| `github.com/lucasb-eyer/go-colorful` | CIELAB + CIEDE2000 | **new (direct)** — already indirect via lipgloss | +| `github.com/charmbracelet/colorprofile` | Colour downsampling per output stream | **new (direct)** — already indirect via lipgloss | +| `github.com/charmbracelet/x/term` | `IsTerminal` for the non-colour TTY decisions | **new (direct)** — already indirect via lipgloss | **Why go-github rather than a hand-rolled client or `go-gh`'s REST client:** the deciding factor is typed `*github.RateLimitError` and `*github.AbuseRateLimitError`. Secondary rate limits are the diff --git a/go.mod b/go.mod index d2e3dfa..825cc4e 100644 --- a/go.mod +++ b/go.mod @@ -2,6 +2,25 @@ module github.com/specsnl/labelsync go 1.26.5 -require github.com/adrg/xdg v0.5.3 +require ( + charm.land/lipgloss/v2 v2.0.5 + github.com/adrg/xdg v0.5.3 + github.com/charmbracelet/colorprofile v0.4.3 + github.com/charmbracelet/x/term v0.2.2 +) -require golang.org/x/sys v0.26.0 // indirect +require ( + github.com/charmbracelet/ultraviolet v0.0.0-20251205161215-1948445e3318 // indirect + github.com/charmbracelet/x/ansi v0.11.7 // indirect + github.com/charmbracelet/x/termios v0.1.1 // indirect + github.com/charmbracelet/x/windows v0.2.2 // indirect + github.com/clipperhouse/displaywidth v0.11.0 // indirect + github.com/clipperhouse/uax29/v2 v2.7.0 // indirect + github.com/lucasb-eyer/go-colorful v1.4.0 // indirect + github.com/mattn/go-runewidth v0.0.23 // indirect + github.com/muesli/cancelreader v0.2.2 // indirect + github.com/rivo/uniseg v0.4.7 // indirect + github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e // indirect + golang.org/x/sync v0.18.0 // indirect + golang.org/x/sys v0.46.0 // indirect +) diff --git a/go.sum b/go.sum index 1e603a2..e1598c1 100644 --- a/go.sum +++ b/go.sum @@ -1,12 +1,44 @@ +charm.land/lipgloss/v2 v2.0.5 h1:kbNxgeeUOYv5J0YdpxFjfvf3dFvqH8Aci4zB6xqFtrY= +charm.land/lipgloss/v2 v2.0.5/go.mod h1:9oqhxt4yxIMe6q5A4kHr44DremZk7J9UNh74GlWa5nc= github.com/adrg/xdg v0.5.3 h1:xRnxJXne7+oWDatRhR1JLnvuccuIeCoBu2rtuLqQB78= github.com/adrg/xdg v0.5.3/go.mod h1:nlTsY+NNiCBGCK2tpm09vRqfVzrc2fLmXGpBLF0zlTQ= +github.com/charmbracelet/colorprofile v0.4.3 h1:QPa1IWkYI+AOB+fE+mg/5/4HRMZcaXex9t5KX76i20Q= +github.com/charmbracelet/colorprofile v0.4.3/go.mod h1:/zT4BhpD5aGFpqQQqw7a+VtHCzu+zrQtt1zhMt9mR4Q= +github.com/charmbracelet/ultraviolet v0.0.0-20251205161215-1948445e3318 h1:OqDqxQZliC7C8adA7KjelW3OjtAxREfeHkNcd66wpeI= +github.com/charmbracelet/ultraviolet v0.0.0-20251205161215-1948445e3318/go.mod h1:Y6kE2GzHfkyQQVCSL9r2hwokSrIlHGzZG+71+wDYSZI= +github.com/charmbracelet/x/ansi v0.11.7 h1:kzv1kJvjg2S3r9KHo8hDdHFQLEqn4RBCb39dAYC84jI= +github.com/charmbracelet/x/ansi v0.11.7/go.mod h1:9qGpnAVYz+8ACONkZBUWPtL7lulP9No6p1epAihUZwQ= +github.com/charmbracelet/x/term v0.2.2 h1:xVRT/S2ZcKdhhOuSP4t5cLi5o+JxklsoEObBSgfgZRk= +github.com/charmbracelet/x/term v0.2.2/go.mod h1:kF8CY5RddLWrsgVwpw4kAa6TESp6EB5y3uxGLeCqzAI= +github.com/charmbracelet/x/termios v0.1.1 h1:o3Q2bT8eqzGnGPOYheoYS8eEleT5ZVNYNy8JawjaNZY= +github.com/charmbracelet/x/termios v0.1.1/go.mod h1:rB7fnv1TgOPOyyKRJ9o+AsTU/vK5WHJ2ivHeut/Pcwo= +github.com/charmbracelet/x/windows v0.2.2 h1:IofanmuvaxnKHuV04sC0eBy/smG6kIKrWG2/jYn2GuM= +github.com/charmbracelet/x/windows v0.2.2/go.mod h1:/8XtdKZzedat74NQFn0NGlGL4soHB0YQZrETF96h75k= +github.com/clipperhouse/displaywidth v0.11.0 h1:lBc6kY44VFw+TDx4I8opi/EtL9m20WSEFgwIwO+UVM8= +github.com/clipperhouse/displaywidth v0.11.0/go.mod h1:bkrFNkf81G8HyVqmKGxsPufD3JhNl3dSqnGhOoSD/o0= +github.com/clipperhouse/uax29/v2 v2.7.0 h1:+gs4oBZ2gPfVrKPthwbMzWZDaAFPGYK72F0NJv2v7Vk= +github.com/clipperhouse/uax29/v2 v2.7.0/go.mod h1:EFJ2TJMRUaplDxHKj1qAEhCtQPW2tJSwu5BF98AuoVM= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/lucasb-eyer/go-colorful v1.4.0 h1:UtrWVfLdarDgc44HcS7pYloGHJUjHV/4FwW4TvVgFr4= +github.com/lucasb-eyer/go-colorful v1.4.0/go.mod h1:R4dSotOR9KMtayYi1e77YzuveK+i7ruzyGqttikkLy0= +github.com/mattn/go-runewidth v0.0.23 h1:7ykA0T0jkPpzSvMS5i9uoNn2Xy3R383f9HDx3RybWcw= +github.com/mattn/go-runewidth v0.0.23/go.mod h1:XBkDxAl56ILZc9knddidhrOlY5R/pDhgLpndooCuJAs= +github.com/muesli/cancelreader v0.2.2 h1:3I4Kt4BQjOR54NavqnDogx/MIoWBFa0StPA8ELUXHmA= +github.com/muesli/cancelreader v0.2.2/go.mod h1:3XuTXfFS2VjM+HTLZY9Ak0l6eUKfijIfMUZ4EgX0QYo= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= +github.com/rivo/uniseg v0.4.7 h1:WUdvkW8uEhrYfLC4ZzdpI2ztxP1I582+49Oc5Mq64VQ= +github.com/rivo/uniseg v0.4.7/go.mod h1:FN3SvrM+Zdj16jyLfmOkMNblXMcoc8DfTHruCPUcx88= github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsTg= github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= -golang.org/x/sys v0.26.0 h1:KHjCJyddX0LoSTb3J+vWpupP9p0oznkqVk/IfjymZbo= -golang.org/x/sys v0.26.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= +github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e h1:JVG44RsyaB9T2KIHavMF/ppJZNG9ZpyihvCd0w101no= +github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e/go.mod h1:RbqR21r5mrJuqunuUZ/Dhy/avygyECGrLceyNeo4LiM= +golang.org/x/exp v0.0.0-20231006140011-7918f672742d h1:jtJma62tbqLibJ5sFQz8bKtEM8rJBtfilJ2qTU199MI= +golang.org/x/exp v0.0.0-20231006140011-7918f672742d/go.mod h1:ldy0pHrwJyGW56pPQzzkH36rKxoZW1tw7ZJpeKx+hdo= +golang.org/x/sync v0.18.0 h1:kr88TuHDroi+UVf+0hZnirlk8o8T+4MrK6mr60WkH/I= +golang.org/x/sync v0.18.0/go.mod h1:9KTHXmSnoGruLpwFjVSX0lNNA75CykiMECbovNTZqGI= +golang.org/x/sys v0.46.0 h1:noSf2Fq6F8DBgS+LysIkx7rIExoNHJsxOAtPp4rthXw= +golang.org/x/sys v0.46.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/internal/util/exit/exit.go b/internal/util/exit/exit.go new file mode 100644 index 0000000..7d46dde --- /dev/null +++ b/internal/util/exit/exit.go @@ -0,0 +1,114 @@ +// Package exit holds the process exit codes labelsync returns, and the error +// type that carries one out of a command. +// +// The scheme is borrowed from `terraform plan -detailed-exitcode`: a dry run +// that finds pending work exits non-zero without that meaning "the tool broke". +// Without it a CI dry-run can only ever pass, which makes it useless as a check. +// +// # Outcomes combine; failure does not +// +// A single run can satisfy more than one outcome — a dry run that finds pending +// actions and also cannot reach a repository is both [Drift] and [Skipped]. The +// outcome codes are therefore disjoint bits that OR together, and that run exits +// 6. Ranking them would mean throwing half the answer away. +// +// [Error] is deliberately not part of that bit space. It is the classic Unix +// generic failure and it is exclusive: when a run fails, the live state is +// unknown, so "failed and drifted" is not a statement labelsync can honestly +// make. A failure exits 1 and nothing else — which is also what keeps every +// combination meaningful, since with Error in the mask 3 would have to mean +// "failed and drifted", the very claim the failure invalidates. +// +// The numbers are a public contract — CI pipelines branch on them — so bits may +// be added, never reassigned, and Error stays 1 forever. +package exit + +import ( + "errors" + "strconv" +) + +// Code is a process exit code. The outcome codes are single bits and combine +// with |; see the package doc. +type Code int + +const ( + // OK means the run succeeded and left nothing outstanding: either the live + // state already matched the config, or every planned action was applied and + // no repository was skipped. + OK Code = 0 + + // Error means the run failed. Config invalid, no token, an unrecoverable API + // error, or a rate-limit wait that would exceed --max-wait. Nothing about the + // live state can be inferred from this code, which is why it is exclusive + // rather than a bit combined with the outcomes below. + Error Code = 1 + + // Drift means the run completed without writing and found pending actions: + // `sync --dry-run` against repositories whose labels disagree with the + // config. This is the code a pull-request check fails on. + Drift Code = 1 << 1 + + // Skipped means the plan was applied successfully, but one or more + // repositories could not be reached — missing, archived, or outside the + // token's scopes. Those failures are collected per repository rather than + // aborting the run, so the work that could be done was done; this code is how + // the caller learns it was not the whole set. + Skipped Code = 1 << 2 +) + +// Err carries a code out of a command, because RunE returns an error and +// nothing else. +// +// A nil Err field means silent: the command has already reported everything the +// user needs through the output.Writer, and there is no failure to print. A dry +// run that found drift is not an error — the drift was the successful result, +// and the diff is already on stdout — but it still has to exit 2. +// +// An ordinary failure needs no carrier at all: [Of] maps every other error to +// [Error]. +type Err struct { + // Code is the code the process should exit with. Combine outcome bits with |. + Code Code + + // Err is the underlying failure, or nil when the non-zero code reports an + // outcome rather than a failure. + Err error +} + +// Error implements error. The message is only ever read by a caller that +// formats the carrier itself — main prints the wrapped error, or nothing. +func (e *Err) Error() string { + if e.Err != nil { + return e.Err.Error() + } + + return "exit code " + e.Code.String() +} + +// Unwrap exposes the underlying failure to errors.Is and errors.As, so putting +// an error in a carrier does not hide its sentinel from labelsync.KindOf. +func (e *Err) Unwrap() error { return e.Err } + +// Of returns the code the process should exit with: OK for a nil error, the +// carried code for an [Err], and Error for anything else. +// +// A carrier holding a real failure never reports success, even when it was +// built with Code left unset — a zero exit on a failed run is the one mistake +// here that a pipeline cannot detect. +func Of(err error) Code { + if err == nil { + return OK + } + + var carrier *Err + if errors.As(err, &carrier) && carrier.Code != OK { + return carrier.Code + } + + return Error +} + +// String renders a code for a debug log or a test failure. A combined code +// renders as its sum, which is what the shell sees. +func (c Code) String() string { return strconv.Itoa(int(c)) } diff --git a/internal/util/exit/exit_test.go b/internal/util/exit/exit_test.go new file mode 100644 index 0000000..12e5c15 --- /dev/null +++ b/internal/util/exit/exit_test.go @@ -0,0 +1,138 @@ +package exit_test + +import ( + "errors" + "fmt" + "testing" + + "github.com/specsnl/labelsync/internal/labelsync" + "github.com/specsnl/labelsync/internal/util/exit" +) + +// The codes are a public contract: CI pipelines branch on the numbers, so a +// renumbering is a breaking change and has to fail here rather than in a +// pipeline. Each entry is spelled out literally on purpose — asserting +// exit.OK == exit.OK would test nothing. +func TestCodes_AreStable(t *testing.T) { + for _, tc := range []struct { + name string + got exit.Code + want exit.Code + }{ + {"OK", exit.OK, 0}, + {"Error", exit.Error, 1}, + {"Drift", exit.Drift, 2}, + {"Skipped", exit.Skipped, 4}, + } { + t.Run(tc.name, func(t *testing.T) { + if tc.got != tc.want { + t.Errorf("exit.%s = %d, want %d", tc.name, tc.got, tc.want) + } + }) + } +} + +// The outcome codes have to be single, non-overlapping bits or they cannot be +// OR'd: two outcomes sharing a bit would make 6 ambiguous, and an outcome with +// two bits set would collide with a combination of others. +func TestOutcomeCodes_AreDisjointSingleBits(t *testing.T) { + var union exit.Code + + for _, tc := range []struct { + name string + code exit.Code + }{ + {"Drift", exit.Drift}, + {"Skipped", exit.Skipped}, + } { + if tc.code&(tc.code-1) != 0 { + t.Errorf("exit.%s = %d has more than one bit set", tc.name, tc.code) + } + + if union&tc.code != 0 { + t.Errorf("exit.%s = %d overlaps an earlier outcome code", tc.name, tc.code) + } + + union |= tc.code + } + + // Error is not in the bit space: it must not be reachable by combining + // outcomes, or a pipeline masking for failure would see one. + if union&exit.Error != 0 { + t.Errorf("outcome codes %d overlap exit.Error", union) + } +} + +// The combination the scheme exists for: a dry run that finds drift and also +// cannot reach a repository reports both, rather than the more alarming one. +func TestCodes_Combine(t *testing.T) { + code := exit.OK + code |= exit.Drift + code |= exit.Skipped + + if code != 6 { + t.Errorf("Drift|Skipped = %d, want 6", code) + } + + if code&exit.Drift == 0 || code&exit.Skipped == 0 { + t.Errorf("%d does not carry both outcomes", code) + } +} + +func TestOf(t *testing.T) { + sentinel := fmt.Errorf("%w: specsnl/old-thing", labelsync.ErrRepoInaccessible) + + for _, tc := range []struct { + name string + err error + want exit.Code + }{ + {"nil is success", nil, exit.OK}, + {"a plain error is a failure", errors.New("boom"), exit.Error}, + {"a wrapped sentinel is a failure", sentinel, exit.Error}, + {"a carrier reports its code", &exit.Err{Code: exit.Drift}, exit.Drift}, + {"a carrier reports a combination", &exit.Err{Code: exit.Drift | exit.Skipped}, 6}, + {"a carrier with a failure", &exit.Err{Code: exit.Error, Err: sentinel}, exit.Error}, + // Left unset by mistake: a failed run must not exit zero, because that is + // the one wrong answer a pipeline cannot detect. + {"a carrier with no code but a failure", &exit.Err{Err: sentinel}, exit.Error}, + {"a wrapped carrier", fmt.Errorf("syncing: %w", &exit.Err{Code: exit.Skipped}), exit.Skipped}, + } { + t.Run(tc.name, func(t *testing.T) { + if got := exit.Of(tc.err); got != tc.want { + t.Errorf("Of(%v) = %d, want %d", tc.err, got, tc.want) + } + }) + } +} + +// A carrier must not hide the sentinel it wraps, or the error_kind field in JSON +// output would go missing for exactly the failures that carry a code. +func TestErr_UnwrapsToTheSentinel(t *testing.T) { + err := error(&exit.Err{ + Code: exit.Error, + Err: fmt.Errorf("%w: specsnl/old-thing", labelsync.ErrRepoInaccessible), + }) + + if !errors.Is(err, labelsync.ErrRepoInaccessible) { + t.Error("errors.Is could not see through the carrier") + } + + if kind := labelsync.KindOf(err); kind != "repo_inaccessible" { + t.Errorf("KindOf = %q, want %q", kind, "repo_inaccessible") + } +} + +// A silent carrier still has to satisfy error, because it travels as one. The +// message is a fallback for a caller that formats it directly; main prints +// nothing for this case. +func TestErr_Error(t *testing.T) { + if got := (&exit.Err{Code: exit.Drift}).Error(); got != "exit code 2" { + t.Errorf("Error() = %q, want %q", got, "exit code 2") + } + + wrapped := &exit.Err{Code: exit.Error, Err: errors.New("boom")} + if got := wrapped.Error(); got != "boom" { + t.Errorf("Error() = %q, want %q", got, "boom") + } +} diff --git a/internal/util/output/golden_test.go b/internal/util/output/golden_test.go new file mode 100644 index 0000000..0da8cd1 --- /dev/null +++ b/internal/util/output/golden_test.go @@ -0,0 +1,50 @@ +package output_test + +import ( + "flag" + "os" + "path/filepath" + "testing" +) + +var update = flag.Bool("update", false, "rewrite the .golden files from the current output") + +// goldenPlain is the environment a PrettyWriter is given when the test wants +// unstyled output: empty, so colorprofile finds nothing to enable, and the +// stream is a buffer rather than a terminal. This is the CI-log rendering. +var goldenPlain = []string{} + +// goldenColor forces colour on regardless of where the test runs. +// CLICOLOR_FORCE overrides the not-a-terminal answer; TERM picks the depth. +var goldenColor = []string{"CLICOLOR_FORCE=1", "TERM=xterm-256color"} + +// assertGolden compares got against testdata/.golden, rewriting the file +// instead when -update is passed: +// +// task dc:run:go-builder -- go test ./internal/util/output/ -update +func assertGolden(t *testing.T, name, got string) { + t.Helper() + + path := filepath.Join("testdata", name+".golden") + + if *update { + if err := os.MkdirAll("testdata", 0o755); err != nil { + t.Fatalf("creating testdata: %v", err) + } + + if err := os.WriteFile(path, []byte(got), 0o644); err != nil { + t.Fatalf("writing %s: %v", path, err) + } + + return + } + + want, err := os.ReadFile(path) + if err != nil { + t.Fatalf("reading %s: %v (run the tests with -update to create it)", path, err) + } + + if got != string(want) { + t.Errorf("output does not match %s\n--- got ---\n%q\n--- want ---\n%q", path, got, string(want)) + } +} diff --git a/internal/util/output/log.go b/internal/util/output/log.go new file mode 100644 index 0000000..7c8e678 --- /dev/null +++ b/internal/util/output/log.go @@ -0,0 +1,59 @@ +package output + +import ( + "io" + "log/slog" + "os" +) + +// LevelSilent is above every level slog defines, so a logger set to it emits +// nothing at all. +// +// It is the default, and that is the whole point of the boundary this package +// draws: slog is a diagnostic channel for someone debugging labelsync, not a +// reporting channel for someone using it. A normal run produces no slog records, +// and anything a user is meant to read went through a [Writer] instead. Without +// a silent default, warn- and error-level diagnostics would leak onto stderr and +// interleave with the real output. +const LevelSilent = slog.LevelError + 1 + +// SetupLogger installs the process-wide default slog logger on stderr and +// returns the [slog.LevelVar] gating it. +// +// debug selects [slog.LevelDebug]; otherwise the level is [LevelSilent] and +// nothing is written. Records are formatted to match the output format, so +// `--debug --output=json` yields a stderr stream that is JSON all the way down. +// +// Cobra parses persistent flags after the root command is built, so the level is +// returned rather than baked in: call SetupLogger once during construction, then +// level.Set(slog.LevelDebug) from PersistentPreRunE once --debug is known. +// +// Stderr, never stdout: `labelsync groups --output=json | jq` must not have debug +// lines spliced into the object stream. +func SetupLogger(stderr io.Writer, format Format, debug bool) *slog.LevelVar { + level := new(slog.LevelVar) + level.Set(LevelSilent) + + if debug { + level.Set(slog.LevelDebug) + } + + slog.SetDefault(slog.New(newHandler(stderr, format, level))) + + return level +} + +// SetupDefaultLogger installs the default slog logger on os.Stderr. +func SetupDefaultLogger(format Format, debug bool) *slog.LevelVar { + return SetupLogger(os.Stderr, format, debug) +} + +func newHandler(w io.Writer, format Format, level *slog.LevelVar) slog.Handler { + opts := &slog.HandlerOptions{Level: level} + + if format == FormatJSON { + return slog.NewJSONHandler(w, opts) + } + + return slog.NewTextHandler(w, opts) +} diff --git a/internal/util/output/log_test.go b/internal/util/output/log_test.go new file mode 100644 index 0000000..f1f0c87 --- /dev/null +++ b/internal/util/output/log_test.go @@ -0,0 +1,138 @@ +package output_test + +import ( + "bytes" + "encoding/json" + "io" + "log/slog" + "os" + "strings" + "testing" + + "github.com/specsnl/labelsync/internal/util/output" +) + +// SetupLogger installs a process-wide default logger, so every test here has to +// put it back. Restoring in TestMain as well keeps a failure inside one test +// from leaking a chatty logger into the rest of the package. +func TestMain(m *testing.M) { + slog.SetDefault(slog.New(slog.NewTextHandler(io.Discard, nil))) + os.Exit(m.Run()) +} + +func withRestoredLogger(t *testing.T) { + t.Helper() + + previous := slog.Default() + + t.Cleanup(func() { slog.SetDefault(previous) }) +} + +// The rule the whole package is built around: a normal run says nothing on the +// diagnostic channel. Anything a user should see went through a Writer. +func TestSetupLogger_SilentWithoutDebug(t *testing.T) { + withRestoredLogger(t) + + var stderr bytes.Buffer + + output.SetupLogger(&stderr, output.FormatPretty, false) + + slog.Debug("resolving groups") + slog.Info("read 12 repositories") + slog.Warn("cache miss") + slog.Error("write failed") + + if stderr.Len() != 0 { + t.Errorf("logger emitted records without --debug: %q", stderr.String()) + } +} + +func TestSetupLogger_DebugEnables(t *testing.T) { + withRestoredLogger(t) + + var stderr bytes.Buffer + + output.SetupLogger(&stderr, output.FormatPretty, true) + + slog.Debug("resolving groups", "count", 3) + + got := stderr.String() + if !strings.Contains(got, "resolving groups") { + t.Errorf("--debug produced no record: %q", got) + } + + if !strings.Contains(got, "count=3") { + t.Errorf("text handler dropped the attribute: %q", got) + } +} + +// Cobra parses persistent flags after the command tree is built, so the level +// has to stay adjustable once the logger is installed. +func TestSetupLogger_LevelVarFlipsLater(t *testing.T) { + withRestoredLogger(t) + + var stderr bytes.Buffer + + level := output.SetupLogger(&stderr, output.FormatPretty, false) + + slog.Debug("before") + + if stderr.Len() != 0 { + t.Fatalf("logger was not silent before the flip: %q", stderr.String()) + } + + level.Set(slog.LevelDebug) + slog.Debug("after") + + if !strings.Contains(stderr.String(), "after") { + t.Errorf("level.Set(LevelDebug) did not enable the logger: %q", stderr.String()) + } +} + +// --debug --output=json has to leave stderr parseable too, not text lines +// interleaved with a JSON stdout stream. +func TestSetupLogger_JSONFormat(t *testing.T) { + withRestoredLogger(t) + + var stderr bytes.Buffer + + output.SetupLogger(&stderr, output.FormatJSON, true) + + slog.Debug("resolving groups", "count", 3) + + var record map[string]any + if err := json.Unmarshal([]byte(strings.TrimSpace(stderr.String())), &record); err != nil { + t.Fatalf("json format did not produce a JSON record: %v (%q)", err, stderr.String()) + } + + if record["msg"] != "resolving groups" { + t.Errorf("msg = %v, want %q", record["msg"], "resolving groups") + } +} + +// The diagnostic channel never touches stdout: `labelsync groups --output=json | +// jq` must not have debug lines spliced into the object stream. +func TestSetupLogger_NeverWritesToStdout(t *testing.T) { + withRestoredLogger(t) + + var stdout, stderr bytes.Buffer + + output.SetupLogger(&stderr, output.FormatPretty, true) + output.NewPrettyWriter(&stdout, &stderr, goldenPlain).Info("resolving groups") + + before := stdout.Len() + + slog.Debug("this is a diagnostic") + + if stdout.Len() != before { + t.Errorf("a slog record reached stdout: %q", stdout.String()) + } +} + +func TestLevelSilent_IsAboveEveryLevel(t *testing.T) { + for _, level := range []slog.Level{slog.LevelDebug, slog.LevelInfo, slog.LevelWarn, slog.LevelError} { + if output.LevelSilent <= level { + t.Errorf("LevelSilent (%v) does not suppress %v", slog.Level(output.LevelSilent), level) + } + } +} diff --git a/internal/util/output/rows.go b/internal/util/output/rows.go new file mode 100644 index 0000000..57f8602 --- /dev/null +++ b/internal/util/output/rows.go @@ -0,0 +1,91 @@ +package output + +// Column describes one column of a pretty table: the heading a human reads, and +// how a row renders in that column. +// +// The cell function is what lets the two audiences disagree without either one +// compromising. A size is an int64 in the JSON record and "1.2 MiB" in the +// table; a timestamp is RFC 3339 in the record and "3 days ago" in the table. A +// column with no backing field at all — a computed total, a marker — is just a +// function that ignores most of its argument. +type Column[T any] struct { + // Header is the column heading. Prose: it may be reworded freely, because no + // machine reads it. + Header string + + // Cell renders one row in this column. + Cell func(T) string +} + +// Col builds a [Column]. It exists for type inference: T is deduced from the +// cell function, so a call site names the row type once rather than once per +// column. +// +// output.Table(w, groups, +// output.Col("Group", func(g GroupRow) string { return g.Name }), +// output.Col("Repositories", func(g GroupRow) string { return strconv.Itoa(g.Count) }), +// ) +func Col[T any](header string, cell func(T) string) Column[T] { + return Column[T]{Header: header, Cell: cell} +} + +// TableData is a table prepared for both audiences: display strings for the +// human, and the source values themselves for the machine. +// +// Build it with [Table] rather than by hand. Going through the constructor is +// what guarantees every row has exactly one cell per header — the alignment a +// [][]string could silently get wrong. +type TableData struct { + // Headers are the column headings, in order. + Headers []string + + // Cells are the rendered rows, each with one entry per header. + Cells [][]string + + // Records are the source rows, one per entry in Cells, marshalled as-is by + // [JSONWriter.WriteTable]. + Records []any +} + +// Table writes rows as the command's product: a bordered table for a human, one +// JSON object per row for a machine. +// +// The two renderings come from different places, which is the point. The table +// comes from cols. The JSON comes from marshalling each row directly, so its +// keys and types are the struct's own: +// +// type GroupRow struct { +// Name string `json:"group"` +// Repositories int `json:"repositories"` +// } +// +// gives {"group":"websites","repositories":12} — a number, not "12", so a +// consumer can filter on it. Those json tags are a public contract in the same +// way error_kind is: they may be added to, not renamed. +// +// Passing something other than a tagged struct is legal and rarely what you +// want: a plain string row marshals to a bare JSON string, which is a valid +// NDJSON line that no field selector can read. +func Table[T any](w Writer, rows []T, cols ...Column[T]) { + data := TableData{ + Headers: make([]string, len(cols)), + Cells: make([][]string, len(rows)), + Records: make([]any, len(rows)), + } + + for i, col := range cols { + data.Headers[i] = col.Header + } + + for i, row := range rows { + cells := make([]string, len(cols)) + for j, col := range cols { + cells[j] = col.Cell(row) + } + + data.Cells[i] = cells + data.Records[i] = row + } + + w.WriteTable(data) +} diff --git a/internal/util/output/table.go b/internal/util/output/table.go new file mode 100644 index 0000000..10b011d --- /dev/null +++ b/internal/util/output/table.go @@ -0,0 +1,121 @@ +package output + +import ( + "strings" + + "charm.land/lipgloss/v2" +) + +var ( + tableHeaderStyle = lipgloss.NewStyle().Bold(true) + tableBorderStyle = lipgloss.NewStyle().Foreground(lipgloss.ANSIColor(240)) +) + +// columnGap separates adjacent columns. Two spaces read as a column break +// without needing a rule between them. +const columnGap = " " + +// RenderColumns aligns rows into columns and returns them as lines, with no +// header, no border, and no trailing whitespace. +// +// This is the renderer behind the pretty diff. The diff is a list, not a table — +// there is nothing to put in a header row and a box around it would fight the +// per-repository grouping — but the action, name, and colour columns still have +// to line up down the page or the changes are unreadable: +// +// + create type: bug #d73a4a "Something isn't working" +// ~ recolour wontfix #d73a4a → #16a3c4 +// = ok priority: high +// +// Callers supply the gutter (the +/~/=) as the first cell. Short rows are fine: +// a row with fewer cells than the widest simply ends early. +func RenderColumns(rows [][]string) string { + widths := columnWidths(rows) + lines := make([]string, len(rows)) + + for i, row := range rows { + lines[i] = strings.TrimRight(joinCells(row, widths), " ") + } + + return strings.Join(lines, "\n") +} + +// RenderTable renders headers and rows as a bordered, column-aligned table. +// Column widths are computed from the content, headers included. +// +// This is what [PrettyWriter.WriteTable] uses — list output such as `groups` or +// `cache info`, where a header row is meaningful. For the diff, see +// [RenderColumns]. +func RenderTable(headers []string, rows [][]string) string { + widths := columnWidths(append([][]string{headers}, rows...)) + + var sb strings.Builder + + sb.WriteString(tableHeaderStyle.Render(strings.TrimRight(joinCells(headers, widths), " "))) + sb.WriteString("\n") + sb.WriteString(tableBorderStyle.Render(strings.Repeat("─", totalWidth(widths)))) + + for _, row := range rows { + sb.WriteString("\n") + sb.WriteString(strings.TrimRight(joinCells(row, widths), " ")) + } + + return lipgloss.NewStyle(). + Border(lipgloss.NormalBorder()). + BorderForeground(lipgloss.ANSIColor(240)). + Padding(0, 1). + Render(sb.String()) +} + +// columnWidths returns the display width of the widest cell in each column. +// Width, not byte length: a label name is free to contain an emoji, and len() +// would over-pad the column by several characters. +func columnWidths(rows [][]string) []int { + var widths []int + + for _, row := range rows { + for i, cell := range row { + for len(widths) <= i { + widths = append(widths, 0) + } + + if w := lipgloss.Width(cell); w > widths[i] { + widths[i] = w + } + } + } + + return widths +} + +// joinCells pads each cell to its column width and joins them with columnGap. +// The final cell is padded too; callers trim the line. +func joinCells(row []string, widths []int) string { + var sb strings.Builder + + for i, cell := range row { + if i > 0 { + sb.WriteString(columnGap) + } + + sb.WriteString(cell) + sb.WriteString(strings.Repeat(" ", max(0, widths[i]-lipgloss.Width(cell)))) + } + + return sb.String() +} + +// totalWidth is the rendered width of a full row: every column plus the gaps +// between them. +func totalWidth(widths []int) int { + total := 0 + for _, w := range widths { + total += w + } + + if len(widths) > 1 { + total += (len(widths) - 1) * len(columnGap) + } + + return total +} diff --git a/internal/util/output/table_test.go b/internal/util/output/table_test.go new file mode 100644 index 0000000..d19b94a --- /dev/null +++ b/internal/util/output/table_test.go @@ -0,0 +1,84 @@ +package output_test + +import ( + "strings" + "testing" + + "github.com/specsnl/labelsync/internal/util/output" +) + +// diffRows is the design plan's pretty diff sample, as the planner will hand it +// over: a gutter cell, an action, a name, a colour transition, and an optional +// trailing note. Rows are deliberately ragged — a no-op has nothing to say about +// colour. +var diffRows = [][]string{ + {"+", "create", "type: bug", "#d73a4a", `"Something isn't working"`}, + {"~", "update", "type: feature", "#1d76db → #0e8a16"}, + {"~", "recolour", "wontfix", "#d73a4a → #16a3c4", `(displaced by "type: bug")`}, + {"=", "ok", "priority: high"}, + {"-", "delete", "old-label", "", "(unconfigured)"}, +} + +func TestRenderColumns_Golden(t *testing.T) { + assertGolden(t, "columns_diff", output.RenderColumns(diffRows)) +} + +func TestRenderColumns_NoTrailingWhitespace(t *testing.T) { + for i, line := range strings.Split(output.RenderColumns(diffRows), "\n") { + if line != strings.TrimRight(line, " ") { + t.Errorf("line %d has trailing whitespace: %q", i+1, line) + } + } +} + +// Every cell in a column starts at the same offset, or the diff is unreadable. +func TestRenderColumns_AlignsColumns(t *testing.T) { + lines := strings.Split(output.RenderColumns(diffRows), "\n") + + // Column 2 is the action verb, present in every sample row. "recolour" is + // the widest at 8 characters, and column 0 is one character plus the gap. + const wantOffset = 1 + 2 + + for i, line := range lines { + verb := strings.Fields(line)[1] + if got := strings.Index(line, verb); got != wantOffset { + t.Errorf("line %d: action column starts at %d, want %d: %q", i+1, got, wantOffset, line) + } + } +} + +func TestRenderColumns_Empty(t *testing.T) { + if got := output.RenderColumns(nil); got != "" { + t.Errorf("RenderColumns(nil) = %q, want empty", got) + } +} + +// Width is measured in display columns, not bytes: a label name may contain an +// emoji, and len() would over-pad the column by several characters. +func TestRenderColumns_WideRunes(t *testing.T) { + // "🐛 bug" is six display columns but ten bytes. Measured by len() the first + // column would be sized at ten and every other row pushed four spaces too far + // right. + got := output.RenderColumns([][]string{ + {"🐛 bug", "#d73a4a"}, + {"wide", "#000000"}, + }) + + want := "🐛 bug #d73a4a\nwide #000000" + if got != want { + t.Errorf("RenderColumns measured bytes rather than display width:\ngot:\n%s\nwant:\n%s", got, want) + } +} + +func TestRenderTable_Golden(t *testing.T) { + data := sampleTable() + + assertGolden(t, "table_groups", output.RenderTable(data.Headers, data.Cells)) +} + +func TestRenderTable_NoRows(t *testing.T) { + got := output.RenderTable(sampleTable().Headers, nil) + if !strings.Contains(got, "Group") { + t.Errorf("header row missing from an empty table:\n%s", got) + } +} diff --git a/internal/util/output/testdata/columns_diff.golden b/internal/util/output/testdata/columns_diff.golden new file mode 100644 index 0000000..c24a8a5 --- /dev/null +++ b/internal/util/output/testdata/columns_diff.golden @@ -0,0 +1,5 @@ ++ create type: bug #d73a4a "Something isn't working" +~ update type: feature #1d76db → #0e8a16 +~ recolour wontfix #d73a4a → #16a3c4 (displaced by "type: bug") += ok priority: high +- delete old-label (unconfigured) \ No newline at end of file diff --git a/internal/util/output/testdata/json_stderr.golden b/internal/util/output/testdata/json_stderr.golden new file mode 100644 index 0000000..ba0d446 --- /dev/null +++ b/internal/util/output/testdata/json_stderr.golden @@ -0,0 +1,3 @@ +{"level":"info","message":"resolving 3 groups"} +{"level":"warn","message":"skipping specsnl/old-thing: archived"} +{"error_kind":"repo_inaccessible","level":"error","message":"repository is inaccessible: specsnl/old-thing"} diff --git a/internal/util/output/testdata/json_stdout.golden b/internal/util/output/testdata/json_stdout.golden new file mode 100644 index 0000000..2692772 --- /dev/null +++ b/internal/util/output/testdata/json_stdout.golden @@ -0,0 +1,3 @@ +{"group":"websites","repositories":12,"source":"org: specsnl"} +{"group":"platform","repositories":3,"source":"repos:"} +{"group":"archive","repositories":0,"source":"org: specsnl (excluded)"} diff --git a/internal/util/output/testdata/pretty_stderr_color.golden b/internal/util/output/testdata/pretty_stderr_color.golden new file mode 100644 index 0000000..a44a0c2 --- /dev/null +++ b/internal/util/output/testdata/pretty_stderr_color.golden @@ -0,0 +1,3 @@ +info resolving 3 groups +warn skipping specsnl/old-thing: archived +error repository is inaccessible: specsnl/old-thing diff --git a/internal/util/output/testdata/pretty_stderr_nocolor.golden b/internal/util/output/testdata/pretty_stderr_nocolor.golden new file mode 100644 index 0000000..7f25676 --- /dev/null +++ b/internal/util/output/testdata/pretty_stderr_nocolor.golden @@ -0,0 +1,3 @@ +info resolving 3 groups +warn skipping specsnl/old-thing: archived +error repository is inaccessible: specsnl/old-thing diff --git a/internal/util/output/testdata/pretty_stdout_color.golden b/internal/util/output/testdata/pretty_stdout_color.golden new file mode 100644 index 0000000..2b9e7b9 --- /dev/null +++ b/internal/util/output/testdata/pretty_stdout_color.golden @@ -0,0 +1,7 @@ +┌─────────────────────────────────────────────────┐ +│ Group Repositories Source │ +│ ─────────────────────────────────────────────── │ +│ websites 12 org: specsnl │ +│ platform 3 repos: │ +│ archive 0 org: specsnl (excluded) │ +└─────────────────────────────────────────────────┘ diff --git a/internal/util/output/testdata/pretty_stdout_nocolor.golden b/internal/util/output/testdata/pretty_stdout_nocolor.golden new file mode 100644 index 0000000..4556108 --- /dev/null +++ b/internal/util/output/testdata/pretty_stdout_nocolor.golden @@ -0,0 +1,7 @@ +┌─────────────────────────────────────────────────┐ +│ Group Repositories Source │ +│ ─────────────────────────────────────────────── │ +│ websites 12 org: specsnl │ +│ platform 3 repos: │ +│ archive 0 org: specsnl (excluded) │ +└─────────────────────────────────────────────────┘ diff --git a/internal/util/output/testdata/table_groups.golden b/internal/util/output/testdata/table_groups.golden new file mode 100644 index 0000000..3775e2e --- /dev/null +++ b/internal/util/output/testdata/table_groups.golden @@ -0,0 +1,7 @@ +┌─────────────────────────────────────────────────┐ +│ Group Repositories Source │ +│ ─────────────────────────────────────────────── │ +│ websites 12 org: specsnl │ +│ platform 3 repos: │ +│ archive 0 org: specsnl (excluded) │ +└─────────────────────────────────────────────────┘ \ No newline at end of file diff --git a/internal/util/output/tty.go b/internal/util/output/tty.go new file mode 100644 index 0000000..4273058 --- /dev/null +++ b/internal/util/output/tty.go @@ -0,0 +1,41 @@ +package output + +import ( + "github.com/charmbracelet/x/term" +) + +// fileDescriptor is the only thing a terminal check actually needs. Asserting +// this rather than an io.Writer is what lets [IsTTY] answer for stdin as +// naturally as for stdout, and what lets it accept the io.Reader that +// cmd.InOrStdin returns. +type fileDescriptor interface { + Fd() uintptr +} + +// IsTTY reports whether stream is an interactive terminal. It takes any stream — +// os.Stdin as readily as os.Stderr — because the two decisions below ask about +// different ones. +// +// Styling does not need this. [NewPrettyWriter] wraps each stream in a +// colorprofile.Writer, which makes the same determination itself and strips the +// escape sequences when the answer is no. This is for the decisions that are not +// about colour: +// +// - the rate-limit countdown, which rewrites one line with \r on a terminal and +// logs at a fixed interval into a pipe, because control characters in a CI +// log are unreadable. It asks about stderr, where it draws; +// - the prune prompt, which must never be shown to a pipe — a CI job blocked on +// an invisible prompt hangs until it is cancelled. It asks about stdin, +// because the hang it prevents is a read with nobody to answer it. A job with +// a terminal on stderr and its stdin closed must still refuse to prompt. +// +// Anything without a file descriptor — a bytes.Buffer in a test, say — is not a +// terminal. +func IsTTY(stream any) bool { + f, ok := stream.(fileDescriptor) + if !ok { + return false + } + + return term.IsTerminal(f.Fd()) +} diff --git a/internal/util/output/writer.go b/internal/util/output/writer.go new file mode 100644 index 0000000..38c21d4 --- /dev/null +++ b/internal/util/output/writer.go @@ -0,0 +1,244 @@ +// Package output is the single channel for user-facing output. +// +// Everything a user is meant to read goes through a [Writer] — never +// fmt.Println. Two implementations back the --output flag: [PrettyWriter] +// renders lipgloss-styled text for a human, [JSONWriter] renders NDJSON for a +// machine. Both are constructed with explicit streams so a test can capture +// them. +// +// # slog is not this +// +// log/slog is a debug-only diagnostic channel on stderr, silent on a normal run +// and never used for reporting. See [SetupLogger]. The boundary is settled here +// so that no later package has to think about it: if a user is supposed to see +// it, it is a Writer call; if it only helps someone debugging, it is slog. +// +// # Writes are best-effort +// +// Writer has no error channel, and reporting a failed write would itself need a +// working stream. Output calls therefore discard their error, which is why +// .golangci.yml excludes fmt.Fprintln and lipgloss.Fprintln from errcheck. +package output + +import ( + "encoding/json" + "fmt" + "io" + "os" + + "charm.land/lipgloss/v2" + "github.com/charmbracelet/colorprofile" + "github.com/specsnl/labelsync/internal/labelsync" +) + +// Format names an --output value. The strings are the flag values themselves. +type Format string + +const ( + // FormatPretty is lipgloss-styled text for a human reader. The default. + FormatPretty Format = "pretty" + + // FormatJSON is newline-delimited JSON: one object per line, so a consumer + // can parse the stream as it arrives rather than waiting for the run to end. + FormatJSON Format = "json" +) + +var ( + styleInfo = lipgloss.NewStyle().Bold(true).Foreground(lipgloss.ANSIColor(12)) // bright blue + styleWarn = lipgloss.NewStyle().Bold(true).Foreground(lipgloss.ANSIColor(11)) // bright yellow + styleError = lipgloss.NewStyle().Bold(true).Foreground(lipgloss.ANSIColor(9)) // bright red +) + +// Writer is the interface every user-facing message passes through. +// +// [Writer.WriteTable] is the only method on stdout, and that asymmetry is the point: +// stdout is the product, and everything else — progress, warnings, failures — +// is the story of making it. It is what keeps `labelsync groups --output=json | +// jq` working when a repository turns out to be inaccessible halfway through, +// and it means a JSON run puts nothing on stdout that a consumer cannot type. +type Writer interface { + // Info reports normal progress on stderr. Progress is narration, not the + // result the command exists to produce: it must not land in the file when a + // user redirects stdout. If a command needs a result line that is not a + // table, that is a new method, not this one. + Info(format string, args ...any) + + // Warn reports a recoverable problem on stderr — a skipped repository, say. + Warn(format string, args ...any) + + // Error reports a failure on stderr. + Error(format string, args ...any) + + // WriteErr renders err as an error-level message on stderr. JSON output + // carries an "error_kind" field when err wraps a known labelsync sentinel. + WriteErr(err error) + + // WriteTable renders a prepared table. Pretty output aligns the cells into a + // bordered table; JSON output marshals one source record per line. + // + // Call [Table] rather than this: the generic constructor is what keeps the + // cells aligned with the headers and pairs each rendered row with the record + // it came from. + WriteTable(t TableData) +} + +// Both implementations satisfy Writer. +var ( + _ Writer = (*PrettyWriter)(nil) + _ Writer = (*JSONWriter)(nil) +) + +// PrettyWriter writes lipgloss-styled output for a human reader. +// +// Colour support is detected once, at construction, from the streams and the +// environment — see [NewPrettyWriter]. A run piped to a file or a CI log gets +// the same text with the escape sequences stripped, not a wall of \x1b[. +type PrettyWriter struct { + stdout io.Writer + stderr io.Writer +} + +// NewPrettyWriter creates a PrettyWriter over the given streams. +// +// Each stream is wrapped in a colorprofile.Writer, which downsamples or strips +// colour based on what that stream can actually render: truecolor, 256-colour, +// 16-colour, or none at all when it is not a terminal. NO_COLOR, CLICOLOR, and +// CLICOLOR_FORCE are honoured. +// +// Pass nil for environ to use the process environment. Tests pass an explicit +// slice — []string{} for a plain, golden-comparable rendering — so the output +// does not depend on whoever is running them. +func NewPrettyWriter(stdout, stderr io.Writer, environ []string) *PrettyWriter { + return &PrettyWriter{ + stdout: colorprofile.NewWriter(stdout, environ), + stderr: colorprofile.NewWriter(stderr, environ), + } +} + +// NewDefaultPrettyWriter creates a PrettyWriter over os.Stdout and os.Stderr, +// detecting colour support from the process environment. +func NewDefaultPrettyWriter() *PrettyWriter { + return NewPrettyWriter(os.Stdout, os.Stderr, nil) +} + +// Info reports normal progress on stderr. +func (w *PrettyWriter) Info(format string, args ...any) { + fmt.Fprintln(w.stderr, styleInfo.Render("info")+" "+fmt.Sprintf(format, args...)) +} + +// Warn reports a recoverable problem on stderr. +func (w *PrettyWriter) Warn(format string, args ...any) { + fmt.Fprintln(w.stderr, styleWarn.Render("warn")+" "+fmt.Sprintf(format, args...)) +} + +// Error reports a failure on stderr. +func (w *PrettyWriter) Error(format string, args ...any) { + fmt.Fprintln(w.stderr, styleError.Render("error")+" "+fmt.Sprintf(format, args...)) +} + +// WriteErr renders err as an error-level message. The sentinel kind is not +// shown: it exists for machines, and the message already reads well. +func (w *PrettyWriter) WriteErr(err error) { + w.Error("%v", err) +} + +// WriteTable renders a bordered, column-aligned table on stdout. The records +// are ignored: a human reads the cells. +func (w *PrettyWriter) WriteTable(t TableData) { + fmt.Fprintln(w.stdout, RenderTable(t.Headers, t.Cells)) +} + +// JSONWriter writes NDJSON: exactly one self-contained JSON object per line, so +// a consumer can parse the stream mid-run instead of waiting for a closing +// bracket that a killed process would never write. +// +// Objects carry a "level" field. Tables go to stdout; progress, warnings, and +// errors go to stderr — so every line on stdout is a data record with the same +// keys, and `jq -r .group` never trips over a narration object that has no +// group. +type JSONWriter struct { + stdout io.Writer + stderr io.Writer +} + +// NewJSONWriter creates a JSONWriter over the given streams. There is no colour +// to detect, so no environment is consulted. +func NewJSONWriter(stdout, stderr io.Writer) *JSONWriter { + return &JSONWriter{stdout: stdout, stderr: stderr} +} + +// NewDefaultJSONWriter creates a JSONWriter over os.Stdout and os.Stderr. +func NewDefaultJSONWriter() *JSONWriter { + return NewJSONWriter(os.Stdout, os.Stderr) +} + +// Info emits an info-level object on stderr. +func (w *JSONWriter) Info(format string, args ...any) { + writeJSONLine(w.stderr, map[string]string{ + "level": "info", + "message": fmt.Sprintf(format, args...), + }) +} + +// Warn emits a warn-level object on stderr. +func (w *JSONWriter) Warn(format string, args ...any) { + writeJSONLine(w.stderr, map[string]string{ + "level": "warn", + "message": fmt.Sprintf(format, args...), + }) +} + +// Error emits an error-level object on stderr. +func (w *JSONWriter) Error(format string, args ...any) { + writeJSONLine(w.stderr, map[string]string{ + "level": "error", + "message": fmt.Sprintf(format, args...), + }) +} + +// WriteErr emits an error-level object on stderr, with an "error_kind" field +// when err wraps a known sentinel. The kind strings are the stable half of the +// contract — the message is free to be reworded, error_kind is not. +func (w *JSONWriter) WriteErr(err error) { + payload := map[string]string{ + "level": "error", + "message": err.Error(), + } + + if kind := labelsync.KindOf(err); kind != "" { + payload["error_kind"] = kind + } + + writeJSONLine(w.stderr, payload) +} + +// WriteTable emits one object per source record on stdout — not a single array. +// An array could only be written once the last row was known, which is exactly +// the mid-run parseability NDJSON exists to preserve. +// +// The records are marshalled as they are, so the keys and the types are the +// row struct's own: a count stays a number, and a heading reworded for the +// pretty table cannot disturb a consumer's filter. The headers and cells are +// ignored here — they are the human's rendering of the same rows. +func (w *JSONWriter) WriteTable(t TableData) { + for _, record := range t.Records { + writeJSONLine(w.stdout, record) + } +} + +// writeJSONLine writes v as exactly one line of JSON. json.Encoder.Encode +// terminates every value with a newline, which is what makes the stream NDJSON +// by construction rather than by convention. +// +// HTML escaping is off: label names legitimately contain characters like & and +// <, and rendering them as & helps nobody reading a log. +// +// The error is discarded for the reason given in the package doc — Writer has no +// error channel. Encoding cannot fail for the map[string]string payloads used +// here, so a non-nil error can only mean the stream is already gone. +func writeJSONLine(w io.Writer, v any) { + enc := json.NewEncoder(w) + enc.SetEscapeHTML(false) + + _ = enc.Encode(v) +} diff --git a/internal/util/output/writer_test.go b/internal/util/output/writer_test.go new file mode 100644 index 0000000..e612727 --- /dev/null +++ b/internal/util/output/writer_test.go @@ -0,0 +1,383 @@ +package output_test + +import ( + "bytes" + "encoding/json" + "errors" + "fmt" + "os" + "strconv" + "strings" + "testing" + + "github.com/specsnl/labelsync/internal/labelsync" + "github.com/specsnl/labelsync/internal/util/output" +) + +// groupRow is the `labelsync groups` shape: the row type both writers project, +// one into cells and the other into JSON. Repositories is an int on purpose — +// the count a consumer would want to filter on numerically. +type groupRow struct { + Group string `json:"group"` + Repositories int `json:"repositories"` + Source string `json:"source"` +} + +var ( + sampleRows = []groupRow{ + {Group: "websites", Repositories: 12, Source: "org: specsnl"}, + {Group: "platform", Repositories: 3, Source: "repos:"}, + {Group: "archive", Repositories: 0, Source: "org: specsnl (excluded)"}, + } + + sampleColumns = []output.Column[groupRow]{ + output.Col("Group", func(g groupRow) string { return g.Group }), + output.Col("Repositories", func(g groupRow) string { return strconv.Itoa(g.Repositories) }), + output.Col("Source", func(g groupRow) string { return g.Source }), + } +) + +// sampleTable is what the writers receive, prepared the way a command would +// prepare it. +func sampleTable() output.TableData { + var captured output.TableData + + output.Table(captureWriter{onTable: func(t output.TableData) { captured = t }}, sampleRows, sampleColumns...) + + return captured +} + +// captureWriter records the prepared table and discards everything else, so a +// test can assert on what output.Table built rather than on how it rendered. +type captureWriter struct { + onTable func(output.TableData) +} + +func (captureWriter) Info(string, ...any) {} +func (captureWriter) Warn(string, ...any) {} +func (captureWriter) Error(string, ...any) {} +func (captureWriter) WriteErr(error) {} + +func (w captureWriter) WriteTable(t output.TableData) { w.onTable(t) } + +// writeSampleRun exercises every Writer method once, in the order a real run +// would: progress, a table, a recoverable skip, then a failure. +func writeSampleRun(w output.Writer) { + w.Info("resolving %d groups", 3) + output.Table(w, sampleRows, sampleColumns...) + w.Warn("skipping %s: archived", "specsnl/old-thing") + w.WriteErr(fmt.Errorf("%w: specsnl/old-thing", labelsync.ErrRepoInaccessible)) +} + +func TestPrettyWriter_Golden_NoColor(t *testing.T) { + var stdout, stderr bytes.Buffer + + writeSampleRun(output.NewPrettyWriter(&stdout, &stderr, goldenPlain)) + + assertGolden(t, "pretty_stdout_nocolor", stdout.String()) + assertGolden(t, "pretty_stderr_nocolor", stderr.String()) +} + +// The colour golden is the other half of the same claim: the styling is really +// there, and the plain golden above is degradation rather than an absence of +// styles. +func TestPrettyWriter_Golden_Color(t *testing.T) { + var stdout, stderr bytes.Buffer + + writeSampleRun(output.NewPrettyWriter(&stdout, &stderr, goldenColor)) + + assertGolden(t, "pretty_stdout_color", stdout.String()) + assertGolden(t, "pretty_stderr_color", stderr.String()) +} + +// A buffer is not a terminal, so nothing styled may survive to it. +func TestPrettyWriter_StripsEscapesOffTerminal(t *testing.T) { + var stdout, stderr bytes.Buffer + + writeSampleRun(output.NewPrettyWriter(&stdout, &stderr, goldenPlain)) + + for name, got := range map[string]string{"stdout": stdout.String(), "stderr": stderr.String()} { + if strings.Contains(got, "\x1b[") { + t.Errorf("%s contains an ANSI escape sequence off a terminal: %q", name, got) + } + } +} + +// Table is the only method on stdout. Everything else is narration, and +// narration in a redirected file is the defect this split exists to prevent. +func TestWriters_OnlyTableReachesStdout(t *testing.T) { + for _, tc := range []struct { + name string + make func(stdout, stderr *bytes.Buffer) output.Writer + }{ + {"pretty", func(stdout, stderr *bytes.Buffer) output.Writer { + return output.NewPrettyWriter(stdout, stderr, goldenPlain) + }}, + {"json", func(stdout, stderr *bytes.Buffer) output.Writer { + return output.NewJSONWriter(stdout, stderr) + }}, + } { + t.Run(tc.name, func(t *testing.T) { + var stdout, stderr bytes.Buffer + + w := tc.make(&stdout, &stderr) + w.Info("resolving %d groups", 3) + w.Warn("careful") + w.Error("broken") + w.WriteErr(errors.New("fatal")) + + if stdout.Len() != 0 { + t.Errorf("non-table output reached stdout: %q", stdout.String()) + } + + for _, want := range []string{"resolving 3 groups", "careful", "broken", "fatal"} { + if !strings.Contains(stderr.String(), want) { + t.Errorf("stderr missing %q, got: %q", want, stderr.String()) + } + } + }) + } +} + +// The reason Info is on stderr rather than a style preference: every line on +// stdout has to be a data record with the same keys, or a consumer's filter +// silently yields null for the narration. +func TestJSONWriter_StdoutIsUniformlyTyped(t *testing.T) { + var stdout, stderr bytes.Buffer + + writeSampleRun(output.NewJSONWriter(&stdout, &stderr)) + + for i, line := range strings.Split(strings.TrimSuffix(stdout.String(), "\n"), "\n") { + var obj map[string]any + if err := json.Unmarshal([]byte(line), &obj); err != nil { + t.Fatalf("stdout line %d is not a JSON object: %v", i+1, err) + } + + if _, ok := obj["group"]; !ok { + t.Errorf("stdout line %d is not a data record: %q", i+1, line) + } + + if _, ok := obj["level"]; ok { + t.Errorf("stdout line %d carries a level, so it is narration: %q", i+1, line) + } + } +} + +func TestJSONWriter_Golden(t *testing.T) { + var stdout, stderr bytes.Buffer + + writeSampleRun(output.NewJSONWriter(&stdout, &stderr)) + + assertGolden(t, "json_stdout", stdout.String()) + assertGolden(t, "json_stderr", stderr.String()) +} + +// The NDJSON contract: every line is a complete object on its own, so a consumer +// can parse the stream as it arrives and a run killed halfway still leaves valid +// lines behind. A single JSON array would satisfy neither. +func TestJSONWriter_OneObjectPerLine(t *testing.T) { + var stdout, stderr bytes.Buffer + + writeSampleRun(output.NewJSONWriter(&stdout, &stderr)) + + for stream, out := range map[string]string{"stdout": stdout.String(), "stderr": stderr.String()} { + if !strings.HasSuffix(out, "\n") { + t.Errorf("%s does not end with a newline: %q", stream, out) + } + + for i, line := range strings.Split(strings.TrimSuffix(out, "\n"), "\n") { + var obj map[string]any + if err := json.Unmarshal([]byte(line), &obj); err != nil { + t.Errorf("%s line %d is not a JSON object: %v (%q)", stream, i+1, err, line) + } + } + } +} + +// A table is one object per row — three lines for the three sample rows — and +// never a single array. +func TestJSONWriter_TableEmitsOneObjectPerRow(t *testing.T) { + var stdout bytes.Buffer + + output.NewJSONWriter(&stdout, &bytes.Buffer{}).WriteTable(sampleTable()) + + lines := strings.Split(strings.TrimSuffix(stdout.String(), "\n"), "\n") + if len(lines) != len(sampleRows) { + t.Fatalf("got %d lines, want %d: %q", len(lines), len(sampleRows), stdout.String()) + } + + var first map[string]any + if err := json.Unmarshal([]byte(lines[0]), &first); err != nil { + t.Fatalf("first row is not a JSON object: %v", err) + } + + // The keys are the row struct's own json tags, not a normalisation of the + // column headings, so rewording a heading cannot break a consumer's filter. + want := map[string]any{"group": "websites", "repositories": float64(12), "source": "org: specsnl"} + for key, value := range want { + if first[key] != value { + t.Errorf("row[%q] = %#v, want %#v (got %v)", key, first[key], value, first) + } + } +} + +// The reason the JSON side marshals records rather than cells: a count stays a +// number, so `jq 'select(.repositories > 5)'` works. Rendering it through the +// table would have made it the string "12". +func TestJSONWriter_TableKeepsValueTypes(t *testing.T) { + var stdout bytes.Buffer + + output.NewJSONWriter(&stdout, &bytes.Buffer{}).WriteTable(sampleTable()) + + first := strings.SplitN(stdout.String(), "\n", 2)[0] + if !strings.Contains(first, `"repositories":12`) { + t.Errorf("count was not emitted as a number: %q", first) + } +} + +// A row can no longer disagree with its headers: the cells are built from the +// columns, for every row, by the same loop. This is the defect the [][]string +// signature could not rule out. +func TestTable_CellsAlignWithHeaders(t *testing.T) { + data := sampleTable() + + if len(data.Headers) != len(sampleColumns) { + t.Fatalf("got %d headers, want %d", len(data.Headers), len(sampleColumns)) + } + + if len(data.Records) != len(data.Cells) { + t.Errorf("got %d records for %d rendered rows", len(data.Records), len(data.Cells)) + } + + for i, cells := range data.Cells { + if len(cells) != len(data.Headers) { + t.Errorf("row %d has %d cells for %d headers: %q", i, len(cells), len(data.Headers), cells) + } + } +} + +// A column need not correspond to a field: the cell function is free to compute, +// reformat, or ignore the row. That is what lets the human rendering differ from +// the record without the record giving up its types. +func TestTable_ComputedColumn(t *testing.T) { + var captured output.TableData + + output.Table( + captureWriter{onTable: func(t output.TableData) { captured = t }}, + sampleRows, + output.Col("Group", func(g groupRow) string { return g.Group }), + output.Col("Empty", func(g groupRow) string { + if g.Repositories == 0 { + return "yes" + } + + return "" + }), + ) + + if got := captured.Cells[2][1]; got != "yes" { + t.Errorf("computed cell = %q, want %q", got, "yes") + } + + // The record is untouched by the rendering: it has no such field. + if got, ok := captured.Records[2].(groupRow); !ok || got.Repositories != 0 { + t.Errorf("record was not passed through intact: %#v", captured.Records[2]) + } +} + +func TestJSONWriter_WriteErr_KnownSentinel(t *testing.T) { + var stderr bytes.Buffer + + err := fmt.Errorf("reading labels: %w", fmt.Errorf("%w: specsnl/old-thing", labelsync.ErrRepoInaccessible)) + output.NewJSONWriter(&bytes.Buffer{}, &stderr).WriteErr(err) + + var obj map[string]string + if uerr := json.Unmarshal([]byte(strings.TrimSpace(stderr.String())), &obj); uerr != nil { + t.Fatalf("not a JSON object: %v", uerr) + } + + if obj["error_kind"] != "repo_inaccessible" { + t.Errorf("error_kind = %q, want %q", obj["error_kind"], "repo_inaccessible") + } + + if obj["level"] != "error" { + t.Errorf("level = %q, want %q", obj["level"], "error") + } + + if obj["message"] != err.Error() { + t.Errorf("message = %q, want %q", obj["message"], err.Error()) + } +} + +// An error wrapping no sentinel gets no error_kind at all, rather than an empty +// one — an empty string would look like a kind that failed to resolve. +func TestJSONWriter_WriteErr_UnknownError(t *testing.T) { + var stderr bytes.Buffer + + output.NewJSONWriter(&bytes.Buffer{}, &stderr).WriteErr(errors.New("something unexpected")) + + if strings.Contains(stderr.String(), "error_kind") { + t.Errorf("unknown error carried an error_kind: %q", stderr.String()) + } +} + +// Label names and descriptions legitimately contain & and <; escaping them to +// & makes a log harder to read and a `jq` result harder to eyeball. +func TestJSONWriter_DoesNotEscapeHTML(t *testing.T) { + var stderr bytes.Buffer + + output.NewJSONWriter(&bytes.Buffer{}, &stderr).Info("status: %s", "blocked & ") + + if !strings.Contains(stderr.String(), "blocked & ") { + t.Errorf("HTML-escaped output: %q", stderr.String()) + } +} + +func TestPrettyWriter_WriteErr_OmitsKind(t *testing.T) { + var stderr bytes.Buffer + + output.NewPrettyWriter(&bytes.Buffer{}, &stderr, goldenPlain). + WriteErr(fmt.Errorf("%w: specsnl/old-thing", labelsync.ErrRepoInaccessible)) + + got := stderr.String() + if !strings.Contains(got, labelsync.ErrRepoInaccessible.Error()) { + t.Errorf("stderr missing the error message: %q", got) + } + + if strings.Contains(got, "repo_inaccessible") { + t.Errorf("pretty output leaked the machine kind string: %q", got) + } +} + +// fdReader is stdin's shape: readable, carrying a descriptor, and with no Write +// method at all. It would not compile against an io.Writer parameter, which is +// the whole reason IsTTY takes any. +type fdReader struct{ f *os.File } + +func (r fdReader) Read(p []byte) (int, error) { return r.f.Read(p) } +func (r fdReader) Fd() uintptr { return r.f.Fd() } + +func TestIsTTY(t *testing.T) { + f, err := os.CreateTemp(t.TempDir(), "not-a-tty") + if err != nil { + t.Fatalf("creating temp file: %v", err) + } + + t.Cleanup(func() { _ = f.Close() }) + + for _, tc := range []struct { + name string + stream any + }{ + {"a buffer has no descriptor", &bytes.Buffer{}}, + {"a regular file is not a terminal", f}, + {"a read-only stream is answerable", fdReader{f: f}}, + {"a plain string is not a stream at all", "stdin"}, + } { + t.Run(tc.name, func(t *testing.T) { + if output.IsTTY(tc.stream) { + t.Errorf("IsTTY(%T) = true, want false", tc.stream) + } + }) + } +}