Skip workspace chown when agent already runs as non-root

[mfreeman451]

Summary

• skip workspace ownership changes when the process already runs as the target non-root user
• keep the existing clone path behavior for root-owned startup cases
• add focused coverage for the non-root no-op path

Testing

• go test ./cmd/sciontool/commands -run 'Test.*(Workspace|Chown|Clone|NonRoot).*'\n

[mfreeman451]

lgtm

[mfreeman451]

lgtm

[ptone]

PR #118 Review: Skip workspace chown when agent already runs as non-root

Executive Summary

Low-risk, well-scoped refactor. This PR extracts the workspace ownership logic into a testable helper function and correctly skips chown when the process is already running as a non-root user — a practical fix for restricted Kubernetes pods using securityContext. The behavioral change from os.Getuid() to os.Geteuid() is deliberate and correct for this use case.

Critical Issues

None.

Observations

1. Behavioral change: Getuid() → Geteuid() (intentional, worth noting)

File: cmd/sciontool/commands/init.go, line 1005

The old code used os.Getuid() == 0 to decide whether to chown. The new code uses os.Geteuid(). This is actually a correct improvement — Geteuid() reflects the effective UID (e.g., after setuid), which is what determines whether chown(2) will succeed. In container runtimes the two are almost always identical, but Geteuid is the semantically correct check.

No action needed — just documenting the change for reviewers.

2. Missing test: uid <= 0 early-return path

The new ensureWorkspaceOwnership function has three branches:

1. uid <= 0 → return (no chown)
2. currentEUID != 0 → log and return (no chown)
3. root → call chown

Tests cover paths (2) and (3) but not path (1). A simple test would improve completeness:

func TestEnsureWorkspaceOwnership_SkipsChownWhenUIDZero(t *testing.T) {
    chownCalled := false
    chown := func(string, int, int) error {
        chownCalled = true
        return nil
    }
    ensureWorkspaceOwnership("/workspace", 0, 0, 0, chown)
    if chownCalled {
        t.Fatal("expected chown to be skipped when uid <= 0")
    }
}

3. Minor: deleted blank line between test functions

File: cmd/sciontool/commands/init_test.go, line 747

A blank line separating TestGitCloneWorkspace_NonZeroUIDChownsWorkspace and TestConfigureGitCommand_SkipsCredentialOverrideWhenAlreadyRunningAsTargetUser was removed. This is cosmetic and doesn't affect anything, but the Go convention is to keep a blank line between top-level declarations. Non-blocking.

4. Chown error is logged but not returned

In the extracted function, a failed chown logs an error but does not return it. This matches the pre-existing behavior — the original inline code also swallowed the error. Not a regression, just noting that this is a deliberate design choice (fail-open for workspace setup).

Positive Feedback

• Dependency injection of chown via a function parameter is a clean pattern that makes the function fully unit-testable without root privileges or filesystem side effects.
• Clear, descriptive log message when skipping chown (log.Info with UID and path) will aid debugging in restricted K8s environments.
• Good separation of concerns — extracting ensureWorkspaceOwnership keeps gitCloneWorkspace focused on git operations.
• The test names are descriptive and follow Go conventions.

Final Verdict

Approve. The change is correct, minimal, well-tested, and improves both testability and Kubernetes compatibility. The missing test for the uid <= 0 path is a nice-to-have but not blocking.