amadan
environments: whole-branch review fix round
Two CRITICAL, three IMPORTANT and three MINOR findings from the adversarial branch review, each with a regression test written first. CRITICAL 1 — binding an environment could replace a hand-staged production env file. An UNBOUND route is protected by Sync's "no config.json at all ⇒ leave the file alone"; a BOUND route resolves through LoadMerged, which treats a missing default as an empty base, so the documented migration step (create a small override bundle, bind the routes) replaced every var the staged file held with just the override's, as root, and restarted the unit. The same protection now covers the default half of a bound route's merge: default missing + non-empty file on disk refuses that route loudly, every pass, naming `carlos env set` as the import; default present (even with zero vars) means the operator has adopted bucket config and the merged render is authoritative; nothing staged means nothing to lose and the environment-only render proceeds. Sync reads the default once per (acct, app) and reuses it across that app's environments, so the guard costs no extra fetches. CRITICAL 2 — `env`/`secrets unset` CREATED the bundle they were asked to remove from, on both the default and named paths. An empty manifest that EXISTS means "configured, then fully unset" by this package's contract, so a routine cleanup against an unconfigured app armed Sync to empty every one of its route env files; and a typo'd `-environment` minted a phantom bundle that then satisfied `carlos route -environment`'s existence rail. applyAt now refuses an unset-only call against an object that does not exist. Sets still create, which is how a bundle is meant to come into being. IMPORTANT 3 — registry.Upsert preserves a non-empty existing environment on conflict. `carlos add` has no -environment flag, so any re-add silently unbound the route (and reconcile's re-mint did the same). Clearing keeps exactly one door: `carlos route -environment -`. The Route.Environment comment claiming a single writer was false and is corrected. IMPORTANT 4 — `-environment` is refused on Platform+Canary routes, through the same shared function as the channel refusal (renamed platformCanaryOwnedRow): the reconciler rebuilds those rows, and IMPORTANT 3's preservation makes a binding survivable without making it owned. IMPORTANT 5 — prod-wired is the environment set-membership rule OR the apex host, not a replacement. They answer different halves of §5: what glue a canary is wired to, and whether it is in front of everybody. One function. MINOR 6 the environment existence cache is keyed on (account, app), not account alone; MINOR 7 `route -environment default` gets its own refusal naming the reservation, pinned by asserting the reason; MINOR 8 the cross-account byApp exposure shape is recorded at its source (comment only, pre-existing); MINOR 9 unset reports how many keys it actually removed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
15 files changed,
+827
−158
appconfig_cli.go+13 −7cli.go+69 −29e2e_test.go+130 −11internal/appconfig/environments.go+19 −4internal/appconfig/environments_test.go+55 −0internal/appconfig/store.go+68 −38internal/appconfig/store_test.go+49 −1internal/appconfig/sync.go+74 −22internal/appconfig/sync_test.go+165 −13internal/console/ladder.go+41 −10internal/console/ladder_test.go+36 −1internal/console/view.go+12 −0internal/domains/reconcile.go+11 −9internal/registry/registry.go+30 −13internal/registry/registry_test.go+55 −0
diff --git a/appconfig_cli.go b/appconfig_cli.go| index 35cdf81e8..4ee80d1b5 100644 |
| --- a/appconfig_cli.go |
| +++ b/appconfig_cli.go |
| @@ -129,12 +129,16 @@ func saveSecrets(ctx context.Context, s release.Store, acct, app, environment st |
| // unsetVars removes vars (env or secret) by key from the default bundle |
| // when environment=="", or from the named environment's own bundle |
| // otherwise — shared by `env unset` and `secrets unset`, same as |
| -// appconfig.Unset is shared by their non-environment paths today. |
| -func unsetVars(ctx context.Context, s release.Store, acct, app, environment string, keys []string) error { |
| +// appconfig.Unset is shared by their non-environment paths today. It returns |
| +// how many keys were ACTUALLY removed, which is what the caller prints: a |
| +// count of arguments reads as success for a typo'd key or a typo'd |
| +// -environment (2026-08-02 review, MINOR 9). Unsetting from a bundle that |
| +// does not exist is an error from appconfig, not a create — see applyAt. |
| +func unsetVars(ctx context.Context, s release.Store, acct, app, environment string, keys []string) (int, error) { |
| if environment == "" { |
| return appconfig.Unset(ctx, s, acct, app, keys) |
| } |
| - return appconfig.SaveEnvironment(ctx, s, acct, app, environment, nil, keys) |
| + return appconfig.UnsetEnvironment(ctx, s, acct, app, environment, keys) |
| } |
| // parseKV turns ["KEY=value", "KEY2=value2"] into a map, refusing anything |
| @@ -187,10 +191,11 @@ func runEnvUnset(args []string) { |
| log.Fatal("usage: carlos env unset -app <a> [-environment <name>] KEY [KEY2 ...]") |
| } |
| checkEnvironmentFlag(*environment) |
| - if err := unsetVars(context.Background(), openStore(), acctOf(), *app, *environment, fs.Args()); err != nil { |
| + removed, err := unsetVars(context.Background(), openStore(), acctOf(), *app, *environment, fs.Args()) |
| + if err != nil { |
| log.Fatal(err) |
| } |
| - fmt.Printf("unset %d var(s) for %s (%s)\n", fs.NArg(), *app, bundleLabel(*environment)) |
| + fmt.Printf("unset %d of %d var(s) for %s (%s)\n", removed, fs.NArg(), *app, bundleLabel(*environment)) |
| } |
| func runEnvList(args []string) { |
| @@ -332,10 +337,11 @@ func runSecretsUnset(args []string) { |
| log.Fatal("usage: carlos secrets unset -app <a> [-environment <name>] KEY [KEY2 ...]") |
| } |
| checkEnvironmentFlag(*environment) |
| - if err := unsetVars(context.Background(), openStore(), acctOf(), *app, *environment, fs.Args()); err != nil { |
| + removed, err := unsetVars(context.Background(), openStore(), acctOf(), *app, *environment, fs.Args()) |
| + if err != nil { |
| log.Fatal(err) |
| } |
| - fmt.Printf("unset %d secret(s) for %s (%s)\n", fs.NArg(), *app, bundleLabel(*environment)) |
| + fmt.Printf("unset %d of %d secret(s) for %s (%s)\n", removed, fs.NArg(), *app, bundleLabel(*environment)) |
| } |
| func runSecretsList(args []string) { |
diff --git a/cli.go b/cli.go| index 29d56746a..3f46ea3cb 100644 |
| --- a/cli.go |
| +++ b/cli.go |
| @@ -226,14 +226,19 @@ func runInstances(args []string) { |
| // from `firstReadyUpstream`, silently reverting this verb's write and |
| // blanking version/status/dns/cert/ENVIRONMENT columns to mintedRoute's |
| // zero values (mintedRoute never sets Environment — see its doc comment). |
| -// platformCanaryOwnsChannel refuses this unconditionally, structurally, for |
| +// platformCanaryOwnedRow refuses this unconditionally, structurally, for |
| // every candidate route in pass 1 below — not only in the -host branch — |
| // so it holds even if a future change lets -app include Platform routes. |
| -// This refusal is CHANNEL-specific, though: -environment on its own is |
| -// allowed on ANY Platform route, including Platform+Canary, because |
| -// reconcile does not derive Environment the way it derives Channel for |
| -// static mints — the only thing that can still blank it is a Channel change |
| -// on the SAME route later reviving the revert above. |
| +// The SAME function guards the -environment axis (2026-08-02 review, |
| +// IMPORTANT 4): the re-mint above rebuilds the whole ROW from mintedRoute, |
| +// not just the channel it derives, so an environment binding on a |
| +// Platform+Canary route is a binding on a row this verb does not own. |
| +// registry.Upsert now preserves a non-empty Environment, which makes such a |
| +// binding survivable rather than silently reverted — but "survives the |
| +// reconciler" is not the same as "is owned by the operator", and an earlier |
| +// revision of this comment argued from "reconcile does not DERIVE |
| +// Environment", which was true and not the point. A plain Platform apex |
| +// (non-Canary) is still bindable via -host on either axis. |
| // (A static, non-Canary Platform apex has no such trap: its idempotency |
| // guard, `hasApex`, never reads Channel at all, so -host may freely repoint |
| // one — though doing so while it happens to sit on a canary/* channel does |
| @@ -276,8 +281,20 @@ func runRoute(args []string) { |
| // that's an accepted asymmetry, not a bug (matches "-" as a stdin/stdout |
| // sentinel elsewhere in Unix tooling). |
| envClear := *environment == "-" |
| + // "default" gets its own refusal rather than falling through to the |
| + // generic one (2026-08-02 review, MINOR 7): it is a well-formed name that |
| + // is reserved, not a malformed one, and the generic message sent the |
| + // operator looking for a typo. It also keeps this refusal pinned to the |
| + // RESERVATION — with the reservation gone, the existence rail below would |
| + // still refuse "default" for an unrelated reason and a test asserting |
| + // only "refused" would not notice. Same wording as checkEnvironmentFlag's |
| + // (appconfig_cli.go), which every `carlos env`/`secrets` verb uses. |
| + if *environment == "default" { |
| + log.Fatal(`route: -environment default is refused: "default" already names the base bundle ` + |
| + "(config.json) every route gets when nothing is bound — use \"-environment -\" to clear a binding back to it") |
| + } |
| if *environment != "" && !envClear && !appconfig.ValidEnvironmentName(*environment) { |
| - log.Fatalf("route: %q is not a valid environment name", *environment) |
| + log.Fatalf("route: %q is not a valid environment name — want [a-z0-9-]{1,32}", *environment) |
| } |
| // resolvedEnv is what actually lands in the registry column: "" either |
| // because -environment was never given (untouched — see the write step) |
| @@ -324,7 +341,7 @@ func runRoute(args []string) { |
| // in one command is a different, riskier operation than |
| // "move this app's ordinary routes" — Platform routes stay |
| // opt-in, one -host at a time. (Platform+Canary routes are |
| - // refused even via -host — see platformCanaryOwnsChannel, |
| + // refused even via -host — see platformCanaryOwnedRow, |
| // applied structurally below for every candidate, not just |
| // here — so this exclusion is belt-and-braces for those, but |
| // load-bearing for a plain Platform apex.) |
| @@ -382,7 +399,7 @@ func runRoute(args []string) { |
| switch { |
| case from == *channel: |
| chanVerdicts[i] = verdict{rt: rt, from: from, kind: vNoop} |
| - case platformCanaryOwnsChannel(rt): |
| + case platformCanaryOwnedRow(rt): |
| chanVerdicts[i] = verdict{rt: rt, from: from, kind: vPlatformOwned, note: fmt.Sprintf( |
| "Platform+Canary route on %s — its channel is derived and re-asserted by the domains\n"+ |
| "reconciler's canary-minting pass (internal/domains/reconcile.go), which is idempotent via a\n"+ |
| @@ -438,9 +455,10 @@ func runRoute(args []string) { |
| // anyway" reading of binding a route to config that isn't there. |
| type envVerdictKind int |
| const ( |
| - eNoop envVerdictKind = iota // already bound to the target |
| - eWillWrite // clearing, or the bundle exists — proceed |
| - eMissing // named but absent in the bucket — refuse, no override |
| + eNoop envVerdictKind = iota // already bound to the target |
| + eWillWrite // clearing, or the bundle exists — proceed |
| + eMissing // named but absent in the bucket — refuse, no override |
| + ePlatformOwned // Platform+Canary: the reconciler owns this row, never overridable |
| ) |
| type envVerdict struct { |
| rt registry.Route |
| @@ -452,24 +470,38 @@ func runRoute(args []string) { |
| if *environment != "" { |
| envVerdicts = make([]envVerdict, len(candidates)) |
| var store release.Store |
| - // existsByAcct caches one ListEnvironments call per distinct account |
| - // among the candidates (an -app -account all sweep can span several), |
| - // mirroring the channel rail's "resolve once" pattern — never opened |
| - // at all when every candidate is already bound to the target or the |
| - // operator is clearing (envClear needs no bucket check per the doc |
| - // comment above). |
| - existsByAcct := map[string]map[string]bool{} |
| + // existsByApp caches one ListEnvironments call per distinct |
| + // (account, app) among the candidates (an -app -account all sweep can |
| + // span several accounts), mirroring the channel rail's "resolve once" |
| + // pattern — never opened at all when every candidate is already bound |
| + // to the target or the operator is clearing (envClear needs no bucket |
| + // check per the doc comment above). The key is BOTH halves of the |
| + // query it caches: keyed on account alone (2026-08-02 review, MINOR |
| + // 6) it would approve a bundle that exists for a different app in the |
| + // same account. Unreachable today — -app fixes rt.App and -host has |
| + // one candidate — which is exactly why it is worth keying correctly |
| + // now rather than after a future widening. |
| + existsByApp := map[string]map[string]bool{} |
| ctx := context.Background() |
| for i, rt := range candidates { |
| from := rt.Environment |
| switch { |
| case from == resolvedEnv: |
| envVerdicts[i] = envVerdict{rt: rt, from: from, kind: eNoop} |
| + case platformCanaryOwnedRow(rt): |
| + // Refused on the same terms as the channel axis, and for the |
| + // same structural reason: the reconciler rebuilds this row. |
| + // Placed before envClear so CLEARING is refused too — a |
| + // half-owned column would be the worst of both. |
| + envVerdicts[i] = envVerdict{rt: rt, from: from, kind: ePlatformOwned, note: "Platform+Canary route — its whole row is rebuilt by the domains\n" + |
| + "reconciler's canary-minting pass (internal/domains/reconcile.go) whenever that pass's `followed`\n" + |
| + "check goes false, from values it derives itself. This verb does not own this row"} |
| case envClear: |
| envVerdicts[i] = envVerdict{rt: rt, from: from, kind: eWillWrite} |
| default: |
| acct := release.RouteAccount(rt) |
| - names, cached := existsByAcct[acct] |
| + ck := acct + "/" + rt.App |
| + names, cached := existsByApp[ck] |
| if !cached { |
| if store == nil { |
| store = openStore() |
| @@ -482,7 +514,7 @@ func runRoute(args []string) { |
| for _, n := range list { |
| names[n] = true |
| } |
| - existsByAcct[acct] = names |
| + existsByApp[ck] = names |
| } |
| if names[*environment] { |
| envVerdicts[i] = envVerdict{rt: rt, from: from, kind: eWillWrite} |
| @@ -531,6 +563,9 @@ func runRoute(args []string) { |
| envToWrite = append(envToWrite, v.rt.Host) |
| case eMissing: |
| refusals = append(refusals, fmt.Sprintf("%s: %s", v.rt.Host, v.note)) |
| + case ePlatformOwned: |
| + refusals = append(refusals, fmt.Sprintf( |
| + "%s: %s — no flag overrides a row this verb does not own", v.rt.Host, v.note)) |
| } |
| } |
| if len(refusals) > 0 { |
| @@ -601,14 +636,19 @@ func runRoute(args []string) { |
| } |
| } |
| -// platformCanaryOwnsChannel reports whether the domains reconciler, not this |
| -// verb, owns rt's Channel column: true for any Platform+Canary route. See |
| -// runRoute's doc comment for why — the canary-minting pass in |
| -// internal/domains/reconcile.go is idempotent via `followed`, a check |
| -// computed from THIS column, not from the `byHost` map the same loop also |
| -// sets but never reads for that purpose, so ANY change here (not only a move |
| -// off canary/*) is silently reverted on the next reconcile pass. |
| -func platformCanaryOwnsChannel(rt registry.Route) bool { |
| +// platformCanaryOwnedRow reports whether the domains reconciler, not this |
| +// verb, owns rt's row: true for any Platform+Canary route. See runRoute's doc |
| +// comment for why — the canary-minting pass in internal/domains/reconcile.go |
| +// is idempotent via `followed`, a check computed from THAT ROUTE'S OWN |
| +// Channel column, not from the `byHost` map the same loop also sets but never |
| +// reads for that purpose. Any change to the channel (not only a move off |
| +// canary/*) makes `followed` false, and the next pass re-Upserts the whole |
| +// row from mintedRoute's derived values. |
| +// |
| +// One function, both axes: -channel because the reconciler re-derives that |
| +// column, -environment because it rebuilds the row the column lives on |
| +// (2026-08-02 review, IMPORTANT 4). |
| +func platformCanaryOwnedRow(rt registry.Route) bool { |
| return rt.Platform && rt.Canary |
| } |
diff --git a/e2e_test.go b/e2e_test.go| index 7e5e15f72..28ca3b04b 100644 |
| --- a/e2e_test.go |
| +++ b/e2e_test.go |
| @@ -1482,12 +1482,35 @@ func TestEnvironmentBundlesCLI(t *testing.T) { |
| if !regexp.MustCompile(`(?m)^LOG_LEVEL\s+trace$`).MatchString(out) { |
| t.Fatalf("env list -environment demo = %q, want LOG_LEVEL=trace", out) |
| } |
| - run("env", "unset", "-account", "bab", "-app", "keymail", "-environment", "demo", "LOG_LEVEL") |
| + // The count is of REMOVALS, not of arguments (review MINOR 9): a key that |
| + // was never set must not read as a successful removal. |
| + out = run("env", "unset", "-account", "bab", "-app", "keymail", "-environment", "demo", "LOG_LEVEL", "NEVER_SET") |
| + if !strings.Contains(out, "unset 1 of 2") { |
| + t.Fatalf("env unset = %q, want it to report 1 of 2 keys actually removed", out) |
| + } |
| out = run("env", "list", "-account", "bab", "-app", "keymail", "-environment", "demo") |
| if strings.Contains(out, "LOG_LEVEL") { |
| t.Fatalf("env list -environment demo = %q, LOG_LEVEL survived unset", out) |
| } |
| + // Review CRITICAL 2: unsetting from a bundle that does not exist is a |
| + // refusal, not a create — for a named bundle AND for the default one. |
| + // Creating it would mint a manifest that EXISTS with zero vars, which is |
| + // the state config sync converges every one of that app's env files to |
| + // empty for, as root. |
| + msg := runFails("env", "unset", "-account", "bab", "-app", "keymail", "-environment", "neverexisted", "ANY") |
| + if !strings.Contains(msg, "nothing to unset") { |
| + t.Fatalf("unset on a nonexistent bundle = %q, want it to say there is nothing to unset", msg) |
| + } |
| + msg = runFails("secrets", "unset", "-account", "bab", "-app", "keymail-never-configured", "ANY") |
| + if !strings.Contains(msg, "nothing to unset") { |
| + t.Fatalf("secrets unset on a never-configured app = %q, want a refusal", msg) |
| + } |
| + store := release.NewDirStore(storeDir) |
| + if _, _, err := store.Get(context.Background(), "apps/bab/keymail-never-configured/config.json"); err == nil { |
| + t.Fatal("an unset against a never-configured app created its config.json") |
| + } |
| + |
| // Default untouched by a named-bundle write: set something on demo, and |
| // on a second, never-before-seen environment; the default bundle (no |
| // -environment) still shows nothing. |
| @@ -1526,7 +1549,7 @@ func TestEnvironmentBundlesCLI(t *testing.T) { |
| // Reserved-name refusal: "default" is refused, with a message that |
| // explains default already names the base bundle rather than a bare |
| // "invalid name". An invalid-charset name is refused too. |
| - msg := runFails("env", "set", "-account", "bab", "-app", "keymail", "-environment", "default", "X=1") |
| + msg = runFails("env", "set", "-account", "bab", "-app", "keymail", "-environment", "default", "X=1") |
| if !strings.Contains(msg, "default") || !strings.Contains(strings.ToLower(msg), "base bundle") { |
| t.Fatalf("env set -environment default = %q, want a refusal naming default as the base bundle", msg) |
| } |
| @@ -1547,7 +1570,6 @@ func TestEnvironmentBundlesCLI(t *testing.T) { |
| } |
| // The named bundle lands at the sibling bucket key the design specifies. |
| - store := release.NewDirStore(storeDir) |
| if _, _, err := store.Get(context.Background(), "apps/bab/keymail/environments/demo.json"); err != nil { |
| t.Fatalf("named environment manifest not at the sibling key: %v", err) |
| } |
| @@ -2475,12 +2497,28 @@ func TestRouteEnvironmentInvalidNameRefused(t *testing.T) { |
| f := newRouteTestFixture(t) |
| f.addRoute(registry.Route{Host: "demo.tito.example.com", Kind: "instance", App: "tito", Status: "ready"}) |
| - for _, bad := range []string{"default", "Has-Upper", "has_underscore"} { |
| + for _, bad := range []string{"Has-Upper", "has_underscore", strings.Repeat("a", 33)} { |
| out, err := f.run(os.Environ(), "route", "-registry", f.regPath, |
| "-host", "demo.tito.example.com", "-environment", bad) |
| if err == nil { |
| t.Fatalf("invalid environment name %q was accepted:\n%s", bad, out) |
| } |
| + if !strings.Contains(out, "not a valid environment name") { |
| + t.Errorf("refusal of %q does not name the reason:\n%s", bad, out) |
| + } |
| + } |
| + // "default" is refused for its OWN reason — it names the base bundle, it |
| + // is not merely malformed (review MINOR 7). Asserting the reason is what |
| + // keeps this pinned to the reservation: with the reservation removed, the |
| + // existence rail would still refuse "default", for an unrelated reason, |
| + // and an assertion on err alone would stay green. |
| + out, err := f.run(os.Environ(), "route", "-registry", f.regPath, |
| + "-host", "demo.tito.example.com", "-environment", "default") |
| + if err == nil { |
| + t.Fatalf("-environment default was accepted:\n%s", out) |
| + } |
| + if !strings.Contains(out, "base bundle") { |
| + t.Errorf("refusal of \"default\" does not explain that it already names the base bundle:\n%s", out) |
| } |
| } |
| @@ -2622,7 +2660,15 @@ func TestRouteCombinedAtomicityChannelRefusalBlocksEnvironmentToo(t *testing.T) |
| // domains reconciler never derives Environment for a minted route (only |
| // Channel, for static mints), so this verb owns the column unconditionally |
| // on every route kind. |
| -func TestRouteEnvironmentAllowedOnPlatformCanaryRoute(t *testing.T) { |
| +// Review IMPORTANT 4: a Platform+Canary route's whole ROW is the domains |
| +// reconciler's, not this verb's — its canary-minting pass re-Upserts the row |
| +// from mintedRoute's zero values whenever its `followed` check goes false. |
| +// Upsert now preserves a non-empty Environment (IMPORTANT 3), which makes the |
| +// binding survivable rather than silently reverted, but a column the |
| +// reconciler rebuilds is still not one this verb hands an operator. The |
| +// refusal is structural, exactly like the Channel one, and shares its |
| +// function (platformCanaryOwnedRow). |
| +func TestRouteEnvironmentRefusedOnPlatformCanaryRoute(t *testing.T) { |
| f := newRouteTestFixture(t) |
| f.ship(release.OpsAccount, "site", "v1") |
| f.promote(release.OpsAccount, "site", "canary/preview", "v1") |
| @@ -2632,15 +2678,88 @@ func TestRouteEnvironmentAllowedOnPlatformCanaryRoute(t *testing.T) { |
| out, err := f.run(f.fullEnv(), "route", "-registry", f.regPath, |
| "-host", "preview.site.bab.apps.example.com", "-environment", "demo") |
| + if err == nil { |
| + t.Fatalf("route -environment on a Platform+Canary route was accepted:\n%s", out) |
| + } |
| + if !strings.Contains(out, "reconciler") { |
| + t.Errorf("refusal does not say who owns the row:\n%s", out) |
| + } |
| + if got := f.environmentOf("preview.site.bab.apps.example.com"); got != "" { |
| + t.Fatalf("environment = %q, want it unwritten after the refusal", got) |
| + } |
| + // The refusal is on the ROW, not on one direction of travel: a binding |
| + // made before this rail existed (or by an older binary) is equally not |
| + // this verb's to CLEAR. Only a genuine no-op stays quiet, matching the |
| + // channel axis, which also refuses moving back to stable. |
| + f.addRoute(registry.Route{Host: "legacy.site.bab.apps.example.com", Kind: "static", App: "site", |
| + Channel: "canary/preview", Status: "ready", Platform: true, Canary: true, Environment: "demo"}) |
| + out, err = f.run(f.fullEnv(), "route", "-registry", f.regPath, |
| + "-host", "legacy.site.bab.apps.example.com", "-environment", "-") |
| + if err == nil { |
| + t.Fatalf("clearing an environment on a Platform+Canary route was accepted:\n%s", out) |
| + } |
| + if got := f.environmentOf("legacy.site.bab.apps.example.com"); got != "demo" { |
| + t.Fatalf("environment = %q, want the pre-existing binding left alone by the refusal", got) |
| + } |
| + // A no-op is not a refusal — nothing is being written. |
| + if out, err := f.run(f.fullEnv(), "route", "-registry", f.regPath, |
| + "-host", "preview.site.bab.apps.example.com", "-environment", "-"); err != nil { |
| + t.Fatalf("a no-op clear on an unbound Platform+Canary route was refused: %v\n%s", err, out) |
| + } |
| +} |
| + |
| +// Review IMPORTANT 3: `carlos add` on an existing host must not unbind it. |
| +// add has no -environment flag, so a plain re-add (a routine operation — it |
| +// is how a route's socket, backing or db path is corrected) used to clear the |
| +// binding, and the next config sync then rendered the default bundle over |
| +// that route's env file and restarted the unit. |
| +func TestAddOnABoundHostPreservesItsEnvironment(t *testing.T) { |
| + f := newRouteTestFixture(t) |
| + f.addRoute(registry.Route{Host: "demo.tito.example.com", Kind: "instance", App: "tito", Status: "ready"}) |
| + f.saveEnvironment(release.OpsAccount, "tito", "demo") |
| + if out, err := f.run(f.fullEnv(), "route", "-registry", f.regPath, |
| + "-host", "demo.tito.example.com", "-environment", "demo"); err != nil { |
| + t.Fatalf("route -environment demo: %v\n%s", err, out) |
| + } |
| + |
| + if out, err := f.run(f.fullEnv(), "add", "-registry", f.regPath, |
| + "-app", "tito", "demo.tito.example.com"); err != nil { |
| + t.Fatalf("re-add: %v\n%s", err, out) |
| + } |
| + if got := f.environmentOf("demo.tito.example.com"); got != "demo" { |
| + t.Fatalf("environment = %q after `carlos add` on a bound host, want demo", got) |
| + } |
| +} |
| + |
| +// Review CRITICAL 2, end-to-end: a typo'd `env unset -environment` must not |
| +// mint the bundle it names. A phantom empty bundle satisfies the route |
| +// existence rail — whose entire purpose is refusing a bundle that is not |
| +// there — and a route bound to one silently loses the environment it was on. |
| +func TestRouteEnvironmentRailNotSatisfiableByATypoedUnset(t *testing.T) { |
| + f := newRouteTestFixture(t) |
| + f.addRoute(registry.Route{Host: "demo.tito.example.com", Kind: "instance", App: "tito", Status: "ready"}) |
| + |
| + out, err := f.run(f.fullEnv(), "env", "unset", "-app", "tito", "-environment", "typoo", "NOTHING") |
| + if err == nil { |
| + t.Fatalf("env unset against a nonexistent bundle succeeded:\n%s", out) |
| + } |
| + if !strings.Contains(out, "nothing to unset") { |
| + t.Errorf("refusal does not say there was nothing to unset:\n%s", out) |
| + } |
| + out, err = f.run(f.fullEnv(), "env", "environments", "-app", "tito") |
| if err != nil { |
| - t.Fatalf("route -environment on a Platform+Canary route: %v\n%s", err, out) |
| + t.Fatalf("env environments: %v\n%s", err, out) |
| } |
| - if got := f.environmentOf("preview.site.bab.apps.example.com"); got != "demo" { |
| - t.Fatalf("environment = %q, want demo", got) |
| + if strings.Contains(out, "typoo") { |
| + t.Fatalf("the unset minted a phantom bundle:\n%s", out) |
| } |
| - // Its channel is untouched, since -channel was not passed. |
| - if got := f.channelOf("preview.site.bab.apps.example.com"); got != "canary/preview" { |
| - t.Fatalf("channel = %q, want untouched canary/preview", got) |
| + out, err = f.run(f.fullEnv(), "route", "-registry", f.regPath, |
| + "-host", "demo.tito.example.com", "-environment", "typoo") |
| + if err == nil { |
| + t.Fatalf("the existence rail accepted a bundle that was never created:\n%s", out) |
| + } |
| + if got := f.environmentOf("demo.tito.example.com"); got != "" { |
| + t.Fatalf("environment = %q, want it unwritten", got) |
| } |
| } |
diff --git a/internal/appconfig/environments.go b/internal/appconfig/environments.go| index 9c3a827c2..3fdcb21fa 100644 |
| --- a/internal/appconfig/environments.go |
| +++ b/internal/appconfig/environments.go |
| @@ -84,16 +84,31 @@ func LoadEnvironment(ctx context.Context, s release.Store, acct, app, envName st |
| // bundle (applyAt) — a named environment gets the same racing-write |
| // ErrCASConflict discipline the default bundle already has. |
| func SaveEnvironment(ctx context.Context, s release.Store, acct, app, envName string, sets map[string]Var, unsets []string) error { |
| + _, err := applyEnvironment(ctx, s, acct, app, envName, sets, unsets) |
| + return err |
| +} |
| + |
| +// UnsetEnvironment removes vars by key from app's envName bundle, returning |
| +// how many of keys were actually there. Split out from SaveEnvironment |
| +// because the count is the difference between "unset 1 var(s)" and the truth |
| +// (2026-08-02 review, MINOR 9) — and because a bundle that does not exist is |
| +// refused here rather than minted (applyAt's doc comment has the why; a |
| +// phantom bundle satisfies `carlos route -environment`'s existence rail). |
| +func UnsetEnvironment(ctx context.Context, s release.Store, acct, app, envName string, keys []string) (int, error) { |
| + return applyEnvironment(ctx, s, acct, app, envName, nil, keys) |
| +} |
| + |
| +func applyEnvironment(ctx context.Context, s release.Store, acct, app, envName string, sets map[string]Var, unsets []string) (int, error) { |
| if !release.ValidAccount(acct) { |
| - return fmt.Errorf("config: invalid account %q", acct) |
| + return 0, fmt.Errorf("config: invalid account %q", acct) |
| } |
| if !release.ValidApp(app) { |
| - return fmt.Errorf("config: invalid app %q", app) |
| + return 0, fmt.Errorf("config: invalid app %q", app) |
| } |
| if !ValidEnvironmentName(envName) { |
| - return fmt.Errorf("config: invalid environment name %q", envName) |
| + return 0, fmt.Errorf("config: invalid environment name %q", envName) |
| } |
| - return applyAt(ctx, s, EnvironmentKey(acct, app, envName), app, sets, unsets) |
| + return applyAt(ctx, s, EnvironmentKey(acct, app, envName), app, envName, sets, unsets) |
| } |
| // ListEnvironments returns app's named environment bundle names (bare |
diff --git a/internal/appconfig/environments_test.go b/internal/appconfig/environments_test.go| index 4d40663f6..24ef81079 100644 |
| --- a/internal/appconfig/environments_test.go |
| +++ b/internal/appconfig/environments_test.go |
| @@ -3,6 +3,7 @@ package appconfig |
| import ( |
| "context" |
| + "strings" |
| "testing" |
| "github.com/carlosframework/platform/internal/release" |
| @@ -276,3 +277,57 @@ func TestLoadMergedSchemaTooNewOnEitherSideRefuses(t *testing.T) { |
| t.Fatal("LoadMerged over a schema-2 environment bundle: got nil error, want one") |
| } |
| } |
| + |
| +// Review CRITICAL 2, named-bundle half: an unset against a bundle that does |
| +// not exist must not MINT it. A phantom empty bundle passes the route |
| +// existence rail (`carlos route -environment`), which exists precisely to |
| +// refuse binding a bundle that isn't there — and a route bound to it silently |
| +// loses whatever environment it was on. |
| +func TestUnsetEnvironmentOnMissingBundleErrorsAndMintsNothing(t *testing.T) { |
| + s := release.NewDirStore(t.TempDir()) |
| + ctx := context.Background() |
| + removed, err := UnsetEnvironment(ctx, s, "bab", "keymail", "typoo", []string{"NOTHING"}) |
| + if err == nil { |
| + t.Fatalf("unset against a nonexistent bundle succeeded (removed %d), want a refusal", removed) |
| + } |
| + if !strings.Contains(err.Error(), "nothing to unset") { |
| + t.Errorf("error = %v, want it to say there is nothing to unset", err) |
| + } |
| + names, err := ListEnvironments(ctx, s, "bab", "keymail") |
| + if err != nil { |
| + t.Fatalf("ListEnvironments: %v", err) |
| + } |
| + if len(names) != 0 { |
| + t.Fatalf("environments = %v, want none — the unset minted a phantom bundle", names) |
| + } |
| +} |
| + |
| +// The same command against a bundle that DOES exist works, and reports how |
| +// many keys it actually removed (review MINOR 9), not how many it was given. |
| +func TestUnsetEnvironmentReportsActualRemovals(t *testing.T) { |
| + s := release.NewDirStore(t.TempDir()) |
| + ctx := context.Background() |
| + if err := SaveEnvironment(ctx, s, "bab", "keymail", "demo", map[string]Var{ |
| + "A": {Kind: "env", Value: "1"}, |
| + "B": {Kind: "env", Value: "2"}, |
| + }, nil); err != nil { |
| + t.Fatal(err) |
| + } |
| + removed, err := UnsetEnvironment(ctx, s, "bab", "keymail", "demo", []string{"A", "NEVER_SET"}) |
| + if err != nil { |
| + t.Fatalf("UnsetEnvironment: %v", err) |
| + } |
| + if removed != 1 { |
| + t.Fatalf("removed = %d, want 1 (A removed, NEVER_SET was never there)", removed) |
| + } |
| + m, err := LoadEnvironment(ctx, s, "bab", "keymail", "demo") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, ok := m.Vars["A"]; ok { |
| + t.Fatal("A survived the unset") |
| + } |
| + if m.Vars["B"].Value != "2" { |
| + t.Fatalf("B = %+v, want it untouched", m.Vars["B"]) |
| + } |
| +} |
diff --git a/internal/appconfig/store.go b/internal/appconfig/store.go| index 3dc0be5ec..33883ec37 100644 |
| --- a/internal/appconfig/store.go |
| +++ b/internal/appconfig/store.go |
| @@ -72,59 +72,70 @@ func getIfExistsAt(ctx context.Context, s release.Store, key string) (*Manifest, |
| return &m, true, nil |
| } |
| -// getForWrite is Get plus the etag a caller needs to write back — a |
| -// not-found manifest reads as empty with etag "" (create-only Put). |
| -func getForWrite(ctx context.Context, s release.Store, acct, app string) (Manifest, string, error) { |
| - return getForWriteAt(ctx, s, Key(acct, app), app) |
| -} |
| - |
| -// getForWriteAt is getForWrite generalized to any manifest key — see |
| -// getIfExistsAt's doc comment for why one path serves both the default |
| -// bundle and a named environment. |
| -func getForWriteAt(ctx context.Context, s release.Store, key, app string) (Manifest, string, error) { |
| +// getForWriteAt is Get plus the two bits a caller needs to write back: the |
| +// etag (a not-found manifest reads as empty with etag "", a create-only Put) |
| +// and whether the object exists at all. found is NOT inferable from the etag |
| +// — a store is free to hand back an empty etag for an object that exists — |
| +// and applyAt's refusal to create a bundle on an unset-only call turns on it. |
| +// Generalized to any manifest key; see getIfExistsAt's doc comment for why |
| +// one path serves both the default bundle and a named environment. |
| +func getForWriteAt(ctx context.Context, s release.Store, key, app string) (Manifest, string, bool, error) { |
| raw, etag, err := s.Get(ctx, key) |
| switch { |
| case err == nil: |
| var m Manifest |
| if uerr := json.Unmarshal(raw, &m); uerr != nil { |
| - return Manifest{}, "", fmt.Errorf("config %s: %w", key, uerr) |
| + return Manifest{}, "", false, fmt.Errorf("config %s: %w", key, uerr) |
| } |
| // Same newer-schema refusal as getIfExists — writing back through |
| // this path would downgrade the object to schema 1 minus the fields |
| // unmarshal already dropped. |
| if m.Schema > 1 { |
| - return Manifest{}, "", fmt.Errorf("config %s: schema %d is newer than this binary understands — upgrade carlos", key, m.Schema) |
| + return Manifest{}, "", false, fmt.Errorf("config %s: schema %d is newer than this binary understands — upgrade carlos", key, m.Schema) |
| } |
| - return m, etag, nil |
| + return m, etag, true, nil |
| case errors.Is(err, os.ErrNotExist): |
| - return Manifest{Schema: 1, App: app, Vars: map[string]Var{}}, "", nil |
| + return Manifest{Schema: 1, App: app, Vars: map[string]Var{}}, "", false, nil |
| default: |
| - return Manifest{}, "", err |
| + return Manifest{}, "", false, err |
| } |
| } |
| -// apply reads, applies sets and unsets, and writes back via CAS. A racing |
| -// concurrent write surfaces as release.ErrCASConflict — callers report it |
| -// and let the operator re-run; there is no automatic retry, matching |
| -// release.Promote's own behavior. |
| -func apply(ctx context.Context, s release.Store, acct, app string, sets map[string]Var, unsets []string) error { |
| +// apply reads, applies sets and unsets, and writes back via CAS, reporting |
| +// how many of unsets were actually present. A racing concurrent write |
| +// surfaces as release.ErrCASConflict — callers report it and let the operator |
| +// re-run; there is no automatic retry, matching release.Promote's own |
| +// behavior. |
| +func apply(ctx context.Context, s release.Store, acct, app, bundle string, sets map[string]Var, unsets []string) (int, error) { |
| if !release.ValidAccount(acct) { |
| - return fmt.Errorf("config: invalid account %q", acct) |
| + return 0, fmt.Errorf("config: invalid account %q", acct) |
| } |
| if !release.ValidApp(app) { |
| - return fmt.Errorf("config: invalid app %q", app) |
| + return 0, fmt.Errorf("config: invalid app %q", app) |
| } |
| - return applyAt(ctx, s, Key(acct, app), app, sets, unsets) |
| + return applyAt(ctx, s, Key(acct, app), app, bundle, sets, unsets) |
| } |
| // applyAt is apply generalized to any manifest key — see getIfExistsAt's doc |
| // comment for why one path serves both the default bundle and a named |
| // environment. Callers validate acct/app/environment-name themselves before |
| -// reaching here, same division as apply does for Key(acct, app). |
| -func applyAt(ctx context.Context, s release.Store, key, app string, sets map[string]Var, unsets []string) error { |
| +// reaching here, same division as apply does for Key(acct, app). bundle is |
| +// how to name this object to a human ("default", or the environment name); |
| +// it never affects which key is written. |
| +// |
| +// An unset-only call against an object that does not exist is REFUSED, not |
| +// silently turned into a create (2026-08-02 review, CRITICAL 2). Creating it |
| +// would mint an empty manifest, and an empty manifest that EXISTS means |
| +// "configured, then fully unset" by this package's own contract — which is |
| +// exactly the state Sync converges every one of that app's route env files |
| +// to empty for, as root, restarting each unit. It also minted phantom |
| +// environment bundles that then satisfied `carlos route -environment`'s |
| +// existence rail. Sets still create, which is how a bundle is meant to come |
| +// into being. |
| +func applyAt(ctx context.Context, s release.Store, key, app, bundle string, sets map[string]Var, unsets []string) (int, error) { |
| for k, v := range sets { |
| if !ValidVarName(k) { |
| - return fmt.Errorf("config: invalid var name %q", k) |
| + return 0, fmt.Errorf("config: invalid var name %q", k) |
| } |
| // Env values are validated here, while the operator can still see |
| // the plaintext — a bad value that only failed at render time would |
| @@ -133,18 +144,25 @@ func applyAt(ctx context.Context, s release.Store, key, app string, sets map[str |
| // CLI, before sealing; by this point they're opaque envelopes. |
| if v.Kind == "env" { |
| if err := ValidVarValue(v.Value); err != nil { |
| - return fmt.Errorf("config: var %s: %w", k, err) |
| + return 0, fmt.Errorf("config: var %s: %w", k, err) |
| } |
| } |
| } |
| - m, etag, err := getForWriteAt(ctx, s, key, app) |
| + m, etag, found, err := getForWriteAt(ctx, s, key, app) |
| if err != nil { |
| - return err |
| + return 0, err |
| + } |
| + if !found && len(sets) == 0 { |
| + return 0, fmt.Errorf("config: %s has no %s bundle — nothing to unset", app, bundle) |
| } |
| if m.Vars == nil { |
| m.Vars = map[string]Var{} |
| } |
| + removed := 0 |
| for _, k := range unsets { |
| + if _, ok := m.Vars[k]; ok { |
| + removed++ |
| + } |
| delete(m.Vars, k) |
| } |
| for k, v := range sets { |
| @@ -153,19 +171,28 @@ func applyAt(ctx context.Context, s release.Store, key, app string, sets map[str |
| m.Schema, m.App = 1, app |
| raw, err := json.MarshalIndent(m, "", " ") |
| if err != nil { |
| - return err |
| + return 0, err |
| } |
| - _, err = s.Put(ctx, key, raw, etag) |
| - return err |
| + if _, err := s.Put(ctx, key, raw, etag); err != nil { |
| + return 0, err |
| + } |
| + return removed, nil |
| } |
| +// DefaultBundle is how the default bundle (config.json) is named to a human |
| +// in messages — the same vocabulary ValidEnvironmentName reserves and the CLI |
| +// prints, kept in one place so an error from this package and a `list` header |
| +// can never drift apart. |
| +const DefaultBundle = "default" |
| + |
| // SetEnv writes one or more plaintext config vars. |
| func SetEnv(ctx context.Context, s release.Store, acct, app string, kv map[string]string) error { |
| sets := make(map[string]Var, len(kv)) |
| for k, v := range kv { |
| sets[k] = Var{Kind: "env", Value: v} |
| } |
| - return apply(ctx, s, acct, app, sets, nil) |
| + _, err := apply(ctx, s, acct, app, DefaultBundle, sets, nil) |
| + return err |
| } |
| // SetSecret writes one or more secrets. sealed values must already be |
| @@ -176,11 +203,14 @@ func SetSecret(ctx context.Context, s release.Store, acct, app string, sealed ma |
| for k, v := range sealed { |
| sets[k] = Var{Kind: "secret", Envelope: v} |
| } |
| - return apply(ctx, s, acct, app, sets, nil) |
| + _, err := apply(ctx, s, acct, app, DefaultBundle, sets, nil) |
| + return err |
| } |
| -// Unset removes vars (env or secret) by key. Unsetting a key that isn't set |
| -// is a no-op success, matching heroku config:unset's idempotence. |
| -func Unset(ctx context.Context, s release.Store, acct, app string, keys []string) error { |
| - return apply(ctx, s, acct, app, nil, keys) |
| +// Unset removes vars (env or secret) by key from the default bundle, |
| +// returning how many of keys were actually there. Unsetting a key that isn't |
| +// set is a no-op success, matching heroku config:unset's idempotence — |
| +// unsetting from a bundle that does not EXIST is not (see applyAt). |
| +func Unset(ctx context.Context, s release.Store, acct, app string, keys []string) (int, error) { |
| + return apply(ctx, s, acct, app, DefaultBundle, nil, keys) |
| } |
diff --git a/internal/appconfig/store_test.go b/internal/appconfig/store_test.go| index 9a8a2aa79..186af2492 100644 |
| --- a/internal/appconfig/store_test.go |
| +++ b/internal/appconfig/store_test.go |
| @@ -4,6 +4,7 @@ package appconfig |
| import ( |
| "context" |
| "errors" |
| + "strings" |
| "testing" |
| "github.com/carlosframework/platform/internal/release" |
| @@ -41,9 +42,13 @@ func TestUnsetRemovesVar(t *testing.T) { |
| if err := SetEnv(ctx, s, "bab", "keymail", map[string]string{"LOG_LEVEL": "debug"}); err != nil { |
| t.Fatalf("SetEnv: %v", err) |
| } |
| - if err := Unset(ctx, s, "bab", "keymail", []string{"LOG_LEVEL"}); err != nil { |
| + removed, err := Unset(ctx, s, "bab", "keymail", []string{"LOG_LEVEL"}) |
| + if err != nil { |
| t.Fatalf("Unset: %v", err) |
| } |
| + if removed != 1 { |
| + t.Fatalf("Unset removed %d, want 1", removed) |
| + } |
| m, err := Get(ctx, s, "bab", "keymail") |
| if err != nil { |
| t.Fatalf("Get: %v", err) |
| @@ -53,6 +58,49 @@ func TestUnsetRemovesVar(t *testing.T) { |
| } |
| } |
| +// Review CRITICAL 2: an unset against an app that has never been configured |
| +// must NOT create the manifest it was asked to remove from. A created-empty |
| +// config.json reads as "configured, then fully unset" — the state Sync |
| +// converges every one of that app's env files to EMPTY for, as root. |
| +func TestUnsetOnNeverConfiguredAppErrorsAndWritesNothing(t *testing.T) { |
| + s := release.NewDirStore(t.TempDir()) |
| + ctx := context.Background() |
| + removed, err := Unset(ctx, s, "bab", "keymail", []string{"OLDKEY"}) |
| + if err == nil { |
| + t.Fatalf("Unset on a never-configured app succeeded (removed %d), want a refusal", removed) |
| + } |
| + if !strings.Contains(err.Error(), "nothing to unset") { |
| + t.Errorf("error = %v, want it to say there is nothing to unset", err) |
| + } |
| + if _, found, gerr := getIfExists(ctx, s, "bab", "keymail"); gerr != nil || found { |
| + t.Fatalf("unset created a manifest object (found = %v, err = %v)", found, gerr) |
| + } |
| +} |
| + |
| +// Unsetting a key that is not set, on a bundle that DOES exist, stays a |
| +// success (heroku config:unset's idempotence) — it just removes nothing. |
| +func TestUnsetOfAnAbsentKeyOnAnExistingBundleIsANoopSuccess(t *testing.T) { |
| + s := release.NewDirStore(t.TempDir()) |
| + ctx := context.Background() |
| + if err := SetEnv(ctx, s, "bab", "keymail", map[string]string{"KEEP": "1"}); err != nil { |
| + t.Fatalf("SetEnv: %v", err) |
| + } |
| + removed, err := Unset(ctx, s, "bab", "keymail", []string{"NEVER_SET"}) |
| + if err != nil { |
| + t.Fatalf("Unset of an absent key: %v", err) |
| + } |
| + if removed != 0 { |
| + t.Fatalf("Unset reported %d removals, want 0 — the count is of removals, not arguments", removed) |
| + } |
| + m, err := Get(ctx, s, "bab", "keymail") |
| + if err != nil { |
| + t.Fatalf("Get: %v", err) |
| + } |
| + if m.Vars["KEEP"].Value != "1" { |
| + t.Fatalf("KEEP = %+v, want it untouched", m.Vars["KEEP"]) |
| + } |
| +} |
| + |
| func TestSetSecretStoresEnvelopeOnly(t *testing.T) { |
| s := release.NewDirStore(t.TempDir()) |
| ctx := context.Background() |
diff --git a/internal/appconfig/sync.go b/internal/appconfig/sync.go| index e019c75c3..ea2f6eadb 100644 |
| --- a/internal/appconfig/sync.go |
| +++ b/internal/appconfig/sync.go |
| @@ -54,8 +54,47 @@ func Sync(ctx context.Context, reg *registry.Registry, s release.Store, priv *ec |
| type resolution struct { |
| m *Manifest |
| err error |
| + // defaultMissing marks a bound route's merged view as the named |
| + // bundle ALONE, because the app has no config.json at all. Such a |
| + // render is not an overlay, it is a wholesale replacement, so |
| + // syncHost refuses to let it overwrite a non-empty file (see its doc |
| + // comment; 2026-08-02 review, CRITICAL 1). |
| + defaultMissing bool |
| } |
| resolved := map[groupKey]resolution{} |
| + // The default bundle is read once per (acct, app) and reused across that |
| + // app's environments: every bound route needs it twice over — LoadMerged |
| + // merges it, and the CRITICAL 1 guard turns on whether it exists at all. |
| + type appKey struct{ acct, app string } |
| + type defaultRes struct { |
| + m *Manifest |
| + found bool |
| + err error |
| + } |
| + defaults := map[appKey]defaultRes{} |
| + // getIfExists, not Get: an app with no manifest object in the bucket at |
| + // all must be distinguishable from one whose every var was unset. Get |
| + // folds those together into an empty manifest; the first has to leave a |
| + // host's hand-staged env file untouched and the second has to converge it |
| + // to empty. LoadMerged makes the same fold, which is why this read exists |
| + // alongside it rather than being replaced by it. |
| + loadDefault := func(acct, app string) defaultRes { |
| + ak := appKey{acct, app} |
| + if d, done := defaults[ak]; done { |
| + return d |
| + } |
| + m, found, err := getIfExists(ctx, s, acct, app) |
| + if err != nil { |
| + // Reported per route below rather than here (where PR #9 printed |
| + // one line per app): the operator's question on a fetch failure |
| + // is which routes are now running on a stale file, and the answer |
| + // is per route either way. |
| + err = fmt.Errorf("%s/%s: config fetch failed: %w", acct, app, err) |
| + } |
| + d := defaultRes{m: m, found: found, err: err} |
| + defaults[ak] = d |
| + return d |
| + } |
| for _, rt := range routes { |
| if !syncable(rt) { |
| continue |
| @@ -64,29 +103,17 @@ func Sync(ctx context.Context, reg *registry.Registry, s release.Store, priv *ec |
| if _, done := resolved[k]; done { |
| continue |
| } |
| + def := loadDefault(k.acct, k.app) |
| + if def.err != nil { |
| + resolved[k] = resolution{err: def.err} |
| + continue |
| + } |
| if k.env == "" { |
| - // getIfExists, not Get: an app with no manifest object in the |
| - // bucket at all is left entirely alone (nil manifest => skipped |
| - // below), so turning this feature on does not truncate env files |
| - // that are still hand-staged on the host. Get folds not-found |
| - // into an empty manifest, which would be indistinguishable from a |
| - // real, fully unset one — and those must converge. LoadMerged |
| - // makes the same fold for envName == "", which is why the |
| - // unbound case still goes through getIfExists here. |
| - m, found, err := getIfExists(ctx, s, k.acct, k.app) |
| - if err != nil { |
| - // Reported per route below rather than here (where PR #9 |
| - // printed one line per app): the operator's question on a |
| - // fetch failure is which routes are now running on a stale |
| - // file, and the answer is per route either way. |
| - resolved[k] = resolution{err: fmt.Errorf("%s/%s: config fetch failed: %w", k.acct, k.app, err)} |
| - continue |
| - } |
| - if !found { |
| + if !def.found { |
| resolved[k] = resolution{} |
| continue |
| } |
| - resolved[k] = resolution{m: m} |
| + resolved[k] = resolution{m: def.m} |
| continue |
| } |
| // A bound environment asserts the bundle exists, so LoadMerged treats |
| @@ -103,7 +130,7 @@ func Sync(ctx context.Context, reg *registry.Registry, s release.Store, priv *ec |
| resolved[k] = resolution{err: err} |
| continue |
| } |
| - resolved[k] = resolution{m: m} |
| + resolved[k] = resolution{m: m, defaultMissing: !def.found} |
| } |
| for _, rt := range routes { |
| if !syncable(rt) { |
| @@ -135,7 +162,7 @@ func Sync(ctx context.Context, reg *registry.Registry, s release.Store, priv *ec |
| fmt.Printf(" %s: has bucket config but is exec-backed — config sync only reaches carlos-app@ unit routes, skipped\n", rt.Host) |
| continue |
| } |
| - syncHost(rt, m, priv, envDir, restart) |
| + syncHost(rt, m, r.defaultMissing, priv, envDir, restart) |
| } |
| // An empty route list is never authority to delete anything: |
| // registry.Open silently creates an empty database when handed a wrong |
| @@ -217,7 +244,10 @@ func syncable(rt registry.Route) bool { |
| // was unset" and converges the file to empty. The "never configured at all" |
| // and "resolution failed" cases are both filtered out by Sync, before this: |
| // either one reaching here would truncate a live env file. |
| -func syncHost(rt registry.Route, m *Manifest, priv *ecdh.PrivateKey, envDir string, restart func(host string) error) { |
| +// |
| +// defaultMissing says the merged view is a bound environment's bundle ALONE |
| +// because the app has no config.json — see the guard below. |
| +func syncHost(rt registry.Route, m *Manifest, defaultMissing bool, priv *ecdh.PrivateKey, envDir string, restart func(host string) error) { |
| // The host is joined into a root-written path; a registry row carrying |
| // a separator must never place a file outside envDir. Same trust level |
| // as release's live/<host> symlinks, but this one is one line to pin. |
| @@ -232,6 +262,28 @@ func syncHost(rt registry.Route, m *Manifest, priv *ecdh.PrivateKey, envDir stri |
| fmt.Printf(" %s: read %s: %v\n", rt.Host, path, err) |
| return |
| } |
| + // The same protection an UNBOUND route gets from Sync's "no manifest |
| + // object at all ⇒ leave the file alone" skip, applied to the DEFAULT half |
| + // of a bound route's merge (2026-08-02 review, CRITICAL 1). Binding an |
| + // environment to an app that has never had a config.json makes the merged |
| + // render the override bundle ALONE — so the documented migration step |
| + // (create a small override, bind the routes) would replace every var a |
| + // hand-staged file holds with just the override's, as root, and restart |
| + // the unit. Three states, deliberately: |
| + // |
| + // - default bundle EXISTS (even with zero vars): the operator has |
| + // adopted bucket config for this app, the merged render is |
| + // authoritative, nothing below is skipped. |
| + // - default MISSING, non-empty file on disk: refuse this route, loudly, |
| + // every pass. The file is not ours to replace yet. |
| + // - default MISSING, nothing (or an empty file) on disk: nothing to |
| + // lose — a fresh route renders the environment alone. |
| + if defaultMissing && existed && len(cur) > 0 { |
| + fmt.Printf(" %s: environment %q is bound but %s has no default config bundle, so the merged view is that environment ALONE — "+ |
| + "refusing to replace the existing %s with it; import the staged config first (`carlos env set -app %s KEY=value ...`), "+ |
| + "then this route converges on the next pass\n", rt.Host, rt.Environment, rt.App, path, rt.App) |
| + return |
| + } |
| rendered, err := RenderEnv(m, priv) |
| if err != nil { |
| fmt.Printf(" %s: render config: %v\n", rt.Host, err) |
diff --git a/internal/appconfig/sync_test.go b/internal/appconfig/sync_test.go| index d2849cb88..9e42ca295 100644 |
| --- a/internal/appconfig/sync_test.go |
| +++ b/internal/appconfig/sync_test.go |
| @@ -196,7 +196,7 @@ func TestSyncConvergesToEmptyAfterEveryVarIsUnset(t *testing.T) { |
| t.Fatalf("after set: env = %q, err = %v; want LOG_LEVEL=debug", got, err) |
| } |
| - if err := Unset(ctx, s, "bab", "keymail", []string{"LOG_LEVEL"}); err != nil { |
| + if _, err := Unset(ctx, s, "bab", "keymail", []string{"LOG_LEVEL"}); err != nil { |
| t.Fatalf("Unset: %v", err) |
| } |
| restarted = nil |
| @@ -396,7 +396,7 @@ func TestSyncHostRefusesNonBareHostname(t *testing.T) { |
| } |
| m := &Manifest{Schema: 1, App: "evil", Vars: map[string]Var{"A": {Kind: "env", Value: "1"}}} |
| for _, host := range []string{"../escape", "a/b", "", ".", ".."} { |
| - syncHost(registry.Route{Host: host}, m, nil, envDir, func(string) error { return nil }) |
| + syncHost(registry.Route{Host: host}, m, false, nil, envDir, func(string) error { return nil }) |
| } |
| if _, err := os.Stat(filepath.Join(envDir, "..", "escape.env")); !os.IsNotExist(err) { |
| t.Fatalf("a path-escaping host wrote outside envDir, stat err = %v", err) |
| @@ -657,10 +657,14 @@ func TestSyncDoesNotReapEnvFileWhenEnvironmentIsDeleted(t *testing.T) { |
| } |
| } |
| -func TestSyncEnvironmentOnlyAppWithNoDefaultBundle(t *testing.T) { |
| - // A named environment does not require a default bundle to exist: |
| - // LoadMerged resolves to the environment alone, and Sync must converge it |
| - // (the object exists, so this is a configured app, not an unconfigured one). |
| +// Review CRITICAL 1: binding an environment must not be a way to replace a |
| +// hand-staged production env file. The unbound path is protected by |
| +// getIfExists ("no config.json at all ⇒ leave the file alone"); a bound route |
| +// resolves through LoadMerged, which treats a missing default as an empty |
| +// base — so without this guard, the documented migration step (create a small |
| +// override bundle, bind the routes) replaces every var the staged file held |
| +// with just the override's. |
| +func TestSyncRefusesToReplaceStagedFileWhenAppHasNoDefaultBundle(t *testing.T) { |
| s := release.NewDirStore(t.TempDir()) |
| ctx := context.Background() |
| reg := testRegistry(t) |
| @@ -673,26 +677,174 @@ func TestSyncEnvironmentOnlyAppWithNoDefaultBundle(t *testing.T) { |
| if err != nil { |
| t.Fatalf("ParsePrivateKey: %v", err) |
| } |
| + // An override bundle only — the app has never had a config.json. |
| if err := SaveEnvironment(ctx, s, "bab", "keymail", "demo", map[string]Var{ |
| - "ONLY": {Kind: "env", Value: "here"}, |
| + "LOG_LEVEL": {Kind: "env", Value: "debug"}, |
| + }, nil); err != nil { |
| + t.Fatalf("SaveEnvironment: %v", err) |
| + } |
| + for host, env := range map[string]string{ |
| + "prod.example.com": "demo", |
| + "fresh.example.com": "demo", |
| + } { |
| + if err := reg.Upsert(registry.Route{ |
| + Host: host, Kind: "instance", App: "keymail", Account: "bab", Status: "ready", |
| + Backing: registry.BackingUnit, Environment: env, |
| + }); err != nil { |
| + t.Fatalf("Upsert %s: %v", host, err) |
| + } |
| + } |
| + staged := "DB_URL=postgres://prod\nSTRIPE_KEY=sk_live\nSMTP_HOST=mail\nLOG_LEVEL=info\n" |
| + path := filepath.Join(envDir, "prod.example.com.env") |
| + if err := os.WriteFile(path, []byte(staged), 0o600); err != nil { |
| + t.Fatalf("stage env file: %v", err) |
| + } |
| + fi, err := os.Stat(path) |
| + if err != nil { |
| + t.Fatalf("stat staged file: %v", err) |
| + } |
| + |
| + var restarted []string |
| + if err := Sync(ctx, reg, s, privKey, envDir, func(h string) error { |
| + restarted = append(restarted, h) |
| + return nil |
| + }); err != nil { |
| + t.Fatalf("Sync: %v", err) |
| + } |
| + |
| + got, err := os.ReadFile(path) |
| + if err != nil { |
| + t.Fatalf("staged env file gone after Sync: %v", err) |
| + } |
| + if string(got) != staged { |
| + t.Fatalf("staged env file = %q, want it untouched (%q)", got, staged) |
| + } |
| + if now, err := os.Stat(path); err != nil || !now.ModTime().Equal(fi.ModTime()) { |
| + t.Fatalf("staged env file was rewritten (stat err = %v)", err) |
| + } |
| + for _, h := range restarted { |
| + if h == "prod.example.com" { |
| + t.Fatalf("restarted a route whose staged file was left in place: %v", restarted) |
| + } |
| + } |
| + // The route with nothing staged has nothing to lose: environment-only |
| + // render proceeds for it, in the same pass. |
| + fresh, err := os.ReadFile(filepath.Join(envDir, "fresh.example.com.env")) |
| + if err != nil { |
| + t.Fatalf("fresh route not materialized: %v", err) |
| + } |
| + if string(fresh) != "LOG_LEVEL=debug\n" { |
| + t.Fatalf("fresh route env = %q, want LOG_LEVEL=debug", fresh) |
| + } |
| + if len(restarted) != 1 || restarted[0] != "fresh.example.com" { |
| + t.Fatalf("restarted = %v, want exactly [fresh.example.com]", restarted) |
| + } |
| +} |
| + |
| +func TestSyncMergedRenderIsAuthoritativeOnceADefaultBundleExists(t *testing.T) { |
| + // The other side of the guard above: once the app HAS a config.json — |
| + // even one with every var unset — the operator has adopted bucket config |
| + // for it, and the merged render is authoritative over whatever is on |
| + // disk. Anything else would freeze a migrated app's file forever. |
| + s := release.NewDirStore(t.TempDir()) |
| + ctx := context.Background() |
| + reg := testRegistry(t) |
| + envDir := t.TempDir() |
| + priv, _, err := GenerateKey() |
| + if err != nil { |
| + t.Fatalf("GenerateKey: %v", err) |
| + } |
| + privKey, err := ParsePrivateKey(priv, "test") |
| + if err != nil { |
| + t.Fatalf("ParsePrivateKey: %v", err) |
| + } |
| + if err := SetEnv(ctx, s, "bab", "keymail", map[string]string{"SEED": "1"}); err != nil { |
| + t.Fatalf("SetEnv: %v", err) |
| + } |
| + if _, err := Unset(ctx, s, "bab", "keymail", []string{"SEED"}); err != nil { |
| + t.Fatalf("Unset: %v", err) // object now exists with zero vars |
| + } |
| + if err := SaveEnvironment(ctx, s, "bab", "keymail", "demo", map[string]Var{ |
| + "LOG_LEVEL": {Kind: "env", Value: "debug"}, |
| }, nil); err != nil { |
| t.Fatalf("SaveEnvironment: %v", err) |
| } |
| if err := reg.Upsert(registry.Route{ |
| - Host: "demo.example.com", Kind: "instance", App: "keymail", Account: "bab", Status: "ready", |
| + Host: "prod.example.com", Kind: "instance", App: "keymail", Account: "bab", Status: "ready", |
| Backing: registry.BackingUnit, Environment: "demo", |
| }); err != nil { |
| t.Fatalf("Upsert: %v", err) |
| } |
| - if err := Sync(ctx, reg, s, privKey, envDir, func(string) error { return nil }); err != nil { |
| + path := filepath.Join(envDir, "prod.example.com.env") |
| + if err := os.WriteFile(path, []byte("DB_URL=postgres://prod\n"), 0o600); err != nil { |
| + t.Fatalf("stage env file: %v", err) |
| + } |
| + |
| + var restarted []string |
| + if err := Sync(ctx, reg, s, privKey, envDir, func(h string) error { |
| + restarted = append(restarted, h) |
| + return nil |
| + }); err != nil { |
| t.Fatalf("Sync: %v", err) |
| } |
| - got, err := os.ReadFile(filepath.Join(envDir, "demo.example.com.env")) |
| + |
| + got, err := os.ReadFile(path) |
| if err != nil { |
| - t.Fatalf("read rendered env: %v", err) |
| + t.Fatalf("read env: %v", err) |
| } |
| - if string(got) != "ONLY=here\n" { |
| - t.Fatalf("rendered env = %q, want ONLY=here", got) |
| + if string(got) != "LOG_LEVEL=debug\n" { |
| + t.Fatalf("env = %q, want the merged render to win once a default bundle exists", got) |
| + } |
| + if len(restarted) != 1 || restarted[0] != "prod.example.com" { |
| + t.Fatalf("restarted = %v, want exactly [prod.example.com]", restarted) |
| + } |
| +} |
| + |
| +func TestSyncEnvironmentOnlyAppWithNoDefaultBundle(t *testing.T) { |
| + // A named environment does not require a default bundle to exist: |
| + // LoadMerged resolves to the environment alone, and Sync must converge it |
| + // (the object exists, so this is a configured app, not an unconfigured one). |
| + s := release.NewDirStore(t.TempDir()) |
| + ctx := context.Background() |
| + reg := testRegistry(t) |
| + envDir := t.TempDir() |
| + priv, _, err := GenerateKey() |
| + if err != nil { |
| + t.Fatalf("GenerateKey: %v", err) |
| + } |
| + privKey, err := ParsePrivateKey(priv, "test") |
| + if err != nil { |
| + t.Fatalf("ParsePrivateKey: %v", err) |
| + } |
| + if err := SaveEnvironment(ctx, s, "bab", "keymail", "demo", map[string]Var{ |
| + "ONLY": {Kind: "env", Value: "here"}, |
| + }, nil); err != nil { |
| + t.Fatalf("SaveEnvironment: %v", err) |
| + } |
| + for _, host := range []string{"demo.example.com", "emptyfile.example.com"} { |
| + if err := reg.Upsert(registry.Route{ |
| + Host: host, Kind: "instance", App: "keymail", Account: "bab", Status: "ready", |
| + Backing: registry.BackingUnit, Environment: "demo", |
| + }); err != nil { |
| + t.Fatalf("Upsert %s: %v", host, err) |
| + } |
| + } |
| + // An EMPTY file on disk is not staged config — there is nothing in it to |
| + // lose, so the no-default guard (CRITICAL 1) must not freeze it. |
| + if err := os.WriteFile(filepath.Join(envDir, "emptyfile.example.com.env"), nil, 0o600); err != nil { |
| + t.Fatalf("stage empty env file: %v", err) |
| + } |
| + if err := Sync(ctx, reg, s, privKey, envDir, func(string) error { return nil }); err != nil { |
| + t.Fatalf("Sync: %v", err) |
| + } |
| + for _, host := range []string{"demo.example.com", "emptyfile.example.com"} { |
| + got, err := os.ReadFile(filepath.Join(envDir, host+".env")) |
| + if err != nil { |
| + t.Fatalf("%s: read rendered env: %v", host, err) |
| + } |
| + if string(got) != "ONLY=here\n" { |
| + t.Fatalf("%s: rendered env = %q, want ONLY=here", host, got) |
| + } |
| } |
| } |
diff --git a/internal/console/ladder.go b/internal/console/ladder.go| index ee7a05749..2d7e290a9 100644 |
| --- a/internal/console/ladder.go |
| +++ b/internal/console/ladder.go |
| @@ -16,11 +16,13 @@ package console |
| // |
| // Every route also shows the environment it is wired to (spec §4a) beside |
| // the channel it is listed under — "default" for a route with no binding — |
| -// and prodWiredCanaryHosts below answers "production" honestly from that: |
| -// no more inferring it from which host looks like the apex. |
| +// and prodWiredCanaryHosts below answers "production" from that AND from |
| +// which host is the app's apex: the environment says what glue a canary is |
| +// wired to, the apex says whether it is in front of everybody. |
| import ( |
| "sort" |
| + "strings" |
| "time" |
| "github.com/carlosframework/platform/internal/release" |
| @@ -68,7 +70,8 @@ type LadderRoute struct { |
| // Canary is whether this host follows a canary channel at all. |
| Canary bool |
| // ProdWired is the conspicuous case: a canary bound to the same |
| - // environment as the app's stable route. See prodWiredCanaryHosts. |
| + // environment as one of the app's stable routes, OR sitting on the app's |
| + // apex host. See prodWiredCanaryHosts for why it is both. |
| ProdWired bool |
| } |
| @@ -192,11 +195,23 @@ func stepUp(from, to string, chans []LadderChannel) *LadderStep { |
| // An app can have more than one stable-channel route (e.g. two hosts both |
| // following `stable`); this treats stable's set of bound environments as |
| // "production", so a canary matching ANY of them is flagged. An app with no |
| -// stable route at all has no production environment to be wired in front |
| -// of, so nothing is flagged — that is the correct answer, not a gap. |
| +// stable route at all has no production environment to be wired in front of, |
| +// so that signal flags nothing — the correct answer, not a gap. |
| +// |
| +// The apex-host heuristic this replaced is KEPT as a second signal, OR'd in |
| +// (2026-08-02 review, IMPORTANT 5). The two answer different halves of §5's |
| +// worry and neither is a superset of the other: the environment set says |
| +// "wired to production GLUE", the apex host says "in front of production |
| +// TRAFFIC". A canary on the app's apex bound to `sandbox` serves everybody |
| +// and binds nothing production — the environment rule alone would say |
| +// nothing about it, which is a warning the console used to give and would |
| +// have silently stopped giving. Production is the app's PRIMARY host: fewest |
| +// labels, then shortest, then alphabetical. |
| // |
| // It flags, it never restricts: callers only ever ask "is this host a |
| -// prod-wired canary". |
| +// prod-wired canary", and both signals are allowed to be wrong in the |
| +// flagging direction. More flags, never fewer, is the conservative side of |
| +// this particular trade. |
| func prodWiredCanaryHosts(routes []RouteView) map[string]bool { |
| stableEnvs := map[string]bool{} |
| for _, rt := range routes { |
| @@ -204,16 +219,32 @@ func prodWiredCanaryHosts(routes []RouteView) map[string]bool { |
| stableEnvs[rt.Environment] = true |
| } |
| } |
| - out := map[string]bool{} |
| - if len(stableEnvs) == 0 { |
| - return out |
| + apex := "" |
| + better := func(a, b string) bool { |
| + la, lb := strings.Count(a, "."), strings.Count(b, ".") |
| + if la != lb { |
| + return la < lb |
| + } |
| + if len(a) != len(b) { |
| + return len(a) < len(b) |
| + } |
| + return a < b |
| + } |
| + for _, rt := range routes { |
| + if rt.Host == "" { |
| + continue |
| + } |
| + if apex == "" || better(rt.Host, apex) { |
| + apex = rt.Host |
| + } |
| } |
| + out := map[string]bool{} |
| for _, rt := range routes { |
| rung, ok := release.Rung(rt.Channel) |
| if !ok || rung != release.RungCanary { |
| continue |
| } |
| - if stableEnvs[rt.Environment] { |
| + if stableEnvs[rt.Environment] || rt.Host == apex { |
| out[rt.Host] = true |
| } |
| } |
diff --git a/internal/console/ladder_test.go b/internal/console/ladder_test.go| index 165978ae7..4742efb0d 100644 |
| --- a/internal/console/ladder_test.go |
| +++ b/internal/console/ladder_test.go |
| @@ -245,8 +245,38 @@ func TestProdWiredCanaryIsBadgedDistinctly(t *testing.T) { |
| } |
| } |
| +// A canary on the app's APEX host serves everybody, whatever it binds — so |
| +// it stays flagged even when its environment differs from stable's (review |
| +// IMPORTANT 5). The environment set-membership rule is the honest definition |
| +// of "wired to production glue"; the apex signal is the honest definition of |
| +// "in front of production traffic". Losing the second one when the first |
| +// landed would have shipped a silent reduction in warning coverage. |
| +func TestProdWiredCanaryOnTheApexHostIsFlaggedWhateverItBinds(t *testing.T) { |
| + fx := newConsoleFixture(t) |
| + reg, err := registry.Open(fx.regPath) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + // Put a canary on the apex host, bound to an environment nothing on |
| + // stable binds; docs.kass.carlosapp.dev stays as the app's stable route. |
| + if err := reg.Delete("kass.carlosapp.dev"); err != nil { |
| + t.Fatal(err) |
| + } |
| + fx.addRoute(t, registry.Route{Host: "kass.carlosapp.dev", Kind: "instance", Status: "ready", |
| + Version: "v2", App: "kass", Channel: "canary/x", Environment: "sandbox", Addr: "127.0.0.1:9007"}) |
| + |
| + c := signInPaul(t, fx) |
| + _, _, body := get(t, c, fx.u("/apps/kass")) |
| + block := rungBlock(t, body, "canary") |
| + line := routeLine(t, block, "kass.carlosapp.dev") |
| + if !strings.Contains(line, "chip-prod") { |
| + t.Errorf("a canary on the apex host is not flagged prod-wired:\n%s", line) |
| + } |
| +} |
| + |
| // No stable route at all means no production environment to be wired in |
| -// front of — a canary is never flagged in that case, whatever it binds. |
| +// front of — a canary is never flagged by the ENVIRONMENT signal in that |
| +// case, whatever it binds. |
| func TestProdWiredCanaryNeedsAStableRouteToCompareAgainst(t *testing.T) { |
| fx := newConsoleFixture(t) |
| reg, err := registry.Open(fx.regPath) |
| @@ -261,6 +291,11 @@ func TestProdWiredCanaryNeedsAStableRouteToCompareAgainst(t *testing.T) { |
| if err := reg.Delete("docs.kass.carlosapp.dev"); err != nil { |
| t.Fatal(err) |
| } |
| + // A non-canary route holds the apex, so the OTHER prod-wired signal (the |
| + // apex-host one, review IMPORTANT 5) cannot fire and this test stays |
| + // about the environment rule alone. |
| + fx.addRoute(t, registry.Route{Host: "kass.carlosapp.dev", Kind: "instance", Status: "ready", |
| + Version: "v2", App: "kass", Channel: "beta", Addr: "127.0.0.1:9005"}) |
| fx.addRoute(t, registry.Route{Host: "rc.kass.carlosapp.dev", Kind: "instance", Status: "ready", |
| Version: "v2", App: "kass", Channel: "canary/x", Addr: "127.0.0.1:9004"}) |
diff --git a/internal/console/view.go b/internal/console/view.go| index 77e916929..4da58704c 100644 |
| --- a/internal/console/view.go |
| +++ b/internal/console/view.go |
| @@ -183,6 +183,18 @@ func Assemble(regPath string, s release.Store, pub ed25519.PublicKey, acct strin |
| if err != nil { |
| return nil, err |
| } |
| + // byApp is keyed on app NAME with no account filter, so two accounts that |
| + // happen to name an app the same way share these buckets — pre-existing |
| + // (Host/Version/Channel/Status already cross that boundary, which is why |
| + // the RouteAccount mismatch machinery below exists) and gated: the app |
| + // page runs session → membership → HasAppIn → byte-identical 404 before |
| + // reaching here, so this is not a non-member leak. Recorded because the |
| + // blast radius grew rather than the gate weakening (2026-08-02 review, |
| + // MINOR 8): the Environment chip now shows one tenant's bundle NAMES to |
| + // the other, and prodWiredCanaryHosts' stable-environment set reads |
| + // across the same boundary, so one tenant's stable route can make |
| + // another's canary badge prod-wired. The fix is at this source — filter |
| + // by release.RouteAccount(rt) == acct — not further downstream. |
| byApp := map[string][]RouteView{} |
| domainsByApp := map[string][]DomainView{} |
| // What the ROUTES say each app's account is. A disagreement with the |
diff --git a/internal/domains/reconcile.go b/internal/domains/reconcile.go| index b9c49609a..fe691bea7 100644 |
| --- a/internal/domains/reconcile.go |
| +++ b/internal/domains/reconcile.go |
| @@ -508,15 +508,17 @@ func firstReadyUpstream(routes []registry.Route, k appKey) *registry.Route { |
| // |
| // Environment is never set here, by design (2026-08-02-plan-environments.md |
| // Task 2): reconcile does not derive an environment binding for a minted |
| -// route, so `carlos route -environment` is allowed on Platform routes, |
| -// unlike Channel, which platformCanaryOwnsChannel refuses on a |
| -// Platform+Canary route because THIS function re-derives Channel for static |
| -// mints. The same zero-value-on-re-mint caveat cli.go's runRoute documents |
| -// for Channel/Version/Status/DNS/Cert applies here too, though: if a |
| -// Platform+Canary route's `followed` check (below) goes false again, the |
| -// Upsert(nr) that follows rebuilds the row from this function's zero |
| -// values, silently clearing a previously-bound Environment along with |
| -// everything else that isn't copied above. |
| +// route, so `carlos route -environment` is allowed on a plain Platform apex. |
| +// It is NOT allowed on a Platform+Canary route (platformCanaryOwnedRow, |
| +// cli.go) — not because this function derives Environment, but because if |
| +// such a route's `followed` check (below) goes false again, the Upsert(nr) |
| +// that follows rebuilds the whole row from this function's values. That is |
| +// the same zero-value-on-re-mint caveat runRoute documents for |
| +// Channel/Version/Status/DNS/Cert. Since the 2026-08-02 review, |
| +// registry.Upsert preserves a non-empty existing environment through exactly |
| +// this path (IMPORTANT 3), so a binding is no longer silently lost here — |
| +// but a row this function rebuilds is still not one the route verb hands an |
| +// operator, which is why the CLI refusal exists on top of the preservation. |
| func mintedRoute(host, app, account string, src registry.Route) registry.Route { |
| rt := registry.Route{ |
| Host: host, |
diff --git a/internal/registry/registry.go b/internal/registry/registry.go| index 5e0795124..d3431f19e 100644 |
| --- a/internal/registry/registry.go |
| +++ b/internal/registry/registry.go |
| @@ -49,17 +49,24 @@ type Route struct { |
| // the app's default bundle, config.json, alone — see |
| // internal/appconfig.LoadMerged). Independent of Channel: channel is |
| // how mature a build is, environment is what glue it's wired to |
| - // (2026-08-02-pipelines-design.md §2/§4a). Set only by `carlos route |
| - // -environment` (cli.go's runRoute) — reconcile.go's mintedRoute never |
| - // sets this field, so a platform-minted route's Environment starts at |
| - // its zero value "" and IS eligible for `-environment` binding (unlike |
| - // Channel, which platformCanaryOwnsChannel refuses on a Platform+Canary |
| - // route). One caveat inherited from that same column: if a |
| - // Platform+Canary route's `followed` check ever goes false again, |
| - // reconcile's Upsert(nr) re-derives every column from mintedRoute's |
| - // zero values, Environment included — so a bound environment on such a |
| - // route is subject to the identical silent-revert risk runRoute's doc |
| - // comment already documents for Channel/Version/Status/DNS/Cert. |
| + // (2026-08-02-pipelines-design.md §2/§4a). |
| + // |
| + // Written INTENTIONALLY only by `carlos route -environment` (cli.go's |
| + // runRoute), through the narrow setters below (SetEnvironment, |
| + // SetEnvironments, SetChannelsAndEnvironments). An earlier revision of |
| + // this comment claimed that was the only writer at all; it was wrong — |
| + // every full-row Upsert (`carlos add`, reconcile.go's two mint paths) |
| + // also writes this column, and did so with the zero value, silently |
| + // unbinding a route on re-add or re-mint (2026-08-02 review, IMPORTANT |
| + // 3). Upsert now preserves a non-empty existing value instead; see its |
| + // doc comment. Clearing therefore has exactly one door: `-environment -`. |
| + // |
| + // Platform+Canary routes are refused by `carlos route -environment` |
| + // (platformCanaryOwnedRow, cli.go) for the same structural reason |
| + // Channel is: the reconciler owns those rows and re-Upserts them |
| + // whenever its `followed` check goes false. Upsert's preservation makes |
| + // that survivable rather than silent, but a column the reconciler |
| + // rebuilds is not one this verb should be handing an operator. |
| Environment string |
| Hibernate bool |
| @@ -406,7 +413,17 @@ func (r *Registry) Delete(host string) error { |
| return err |
| } |
| -// Upsert inserts or updates a route by host. |
| +// Upsert inserts or updates a route by host. One column is deliberately not |
| +// a plain overwrite: `environment` keeps the existing row's value when the |
| +// incoming Route carries the zero value (2026-08-02 review, IMPORTANT 3). |
| +// Every full-row Upsert caller — `carlos add` and the domains reconciler's |
| +// two mint paths — builds its Route without knowing anything about |
| +// environment bindings, so a plain excluded.environment silently unbound a |
| +// route on any re-add or re-mint, and the next config sync then rendered the |
| +// default bundle over that route's env file and restarted its unit. An |
| +// explicit non-empty value still wins, and clearing goes through |
| +// SetEnvironment (what `carlos route -environment -` calls), which is the |
| +// only place a clear is ever intended. |
| func (r *Registry) Upsert(t Route) error { |
| if t.CreatedAt == 0 { |
| t.CreatedAt = time.Now().Unix() |
| @@ -438,7 +455,7 @@ ON CONFLICT(host) DO UPDATE SET |
| platform=excluded.platform, dns_status=excluded.dns_status, |
| dns_checked_at=excluded.dns_checked_at, |
| cert_expires_at=excluded.cert_expires_at, account=excluded.account, |
| - environment=excluded.environment`, |
| + environment=CASE WHEN excluded.environment = '' THEN routes.environment ELSE excluded.environment END`, |
| t.Host, t.Slug, t.Kind, t.Socket, t.Addr, t.Status, t.LastError, |
| t.DBPath, t.Version, t.PrevVersion, t.App, t.Pool, t.Region, t.Channel, |
| b2i(t.Hibernate), b2i(t.Canary), t.Replication, t.NextDue, t.Backing, |
diff --git a/internal/registry/registry_test.go b/internal/registry/registry_test.go| index e24c52951..7a943f0fd 100644 |
| --- a/internal/registry/registry_test.go |
| +++ b/internal/registry/registry_test.go |
| @@ -706,3 +706,58 @@ func TestSetChannelsAndEnvironmentsAllOrNothing(t *testing.T) { |
| t.Fatalf("a.example.com = channel %q environment %q after a failed batch, want both untouched", got.Channel, got.Environment) |
| } |
| } |
| + |
| +// Review IMPORTANT 3: Upsert must not clear an existing row's Environment. |
| +// `carlos add` (and any other full-row Upsert caller) does not know about |
| +// environment bindings and passes the zero value, so a re-run on a bound host |
| +// used to silently unbind it — and the next config sync then re-rendered the |
| +// default bundle over that route's env file and restarted the unit. |
| +func TestUpsertPreservesAnExistingEnvironmentBinding(t *testing.T) { |
| + reg, err := Open(filepath.Join(t.TempDir(), "registry.db")) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + base := Route{Host: "demo.example.com", Kind: "instance", App: "tito", Status: "ready"} |
| + if err := reg.Upsert(base); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := reg.SetEnvironment("demo.example.com", "demo"); err != nil { |
| + t.Fatal(err) |
| + } |
| + // The re-add: same row, no Environment in hand. |
| + if err := reg.Upsert(base); err != nil { |
| + t.Fatal(err) |
| + } |
| + got, err := reg.Get("demo.example.com") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if got.Environment != "demo" { |
| + t.Fatalf("Environment = %q after a re-Upsert, want the binding preserved", got.Environment) |
| + } |
| + // An explicit non-empty value still wins — preservation is a floor, not a |
| + // freeze, so a caller that genuinely knows the binding can still set it. |
| + base.Environment = "staging" |
| + if err := reg.Upsert(base); err != nil { |
| + t.Fatal(err) |
| + } |
| + got, err = reg.Get("demo.example.com") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if got.Environment != "staging" { |
| + t.Fatalf("Environment = %q, want an explicit Upsert value to win", got.Environment) |
| + } |
| + // Clearing stays possible, through the narrow setter the clear verb uses |
| + // (`carlos route -environment -`), which is the only intentional clear. |
| + if err := reg.SetEnvironment("demo.example.com", ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + got, err = reg.Get("demo.example.com") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if got.Environment != "" { |
| + t.Fatalf("Environment = %q after an explicit clear, want it cleared", got.Environment) |
| + } |
| +} |