fix(sessions): chmod ~/.config/agent-box after mkdir -p, add test coverage (#78) - #491
Conversation
…in it (#78) The first-boot subtest only checked sessions.json's owner/mode, which proves the config dir exists but never pins its own owner/mode. That's exactly what #356's parent-dir tmpfiles rule fixed (tmpfiles used to auto-create ~/.config as root and then refuse to descend into it), and the VM's disk is fresh on every run — the one condition the bug needed. Assert the directory directly so a regression there fails loudly instead of only failing whatever happens to write into it next.
|
Warning Review limit reachedNext included review available in 1 minute. View limit detailsLimit details: You’ve used all 8 included reviews currently available. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (7)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 8 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change enforces mode ChangesAgent Box permissions
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change ensures the registry directory is created with private permissions across supported paths and adds direct test coverage; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes address issue Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files. (6 skipped: 6 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
PR #491's new assertion caught a real gap: on a fresh boot the `d /home/<user>/.config/agent-box 0700 ...` tmpfiles rule loses a race against the declarative user's own group creation — systemd-tmpfiles[407]: /etc/tmpfiles.d/00-nixos.conf:13: Failed to resolve group 'agent': Unknown group `agent` is a normal (isNormalUser) user, created by the legacy activation script rather than systemd-sysusers, and nothing orders tmpfiles-setup after that activation — so both the parent and child tmpfiles lines are silently skipped. The directory still gets made, by registry.sh's own `mkdir -p "${REGISTRY_FILE%/*}"` (two call sites), but mkdir only applies a mode when it creates every missing component, and does so under umask — landing on ~0755 instead of the intended 0700. envstore.py's save() already chmods its directory after makedirs for exactly this reason ("a directory made by an older mkdir -p under umask 022 is 0755"). Do the same in the shell registry helper so the mode self-heals regardless of which side of the boot-order race wins, rather than trying to fix the race itself. modules/agent-box.nix and tests/golden regenerated via `nix run .#assemble` / `nix run .#update-golden`.
defangdevs
changed the title
CI caught it: test_a_write_that_cannot_land_is_reported_as_a_failure deliberately chmods an EXISTING registry directory to 0500 to simulate a full disk / read-only $HOME, then expects the write to fail. The previous commit's unconditional `chmod 0700` after `mkdir -p` ran on every call regardless of whether the directory was already there, so it silently undid that simulation and the write succeeded instead of failing. Guard both call sites on `[ -d ... ]` first, so the chmod (and the mkdir) only run when this call is the one creating the directory — matching the actual bug (tmpfiles lost the boot-order race and the directory does not exist yet), not touching a directory that already exists for any other reason, deliberate or not. modules/agent-box.nix and tests/golden regenerated.
1 participant
Summary
Investigated #78 (the tmpfiles rule for
~/.config/agent-box"possibly not applied on rebuild"). What started as a test-coverage PR turned up a real, still-live bug — CI on the first push caught it directly.Root cause, confirmed from the CI VM's boot journal:
agentis a normal (isNormalUser) declarative user, created by the legacyNixOS user/group activation script rather than
systemd-sysusers. Nothingorders
systemd-tmpfiles-setup.serviceafter that activation, so on a freshboot it can run before the
agentgroup exists — losing the race andsilently skipping both the parent (
~/.config, #356) and child(
~/.config/agent-box) tmpfiles lines. The directory still gets made, byregistry.sh's ownmkdir -p "${REGISTRY_FILE%/*}"(two call sites) — butmkdir -ponly applies a mode when it creates every missing path component,and does so under umask, landing on ~0755 instead of the declared 0700.
envstore.py'ssave()already works around the identical class of problemby
chmoding its directory afteros.makedirs()("a directory made by anolder
mkdir -punder umask 022 is 0755"). This PR does the same in theshared shell registry helper, so
~/.config/agent-box's mode self-healsregardless of which side of the boot-order race wins, rather than trying to
fix the race itself (a much bigger, riskier change to NixOS's own unit
ordering assumptions).
What was unverified before this PR:
tests/sessions.nix's first-bootsubtest already runs against a disk that's fresh every time, but only
checked
sessions.json's owner/mode — a file inside the dir, not the diritself. Asserting the directory directly (this PR's first commit) is what
surfaced the gap; the second commit fixes it.
Out of scope: the issue also flags "Codex-side unverified (0700 home)"
— that's the native (non-NixOS) renderer's home-dir permissions, a
different code path, and its settings daemon already works per the issue.
Left alone here.
Changes
tests/sessions.nix: assert/home/agent/.config/agent-boxisagent:agent 0700before checkingsessions.jsoninside it.modules/src/lib/registry.sh:chmod 0700the registry directory rightafter each of its two
mkdir -pcall sites, mirroringenvstore.py'sexisting precedent.
modules/agent-box.nixandtests/goldenregenerated (
nix run .#assemble,nix run .#update-golden) — the fixreaches all 5 payload sites that splice in
registry.sh(session CLI,supervisor, mark-stopped x2, webhook-spawn), on both the NixOS and
native backends (shared source file, confirmed by
one-spec-both-backendsand
backend-parity).Test plan
nix-instantiate --parse tests/sessions.nixpython3 tests/test-assemble-module.pynix build .#checks.aarch64-linux.module-generated-up-to-datenix build .#checks.aarch64-linux.golden-snapshot(regenerated, now matches)nix build .#checks.aarch64-linux.one-spec-both-backendsnix build .#checks.aarch64-linux.backend-paritysessionsVM test (x86_64-linux only — this box is aarch64, can't run it locally). First push failed exactly as expected on the un-fixed tree; re-running with the fix included.Closes #78.