Skip to content

fix(shell): tolerate unrelated nsfs mountinfo entries in the private kernel check - #5734

Merged
decode2 merged 2 commits into
Gentleman-Programming:mainfrom
decode2:fix/5733-mountinfo-nsfs
Oct 24, 2026
Merged

decode2 merged 2 commits into
Gentleman-Programming:mainfrom
decode2:fix/5733-mountinfo-nsfs

Conversation

@decode2

@decode2 decode2 commented Oct 24, 2026 •

Copy link
Copy Markdown
Member

🔗 Linked Issue

Closes #5733


🏷️ PR Type

What kind of change does this PR introduce?

  • type:bug — Bug fix (non-breaking change that fixes an issue)
  • type:feature — New feature (non-breaking change that adds functionality)
  • type:docs — Documentation only
  • type:refactor — Code refactoring (no functional changes)
  • type:chore — Build, CI, or tooling changes
  • type:breaking-change — Breaking change (fix or feature that changes existing behavior)

📝 Summary

  • The private-install unit kernel check parsed every /proc/self/mountinfo line and required both root and mountpoint to be canonical absolute paths. On stock Ubuntu GNOME desktops, snapd-desktop-integration always contributes an nsfs mount whose root field is a namespace identifier (mnt:[4026532518]), so every private gentle-shell installation from the TUI failed as uncertain at step 5/5 (bug(shell): private install launch probe fails on every stock Ubuntu desktop — mountinfo parser rejects snapd nsfs mounts ("mnt:[...] is not canonical") #5733).
  • The parser now consumes the root field only for the trusted cgroup2 mount line it actually resolves. The mountpoint stays strictly validated for every line (shadow/duplicate classification still requires it), and noncanonical roots or mountpoints on the trusted cgroup2 line still refuse with the same messages as before.

📂 Changes

File / Area What Changed
internal/shellinstaller/user_runtime_primitives_linux.go privateCgroupPath validates the root field only on the trusted cgroup2 mount line; unrelated lines classify by mountpoint only
internal/shellinstaller/user_runtime_primitives_linux_test.go New mapping cases: unrelated nsfs mount accepted, unrelated noncanonical root accepted, unrelated noncanonical mountpoint refused

🤖 AI Assistance

Select exactly one option. Do not check both options.

  • None — No material AI assistance was used.
  • Material assistance used — Complete all applicable declaration fields below.

Tool/model (if known): GLM 5.3 (pi coding agent)

Material scope: Diagnosis of the mountinfo refusal against a user report; implementation of the parser relaxation and its test cases in internal/shellinstaller; red/green verification via a temporary uncommitted harness calling privateCgroupPath directly.

Verification performed: Red reproduction of the exact mnt:[4026532518] refusal before the fix; green after it with shadow/duplicate and noncanonical trusted-line refusals preserved; go test ./..., go run ./internal/gofmtcheck, go vet ./internal/shellinstaller and go build ./... all pass locally.

Trivial formatting, spelling, minor autocomplete, search/navigation, and trivial, non-substantive mechanical transformations do not need to be itemized. See AI_POLICY.md for the canonical policy.


🧪 Test Plan

Unit Tests

go test ./...

Go Format

go run ./internal/gofmtcheck

E2E Tests (Docker required)

cd e2e && ./docker-test.sh

Benchmark Validation

See the benchmark guide. Benchmark validation applies to review-lifecycle, gates, recovery, delivery, benchmark implementation/corpus/classifier, and benchmark-claim changes. For unrelated changes, explain N/A in the Test Plan.

  • Unit tests pass (go test ./...)
  • Go format passes (go run ./internal/gofmtcheck)
  • E2E tests pass (cd e2e && ./docker-test.sh) — validated by the CI e2e-docker job; local Docker is unavailable in this environment, and the new Guest-mode cases (TestPrivateCgroupMapping) run in the bounded shell-linux-first-ci container
  • Manually tested locally — red/green harness exercised privateCgroupPath with the snapd nsfs line and noncanonical variants before committing

Benchmark validation: N/A — no benchmark, review-lifecycle, gate, or delivery changes.


🤖 Automated Checks

The following checks run automatically on this PR:

Check Status Description
Check PR Cognitive Load ⏳ PR should stay within 400 changed lines (additions + deletions) or use size:exception
Check Issue Reference ⏳ PR body must contain a visible, well-formed closing or non-closing reference to a numeric issue in this base repository
Check Issue Has status:approved ⏳ Linked issue must have been approved before work began
Check PR Has type:* Label ⏳ Exactly one type:* label must be applied
Unit Tests ⏳ go test ./... must pass
Go Format ⏳ go run ./internal/gofmtcheck must pass
E2E Tests ⏳ cd e2e && ./docker-test.sh must pass

✅ Contributor Checklist

  • PR is linked to an issue with status:approved
  • PR stays within 400 changed lines, or canonical size:exception authority (current direct human instruction for the exact target/action, verified actor MAINTAIN/ADMIN, human choice and documented rationale) and observed readback are recorded
  • Target-host read-back confirms exactly one appropriate type:* label; any mutation followed the canonical issue-creation contract and reviewed CONTRIBUTING.md catalog (existing types preserved; conflicts deferred to the human)
  • Unit tests pass (go test ./...)
  • Go format passes (go run ./internal/gofmtcheck)
  • E2E tests pass (cd e2e && ./docker-test.sh)
  • Benchmark validation completed, or this change is not applicable to the benchmark (explain why in the Test Plan).
  • If behavior changed, docs in docs/ are updated in the same PR (reference docs track main)
  • My commits follow Conventional Commits format
  • I understand, reviewed, and take responsibility for the complete submission
  • I selected exactly one AI-assistance option and, if material assistance was used, completed all applicable declaration fields
  • My commits do not include Co-Authored-By trailers

Summary by CodeRabbit

  • Bug Fixes
    • Improved Linux cgroup mapping reliability when unrelated mounts have unusual root values, including namespace-related mounts.
    • Preserved mapping restrictions for noncanonical mountpoints and the trusted cgroup mount, helping ensure mappings are applied only to valid locations.

@decode2 decode2 added the type:bug Bug fix label Oct 24, 2026
@coderabbitai

coderabbitai Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Repository UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: d5a087c9-3d09-412b-a740-f22adeb079fe
Commits

Reviewing files that changed from the base of the PR and between fe84c60 and 5a3a9b1.

Files selected for processing: 1
  • internal/shellinstaller/user_runtime_primitives_linux_test.go

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.

Walkthrough

privateCgroupPath now parses the root only for the /sys/fs/cgroup mount. It retains canonical-root validation for that mount and adds tests for unrelated mount entries and noncanonical mountpoints.

Changes: Private cgroup mountinfo parsing

Layer / File(s) Summary
Mountinfo validation and test coverage
internal/shellinstaller/user_runtime_primitives_linux.go, internal/shellinstaller/user_runtime_primitives_linux_test.go
privateCgroupPath checks the mountpoint before parsing the root. Unrelated mount roots no longer prevent cgroup mapping, while the /sys/fs/cgroup root must remain canonical. Tests cover unrelated mount roots and a noncanonical mountpoint.

Priority: Normal

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix · Severity of issue fixed: Medium

Possibly related issues

  • Issue 5733 reports that an unrelated nsfs mount root causes the private installation launch probe to fail. This change allows unrelated mount roots while retaining validation for the cgroup mount.

Suggested reviewers: alan-thegentleman

Merge Risk: Minimal · up to 5a3a9

The change correctly addresses unrelated mountinfo roots without weakening validation of the trusted cgroup mount.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: allowing unrelated nsfs mountinfo entries during the private kernel check.
Linked Issues check Passed The PR addresses the coding requirements in #5733. privateCgroupPath validates the root field only for /sys/fs/cgroup, while it still validates mountpoints on every line. The trusted cgroup2 line …
Out of Scope Changes check Passed The changes are limited to privateCgroupPath and focused tests in internal/shellinstaller. The implementation and tests directly support #5733. No unrelated functional change is identified.
Full details: Linked Issues check

Explanation

The PR addresses the coding requirements in #5733. privateCgroupPath validates the root field only for /sys/fs/cgroup, while it still validates mountpoints on every line. The trusted cgroup2 line retains canonical-root checks. TestPrivateCgroupMapping covers unrelated nsfs, unrelated noncanonical roots, and unrelated noncanonical mountpoints. The supplied PR evidence reports passing automated checks.

Finishing Touches: 💡 1
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@decode2

decode2 commented Oct 24, 2026

Copy link
Copy Markdown
Member Author

CI triage for the two fail checks (head 5a3a9b1)

Linux installation and recovery qualification — pre-existing, not caused by this branch. The Guest full-harness fails with a byte-identical reason on feat/root-install-optin (already merged: "actual cold acquisition not observed; installer exited 1 after 1.0s output=' || Error: private installation: refused | Error: private installation: uncertain: operation "gentleshell….slice""). The same workflow failed on every PR branch today (fix/5726-windows-reconfigure-confirm, feat/shell-unix-channel-overlay, fix/5713-kernel-check-causes, fix/5714-install-deadline-bound).

The parts of that job that exercise this change passed on this head: the bounded UID65532 Guest primitives run (TestPrivateCgroupMapping, TestPrivateKernel, …) reports PASS, including the new unrelated-nsfs and refusal cases.

macOS installation and recovery qualification — also failing on current main (today's 06:44 and 07:26 runs, user_channel_overlay_unix assertions), untouched by this diff (linux-only files).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:bug Bug fix

Projects

None yet

1 participant