Skip to content

fix(dockerless): preserve image environment after credential cleanup - #1447

Draft
skevetter wants to merge 2 commits into
mainfrom
fix/dockerless-runtime-path
Draft

skevetter wants to merge 2 commits into
mainfrom
fix/dockerless-runtime-path

Conversation

@skevetter

@skevetter skevetter commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Dockerless credential cleanup currently runs after the built image's environment is applied. Cleanup restores the builder's PATH and DOCKER_CONFIG, so Kubernetes workspaces can propagate the wrong environment into remote commands and persisted environment files.

Run cleanup once before applying the image environment, retaining deferred cleanup for build panics and preserving errors, cancellation, and disabled credentials. Add unit regressions using the real credentials helper and a Kubernetes Dockerless build fixture that verifies bare-name executable lookup, lifecycle commands, persisted environment, and helper removal. The fixture uses the existing Devsy-hosted ghcr.io/devsy-org/test-images/base:ubuntu base and explicitly keeps the remote user as root.

Credit to Renato Athaydes for the original cleanup-order fix in #1425, commit 7a69169.

Validation:

  • The final runtime PATH unit regression fails with the original deferred-cleanup sequence and passes with the fix.
  • Relevant Go unit tests, race tests, and go vet pass with Go 1.26.8.
  • Development CLI/agent builds pass for Darwin and Linux on amd64 and arm64.
  • Both registered Kubernetes Dockerless PATH cases pass with the Devsy-hosted Ubuntu base on Kind with Kubernetes 1.36.4 and locally built CLI/agent binaries. Default PATH covers create, repeat, recreate, stop, and restart; appended remote PATH covers fresh creation.
  • The compiled suite selects both named PATH cases under the existing CI matrix filter up-provider-kubernetes (7 specs selected in a registration-only dry run). No workflow change is required.
  • task cli:lint:ci reports zero new issues; pre-commit and commit-message hooks pass. The scoped analyzer comparison preserves the same 31 baseline findings with no additions. Local CodeRabbit and independent source review report no findings.

The existing Linux-only Kubernetes SSH-agent case was preserved and was not executed on macOS.

Closes #1446

Summary by CodeRabbit

  • Bug Fixes
    • Dockerless workspaces on Kubernetes now retain the image’s configured PATH, including across workspace restarts and pod replacement.
    • Dockerless builds now clean up temporary credential settings without losing the runtime environment. Image-provided environment values, including an explicitly empty value, take precedence where configured.

@netlify

netlify Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for images-devsy-sh canceled.

Name Link
🔨 Latest commit 2486821
🔍 Latest deploy log https://app.netlify.com/projects/images-devsy-sh/deploys/6ac9262c592b720008bf82c9

@coderabbitai

coderabbitai Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: af956e0e-192d-43ab-92e7-28da5ff26581

📥 Commits

Reviewing files that changed from the base of the PR and between 4e77d10 and 2486821.


📒 Files selected for processing (6)
  • e2e/tests/up/provider_kubernetes_dockerless_path.go
  • e2e/tests/up/testdata/kubernetes-dockerless-path/.devcontainer/Dockerfile
  • e2e/tests/up/testdata/kubernetes-dockerless-path/.devcontainer/devcontainer.json
  • e2e/tests/up/testdata/kubernetes-dockerless-path/.devcontainer/devsy-dockerless-path-check
  • pkg/agent/dockerless.go
  • pkg/agent/dockerless_test.go

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



📝 Walkthrough

Walkthrough

The build flow now cleans up temporary Dockerless credentials before applying the built image environment. Unit tests and a Kubernetes end-to-end fixture cover environment precedence, cleanup behavior, and workspace PATH handling.

Changes

Dockerless build environment

Layer / File(s) Summary
Cleanup before image environment
pkg/agent/dockerless.go
executeBuild delegates to a helper that cleans up credentials after the build attempt and before applying the image environment. Deferred cleanup remains as a panic fallback.
Build and cleanup unit coverage
pkg/agent/dockerless_test.go
Tests cover image and builder environment precedence, cleanup ordering, build and image errors, panic handling, and credential setup cases.
Kubernetes Dockerless PATH coverage
e2e/tests/up/provider_kubernetes_dockerless_path.go, e2e/tests/up/testdata/kubernetes-dockerless-path/.devcontainer/*
The end-to-end fixture builds an image with defined PATH and DOCKER_CONFIG values. Tests check environment values, Dockerless credential cleanup, and pod reuse or replacement across workspace lifecycle operations.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: Medium


Merge Risk

Merge Risk: ⚪ Minimal · up to 24868

The Dockerless build change has targeted coverage for runtime environment values and credential cleanup. No actionable issue was identified before merge.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 24868

The change corrects cleanup ordering without adding new permissions or external entrypoints. Temporary build credentials are removed before the image environment becomes authoritative. Concurrent setup and interruption recovery remain incompletely established.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The demonstrated impact is on a workspace setup process, its persisted environment, and downstream commands inheriting PATH or DOCKER_CONFIG. The inspected change adds no credential grant or tenant-routing mechanism. Credential access still uses the existing workspace tunnel client; broader credential-account exposure was not established.

Trust Boundaries and Controls

  • observed — Built-image environment values continue to enter runtime configuration through the existing image-config reader and environment merger. The PR changes their precedence relative to temporary build credentials, not their source or admission mechanism. The credential service remains localhost-bound and backed by the existing tunnel client.

Resilience and Maintainability Implications

  • observed — The inspected command calls workspace preparation synchronously, and ReportResult invokes its job once without retrying or spawning parallel jobs. This counters overlapping builds within that invocation. Process-global environment mutation, non-atomic persistence, and skipping a build when image configuration already exists are pre-existing limitations, not demonstrated PR-introduced concerns; independent concurrent setup processes and abrupt termination remain outside established coverage.



🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 22 functions across 3 files. (3 skipped: 3… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ 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: preserving the image environment after Dockerless credential cleanup.
Linked Issues check Passed Issue #1446 requires cleanup before image environment application, fallback cleanup for build panic, error and cancellation propagation, no double cleanup, and disabled or absent credential handling. …
Out of Scope Changes check Passed The changed files are limited to Dockerless build cleanup orchestration, its unit regressions, and the Kubernetes Dockerless PATH regression fixture and fixture image configuration. The lifecycle and …

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 22 functions across 3 files. (3 skipped: 3 unsupported.)



  • Fix all pre-merge checks with AI
✨ Finishing Touches
✨ Simplify code
  • Commit to this branch
  • 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.

@netlify

netlify Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for devsydev canceled.

Name Link
🔨 Latest commit 2486821
🔍 Latest deploy log https://app.netlify.com/projects/devsydev/deploys/6ac9262c7b99140008f5d3b8

@skevetter
skevetter marked this pull request as ready for review October 9, 2026 17:52

Copy link
Copy Markdown
Contributor Author

@greptileai please review this pull request at current head 2486821.

@greptile-apps

greptile-apps Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[Medium impact] The PR appears safe to merge; no actionable issues were found.

Summary

Dockerless builds now clean up temporary credentials before applying the image environment. Deferred cleanup remains available if the build panics.

  • Dockerless builds keep the image environment for workspace commands.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Configure temporary credentials] --> B[Run Dockerless build]
  B -->|Build returns| C[Clean up credentials once]
  B -->|Build panics| D[Deferred cleanup]
  C --> E{Build succeeded?}
  E -->|No| F[Return build error]
  E -->|Yes| G[Apply image environment]
  G --> H[Return without restoring builder environment]
Loading

Reviews (1) · Last reviewed commit: "test(kubernetes): align dockerless PATH ..." · Reviewed by Greptile

@mergify

mergify Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again.

@skevetter
skevetter marked this pull request as draft October 9, 2026 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(dockerless): preserve runtime environment after build cleanup

1 participant