Put narration on stderr and make the outcome codes combine (GH-13)

Four decisions the original boundary left implicit, taken now because all four
are free today and expensive later: Info has no callers, the exit numbers have
not shipped, and IsTTY has no call sites.

Table is now the only Writer method on stdout. Progress is narration, not the
product, and it must not land in the file when a user redirects stdout. In JSON
mode the cost was concrete: an info object carries none of the keys a data
object does, so jq -r .group yielded null for it. Every line on stdout is now a
data record, which the tests assert directly rather than by inspection. This
diverges from specs-cli, where commands already depend on Info being on stdout;
inheriting the defect to stay symmetrical was the wrong trade.

The outcome codes are disjoint bits that OR together, so a dry run that finds
drift and also cannot reach a repository exits 6 instead of forcing a
precedence rule that throws half the answer away. Skipped moves from 3 to 4 to
make room. Error stays outside the bit space and stays exclusive: when a run
fails the live state is unknown, so "failed and drifted" is not a statement
labelsync can honestly make — and with Error in the mask, 3 would have to mean
exactly that. The cost is that callers test bits rather than equality, which is
written down next to the table.

exit.Err carries a code out to main, because RunE returns an error and nothing
else. A nil Err field means silent: exit 2 on a drifting dry run must not print
an error line, because the drift was the successful result and the diff is
already on stdout. Unwrap is load-bearing — a carrier that hid the error it
wraps would strip error_kind from exactly the failures that carry a code — and
Of maps a carrier holding a failure to Error even when Code was left unset,
since a zero exit on a failed run is the one wrong answer a pipeline cannot
detect.

IsTTY takes any rather than an io.Writer. The prune guard has to ask about
stdin, and the old signature let IsTTY(os.Stdin) compile only because *os.File
happens to have a Write method. The two gated decisions ask about different
streams: the countdown about stderr, where it draws, and the prompt about
stdin, because the hang it prevents is a read with nobody to answer it.

The architecture page absorbs the reasoning that produced all of this, and
gains the Cobra wiring and review checklist that #14 has to follow.

Author: Ilyes512

docs/content/docs/architecture/output.md

@@ -592,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
 

docs/design.md

@@ -1,33 +1,114 @@
-// Package exit holds the process exit codes labelsync returns.
+// 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.
 //
-// The codes are a public contract — CI pipelines branch on them — so they may be
-// added to, never renumbered.
+// # 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 = 0
+	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.
-	Error = 1
+	// 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 = 2
+	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 = 3
+	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)) }

internal/util/exit/exit.go

@@ -1,8 +1,11 @@
 package exit_test
 
 import (
+	"errors"
+	"fmt"
 	"testing"
 
+	"github.com/specsnl/labelsync/internal/labelsync"
 	"github.com/specsnl/labelsync/internal/util/exit"
 )
 
@@ -13,13 +16,13 @@ import (
 func TestCodes_AreStable(t *testing.T) {
 	for _, tc := range []struct {
 		name string
-		got  int
-		want int
+		got  exit.Code
+		want exit.Code
 	}{
 		{"OK", exit.OK, 0},
 		{"Error", exit.Error, 1},
 		{"Drift", exit.Drift, 2},
-		{"Skipped", exit.Skipped, 3},
+		{"Skipped", exit.Skipped, 4},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			if tc.got != tc.want {
@@ -29,22 +32,107 @@ func TestCodes_AreStable(t *testing.T) {
 	}
 }
 
-func TestCodes_AreDistinct(t *testing.T) {
-	seen := map[int]string{}
+// 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 int
+		code exit.Code
 	}{
-		{"OK", exit.OK},
-		{"Error", exit.Error},
 		{"Drift", exit.Drift},
 		{"Skipped", exit.Skipped},
 	} {
-		if other, dup := seen[tc.code]; dup {
-			t.Errorf("exit.%s and exit.%s both equal %d", other, tc.name, tc.code)
+		if tc.code&(tc.code-1) != 0 {
+			t.Errorf("exit.%s = %d has more than one bit set", tc.name, tc.code)
 		}
 
-		seen[tc.code] = tc.name
+		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")
 	}
 }

internal/util/exit/exit_test.go

@@ -1,2 +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"}

internal/util/output/testdata/json_stderr.golden

@@ -1,4 +1,3 @@
-{"level":"info","message":"resolving 3 groups"}
 {"group":"websites","repositories":"12","source":"org: specsnl"}
 {"group":"platform","repositories":"3","source":"repos:"}
 {"group":"archive","repositories":"0","source":"org: specsnl (excluded)"}

internal/util/output/testdata/json_stdout.golden

@@ -1,2 +1,3 @@
+info resolving 3 groups
 warn skipping specsnl/old-thing: archived
 error repository is inaccessible: specsnl/old-thing

internal/util/output/testdata/pretty_stderr_color.golden

@@ -1,2 +1,3 @@
+info resolving 3 groups
 warn skipping specsnl/old-thing: archived
 error repository is inaccessible: specsnl/old-thing

internal/util/output/testdata/pretty_stderr_nocolor.golden

@@ -1,4 +1,3 @@
-info resolving 3 groups
 ┌─────────────────────────────────────────────────┐
 │ Group     Repositories  Source                  │
 │ ─────────────────────────────────────────────── │

internal/util/output/testdata/pretty_stdout_color.golden

@@ -1,4 +1,3 @@
-info resolving 3 groups
 ┌─────────────────────────────────────────────────┐
 │ Group     Repositories  Source                  │
 │ ─────────────────────────────────────────────── │