Skip to content

fixups for a weird environment#213

Closed
grahamc wants to merge 3 commits intomainfrom
push-klssqsuyspyn
Closed

fixups for a weird environment#213
grahamc wants to merge 3 commits intomainfrom
push-klssqsuyspyn

Conversation

@grahamc
Copy link
Copy Markdown
Member

@grahamc grahamc commented Dec 2, 2025

Description
Checklist
  • Tested changes against a test repository
  • Added or updated relevant documentation (leave unchecked if not applicable)
  • (If this PR is for a release) Updated README to point to the new tag (leave unchecked if not applicable)

Summary by CodeRabbit

  • Bug Fixes
    • Improved root detection on Linux so privilege escalation and KVM setup behave more reliably.
    • Ensured daemon startup consistently chooses direct or elevated launch based on accurate root checks.
    • Made trusted-user handling more robust: errors now log a warning and avoid adding unintended extra newlines.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Dec 2, 2025

Important

Review skipped

Review was skipped due to path filters

⛔ Files ignored due to path filters (1)
  • dist/index.js is excluded by !**/dist/**

CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including **/dist/** will override the default block on the dist directory, by removing the pattern from both the lists.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

Replaced uid-based root checks with geteuid where available, updated daemon spawn to use that root determination, and wrapped trusted-user line handling in try/catch to log failures and avoid unconditional newline insertion on error.

Changes

Cohort / File(s) Summary
Root detection & spawn behavior
src/index.ts
Replaced userInfo().uid === 0 check with process.geteuid() === 0 (when available) for setupKvm and used the same isRoot result to decide whether to invoke the daemon binary directly or prefix with sudo in spawnDetached.
trusted-users handling & error logging
src/index.ts
Wrapped the logic that appends trusted-user lines in a try/catch; on error it logs a warning and avoids adding an extra newline unconditionally in the catch path.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Review process.geteuid() availability and fallback behavior.
  • Verify the sudo vs direct invocation logic in spawnDetached matches intended privilege semantics.
  • Confirm the try/catch around trusted-user handling logs appropriate details and preserves newline behavior only on success.

Suggested reviewers

  • gustavderdrache
  • colemickens

Poem

🐰 I hopped through code, ears in the breeze,
I checked for geteuid beneath the trees.
If root's about, the daemon runs straight,
otherwise sudo opens the gate.
A gentle catch keeps newlines light — hooray, good night! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title "fixups for a weird environment" is vague and generic, lacking specific details about the actual changes made to the codebase. Replace with a more descriptive title that specifically mentions the main changes, such as "Improve root detection and error handling for trustRunnerUser" or similar.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/index.ts (1)

848-849: Good change: geteuid() is more appropriate for privilege checks.

Using process.geteuid() (effective UID) instead of userInfo().uid (real UID) is the correct approach for determining current privileges, especially in environments with setuid binaries or privilege escalation. The type guard properly handles platforms where geteuid is unavailable.

For consistency, consider updating the similar check at line 555 in the spawnDetached function, which still uses userInfo().uid === 0:

-    if (userInfo().uid === 0) {
+    const isRoot = typeof process.geteuid === "function" && process.geteuid() === 0;
+    if (isRoot) {
       executable = daemonBin;
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e4fb5e6 and 129a4bc.

⛔ Files ignored due to path filters (1)
  • dist/index.js is excluded by !**/dist/**
📒 Files selected for processing (1)
  • src/index.ts (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
  • GitHub Check: Test: macos-14-large with determinate
  • GitHub Check: Test: macos-14-xlarge
  • GitHub Check: Test: macos-14-xlarge with determinate
  • GitHub Check: Test: namespace-profile-default-arm64 with determinate
  • GitHub Check: Test: macos-13-large with determinate
  • GitHub Check: Test: macos-13-large
  • GitHub Check: Test: macos-14-large
  • GitHub Check: Test: nscloud-ubuntu-22.04-amd64-4x16
  • GitHub Check: Test: namespace-profile-default-arm64
  • GitHub Check: Test: ubuntu-latest
  • GitHub Check: Check the dist/ folder is up to date

@grahamc grahamc closed this Feb 26, 2026
@grahamc grahamc deleted the push-klssqsuyspyn branch February 26, 2026 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant