Skip to content

Workstation improvements#264

Open
ptone wants to merge 35 commits into
GoogleCloudPlatform:mainfrom
ptone:workstation-improvements
Open

Workstation improvements#264
ptone wants to merge 35 commits into
GoogleCloudPlatform:mainfrom
ptone:workstation-improvements

Conversation

@ptone
Copy link
Copy Markdown
Member

@ptone ptone commented May 31, 2026

Add an improved onboarding flow for first time users

Scion added 24 commits May 31, 2026 01:13
Add workstation-only system endpoints gated by requireWorkstation:

- GET /system/check: GatherDiagnostics returns structured results (git,
  runtime, config checks) with a ready flag
- GET /system/runtime: detect available runtime, return configured profile
- PUT /system/runtime: validate and persist runtime choice
- GET /system/status: ComputeOnboardingStatus for first-run wizard
- POST /system/init: call InitMachine with user-selected harnesses
- PUT /system/identity: already wired in Phase 1

All endpoints require loopback origin and return JSON.
Adds comprehensive tests for the workstation onboarding endpoints and
security primitives introduced in phases 0-5:

- requireWorkstation middleware: 404 when disabled, pass-through when enabled
- assertLoopback: table-driven IPv4/IPv6 loopback validation
- ClassifyPath: managed path detection, legacy groves path, AlreadyLinked via store
- POST /system/init: valid harnesses, unknown harness rejection, empty list rejection
- PUT /system/identity: writes and echoes display name and email
- POST /system/fs/validate-path: managed-path overlap error, normal path classification
- GET /system/fs/list: home directory listing, hidden file filtering, outside-home rejection

Also adds missing server.auth.display_name, server.auth.email, and
server.auth.username key handling in UpdateVersionedSetting, which the
identity endpoint depends on.
Replace the "not yet able to provide pre-built binaries" note with a
Homebrew quick start path. Lead with `brew install scion` + `scion
server start` which opens the onboarding wizard, then keep the existing
go install path as "Install from Source". Add a tip noting that the
wizard handles machine init automatically.
@google-cla
Copy link
Copy Markdown

google-cla Bot commented May 31, 2026

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a workstation onboarding wizard and supporting APIs to enable a guided, browser-based first-run setup for local Scion installations. It adds workstation-only security fences, configurable cosmetic identity, system status and diagnostic endpoints, Go-native image pulling and local building, and a server-side directory browser for linking local workspaces. The review feedback highlights several critical issues to address: Windows compatibility bugs in the directory browser (path separator normalization, drive letter breadcrumbs, and drive root parent navigation), an infinite spinner on early image pull failures, a potential silent failure in log scanning with bufio.Scanner, the need to gracefully handle context cancellation in the image pull loop, and a lack of concurrency control for local image builds.

Comment on lines +1004 to +1025
if (mode === 'pull' && d['image']) {
const image = d['image'] as string;
const status = d['status'] as string;
const error = d['error'] as string | undefined;

const harness = this.imageNameToHarness(image);
if (harness) {
const next = new Map(this.imageStatuses);
const entry: { status: string; error?: string } = { status };
if (error) entry.error = error;
next.set(harness, entry);
this.imageStatuses = next;
}

if (status === 'done' || status === 'exists' || status === 'error') {
doneCount++;
if (doneCount >= totalImages) {
this.imagePulling = false;
this.cleanupImageEvents();
}
}
}
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.

high

The frontend ignores pull error events that do not contain an image key (such as top-level errors published by the backend when PullImages fails early). This causes the UI spinner to spin indefinitely. Update the event listener to handle top-level error events.

        if (mode === 'pull') {
          if (d['image']) {
            const image = d['image'] as string;
            const status = d['status'] as string;
            const error = d['error'] as string | undefined;

            const harness = this.imageNameToHarness(image);
            if (harness) {
              const next = new Map(this.imageStatuses);
              const entry: { status: string; error?: string } = { status };
              if (error) entry.error = error;
              next.set(harness, entry);
              this.imageStatuses = next;
            }

            if (status === 'done' || status === 'exists' || status === 'error') {
              doneCount++;
              if (doneCount >= totalImages) {
                this.imagePulling = false;
                this.cleanupImageEvents();
              }
            }
          } else if (d['status'] === 'error') {
            this.error = (d['error'] as string) || 'An error occurred during image pull.';
            this.imagePulling = false;
            this.cleanupImageEvents();
          }
        }

Comment on lines +196 to +197
this.currentPath = data.path;
this.entries = data.entries ?? [];
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.

high

Windows path separators (\\) returned by the backend are not normalized on the frontend, which breaks path splitting, breadcrumbs, and navigation on Windows. Normalize the path separators to / upon receiving them from the API.

Suggested change
this.currentPath = data.path;
this.entries = data.entries ?? [];
this.currentPath = data.path.replace(/\\/g, '/');
this.entries = data.entries ?? [];

Comment on lines +218 to +222
private navigateToBreadcrumb(index: number): void {
const segments = this.currentPath.split('/').filter(Boolean);
const path = '/' + segments.slice(0, index + 1).join('/');
void this.navigate(path);
}
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.

high

When navigating to a breadcrumb on Windows, prepending / to a drive letter (e.g., /C:) produces an invalid path. Check if the first segment is a Windows drive letter and handle it accordingly.

Suggested change
private navigateToBreadcrumb(index: number): void {
const segments = this.currentPath.split('/').filter(Boolean);
const path = '/' + segments.slice(0, index + 1).join('/');
void this.navigate(path);
}
private navigateToBreadcrumb(index: number): void {
const segments = this.currentPath.split('/').filter(Boolean);
const subSegments = segments.slice(0, index + 1);
let path = '';
if (subSegments[0] && /^[a-zA-Z]:$/.test(subSegments[0])) {
path = subSegments.join('/');
if (subSegments.length === 1) {
path += '/';
}
} else {
path = '/' + subSegments.join('/');
}
void this.navigate(path);
}

Comment on lines +286 to +291
${segments.length > 0 ? html`
<div class="entry" @click=${() => this.navigateUp()}>
<sl-icon name="arrow-up"></sl-icon>
<span class="name">..</span>
</div>
` : nothing}
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.

medium

On Windows, the root of a drive (e.g., C:/) has a segment length of 1, causing the browser to incorrectly show a .. navigation entry that leads to an invalid C: path. Avoid rendering the .. entry when at the root of a Windows drive.

Suggested change
${segments.length > 0 ? html`
<div class="entry" @click=${() => this.navigateUp()}>
<sl-icon name="arrow-up"></sl-icon>
<span class="name">..</span>
</div>
` : nothing}
${!(segments.length === 0 || (segments.length === 1 && /^[a-zA-Z]:$/.test(segments[0]))) ? html`
<div class=

Comment on lines +516 to +519
scanner := bufio.NewScanner(stdout)
for scanner.Scan() {
s.events.PublishRaw(subject, imageBuildLogEvent{Type: "log", Line: scanner.Text()})
}
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.

medium

bufio.Scanner can silently fail and stop scanning if a log line exceeds its default 64KB buffer limit (returning bufio.ErrTooLong). Checking scanner.Err() is necessary to ensure any scan errors are reported to the user.

		scanner := bufio.NewScanner(stdout)
		for scanner.Scan() {
			s.events.PublishRaw(subject, imageBuildLogEvent{Type: "log", Line: scanner.Text()})
		}
		if err := scanner.Err(); err != nil {
			s.events.PublishRaw(subject, imageBuildLogEvent{Type: "log", Line: "error reading build log: " + err.Error()})
		}

Comment thread pkg/runtime/imagepull.go
Comment on lines +65 to +66
for _, img := range images {
exists, err := rt.ImageExists(ctx, img)
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.

medium

If the server context is cancelled (e.g., during shutdown), the image pull loop continues to iterate through all remaining images, triggering unnecessary error events. Checking ctx.Err() at the start of each iteration allows the loop to terminate gracefully.

	for _, img := range images {
		if err := ctx.Err(); err != nil {
			return err
		}
		exists, err := rt.ImageExists(ctx, img)

Line string `json:"line"`
}

func (s *Server) handleSystemImagesBuild(w http.ResponseWriter, r *http.Request) {
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.

medium

There is no concurrency control or check for an active build job in handleSystemImagesBuild. Multiple concurrent POST requests will spawn multiple concurrent build-images.sh processes, which can easily overwhelm the workstation. Consider tracking whether a build job is currently active (e.g., using an atomic boolean or a mutex-protected flag on the Server struct) and returning 409 Conflict if a build is already in progress, as specified in the design document.

Scion added 7 commits May 31, 2026 03:09
Fixes fmt-check failures across multiple packages including
telegram plugin, agent-viz, chat-app, hub, messages, and runtimebroker.
The @State() property was assigned but never read in the template,
causing TypeScript's noUnusedLocals check to fail.
- Use type conversion instead of struct literal for identical
  systemIdentityRequest/Response types (staticcheck S1016)
- Check error return from srv.Shutdown in test cleanup (errcheck)
…al ports

SCION_PROJECT_ID was added to IsHubContext() but tests clearing hub
env vars were not updated, causing false hub context detection and
test failures in pkg/config, pkg/hubsync, and cmd packages.

Also switch telemetry pipeline tests from hardcoded ports to port 0
(OS-assigned ephemeral ports) to eliminate flaky port-conflict failures
from TCP TIME_WAIT between sequential tests.
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