amadan
docs: plan for the #69/#70 db-path fixes
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 file changed,
+380
−0
docs/superpowers/plans/2026-08-06-issues-69-70-db-path.md+380 −0
diff --git a/docs/superpowers/plans/2026-08-06-issues-69-70-db-path.md b/docs/superpowers/plans/2026-08-06-issues-69-70-db-path.md| new file mode 100644 |
| index 000000000..17708b6b5 |
| --- /dev/null |
| +++ b/docs/superpowers/plans/2026-08-06-issues-69-70-db-path.md |
| @@ -0,0 +1,380 @@ |
| +# Issues #69 + #70: db-path staleness and relative-path failure — Implementation Plan |
| + |
| +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. |
| + |
| +**Goal:** Fix the two defects found in vitogo's first tito deploy (2026-08-06): a re-`add` that changes only `-db` never reaches a running edge (#69), and a relative `-db` on an agent-exec route fails every wake as edge 503s (#70). |
| + |
| +**Architecture:** Both root causes are now diagnosed and are NOT what the issue titles guess. (#69) `registry.Upsert` **does** overwrite `db_path` in the DB — the defect is `Registry.Fingerprint()` (the edge's cheap change poll), which sums weighted lengths of `status/socket/addr/version/hibernate/channel/backing/tunnel` but not `db_path`, so a db-path-only rewrite never triggers a route reload and the activator keeps waking the host with the stale cached path. (#70) exec children have no `STATE_DIRECTORY` and the activator/child both consume `rt.DBPath` verbatim (Restore/Seed/Start `--db`/Replicate), so a relative path resolves against an unwritable cwd; every mint path except `carlos add -db` already produces absolute paths, so the fix is a loud add-time refusal at the one door that can introduce the misconfiguration (issue #70's suggested fix 2). We also make `add` say `updated` vs `added` so an upsert-over-existing is visible to the operator (#69's operator-honesty half). |
| + |
| +**Tech Stack:** Go, modernc sqlite registry, stdlib flag CLI, e2e tests that build and shell out to the carlos binary. |
| + |
| +## Global Constraints |
| + |
| +- `go build ./...`, `go vet ./...`, `go test ./... -count=1` all clean (all 18 packages) before any commit lands (CLAUDE.md). |
| +- `gofmt -l` clean for files touched. |
| +- Sandbox: go commands need `GOCACHE=/tmp/claude-1001/gocache GOFLAGS=-mod=mod GOPROXY=off` and may need the sandbox disabled ("read-only file system" is the tell). |
| +- Wire-shape changes are additive: nothing here changes storage; existing registry rows (all absolute paths in production; vitogo's bad row was already removed) must keep parsing. |
| +- Any e2e test that shells out to the carlos binary must build env through `carlosTestEnv(t)` and set `cmd.Dir` to a scratch dir (the binary walks cwd ancestors for `.carlos/config`). |
| +- Working branch: `worktree-issues-69-70-db-path` (already created). Never push main; open a PR at the end. |
| +- Delegation (CLAUDE.md): registry/CLI door is production-touching — Opus reviews the finished branch before the PR opens. |
| + |
| +--- |
| + |
| +### Task 1: Fingerprint sees db_path rewrites (#69 core) |
| + |
| +**Files:** |
| +- Modify: `internal/registry/registry.go` (Fingerprint, ~line 1009) |
| +- Test: `internal/registry/registry_test.go` (extend the existing fingerprint test that ends with the tunnel-binding check, ~line 190) |
| + |
| +**Interfaces:** |
| +- Consumes: existing `Registry.Fingerprint() (string, error)`, `Registry.Get/Upsert`. |
| +- Produces: no signature changes — only the fingerprint SQL gains a `LENGTH(db_path)*19` term. Weight 19 is the next unused prime (3,5,7,11,13 are column weights; 17 is the rowid multiplier). |
| + |
| +- [ ] **Step 1: Write the failing test** |
| + |
| +In `internal/registry/registry_test.go`, extend the existing fingerprint test (the one containing `t.Fatal("fingerprint blind to a tunnel binding")`) by appending, after that final check: |
| + |
| +```go |
| + // db_path rewrites must reload the edge too: a re-`carlos add` that only |
| + // fixes -db otherwise leaves the running activator waking the host with |
| + // the stale cached path forever — the exact vitogo incident (issue #69): |
| + // `add` reported success, the row was updated, and wakes still died on |
| + // the old path minutes later because this poll never moved. |
| + rt, err = reg.Get("c.example.com") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + rt.DBPath = "/data/somewhere-else.db" |
| + if err := reg.Upsert(*rt); err != nil { |
| + t.Fatal(err) |
| + } |
| + withDBPath, err := reg.Fingerprint() |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if withDBPath == withTunnel { |
| + t.Fatal("fingerprint blind to a db_path rewrite") |
| + } |
| +``` |
| + |
| +Note: `rt` and `err` already exist in that scope (the backing-flip check declared them), so this is `rt, err =`, not `:=`. If the route `c.example.com` in that test already carries a `DBPath`, pick a replacement value of a **different length** — the fingerprint is length-weighted by design and a same-length rewrite is deliberately invisible. |
| + |
| +- [ ] **Step 2: Run the test to verify it fails** |
| + |
| +Run: `go test ./internal/registry/ -run 'Fingerprint' -count=1 -v` (with the sandbox go env vars) |
| +Expected: FAIL with `fingerprint blind to a db_path rewrite` |
| + |
| +- [ ] **Step 3: Add the term to Fingerprint** |
| + |
| +In `internal/registry/registry.go`, change the SUM expression inside `Fingerprint()`: |
| + |
| +```go |
| + COALESCE(SUM(rowid*17 + LENGTH(status)*3 + LENGTH(socket) + LENGTH(addr) |
| + + LENGTH(version)*5 + hibernate*7 + LENGTH(channel) |
| + + LENGTH(backing)*11 + LENGTH(tunnel)*13 + LENGTH(db_path)*19),0) |
| +``` |
| + |
| +And extend the function's doc comment: after the sentence about weights, add one sentence — `db_path is in the sum (issue #69): a re-add that only re-points a route's database must reach a running edge, or the activator keeps waking the host on the stale cached path.` |
| + |
| +- [ ] **Step 4: Run the test to verify it passes** |
| + |
| +Run: `go test ./internal/registry/ -count=1` (full package — the fingerprint change can break other fingerprint assertions if any pin exact values; fix only by updating pinned strings, never by weakening assertions) |
| +Expected: PASS |
| + |
| +- [ ] **Step 5: Commit** |
| + |
| +```bash |
| +git add internal/registry/registry.go internal/registry/registry_test.go |
| +git commit -m "registry: fingerprint sees db_path rewrites (#69)" |
| +``` |
| + |
| +### Task 2: `carlos add` says updated vs added (#69 operator honesty) |
| + |
| +**Files:** |
| +- Modify: `cli.go` (`runAdd`, the tail: the `registry.Open` → `Upsert` → `fmt.Printf("added %s\n", host)` block, ~lines 148–161) |
| +- Test: `e2e_test.go` (new test function, following the shape of the `-account-id` refusal test at ~line 1370) |
| + |
| +**Interfaces:** |
| +- Consumes: `registry.Registry.Get(host string) (*Route, error)` (existing), `registry.Registry.Upsert(Route) error` (existing). |
| +- Produces: `carlos add` prints `updated <host> (existing route)` when the host row already existed, `added <host>` otherwise. No flag changes. |
| + |
| +- [ ] **Step 1: Write the failing test** |
| + |
| +Add to `e2e_test.go`: |
| + |
| +```go |
| +// carlos add on an existing host is an update and must say so — the vitogo |
| +// incident (issue #69) started with `added vito.platform.tito.io` printed |
| +// over what was actually a rewrite of an existing row. |
| +func TestAddOnExistingRouteSaysUpdated(t *testing.T) { |
| + goBin, err := exec.LookPath("go") |
| + if err != nil { |
| + t.Skip("go toolchain not found") |
| + } |
| + tmp := t.TempDir() |
| + carlosBin := filepath.Join(tmp, "carlos") |
| + build := exec.Command(goBin, "build", "-o", carlosBin, ".") |
| + build.Env = os.Environ() |
| + if out, err := build.CombinedOutput(); err != nil { |
| + t.Fatalf("build carlos: %v\n%s", err, out) |
| + } |
| + regPath := filepath.Join(tmp, "registry.db") |
| + |
| + run := func(args ...string) (string, error) { |
| + cmd := exec.Command(carlosBin, args...) |
| + cmd.Env = carlosTestEnv(t) |
| + cmd.Dir = tmp |
| + out, err := cmd.CombinedOutput() |
| + return string(out), err |
| + } |
| + |
| + out, err := run("add", "-registry", regPath, "-app", "vitogo", |
| + "-db", "/data/first.db", "vito.example.com") |
| + if err != nil { |
| + t.Fatalf("first add: %v\n%s", err, out) |
| + } |
| + if !strings.Contains(out, "added vito.example.com") { |
| + t.Errorf("first add output = %q, want %q", out, "added vito.example.com") |
| + } |
| + |
| + out, err = run("add", "-registry", regPath, "-app", "vitogo", |
| + "-db", "/data/second.db", "vito.example.com") |
| + if err != nil { |
| + t.Fatalf("re-add: %v\n%s", err, out) |
| + } |
| + if !strings.Contains(out, "updated vito.example.com") { |
| + t.Errorf("re-add output = %q, want it to say updated, not added", out) |
| + } |
| + |
| + // And the rewrite must actually land — the row carries the new path. |
| + reg, err := registry.Open(regPath) |
| + if err != nil { |
| + t.Fatalf("open registry: %v", err) |
| + } |
| + defer reg.Close() |
| + rt, err := reg.Get("vito.example.com") |
| + if err != nil || rt == nil { |
| + t.Fatalf("get route: %v (rt=%v)", err, rt) |
| + } |
| + if rt.DBPath != "/data/second.db" { |
| + t.Errorf("db_path = %q, want /data/second.db", rt.DBPath) |
| + } |
| +} |
| +``` |
| + |
| +(`registry` is already imported in `e2e_test.go`; `os`, `strings`, `filepath`, `exec` too — verify and add any missing import.) |
| + |
| +- [ ] **Step 2: Run the test to verify it fails** |
| + |
| +Run: `go test . -run TestAddOnExistingRouteSaysUpdated -count=1 -v` |
| +Expected: FAIL on the `updated` assertion (output still says `added`) |
| + |
| +- [ ] **Step 3: Implement** |
| + |
| +In `cli.go` `runAdd`, after `r, err := registry.Open(*reg)` succeeds and before the `Upsert` call, look up the existing row; after the `Upsert`, report accordingly. Replace the tail: |
| + |
| +```go |
| + // An upsert over an existing host is a rewrite, not a creation, and the |
| + // verb the operator reads back must say which one happened (issue #69): |
| + // "added" printed over a rewrite is how vitogo's operator was left |
| + // believing a fresh row existed while a running edge kept the old one. |
| + existing, err := r.Get(host) |
| + if err != nil { |
| + log.Fatal(err) |
| + } |
| + if err := r.Upsert(registry.Route{ |
| + Host: host, Slug: *slug, Kind: *kind, Socket: *socket, Addr: *addr, |
| + App: *app, Account: account, |
| + Channel: *channel, Hibernate: *hibernate, StatusNamed: *statusNamed, Backing: *backing, |
| + DBPath: *dbPath, Status: "ready", |
| + }); err != nil { |
| + log.Fatal(err) |
| + } |
| + if existing != nil { |
| + fmt.Printf("updated %s (existing route)\n", host) |
| + } else { |
| + fmt.Printf("added %s\n", host) |
| + } |
| +``` |
| + |
| +(The comment above the Upsert about account stamping stays where it is.) |
| + |
| +- [ ] **Step 4: Run the test to verify it passes** |
| + |
| +Run: `go test . -run TestAddOnExistingRouteSaysUpdated -count=1 -v` |
| +Expected: PASS |
| + |
| +- [ ] **Step 5: Commit** |
| + |
| +```bash |
| +git add cli.go e2e_test.go |
| +git commit -m "cli: carlos add says updated when the route already existed (#69)" |
| +``` |
| + |
| +### Task 3: refuse a relative -db on agent-exec routes (#70) |
| + |
| +**Files:** |
| +- Modify: `cli.go` (`runAdd`, immediately after the `-backing unit -hibernate requires -db` refusal, ~line 128) |
| +- Test: `e2e_test.go` (new test function, same scaffold as Task 2) |
| + |
| +**Interfaces:** |
| +- Consumes: `filepath.IsAbs` (`path/filepath` is already imported in `cli.go` — verify), `registry.BackingUnit` (existing). |
| +- Produces: `carlos add` with `-backing ""` (exec child) and a relative non-empty `-db` exits non-zero with a message naming the cause (no `STATE_DIRECTORY` for exec children), the requirement (absolute path), and the convention (`provision.DefaultDBPath` result). `-backing unit` keeps accepting relative paths — systemd's `StateDirectory=carlos-app/%i` is what makes the rastrillo activation-contract convention real there. |
| + |
| +- [ ] **Step 1: Write the failing test** |
| + |
| +Add to `e2e_test.go`: |
| + |
| +```go |
| +// A relative -db on an agent-exec route must be refused at add time. The |
| +// activation contract's "relative -db resolves inside $STATE_DIRECTORY" |
| +// convention is real for unit tenants (systemd sets it) but agent-exec |
| +// children are spawned without it and the activator consumes the path |
| +// verbatim (Restore, Seed, Start --db, Replicate) — so a relative path |
| +// resolves against an unwritable cwd and every wake dies as an edge 503, |
| +// discoverable only by correlating 503s with child stderr in the journal |
| +// (issue #70, vitogo's first deploy). |
| +func TestAddRefusesRelativeDBOnExecRoutes(t *testing.T) { |
| + goBin, err := exec.LookPath("go") |
| + if err != nil { |
| + t.Skip("go toolchain not found") |
| + } |
| + tmp := t.TempDir() |
| + carlosBin := filepath.Join(tmp, "carlos") |
| + build := exec.Command(goBin, "build", "-o", carlosBin, ".") |
| + build.Env = os.Environ() |
| + if out, err := build.CombinedOutput(); err != nil { |
| + t.Fatalf("build carlos: %v\n%s", err, out) |
| + } |
| + regPath := filepath.Join(tmp, "registry.db") |
| + |
| + run := func(args ...string) (string, error) { |
| + cmd := exec.Command(carlosBin, args...) |
| + cmd.Env = carlosTestEnv(t) |
| + cmd.Dir = tmp |
| + out, err := cmd.CombinedOutput() |
| + return string(out), err |
| + } |
| + |
| + // The vitogo add, verbatim shape: exec backing (the default), relative -db. |
| + out, err := run("add", "-registry", regPath, "-app", "vitogo", |
| + "-kind", "instance", "-db", "vito.db", "-hibernate", |
| + "-channel", "canary/vitogo", "vito.example.com") |
| + if err == nil { |
| + t.Fatalf("add with relative -db on an exec route: want non-zero exit, got 0\n%s", out) |
| + } |
| + // The refusal must carry the diagnosis and the way out — not just "no". |
| + if !strings.Contains(out, "STATE_DIRECTORY") || !strings.Contains(out, "absolute") { |
| + t.Errorf("refusal = %q, want it to explain STATE_DIRECTORY and require an absolute path", out) |
| + } |
| + // And it must refuse BEFORE writing: no half-registered route. |
| + reg, err := registry.Open(regPath) |
| + if err != nil { |
| + t.Fatalf("open registry: %v", err) |
| + } |
| + defer reg.Close() |
| + rt, err := reg.Get("vito.example.com") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if rt != nil { |
| + t.Fatalf("refused add still wrote a route: %+v", rt) |
| + } |
| + |
| + // A unit tenant keeps the contract: StateDirectory= is real there, a |
| + // relative -db is the documented convention, and this guard must not |
| + // take it away. |
| + out, err = run("add", "-registry", regPath, "-app", "vitogo", |
| + "-kind", "instance", "-backing", "unit", "-db", "vito.db", |
| + "unit.example.com") |
| + if err != nil { |
| + t.Fatalf("add -backing unit with relative -db: %v\n%s", err, out) |
| + } |
| + |
| + // An absolute -db on an exec route is untouched by the guard. |
| + out, err = run("add", "-registry", regPath, "-app", "vitogo", |
| + "-kind", "instance", "-db", "/data/vito.example.com.db", |
| + "vito.example.com") |
| + if err != nil { |
| + t.Fatalf("add with absolute -db: %v\n%s", err, out) |
| + } |
| +} |
| +``` |
| + |
| +- [ ] **Step 2: Run the test to verify it fails** |
| + |
| +Run: `go test . -run TestAddRefusesRelativeDBOnExecRoutes -count=1 -v` |
| +Expected: FAIL at the first assertion (the add exits 0 today) |
| + |
| +- [ ] **Step 3: Implement the guard** |
| + |
| +In `cli.go` `runAdd`, directly after the `-backing unit -hibernate requires -db` refusal block (so it runs after the `-db` defaulting, before `resolveAccount`): |
| + |
| +```go |
| + // An agent-exec child gets no STATE_DIRECTORY — the activation |
| + // contract's "relative -db resolves inside $STATE_DIRECTORY" convention |
| + // is systemd's half, real only under -backing unit. On the exec path the |
| + // activator consumes the stored path verbatim (Restore, Seed, Start's |
| + // --db flag, the replicator), so a relative path resolves against a cwd |
| + // nobody chose and every wake dies as an edge 503 discoverable only in |
| + // the journal (issue #70). Refuse it here, at the only door that can |
| + // introduce one — every other mint path builds absolute paths. |
| + if *backing != registry.BackingUnit && *dbPath != "" && !filepath.IsAbs(*dbPath) { |
| + log.Fatalf("add %s: -db %q is relative, and agent-exec children get no STATE_DIRECTORY to resolve it in — pass an absolute path (the default here is %s), or use -backing unit, whose StateDirectory= makes relative paths real", |
| + host, *dbPath, provision.DefaultDBPath(dataDir(), host)) |
| + } |
| +``` |
| + |
| +Verify `path/filepath` and the `provision` package are in `cli.go`'s imports (both should be — `runAdd` already calls `filepath.Join` and `provision.DefaultDBPath`); add if missing. |
| + |
| +- [ ] **Step 4: Run the test to verify it passes** |
| + |
| +Run: `go test . -run TestAddRefusesRelativeDBOnExecRoutes -count=1 -v` |
| +Expected: PASS |
| + |
| +- [ ] **Step 5: Commit** |
| + |
| +```bash |
| +git add cli.go e2e_test.go |
| +git commit -m "cli: refuse a relative -db on agent-exec routes (#70)" |
| +``` |
| + |
| +### Task 4: full verification, Opus review, PR |
| + |
| +**Files:** |
| +- No new code; fixes from review land as amendments to the files above. |
| + |
| +- [ ] **Step 1: Full suite** |
| + |
| +Run (sandbox env vars, sandbox disabled if the toolchain complains): |
| +```bash |
| +go build ./... && go vet ./... && go test ./... -count=1 |
| +gofmt -l internal/registry/registry.go internal/registry/registry_test.go cli.go e2e_test.go |
| +``` |
| +Expected: build/vet clean, all packages pass, gofmt prints nothing. |
| + |
| +- [ ] **Step 2: Opus review (CLAUDE.md: production-touching work gets an Opus reviewer)** |
| + |
| +Dispatch a review subagent on **Opus** over `git diff main...HEAD`, briefed with both issue bodies and the two root-cause diagnoses (fingerprint blindness; exec-verbatim path consumption). Questions it must answer: does the fingerprint term break any pinned-value test; does the updated/added lookup introduce a TOCTOU worth caring about (two concurrent adds — acceptable: the message is advisory); does the relative-db guard block any legitimate existing workflow (unit tenants must keep relative). Apply fixes, re-run the full suite, amend commits or add a fix commit. |
| + |
| +- [ ] **Step 3: Push and open the PR** |
| + |
| +```bash |
| +git push -u origin worktree-issues-69-70-db-path |
| +gh pr create --repo carlosframework/platform --draft \ |
| + --title "registry+cli: db_path reaches running edges; relative -db refused on exec routes" \ |
| + --body "Fixes #69, fixes #70. ..." |
| +``` |
| + |
| +PR body must state both root-cause diagnoses (they differ from the issue titles' guesses), what changed, and the deferred follow-up: materializing a real per-host `STATE_DIRECTORY` for exec children (issue #70's suggested fix 1) is a design decision — a new on-disk convention with migration implications — left open on the issue, not taken overnight. |
| + |
| +Expected: draft PR open, linked to both issues. |
| + |
| +--- |
| + |
| +## Self-review notes |
| + |
| +- **#69 coverage:** the issue's observed failure (success message + stale behavior) is covered twice — fingerprint (behavioral root cause, Task 1) and updated/added honesty (message half, Task 2). The issue's literal guess ("stored db path silently kept its original value") is disproven by `Upsert`'s `db_path=excluded.db_path`; Task 2's test pins the row rewrite to guard it. |
| +- **#70 coverage:** suggested fix 2, chosen over 1/3 deliberately: resolving relative paths platform-side (fix 3) invents a per-host state-dir convention overnight and threads it through Restore/Seed/Start/Replicate **and** `ops.FleetDBs`' path matching (`filepath.Clean(r.DBPath)`) **and** `caps_pass` stamping — fleet-wide blast radius for a case the door can simply refuse. Fix 1 (real STATE_DIRECTORY) is the same convention decision; deferred explicitly on the issue. |
| +- **Sticky-column caveat:** `Upsert` deliberately keeps `environment/caps/tunnel/provisioned/seed` when incoming is zero; none of these is a `carlos add` flag, so "updates all supplied fields" is accurate as printed. |
| +- **Type consistency:** Tasks 2 and 3 both edit `runAdd`; Task 3's guard sits above Task 2's `existing` lookup and they don't interact. Both e2e tests use the local-build scaffold (each e2e test builds its own binary — the file's existing convention) plus `carlosTestEnv` + `cmd.Dir` per CLAUDE.md. |