Skip to content

fix(lifecycle): enforce waitFor property in lifecycle hook execution #87

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
skevetter merged 4 commits into from
Apr 23, 2026
Merged

fix(lifecycle): enforce waitFor property in lifecycle hook execution #87

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6aaffc9
fix(lifecycle): enforce waitFor property in lifecycle hook execution
skevetter Apr 23, 2026
f87ce46
fix(lifecycle): resolve gosec G204 by constructing exec.Cmd directly
skevetter Apr 23, 2026
5e2f29b
fix(lifecycle): use post-substitution setupInfo for deferred hooks
skevetter Apr 23, 2026
022ebe2
test(lifecycle): add e2e test for waitFor deferred hook execution
skevetter Apr 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cmd/agent/container/container.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ func NewContainerCmd(flags *flags.GlobalFlags) *cobra.Command {
}

containerCmd.AddCommand(NewSetupContainerCmd(flags))
containerCmd.AddCommand(NewDeferredHooksCmd(flags))
containerCmd.AddCommand(NewPostAttachCmd(flags))
containerCmd.AddCommand(NewDaemonCmd())
containerCmd.AddCommand(NewVSCodeAsyncCmd())
Expand Down
69 changes: 69 additions & 0 deletions cmd/agent/container/deferred_hooks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
//go:build !windows

package container

import (
"context"
"encoding/json"

"github.com/devsy-org/devsy/cmd/flags"
"github.com/devsy-org/devsy/pkg/compress"
"github.com/devsy-org/devsy/pkg/devcontainer/config"
"github.com/devsy-org/devsy/pkg/devcontainer/setup"
"github.com/devsy-org/devsy/pkg/log"
"github.com/spf13/cobra"
)

// DeferredHooksCmd runs deferred lifecycle hooks as a detached background process.
type DeferredHooksCmd struct {
*flags.GlobalFlags
SetupInfo string
Prebuild bool
}

// NewDeferredHooksCmd creates a new command.
func NewDeferredHooksCmd(flags *flags.GlobalFlags) *cobra.Command {
cmd := &DeferredHooksCmd{
GlobalFlags: flags,
}
deferredCmd := &cobra.Command{
Use: "deferred-hooks",
Short: "Runs deferred lifecycle hooks (phases after waitFor)",
Args: cobra.NoArgs,
RunE: func(cobraCmd *cobra.Command, _ []string) error {
return cmd.Run(cobraCmd.Context())
},
}
deferredCmd.Flags().
StringVar(&cmd.SetupInfo, "setup-info", "", "The container setup info")
deferredCmd.Flags().
BoolVar(&cmd.Prebuild, "prebuild", false, "If true, prebuild lifecycle mode")
_ = deferredCmd.MarkFlagRequired("setup-info")
return deferredCmd
}

// Run executes the deferred lifecycle hooks.
func (cmd *DeferredHooksCmd) Run(ctx context.Context) error {
decompressed, err := compress.Decompress(cmd.SetupInfo)
if err != nil {
return err
}

setupInfo := &config.Result{}
if err := json.Unmarshal([]byte(decompressed), setupInfo); err != nil {
return err
}

log.Debugf("running deferred lifecycle hooks")
deferred, err := setup.RunPreAttachHooks(ctx, setupInfo, cmd.Prebuild)
if err != nil {
log.Errorf("deferred hooks setup failed: %v", err)
return nil
}

if err := deferred.Run(); err != nil {
log.Errorf("deferred lifecycle hooks failed: %v", err)
}
Comment on lines +57 to +66

@coderabbitai coderabbitai Bot Apr 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Re-running RunPreAttachHooks in the background couples deferred execution to marker state.

Calling RunPreAttachHooks here re-iterates every pre-attach phase up to waitFor and depends entirely on marker files being written by the main process to skip them. Two practical consequences:

  1. If RunPreAttachHooks returns an error (e.g., transient env-probe failure, or a phase that somehow re-runs and fails), the current code logs and returns nil — the deferred hooks are silently never executed.
  2. Any bug or content mismatch that causes a marker check to miss will re-execute hooks that already ran (non-idempotent hooks could corrupt state).

Consider splitting the lifecycle API so the main process can hand the caller an already-computed DeferredHooks set (or a pure "plan" that this subcommand can execute without re-running pre-attach logic). At minimum, treat a failure from RunPreAttachHooks here as user-visible — swallowing it makes deferred-phase regressions invisible.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@cmd/agent/container/deferred_hooks.go` around lines 57 - 66, The
deferred-hooks subcommand is re-running setup.RunPreAttachHooks which couples
deferred execution to marker state and silently swallows errors; change the
approach so the main process computes and passes a prepared DeferredHooks (or a
pure plan) to this subcommand instead of calling setup.RunPreAttachHooks here,
or at minimum propagate any error from setup.RunPreAttachHooks to the caller (do
not log and return nil) so failures are user-visible; update the lifecycle API
to return a DeferredHooks value (or plan) from the pre-attach flow and have this
file call deferred.Run() only on that prepared object (and handle non-idempotent
hooks safely) rather than re-running pre-attach logic and relying on marker
files.


return nil
}
21 changes: 21 additions & 0 deletions cmd/agent/container/deferred_hooks_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
//go:build windows

package container

import (
"fmt"

"github.com/devsy-org/devsy/cmd/flags"
"github.com/spf13/cobra"
)

func NewDeferredHooksCmd(_ *flags.GlobalFlags) *cobra.Command {
return &cobra.Command{
Use: "deferred-hooks",
Short: "Runs deferred lifecycle hooks (phases after waitFor)",
Args: cobra.NoArgs,
RunE: func(_ *cobra.Command, _ []string) error {
return fmt.Errorf("Windows Containers are not supported")
},
}
}
62 changes: 50 additions & 12 deletions cmd/agent/container/setup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,29 +188,67 @@
Prebuild: cmd.Prebuild,
}

if err := setup.SetupContainerPreAttach(sctx.ctx, cfg); err != nil {
deferred, err := setup.SetupContainerPreAttach(sctx.ctx, cfg)
if err != nil {
return err
}

if !cmd.Prebuild {
if err := cmd.installIDE(sctx.setupInfo, &sctx.workspaceInfo.IDE); err != nil {
if err := cmd.setupPostAttach(sctx, deferred); err != nil {
return err
}
}

if err := cmd.startContainerDaemon(sctx.workspaceInfo); err != nil {
return err
}
return cmd.sendSetupResult(sctx.ctx, sctx.setupInfo, sctx.tunnelClient)
}

// Launch postAttachCommand as a detached background process before sending
// the result. Once sendSetupResult returns, the client tears down the SSH
// tunnel which kills this process, so postAttach must already be running
// independently.
if err := cmd.startPostAttachHooks(sctx); err != nil {
log.Errorf("failed to start postAttachCommand: %v", err)
func (cmd *SetupContainerCmd) setupPostAttach(
sctx *setupContext,
deferred setup.DeferredHooks,
) error {
if err := cmd.installIDE(sctx.setupInfo, &sctx.workspaceInfo.IDE); err != nil {
return err
}

if err := cmd.startContainerDaemon(sctx.workspaceInfo); err != nil {
return err
}

if !deferred.Empty() {
if err := cmd.startDeferredHooks(); err != nil {
log.Errorf("failed to start deferred lifecycle hooks: %v", err)
}
}

return cmd.sendSetupResult(sctx.ctx, sctx.setupInfo, sctx.tunnelClient)
if err := cmd.startPostAttachHooks(sctx); err != nil {
log.Errorf("failed to start postAttachCommand: %v", err)
}

return nil
}

func (cmd *SetupContainerCmd) startDeferredHooks() error {
return command.StartBackgroundOnce("devsy.deferred-hooks", func() (*exec.Cmd, error) {
log.Debugf("starting deferred lifecycle hooks as background process")
return cmd.buildDeferredHooksCmd()
})
}

func (cmd *SetupContainerCmd) buildDeferredHooksCmd() (*exec.Cmd, error) {
binaryPath, err := os.Executable()
if err != nil {
return nil, err
}

args := []string{
"agent", "container", "deferred-hooks",
"--setup-info", cmd.SetupInfo,
}
if cmd.Prebuild {
args = append(args, "--prebuild")
}

return exec.Command(binaryPath, args...), nil

Check failure on line 251 in cmd/agent/container/setup.go

View workflow job for this annotation

GitHub Actions / Lint 

G204: Subprocess launched with variable (gosec)

Check failure on line 251 in cmd/agent/container/setup.go

View workflow job for this annotation

GitHub Actions / Lint Gate 

G204: Subprocess launched with variable (gosec)
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

func (cmd *SetupContainerCmd) initializeTunnelClient(
Expand Down
Loading
Loading