Fix the appliance session crash-looping: %U resolved to root, not the user - #33
Merged
Conversation
… user Found live on the Pi 5 immediately after deploying the previous fix (#32/ReadWritePaths=/run/user/%U): the session now failed a different way, crash-looping with: Failed to set up mount namespacing: /run/user/0: No such file or directory Failed at step NAMESPACE spawning .../wait-for-console.sh: No such file or directory systemd's %U specifier is documented to expand to the UID of the unit's configured User=, but resolved to 0 (root) for this unit's mount-namespace setup phase on this systemd version (257.13) -- not randroid's 1000. Whatever the exact cause, relying on it clearly isn't reliable here. Sidesteps the specifier entirely: install-appliance.sh now resolves the console user's numeric UID itself (`id -u`) at install time, the same way it already resolves the group and home directory, validates it's numeric, and templates it directly as @CONSOLE_UID@ instead of leaving %U for systemd to resolve at runtime. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GaoUpejgTieMMycCoTektN
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (7)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2026-08-12T21:23:10.050ZApplied to files:
📚 Learning: 2026-08-09T23:24:38.027ZApplied to files:
🔇 Additional comments (1)
📝 WalkthroughWalkthroughThe installer resolves the console home directory and numeric UID from one passwd lookup. Dry runs use fallback values when needed. Rendered systemd units use the UID, and validation checks the rendered runtime path. ChangesConsole UID templating
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: ⚪ Minimal · up to The change templates the console user's numeric UID at install time to prevent the appliance session crash-loop caused by incorrect runtime UID expansion; no actionable merge-blocking risk remains. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/pi/install-appliance.sh`:
- Around line 132-141: Initialize console_uid to an empty value before the guard
in the appliance installation flow, so inherited environment values cannot
bypass the id -u lookup for console_user. Keep the existing dry-run fallback and
failure handling unchanged.
In `@scripts/pi/validate-appliance-boot.test.mjs`:
- Around line 56-65: Add a dry-run installer fixture using a non-1000 console
UID, then assert the rendered unit output contains /run/user/<selected UID> and
does not contain the systemd %U specifier. Keep the existing template-token
assertion, and anchor the new regression check to the fixture/rendering flow in
validate-appliance-boot.test.mjs.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 042c95d3-18d3-45fd-ba22-56d01805a72c
📒 Files selected for processing (3)
scripts/pi/install-appliance.shscripts/pi/systemd/vcg-console-session.service.inscripts/pi/validate-appliance-boot.test.mjs
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: pi bring-up script
- GitHub Check: e2e / ubuntu-latest
- GitHub Check: node / windows-latest
- GitHub Check: node / ubuntu-latest
- GitHub Check: native / windows-latest
- GitHub Check: native / aarch64 cross
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: randroid88
Repo: Randroids-Dojo/VCG-Console PR: 30
File: scripts/pi/systemd/vcg-console-session.service.in:37-37
Timestamp: 2026-08-11T03:56:33.530Z
Learning: For the Raspberry Pi appliance Bluetooth controller flow, `scripts/pi/install-appliance.sh` conditionally adds the console user to the `bluetooth` group, and the boot validator checks this enrollment. Physical Bluetooth scan, pair, connect, and remove operations as the service user remain target-hardware qualification gates.
📚 Learning: 2026-08-09T23:24:38.027Z
Learnt from: randroid88
Repo: Randroids-Dojo/VCG-Console PR: 21
File: docs/DEVELOPMENT.md:0-0
Timestamp: 2026-08-09T23:24:38.027Z
Learning: In VCG-Console, `benchmarks/tv-conformance/windows-x64-chrome-150-launcher-home-tv-conformance-v1.json` uses `package.json` as a `provenance.productionSourceTree` root. Changes to hash-pinned production-source files require regeneration of the real-Chrome TV-conformance evidence artifact and its validator expectations.
Applied to files:
scripts/pi/validate-appliance-boot.test.mjs
📚 Learning: 2026-08-11T02:09:53.271Z
Learnt from: randroid88
Repo: Randroids-Dojo/VCG-Console PR: 28
File: benchmarks/tv-conformance/windows-x64-chrome-150-launcher-home-tv-conformance-v1.json:0-0
Timestamp: 2026-08-11T02:09:53.271Z
Learning: For the TV conformance evidence in `benchmarks/tv-conformance`, the `chrome-150` artifact and generator naming can coexist with `Chrome/151.0.7922.76` browser metadata as a pre-existing repository-wide convention. Do not flag this mismatch on PRs that only refresh source or build bindings unless the PR changes the naming or browser-version convention itself.
Applied to files:
scripts/pi/validate-appliance-boot.test.mjs
🔇 Additional comments (3)
scripts/pi/install-appliance.sh (2)
142-153: LGTM!
289-289: LGTM!scripts/pi/systemd/vcg-console-session.service.in (1)
56-56: LGTM!
/simplify found the UID-resolution block added in the previous commit was
needlessly duplicative, confirmed independently by all four review angles:
- It re-ran `id "${console_user}"` a third time in this file purely as an
existence probe (console_group's block and the top-level dry_run==0 guard
already do this), then a second `id -u` call to fetch the value, when one
`id -u "${console_user}"` checked for success does both.
- console_home already parses `getent passwd`'s output for field 6 (home);
field 3 of that same record is the UID -- no second lookup needed.
- The `[ -z "${console_uid:-}" ]` guard and its numeric-format validation
copied the shape of console_group's block (which legitimately can be
pre-set via --group), but console_uid has no --uid flag and is never
pre-set, so both were dead code that could never take their "already set"
or "reject bad input" branches -- the value only ever comes from a passwd
field or a hardcoded literal, both always numeric, matching how
console_home has no such validation either.
Folds console_uid into the console_home block, sharing one `getent passwd`
call for both fields, and drops the now-dead guard and validation.
Verified: `bash -n`, the appliance-boot test suite, and a live redeploy on
the Pi 5 (see follow-up).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GaoUpejgTieMMycCoTektN
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/pi/install-appliance.sh`:
- Around line 127-135: Update the console identity resolution block around
console_home, console_uid, and passwd_record to initialize console_uid before
resolution, prefer the getent passwd record, and fall back to id -u
"${console_user}" when no passwd record is available, including when --home
explicitly supplied console_home. Preserve the dry-run fallback and only fail
when the required home or UID still cannot be determined.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 81ab4e64-f6cf-45ed-8def-36fcdb83a74d
📒 Files selected for processing (1)
scripts/pi/install-appliance.sh
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: node / windows-latest
- GitHub Check: pi bring-up script
- GitHub Check: native / windows-latest
- GitHub Check: node / ubuntu-latest
- GitHub Check: native / ubuntu-latest
- GitHub Check: e2e / ubuntu-latest
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: randroid88
Repo: Randroids-Dojo/VCG-Console PR: 30
File: scripts/pi/systemd/vcg-console-session.service.in:37-37
Timestamp: 2026-08-11T03:56:33.530Z
Learning: For the Raspberry Pi appliance Bluetooth controller flow, `scripts/pi/install-appliance.sh` conditionally adds the console user to the `bluetooth` group, and the boot validator checks this enrollment. Physical Bluetooth scan, pair, connect, and remove operations as the service user remain target-hardware qualification gates.
🔇 Additional comments (2)
scripts/pi/install-appliance.sh (2)
127-130: Initializeconsole_uidbefore this guard.An inherited environment variable can make
console_uidnon-empty. The guard then skips thegetent passwdlookup and renders an unrelated UID, which can recreate the/run/user/<UID>mount failure. Setconsole_uid=""with the other installer variables, or derive it without honoring inherited state. (raw.githubusercontent.com)
280-280: LGTM!
CodeRabbit review on PR #33: console_group and console_home are both initialized to "" up front, but console_uid was only ever referenced via ${console_uid:-}. An inherited console_uid environment variable would make the guard's -z check pass, skipping getent resolution entirely and baking the wrong UID into the rendered ReadWritePaths=.../run/user/@CONSOLE_UID@ -- recreating the exact mount-namespace failure this branch fixes. Left the "id -u" fallback suggestion for a missing getent unaddressed: getent is a core glibc component always present on the Debian/ Raspberry Pi OS trixie target this script supports, and non-dry-run mode already requires `id "${console_user}"` to succeed earlier in the script, so that fallback path is unreachable on the supported target.
Contributor
Author
|
@coderabbitai review |
|
CodeRabbit nitpick on PR #33: the existing test only asserts that the @CONSOLE_UID@ token exists in the template, not that rendering actually substitutes a real numeric UID sourced from getent (all the other fixture tests use a fictitious "vcg" user that falls through to the dry-run default of 1000, never exercising the getent lookup itself). Uses the real invoking user via --output-dir so `id -u` and the rendered ReadWritePaths= line can be compared directly end-to-end, proving the getent -> console_uid -> render pipeline works rather than just checking the placeholder token is gone. Verified live on the Pi 5 (also exercises the getent branch there, not the dry-run fallback).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/pi/validate-appliance-boot.test.mjs`:
- Around line 184-224: Update the test around the dry-run invocation to prepend
a temporary getent fixture to PATH that returns the real user’s passwd record
with UID 4242, ensuring the getent branch is exercised. Pass the modified
environment to spawnSync, and change the rendered-unit assertion to require
/run/user/4242 so the fallback UID 1000 path cannot satisfy the test.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 10203411-348f-4fd7-a7dd-dd78d23dd4f1
📒 Files selected for processing (1)
scripts/pi/validate-appliance-boot.test.mjs
📜 Review details
⏰ Context from checks skipped due to timeout. (7)
- GitHub Check: node / windows-latest
- GitHub Check: native / windows-latest
- GitHub Check: native / aarch64 cross
- GitHub Check: node / ubuntu-latest
- GitHub Check: e2e / ubuntu-latest
- GitHub Check: native / ubuntu-latest
- GitHub Check: pi bring-up script
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: randroid88
Repo: Randroids-Dojo/VCG-Console PR: 33
File: scripts/pi/install-appliance.sh:128-136
Timestamp: 2026-08-12T21:23:10.050Z
Learning: For `scripts/pi/install-appliance.sh`, the supported non-dry-run target is Debian/Raspberry Pi OS trixie. `getent` is available on this target. The installer validates `console_user` with `id "${console_user}"` before console home and UID resolution, so an `id -u` fallback for a missing `getent` executable is not required.
🔇 Additional comments (1)
scripts/pi/validate-appliance-boot.test.mjs (1)
3-5: LGTM!
CodeRabbit review on PR #33's own test commit: the previous version compared against the real invoking user's UID, which on many CI runners/dev boxes happens to be 1000 -- the exact same value the dry-run fallback would produce, so the test could pass even if getent resolution were silently broken and it fell through to the fallback. Stub `getent` on PATH to return a synthetic passwd record with UID 4242 for a synthetic username, so the assertion can only pass if the real getent branch actually ran and resolved that value. Verified live on the Pi 5.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/pi/validate-appliance-boot.test.mjs`:
- Around line 179-183: Update the platform guard in the appliance boot
validation test to return early whenever process.platform is not "linux",
ensuring the test runs only on Linux while preserving its existing behavior
there.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1bbf13e8-8ae4-4d14-ade2-6eef59b86902
📒 Files selected for processing (1)
scripts/pi/validate-appliance-boot.test.mjs
📜 Review details
⏰ Context from checks skipped due to timeout. (7)
- GitHub Check: native / ubuntu-latest
- GitHub Check: native / aarch64 cross
- GitHub Check: native / windows-latest
- GitHub Check: e2e / ubuntu-latest
- GitHub Check: node / windows-latest
- GitHub Check: node / ubuntu-latest
- GitHub Check: pi bring-up script
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: randroid88
Repo: Randroids-Dojo/VCG-Console PR: 33
File: scripts/pi/install-appliance.sh:128-136
Timestamp: 2026-08-12T21:23:10.050Z
Learning: For `scripts/pi/install-appliance.sh`, the supported non-dry-run target is Debian/Raspberry Pi OS trixie. `getent` is available on this target. The installer validates `console_user` with `id "${console_user}"` before console home and UID resolution, so an `id -u` fallback for a missing `getent` executable is not required.
📚 Learning: 2026-08-12T21:23:10.050Z
Learnt from: randroid88
Repo: Randroids-Dojo/VCG-Console PR: 33
File: scripts/pi/install-appliance.sh:128-136
Timestamp: 2026-08-12T21:23:10.050Z
Learning: For `scripts/pi/install-appliance.sh`, the supported non-dry-run target is Debian/Raspberry Pi OS trixie. `getent` is available on this target. The installer validates `console_user` with `id "${console_user}"` before console home and UID resolution, so an `id -u` fallback for a missing `getent` executable is not required.
Applied to files:
scripts/pi/validate-appliance-boot.test.mjs
📚 Learning: 2026-08-09T23:24:38.027Z
Learnt from: randroid88
Repo: Randroids-Dojo/VCG-Console PR: 21
File: docs/DEVELOPMENT.md:0-0
Timestamp: 2026-08-09T23:24:38.027Z
Learning: In VCG-Console, `benchmarks/tv-conformance/windows-x64-chrome-150-launcher-home-tv-conformance-v1.json` uses `package.json` as a `provenance.productionSourceTree` root. Changes to hash-pinned production-source files require regeneration of the real-Chrome TV-conformance evidence artifact and its validator expectations.
Applied to files:
scripts/pi/validate-appliance-boot.test.mjs
🔇 Additional comments (3)
scripts/pi/validate-appliance-boot.test.mjs (3)
3-3: LGTM!
58-67: LGTM!
185-207: LGTM!Also applies to: 217-220, 232-240
CodeRabbit review on PR #33: process.platform === "win32" only skips Windows, leaving the test to run on macOS too, where getent doesn't exist system-wide -- the stub on PATH would still work there since it shadows any real getent, but the installer's actual non-dry-run target is Debian/Raspberry Pi OS trixie, so scope the guard to what the test is actually modeling. Re-verified live on the Pi 5.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found live on the Pi 5 immediately after deploying the previous fix (#32's `ReadWritePaths=/run/user/%U`): the session crash-looped a different way this time:
```
Failed to set up mount namespacing: /run/user/0: No such file or directory
Failed at step NAMESPACE spawning .../wait-for-console.sh: No such file or directory
```
systemd's `%U` specifier is documented to expand to the UID of the unit's configured `User=`, but resolved to `0` (root) for this unit's mount-namespace setup phase on this systemd version (257.13) — not
randroid's 1000. Whatever the exact cause, relying on it clearly isn't reliable here.Sidesteps the specifier entirely: `install-appliance.sh` now resolves the console user's numeric UID itself (`id -u`) at install time — the same way it already resolves the group and home directory — validates it's numeric, and templates it directly as `@CONSOLE_UID@` instead of leaving `%U` for systemd to resolve at runtime.
Verified end-to-end live on the Pi: after reinstalling with this fix, `systemctl status vcg-console-session.service` shows `active (running)`, stable (no restart-loop), with a full healthy process tree (`cage -> vcg-host -> chromium` with normal renderer/GPU/utility children), and the VCG Console UI renders correctly on the connected display.
🤖 Generated with Claude Code