carlos / platform Public

caps: fix Opus review findings on f17a64d — nil Notes, exact-key decode, v-range, dbs dupes, app-charset

Addresses every finding from the Opus review of the closed capability
vocabulary:

CRITICAL
- C1: reservedRuntimeDir extended with credentials, sudo, mount, network,
  initramfs, tmpfiles.d, host, machines, netns, blkid, motd.d,
  sendsigs.omit.d, console-setup, setrans, shm, dhcpcd, initctl (plus the
  existing entries and the systemd-* prefix). Comment added: the static
  list is the first guard, converge's existence check (Task 6) is the
  second — no static list will ever cover a box-specific /run/tailscale.
  Same note appended to the Task 6 section of
  docs/superpowers/plans/2026-08-03-app-capabilities.md.

IMPORTANT
- I1: (*Set)(nil).Notes() nil-guarded to return nil instead of panicking —
  every production manifest today is a nil Set.
- I2: Port and Data no longer decode via a whole-object json.Unmarshal,
  which let Go's case-insensitive tag matching silently apply fields like
  "PORT"/"REPLICATE" that the notes scan reported as ignored
  (config-that-lies). Both now decode field-by-field from the fields map
  via decodeIfPresent's exact-key lookup.
- I3: the "v" key must be present — {} and {"V":1} refuse with an honest
  "missing v" message, not "v 0". The accepted range is 1..Version (not
  only == Version), in both UnmarshalJSON and ValidCapabilities, so an
  immutable v1 manifest already in a production bucket keeps validating
  the day Version becomes 2 — rollback depends on it.
- I4: data.dbs refuses a repeated name, mirroring validPorts' seen map —
  two litestream entries on one WAL is the checkpoint fight §3a exists to
  prevent.
- I5: ValidCapabilities' host_role check refuses an app name failing the
  token charset before the length check, duplicating release.ValidApp's
  charset (named in the comment) as defense in depth for the User= sink.
- I6: added a round-trip test decoding the spec §3a keymail block in full,
  ValidCapabilities refusal tests for Set{V:0} and Set{V:2}, and a pin
  that a bare {"v":1} block is valid with Command "" meaning the
  template's own "serve".

MINOR
- UnmarshalJSON now decodes into a local Set and assigns to *s once, at
  the end, so a mid-decode error can never leave *s partially populated.
- The ports/dbs length bounds are checked before the make/allocate, not
  just at ValidCapabilities time, so they bound memory too.
- Boundary tests: port 1023 accepted; AppUser at exactly 32 chars
  accepted, 33 refused.
- Marker's doc comment now says "same SHAPE as carlos ops litestream's
  marker", not the same string.
- Data's doc comment states the "data": null asymmetry: absent means nil
  and valid, explicit null means a non-nil zero-value Data and refused —
  fail-closed on purpose. Backed by a test.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Paul Campbell 2 months ago b784c2bf12eadb5334ca36095576c70646318822 parent f17a64d

3 files changed, +412 −67

docs/superpowers/plans/2026-08-03-app-capabilities.md +2 −0
internal/caps/caps.go +155 −55
internal/caps/caps_test.go +255 −12

diff --git a/docs/superpowers/plans/2026-08-03-app-capabilities.md b/docs/superpowers/plans/2026-08-03-app-capabilities.md

index ceffc01b1..dacef3fa6 100644

--- a/docs/superpowers/plans/2026-08-03-app-capabilities.md

+++ b/docs/superpowers/plans/2026-08-03-app-capabilities.md

@@ -666,6 +666,8 @@ Converge's order of operations, pinned:

**What the reviewer should try to break:** a `Host` carrying a `/` or `..` (a registry row is trusted at the same level as `live/<host>`, but this one joins into a **root-written** `/etc/systemd/system` path — refuse anything where `rt.Host != filepath.Base(rt.Host)`, exactly as `appconfig.syncHost` does at :293). A drop-in directory that exists but is a file. A `Systemctl` that fails mid-sequence in step 7 (the service must not be left stopped — assert the start is attempted even when the socket restart failed, and that the error is still returned). `EnsureUser` succeeding while the user genuinely does not exist. Whether `Changed` can be true when only a *removal* happened. Whether two converges racing (hourly `RunAdopt` and a version-change converge inside `Pass`) can interleave — they are both in the same process on the same box, but `carlos adopt` run by hand while `carlos-adopt.timer` fires is two processes: state whether the atomic rename makes that safe (it does for the file; it does not for the systemctl sequence — say so and accept it, as the repo already accepts it for `appconfig.Sync`).

+**Note (added post-Task-1-review, 2026-08-03):** `internal/caps`'s `reservedRuntimeDir` static list (extended in the Task 1 review) is the *first* guard against `runtime_dir` naming something that matters — but it is necessarily finite, and a box can have anything under `/run` that no static list will ever enumerate (a box-specific `/run/tailscale`, an operator's own daemon, etc.). Converge must ADDITIONALLY refuse a `runtime_dir` grant when `/run/<name>` already exists on the box and is **not** this route's own prior claim (i.e. not something this same host's own drop-in created) — an `os.Lstat` check alongside the existing box-wide `CheckClaims` collision pass, before anything is written. The static list is the first guard; this existence check is the second, and only the second one can catch a name the vocabulary's authors never thought of.

---

### Task 7: converge inside `adoptHost` — before `PointLive`, rolled back with it

diff --git a/internal/caps/caps.go b/internal/caps/caps.go

index f6a00c05a..ccc3a18f2 100644

--- a/internal/caps/caps.go

+++ b/internal/caps/caps.go

@@ -20,14 +20,20 @@ import (

)

// Version is the capability block version THIS binary understands. A block

-// declaring a higher "v" refuses the version on this box (spec §3b) — fail

-// closed in the direction that changes nothing: the box keeps serving what

-// it has rather than materialize a unit missing something the app needs.

+// declaring a "v" outside 1..Version refuses the version on this box (spec

+// §3b) — fail closed in the direction that changes nothing: the box keeps

+// serving what it has rather than materialize a unit missing something the

+// app needs. Accepting the whole 1..Version range (not just == Version) is

+// what lets an immutable v1 manifest already sitting in a production bucket

+// keep validating the day Version becomes 2 — a rollback depends on it.

const Version = 1

// Marker is the first line of every drop-in this package generates — the

-// same marker `carlos ops litestream` uses. A file at a generated path

-// WITHOUT it is never overwritten and never deleted (spec §3c).

+// same SHAPE as the marker `carlos ops litestream` uses (not the same

+// string): a "# GENERATED by carlos ... — do not hand-edit" line, so a

+// hand-edit check can look for that shape without two packages having to

+// agree on one hardcoded literal. A file at a generated path WITHOUT it is

+// never overwritten and never deleted (spec §3c).

const Marker = "# GENERATED by carlos adopt — do not hand-edit"

// DataReplicatePlatform and DataReplicateApp are the only two values

@@ -36,15 +42,27 @@ const Marker = "# GENERATED by carlos adopt — do not hand-edit"

// platform provisions the directory and replicates nothing. Two

// replicators on one WAL fight over checkpointing and corrupt the replica,

// so the two are mutually exclusive — see the "dbs with replicate:\"app\""

-// rule in ValidCapabilities.

+// rule in validData.

const (

DataReplicatePlatform = "platform"

DataReplicateApp = "app"

)

+// maxPorts and maxDBs are bounds, not features (spec §7) — shared between

+// decode time, where they bound the memory a hostile manifest can force

+// this binary to allocate, and ValidCapabilities, the semantic gate a

+// programmatically-built Set (never having passed through UnmarshalJSON)

+// still has to clear.

+const (

+ maxPorts = 4

+ maxDBs = 8

// tokenRe is the charset shared by command, runtime_dir, and ports[].name

// (spec §7): one lowercase token, so "../etc", "carlos/apps", absolute

-// paths, flags, and spaces never parse.

+// paths, flags, and spaces never parse. It is also, byte for byte,

+// release.ValidApp's own app-name charset — see ValidCapabilities's

+// host_role check for why that duplication exists.

var tokenRe = regexp.MustCompile(`^[a-z][a-z0-9-]{0,31}$`)

// dbNameRe is data.dbs's charset: a bare filename, used only as

@@ -55,16 +73,25 @@ var dbNameRe = regexp.MustCompile(`^[A-Za-z0-9._-]{1,64}$`)

// reservedRuntimeDir names a directory the platform or the OS already owns.

// "carlos" is the load-bearing entry: a tenant owning /run/carlos would own

// every tenant socket the edge dials — the single most valuable directory

-// on the box (spec §7). The "systemd-" prefix is checked separately.

+// on the box (spec §7). The rest are /run subdirectories systemd, the

+// kernel, or a common distro tool already owns; the "systemd-" prefix is

+// checked separately, below.

+//

+// static list is the first guard; converge's existence check (Task 6) is

+// the second. This list can never be exhaustive — a box-specific

+// /run/tailscale is not on it and never will be — so the converge pass

+// additionally refuses a runtime_dir grant when /run/<name> already exists

+// on the box and is not this route's own prior claim. See the note

+// appended to the Task 6 section of

+// docs/superpowers/plans/2026-08-03-app-capabilities.md.

var reservedRuntimeDir = map[string]bool{

- "carlos": true,

- "systemd": true,

- "lock": true,

- "log": true,

- "user": true,

- "dbus": true,

- "sshd": true,

- "udev": true,

+ "carlos": true, "systemd": true, "lock": true, "log": true,

+ "user": true, "dbus": true, "sshd": true, "udev": true,

+ "credentials": true, "sudo": true, "mount": true, "network": true,

+ "initramfs": true, "tmpfiles.d": true, "host": true, "machines": true,

+ "netns": true, "blkid": true, "motd.d": true, "sendsigs.omit.d": true,

+ "console-setup": true, "setrans": true, "shm": true, "dhcpcd": true,

+ "initctl": true,

}

// reservedPort is bound absolutely and is never grantable, typo or not

@@ -112,6 +139,15 @@ type Port struct {

// Data is the persistent-data-dir ask: which files under the state dir are

// databases, and who replicates them. The path itself is never in the

// manifest — it is always StateDirectory=carlos-app/%i (spec §2).

+//

+// Asymmetry, on purpose: a manifest with no "data" key at all decodes to a

+// nil *Data on Set — no capability declared, always valid. A manifest that

+// instead writes an explicit "data": null is NOT the same thing:

+// UnmarshalJSON still allocates a non-nil *Data (the zero value, an empty

+// Replicate), which ValidCapabilities then refuses for naming neither

+// accepted replicate value. Fail-closed on purpose: an explicit-but-empty

+// "data" is not silently waved through as "no capability" just because an

+// absent key would have been.

type Data struct {

Replicate string `json:"replicate"` // DataReplicatePlatform | DataReplicateApp

DBs []string `json:"dbs,omitempty"`

@@ -119,7 +155,7 @@ type Data struct {

// UnmarshalJSON is the strict door (spec §3b), and the three outcomes are

// deliberately different:

-// - an unknown CAPABILITY NAME, or a "v" greater than Version: error,

+// - an unknown CAPABILITY NAME, or a "v" outside 1..Version: error,

// naming the capability and "update the platform binary". This

// propagates through json.Unmarshal -> release.GetManifest ->

// ensureStaged, which is exactly how the version comes to be refused on

@@ -131,28 +167,44 @@ type Data struct {

// It decodes into a map[string]json.RawMessage first so it can check every

// key against the seven known names plus "v" before decoding anything

// typed, then re-decodes each object-valued capability with a second

-// map[string]json.RawMessage pass to collect notes before decoding into the

-// typed value. json.Decoder.DisallowUnknownFields is deliberately NOT used

-// on the whole block — it cannot distinguish an unknown name (refuse) from

-// an unknown field (ignore), and that distinction is the entire

-// forward-compatibility contract (spec §3b).

+// map[string]json.RawMessage pass to collect notes AND to decode each known

+// field one at a time by its exact key. That second point matters: a

+// whole-object json.Unmarshal into Port or Data would let Go's decoder

+// case-fold an unrecognised "PORT" or "REPLICATE" onto the typed field via

+// its default case-insensitive tag matching — silently applying exactly the

+// value the notes scan just reported as ignored (config-that-lies). Every

+// field of every object-valued capability is therefore decoded via

+// decodeIfPresent against the fields map's exact string key, never via a

+// whole-object unmarshal.

+//

+// json.Decoder.DisallowUnknownFields is deliberately NOT used on the whole

+// block — it cannot distinguish an unknown name (refuse) from an unknown

+// field (ignore), and that distinction is the entire forward-compatibility

+// contract (spec §3b).

+//

+// Decoding builds into a local Set and assigns to *s only once, at the very

+// end: a mid-decode error can then never leave *s partially populated, by

+// construction rather than by careful statement ordering.

func (s *Set) UnmarshalJSON(b []byte) error {

var raw map[string]json.RawMessage

if err := json.Unmarshal(b, &raw); err != nil {

return err

}

- // "v" absent decodes as the zero value, which is never Version (today

- // Version is 1) — an omitted "v" on a present block is refused exactly

- // like an explicit mismatch, per the validation table.

+ // "v" must be present as its own key. An absent "v" on a present block

+ // is refused with an honest message naming the missing key — never

+ // treated as "v 0", which would conflate "the block forgot to say" with

+ // "the block explicitly said an invalid version".

+ vRaw, ok := raw["v"]

+ if !ok {

+ return fmt.Errorf(`capabilities: missing "v" (capability block version) — refusing rather than guessing which one was meant`)

+ }

var v int

- if vRaw, ok := raw["v"]; ok {

- if err := json.Unmarshal(vRaw, &v); err != nil {

- return fmt.Errorf("capabilities: v: %w", err)

- }

+ if err := json.Unmarshal(vRaw, &v); err != nil {

+ return fmt.Errorf("capabilities: v: %w", err)

}

- if v != Version {

- return fmt.Errorf("capabilities: v %d is not supported by this binary (this binary speaks v%d) — update the platform binary", v, Version)

+ if v < 1 || v > Version {

+ return fmt.Errorf("capabilities: v %d is not supported by this binary (this binary speaks v1..v%d) — update the platform binary", v, Version)

}

for name := range raw {

@@ -164,21 +216,23 @@ func (s *Set) UnmarshalJSON(b []byte) error {

}

+ var out Set

+ out.V = v

var notes []string

- if err := decodeIfPresent(raw, "command", &s.Command); err != nil {

+ if err := decodeIfPresent(raw, "command", &out.Command); err != nil {

return fmt.Errorf("capabilities: command: %w", err)

}

- if err := decodeIfPresent(raw, "daemon", &s.Daemon); err != nil {

+ if err := decodeIfPresent(raw, "daemon", &out.Daemon); err != nil {

return fmt.Errorf("capabilities: daemon: %w", err)

}

- if err := decodeIfPresent(raw, "children", &s.Children); err != nil {

+ if err := decodeIfPresent(raw, "children", &out.Children); err != nil {

return fmt.Errorf("capabilities: children: %w", err)

}

- if err := decodeIfPresent(raw, "runtime_dir", &s.RuntimeDir); err != nil {

+ if err := decodeIfPresent(raw, "runtime_dir", &out.RuntimeDir); err != nil {

return fmt.Errorf("capabilities: runtime_dir: %w", err)

}

- if err := decodeIfPresent(raw, "host_role", &s.HostRole); err != nil {

+ if err := decodeIfPresent(raw, "host_role", &out.HostRole); err != nil {

return fmt.Errorf("capabilities: host_role: %w", err)

}

@@ -187,6 +241,12 @@ func (s *Set) UnmarshalJSON(b []byte) error {

if err := json.Unmarshal(portsRaw, &items); err != nil {

return fmt.Errorf("capabilities: ports: %w", err)

}

+ // Bound BEFORE the make below, so the length a hostile manifest

+ // claims bounds the memory this binary allocates for it, not just

+ // the eventual result ValidCapabilities would reject.

+ if len(items) > maxPorts {

+ return fmt.Errorf("capabilities: ports: %d declared, want at most %d", len(items), maxPorts)

+ }

ports := make([]Port, len(items))

for i, item := range items {

var fields map[string]json.RawMessage

@@ -198,11 +258,14 @@ func (s *Set) UnmarshalJSON(b []byte) error {

notes = append(notes, fmt.Sprintf("capabilities: ports[%d]: unknown field %q ignored", i, f))

}

- if err := json.Unmarshal(item, &ports[i]); err != nil {

- return fmt.Errorf("capabilities: ports[%d]: %w", i, err)

+ if err := decodeIfPresent(fields, "port", &ports[i].Port); err != nil {

+ return fmt.Errorf("capabilities: ports[%d].port: %w", i, err)

+ }

+ if err := decodeIfPresent(fields, "name", &ports[i].Name); err != nil {

+ return fmt.Errorf("capabilities: ports[%d].name: %w", i, err)

}

- s.Ports = ports

+ out.Ports = ports

}

if dataRaw, ok := raw["data"]; ok {

@@ -216,20 +279,36 @@ func (s *Set) UnmarshalJSON(b []byte) error {

}

var d Data

- if err := json.Unmarshal(dataRaw, &d); err != nil {

- return fmt.Errorf("capabilities: data: %w", err)

+ if err := decodeIfPresent(fields, "replicate", &d.Replicate); err != nil {

+ return fmt.Errorf("capabilities: data.replicate: %w", err)

+ }

+ if dbsRaw, ok := fields["dbs"]; ok {

+ var dbs []string

+ if err := json.Unmarshal(dbsRaw, &dbs); err != nil {

+ return fmt.Errorf("capabilities: data.dbs: %w", err)

+ }

+ // Same bound-before-allocation discipline as ports above.

+ if len(dbs) > maxDBs {

+ return fmt.Errorf("capabilities: data.dbs: %d entries, want at most %d", len(dbs), maxDBs)

+ }

+ d.DBs = dbs

}

- s.Data = &d

+ out.Data = &d

}

sort.Strings(notes)

- s.V = v

- s.notes = notes

+ out.notes = notes

+ *s = out

return nil

}

// decodeIfPresent unmarshals raw[key] into out when key is present, and

// leaves out untouched (the zero value, meaning "not declared") otherwise.

+// Because the lookup is an exact Go map-key comparison rather than a

+// whole-object json.Unmarshal, it never falls back to encoding/json's

+// case-insensitive struct-tag matching — a "PORT" or "REPLICATE" the caller

+// has already flagged as an unknown field via the same fields map stays

+// unapplied.

func decodeIfPresent(raw map[string]json.RawMessage, key string, out any) error {

if v, ok := raw[key]; ok {

return json.Unmarshal(v, out)

@@ -238,8 +317,14 @@ func decodeIfPresent(raw map[string]json.RawMessage, key string, out any) error

}

// Notes are the ignore-with-a-log lines the decoder collected, sorted.

-// Callers print them once per pass, never per host.

+// Callers print them once per pass, never per host. A nil Set — every

+// production manifest before this feature, and what RunAdopt sees on every

+// route that never declared capabilities — returns nil rather than

+// panicking.

func (s *Set) Notes() []string {

+ if s == nil {

+ return nil

+ }

return s.notes

}

@@ -247,13 +332,13 @@ func (s *Set) Notes() []string {

// present to read the refusal) and ADOPT (the box refuses the version),

// exactly as appconfig.ValidExecDelivery is shared by set-time and

// sync-time. A nil Set is valid and means no capabilities. app is needed

-// for the host_role user-name length check and nothing else.

+// for the host_role user-name charset and length checks and nothing else.

func ValidCapabilities(app string, s *Set) error {

if s == nil {

return nil

}

- if s.V != Version {

- return fmt.Errorf("capabilities: v %d is not supported by this binary (this binary speaks v%d) — update the platform binary", s.V, Version)

+ if s.V < 1 || s.V > Version {

+ return fmt.Errorf("capabilities: v %d is not supported by this binary (this binary speaks v1..v%d) — update the platform binary", s.V, Version)

}

if s.Command != "" && !tokenRe.MatchString(s.Command) {

return fmt.Errorf("capabilities: command %q must be one lowercase token matching ^[a-z][a-z0-9-]{0,31}$ — one token, no flags, no spaces", s.Command)

@@ -275,6 +360,16 @@ func ValidCapabilities(app string, s *Set) error {

}

if s.HostRole {

+ // release.ValidApp (internal/release/release.go) already refuses an

+ // app name outside this exact charset at every route-creation entry

+ // point. It is duplicated here — rather than imported, since

+ // release comes to depend on this package in Task 2 and importing

+ // it back would cycle — as defense in depth for the

+ // User=<AppUser(app)> sink: a Set can be validated directly,

+ // without ever having passed through a route.

+ if !tokenRe.MatchString(app) {

+ return fmt.Errorf("capabilities: host_role: app %q fails the app-name charset — see release.ValidApp", app)

+ }

if u := AppUser(app); len(u) > 32 {

return fmt.Errorf("capabilities: host_role: user %q is longer than the 32-character account-name limit — refused at ship, where a human can rename", u)

}

@@ -284,10 +379,10 @@ func ValidCapabilities(app string, s *Set) error {

// validPorts encodes the ports rules: only privileged ports are a

// capability, 22/80/443 are reserved absolutely, no duplicates, and a bound

-// of 4 — a bound, not a feature.

+// of maxPorts — a bound, not a feature.

func validPorts(ports []Port) error {

- if len(ports) > 4 {

- return fmt.Errorf("capabilities: ports: %d declared, want at most 4", len(ports))

+ if len(ports) > maxPorts {

+ return fmt.Errorf("capabilities: ports: %d declared, want at most %d", len(ports), maxPorts)

}

seen := make(map[int]bool, len(ports))

for _, p := range ports {

@@ -309,19 +404,24 @@ func validPorts(ports []Port) error {

}

// validData encodes the data rules: replicate must be one of the two

-// constants, dbs is a bound list of bare filenames, and dbs is meaningless

-// (and refused) when the app replicates its own WAL.

+// constants, dbs is a bound list of unique bare filenames, and dbs is

+// meaningless (and refused) when the app replicates its own WAL.

func validData(d *Data) error {

if d.Replicate != DataReplicatePlatform && d.Replicate != DataReplicateApp {

return fmt.Errorf("capabilities: data.replicate %q must be %q or %q", d.Replicate, DataReplicatePlatform, DataReplicateApp)

}

- if len(d.DBs) > 8 {

- return fmt.Errorf("capabilities: data.dbs: %d entries, want at most 8", len(d.DBs))

+ if len(d.DBs) > maxDBs {

+ return fmt.Errorf("capabilities: data.dbs: %d entries, want at most %d", len(d.DBs), maxDBs)

}

if d.Replicate == DataReplicateApp && len(d.DBs) > 0 {

return fmt.Errorf("capabilities: data.dbs is meaningless when replicate is %q — the app streams its own WAL, and accepting it would invite the two-replicators hazard", DataReplicateApp)

}

+ seen := make(map[string]bool, len(d.DBs))

for _, name := range d.DBs {

+ if seen[name] {

+ return fmt.Errorf("capabilities: data.dbs: %q declared more than once — two replicators on one WAL fight over checkpointing", name)

+ }

+ seen[name] = true

if name == "." || name == ".." || !dbNameRe.MatchString(name) {

return fmt.Errorf("capabilities: data.dbs: %q must be a bare filename matching ^[A-Za-z0-9._-]{1,64}$", name)

}

diff --git a/internal/caps/caps_test.go b/internal/caps/caps_test.go

index 0d09f1252..df9a86ed7 100644

--- a/internal/caps/caps_test.go

+++ b/internal/caps/caps_test.go

@@ -2,6 +2,7 @@ package caps

import (

"encoding/json"

+ "fmt"

"strings"

"testing"

)

@@ -45,11 +46,197 @@ func TestVersionBumpIsRefused(t *testing.T) {

}

+// TestVMissingKeyIsRefusedHonestly pins review finding I3: an absent "v"

+// key — whether the object is entirely empty or merely misspells the key

+// ("V" instead of "v") — is refused with a message naming the missing key,

+// never the misleading "v 0" (which would suggest the block explicitly

+// asked for a version number that happens to be zero).

+func TestVMissingKeyIsRefusedHonestly(t *testing.T) {

+ for _, raw := range []string{`{}`, `{"V":1}`} {

+ var s Set

+ err := json.Unmarshal([]byte(raw), &s)

+ if err == nil {

+ t.Errorf("%s: accepted, want a refusal", raw)

+ continue

+ }

+ if !strings.Contains(err.Error(), "missing") {

+ t.Errorf("%s: err = %v, want an honest \"missing v\" message", raw, err)

+ }

+ if strings.Contains(err.Error(), "v 0") {

+ t.Errorf("%s: err = %v, must not say \"v 0\" — the key was never there to say 0", raw, err)

+ }

+// TestVRangeAcceptedUpToVersionRefusedOutsideIt pins review finding I3's

+// range, generically against the Version constant rather than a hardcoded

+// number: every v in 1..Version decodes, and anything outside it (0,

+// negative, or Version+1) is refused. The day Version becomes 2, an

+// immutable v1 manifest already in a production bucket must still decode —

+// rollback depends on it.

+func TestVRangeAcceptedUpToVersionRefusedOutsideIt(t *testing.T) {

+ for v := 1; v <= Version; v++ {

+ var s Set

+ if err := json.Unmarshal([]byte(fmt.Sprintf(`{"v":%d}`, v)), &s); err != nil {

+ t.Errorf("v:%d (within 1..Version=%d) refused: %v", v, Version, err)

+ }

+ for _, v := range []int{0, -1, Version + 1} {

+ var s Set

+ if err := json.Unmarshal([]byte(fmt.Sprintf(`{"v":%d}`, v)), &s); err == nil {

+ t.Errorf("v:%d (outside 1..Version=%d) accepted", v, Version)

+ }

+// TestValidCapabilitiesRefusesVOutsideRange pins review finding I6(b): the

+// same range check inside ValidCapabilities, exercised against Set literals

+// that never passed through UnmarshalJSON at all.

+func TestValidCapabilitiesRefusesVOutsideRange(t *testing.T) {

+ if err := ValidCapabilities("keymail", &Set{V: 0}); err == nil {

+ t.Fatal("Set{V:0} accepted by ValidCapabilities")

+ }

+ if err := ValidCapabilities("keymail", &Set{V: 2}); err == nil {

+ t.Fatal("Set{V:2} accepted by ValidCapabilities (Version is 1)")

+ }

+// TestBareVBlockIsValidAndCommandEmptyMeansServe pins review finding I6:

+// {"v":1} alone (every other field absent) decodes and validates cleanly,

+// and an empty Command means the template's own "serve" — not an invalid

+// or a refused value.

+func TestBareVBlockIsValidAndCommandEmptyMeansServe(t *testing.T) {

+ var s Set

+ if err := json.Unmarshal([]byte(`{"v":1}`), &s); err != nil {

+ t.Fatalf(`bare {"v":1} refused: %v`, err)

+ }

+ if s.Command != "" {

+ t.Fatalf(`Command = %q, want "" (the template's own "serve")`, s.Command)

+ }

+ if err := ValidCapabilities("console", &s); err != nil {

+ t.Fatalf("bare v:1 block refused by ValidCapabilities: %v", err)

+ }

+// TestKeymailSpecBlockRoundTripsAndValidates pins review finding I6(a):

+// decode the exact keymail capabilities block from

+// docs/superpowers/specs/2026-08-03-app-capabilities-design.md §3a, assert

+// every field, and confirm ValidCapabilities accepts it whole.

+func TestKeymailSpecBlockRoundTripsAndValidates(t *testing.T) {

+ const keymailBlock = `{

+ "v": 1,

+ "command": "relay",

+ "daemon": true,

+ "children": true,

+ "runtime_dir": "keymail",

+ "ports": [ { "port": 25, "name": "mx" } ],

+ "data": { "replicate": "app" },

+ "host_role": true

+ }`

+ var s Set

+ if err := json.Unmarshal([]byte(keymailBlock), &s); err != nil {

+ t.Fatalf("spec §3a keymail block failed to decode: %v", err)

+ }

+ if s.V != 1 {

+ t.Errorf("V = %d, want 1", s.V)

+ }

+ if s.Command != "relay" {

+ t.Errorf("Command = %q, want relay", s.Command)

+ }

+ if !s.Daemon {

+ t.Error("Daemon = false, want true")

+ }

+ if !s.Children {

+ t.Error("Children = false, want true")

+ }

+ if s.RuntimeDir != "keymail" {

+ t.Errorf("RuntimeDir = %q, want keymail", s.RuntimeDir)

+ }

+ if len(s.Ports) != 1 || s.Ports[0].Port != 25 || s.Ports[0].Name != "mx" {

+ t.Errorf("Ports = %+v, want [{25 mx}]", s.Ports)

+ }

+ if s.Data == nil || s.Data.Replicate != DataReplicateApp || len(s.Data.DBs) != 0 {

+ t.Errorf("Data = %+v, want {app []}", s.Data)

+ }

+ if !s.HostRole {

+ t.Error("HostRole = false, want true")

+ }

+ if n := s.Notes(); len(n) != 0 {

+ t.Errorf("Notes = %v, want none", n)

+ }

+ if err := ValidCapabilities("keymail", &s); err != nil {

+ t.Fatalf("ValidCapabilities(keymail, spec block) = %v, want nil", err)

+ }

+// TestNilSetNotesDoesNotPanic pins review finding I1: every production

+// manifest today decodes to a nil Set, and the first ranged Notes() call in

+// Task 6's converge pass must not crash RunAdopt fleet-wide.

+func TestNilSetNotesDoesNotPanic(t *testing.T) {

+ var s *Set

+ if n := s.Notes(); n != nil {

+ t.Fatalf("nil Set.Notes() = %v, want nil", n)

+ }

+// TestPortFieldCaseInsensitiveMatchIsNotApplied pins review finding I2: a

+// whole-object json.Unmarshal into Port would let Go's decoder case-fold an

+// unrecognised "PORT" onto the Port field via its default case-insensitive

+// tag matching, applying exactly the value the notes scan reports as

+// ignored. Decoding field-by-field from the exact-key fields map must

+// refuse to do that: "PORT" stays ignored and noted, and Port stays 0.

+func TestPortFieldCaseInsensitiveMatchIsNotApplied(t *testing.T) {

+ var s Set

+ if err := json.Unmarshal([]byte(`{"v":1,"ports":[{"PORT":25}]}`), &s); err != nil {

+ t.Fatalf("an unrecognised-cased field must not fail decode: %v", err)

+ }

+ if len(s.Ports) != 1 || s.Ports[0].Port != 0 || s.Ports[0].Name != "" {

+ t.Fatalf(`Ports = %+v, want [{0 ""}] — "PORT" must be ignored, not case-folded onto Port`, s.Ports)

+ }

+ if n := s.Notes(); len(n) != 1 || !strings.Contains(n[0], "PORT") {

+ t.Fatalf("notes = %v, want one line naming PORT", n)

+ }

+// TestDataFieldCaseInsensitiveMatchIsNotApplied is TestPortFieldCase...'s

+// sibling for Data: "REPLICATE"/"DBS" must stay ignored-and-noted rather

+// than being case-folded onto Replicate/DBs (review finding I2).

+func TestDataFieldCaseInsensitiveMatchIsNotApplied(t *testing.T) {

+ var s Set

+ if err := json.Unmarshal([]byte(`{"v":1,"data":{"REPLICATE":"platform","DBS":["x.db"]}}`), &s); err != nil {

+ t.Fatalf("unrecognised-cased fields must not fail decode: %v", err)

+ }

+ if s.Data == nil || s.Data.Replicate != "" || len(s.Data.DBs) != 0 {

+ t.Fatalf("Data = %+v, want the zero value — REPLICATE/DBS must be ignored, not case-folded", s.Data)

+ }

+ if n := s.Notes(); len(n) != 2 {

+ t.Fatalf("notes = %v, want two lines naming REPLICATE and DBS", n)

+ }

+// TestDataExplicitNullDecodesButIsRefused pins the Data doc comment's

+// asymmetry (review MINOR item): an explicit "data": null still allocates a

+// non-nil *Data (the zero value), which ValidCapabilities then refuses for

+// naming neither accepted replicate value — unlike an absent "data" key,

+// which leaves Data nil and valid.

+func TestDataExplicitNullDecodesButIsRefused(t *testing.T) {

+ var s Set

+ if err := json.Unmarshal([]byte(`{"v":1,"data":null}`), &s); err != nil {

+ t.Fatalf(`{"v":1,"data":null} failed to decode: %v`, err)

+ }

+ if s.Data == nil {

+ t.Fatal(`explicit "data": null must still decode to a non-nil *Data (the zero value)`)

+ }

+ if err := ValidCapabilities("console", &s); err == nil {

+ t.Fatal("a zero-value Data (from explicit \"data\": null) must be refused for naming no replicate value")

+ }

// TestCapabilitiesEmptyObjectWithNoVIsRefused pins the reviewer's sharpest

// edge case: "capabilities": {} must NOT be read as "no capabilities" — an

-// absent "v" on a PRESENT block decodes as the zero value, which is never

-// Version, so it is refused exactly like an explicit mismatch. Only an

-// absent capabilities KEY (a nil *Set, never constructed) means "none".

+// absent "v" on a PRESENT block is refused (now with an honest "missing v"

+// message; see TestVMissingKeyIsRefusedHonestly). Only an absent

+// capabilities KEY (a nil *Set, never constructed) means "none".

func TestCapabilitiesEmptyObjectWithNoVIsRefused(t *testing.T) {

var s Set

if err := json.Unmarshal([]byte(`{}`), &s); err == nil {

@@ -58,17 +245,28 @@ func TestCapabilitiesEmptyObjectWithNoVIsRefused(t *testing.T) {

}

// TestRuntimeDirCharsetAndReservedNames pins the charset and the reserved

-// list. NOTE: the plan's Step-1 listing included "" in this bad list, but

-// that contradicts both TestNilSetIsValidAndMeansNothing and

-// TestPortsReservedAndNonPrivilegedRefused's final assertion (a Set with

-// only Ports set, RuntimeDir at its zero value "", must be valid) — an

-// empty RuntimeDir means "not declared", not "declared as empty". "" is

-// dropped here and pinned separately as valid below.

+// list, including the names added on review (finding C1). NOTE: the plan's

+// Step-1 listing included "" in this bad list, but that contradicts both

+// TestNilSetIsValidAndMeansNothing and TestPortsReservedAndNonPrivilegedRefused's

+// final assertion (a Set with only Ports set, RuntimeDir at its zero value

+// "", must be valid) — an empty RuntimeDir means "not declared", not

+// "declared as empty". "" is dropped here and pinned separately as valid

+// below.

func TestRuntimeDirCharsetAndReservedNames(t *testing.T) {

- for _, bad := range []string{"../etc", "carlos/apps", "/run/keymail", "Keymail", "carlos", "systemd", "systemd-foo", "lock", "log", "user", "dbus", "sshd", "udev", strings.Repeat("a", 33)} {

- s := &Set{V: 1, RuntimeDir: bad}

+ bad := []string{

+ "../etc", "carlos/apps", "/run/keymail", "Keymail",

+ "carlos", "systemd", "systemd-foo", "lock", "log", "user",

+ "dbus", "sshd", "udev", strings.Repeat("a", 33),

+ // added on review (C1): common OS/tool-owned /run subdirectories,

+ // none of which is a substitute for converge's box-wide existence

+ // check (see the note on reservedRuntimeDir and the Task 6 plan

+ // addendum) — this list can never be exhaustive.

+ "credentials", "sudo", "mount", "network", "host", "netns", "shm",

+ }

+ for _, name := range bad {

+ s := &Set{V: 1, RuntimeDir: name}

if err := ValidCapabilities("keymail", s); err == nil {

- t.Errorf("runtime_dir %q was accepted", bad)

+ t.Errorf("runtime_dir %q was accepted", name)

}

if err := ValidCapabilities("keymail", &Set{V: 1, RuntimeDir: "keymail"}); err != nil {

@@ -98,6 +296,15 @@ func TestPortsReservedAndNonPrivilegedRefused(t *testing.T) {

}

+// TestPort1023IsAccepted pins the upper boundary of the privileged range

+// (review MINOR item): 1023 is the last privileged port and must be

+// accepted, not off-by-one refused.

+func TestPort1023IsAccepted(t *testing.T) {

+ if err := ValidCapabilities("keymail", &Set{V: 1, Ports: []Port{{Port: 1023}}}); err != nil {

+ t.Fatalf("port 1023 (upper bound) refused: %v", err)

+ }

// TestDuplicatePortIsRefused pins the plan's "Plus:" line item: the same

// port twice in one ports list is refused rather than silently deduplicated.

func TestDuplicatePortIsRefused(t *testing.T) {

@@ -135,6 +342,31 @@ func TestHostRoleRefusesAnAppNameTooLongForAUser(t *testing.T) {

}

+// TestAppUserLengthBoundary pins the exact edge (review MINOR item):

+// carlos-<app> at exactly 32 characters is accepted, 33 is refused.

+func TestAppUserLengthBoundary(t *testing.T) {

+ at32 := strings.Repeat("a", 25) // "carlos-" (7) + 25 = 32

+ if err := ValidCapabilities(at32, &Set{V: 1, HostRole: true}); err != nil {

+ t.Fatalf("app user at exactly 32 chars refused: %v", err)

+ }

+ at33 := strings.Repeat("a", 26) // "carlos-" (7) + 26 = 33

+ if err := ValidCapabilities(at33, &Set{V: 1, HostRole: true}); err == nil {

+ t.Fatal("app user at 33 chars (one over the limit) was accepted")

+ }

+// TestHostRoleRefusesAppNameCharsetBeforeLength pins review finding I5: an

+// app name failing the token charset is refused even when it would pass

+// the length check — release.ValidApp's charset is checked first, as

+// defense in depth for the User=<AppUser(app)> sink.

+func TestHostRoleRefusesAppNameCharsetBeforeLength(t *testing.T) {

+ for _, bad := range []string{"Keymail", "1relay", "key_mail", "key mail"} {

+ if err := ValidCapabilities(bad, &Set{V: 1, HostRole: true}); err == nil {

+ t.Errorf("host_role with app name %q (fails the charset) was accepted", bad)

+ }

// TestNilSetIsValidAndMeansNothing pins today's every-manifest behaviour:

// no capabilities key parses to a nil *Set, and a nil *Set is valid.

func TestNilSetIsValidAndMeansNothing(t *testing.T) {

@@ -171,6 +403,17 @@ func TestDataDBsCharsetAndSeparatorRefusals(t *testing.T) {

}

+// TestDataDBsDuplicateIsRefused pins review finding I4: a repeated dbs name

+// is refused, mirroring validPorts' duplicate check — two litestream

+// entries pointed at one WAL is the checkpoint fight §3a exists to

+// prevent.

+func TestDataDBsDuplicateIsRefused(t *testing.T) {

+ s := &Set{V: 1, Data: &Data{Replicate: DataReplicatePlatform, DBs: []string{"console.db", "console.db"}}}

+ if err := ValidCapabilities("console", s); err == nil {

+ t.Fatal("duplicate dbs entry \"console.db\" was accepted")

+ }

// TestTooManyDBsIsRefused pins the len(data.dbs) > 8 bound.

func TestTooManyDBsIsRefused(t *testing.T) {

dbs := make([]string, 9)