Skip to content

Fix for issue 4820 - #6053

Open
mohamedmastouri-hue wants to merge 3 commits into
pingdotgg:mainfrom
mohamedmastouri-hue:fix-issue-4820
Open

Fix for issue 4820#6053
mohamedmastouri-hue wants to merge 3 commits into
pingdotgg:mainfrom
mohamedmastouri-hue:fix-issue-4820

Conversation

@mohamedmastouri-hue

@mohamedmastouri-hue mohamedmastouri-hue commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Fixes issue #4820


Note

Low Risk
Display-only labeling change with file read and env lookup; no auth, security, or data-path impact.

Overview
Server environment labels now distinguish Windows Subsystem for Linux when friendly host names are unavailable and the resolver falls back to the hostname.

On Linux, a new isWslEnvironment check reads /proc/sys/kernel/osrelease for microsoft or wsl. If WSL is detected, the label becomes hostname (WSL: <distro>) when WSL_DISTRO_NAME is set and non-empty, or hostname (WSL) otherwise. HostProcessEnvironment is wired in for distro name lookup; tests stub HostProcessEnvironment via an extended withHostPlatform helper and cover both named and generic WSL cases.

Reviewed by Cursor Bugbot for commit 84346fe. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Append WSL distro info to server environment label when running under WSL

  • Adds isWslEnvironment in ServerEnvironmentLabel.ts that detects WSL by checking /proc/sys/kernel/osrelease for 'microsoft' or 'wsl' (case-insensitive); file errors are logged at debug level and treated as non-WSL.
  • Updates resolveServerEnvironmentLabel to append (WSL: <distro>) when WSL_DISTRO_NAME is set, or (WSL) when not, for Linux hosts running under WSL.
  • Adds tests covering both cases using a mocked WslLayer and HostProcessEnvironment.

Macroscope summarized 84346fe.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Aug 10, 2026

@macroscopeapp macroscopeapp Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed the new WSL detection logic in apps/server/src/environment/ServerEnvironmentLabel.ts against the Effect service conventions. Two findings: the filesystem failure is swallowed untyped instead of being modeled/logged like the adjacent probe, and the new backend behavior has no focused test coverage.

Posted via Macroscope — Effect Service Conventions

Comment on lines +214 to +222
const isWsl = yield* isWslEnvironment();
if (isWsl) {
const env = yield* HostProcessEnvironment;
const distroName = env["WSL_DISTRO_NAME"];
if (distroName && distroName.trim().length > 0) {
return `${hostname} (WSL: ${distroName.trim()})`;
}
return `${hostname} (WSL)`;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This changes the resolved server environment label (a backend behavior change) but ServerEnvironmentLabel.test.ts is untouched. Consider adding focused cases there using the existing FileSystem.layerNoop seam plus Layer.succeed(HostProcessEnvironment, { WSL_DISTRO_NAME: "Ubuntu" }): WSL with a distro name, WSL without one, and a non-WSL Linux host that still returns the plain hostname.

Posted via Macroscope — Effect Service Conventions

Comment thread apps/server/src/environment/ServerEnvironmentLabel.ts
@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 21552c32-870f-4a33-93f6-e6a17e2d8c44

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

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

@macroscopeapp

macroscopeapp Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

This PR introduces new runtime behavior by adding WSL detection to server environment labels. While well-tested and self-contained, it adds new capability rather than fixing a bug, and the author is new to this file. Human review recommended to validate the approach.

You can customize Macroscope's approvability policy. Learn more.

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 4a4420a. Configure here.

Comment thread apps/server/src/environment/ServerEnvironmentLabel.test.ts
@CDVolvik

Copy link
Copy Markdown
Contributor

Verified on Linux/WSL against this PR's head (tested 2026-08-11):

apps/server · src/environment/ServerEnvironmentLabel.test.ts10/10 pass.

I hit the same bug first-hand on a stock Ubuntu WSL2 install before this PR existed: /etc/hostname and os.hostname() both return the Windows computer name (GalacticCow / GALACTICCOW), /etc/machine-info is absent, and hostnamectl --pretty is empty, so the label chain lands on the mirrored Windows name. Two distros on one host are also indistinguishable without the distro name in the label.

A few notes from reading the approach:

  1. Detecting WSL via /proc/sys/kernel/osrelease is stronger than env-only when WSL_DISTRO_NAME is unset, and the generic hostname (WSL) fallback is the right shape for that case.
  2. Suffix only on the plain-hostname path (after friendly/pretty resolution fails) is the important constraint — a deliberate PRETTY_HOSTNAME should stay untouched. Your tests cover the hostname-fallback cases; a pin that "pretty name does not get a WSL suffix" would make that invariant visible to the next editor.
  3. Reuse of HostProcessEnvironment matches how externalLauncher already reads WSL_DISTRO_NAME. Worth keeping the test helper defaulting env to empty so Linux CI and WSL hosts cannot diverge on accidental real process.env.

I am not filing a competing PR for #4820.

@CDVolvik CDVolvik left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Verified on Linux (WSL2 Ubuntu, Node 24).

Branch as submitted:

✓ src/environment/ServerEnvironmentLabel.test.ts (10 tests)
  Tests  10 passed (10)

Swapping only ServerEnvironmentLabel.ts for the current main copy, keeping this PR's tests:

× appends WSL distro name when WSL_DISTRO_NAME is present
  AssertionError: expected 'WINDOWS-HOST' to be 'WINDOWS-HOST (WSL: Ubuntu-22.04)'
× appends generic WSL label when WSL_DISTRO_NAME is absent
  AssertionError: expected 'WINDOWS-HOST' to be 'WINDOWS-HOST (WSL)'

  Tests  2 failed | 8 passed (10)

Both new tests fail without the source change and pass with it, and the other eight are untouched, so they pin the added behaviour specifically.

Worth adding that the premise holds on a real setup, which is the part unit tests cannot show. On a Windows host running a WSL2 distro, hostname inside the distro returns the Windows machine name verbatim, and WSL_DISTRO_NAME is populated. So before this change the two environments are genuinely indistinguishable by label, and WSL_DISTRO_NAME is the right discriminator to reach for. That matches what #4820 describes.

One thing to confirm: WSL_DISTRO_NAME is set by the WSL init path, so it is present in an interactive shell and in anything launched from one, but it is not guaranteed for a process started through other entry points. The generic (WSL) fallback covers that, which is the right call, though it does mean the distro name is best-effort rather than always present.

@inayayousfi

Copy link
Copy Markdown
Contributor

Written by inayayousfi, typed by gpt-5.6-sol running in OpenCode.
Every call here is inayayousfi's, and no agent acted on its own.

PR #7267 is a related broader naming implementation that is now ready for review. It lets users assign a server-owned name from web, desktop, or mobile, including for remote environments, then propagates that name to connected clients and T3 Connect. It handles the WSL collision once the user chooses distinct names, and it also covers hostname collisions outside WSL.

This PR still provides something #7267 does not: automatic WSL detection and a zero-configuration distro suffix. I hope this focused fix can land alongside the manual rename support.

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

Labels

size:M 30-99 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants