amadan
cli: fix review findings — refuse -addr over an established Addrs set, honest probe-replace help text, sendJSON collapse
IMPORTANT 1: carlos add -addr over a host that already carries a multi-upstream Addrs set is now refused, not silently cleared — the record owns multi-upstream shape, not this box verb, and the refusal points at `carlos instances set-upstreams`. Silently winning would have built a row ValidateShape forbids elsewhere and been reverted by the next converge tick anyway. TestAddPreservesUpstreamsOnReAdd rewritten to pin the refusal (and that it leaves the row untouched) alongside the still-working flagless-re-add and no-established-set cases. IMPORTANT 2 + MINOR 3/4: instances_cli.go help text corrected — the PATCH/POST probe fields replace the WHOLE probe block (unnamed fields revert to platform defaults), not a per-field update; a single -addr honours -health-path/-status exactly as multi-addr does; -addrs "" is documented as the explicit clear spelling, which 400s on the DerivesPaths rail for a kind:instance record. MINOR 5: postJSON/putJSON/patchJSON collapsed onto one sendJSON(ctx, method, path, in) — the three were byte-identical past the method.
4 files changed,
+120
−102
apimode.go+26 −63cli.go+19 −5cli_test.go+48 −19instances_cli.go+27 −15
diff --git a/apimode.go b/apimode.go| index 981bc9160..1d1d5bfa1 100644 |
| --- a/apimode.go |
| +++ b/apimode.go |
| @@ -283,17 +283,21 @@ func (s *apiSession) getJSON(ctx context.Context, path string) (int, []byte, err |
| return res.StatusCode, body, nil |
| } |
| -// postJSON runs one JSON POST and hands back the status and raw body |
| -// without interpreting a non-200 as an error itself. Unlike |
| -// (*consoleClient).do, which keeps only a bare "error" string, promote and |
| -// rollback need the FULL body on a stable refusal — its console_url would |
| -// otherwise be discarded before the caller ever sees it. |
| -func (s *apiSession) postJSON(ctx context.Context, path string, in any) (int, []byte, error) { |
| +// sendJSON runs one JSON request over the given method and hands back the |
| +// status and raw body without interpreting a non-200 as an error itself — |
| +// the one request-building body postJSON/putJSON/patchJSON share (Task 8 |
| +// review: three byte-identical copies, method aside, was past the line). |
| +// Unlike (*consoleClient).do, which keeps only a bare "error" string, some |
| +// callers (promote, rollback) need the FULL body on a stable refusal — its |
| +// console_url would otherwise be discarded before the caller ever sees it — |
| +// which is why this returns the raw body rather than pre-deciding what |
| +// counts as an error. |
| +func (s *apiSession) sendJSON(ctx context.Context, method, path string, in any) (int, []byte, error) { |
| raw, err := json.Marshal(in) |
| if err != nil { |
| return 0, nil, err |
| } |
| - req, err := http.NewRequestWithContext(ctx, http.MethodPost, s.c.base+path, bytes.NewReader(raw)) |
| + req, err := http.NewRequestWithContext(ctx, method, s.c.base+path, bytes.NewReader(raw)) |
| if err != nil { |
| return 0, nil, err |
| } |
| @@ -313,6 +317,12 @@ func (s *apiSession) postJSON(ctx context.Context, path string, in any) (int, [] |
| return res.StatusCode, body, nil |
| } |
| +// postJSON is sendJSON pinned to POST — the console door shape a create or |
| +// an action verb (promote, restart, ...) always speaks. |
| +func (s *apiSession) postJSON(ctx context.Context, path string, in any) (int, []byte, error) { |
| + return s.sendJSON(ctx, http.MethodPost, path, in) |
| +} |
| + |
| // stableRefusal decodes handleAPIPromote/handleAPIRollback's 403 shape |
| // ({"error", "console_url"}) and, when it matches, returns the error the |
| // caller should see: the console's own refusal text plus where to finish |
| @@ -1011,66 +1021,19 @@ func (s *apiSession) ledgerBlob(ctx context.Context, a apiAcct, app, name string |
| return out.SHA256, nil |
| } |
| -// putJSON runs one JSON PUT and hands back the status and raw body without |
| -// interpreting a non-200 as an error itself — mirrors postJSON exactly |
| -// (apimode.go), only the HTTP method differs: the config door's write route |
| -// is PUT (`PUT /api/cli/apps/{account}/{app}/config`, handleAPIConfigPut) — |
| -// "here is the state this bundle should carry", not an append. |
| +// putJSON is sendJSON pinned to PUT: the config door's write route is PUT |
| +// (`PUT /api/cli/apps/{account}/{app}/config`, handleAPIConfigPut) — "here is |
| +// the state this bundle should carry", not an append. |
| func (s *apiSession) putJSON(ctx context.Context, path string, in any) (int, []byte, error) { |
| - raw, err := json.Marshal(in) |
| - if err != nil { |
| - return 0, nil, err |
| - } |
| - req, err := http.NewRequestWithContext(ctx, http.MethodPut, s.c.base+path, bytes.NewReader(raw)) |
| - if err != nil { |
| - return 0, nil, err |
| - } |
| - req.Header.Set("Content-Type", "application/json") |
| - if s.c.token != "" { |
| - req.Header.Set("Authorization", "Bearer "+s.c.token) |
| - } |
| - res, err := s.c.httpc.Do(req) |
| - if err != nil { |
| - return 0, nil, err |
| - } |
| - defer res.Body.Close() |
| - body, err := io.ReadAll(io.LimitReader(res.Body, 1<<20)) |
| - if err != nil { |
| - return res.StatusCode, nil, err |
| - } |
| - return res.StatusCode, body, nil |
| + return s.sendJSON(ctx, http.MethodPut, path, in) |
| } |
| -// patchJSON runs one JSON PATCH and hands back the status and raw body |
| -// without interpreting a non-200 as an error itself — mirrors postJSON and |
| -// putJSON exactly (apimode.go), only the HTTP method differs: the instance |
| -// upstream-repoint door is PATCH (`PATCH |
| -// /api/cli/apps/{account}/{app}/instances/{host}`, handleAPIInstanceUpdate) |
| -// — "here is what changed about this record", not the whole state (PUT) or a |
| -// new one (POST). |
| +// patchJSON is sendJSON pinned to PATCH: the instance upstream-repoint door |
| +// is PATCH (`PATCH /api/cli/apps/{account}/{app}/instances/{host}`, |
| +// handleAPIInstanceUpdate) — "here is what changed about this record", not |
| +// the whole state (PUT) or a new one (POST). |
| func (s *apiSession) patchJSON(ctx context.Context, path string, in any) (int, []byte, error) { |
| - raw, err := json.Marshal(in) |
| - if err != nil { |
| - return 0, nil, err |
| - } |
| - req, err := http.NewRequestWithContext(ctx, http.MethodPatch, s.c.base+path, bytes.NewReader(raw)) |
| - if err != nil { |
| - return 0, nil, err |
| - } |
| - req.Header.Set("Content-Type", "application/json") |
| - if s.c.token != "" { |
| - req.Header.Set("Authorization", "Bearer "+s.c.token) |
| - } |
| - res, err := s.c.httpc.Do(req) |
| - if err != nil { |
| - return 0, nil, err |
| - } |
| - defer res.Body.Close() |
| - body, err := io.ReadAll(io.LimitReader(res.Body, 1<<20)) |
| - if err != nil { |
| - return res.StatusCode, nil, err |
| - } |
| - return res.StatusCode, body, nil |
| + return s.sendJSON(ctx, http.MethodPatch, path, in) |
| } |
| // apiConfigVar is one row of a config bundle read back through the console — |
diff --git a/cli.go b/cli.go| index df95762dc..16e1b2220 100644 |
| --- a/cli.go |
| +++ b/cli.go |
| @@ -205,11 +205,25 @@ func runAdd(args []string) { |
| // a Route literal built above with no Addrs/Health always overwrites |
| // with "". Preserving what this box-local verb doesn't manage mirrors |
| // Upsert's own CASE-preserved columns (environment, caps, tunnel, ...): |
| - // an explicit value from a flag still wins, an absent one keeps what |
| - // was already there. Addr is the one exception among the three upstream |
| - // columns that IS a flag here, so it only carries forward when this |
| - // invocation named none of its own — an explicit -addr still replaces |
| - // the legacy single-upstream field exactly as it always has. |
| + // an absent flag keeps what was already there. |
| + // |
| + // An explicit -addr is different, and deliberately NOT folded into |
| + // that same "flag wins" story when the row already carries an |
| + // established Addrs set (Task 8 review, IMPORTANT 1): silently |
| + // clearing Addrs here to let -addr win would build a row |
| + // ValidateShape forbids in every other door ("addr and addrs both |
| + // set"), and even if it didn't, AddrList and the edge already prefer |
| + // Addrs over Addr — the -addr would be dead on arrival, and the next |
| + // converge tick would revert it anyway (convergeOwned re-derives Addr |
| + // FROM the record's Addrs), the exact "success printed over nothing" |
| + // shape issue #69 named. The record — not this box verb — owns |
| + // multi-upstream shape, so the fix is a refusal that names the door |
| + // that does: `carlos instances set-upstreams`. |
| + if *addr != "" && existing.Addrs != "" { |
| + log.Fatalf("add %s: -addr %s would coexist with the established multi-upstream set (%s) on this row — "+ |
| + "a route picks one upstream shape, and the record owns it, not this box verb. Repoint the set with "+ |
| + "`carlos instances set-upstreams -app %s -host %s -addrs ...` instead of -addr here", host, *addr, existing.Addrs, *app, host) |
| + } |
| route.Addrs, route.Health = existing.Addrs, existing.Health |
| if *addr == "" { |
| route.Addr = existing.Addr |
diff --git a/cli_test.go b/cli_test.go| index d4e2f8472..064b5c44f 100644 |
| --- a/cli_test.go |
| +++ b/cli_test.go |
| @@ -683,6 +683,14 @@ func TestRouteUnitStampsAnExistingRoute(t *testing.T) { |
| // reconcile pass) had established, the same shape of hazard a plain |
| // excluded.environment once caused. |
| // |
| +// An explicit -addr on that same re-add is its own case (Task 8 review, |
| +// IMPORTANT 1): it does NOT silently win by clearing the established Addrs |
| +// — that would build a row ValidateShape refuses everywhere else ("addr and |
| +// addrs both set") and, even set aside that, would be reverted by the next |
| +// converge tick anyway (convergeOwned re-derives Addr from the record's |
| +// Addrs). It is refused instead, pointing at `carlos instances |
| +// set-upstreams` — the record, not this box verb, owns multi-upstream shape. |
| +// |
| // SetUpstreams (internal/registry) is used directly, not through a CLI verb, |
| // to establish the set: it is the narrow setter reconcile itself converges |
| // through, and no `carlos` command exposes it standalone — set-upstreams |
| @@ -721,28 +729,29 @@ func TestAddPreservesUpstreamsOnReAdd(t *testing.T) { |
| t.Fatalf("SetUpstreams: %v", err) |
| } |
| - // A re-add naming -addr again (an operator's ordinary "the same command I |
| - // used the first time") must still carry the established set forward. |
| - if out, err := run("add", "-registry", regPath, "-app", "kass", |
| - "-addr", "127.0.0.1:9001", host); err != nil { |
| - t.Fatalf("re-add: %v\n%s", err, out) |
| + // A re-add naming -addr again, once the row carries an established |
| + // multi-upstream set, is refused rather than silently clearing it — the |
| + // record owns that shape, not this box verb. |
| + out, err := run("add", "-registry", regPath, "-app", "kass", |
| + "-addr", "127.0.0.1:9001", host) |
| + if err == nil { |
| + t.Fatalf("re-add with -addr over an established Addrs set: want non-zero exit, got 0\n%s", out) |
| + } |
| + if !strings.Contains(out, "carlos instances set-upstreams") { |
| + t.Errorf("refusal = %q, want it to name carlos instances set-upstreams", out) |
| } |
| rt, err := reg.Get(host) |
| if err != nil || rt == nil { |
| - t.Fatalf("get after re-add: %+v, %v", rt, err) |
| - } |
| - if rt.Addrs != wantAddrs { |
| - t.Errorf("Addrs after re-add = %q, want %q preserved", rt.Addrs, wantAddrs) |
| + t.Fatalf("get after the refused re-add: %+v, %v", rt, err) |
| } |
| - if rt.Health != wantHealth { |
| - t.Errorf("Health after re-add = %q, want %q preserved", rt.Health, wantHealth) |
| - } |
| - if rt.Addr != "127.0.0.1:9001" { |
| - t.Errorf("Addr after re-add = %q, want the explicit -addr to still win", rt.Addr) |
| + if rt.Addrs != wantAddrs || rt.Health != wantHealth || rt.Addr != "" { |
| + t.Fatalf("row after a refused re-add = addr %q addrs %q health %q, want it untouched (addr \"\", addrs %q, health %q)", |
| + rt.Addr, rt.Addrs, rt.Health, wantAddrs, wantHealth) |
| } |
| - // A re-add naming NO -addr at all must not blank the legacy Addr column |
| - // either — this box-local verb only carries what its own flags set. |
| + // A re-add naming NO -addr at all must not blank Addrs/Health either — |
| + // this box-local verb only carries what its own flags set, and the |
| + // refusal above is -addr-specific, not a blanket lockout of the host. |
| if out, err := run("add", "-registry", regPath, "-app", "kass", host); err != nil { |
| t.Fatalf("re-add with no -addr: %v\n%s", err, out) |
| } |
| @@ -750,12 +759,32 @@ func TestAddPreservesUpstreamsOnReAdd(t *testing.T) { |
| if err != nil || rt == nil { |
| t.Fatalf("get after flagless re-add: %+v, %v", rt, err) |
| } |
| - if rt.Addr != "127.0.0.1:9001" { |
| - t.Errorf("Addr after a flagless re-add = %q, want the established addr preserved", rt.Addr) |
| - } |
| if rt.Addrs != wantAddrs || rt.Health != wantHealth { |
| t.Errorf("Addrs/Health after a flagless re-add = %q/%q, want both still preserved", rt.Addrs, rt.Health) |
| } |
| + if rt.Addr != "" { |
| + t.Errorf("Addr after a flagless re-add = %q, want it to stay preserved as the establishing SetUpstreams left it", rt.Addr) |
| + } |
| + |
| + // The refusal is Addrs-specific: an ordinary re-add with -addr on a host |
| + // that has no established multi-upstream set is unaffected and still |
| + // works exactly as it always has. |
| + const solo = "solo.example.com" |
| + if out, err := run("add", "-registry", regPath, "-app", "kass", |
| + "-addr", "127.0.0.1:9100", solo); err != nil { |
| + t.Fatalf("add solo: %v\n%s", err, out) |
| + } |
| + if out, err := run("add", "-registry", regPath, "-app", "kass", |
| + "-addr", "127.0.0.1:9200", solo); err != nil { |
| + t.Fatalf("re-add solo with -addr (no established Addrs set): %v\n%s", err, out) |
| + } |
| + soloRt, err := reg.Get(solo) |
| + if err != nil || soloRt == nil { |
| + t.Fatalf("get solo: %+v, %v", soloRt, err) |
| + } |
| + if soloRt.Addr != "127.0.0.1:9200" { |
| + t.Errorf("solo Addr = %q, want the second -addr to have won", soloRt.Addr) |
| + } |
| } |
| // --- release versioning: the CLI surface ------------------------------- |
diff --git a/instances_cli.go b/instances_cli.go| index 92c79a9c1..1f1f909d7 100644 |
| --- a/instances_cli.go |
| +++ b/instances_cli.go |
| @@ -221,9 +221,12 @@ create-only flags: |
| -addr <a:p,...> comma-separated TCP upstream set, host:port each |
| (e.g. 10.0.0.1:8090,10.0.0.2:8090); omitted means |
| the platform's own default upstream |
| - -health-path <p> probe request path (default "/"); only meaningful |
| - alongside more than one -addr |
| - -health-status <n> probe's expected response status (default 200) |
| + -health-path <p> states the WHOLE probe alongside -health-status |
| + (default "/"); honoured for a single -addr too |
| + — that is exactly how you probe one |
| + -health-status <n> probe's expected response status (default 200); |
| + fields neither flag names take the platform's |
| + own defaults, not "unset" |
| list-only flags: |
| @@ -235,13 +238,22 @@ set-upstreams-only flags: |
| -addrs <a:p,...> comma-separated TCP upstream set, host:port each, |
| replacing the record's whole upstream set; |
| - omitted leaves the existing set alone |
| - -health-path <p> replaces the probe's request path |
| - -health-status <n> replaces the probe's expected response status |
| - (-health-path and -health-status may be given |
| - together or alone; either one, given at all, |
| - writes a probe block — omitting both leaves the |
| - existing probe alone) |
| + omitted leaves the existing set alone, an |
| + explicit -addrs "" clears it to empty (refused |
| + with a 400 for a kind:instance record — emptying |
| + an addr-backed instance route reverts it to a |
| + socket-backed shape, and the DerivesPaths rail |
| + refuses that transition in place; delete and |
| + re-create instead) |
| + -health-path <p> states the WHOLE probe block, replacing whatever |
| + was there — NOT a per-field update: a field |
| + neither flag names reverts to the platform's own |
| + default, even if a prior probe had set it |
| + -health-status <n> probe's expected response status; -health-path |
| + and -health-status may be given together or |
| + alone — either one, given at all, replaces the |
| + whole probe (omitting both leaves the existing |
| + probe untouched) |
| Operators placing routes by hand on a box use carlos add, not this. |
| ` |
| @@ -270,8 +282,8 @@ func runInstancesCreate(args []string) { |
| region := fs.String("region", "", "region code for the record (subject to the manifest's allow-list)") |
| channel := fs.String("channel", "", "release channel the instance follows (default: the manifest's, then stable)") |
| addr := fs.String("addr", "", "comma-separated TCP upstream set, host:port each (e.g. 10.0.0.1:8090,10.0.0.2:8090); omitted means the platform's own default upstream") |
| - healthPath := fs.String("health-path", "", "probe request path (default \"/\"); only meaningful alongside more than one -addr") |
| - healthStatus := fs.Int("health-status", 0, "probe's expected response status (default 200)") |
| + healthPath := fs.String("health-path", "", "states the whole probe alongside -health-status: request path (default \"/\"); honoured for a single -addr too") |
| + healthStatus := fs.Int("health-status", 0, "probe's expected response status (default 200); fields neither flag names take the platform's own defaults") |
| consoleFlag := fs.String("console", "", "console to create through (default: the only logged-in one)") |
| acctRaw := fs.String("account", "ops", "account (sqid or name) the app belongs to") |
| acctIDRaw := fs.String("account-id", "", "account sqid, matched as a sqid ONLY — supersedes -account") |
| @@ -423,9 +435,9 @@ func runInstancesSetUpstreams(args []string) { |
| fs := flag.NewFlagSet("instances set-upstreams", flag.ExitOnError) |
| app := fs.String("app", "", "app the instance belongs to") |
| host := fs.String("host", "", "instance host whose upstream set to repoint") |
| - addrsFlag := fs.String("addrs", "", "comma-separated TCP upstream set, host:port each, replacing the record's whole upstream set; omitted leaves the existing set alone") |
| - healthPath := fs.String("health-path", "", "replaces the probe's request path") |
| - healthStatus := fs.Int("health-status", 0, "replaces the probe's expected response status") |
| + addrsFlag := fs.String("addrs", "", "comma-separated TCP upstream set, host:port each, replacing the record's whole upstream set; omitted leaves the existing set alone; an explicit -addrs \"\" clears it (400 for a kind:instance record — the DerivesPaths rail refuses that shape transition in place)") |
| + healthPath := fs.String("health-path", "", "states the WHOLE probe block, replacing whatever was there — not a per-field update; a field neither flag names reverts to the platform's own default") |
| + healthStatus := fs.Int("health-status", 0, "probe's expected response status; either -health-path or -health-status, given at all, replaces the whole probe") |
| consoleFlag := fs.String("console", "", "console to update through (default: the only logged-in one)") |
| acctRaw := fs.String("account", "ops", "account (sqid or name) the app belongs to") |
| acctIDRaw := fs.String("account-id", "", "account sqid, matched as a sqid ONLY — supersedes -account") |