amadan

carlos / platform Public

appconfig/cli: fix-first review — observability for silent states, header

Addresses the 2026-08-03 Opus review (FIX-FIRST, three IMPORTANT — all
about silence, not damage) on top of the issue #18 platform-half branch.

IMPORTANT 1: a transient default-bundle fetch failure against an
ALREADY-opted-in app printed nothing, indistinguishable in the log from
an app that was never opted in. checkOptedIn now splits def.err != nil
into its own branch with its own log line (matching the socket path's
existing "unresolved ... left as-is" contract), and the comment now says
the actual reason the file stays safe either way: reapConfigEnv keys on
ROUTE EXISTENCE, never on scope, so a route out of scope for one pass —
for any reason — is simply never visited by resolution, materialization,
or reap.

IMPORTANT 2 (ruling, recorded here and at the code): stale
<host>.config.env after opt-out stays in place — deleting it would change
what the app's own supervisor layers on its next respawn, at a moment
carlos does not control and cannot announce, and no other ineligibility
transition in this file deletes proactively either. But the file holds
DECRYPTED secrets and nothing updates it again once opted out, so silence
was still wrong: every pass now names any stale file whose app is
DEFINITIVELY (not merely transiently-erroring) not opted in, and says
what to do about it.

IMPORTANT 3: the contract's file-header requirement — "the file's header
comment says so" — was simply missing. execAddrConfigHeader is prepended
to the addr shape's file ONLY (never global, never RenderEnv itself,
which sync.go's unit path shares): the addr shape has no installed base
to churn and never bounces, so the header is free there; adding it
fleet-wide would change every existing socket-exec file's bytes and
bounce every running instance once on upgrade. Format follows this repo's
own EnvironmentFile-comment precedent (lostKeys, sync.go) — verifying
titogo's actual parser against that assumption is called out as an
open item for the app-half PR, since this repo has no titogo checkout to
confirm it against directly.

MINOR 4: the CLI's -environment refusal claimed "SyncExec ignores it
there with a log" — there is no such log; LoadMerged structurally never
copies ExecDelivery into its result at all. Says the stronger, true thing
instead.

MINOR 5: the unknown-value sync log is now captured and asserted
(captureStdout), not just its filesystem-visible effect.

Tests added: TestSyncExecOptedInAddrRouteFetchFailureLogsAndLeavesFileAsIs,
TestSyncExecOptOutLeavesStaleFileButWarnsEveryPass (plus
TestSyncExecNoOptOutWarningWhenNeverOptedIn for the negative),
TestSyncExecAddrFileCarriesHeaderSocketFileDoesNot, and an extension of
TestSyncExecUnrecognizedExecDeliveryValueIgnoredNotFatal to assert its
log line. All pre-existing addr-shape tests' expected file content
updated for the new header. Full suite (15 packages), go vet, gofmt all
clean.
Paul Campbell 9b7b849e323d726383449b451f4babf35bfa5aa5 parent b660dbb
3 files changed, +469 −39
  • appconfig_cli.go +5 −4
  • internal/appconfig/syncexec.go +141 −22
  • internal/appconfig/syncexec_test.go +323 −13
diff --git a/appconfig_cli.go b/appconfig_cli.go
index 0aaeec0f3..1a4528ae1 100644
--- a/appconfig_cli.go
+++ b/appconfig_cli.go
@@ -271,10 +271,11 @@ func runEnvExecDelivery(args []string) {
}
if *environment != "" {
log.Fatal(`carlos env exec-delivery: -environment is refused — exec_delivery is APP-LEVEL ONLY, read from the ` +
- "default bundle (config.json) alone. A named environment's manifest is never consulted for it (SyncExec " +
- "ignores it there with a log, per the issue #18 design comment's \"config-that-lies\" rationale: the reader " +
- "is the app's own supervisor, one program per app, not per-environment) — so setting it on one would " +
- "silently do nothing useful. Omit -environment.")
+ "default bundle (config.json) alone. LoadMerged never carries this field from a named environment's " +
+ "manifest into anything the sync path reads — it builds its merged result from Vars alone — so it is not " +
+ "merely ignored there, it is structurally impossible for it to take effect there at all, per the issue #18 " +
+ "design comment's \"config-that-lies\" rationale: the reader is the app's own supervisor, one program per " +
+ "app, not per-environment. Omit -environment.")
}
var value string
switch fs.Arg(0) {
diff --git a/internal/appconfig/syncexec.go b/internal/appconfig/syncexec.go
index a8d18c438..fe81811db 100644
--- a/internal/appconfig/syncexec.go
+++ b/internal/appconfig/syncexec.go
@@ -101,42 +101,69 @@ func SyncExec(ctx context.Context, reg *registry.Registry, s release.Store, priv
defaults[ak] = d
return d
}
+ // optedInRes is checkOptedIn's cached verdict. known is false ONLY for
+ // a fetch error: v is a placeholder in that case, never a
+ // determination — see checkOptedIn's def.err branch for why that
+ // distinction matters to the stale-file notice below.
+ type optedInRes struct {
+ v bool
+ known bool
+ }
// optedIn caches the app-level exec_delivery decision so a box with
// many TCP-addr routes on one app doesn't cost more than one manifest
// read for it (loadDefault's own cache already dedupes the fetch; this
- // dedupes the value-interpretation, including the unknown-value log,
+ // dedupes the value-interpretation, including the two logs below,
// which must fire at most once per (acct, app) per pass, not once per
// route).
- optedIn := map[appKey]bool{}
- checkOptedIn := func(acct, app string) bool {
+ optedIn := map[appKey]optedInRes{}
+ checkOptedIn := func(acct, app string) optedInRes {
ak := appKey{acct, app}
- if v, done := optedIn[ak]; done {
- return v
+ if c, done := optedIn[ak]; done {
+ return c
}
def := loadDefault(acct, app)
- v := false
+ var c optedInRes
switch {
- case def.err != nil, !def.found:
- // A fetch failure or a genuinely never-configured app both read
- // as "not opted in" — the difference between them (retry next
- // pass vs. steady state) doesn't matter here: neither is
- // consulted below except as a false return, so nothing this
- // pass does can distinguish them, and nothing needs to.
- v = false
+ case def.err != nil:
+ // NOT a determination: this app may well be opted in and this
+ // box just failed to see it this pass — indistinguishable here
+ // from any other route's ordinary fetch-failure blip. Silence
+ // would mean an opted-in app's delivery can stall with zero
+ // trace in the log, on the exact path this feature exists to
+ // light up, so say so loudly — the same "unresolved, left
+ // as-is" contract the resolution loop below already gives a
+ // bound-group fetch failure.
+ //
+ // It is safe to leave any existing file exactly as it is
+ // regardless of how long this persists: NOT because a fetch
+ // failure and "never opted in" are indistinguishable (they
+ // aren't, that's the whole point of this branch existing
+ // separately from the ones below) but because reapConfigEnv
+ // keys on ROUTE EXISTENCE, never on scope (its live map is
+ // built from every route's Host, full stop — see its own doc
+ // comment). A route that drops out of scope for one pass, for
+ // ANY reason, is simply not visited by the resolution loop,
+ // the materialization loop, or reap; its file is untouched by
+ // construction, not because this function decided to spare
+ // it. Next pass retries from scratch.
+ fmt.Printf(" %s/%s: exec_delivery unresolved (config fetch failed), leaving this app's TCP-addr route file(s) as-is: %v\n", acct, app, def.err)
+ c = optedInRes{v: false, known: false}
+ case !def.found:
+ c = optedInRes{v: false, known: true}
case def.m.ExecDelivery == ExecDeliveryFile:
- v = true
+ c = optedInRes{v: true, known: true}
case def.m.ExecDelivery == "":
- v = false
+ c = optedInRes{v: false, known: true}
default:
// A manifest a NEWER carlos wrote with a value THIS binary
// doesn't understand must not brick this box's sync (additive
// discipline, CLAUDE.md) — ignore it, loudly, once, and treat
// the app as not opted in rather than refusing the whole pass.
fmt.Printf(" %s/%s: config: exec_delivery %q in the default bundle is not a value this binary understands — ignoring it (not opted in); upgrade carlos if this is expected\n", acct, app, def.m.ExecDelivery)
- v = false
+ c = optedInRes{v: false, known: true}
}
- optedIn[ak] = v
- return v
+ optedIn[ak] = c
+ return c
}
// inScope is the single door both loops below use to decide whether a
// route is in scope this pass, and — for the addr shape — that it must
@@ -148,7 +175,7 @@ func SyncExec(ctx context.Context, reg *registry.Registry, s release.Store, priv
if execSyncable(rt) {
return true, false
}
- if execAddrCandidate(rt) && checkOptedIn(release.RouteAccount(rt), rt.App) {
+ if execAddrCandidate(rt) && checkOptedIn(release.RouteAccount(rt), rt.App).v {
return true, true
}
return false, false
@@ -279,12 +306,62 @@ func SyncExec(ctx context.Context, reg *registry.Registry, s release.Store, priv
// own an app-supervised process, so there is structurally nothing
// to hand syncExecHost that COULD invoke it for this route (issue
// #18 design comment) — bounceLogged's existing nil-is-a-no-op
- // contract does the rest.
+ // contract does the rest. Same shape for the header: addrShape
+ // routes get execAddrConfigHeader, socket routes get "" (no
+ // header at all) — see execAddrConfigHeader's doc comment for why
+ // this must never apply to the socket shape.
hostBounce := bounce
+ header := ""
if addrShape {
hostBounce = nil
+ header = execAddrConfigHeader
+ }
+ syncExecHost(rt, r.m, Key(k.acct, k.app), priv, dataDir, hostBounce, header)
+ }
+ // Stale-secrets notice (issue #18 design ruling, ratified 2026-08-03
+ // review): an addr-shaped route whose app is DEFINITIVELY not opted in
+ // this pass (off, never configured, or an unrecognized exec_delivery
+ // value — checkOptedIn's known == true, as opposed to a transient
+ // fetch error) but still has a route row keeps any earlier
+ // <host>.config.env EXACTLY as it is: it was never added to
+ // inScope/resolved above, so neither the resolution loop nor the
+ // materialization loop touched it, and reap (below) never removes a
+ // file whose route still has a row.
+ //
+ // RULING: do not auto-delete it. Removing the file would change what
+ // the app's OWN supervisor layers in on its next respawn — a live
+ // environment change caused by a flag flip, taking effect at a moment
+ // carlos does not control and cannot announce, with no bounce (there
+ // never is one for this shape) to make it observable. That is worse
+ // than a stale file. It is also consistent with every other
+ // ineligibility transition in this file: a route losing
+ // Status: "ready" or gaining Platform: true doesn't get its file
+ // deleted either, only stops being written to.
+ //
+ // BUT staying silent about it is wrong, more than for those other
+ // transitions, because this file holds DECRYPTED secrets, and once an
+ // app opts out nothing will EVER update it again: rotate a secret in
+ // the bucket afterward and this file keeps serving yesterday's
+ // plaintext, indefinitely, with carlos reporting nothing. "Off" reads
+ // to an operator as "stop delivering config"; what it actually does
+ // is "freeze whatever was last delivered, forever". Name the file and
+ // the manual remedy, once per pass, so an operator doesn't mistake
+ // the one for the other — the remedy has to be manual (delete it once
+ // the app's own supervisor has stopped reading it) because carlos has
+ // no way to know when that is.
+ for _, rt := range routes {
+ if !execAddrCandidate(rt) {
+ continue
}
- syncExecHost(rt, r.m, Key(k.acct, k.app), priv, dataDir, hostBounce)
+ c := checkOptedIn(release.RouteAccount(rt), rt.App)
+ if c.v || !c.known {
+ continue
+ }
+ path := filepath.Join(dataDir, rt.Host+configEnvSuffix)
+ if _, err := os.Stat(path); err != nil {
+ continue // nothing was ever staged for this route: nothing to warn about
+ }
+ fmt.Printf(" %s: exec_delivery is off (or was never set) but %s still exists from an earlier opt-in — it holds DECRYPTED secrets and carlos will never update or remove it while this app stays un-opted-in; delete it by hand once the app's own supervisor has stopped reading it\n", rt.Host, path)
}
// An empty route list is never authority to delete anything —
// registry.Open silently creates an empty database on a wrong path or
@@ -336,6 +413,38 @@ func execAddrCandidate(rt registry.Route) bool {
return execRouteBase(rt) && rt.Socket == "" && rt.Addr != ""
}
+// execAddrConfigHeader is prepended to a TCP-addr (issue #18) route's
+// <host>.config.env — and ONLY that shape. RenderEnv itself stays
+// header-free, deliberately: it is shared by sync.go's unit path AND this
+// file's socket-exec path, and either one growing a header would change
+// the byte content of every EXISTING file of that shape on upgrade —
+// sync.go's own comment on lostKeys already weighs a "# managed by
+// carlos" marker for the unit path and rejects it for exactly this
+// churn. The socket-exec shape has the same installed-base problem: every
+// currently-running instance's file would look "changed" on the pass
+// after this ships, and — unlike the addr shape — a socket route DOES
+// bounce on a content change, so that churn would restart every
+// socket-exec instance on the fleet once, for a comment. The addr shape
+// has no installed base at all (this is its first pass ever) and never
+// bounces, so the header costs it nothing.
+//
+// Format: "#"-prefixed and blank lines are the universal EnvironmentFile
+// comment convention — systemd's own EnvironmentFile= parser skips them,
+// and this repo's own guard for the unit path, lostKeys (sync.go),
+// already relies on exactly that tolerance when parsing a hand-staged
+// file. The issue #18 design comment specifies titogo's controld layers
+// this file the same way systemd's EnvironmentFile= does (default bundle
+// under the app-supervisor's own env), so a plain KEY=value-plus-#-comments
+// reader is the assumed parse contract on that side too; this repo has no
+// titogo checkout to verify it against directly, so treat this as an
+// assumption to confirm against titogo's actual parser before or during
+// the app-half PR, not a verified fact.
+const execAddrConfigHeader = `# Written by carlos (SyncExec, issue #18). Carlos may overwrite or remove
+# this file on any future sync pass, but NEVER restarts this process because
+# of it — carlos does not own it. This app's own supervisor owns restarts
+# and must notice and act on changes here on its own schedule.
+`
+
// syncExecHost converges one route's config file. m == nil means the
// manifest object is absent from the bucket: remove the file (sync owns
// the namespace; a file with no manifest is a leftover, and leaving it
@@ -352,7 +461,14 @@ func execAddrCandidate(rt registry.Route) bool {
// same logging — while never restarting anything, because carlos does
// not own that process. The caller (SyncExec) passes nil for those routes
// structurally, not a flag this function branches on.
-func syncExecHost(rt registry.Route, m *Manifest, key string, priv *ecdh.PrivateKey, dataDir string, bounce func(host string) error) {
+//
+// header is prepended to the rendered content when non-empty
+// (execAddrConfigHeader for the addr shape, "" for socket — see that
+// constant's doc comment for why the two must never be swapped or
+// unified) — folded into the SAME diff/write/rename path as the vars
+// themselves, so a header-only change (e.g. its text edited in a future
+// release) converges exactly like a var change would, once.
+func syncExecHost(rt registry.Route, m *Manifest, key string, priv *ecdh.PrivateKey, dataDir string, bounce func(host string) error, header string) {
if rt.Host == "" || rt.Host == "." || rt.Host == ".." || rt.Host != filepath.Base(rt.Host) {
fmt.Printf(" %q: not a bare hostname, refusing to derive a config file path from it\n", rt.Host)
return
@@ -390,6 +506,9 @@ func syncExecHost(rt registry.Route, m *Manifest, key string, priv *ecdh.Private
fmt.Printf(" %s: render config: %v\n", rt.Host, err)
return
}
+ if header != "" {
+ rendered = append([]byte(header), rendered...)
+ }
if existed && bytes.Equal(cur, rendered) {
return // up to date — stay quiet on the periodic tick
}
diff --git a/internal/appconfig/syncexec_test.go b/internal/appconfig/syncexec_test.go
index 118cb3f8e..8c65f7371 100644
--- a/internal/appconfig/syncexec_test.go
+++ b/internal/appconfig/syncexec_test.go
@@ -1,10 +1,14 @@
package appconfig
import (
+ "bytes"
"context"
+ "errors"
+ "io"
"os"
"path/filepath"
"slices"
+ "strings"
"testing"
"github.com/carlosframework/platform/internal/registry"
@@ -26,6 +30,52 @@ func execKit(t *testing.T) (ctx context.Context, s release.Store, reg *registry.
return
}
+// captureStdout runs fn with os.Stdout redirected to a pipe and returns
+// everything fn printed. SyncExec logs via fmt.Printf directly to
+// os.Stdout, matching every other pass in this package — this is the only
+// way to pin a log line's presence without restructuring every call site
+// to take a Writer, so it's used sparingly: only where a test's whole
+// point IS that a specific line got printed (a `fmt.Printf` deleted by a
+// future edit would otherwise go unnoticed by every other test, which
+// only assert on the filesystem).
+func captureStdout(t *testing.T, fn func()) string {
+ t.Helper()
+ r, w, err := os.Pipe()
+ if err != nil {
+ t.Fatalf("os.Pipe: %v", err)
+ }
+ old := os.Stdout
+ os.Stdout = w
+ defer func() { os.Stdout = old }()
+ fn()
+ if err := w.Close(); err != nil {
+ t.Fatalf("close pipe writer: %v", err)
+ }
+ var buf bytes.Buffer
+ if _, err := io.Copy(&buf, r); err != nil {
+ t.Fatalf("read captured stdout: %v", err)
+ }
+ return buf.String()
+}
+
+// failingKeyStore wraps a release.Store and makes Get fail with a fixed
+// error for exactly one key — simulating a transient bucket blip against a
+// single app's default bundle while every other key (other apps' bundles,
+// including in the SAME pass) resolves normally, the shape a real S3
+// SlowDown/503 against one object takes.
+type failingKeyStore struct {
+ release.Store
+ failKey string
+ err error
+}
+
+func (f *failingKeyStore) Get(ctx context.Context, key string) ([]byte, string, error) {
+ if key == f.failKey {
+ return nil, "", f.err
+ }
+ return f.Store.Get(ctx, key)
+}
+
func TestSyncExecWritesConfigFileAndBouncesOnChange(t *testing.T) {
ctx, s, reg, dd, priv, _ := execKit(t)
pk, err := ParsePrivateKey(priv, "test")
@@ -554,7 +604,7 @@ func TestSyncExecRefusesNonBareHostname(t *testing.T) {
pk, _ := ParsePrivateKey(priv, "test")
m := &Manifest{Schema: 1, App: "eleven", Vars: map[string]Var{"A": {Kind: "env", Value: "1"}}}
for _, host := range []string{"../escape", "a/b", "", ".", ".."} {
- syncExecHost(registry.Route{Host: host}, m, "apps/bab/eleven/config.json", pk, dd, func(string) error { return nil })
+ syncExecHost(registry.Route{Host: host}, m, "apps/bab/eleven/config.json", pk, dd, func(string) error { return nil }, "")
}
ents, err := os.ReadDir(dd)
if err != nil {
@@ -596,8 +646,9 @@ func TestSyncExecOptedInAddrRouteMaterializesFile(t *testing.T) {
if err != nil {
t.Fatalf("opted-in TCP-addr route got no config file: %v", err)
}
- if string(got) != "TITOGO_DEMO_BANNER=1\n" {
- t.Fatalf("config env = %q, want TITOGO_DEMO_BANNER=1", got)
+ want := execAddrConfigHeader + "TITOGO_DEMO_BANNER=1\n"
+ if string(got) != want {
+ t.Fatalf("config env = %q, want %q (issue #18's required header, then the vars)", got, want)
}
}
@@ -663,8 +714,12 @@ func TestSyncExecUnrecognizedExecDeliveryValueIgnoredNotFatal(t *testing.T) {
t.Fatalf("Upsert %s: %v", rt.Host, err)
}
}
- if err := SyncExec(ctx, reg, s, pk, dd, func(string) error { return nil }); err != nil {
- t.Fatalf("SyncExec: %v", err)
+ var syncErr error
+ out := captureStdout(t, func() {
+ syncErr = SyncExec(ctx, reg, s, pk, dd, func(string) error { return nil })
+ })
+ if syncErr != nil {
+ t.Fatalf("SyncExec: %v", syncErr)
}
if _, err := os.Stat(filepath.Join(dd, "demo.go.tito.io.config.env")); !os.IsNotExist(err) {
t.Fatalf("unrecognized exec_delivery value was treated as opted-in, stat err = %v", err)
@@ -672,6 +727,84 @@ func TestSyncExecUnrecognizedExecDeliveryValueIgnoredNotFatal(t *testing.T) {
if _, err := os.Stat(filepath.Join(dd, "inst.example.com.config.env")); err != nil {
t.Fatalf("eleven's ordinary socket route did not converge despite titogo's unrecognized value: %v", err)
}
+ // MINOR 5 (2026-08-03 review): the whole safety story for this case is
+ // "ignored LOUDLY" — pin the loudness itself, not just the tolerance,
+ // so a future edit that quietly dropped the fmt.Printf would fail a
+ // test instead of only being noticed by someone reading a log by hand.
+ if !strings.Contains(out, "titogo") || !strings.Contains(out, "grpc") {
+ t.Fatalf("stdout = %q, want a line naming the app (titogo) and the unrecognized value (grpc)", out)
+ }
+}
+
+// TestSyncExecOptedInAddrRouteFetchFailureLogsAndLeavesFileAsIs is
+// IMPORTANT 1 from the 2026-08-03 review: a transient default-bundle read
+// failure against an ALREADY-opted-in app must not go silent. Before this
+// fix, checkOptedIn folded def.err != nil into the same "not opted in,
+// say nothing" branch as a genuinely never-configured app — so an opted-in
+// app whose bucket blips became indistinguishable, in the log, from an
+// app that was never opted in at all, on the exact path this feature
+// exists to make observable. The safety property (the file survives
+// byte-identical) was already correct; only the silence was the bug.
+func TestSyncExecOptedInAddrRouteFetchFailureLogsAndLeavesFileAsIs(t *testing.T) {
+ ctx, s, reg, dd, priv, _ := execKit(t)
+ pk, err := ParsePrivateKey(priv, "test")
+ if err != nil {
+ t.Fatalf("ParsePrivateKey: %v", err)
+ }
+ if err := SetEnv(ctx, s, "bab", "titogo", map[string]string{"TITOGO_DEMO_BANNER": "1"}); err != nil {
+ t.Fatalf("SetEnv: %v", err)
+ }
+ if err := SetExecDelivery(ctx, s, "bab", "titogo", ExecDeliveryFile); err != nil {
+ t.Fatalf("SetExecDelivery: %v", err)
+ }
+ if err := reg.Upsert(registry.Route{
+ Host: "demo.go.tito.io", Kind: "instance", App: "titogo", Account: "bab",
+ Status: "ready", Addr: "127.0.0.1:8444",
+ }); err != nil {
+ t.Fatalf("Upsert: %v", err)
+ }
+ // First pass materializes normally — this is the file a transient
+ // blip must leave exactly as it is.
+ if err := SyncExec(ctx, reg, s, pk, dd, func(string) error { return nil }); err != nil {
+ t.Fatalf("SyncExec (initial): %v", err)
+ }
+ path := filepath.Join(dd, "demo.go.tito.io.config.env")
+ original, err := os.ReadFile(path)
+ if err != nil {
+ t.Fatalf("read initial config env: %v", err)
+ }
+
+ blipErr := errors.New("s3: transient 503 SlowDown")
+ failing := &failingKeyStore{Store: s, failKey: Key("bab", "titogo"), err: blipErr}
+ var bounced []string
+ bounce := func(h string) error { bounced = append(bounced, h); return nil }
+ var syncErr error
+ out := captureStdout(t, func() {
+ syncErr = SyncExec(ctx, reg, failing, pk, dd, bounce)
+ })
+ if syncErr != nil {
+ t.Fatalf("SyncExec (blip): %v", syncErr)
+ }
+ got, err := os.ReadFile(path)
+ if err != nil {
+ t.Fatalf("read config env after the blip: %v", err)
+ }
+ if string(got) != string(original) {
+ t.Fatalf("config env changed across a transient fetch failure: got %q, want the original %q untouched", got, original)
+ }
+ if len(bounced) != 0 {
+ t.Fatalf("bounced = %v, want none — a fetch failure is not a change", bounced)
+ }
+ // The fix: this must no longer be silent. Name the app and the
+ // underlying error, the same "unresolved ... left as-is" contract the
+ // socket path's resolution loop already gives an ordinary fetch
+ // failure.
+ if !strings.Contains(out, "titogo") {
+ t.Fatalf("stdout = %q, want a line naming the app (titogo)", out)
+ }
+ if !strings.Contains(out, "SlowDown") {
+ t.Fatalf("stdout = %q, want the underlying fetch error surfaced", out)
+ }
}
// TestSyncExecOptedInAddrRouteIsNeverBounced is the structural pin the
@@ -726,8 +859,9 @@ func TestSyncExecOptedInAddrRouteIsNeverBounced(t *testing.T) {
if err != nil {
t.Fatalf("read config env: %v", err)
}
- if string(got) != "TITOGO_DEMO_BANNER=2\n" {
- t.Fatalf("config env = %q, want the changed content (TITOGO_DEMO_BANNER=2) — the write path must still run", got)
+ want := execAddrConfigHeader + "TITOGO_DEMO_BANNER=2\n"
+ if string(got) != want {
+ t.Fatalf("config env = %q, want %q — the changed content, header included — the write path must still run", got, want)
}
}
@@ -817,7 +951,7 @@ func TestSyncExecOptedInAddrRouteHonorsBoundEnvironment(t *testing.T) {
if err != nil {
t.Fatalf("read config env: %v", err)
}
- want := "BASE_ONLY=1\nDEMO_ONLY=yes\nLOG_LEVEL=debug\n"
+ want := execAddrConfigHeader + "BASE_ONLY=1\nDEMO_ONLY=yes\nLOG_LEVEL=debug\n"
if string(got) != want {
t.Fatalf("config env = %q, want %q", got, want)
}
@@ -854,10 +988,13 @@ func TestSyncExecOptedInAddrAppWithNoVarsConvergesToEmptyFile(t *testing.T) {
}
got, err := os.ReadFile(filepath.Join(dd, "demo.go.tito.io.config.env"))
if err != nil {
- t.Fatalf("opted-in-but-unconfigured app's route got no file at all (want an empty one, not none): %v", err)
+ t.Fatalf("opted-in-but-unconfigured app's route got no file at all (want the header alone, not none): %v", err)
}
- if string(got) != "" {
- t.Fatalf("config env = %q, want empty", got)
+ // No vars, but never a truly empty file: the header (issue #18) is
+ // unconditional for this shape, whether or not the app has any vars
+ // set yet.
+ if string(got) != execAddrConfigHeader {
+ t.Fatalf("config env = %q, want just the header (%q)", got, execAddrConfigHeader)
}
}
@@ -908,8 +1045,9 @@ func TestSyncExecMassRemovalGuardUnaffectedByOptedInAddrAppWithContent(t *testin
if err != nil {
t.Fatalf("titogo's config file wasn't written: %v", err)
}
- if string(got) != "A=1\n" {
- t.Fatalf("titogo config env = %q, want A=1", got)
+ want := execAddrConfigHeader + "A=1\n"
+ if string(got) != want {
+ t.Fatalf("titogo config env = %q, want %q", got, want)
}
if _, err := os.Stat(pathEleven); !os.IsNotExist(err) {
t.Fatalf("eleven's config file survived its manifest's genuine absence, stat err = %v (guard incorrectly disarmed convergence, or incorrectly held it back)", err)
@@ -928,6 +1066,178 @@ func TestSyncExecMassRemovalGuardUnaffectedByOptedInAddrAppWithContent(t *testin
}
}
+// TestSyncExecOptOutLeavesStaleFileButWarnsEveryPass is IMPORTANT 2 from
+// the 2026-08-03 review's ruling: turning exec_delivery off does NOT
+// delete an already-materialized <host>.config.env (deleting it would
+// change what the app's own supervisor layers in on its next respawn, at
+// a moment carlos does not control and cannot announce — worse than a
+// stale file, and consistent with every other ineligibility transition in
+// this file). But the file holds DECRYPTED secrets and nothing will ever
+// update it again once opted out, so the silence itself was the bug:
+// every pass must name the stale file and the manual remedy.
+func TestSyncExecOptOutLeavesStaleFileButWarnsEveryPass(t *testing.T) {
+ ctx, s, reg, dd, priv, _ := execKit(t)
+ pk, err := ParsePrivateKey(priv, "test")
+ if err != nil {
+ t.Fatalf("ParsePrivateKey: %v", err)
+ }
+ if err := SetEnv(ctx, s, "bab", "titogo", map[string]string{"TITOGO_DEMO_BANNER": "1"}); err != nil {
+ t.Fatalf("SetEnv: %v", err)
+ }
+ if err := SetExecDelivery(ctx, s, "bab", "titogo", ExecDeliveryFile); err != nil {
+ t.Fatalf("SetExecDelivery: %v", err)
+ }
+ if err := reg.Upsert(registry.Route{
+ Host: "demo.go.tito.io", Kind: "instance", App: "titogo", Account: "bab",
+ Status: "ready", Addr: "127.0.0.1:8444",
+ }); err != nil {
+ t.Fatalf("Upsert: %v", err)
+ }
+ if err := SyncExec(ctx, reg, s, pk, dd, func(string) error { return nil }); err != nil {
+ t.Fatalf("SyncExec (opted in): %v", err)
+ }
+ path := filepath.Join(dd, "demo.go.tito.io.config.env")
+ staged, err := os.ReadFile(path)
+ if err != nil {
+ t.Fatalf("read staged config env: %v", err)
+ }
+
+ if err := SetExecDelivery(ctx, s, "bab", "titogo", ""); err != nil {
+ t.Fatalf("SetExecDelivery (off): %v", err)
+ }
+ var bounced []string
+ bounce := func(h string) error { bounced = append(bounced, h); return nil }
+ var syncErr error
+ out := captureStdout(t, func() {
+ syncErr = SyncExec(ctx, reg, s, pk, dd, bounce)
+ })
+ if syncErr != nil {
+ t.Fatalf("SyncExec (opted out): %v", syncErr)
+ }
+ got, err := os.ReadFile(path)
+ if err != nil {
+ t.Fatalf("stale config env was deleted on opt-out, want it left in place: %v", err)
+ }
+ if string(got) != string(staged) {
+ t.Fatalf("stale config env changed on opt-out: got %q, want the untouched original %q", got, staged)
+ }
+ if len(bounced) != 0 {
+ t.Fatalf("bounced = %v, want none", bounced)
+ }
+ if !strings.Contains(out, "demo.go.tito.io") || !strings.Contains(out, path) {
+ t.Fatalf("stdout = %q, want a line naming the host and the stale file's path", out)
+ }
+ if !strings.Contains(strings.ToLower(out), "secret") {
+ t.Fatalf("stdout = %q, want the warning to say the file holds secrets", out)
+ }
+
+ // The warning is PER PASS, not one-shot: a second pass with nothing
+ // changed must repeat it, since nothing about the risk (stale
+ // plaintext secrets, forever) has gone away.
+ out2 := captureStdout(t, func() {
+ syncErr = SyncExec(ctx, reg, s, pk, dd, bounce)
+ })
+ if syncErr != nil {
+ t.Fatalf("SyncExec (opted out, second pass): %v", syncErr)
+ }
+ if !strings.Contains(out2, "demo.go.tito.io") {
+ t.Fatalf("second pass stdout = %q, want the warning repeated", out2)
+ }
+ if len(bounced) != 0 {
+ t.Fatalf("bounced = %v after the second pass, want none", bounced)
+ }
+}
+
+// TestSyncExecNoOptOutWarningWhenNeverOptedIn: the stale-file warning
+// fires only when a file actually exists — an app that was simply NEVER
+// opted in must stay exactly as quiet as it was before issue #18 existed,
+// not gain a new per-pass line for a file it never had.
+func TestSyncExecNoOptOutWarningWhenNeverOptedIn(t *testing.T) {
+ ctx, s, reg, dd, priv, _ := execKit(t)
+ pk, err := ParsePrivateKey(priv, "test")
+ if err != nil {
+ t.Fatalf("ParsePrivateKey: %v", err)
+ }
+ if err := SetEnv(ctx, s, "bab", "titogo", map[string]string{"K": "1"}); err != nil {
+ t.Fatalf("SetEnv: %v", err)
+ }
+ // No SetExecDelivery call at all.
+ if err := reg.Upsert(registry.Route{
+ Host: "demo.go.tito.io", Kind: "instance", App: "titogo", Account: "bab",
+ Status: "ready", Addr: "127.0.0.1:8444",
+ }); err != nil {
+ t.Fatalf("Upsert: %v", err)
+ }
+ var syncErr error
+ out := captureStdout(t, func() {
+ syncErr = SyncExec(ctx, reg, s, pk, dd, func(string) error { return nil })
+ })
+ if syncErr != nil {
+ t.Fatalf("SyncExec: %v", syncErr)
+ }
+ if strings.Contains(out, "demo.go.tito.io") {
+ t.Fatalf("stdout = %q, want no mention of a route that never had a file", out)
+ }
+}
+
+// TestSyncExecAddrFileCarriesHeaderSocketFileDoesNot is IMPORTANT 3 from
+// the 2026-08-03 review: the issue #18 design comment's contract line —
+// "the file's header comment says so" — pinned directly. The addr shape's
+// file must carry it; the socket shape's file (same function, same pass)
+// must NOT, so an existing fleet's socket-exec files never churn or bounce
+// on upgrade (see execAddrConfigHeader's doc comment for the full reason).
+func TestSyncExecAddrFileCarriesHeaderSocketFileDoesNot(t *testing.T) {
+ ctx, s, reg, dd, priv, _ := execKit(t)
+ pk, err := ParsePrivateKey(priv, "test")
+ if err != nil {
+ t.Fatalf("ParsePrivateKey: %v", err)
+ }
+ if err := SetEnv(ctx, s, "bab", "titogo", map[string]string{"A": "1"}); err != nil {
+ t.Fatalf("SetEnv: %v", err)
+ }
+ if err := SetExecDelivery(ctx, s, "bab", "titogo", ExecDeliveryFile); err != nil {
+ t.Fatalf("SetExecDelivery: %v", err)
+ }
+ if err := SetEnv(ctx, s, "bab", "eleven", map[string]string{"B": "1"}); err != nil {
+ t.Fatalf("SetEnv: %v", err)
+ }
+ routes := []registry.Route{
+ {Host: "demo.go.tito.io", Kind: "instance", App: "titogo", Account: "bab", Status: "ready", Addr: "127.0.0.1:8444"},
+ {Host: "inst.example.com", Kind: "instance", App: "eleven", Account: "bab", Status: "ready", Socket: "/run/carlos/inst.sock"},
+ }
+ for _, rt := range routes {
+ if err := reg.Upsert(rt); err != nil {
+ t.Fatalf("Upsert %s: %v", rt.Host, err)
+ }
+ }
+ if err := SyncExec(ctx, reg, s, pk, dd, func(string) error { return nil }); err != nil {
+ t.Fatalf("SyncExec: %v", err)
+ }
+ addrGot, err := os.ReadFile(filepath.Join(dd, "demo.go.tito.io.config.env"))
+ if err != nil {
+ t.Fatalf("read addr config env: %v", err)
+ }
+ if !strings.HasPrefix(string(addrGot), "#") {
+ t.Fatalf("addr config env = %q, want it to start with a header comment", addrGot)
+ }
+ if !strings.Contains(string(addrGot), "carlos") {
+ t.Fatalf("addr config env = %q, want the header to say carlos wrote it", addrGot)
+ }
+ if !strings.Contains(strings.ToLower(string(addrGot)), "never") || !strings.Contains(strings.ToLower(string(addrGot)), "restart") {
+ t.Fatalf("addr config env = %q, want the header to say carlos never restarts this process", addrGot)
+ }
+ if !strings.Contains(strings.ToLower(string(addrGot)), "supervisor") {
+ t.Fatalf("addr config env = %q, want the header to say the app's supervisor owns restarts", addrGot)
+ }
+ socketGot, err := os.ReadFile(filepath.Join(dd, "inst.example.com.config.env"))
+ if err != nil {
+ t.Fatalf("read socket config env: %v", err)
+ }
+ if want := "B=1\n"; string(socketGot) != want {
+ t.Fatalf("socket config env = %q, want exactly %q — no header on this shape, ever", socketGot, want)
+ }
+}
+
// TestSyncExecConvergesToEmptyFileWhenAllVarsUnset covers the lifecycle
// F8 flagged as missing: a manifest object that still EXISTS but has had
// every var unset must converge its route to an EMPTY file, not have the