From b427e89d052a497c815f26d6329ac4f32afae142 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Fri, 22 May 2026 16:25:00 +0100 Subject: [PATCH] feat(routingerr): auto-recover from npm _npx cache corruption (#1013) --- .../agent/runtime/lifecycle/manager.go | 7 ++ .../runtime/lifecycle/manager_interaction.go | 38 ++++++-- .../manager_interaction_remediate_test.go | 80 +++++++++++++++ .../agent/runtime/lifecycle/manager_test.go | 7 ++ .../agent/runtime/routingerr/remediate.go | 64 ++++++++++++ .../runtime/routingerr/remediate_test.go | 97 +++++++++++++++++++ .../agent/runtime/routingerr/routingerr.go | 11 +++ .../agent/runtime/routingerr/runtime_rules.go | 69 +++++++++++++ .../runtime/routingerr/runtime_rules_test.go | 97 +++++++++++++++++++ 9 files changed, 464 insertions(+), 6 deletions(-) create mode 100644 apps/backend/internal/agent/runtime/lifecycle/manager_interaction_remediate_test.go create mode 100644 apps/backend/internal/agent/runtime/routingerr/remediate.go create mode 100644 apps/backend/internal/agent/runtime/routingerr/remediate_test.go create mode 100644 apps/backend/internal/agent/runtime/routingerr/runtime_rules.go create mode 100644 apps/backend/internal/agent/runtime/routingerr/runtime_rules_test.go diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager.go b/apps/backend/internal/agent/runtime/lifecycle/manager.go index 268bfb839d..2fea3151fe 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager.go @@ -14,6 +14,7 @@ import ( "github.com/kandev/kandev/internal/agent/executor" "github.com/kandev/kandev/internal/agent/registry" agentctl "github.com/kandev/kandev/internal/agent/runtime/agentctl" + "github.com/kandev/kandev/internal/agent/runtime/routingerr" "github.com/kandev/kandev/internal/common/logger" "github.com/kandev/kandev/internal/events/bus" "github.com/kandev/kandev/internal/secrets" @@ -116,6 +117,11 @@ type Manager struct { // the agent process starts. Defaults to a no-op deployer; office wires // its concrete implementation via SetSkillDeployer. skillDeployer SkillDeployer + + // remediateNpxCache is the hook fired when the routing classifier + // returns CodeNpxCacheCorrupted. NewManager wires routingerr.RemediateNpxCache; + // tests override it to avoid touching the real filesystem. + remediateNpxCache func(path string, log *zap.Logger) error } // NewManager creates a new lifecycle manager. @@ -174,6 +180,7 @@ func NewManager( remoteStatusBySession: make(map[string]*RemoteStatus), stopCh: stopCh, skillDeployer: NoopSkillDeployer(), + remediateNpxCache: routingerr.RemediateNpxCache, } // Initialize stream manager with callbacks that delegate to manager methods diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_interaction.go b/apps/backend/internal/agent/runtime/lifecycle/manager_interaction.go index efeaedd270..9e383e62fe 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_interaction.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_interaction.go @@ -970,17 +970,23 @@ func (m *Manager) MarkCompleted(executionID string, exitCode int, errorMessage s eventType := events.AgentCompleted if execution.Status == v1.AgentStatusFailed { eventType = events.AgentFailed - m.logRoutingClassification(execution, exitCode, errorMessage) + m.classifyAndMaybeRemediate(execution, exitCode, errorMessage) } m.eventPublisher.PublishAgentEvent(context.Background(), eventType, execution) return nil } -// logRoutingClassification runs routingerr.Classify at the failure boundary -// and logs the structured result. Acting on the result is wired by Phase 4 -// of office-provider-routing. -func (m *Manager) logRoutingClassification(execution *AgentExecution, exitCode int, errorMessage string) { +// classifyAndMaybeRemediate runs routingerr.Classify at the failure +// boundary, logs the structured result, and - for codes that carry a +// safe remediation (currently CodeNpxCacheCorrupted) - fires a +// best-effort cleanup goroutine so the next launch attempt (Office +// scheduler retry or Kanban "Resume") finds a clean environment. +// +// The remediation hook is injectable via Manager.remediateNpxCache; +// production wiring points it at routingerr.RemediateNpxCache. Tests +// stub it to avoid touching the real filesystem. +func (m *Manager) classifyAndMaybeRemediate(execution *AgentExecution, exitCode int, errorMessage string) { phase := routingerr.PhaseSessionInit if execution.sessionInitialized { phase = routingerr.PhasePromptSend @@ -1010,7 +1016,27 @@ func (m *Manager) logRoutingClassification(execution *AgentExecution, exitCode i zap.String("classifier_rule", e.ClassifierRule), zap.Bool("fallback_allowed", e.FallbackAllowed), zap.Bool("auto_retryable", e.AutoRetryable), - zap.Bool("user_action", e.UserAction)) + zap.Bool("user_action", e.UserAction), + zap.String("remediation_path", e.RemediationPath)) + + if e.Code != routingerr.CodeNpxCacheCorrupted || e.RemediationPath == "" { + return + } + remediate := m.remediateNpxCache + path := e.RemediationPath + execID := execution.ID + zapLog := m.logger.Zap().With( + zap.String("execution_id", execID), + zap.String("path", path), + ) + go func() { + if err := remediate(path, zapLog); err != nil { + m.logger.Warn("npx cache remediation failed", + zap.String("execution_id", execID), + zap.String("path", path), + zap.Error(err)) + } + }() } // RespondToPermission sends a response to an agent's permission request. diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_interaction_remediate_test.go b/apps/backend/internal/agent/runtime/lifecycle/manager_interaction_remediate_test.go new file mode 100644 index 0000000000..6660d6dee5 --- /dev/null +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_interaction_remediate_test.go @@ -0,0 +1,80 @@ +package lifecycle + +import ( + "strings" + "sync" + "testing" + "time" + + "github.com/kandev/kandev/internal/common/logger" + "go.uber.org/zap" +) + +// newRemediateTestManager builds a Manager with just enough wiring for +// classifyAndMaybeRemediate to run: a logger wrapper around a no-op zap +// and nothing else. Other manager subsystems are nil; the method under +// test must not touch them. We avoid the package-wide newTestManager +// helper because it spins up the full lifecycle DI graph, which is more +// surface area than these failure-boundary tests need. +func newRemediateTestManager(t *testing.T) *Manager { + t.Helper() + log, err := logger.NewFromZap(zap.NewNop()) + if err != nil { + t.Fatalf("NewFromZap: %v", err) + } + return &Manager{logger: log} +} + +func TestClassifyAndMaybeRemediate_TriggersRemediation_OnNpxCacheCorrupted(t *testing.T) { + stderr := strings.Join([]string{ + "npm error code ENOTEMPTY", + "npm error path /Users/test/.npm/_npx/d820eb7d96bc2600/node_modules/foo", + "npm error ENOTEMPTY: directory not empty", + }, "\n") + + var ( + mu sync.Mutex + calledWith string + done = make(chan struct{}, 1) + ) + m := newRemediateTestManager(t) + m.remediateNpxCache = func(path string, _ *zap.Logger) error { + mu.Lock() + calledWith = path + mu.Unlock() + done <- struct{}{} + return nil + } + + exec := &AgentExecution{ID: "exec-1", AgentID: "claude-acp", TaskID: "t-1", SessionID: "s-1"} + m.classifyAndMaybeRemediate(exec, 190, stderr) + + select { + case <-done: + case <-time.After(time.Second): + t.Fatal("remediation hook was not invoked within 1s") + } + + mu.Lock() + defer mu.Unlock() + want := "/Users/test/.npm/_npx/d820eb7d96bc2600" + if calledWith != want { + t.Errorf("remediateNpxCache called with %q, want %q", calledWith, want) + } +} + +func TestClassifyAndMaybeRemediate_NoCallOnUnrelatedFailure(t *testing.T) { + m := newRemediateTestManager(t) + called := make(chan struct{}, 1) + m.remediateNpxCache = func(string, *zap.Logger) error { + called <- struct{}{} + return nil + } + + exec := &AgentExecution{ID: "exec-2", AgentID: "claude-acp"} + m.classifyAndMaybeRemediate(exec, 1, "some unrelated stderr") + + if len(called) != 0 { + t.Fatal("remediation hook should not have been called") + } +} diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_test.go b/apps/backend/internal/agent/runtime/lifecycle/manager_test.go index a9d6dc7c80..760c24dd26 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_test.go @@ -219,6 +219,13 @@ func TestNewManager(t *testing.T) { } } +func TestNewManager_WiresRemediateNpxCacheDefault(t *testing.T) { + m := newTestManager() + if m.remediateNpxCache == nil { + t.Fatal("NewManager must wire default remediateNpxCache") + } +} + func TestManager_GetExecution(t *testing.T) { mgr := newTestManager() diff --git a/apps/backend/internal/agent/runtime/routingerr/remediate.go b/apps/backend/internal/agent/runtime/routingerr/remediate.go new file mode 100644 index 0000000000..a5f3070747 --- /dev/null +++ b/apps/backend/internal/agent/runtime/routingerr/remediate.go @@ -0,0 +1,64 @@ +package routingerr + +import ( + "fmt" + "os" + "path/filepath" + "strings" + + "go.uber.org/zap" +) + +// RemediateNpxCache wipes a single npx package cache directory (the +// `_npx/` subtree) so the next `npx -y ` invocation extracts +// cleanly. It is a no-op-safe operation: if the directory is already +// gone, the call succeeds. +// +// Safety: the path MUST resolve (with symlinks evaluated) to a child of +// `$HOME/.npm/_npx/`. Any other path is rejected without modification, +// so a malformed log line can never trick the caller into removing an +// unrelated tree. +func RemediateNpxCache(path string, log *zap.Logger) error { + if path == "" { + return fmt.Errorf("RemediateNpxCache: empty path") + } + if log == nil { + log = zap.NewNop() + } + resolved, err := filepath.Abs(path) + if err != nil { + return fmt.Errorf("RemediateNpxCache: resolve %q: %w", path, err) + } + evaluated, err := filepath.EvalSymlinks(resolved) + if err != nil { + if os.IsNotExist(err) { + log.Info("npx cache already absent", zap.String("path", resolved)) + return nil + } + return fmt.Errorf("RemediateNpxCache: eval symlinks %q: %w", resolved, err) + } + home, err := os.UserHomeDir() + if err != nil { + return fmt.Errorf("RemediateNpxCache: user home: %w", err) + } + expectedRoot := filepath.Join(home, ".npm", "_npx") + evaluatedRoot, err := filepath.EvalSymlinks(expectedRoot) + if err != nil { + if !os.IsNotExist(err) { + return fmt.Errorf("RemediateNpxCache: eval symlinks %q: %w", expectedRoot, err) + } + evaluatedRoot = expectedRoot + } + rootWithSep := evaluatedRoot + string(os.PathSeparator) + if !strings.HasPrefix(evaluated+string(os.PathSeparator), rootWithSep) { + return fmt.Errorf("RemediateNpxCache: refusing path %q outside %q", evaluated, rootWithSep) + } + if evaluated == evaluatedRoot { + return fmt.Errorf("RemediateNpxCache: refusing to delete _npx root %q", evaluated) + } + if err := os.RemoveAll(evaluated); err != nil { + return fmt.Errorf("RemediateNpxCache: remove %q: %w", evaluated, err) + } + log.Info("removed corrupted npx cache directory", zap.String("path", evaluated)) + return nil +} diff --git a/apps/backend/internal/agent/runtime/routingerr/remediate_test.go b/apps/backend/internal/agent/runtime/routingerr/remediate_test.go new file mode 100644 index 0000000000..918c486f6d --- /dev/null +++ b/apps/backend/internal/agent/runtime/routingerr/remediate_test.go @@ -0,0 +1,97 @@ +package routingerr + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "go.uber.org/zap" +) + +func TestRemediateNpxCache_RemovesHashDirAndPreservesParent(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + + cacheRoot := filepath.Join(home, ".npm", "_npx") + hashDir := filepath.Join(cacheRoot, "d820eb7d96bc2600") + if err := os.MkdirAll(filepath.Join(hashDir, "node_modules", "@anthropic-ai", "claude-agent-sdk-darwin-arm64"), 0o755); err != nil { + t.Fatalf("setup: %v", err) + } + + if err := RemediateNpxCache(hashDir, zap.NewNop()); err != nil { + t.Fatalf("RemediateNpxCache: %v", err) + } + + if _, err := os.Stat(hashDir); !os.IsNotExist(err) { + t.Errorf("hashDir still exists, err = %v", err) + } + // _npx parent should survive (we only wipe a specific hash dir). + if _, err := os.Stat(cacheRoot); err != nil { + t.Errorf("_npx parent should still exist, err = %v", err) + } +} + +func TestRemediateNpxCache_IdempotentWhenAbsent(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + + // Create the _npx root but not the hash dir. + if err := os.MkdirAll(filepath.Join(home, ".npm", "_npx"), 0o755); err != nil { + t.Fatalf("setup: %v", err) + } + + absent := filepath.Join(home, ".npm", "_npx", "deadbeefcafef00d") + if err := RemediateNpxCache(absent, zap.NewNop()); err != nil { + t.Errorf("RemediateNpxCache on absent path = %v, want nil", err) + } +} + +func TestRemediateNpxCache_RejectsPrefixOnlySibling(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + + // _npxFOO is NOT a child of _npx/ — the trailing-separator guard must reject it. + bad := filepath.Join(home, ".npm", "_npxFOO", "anything") + if err := os.MkdirAll(bad, 0o755); err != nil { + t.Fatalf("setup: %v", err) + } + + err := RemediateNpxCache(bad, zap.NewNop()) + if err == nil { + t.Fatal("expected guard error, got nil") + } + if !strings.Contains(err.Error(), "outside") { + t.Errorf("error = %v, want one mentioning 'outside'", err) + } + if _, statErr := os.Stat(bad); statErr != nil { + t.Errorf("guard should not delete the path, stat err = %v", statErr) + } +} + +func TestRemediateNpxCache_RejectsPathOutsideNpxCache(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + + bad := filepath.Join(home, "etc") + if err := os.MkdirAll(bad, 0o755); err != nil { + t.Fatalf("setup: %v", err) + } + + err := RemediateNpxCache(bad, zap.NewNop()) + if err == nil { + t.Fatal("expected guard error, got nil") + } + if !strings.Contains(err.Error(), "outside") { + t.Errorf("error = %v, want one mentioning 'outside'", err) + } + if _, statErr := os.Stat(bad); statErr != nil { + t.Errorf("guard should not delete the path, stat err = %v", statErr) + } +} + +func TestRemediateNpxCache_RejectsEmptyPath(t *testing.T) { + if err := RemediateNpxCache("", zap.NewNop()); err == nil { + t.Fatal("expected error for empty path") + } +} diff --git a/apps/backend/internal/agent/runtime/routingerr/routingerr.go b/apps/backend/internal/agent/runtime/routingerr/routingerr.go index d853dda5ac..176d40a739 100644 --- a/apps/backend/internal/agent/runtime/routingerr/routingerr.go +++ b/apps/backend/internal/agent/runtime/routingerr/routingerr.go @@ -29,6 +29,7 @@ const ( CodeTask Code = "task_error" CodeRepo Code = "repo_error" CodePermissionDeniedByUser Code = "permission_denied_by_user" + CodeNpxCacheCorrupted Code = "npx_cache_corrupted" ) // Confidence reflects how strongly the classifier trusts the matched signal. @@ -66,6 +67,7 @@ type Error struct { ExitCode *int ResetHint *time.Time RawExcerpt string + RemediationPath string // path to clean before retry; only set for codes that have a known remediation } func (e *Error) Error() string { @@ -100,6 +102,12 @@ func Classify(in Input) *Error { e.RawExcerpt = excerpt return applyInvariants(e) } + if e, ok := matchRuntimeEnvironmentRules(in.Stderr + "\n" + in.Stdout); ok { + e.Phase = in.Phase + e.ExitCode = in.ExitCode + e.RawExcerpt = excerpt + return applyInvariants(e) + } return applyInvariants(classifyByPhase(in, excerpt)) } @@ -201,6 +209,9 @@ func applyInvariants(e *Error) *Error { case CodeProviderUnavailable, CodeUnknownProvider: e.AutoRetryable = true e.FallbackAllowed = true + case CodeNpxCacheCorrupted: + e.AutoRetryable = true + e.FallbackAllowed = true case CodePermissionDeniedByUser, CodeTask, CodeRepo, CodeAgentRuntime: e.FallbackAllowed = false e.AutoRetryable = false diff --git a/apps/backend/internal/agent/runtime/routingerr/runtime_rules.go b/apps/backend/internal/agent/runtime/routingerr/runtime_rules.go new file mode 100644 index 0000000000..853ac35985 --- /dev/null +++ b/apps/backend/internal/agent/runtime/routingerr/runtime_rules.go @@ -0,0 +1,69 @@ +package routingerr + +import "regexp" + +// runtimeEnvironmentRules match failures caused by the local execution +// environment (npm/npx cache state, missing binaries' install steps, +// etc.) rather than by the provider or remote API. They are tried by +// Classify after provider-specific rules and before the phase-based +// fallback, so a specific environment fingerprint wins over the +// low-confidence "phase.prestart.unknown" verdict. +// +// Each entry pairs a regex matcher with a builder that can extract +// signal-specific metadata (e.g. RemediationPath) from the raw text. +var runtimeEnvironmentRules = []runtimeRule{ + { + id: "npm.enotempty.npx.v1", + pattern: regexp.MustCompile(`(?s)npm error code ENOTEMPTY.*?_npx/[0-9a-f]+`), + build: func(text string) *Error { + path := extractNpxCachePath(text) + if path == "" { + return nil + } + return &Error{ + Code: CodeNpxCacheCorrupted, + Confidence: ConfHigh, + RemediationPath: path, + } + }, + }, +} + +type runtimeRule struct { + id string + pattern *regexp.Regexp + build func(text string) *Error +} + +// npxCachePathRe captures the cache root, e.g. +// `/Users/cfl/.npm/_npx/d820eb7d96bc2600` from any npm error line that +// references a file beneath it. The hash segment is hex (npm uses an +// 8-byte hex of the package spec). `[^\n]*?` lets the home prefix +// contain spaces (e.g. `/Users/John Doe/...`); RemediateNpxCache's +// `EvalSymlinks` + `$HOME/.npm/_npx/` prefix guard validates the path +// before any deletion, so we don't need the regex to do that work too. +var npxCachePathRe = regexp.MustCompile(`(/[^\n]*?/\.npm/_npx/[0-9a-f]+)`) + +func extractNpxCachePath(text string) string { + m := npxCachePathRe.FindStringSubmatch(text) + if len(m) < 2 { + return "" + } + return m[1] +} + +func matchRuntimeEnvironmentRules(text string) (*Error, bool) { + if text == "" { + return nil, false + } + for _, r := range runtimeEnvironmentRules { + if !r.pattern.MatchString(text) { + continue + } + if e := r.build(text); e != nil { + e.ClassifierRule = r.id + return e, true + } + } + return nil, false +} diff --git a/apps/backend/internal/agent/runtime/routingerr/runtime_rules_test.go b/apps/backend/internal/agent/runtime/routingerr/runtime_rules_test.go new file mode 100644 index 0000000000..40065d53f3 --- /dev/null +++ b/apps/backend/internal/agent/runtime/routingerr/runtime_rules_test.go @@ -0,0 +1,97 @@ +package routingerr + +import ( + "strings" + "testing" +) + +func TestMatchRuntimeEnvironmentRules_NpxENOTEMPTY(t *testing.T) { + stderr := strings.Join([]string{ + "npm error code ENOTEMPTY", + "npm error syscall rename", + "npm error path /Users/cfl/.npm/_npx/d820eb7d96bc2600/node_modules/@anthropic-ai/claude-agent-sdk-darwin-arm64", + "npm error dest /Users/cfl/.npm/_npx/d820eb7d96bc2600/node_modules/@anthropic-ai/.claude-agent-sdk-darwin-arm64-n6izKK54", + "npm error errno -66", + "npm error ENOTEMPTY: directory not empty", + }, "\n") + + got, ok := matchRuntimeEnvironmentRules(stderr) + if !ok { + t.Fatalf("expected match, got none") + } + if got.Code != CodeNpxCacheCorrupted { + t.Fatalf("Code = %q, want %q", got.Code, CodeNpxCacheCorrupted) + } + if got.ClassifierRule != "npm.enotempty.npx.v1" { + t.Fatalf("ClassifierRule = %q, want npm.enotempty.npx.v1", got.ClassifierRule) + } + if got.Confidence != ConfHigh { + t.Fatalf("Confidence = %q, want %q", got.Confidence, ConfHigh) + } + wantPath := "/Users/cfl/.npm/_npx/d820eb7d96bc2600" + if got.RemediationPath != wantPath { + t.Fatalf("RemediationPath = %q, want %q", got.RemediationPath, wantPath) + } +} + +func TestMatchRuntimeEnvironmentRules_NoMatch(t *testing.T) { + cases := []string{ + "", + "some unrelated error", + "npm error code EACCES", + "npm error code ENOTEMPTY without any _npx path", + } + for _, in := range cases { + if _, ok := matchRuntimeEnvironmentRules(in); ok { + t.Errorf("expected no match for %q", in) + } + } +} + +func TestExtractNpxCachePath_AllowsSpacesInHomePath(t *testing.T) { + // macOS user dirs commonly contain spaces; the regex must not stop + // at whitespace or the rule silently degrades on those machines. + text := "npm error path /Users/John Doe/.npm/_npx/abc123def4/node_modules/foo" + got := extractNpxCachePath(text) + want := "/Users/John Doe/.npm/_npx/abc123def4" + if got != want { + t.Errorf("extractNpxCachePath = %q, want %q", got, want) + } +} + +func TestClassify_NpxCacheCorrupted_WiredThroughClassify(t *testing.T) { + resetInjection() + exit := 190 + in := Input{ + Phase: PhaseSessionInit, + ProviderID: "claude-acp", + ExitCode: &exit, + Stderr: strings.Join([]string{ + "npm error code ENOTEMPTY", + "npm error path /Users/cfl/.npm/_npx/d820eb7d96bc2600/node_modules/@anthropic-ai/claude-agent-sdk-darwin-arm64", + "npm error ENOTEMPTY: directory not empty", + }, "\n"), + } + e := Classify(in) + if e == nil { + t.Fatal("expected non-nil Error") + } + if e.Code != CodeNpxCacheCorrupted { + t.Fatalf("Code = %q, want %q", e.Code, CodeNpxCacheCorrupted) + } + if !e.AutoRetryable { + t.Errorf("AutoRetryable = false, want true") + } + if !e.FallbackAllowed { + t.Errorf("FallbackAllowed = false, want true") + } + if e.RemediationPath != "/Users/cfl/.npm/_npx/d820eb7d96bc2600" { + t.Errorf("RemediationPath = %q, want /Users/cfl/.npm/_npx/d820eb7d96bc2600", e.RemediationPath) + } + if e.Phase != PhaseSessionInit { + t.Errorf("Phase = %q, want %q", e.Phase, PhaseSessionInit) + } + if e.ExitCode == nil || *e.ExitCode != 190 { + t.Errorf("ExitCode lost, got %v", e.ExitCode) + } +}