Skip to content

codemode: fix Start() fail-fast and use tools.As for wrapper unwrapping #2007

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

Open
dgageot wants to merge 1 commit into
base: main
Choose a base branch
from
+45 −17
Open

codemode: fix Start() fail-fast and use tools.As for wrapper unwrapping #2007

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 9 additions & 13 deletions pkg/tools/codemode/codemode.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,27 +103,23 @@ func (c *codeModeTool) Tools(ctx context.Context) ([]tools.Tool, error) {
}

func (c *codeModeTool) Start(ctx context.Context) error {
for _, t := range c.toolsets {
if startable, ok := t.(tools.Startable); ok {
if err := startable.Start(ctx); err != nil {
return err
}
}
}

return nil
return c.forEachStartable(func(s tools.Startable) error { return s.Start(ctx) })
Copy link

@docker-agent docker-agent bot Mar 9, 2026

Choose a reason for hiding this comment

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

🔴 CRITICAL: Semantic breaking change — fail-fast → collect-all-errors

The original Start() implementation returned immediately on the first error:

if err := startable.Start(ctx); err != nil {
    return err  // fail-fast
}

The new implementation attempts all toolsets regardless of failures:

return c.forEachStartable(func(s tools.Startable) error { return s.Start(ctx) })
// forEachStartable collects all errors with errors.Join

Problem: If toolset A starts successfully but toolset B fails, the system is left in a partially-started state. There's no tracking of which toolsets succeeded, so:

  1. The caller cannot determine which toolsets are running
  2. No rollback mechanism exists to stop successfully-started toolsets
  3. Stop() will attempt to stop all toolsets regardless of whether they started

Impact: This changes startup semantics from "abort on first failure" to "try everything and report all failures". While the PR description mentions this is intentional ("one broken toolset shouldn't prevent others"), this could leave the system in an inconsistent state.

Recommendation: Consider adding state tracking (e.g., a started []tools.Startable field) to enable cleanup/rollback when Start() returns an error, or document that toolset Stop() implementations must handle being stopped without being started.

Copy link

@docker-agent docker-agent bot Mar 9, 2026

Choose a reason for hiding this comment

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

🟡 NOTABLE: No state tracking for cleanup after partial startup failures

When Start() returns an error after attempting all toolsets, the caller has no visibility into which toolsets successfully started and which failed. This makes cleanup/rollback impossible.

The codeModeTool maintains no internal state about which toolsets are running, so if Start() fails:

  • Successfully-started toolsets remain running
  • No mechanism exists to selectively stop only the started ones
  • Calling Stop() will attempt to stop all toolsets, including those that never started (which may or may not handle this gracefully)

Recommendation: Either:

  1. Add internal state tracking: started []tools.Startable
  2. Implement automatic rollback in Start() on any error
  3. Document that toolset Stop() implementations must be idempotent and safe to call on non-started toolsets

}

func (c *codeModeTool) Stop(ctx context.Context) error {
var errs []error
return c.forEachStartable(func(s tools.Startable) error { return s.Stop(ctx) })
}

// forEachStartable applies fn to every toolset that implements Startable,
// collecting all errors so that one broken toolset doesn't block the others.
func (c *codeModeTool) forEachStartable(fn func(tools.Startable) error) error {
var errs []error
for _, t := range c.toolsets {
if startable, ok := t.(tools.Startable); ok {
if err := startable.Stop(ctx); err != nil {
if s, ok := tools.As[tools.Startable](t); ok {
if err := fn(s); err != nil {
errs = append(errs, err)
}
}
}

return errors.Join(errs...)
}
40 changes: 36 additions & 4 deletions pkg/tools/codemode/codemode_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,10 +187,42 @@ func TestCodeModeTool_CallEcho(t *testing.T) {
require.Empty(t, scriptResult.StdOut)
}

// TestCodeModeTool_StartContinuesOnError verifies that a failing Start on one
// toolset does not prevent the remaining toolsets from being started.
func TestCodeModeTool_StartContinuesOnError(t *testing.T) {
failing := &testToolSet{startErr: assert.AnError}
healthy := &testToolSet{}

tool := Wrap(failing, healthy).(tools.Startable)

err := tool.Start(t.Context())
require.ErrorIs(t, err, assert.AnError)
assert.Equal(t, 1, failing.start, "failing toolset should have been started")
assert.Equal(t, 1, healthy.start, "healthy toolset should still be started")
}

// TestCodeModeTool_StartStopWrappedToolSet verifies that Start/Stop find
// Startable through a StartableToolSet wrapper via tools.As.
func TestCodeModeTool_StartStopWrappedToolSet(t *testing.T) {
inner := &testToolSet{}
wrapped := tools.NewStartable(inner)

tool := Wrap(wrapped).(tools.Startable)

err := tool.Start(t.Context())
require.NoError(t, err)
assert.Equal(t, 1, inner.start)

err = tool.Stop(t.Context())
require.NoError(t, err)
assert.Equal(t, 1, inner.stop)
}

type testToolSet struct {
tools []tools.Tool
start int
stop int
tools []tools.Tool
start int
stop int
startErr error
}

// Verify interface compliance
Expand All @@ -205,7 +237,7 @@ func (t *testToolSet) Tools(context.Context) ([]tools.Tool, error) {

func (t *testToolSet) Start(context.Context) error {
t.start++
return nil
return t.startErr
}

func (t *testToolSet) Stop(context.Context) error {
Expand Down