Open
Conversation
Introduce RunPreAttachHooks (onCreate through postStart) and RunPostAttachHooks (postAttach only). The original RunLifecycleHooks now delegates to both, preserving existing behavior. Part of #708
SetupContainerPreAttach runs ownership, environment, optional features, and lifecycle hooks through postStartCommand. SetupContainerPostAttach runs postAttachCommand only. The original SetupContainer delegates to both, preserving existing behavior. Part of #708
Move sendSetupResult into finalizeSetup, called after pre-attach hooks and IDE installation but before postAttachCommand. This unblocks the client to open the IDE while postAttachCommand runs in the container. postAttachCommand errors are logged but don't block IDE opening. Fixes #708
Validates that after devpod up returns: - postStartCommand has completed (marker file exists) - postAttachCommand is still running (marker file absent) - postAttachCommand eventually completes in the background Closes #708
These wrapper functions had no callers after the container setup was changed to call the pre-attach and post-attach phases directly.
📝 WalkthroughWalkthroughThis PR implements non-blocking execution of Changes
Sequence DiagramsequenceDiagram
actor Agent as Agent Process
participant PreSetup as SetupContainerPreAttach
participant IDELauncher as IDE Launcher
participant PostSetup as SetupContainerPostAttach
participant BgCmd as Background Command<br/>(post-attach)
participant Hooks as Lifecycle Hooks
Agent->>PreSetup: Run pre-attach setup phase
PreSetup->>Hooks: RunPreAttachHooks (onCreate, postStart, etc.)
Hooks-->>PreSetup: ✓ Complete
PreSetup-->>Agent: ✓ Return
Agent->>IDELauncher: Start IDE
IDELauncher-->>Agent: ✓ IDE Ready
Agent->>PostSetup: Trigger post-attach phase
PostSetup->>BgCmd: startPostAttachHooks (spawn detached process)
BgCmd->>BgCmd: agent container post-attach --setup-info <...>
BgCmd->>Hooks: RunPostAttachHooks (async)
Hooks-->>BgCmd: ✓ Hooks execute in background
PostSetup-->>Agent: ✓ Return (non-blocking)
Agent-->>Agent: Setup complete, IDE open
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Extract resolveLifecycleEnv to avoid probing the environment twice (once in pre-attach, once in post-attach). Bump e2e test fixture sleep from 5s to 15s for CI robustness.
skevetter
force-pushed
the
investigate/issue-707
branch
from
6642345 to
e457310
Compare
After sendSetupResult, the client tears down the SSH tunnel which kills the container setup process. postAttachCommand must run as an independent process that survives the tunnel teardown. Add `devpod agent container post-attach` subcommand that runs postAttachCommand lifecycle hooks. The setup process spawns this as a background process via StartBackgroundOnce before sending the result back to the client.
The post_attach.go file has a !windows build constraint but container.go references NewPostAttachCmd unconditionally, causing the Windows build to fail with "undefined: NewPostAttachCmd".
skevetter
force-pushed
the
investigate/issue-707
branch
from
447af08 to
7822cb6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cmd/agent/container/post_attach_windows.go (1)
15-17: Consider returning an error instead of panicking.Using
panic()here will crash the process ungracefully. SinceRunEexpects anerrorreturn, you can return an error that will be handled properly by Cobra and provide a cleaner user experience.♻️ Proposed fix
+ "errors" + "github.com/skevetter/devpod/cmd/flags" "github.com/spf13/cobra"RunE: func(cmd *cobra.Command, args []string) error { - panic("Windows Containers are not supported") + return errors.New("Windows Containers are not supported") },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/agent/container/post_attach_windows.go` around lines 15 - 17, Replace the panic inside the RunE handler with a returned error so Cobra can handle it gracefully: in the RunE function (the anonymous RunE handler) return an error like fmt.Errorf("Windows Containers are not supported") (or errors.New) instead of calling panic; add the necessary import for fmt or errors if not already present.pkg/devcontainer/setup/lifecyclehooks_test.go (1)
85-103: Test validates the no-op path but could benefit from a clearer name.The test verifies both functions return
nilwith empty configs, which is useful as a smoke test. However, the nameTestRunPreAttachHooksDoesNotRunPostAttachimplies it validates separation of hook execution, which isn't what's being tested. Consider renaming to something likeTestLifecycleHooksNoOpWithEmptyConfig.The actual separation is validated by the e2e test in
provider_docker.go, so this is acceptable as-is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/devcontainer/setup/lifecyclehooks_test.go` around lines 85 - 103, Rename the test method TestRunPreAttachHooksDoesNotRunPostAttach to a clearer name like TestLifecycleHooksNoOpWithEmptyConfig and update any references/usages accordingly; the test currently calls RunPreAttachHooks and RunPostAttachHooks with an empty config and only asserts no errors, so keep the body as-is but change the function name (method on LifecycleHookTestSuite) to reflect that it verifies the no-op path rather than hook separation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/tests/up/provider_docker.go`:
- Around line 327-330: Update the misleading comment above the execSSH check:
change "it sleeps 5s" to "it sleeps 15s" so the comment accurately reflects the
postAttachCommand duration; the relevant spot is the comment immediately before
the dtc.execSSH(...) call that asserts postAttachCommand should still be running
when devpod up returns.
---
Nitpick comments:
In `@cmd/agent/container/post_attach_windows.go`:
- Around line 15-17: Replace the panic inside the RunE handler with a returned
error so Cobra can handle it gracefully: in the RunE function (the anonymous
RunE handler) return an error like fmt.Errorf("Windows Containers are not
supported") (or errors.New) instead of calling panic; add the necessary import
for fmt or errors if not already present.
In `@pkg/devcontainer/setup/lifecyclehooks_test.go`:
- Around line 85-103: Rename the test method
TestRunPreAttachHooksDoesNotRunPostAttach to a clearer name like
TestLifecycleHooksNoOpWithEmptyConfig and update any references/usages
accordingly; the test currently calls RunPreAttachHooks and RunPostAttachHooks
with an empty config and only asserts no errors, so keep the body as-is but
change the function name (method on LifecycleHookTestSuite) to reflect that it
verifies the no-op path rather than hook separation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ae31acda-6cec-49ae-a888-8ee81b8df503
📒 Files selected for processing (9)
cmd/agent/container/container.gocmd/agent/container/post_attach.gocmd/agent/container/post_attach_windows.gocmd/agent/container/setup.goe2e/tests/up/provider_docker.goe2e/tests/up/testdata/docker-post-attach-nonblocking/.devcontainer.jsonpkg/devcontainer/setup/lifecyclehooks.gopkg/devcontainer/setup/lifecyclehooks_test.gopkg/devcontainer/setup/setup.go
Comment on lines
+327
to
+330
| // postAttachCommand should NOT have completed yet (it sleeps 5s) | ||
| _, err = dtc.execSSH(ctx, tempDir, "cat $HOME/post-attach.out") | ||
| gomega.Expect(err).To(gomega.HaveOccurred(), | ||
| "postAttachCommand should still be running when devpod up returns") |
There was a problem hiding this comment.
Comment mentions incorrect sleep duration.
The comment says "it sleeps 5s" but the actual postAttachCommand in the devcontainer.json sleeps for 15 seconds. Update the comment for accuracy.
📝 Proposed fix
- // postAttachCommand should NOT have completed yet (it sleeps 5s)
+ // postAttachCommand should NOT have completed yet (it sleeps 15s)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // postAttachCommand should NOT have completed yet (it sleeps 5s) | |
| _, err = dtc.execSSH(ctx, tempDir, "cat $HOME/post-attach.out") | |
| gomega.Expect(err).To(gomega.HaveOccurred(), | |
| "postAttachCommand should still be running when devpod up returns") | |
| // postAttachCommand should NOT have completed yet (it sleeps 15s) | |
| _, err = dtc.execSSH(ctx, tempDir, "cat $HOME/post-attach.out") | |
| gomega.Expect(err).To(gomega.HaveOccurred(), | |
| "postAttachCommand should still be running when devpod up returns") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/tests/up/provider_docker.go` around lines 327 - 330, Update the
misleading comment above the execSSH check: change "it sleeps 5s" to "it sleeps
15s" so the comment accurately reflects the postAttachCommand duration; the
relevant spot is the comment immediately before the dtc.execSSH(...) call that
asserts postAttachCommand should still be running when devpod up returns.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
postAttachCommand, unblocking IDE openingpostAttachCommanderrors are logged but don't block IDE accesspostAttachCommandas running after tool attachmentFixes #708
Summary by CodeRabbit
Release Notes
New Features
Tests