amadan
cli: add --unit validates the row that would result, not the raw flags
Fix round 1 review findings: the early refusal tested *addr=="" from this invocation's flags before the existing-row addr-preservation merge, so a bare --unit re-add of an addr-backed route was wrongly refused. Now only the unambiguous --socket/--hibernate combinations are refused early; the addr-upstream check runs on the built route (post-merge) via appconfig.StampShape. Also covers carlos add --unit with a new test.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
2 files changed,
+97
−8
cli.go+23 −8cli_test.go+74 −0
diff --git a/cli.go b/cli.go| index 5efe2e338..c5e130aae 100644 |
| --- a/cli.go |
| +++ b/cli.go |
| @@ -114,14 +114,18 @@ func runAdd(args []string) { |
| } |
| // The root pass delivers config to a stamped route only in one shape |
| // (appconfig.StampShape); this door must not mint a row it would refuse |
| - // every pass. --socket is refused even though the default below would |
| - // add one for a bare instance: a stamped route needs --addr. |
| - if *unit != "" { |
| - if *socket != "" || *addr == "" || *hibernate { |
| - log.Fatalf("add %s: --unit %s needs --addr and neither --socket nor --hibernate: the root pass delivers config to a stamped route only when a unit owns an addr-backed, always-on process", host, *unit) |
| - } |
| - } |
| - if *socket == "" && *addr == "" && *kind == "instance" { |
| + // every pass. Only the unambiguous flag-level combinations are refused |
| + // here — --socket and --hibernate never belong on a stamped route, |
| + // whatever else the invocation carries. Whether the row ends up with an |
| + // addr is NOT decided here: a bare --unit re-add of an existing |
| + // addr-backed route has no --addr on this invocation at all, and the |
| + // existing row's Addr is merged in below — so that check waits until |
| + // the row that would actually result is built. |
| + if *unit != "" && (*socket != "" || *hibernate) { |
| + log.Fatalf("add %s: --unit %s needs --addr and neither --socket nor --hibernate: the root pass delivers config to a stamped route only when a unit owns an addr-backed, always-on process", host, *unit) |
| + } |
| + // A stamped route never gets a default socket; it needs an addr. |
| + if *socket == "" && *addr == "" && *kind == "instance" && *unit == "" { |
| if *backing == registry.BackingUnit { |
| // The carlos-app@<host>.socket unit's path — enable that unit |
| // alongside this route (or use --addr for a TCP-only app). Not a |
| @@ -246,6 +250,17 @@ func runAdd(args []string) { |
| // starts converging DNS from it. Preserve it the same way. |
| route.Steering = existing.Steering |
| } |
| + // Now that --addr has had its chance to come from the existing row (an |
| + // addr-preserving re-add), validate the ROW THAT WOULD RESULT the way |
| + // unitRail does: a stamped route the root pass would refuse every pass |
| + // must not be minted here either. Its "no addr upstream" reason is |
| + // accurate for a fresh add with no --addr and no existing row to fall |
| + // back to. |
| + if route.Unit != "" { |
| + if err := appconfig.StampShape(route); err != nil { |
| + log.Fatalf("add %s: %v — the root pass delivers config to a stamped route only when a unit owns an addr-backed, always-on process", host, err) |
| + } |
| + } |
| if err := r.Upsert(route); err != nil { |
| log.Fatal(err) |
| } |
diff --git a/cli_test.go b/cli_test.go| index 74bc19b09..609b5daac 100644 |
| --- a/cli_test.go |
| +++ b/cli_test.go |
| @@ -848,6 +848,80 @@ func TestAddPreservesUpstreamsOnReAdd(t *testing.T) { |
| } |
| } |
| +// TestAddUnitStamp exercises `carlos add --unit` at the door, validating the |
| +// ROW THAT WOULD RESULT the way unitRail does — not the raw flags on this |
| +// one invocation. A bare --unit re-add of an existing addr-backed route |
| +// carries no --addr of its own, but the existing row's Addr is merged in |
| +// before the shape check runs, so it must be accepted (review fix round 1, |
| +// finding 1). A fresh --unit add with no --addr anywhere, or with --socket, |
| +// has no way to reach the supported stamped shape and must be refused. |
| +func TestAddUnitStamp(t *testing.T) { |
| + bin := buildCarlos(t) |
| + tmp := t.TempDir() |
| + scratch := t.TempDir() |
| + regPath := filepath.Join(tmp, "registry.db") |
| + |
| + run := func(args ...string) (string, error) { |
| + t.Helper() |
| + cmd := exec.Command(bin, args...) |
| + cmd.Env = carlosTestEnv(t) |
| + // pick() walks cwd ancestors for .carlos/config, so the subprocess must not run from the real checkout. |
| + cmd.Dir = scratch |
| + out, err := cmd.CombinedOutput() |
| + return string(out), err |
| + } |
| + |
| + // (a) An existing addr-backed route, re-added with --unit and no --addr: |
| + // the existing row's Addr is merged in before the shape check runs, so |
| + // this must be accepted, not refused for "no addr upstream". |
| + const host = "stamped.example.com" |
| + if out, err := run("add", "-registry", regPath, "-app", "stamped", |
| + "-addr", "127.0.0.1:8090", host); err != nil { |
| + t.Fatalf("add: %v\n%s", err, out) |
| + } |
| + if out, err := run("add", "-registry", regPath, "-app", "stamped", |
| + "-unit", "stamped.service", host); err != nil { |
| + t.Fatalf("re-add with --unit and no --addr: %v\n%s", err, out) |
| + } |
| + reg, err := registry.Open(regPath) |
| + if err != nil { |
| + t.Fatalf("open registry: %v", err) |
| + } |
| + defer reg.Close() |
| + rt, err := reg.Get(host) |
| + if err != nil || rt == nil { |
| + t.Fatalf("get %s: %+v, %v", host, rt, err) |
| + } |
| + if rt.Unit != "stamped.service" { |
| + t.Errorf("Unit = %q, want stamped.service", rt.Unit) |
| + } |
| + if rt.Addr != "127.0.0.1:8090" { |
| + t.Errorf("Addr = %q, want the existing row's addr preserved", rt.Addr) |
| + } |
| + |
| + // (b) A fresh add naming --unit with no --addr anywhere has no addr |
| + // upstream in the row that would result, and StampShape refuses it. |
| + out, err := run("add", "-registry", regPath, "-app", "fresh", |
| + "-unit", "fresh.service", "fresh.example.com") |
| + if err == nil { |
| + t.Fatalf("add --unit with no --addr and no existing row: want non-zero exit, got 0\n%s", out) |
| + } |
| + if !strings.Contains(out, "addr") { |
| + t.Errorf("refusal = %q, want it to mention addr", out) |
| + } |
| + |
| + // (c) --unit with --socket is refused at the flag level, before any row |
| + // is built. |
| + out, err = run("add", "-registry", regPath, "-app", "sockety", |
| + "-unit", "sockety.service", "-socket", "/run/sockety.sock", "sockety.example.com") |
| + if err == nil { |
| + t.Fatalf("add --unit --socket: want non-zero exit, got 0\n%s", out) |
| + } |
| + if !strings.Contains(out, "socket") { |
| + t.Errorf("refusal = %q, want it to mention socket", out) |
| + } |
| +} |
| + |
| // --- release versioning: the CLI surface ------------------------------- |
| // |
| // (release versioning spec, 2026-08-08.) Driven through the real binary in |