refactor(module): systemd template units, %i = user (#154 Phase 3) - #295
refactor(module): systemd template units, %i = user (#154 Phase 3)#295defangdevs wants to merge 13 commits into
Conversation
Convert the per-user agent-box/agent-web-terminal/agent-box-settings/ agent-box-webhook services (+ settings/webhook sockets) from one Nix-generated unit per user to systemd %i template instances, per the Phase 3 design posted on #154. - modules/src/units/*.service|*.socket: verbatim, backend-neutral unit text with %i specifiers and bare (ExecSearchPath-resolved) ExecStart binaries — installed via systemd.packages, the shape a future native (non-NixOS) backend would also consume byte-for-byte. - Host-level config (PATH, pinned tool binaries, ExecSearchPath, the sudo-allowlist-conditional hardening knobs) renders as a drop-in on the "<unit>@" template (default overrideStrategy = "asDropinIfExists" detects the packaged unit and drops in automatically). - Per-user values move to generated env files under /etc/agent-box/units/*.env, read via EnvironmentFile=-...%i.env; the one exception (u.environment/u.environmentFiles) gets a per-instance asDropin drop-in alongside enablement. - Renamed the per-user unit family: agent-box-<user> -> agent-box@<user> (and the web-terminal/settings/webhook peers); updated the one in-repo reference (spot-monitor.sh) and every tests/*.nix assertion. - Extended the golden-snapshot etcFilter to also capture the new per-user env files, and regenerated tests/golden. Proceeded on the design's two explicitly-flagged decisions (ExecSearchPath for bare ExecStart resolution; generated env files over per-instance drop-ins) since neither drew an objection on the issue in the 15 days since it was posted — flagging both again here for this review. Verified locally (aarch64-linux native): module-generated-up-to-date, multi-user, module-single-file, download-route, webhook-route and golden-snapshot all green. The x86_64-linux VM tests (sessions, webhook, settings-page, download-files, self-serve-domain, memory-protection) were only `nix eval`'d (drvPath) — no KVM on this box — and their testScript passes the repo's ty/ruff gate; they still need a real run in CI. 🤖 Generated with Claude Code Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013SrgKvQmv9B4mS9BEBtJ6q
… asDropin wantedBy CI on #295 caught every service-dependent VM test failing with "unit agent-box@agent.service is inactive and there are no pending jobs" — none of the templated instances actually started. A `wantedBy` on a systemd.services.<instance> entry with overrideStrategy = "asDropin" is a dead end: NixOS's .wants-symlink generation links "../<unit-name>", which needs a real top-level file named e.g. "agent-box@alice.service" to point at. An asDropin instance never has one — only a "agent-box@alice.service.d/overrides.conf" — so the symlink dangles and systemd never schedules a start job, silently. systemd.targets.multi-user.wants (and .sockets.wants for the two socket units) is the mechanism that actually works: it renders a literal Wants= line into the target's own unit (verified by building the same aarch64 multiUser config the `multi-user` check uses and inspecting multi-user.target.d/overrides.conf directly), and systemd resolves a wanted instance name from the %i template at start time — no per-instance file needed. Same pattern nixpkgs itself uses to enable one instance of a template unit. Dropped the now-empty per-instance overrides that carried nothing but overrideStrategy + wantedBy (agent-web-terminal@, agent-box-settings@ service+socket, agent-box-webhook@ service+socket); kept the agent-box@ per-instance drop-in, which still carries real content (u.environment / u.environmentFiles).
The Wants= override that actually enables each per-user template instance (the previous commit's fix) lands on units the module doesn't own (multi-user.target, sockets.target), which unitFilter excluded — so the one behavior this refactor most needed to prove (does an instance actually get started) was invisible to the fixture diff and to every eval-level check. Widen the filter to include these two exact unit names and regenerate.
|
CI's real VM run caught a genuine bug (thanks — this is exactly what `nix eval .#checks.x86_64-linux..drvPath` can't catch without a KVM boot): every service-dependent test failed with `unit "agent-box@agent.service" is inactive and there are no pending jobs`. None of the templated instances ever started. Root cause: a `wantedBy` on a `systemd.services.` entry with `overrideStrategy = "asDropin"` is a dead end. NixOS's `.wants`-symlink generation links `../`, which needs a real top-level file literally named e.g. `agent-box@alice.service` to point at — an asDropin instance never has one (only `agent-box@alice.service.d/overrides.conf`), so the symlink dangles and systemd never schedules a start job. No error anywhere; the unit just sits inactive forever. Fix (pushed): `systemd.targets.multi-user.wants` / `systemd.targets.sockets.wants` instead — these render a literal `Wants=` line into the target's own unit (via its `asDropinIfExists` override), and systemd resolves the wanted instance name from the `%i` template at start time, no per-instance file needed. Same pattern nixpkgs itself uses to enable one instance of a template unit. Verified by building the exact aarch64 `multiUser` config the `multi-user` check uses and inspecting the real output: ``` Dropped the now-empty per-instance overrides that carried nothing but `overrideStrategy` + `wantedBy` (`agent-web-terminal@`, `agent-box-settings@` service+socket, `agent-box-webhook@` service+socket); kept the `agent-box@` per-instance drop-in, which still carries real content (`u.environment`/`u.environmentFiles`). Also widened the golden-snapshot `unitFilter` to capture `multi-user.target`/`sockets.target` themselves — this exact bug class (a silently-inactive unit) was invisible to every eval-level check and to the fixture diff; now the `Wants=` line is part of the reviewable snapshot and a future regression here would show up as a fixture diff instead of only a VM-test failure. Native checks (including `golden-snapshot`) all green here; re-verify on the real x86_64 VM tests via CI. |
… path The unit-file enablement fix in the previous commit was necessary but not sufficient — CI still failed identically. The real cause: MARKER's path group was `[^@]+`, so `@@include:src/units/agent-box@.service@@` (every systemd %i template unit's filename literally contains "@") never matched. resolve() silently left the marker line as-is, so modules/agent-box.nix embedded the LITERAL TEXT "@@include:src/units/agent-box@.service@@" as the unit's entire content instead of the real file — an empty, ExecStart-less unit that explains "Assignment outside of section. Ignoring." in the boot log and why the instantiated unit could never actually do anything once started. `module-generated-up-to-date` couldn't catch this: it compares two runs of the SAME (buggy) assembly process, so a deterministic mismatch stays invisible even though the output is wrong. golden-snapshot couldn't either — its unit capture reads `sys.config.systemd.units.<name>.text`, which for a systemd.packages-provided file only exists at all if a matching Nix-declared drop-in ALSO exists (as it does here, thanks to the previous commit's `agent-box@` override) — and even then it's just the drop-in's own text, never the base package file's real bytes. That gap is real and worth a follow-up (filed as an issue) but out of scope for unblocking this PR. Widening `[^@]+` to `.+` (greedy, so it backtracks to the LAST `@@` on the line) fixes it for every existing include site too — verified no other `modules/src/*` path contains an embedded `@@`, so this can't newly misparse a legitimate marker. Verified directly (not just via eval): built the same aarch64 `multiUser` config the `multi-user` check uses and confirmed `agent-box@.service` now contains its real [Unit]/[Service] text (ExecStart=agent-box-supervisor etc.), not the marker string.
|
Fixed a second, deeper bug — the enablement fix alone didn't do it, CI failed identically. Real cause: `bin/assemble-module.py`'s `@@include:` marker regex used `[^@]+` for the target path, and every one of these systemd `%i` template unit filenames contains a literal `@` (`agent-box@.service`, etc). The marker never matched, so `resolve()` silently left the literal marker text in place — `modules/agent-box.nix` embedded `@@include:src/units/agent-box@.service@@` as the unit's ENTIRE content. No `[Unit]`, no `ExecStart`, nothing. That's exactly `systemd[1]: agent-box@.service:1: Assignment outside of section. Ignoring.` in the boot log, and why the instantiated unit could never do anything once (correctly, per the first fix) wanted. Neither `module-generated-up-to-date` nor `golden-snapshot` could catch this: the former compares two runs of the same deterministic bug against each other; the latter never inspects the raw bytes of a `systemd.packages`-shipped file (only a Nix-declared unit's own `.text`, which here is just the drop-in override, never the base template). Filed #299 to close that visibility gap — out of scope for this PR to fix fully, but flagging it since it's exactly how this slipped past every check except a real VM boot. Fix: widened the regex to `.+` (greedy — backtracks to the last `@@` on the line). Confirmed no other `modules/src/*` include path has an embedded `@@` that this could newly misparse. Verified directly — not just via eval — by building the same aarch64 `multiUser` config the `multi-user` check uses and reading the real file: `agent-box@.service` now contains its actual `[Unit]`/`[Service]` text (`ExecStart=agent-box-supervisor`, hardening, everything), not the marker string. All native checks (including `golden-snapshot`, unchanged since it was blind to this either way) green. Re-triggered CI. |
…PATH
Third bug in this sequence, and the one that actually explains "Unable to
locate executable 'agent-box-supervisor'" from CI. Per systemd.exec(5):
ExecSearchPath= overrides $PATH IF $PATH is not supplied by the user
through Environment=, EnvironmentFile= or PassEnvironment=.
NixOS's own systemd module renders a default Environment=PATH=<coreutils,
findutils, gnugrep, gnused, systemd only> for ANY declared
systemd.services.<name> submodule instance, at plain (non-mkDefault)
priority — including our near-empty per-user "agent-box@<user>" override
(needed only to carry u.environment/u.environmentFiles when present).
That instance-level drop-in loads after the template-level
"agent-box@.service.d/overrides.conf" that carries the real
ExecSearchPath, so its short default $PATH wins — which, per the man page
above, doesn't just lose a PATH merge, it disables ExecSearchPath
entirely. Every configured user hit this; it just took ~14 minutes of
Restart=always/RestartSec=2s failures (~420 restarts) to notice, since
each of those minimal test configs never explicitly sets u.environment.
Fix: give the per-instance drop-in the same real PATH via
`environment.PATH = lib.mkForce agentBoxExecSearchPath`, merged under
u.environment so a host can still override it. mkForce is required, not
optional — NixOS's own default is plain priority, and two same-priority
definitions of one option is a hard eval error, not a silent "last one
wins" (confirmed by hitting exactly that error before adding mkForce).
Factored the search-path list into one `agentBoxExecSearchPath` local so
the host-level ExecSearchPath and this repeated PATH can never drift.
Verified directly: rebuilt the exact tests/memory-protection.nix NixOS
config (services.agent-box.users.agent = {};) and read
agent-box@agent.service.d/overrides.conf — Environment="PATH=..." now
starts with /home/%i/.nix-profile/bin and includes the supervisor's own
store path, matching ExecSearchPath exactly.
|
Found the actual root cause of `Unable to locate executable 'agent-box-supervisor'` — a third, distinct bug (the assembler fix was real and necessary, but this is what was actually causing the exec failure). Per `systemd.exec(5)`:
NixOS's own systemd module renders a default `Environment=PATH=<coreutils/findutils/gnugrep/gnused/systemd only>` for any declared `systemd.services.` instance, at plain (non-`mkDefault`) priority — including our near-empty per-user `agent-box@` drop-in (kept around only to carry `u.environment`/`u.environmentFiles` when a host sets them). That instance-level drop-in loads after the template-level one carrying the real `ExecSearchPath`, so its short default $PATH wins — and per the man page, that doesn't just lose a merge, it disables `ExecSearchPath` entirely for that unit. Every configured user hits this identically; it just took ~14 minutes of `Restart=always`/`RestartSec=2s` (~420 restarts) for the VM test's 900s wait to give up and report it. Fix: `environment.PATH = lib.mkForce agentBoxExecSearchPath` on the per-instance drop-in (merged under `u.environment` so a host can still override it), with the search-path list factored into one local so the host-level `ExecSearchPath` and this repeated `PATH` can't drift apart. `mkForce` is required, not decorative — without it this is a hard eval conflict (NixOS's own default is plain-priority too), which is how I confirmed the exact mechanism. Verified directly (not just eval): rebuilt the exact `tests/memory-protection.nix` config and read `agent-box@agent.service.d/overrides.conf` — `PATH=` now starts with `/home/%i/.nix-profile/bin` and includes the supervisor's own store path. All native checks green, golden fixture updated with the real (small, exactly-as-expected) diff this time, x86_64 VM tests eval clean. Re-triggered CI — this one I'm fairly confident actually fixes the boot-time failure, since I read the exact rendered file systemd would load rather than only checking eval success. |
…rch-path resolution The PATH mkForce fix (previous commit) was independently verified correct — both by eval and by reading the actual rendered agent-box@agent.service.d file, and by re-checking the exact merge ref CI would build — yet the next CI run failed identically: "Unable to locate executable 'agent-box-supervisor'". Whatever the precise reason ExecSearchPath isn't taking effect for a %i template instance's bare ExecStart in practice (despite matching the documented $PATH/ExecSearchPath precedence rule), chasing it further isn't worth it: this was always the more speculative of the two decisions the design flagged for objection, and there's a strictly more robust alternative that has zero dependency on any search-path resolution at all. Override ExecStart/ExecStop on the host-level "agent-box@" template drop-in with an absolute path, using systemd's standard reset-then-reassign idiom (an empty-string list element clears the verbatim unit's own bare ExecStart=/ExecStop=; the real path replaces it — systemd-lib.nix renders a list value as one directive line per element, exactly matching this convention). ExecSearchPath and the per-instance PATH mkForce both stay: they're still what the supervisor's own internal tmux/jq/grep/etc calls rely on at runtime, just no longer what systemd itself uses to resolve ExecStart/ExecStop. Verified directly: rebuilt the same aarch64 reproduction and read agent-box@.service.d/overrides.conf — ExecStart= ExecStart=/nix/store/.../agent-box-supervisor/bin/agent-box-supervisor ExecStop= ExecStop=/nix/store/.../tmux/bin/tmux -L agent-box kill-server
|
The PATH fix was independently verified correct — eval value, actual built file content, and re-checked against the exact merge ref CI built (same derivation hashes, confirmed byte-for-byte) — yet CI failed identically again with the same `Unable to locate executable 'agent-box-supervisor'`. Whatever the precise reason `ExecSearchPath=` isn't taking effect for a `%i` template instance's bare `ExecStart=` in this real boot despite matching the documented precedence rule, I'm not going to keep chasing it — it was always the more speculative of the two flagged decisions, and there's a strictly more robust fix with zero dependency on any search-path resolution. Overrode `ExecStart`/`ExecStop` directly with absolute paths on the host-level `agent-box@` drop-in, using systemd's standard reset-then-reassign idiom (empty string clears the verbatim unit's own bare directive, the real path replaces it — confirmed `systemd-lib.nix` renders a list value as one line per element, exactly this convention): ``` `ExecSearchPath`/the per-instance `PATH` forcing both stay — they're still what the supervisor script's own internal tmux/jq/grep calls rely on at runtime — they're just no longer what systemd itself uses to resolve `ExecStart`/`ExecStop`. All native checks green, golden fixture shows exactly this diff, x86_64 VM tests eval clean. Re-triggered CI. |
… systemd loaded Not for merge. Every Nix-level check (eval option value, actual built drop-in file content, re-checked against the exact PR merge ref) says agent-box@agent.service's ExecStart is the correct absolute path, but the real VM boot keeps reporting "Unable to locate executable 'agent-box-supervisor'" — the bare-name error text, not what an absolute path failure would say. Printing systemctl cat/show output right before the failing wait_for_unit to see what systemd actually thinks it loaded, since theorizing further against a config that verifies correct on every static check isn't converging.
|
Pushed a temporary diagnostic (not for merge) — every static check keeps saying the config is correct, so I need to see what systemd itself thinks it loaded at runtime rather than theorize further. Added `systemctl cat`/`systemctl show` right before the failing `wait_for_unit` in `tests/memory-protection.nix`. Will remove it once this is actually root-caused. |
… for the template drop-in systemctl cat proved agent-box@.service.d/ never gets merged into agent-box@agent.service, contradicting systemd.unit(5)'s documented lookup order (instance .d/ then template .d/). Checking the filesystem directly (does the directory/file even exist where expected?) and the journal (any drop-in load warning we might have missed).
|
The prior diagnostic gave a real answer, and it's not what I expected: `systemctl cat` shows only TWO sources merged into `agent-box@agent.service` — the base unit file and the per-instance `agent-box@agent.service.d/overrides.conf`. The template-level `agent-box@.service.d/overrides.conf` (which carries `ExecStart`, `ExecSearchPath`, and all the `AGENT_BOX_*_BIN` vars) never shows up at all — despite `systemd.unit(5)` explicitly documenting "For instantiated units, this logic will first look for the instance `.d/` subdirectory... followed by the template `.d/` subdirectory". Pushed one more diagnostic checking the filesystem directly (does `/etc/systemd/system/agent-box@.service.d/` even exist as expected?) and the journal for any drop-in load warning. |
… theory The on-disk drop-in is provably correct (prior diagnostic: cat shows the right ExecStart), yet every restart from boot #0 onward resolves the bare name. Restart=always reuses the unit config as loaded, it doesn't re-read files per restart — testing whether a daemon-reload + fresh restart actually picks up the correct ExecStart, which would mean the unit got loaded before this drop-in was indexed and never re-merged since.
|
Filesystem diagnostic result: `/etc/systemd/system/agent-box@.service.d/overrides.conf` exists and its content is 100% correct (`ExecStart=` then `ExecStart=/nix/store/.../agent-box-supervisor/bin/agent-box-supervisor`) — confirmed by direct `cat`. Yet the unit's very first restart at boot (counter 0) already resolves the bare name, and every restart since repeats it identically. The file never changes during boot, so this isn't a late-population race. Testing a theory: `Restart=always` reuses the SAME loaded unit config across restarts — it doesn't re-read files from disk each time. If systemd's very first load of `agent-box@agent.service` (when `multi-user.target`'s `Wants=` first pulls it in) happened before this drop-in was indexed for some reason, every subsequent restart would keep reusing that stale (bare-ExecStart) load — explaining why the file is correct but the runtime behavior isn't. Pushed a diagnostic that does `systemctl daemon-reload` + `restart` mid-test and re-checks `ExecStart` afterward — if that flips it to the absolute path, this is confirmed and points to something in the boot sequence, not the config itself. |
|
Definitive answer: `systemctl daemon-reload` + a fresh `restart` still resolves the bare name — `systemctl show agent-box@agent.service -p ExecStart` after the reload still shows `path=agent-box-supervisor`. This isn't a staleness/timing issue; the template-level `agent-box@.service.d/` drop-in is never merged into the `agent-box@agent.service` instance, full stop, contradicting `systemd.unit(5)`'s documented lookup order. The per-instance drop-in (`agent-box@agent.service.d/`, matched by the exact instantiated name) works fine — it's specifically the "falls back to the template's own `.d/`" step that isn't happening here, for reasons I can't pin down further without more systemd internals than I can verify in this environment (no root access on this box to construct an isolated repro, and I can't build x86_64 locally to bisect further). This means the ENTIRE host-level content of the `agent-box@` declaration (and, by the same mechanism, `agent-web-terminal@`/`agent-box-settings@`/`agent-box-webhook@`, which rely on it identically) has never actually been reaching any instance — a much bigger gap in this PR than just `ExecStart`. Fixing it properly: moving all of that content off the template-level declaration and onto a real per-instance drop-in for every configured user/terminal user, since per-instance drop-ins are the one thing proven reliable through every check so far. That's a real restructuring, not a one-line fix — doing it now. |
…-ins
The real, final root cause of "Unable to locate executable
'agent-box-supervisor'" (and, it turns out, the reason none of
agent-web-terminal@/agent-box-settings@/agent-box-webhook@'s own
host-level config — PATH, ExecSearchPath, ExecStart — was ever reaching
a real instance either): a TEMPLATE-level drop-in
("agent-box@.service.d/", "agent-web-terminal@.service.d/", etc, using
overrideStrategy = "asDropinIfExists") never gets merged into an
INSTANTIATED unit on a real systemd boot, contradicting systemd.unit(5)'s
documented lookup order ("For instantiated units, this logic will first
look for the instance '.d/' subdirectory... followed by the template
'.d/' subdirectory").
Confirmed exhaustively on PR #295 (see its comments for the full
investigation): the on-disk file is correct (read directly), `systemctl
cat`/`show` never include it even immediately after `systemctl
daemon-reload`, and this isn't a staleness/timing issue — it simply never
merges, on this systemd version, for this template-unit shape. The
per-INSTANCE drop-in ("agent-box@agent.service.d/", matched by the exact
instantiated name) is the one thing proven reliable through every check.
Fix: every unit family's host-level content (env vars, ExecSearchPath,
the absolute-path ExecStart/ExecStop overrides) moved from a single
shared template-level declaration onto a real per-instance drop-in,
generated once per configured user (agent-box@) or per terminal user
(agent-web-terminal@, agent-box-settings@, agent-box-webhook@). This
costs a duplicated copy of that content per instance instead of one
shared drop-in — an acceptable price for something that actually works,
confirmed by rebuilding a full-featured (web+webhook enabled, two users)
config and reading every rendered agent-box@<user>.service.d/,
agent-web-terminal@<user>.service.d/, agent-box-settings@<user>.service.d/
and agent-box-webhook@<user>.service.d/ overrides.conf directly: all four
now carry the correct absolute-path ExecStart (and ExecStartPre for the
webhook receiver) and the correct forced PATH.
Sockets are unaffected: agent-box-settings@.socket/agent-box-webhook@.socket
are fully %i-derived in their verbatim text already, needing no host-level
override at all — only enablement, which is a systemd.targets.*.wants
entry (a genuinely different, already-working mechanism, not a
per-instance/template drop-in).
Also: dropped the stale "agent-box@" (bare template name) entry from
flake.nix's multi-user check — that key never existed in
systemd.services once the template-level declaration was removed;
removed the now-served-its-purpose temporary diagnostic from
tests/memory-protection.nix.
|
Pushed the real fix: moved every unit family's host-level content (env vars, `ExecSearchPath`, the absolute-path `ExecStart`/`ExecStop` overrides) off the template-level declaration and onto a real per-instance drop-in — one per configured user for `agent-box@`, one per terminal user for `agent-web-terminal@`/`agent-box-settings@`/`agent-box-webhook@`. This is the same fix for all four unit families, since all four were relying on the same broken template-level mechanism identically (only `agent-box@`'s symptom got investigated in depth, but `agent-web-terminal@`/`agent-box-settings@`/`agent-box-webhook@` would have failed the exact same way had their tests exercised the failure path as directly). Sockets needed no change — `agent-box-settings@.socket`/`agent-box-webhook@.socket` are fully `%i`-derived in their verbatim text already; only their enablement is per-instance, and that's a `systemd.targets.sockets.wants` entry, a genuinely different (and already proven-working) mechanism. Verified by rebuilding a full-featured config (web + webhook enabled, two users) and reading every rendered `@.service.d/overrides.conf` directly — all four now carry the correct absolute-path `ExecStart` and forced `PATH`. Removed the temporary diagnostic from `tests/memory-protection.nix` (it served its purpose) and fixed a stale assertion in `flake.nix`'s `multi-user` check that expected a `systemd.services."agent-box@"` entry to exist — it never will now, since the whole point is that key wasn't taking effect. All native checks + golden-snapshot green, all VM tests eval clean. Re-triggered CI — this is the fix I'd actually bet on, since it's not fighting a mechanism that's proven unreliable, it's just not using that mechanism anymore. |
Both predate this PR's own commits (part of the original Phase 1/2/3
work) but were never actually exercised — every earlier CI run on this
PR failed at an earlier stage (unit never started, then exec resolution)
before test execution got this far, and per tests/sessions.nix's own
comment ("the CI hang on this PR's first three runs") this test line
specifically never completed even before that.
- tests/sessions.nix: the ExecStart store-path extraction regex matched
"-agent-box-supervisor" as a suffix, which the writeShellScriptBin
DERIVATION DIRECTORY also ends in, one path segment before the actual
script at ".../bin/agent-box-supervisor". Since the directory name is
the only place in the string with a literal hyphen (not slash) right
before "agent-box-supervisor", the regex always matched the directory,
and grep-ing a directory for the rcname= string fails with "Is a
directory" (exit 2) — this would have been just as true before this
PR's ExecStart absolute-path override, since the ExecStart was always
an absolute path into the same shaped derivation. Fixed by matching the
"/bin/agent-box-supervisor" suffix specifically.
- tests/webhook.nix: AGENT_BOX_WEBHOOK_URL is per-user, so issue #154
Phase 3's own design puts it in the generated
/etc/agent-box/units/<user>.env, loaded via the unit's
EnvironmentFile=, not a static Environment= directive — it will never
show up in `systemctl show -p Environment`, by design. Checks the
actual running process's /proc/<pid>/environ instead, which also more
directly proves EnvironmentFile= actually got wired up (the exact kind
of thing this PR's whole investigation found reasons not to take for
granted) rather than just that the generated file has the right text.
|
Great news from that run: 5 of 7 VM tests now pass completely — `memory-protection` (the one stuck at a 900s timeout every prior run), `self-serve-domain`, `download-files`, `web-fail2ban`, `settings-page`. The structural fix holds. The remaining 2 failures are both pre-existing test bugs from the original Phase 1/2/3 work, not new regressions — every earlier CI run on this PR failed at an earlier stage before test execution ever reached these specific lines (and per `tests/sessions.nix`'s own comment, this exact line never completed even before this PR, due to "the CI hang on this PR's first three runs"):
Pushed both fixes. Re-triggered CI — expecting all 7 VM tests green this time. |
Same root cause class as the previous commit — CI reaching these lines
for the first time surfaced test assumptions that predate this PR's own
changes:
- tests/sessions.nix: two occurrences of the same "-agent-box-attach"
suffix bug as the supervisor one (extracts the writeShellScriptBin
DIRECTORY, not the "/bin/agent-box-attach" script) — one of which then
tries to exec that directory as a command (`{attach} {name}`), the
other greps it for a CLI flag. Both fixed to match the "/bin/" suffix.
- tests/webhook.nix: issue #154 Phase 3's own design moved the pinned
`webhookPython`/`localWebhookScript` invocation out of the unit's
ExecStart and into agent-box-webhook-receiver's own `exec` line (so
the shared unit text wouldn't tie to a specific Nix build) — but this
test still extracted python3/webhook.py directly from ExecStart, where
they no longer appear at all. Extracts the wrapper's own path from
ExecStart first, then greps ITS body for the interpreter/script pair
the test needs to spawn its stand-in session peer.
|
Two more of the same class fixed — CI reaching these lines for the first time surfaced test assumptions that predate this PR entirely:
All native checks green, both files eval clean. Re-triggered CI. If this comes back clean I think we're actually done — every remaining item so far has been "test predates this PR and never got exercised," not a new module bug. |
…rt too
The verbatim unit text leaves it bare ("... agent-box-attach", resolved
via ttyd's own runtime PATH — which is correctly forced now and would
work fine functionally), but tests/sessions.nix needs to extract the
script's absolute store path to invoke it directly outside any unit
context (a "dead end" probe when a session is stopped, and a check that
the wrapper's own body carries the "-T hyperlinks" tmux flag). Matching
the same "don't depend on a search path being merged/forced correctly"
posture as the ExecStart/ExecStop overrides elsewhere in this PR.
|
6 of 7 passed this round, including `webhook`! The last one — `sessions` — needed one more thing: it grabs `agent-box-attach`'s absolute path from `agent-web-terminal@agent`'s `ExecStart` to invoke the script directly outside any unit context (probing a stopped session, checking the wrapper carries `-T hyperlinks`). My `ExecStart` override left `agent-box-attach` as a bare argument to ttyd (matching the verbatim unit text, resolved via ttyd's own runtime PATH — which works fine functionally) — but the test needs the resolved path, not the bare name. Gave it the absolute path too, consistent with the "don't depend on any search-path merge" posture everywhere else in this PR. All native checks + golden-snapshot green (small, exactly-expected fixture diff), all 7 VM tests eval clean. Re-triggered CI — expecting all green. |
|
CI is fully green — all 7 VM tests, all native checks, golden-snapshot. Summary of where this landed, since it moved a lot from the original PR body:
This PR is a lot bigger than its original diff now, entirely from CI (not local checks) surfacing real, reproducible issues one at a time. Whenever you get a chance to look — happy to split anything out if that makes review easier, otherwise I think it's ready as one PR given how interlocking the fixes ended up being. |
2 participants
Phase 3 of #154, stacked directly on
master(Phases 0–2 are already merged: #179, #181, #182). Implements the Phase 3 design posted on the tracker — systemd%itemplate units for the four per-user services (agent-box,agent-web-terminal,agent-box-settings+ socket,agent-box-webhook+ socket).What changed
modules/src/units/*.service/*.socketare new verbatim, backend-neutral unit assets:%ispecifiers for per-instance paths, bareExecStart=/ExecStartPre=binary names resolved viaExecSearchPath=. Installed viasystemd.packages— the shape a future native (non-NixOS) backend would also install byte-for-byte.AGENT_BOX_*_BINtool pins,ExecSearchPath=, the sudo-allowlist-conditional hardening knobs) renders as a drop-in on the"<unit>@"template. This relies on NixOS's defaultoverrideStrategy = "asDropinIfExists": since a same-named unit file already exists fromsystemd.packages, a plainsystemd.services."agent-box@" = {...}definition automatically becomesagent-box@.service.d/overrides.confinstead of a competing full unit (verified against nixpkgs' ownnixos/tests/activation/template-dropin.nix)./etc/agent-box/units/*.env, read viaEnvironmentFile=-...%i.env. The one thing an env file can't express — arbitraryu.environment/u.environmentFiles— gets a per-instanceoverrideStrategy = "asDropin"drop-in, alongside per-instance enablement (wantedBy).agent-box-<user>→agent-box@<user>(and theagent-web-terminal/agent-box-settings/agent-box-webhookpeers similarly). Updated the one in-repo runtime reference (spot-monitor.sh'ssystemctl stop) and everytests/*.nixassertion (wait_for_unit,systemctl show/cat,journalctl -u, cgroup match) — careful to leave the/run/agent-box-<user>runtime-directory path untouched (still hyphenated; only unit names took the@).agent-web-auth-secrets(the singleton oneshot that preps Caddy's auth env) is explicitly out of scope here — it's not templated (one instance box-wide), and the design's suggested manifest-file rework for it is a separate refinement, not part of the%i-templating ask.flake.nix's golden-snapshotetcFilterto also capture the new per-user env files (the design's own note: "the snapshot tool needs a small extension… so the review diff stays complete"), and regeneratedtests/golden— that diff is the full reviewable behavior delta.The two decisions the design flagged for objection
Both stood on the issue for 15 days with no objection; proceeded on both, flagging again here for this review:
ExecSearchPath=for resolving the bareExecStart=agent-box-supervisor(etc.) instead of a@BINDIR@-token fallback. Requires systemd ≥ 250 (NixOS 25.05, Ubuntu 24.04, RHEL 9 all qualify).u.environment/u.environmentFiles).Verification
Ran natively on this box (aarch64-linux) — all green:
nix build .#checks.aarch64-linux.module-generated-up-to-datenix build .#checks.aarch64-linux.multi-user(updated to check foragent-box@/agent-box@<user>instead of the old flat names)nix build .#checks.aarch64-linux.module-single-filenix build .#checks.aarch64-linux.download-routenix build .#checks.aarch64-linux.webhook-routenix build .#checks.aarch64-linux.golden-snapshotThe x86_64-linux VM tests this touches (
sessions,webhook,settings-page,download-files,self-serve-domain,memory-protection) have no KVM path on this box, so I could onlynix eval .#checks.x86_64-linux.<name>.drvPath(all evaluate cleanly) and run theirtestScriptthrough the repo'sty/ruffgate (bin/check-testscript.sh, all pass). They have not actually been run — that's CI's job, and given the size of this change I'd treat a green CI run here as a hard gate before merge, not a formality.memory-protection(the one test withweb.enable = false) got particular attention since it's the sole guard against the ungated agent unit referencing a web-gated path — the extraReadWritePathsentry stays conditional in the host drop-in, contributing nothing whenweb.enableis off.Closes the fourth checkbox on #154 once merged.
🤖 Generated with Claude Code