| 1 | # Review brief: isolated exec design |
| 2 | |
| 3 | You are GPT-6 reviewing a design spec independently of its author. Your |
| 4 | output is a findings report, not an edit. Do not modify any file. |
| 5 | |
| 6 | ## What to read |
| 7 | |
| 8 | 1. The spec under review: |
| 9 | `docs/superpowers/specs/2026-09-11-isolated-exec-design.md`. |
| 10 | 2. The problem it answers, as filed: amadan discussion #126 on |
| 11 | carlos/platform, reproduced in spec §1; its companion #127 (spec §7). |
| 12 | 3. The code it fits into, in this worktree: |
| 13 | - `internal/activator/backend_exec.go` (Start 557-773, spawnInCgroup |
| 14 | 154-156, childProc 992-1033, instanceEnv 775-795, childEnv 802-814, |
| 15 | teeStd 123-137, Seed 332-440, StartSidecar 879-912, Restore 220-290, |
| 16 | PurgeLocalDBArtifacts 1041-1057); |
| 17 | - `internal/activator/activator.go` (Proc/Rep 194-207, doWake 772-900, |
| 18 | Bounce 946-956, stop 962-1047, sweepOnce 1082); |
| 19 | - `internal/activator/backend_unit.go` (systemctl 94-103, package doc); |
| 20 | - `internal/activator/admintoken.go` (6-14, 111-195); |
| 21 | - `internal/edge/purge.go` (53-107), `internal/edge/edge.go` (449-451, |
| 22 | 536-538); |
| 23 | - `internal/cgroup/cgroup.go` (37-62, 219, 242-285, 398-403) and the |
| 24 | `_linux.go` files; |
| 25 | - `internal/lease/lease.go` (257-299); |
| 26 | - `internal/appconfig/syncexec.go` (1-31, 465, 566-628, 860-956, |
| 27 | 977-1024); |
| 28 | - `internal/provision/provision.go` (36-135, 246-262, 995-1012), |
| 29 | `internal/provision/reconcile.go` (280-301, 374-436, 461-464, |
| 30 | 862-892); |
| 31 | - `internal/registry/registry.go` (58, 152-156, 413, 1057-1075); |
| 32 | - `internal/ops/litestream.go` (273-428); |
| 33 | - `internal/host/units/*` (all of them; especially |
| 34 | `carlos-edge.service`, `carlos-app@.service`, `carlos-app@.socket`, |
| 35 | `carlos-stage.*`, `carlos-restart.*`, `carlos-litestream.service`); |
| 36 | - `internal/host/bootstrap.go` (33-34, 116-180), |
| 37 | `internal/host/systemapply.go` (339-398, 517-589), |
| 38 | `internal/host/systemunits.go` (34-83); |
| 39 | - `internal/host/movespass.go` (100-132, 285), `internal/host/agent.go` |
| 40 | (186-192, 285, 305-309, 1771), `internal/schedule/runner.go` (461), |
| 41 | `internal/host/schedule_wiring.go` (163, 343-369); |
| 42 | - `cli.go` (83-199, 1622-1625); |
| 43 | - `AGENTS.md`. |
| 44 | 4. The precedent the namespace recipe is taken from: |
| 45 | `/home/paulca/github.com/tito/titogo/deploy/systemd/titogo@.service` |
| 46 | and `titogo-instance-dir@.service` (read-only; another repo). |
| 47 | 5. The pinfra scripts the rollout depends on: |
| 48 | `/home/paulca/github.com/carlosframework/platform-infrastructure/scripts/update-platform-binary.sh` |
| 49 | and `harden-host.sh` (read-only; another repo), and the two earlier |
| 50 | designs the spec moves resources out of: |
| 51 | `/home/paulca/github.com/carlosframework/platform-infrastructure/platform-design/superpowers/specs/2026-08-07-exec-child-accounting-design.md` |
| 52 | and `2026-08-28-memory-pressure-containment-design.md`. |
| 53 | 6. systemd's documented semantics (systemd.exec(5), systemd.service(5), |
| 54 | systemd.resource-control(5), systemd.socket(5)) for every directive |
| 55 | the spec relies on: `TemporaryFileSystem=`, `BindPaths=`, |
| 56 | `PrivatePIDs=`, `ProtectProc=`, `RestrictNamespaces=`, |
| 57 | `StandardOutput=file:` on an `AF_UNIX` path, `PartOf=`, `KillMode=mixed`, |
| 58 | `OOMPolicy=kill`, `OOMScoreAdjust=`, `Slice=`, |
| 59 | `systemctl set-property --runtime` on a not-yet-started template |
| 60 | instance, and which `systemctl` verbs an unprivileged caller may run |
| 61 | without polkit. |
| 62 | |
| 63 | ## Rounds after the first |
| 64 | |
| 65 | From round 2 on, the prior rounds' findings files |
| 66 | (`CODEX-FINDINGS-ISOLATED-EXEC.md`, then `-R2.md`, ...) are committed |
| 67 | beside the spec, and the spec's §13 records how each finding was |
| 68 | taken. **Start your report with a disposition table**: one row per |
| 69 | prior finding, stating whether the revision resolves it, resolves it |
| 70 | partially (say what is missing), or does not resolve it. Then report |
| 71 | only findings that are new in this revision, numbered `R<n>-1` onward |
| 72 | for the current round. Do not repeat a resolved finding; do not |
| 73 | re-argue a ruling. |
| 74 | |
| 75 | ## Decisions already taken (report a mechanism that fails one; do not re-argue one) |
| 76 | |
| 77 | - Paul approved approach 3 on 2026-09-11: the activator keeps ownership; |
| 78 | the process becomes a systemd template unit; still uid `carlos`; |
| 79 | namespace isolation; no console change; migration by rename. |
| 80 | - Approaches 1 and 2 (spec §12) are rejected. |
| 81 | - Ruled with codex R1-1 (2026-09-11): the root channel is the |
| 82 | `carlos-instance-control` service with a bearer token; polkit is not |
| 83 | installed and not relied on. Report a way the token or the channel |
| 84 | fails; do not re-open the polkit comparison. |
| 85 | |
| 86 | ## Things to check hard in revision 2 |
| 87 | |
| 88 | - **`nosymfollow` (§3.2).** Does a bind mount of a directory onto itself |
| 89 | with `nosymfollow` hold for lookups through the covering mount from |
| 90 | the agent's namespace, and through systemd's `BindPaths=` bind inside |
| 91 | the instance's namespace? Is there any path the agent, the edge, PID 1 |
| 92 | (`StandardOutput=file:`) or litestream resolves under the instance |
| 93 | trees that is not on that mount? Is the `openat2` helper redundant, |
| 94 | insufficient, or wrong in any flag? |
| 95 | - **The token (§5.4).** Where does it live, who writes it, who reads it, |
| 96 | and can an instance obtain it or cause the agent to present it on the |
| 97 | instance's behalf? |
| 98 | - **Exit observation (§6.5).** `cgroup.events` inotify semantics for |
| 99 | `populated`; the invocation id match; the fallback poll; what happens |
| 100 | to a proc whose unit is restarted by something other than the agent. |
| 101 | - **Slices (§4.3).** `Slice=carlos-instances-%i.slice` from a template |
| 102 | with an escaped instance name; on-demand slice creation; |
| 103 | `set-property --runtime` on a slice that does not exist yet; whether |
| 104 | moving the edge into `carlos.slice` keeps the 2026-08-28 wall's |
| 105 | semantics. |
| 106 | - **The fence (§8.1) and the reverse migration (§8.3).** Every consumer |
| 107 | that must honour the marker; the recovery table; the "two databases" |
| 108 | refusal; what `carlos system rollback` does today and whether a leg |
| 109 | can be added before the binary swap. |
| 110 | - **The probe and the gate (§8.4).** Can the probe pass while an instance |
| 111 | would not be isolated? Can it fail while instances would be? |
| 112 | |
| 113 | ## What matters most |
| 114 | |
| 115 | - **The isolation claim in §4.1.** From inside the unit, can an instance |
| 116 | reach a sibling's database, env file, config file, listening socket, |
| 117 | control socket, log socket, pid, or the control service socket? Can it |
| 118 | reach IMDS? Can it reach `/data/registry.db`, `/data/litestream.env`, |
| 119 | the console DB? Check each directive does what the spec says, and |
| 120 | check the order systemd applies them in (bind sources, tmpfs, ro). |
| 121 | - **The lifecycle in §6.** Exit observation with `Restart=no`; the |
| 122 | stop ordering (stop → done → close listeners) against |
| 123 | `backend_exec.go:992-1009`'s stated invariant; the log socket as a |
| 124 | replacement for the pipe (`teeStd`), including a grandchild holding it |
| 125 | open and `KillMode=mixed`; the race between binding `log.sock` and |
| 126 | PID 1 connecting; a wake that fails health and must stop; `Bounce`; |
| 127 | the agent-start sweep (§6.7) and `PartOf=`. |
| 128 | - **Memory (§4.3, §6.6).** The wall moves from one unit to a slice plus a |
| 129 | smaller edge cap. Is 60/20/20 defensible against the 2026-08-28 |
| 130 | design's reasoning? Does `OOMPolicy=kill` on the instance unit |
| 131 | reproduce `memory.oom.group=1`? Does `OOMScoreAdjust=500` reproduce |
| 132 | `ExecOOMScoreAdj`? Does the sampler's new scan root give the console |
| 133 | the same per-host numbers? |
| 134 | - **Migration (§8).** Idempotence between the file renames and the row |
| 135 | CAS; the ineligible-row policy (refuse to wake, name the fix); the |
| 136 | rollback story (old binary restores flat, nested copy retained); the |
| 137 | reconciler's drift log; the box-wide litestream generator's flat glob |
| 138 | (`internal/ops/litestream.go:354`) and what `FleetDBsWithSkipped` |
| 139 | claims after the row changes; what an in-flight residence move |
| 140 | (`movespass.go`) sees. |
| 141 | - **The trampoline (§6.2).** `instance-exec` reads an agent-written JSON |
| 142 | from the instance directory and execs. Is there a way the tenant |
| 143 | influences that file? Is refusing `argv[0] != /opt/carlos/live/<host>` |
| 144 | the right check? Anything the current `exec.Command` path does that |
| 145 | the trampoline drops (working dir, fds, `WaitDelay`, `Pdeathsig`)? |
| 146 | - **Every consumer (§9).** AGENTS.md's rule: everything that PARSES the |
| 147 | shape. Find a consumer the list misses. |
| 148 | - **Claims about the code.** Every `file:line` and every "already |
| 149 | exists" or "unchanged" claim: check it. Report each one that is wrong. |
| 150 | - **Operability.** Can a member, with no box access, tell that their |
| 151 | instance is isolated, see why it refused to wake, and read its logs as |
| 152 | before? Can an operator roll this back? |
| 153 | - **Scope.** Anything in the spec that is not needed for the isolation |
| 154 | property and could be cut; anything needed that is missing. |
| 155 | |
| 156 | ## How to report |
| 157 | |
| 158 | Number findings R1-1 onward. For each: severity (P1 breaks the isolation |
| 159 | property, breaks hibernation, loses data, or makes the change unbuildable |
| 160 | as written; P2 a contract, claim or migration defect to fix before the |
| 161 | plan; P3 advisory), the section, the problem, the evidence with |
| 162 | citations, a proposed correction. Put the §5.4 advisement first, as its |
| 163 | own finding, with a clear recommendation. End with a verdict: accept, |
| 164 | accept with corrections, or request changes. Do not soften, do not pad, |
| 165 | do not restate the spec. |
| 166 | |
amadan