diff --git a/.github/scripts/e2e-tests-workflow-contract_test.py b/.github/scripts/e2e-tests-workflow-contract_test.py index d238efb5d4..9ad16b61e8 100644 --- a/.github/scripts/e2e-tests-workflow-contract_test.py +++ b/.github/scripts/e2e-tests-workflow-contract_test.py @@ -89,6 +89,17 @@ class DesktopE2EWorkflowContractTest(unittest.TestCase): ): self.assertIn(pattern, changes_job) + def test_normal_shard_has_queue_safe_timeout(self) -> None: + workflow = E2E_WORKFLOW.read_text(encoding="utf-8") + normal_job = job_block(workflow, "e2e", "e2e-containers") + + self.assertIn( + "# 35 min covers the serial count-fallback tail and setup overhead", + normal_job, + ) + self.assertIn("timeout-minutes: 35", normal_job) + self.assertNotIn("timeout-minutes: 25", normal_job) + def test_contract_runs_in_the_unfiltered_required_workflow(self) -> None: workflow = LINT_WORKFLOW.read_text(encoding="utf-8") diff --git a/.github/workflows/e2e-tests.yml b/.github/workflows/e2e-tests.yml index 198b8e74fe..8bf210ca69 100644 --- a/.github/workflows/e2e-tests.yml +++ b/.github/workflows/e2e-tests.yml @@ -347,10 +347,10 @@ jobs: container: image: ghcr.io/kdlbs/kandev-ci:runtime-latest options: --ipc=host - # 25 min (was 22) — leaves room for the worst-case "every shard hits a few - # flake-retries" combination without blowing past the cap. Average shard - # runs ~10 min, so this only kicks in when something is going wrong. - timeout-minutes: 25 + # 35 min covers the serial count-fallback tail and setup overhead. The + # first queue runs reached the old 25-minute cap with a healthy 167-test + # shard before the main timing profile had bootstrapped. + timeout-minutes: 35 # Container jobs default to `sh`; pin to bash. defaults: run: diff --git a/apps/backend/internal/agent/handlers/workspace_file_handlers.go b/apps/backend/internal/agent/handlers/workspace_file_handlers.go index 9c5619aab1..941475aeff 100644 --- a/apps/backend/internal/agent/handlers/workspace_file_handlers.go +++ b/apps/backend/internal/agent/handlers/workspace_file_handlers.go @@ -2,6 +2,7 @@ package handlers import ( "context" + "errors" "fmt" "strings" "unicode/utf8" @@ -117,6 +118,12 @@ func (h *WorkspaceFileHandlers) wsGetFileContent(ctx context.Context, msg *ws.Me // Request file content from agentctl response, err := client.RequestFileContent(ctx, req.Path, req.Repo) if err != nil { + if isMissingFileContentError(err) { + h.logger.Debug("file not found (expected for deleted or stale files)", + zap.String("session_id", req.SessionID), + zap.String("path", req.Path)) + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeNotFound, err.Error(), nil) + } h.logger.Error("failed to get file content", zap.Error(err), zap.String("session_id", req.SessionID), zap.String("path", req.Path)) return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, fmt.Sprintf("Failed to get file content: %v", err), nil) } @@ -124,6 +131,11 @@ func (h *WorkspaceFileHandlers) wsGetFileContent(ctx context.Context, msg *ws.Me return ws.NewResponse(msg.ID, msg.Action, response) } +func isMissingFileContentError(err error) bool { + // The client derives this sentinel from the agentctl file-content 404 status. + return errors.Is(err, agentctl.ErrFileNotFound) +} + // wsGetFileContentAtRef handles workspace.file.get_at_ref action func (h *WorkspaceFileHandlers) wsGetFileContentAtRef(ctx context.Context, msg *ws.Message) (*ws.Message, error) { var req struct { diff --git a/apps/backend/internal/agent/handlers/workspace_file_handlers_test.go b/apps/backend/internal/agent/handlers/workspace_file_handlers_test.go index 336d82819f..b066ea4a7e 100644 --- a/apps/backend/internal/agent/handlers/workspace_file_handlers_test.go +++ b/apps/backend/internal/agent/handlers/workspace_file_handlers_test.go @@ -12,7 +12,11 @@ import ( agentctl "github.com/kandev/kandev/internal/agent/runtime/agentctl" "github.com/kandev/kandev/internal/agent/runtime/lifecycle" + commonlogger "github.com/kandev/kandev/internal/common/logger" ws "github.com/kandev/kandev/pkg/websocket" + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" ) func TestWorkspaceFileHandlersValidateRequestsBeforeLookup(t *testing.T) { @@ -117,7 +121,80 @@ func TestWorkspaceFileHandlersMapDependencyFailures(t *testing.T) { } } +func TestWorkspaceFileHandlersMissingContentIsNotFoundAndDebug(t *testing.T) { + core, logs := observer.New(zapcore.DebugLevel) + log, err := commonlogger.NewFromZap(zap.New(core)) + if err != nil { + t.Fatalf("create observer logger: %v", err) + } + h := workspaceHandlerServerWithLogger(t, log, func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"error":"file not found: stat /workspace/gone.go: no such file or directory"}`)) + }) + msg, err := ws.NewRequest("id", ws.ActionWorkspaceFileContentGet, map[string]any{ + "session_id": "s", + "path": "gone.go", + }) + if err != nil { + t.Fatal(err) + } + + response, err := h.wsGetFileContent(context.Background(), msg) + if err != nil { + t.Fatalf("wsGetFileContent returned error: %v", err) + } + assertWorkspaceContentSearchErrorCode(t, response, ws.ErrorCodeNotFound) + + entries := logs.All() + if len(entries) != 1 { + t.Fatalf("observed %d log entries, want 1: %v", len(entries), entries) + } + if entries[0].Level != zap.DebugLevel { + t.Fatalf("log level = %s, want debug", entries[0].Level) + } + if entries[0].Message != "file not found (expected for deleted or stale files)" { + t.Fatalf("log message = %q, want expected missing-file message", entries[0].Message) + } +} + +func TestWorkspaceFileHandlersNonMissingContentFailureRemainsError(t *testing.T) { + core, logs := observer.New(zapcore.DebugLevel) + log, err := commonlogger.NewFromZap(zap.New(core)) + if err != nil { + t.Fatalf("create observer logger: %v", err) + } + h := workspaceHandlerServerWithLogger(t, log, func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(`{"error":"file not found: permission denied"}`)) + }) + msg, err := ws.NewRequest("id", ws.ActionWorkspaceFileContentGet, map[string]any{ + "session_id": "s", + "path": "private.go", + }) + if err != nil { + t.Fatal(err) + } + + response, err := h.wsGetFileContent(context.Background(), msg) + if err != nil { + t.Fatalf("wsGetFileContent returned error: %v", err) + } + assertWorkspaceContentSearchErrorCode(t, response, ws.ErrorCodeInternalError) + + entries := logs.All() + if len(entries) != 1 { + t.Fatalf("observed %d log entries, want 1: %v", len(entries), entries) + } + if entries[0].Level != zap.ErrorLevel { + t.Fatalf("log level = %s, want error", entries[0].Level) + } +} + func workspaceHandlerServer(t *testing.T, handler http.HandlerFunc) *WorkspaceFileHandlers { + return workspaceHandlerServerWithLogger(t, newTestLogger(), handler) +} + +func workspaceHandlerServerWithLogger(t *testing.T, log *commonlogger.Logger, handler http.HandlerFunc) *WorkspaceFileHandlers { t.Helper() server := httptest.NewServer(handler) t.Cleanup(server.Close) @@ -127,7 +204,7 @@ func workspaceHandlerServer(t *testing.T, handler http.HandlerFunc) *WorkspaceFi execution := &lifecycle.AgentExecution{SessionID: "s"} execution.SetAgentCtlClientForTesting(client) lookup := &mockExecutionLookup{executions: map[string]*lifecycle.AgentExecution{"s": execution}} - return NewWorkspaceFileHandlers(lookup, newTestLogger()) + return NewWorkspaceFileHandlers(lookup, log) } func expectWorkspaceQuery(key, want string) func(*testing.T, *http.Request) { diff --git a/apps/backend/internal/agent/runtime/agentctl/client_files.go b/apps/backend/internal/agent/runtime/agentctl/client_files.go index 90f27fd049..aeb2e635b5 100644 --- a/apps/backend/internal/agent/runtime/agentctl/client_files.go +++ b/apps/backend/internal/agent/runtime/agentctl/client_files.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -14,6 +15,9 @@ import ( "go.uber.org/zap" ) +// ErrFileNotFound identifies a file-content response with HTTP 404 status. +var ErrFileNotFound = errors.New("file not found") + // RequestFileTree requests a file tree via HTTP GET func (c *Client) RequestFileTree(ctx context.Context, path string, depth int) (*FileTreeResponse, error) { reqURL := fmt.Sprintf( @@ -149,6 +153,9 @@ func (c *Client) RequestFileContent(ctx context.Context, path, repo string) (*Fi } if response.Error != "" { + if resp.StatusCode == http.StatusNotFound { + return nil, fmt.Errorf("%w: file content error: %s", ErrFileNotFound, response.Error) + } return nil, fmt.Errorf("file content error: %s", response.Error) } diff --git a/apps/backend/internal/agent/runtime/agentctl/client_files_test.go b/apps/backend/internal/agent/runtime/agentctl/client_files_test.go index abddfe7743..f895c06d7f 100644 --- a/apps/backend/internal/agent/runtime/agentctl/client_files_test.go +++ b/apps/backend/internal/agent/runtime/agentctl/client_files_test.go @@ -3,6 +3,7 @@ package client import ( "context" "encoding/json" + "errors" "mime" "net/http" "strings" @@ -209,12 +210,27 @@ func TestRequestFileContent_AppendsEscapedRepoSubpath(t *testing.T) { } func TestRequestFileContent_SurfacesResponseErrorField(t *testing.T) { - srv, _ := captureServer(t, jsonResponder(http.StatusOK, `{"error":"file not found"}`)) + srv, _ := captureServer(t, jsonResponder(http.StatusNotFound, `{"error":"file not found"}`)) _, err := newHTTPOnlyClient(srv.URL).RequestFileContent(context.Background(), "gone.go", "") if err == nil || !strings.Contains(err.Error(), "file content error: file not found") { t.Fatalf("error = %v, want wrapped file content error", err) } + if !errors.Is(err, ErrFileNotFound) { + t.Fatalf("error = %v, want ErrFileNotFound", err) + } +} + +func TestRequestFileContent_DoesNotClassifyNonNotFoundStatus(t *testing.T) { + srv, _ := captureServer(t, jsonResponder(http.StatusBadRequest, `{"error":"file not found: permission denied"}`)) + + _, err := newHTTPOnlyClient(srv.URL).RequestFileContent(context.Background(), "private.go", "") + if err == nil { + t.Fatal("RequestFileContent returned nil error for error-bearing body") + } + if errors.Is(err, ErrFileNotFound) { + t.Fatalf("error = %v, want non-not-found error", err) + } } func TestRequestFileContentAtRef_SendsPathRefAndRepo(t *testing.T) { diff --git a/apps/backend/internal/agentctl/server/api/workspace.go b/apps/backend/internal/agentctl/server/api/workspace.go index 9be501054f..84c5a56688 100644 --- a/apps/backend/internal/agentctl/server/api/workspace.go +++ b/apps/backend/internal/agentctl/server/api/workspace.go @@ -168,7 +168,11 @@ func (s *Server) handleFileContent(c *gin.Context) { content, size, isBinary, resolvedPath, err := s.procMgr.GetWorkspaceTracker().GetFileContent(scopedPath) if err != nil { - c.JSON(400, types.FileContentResponse{Path: path, Error: err.Error(), Size: size}) + status := http.StatusBadRequest + if errors.Is(err, process.ErrFileNotFound) { + status = http.StatusNotFound + } + c.JSON(status, types.FileContentResponse{Path: path, Error: err.Error(), Size: size}) return } diff --git a/apps/backend/internal/agentctl/server/api/workspace_handlers_test.go b/apps/backend/internal/agentctl/server/api/workspace_handlers_test.go index 6550ea1c63..49f5f2d161 100644 --- a/apps/backend/internal/agentctl/server/api/workspace_handlers_test.go +++ b/apps/backend/internal/agentctl/server/api/workspace_handlers_test.go @@ -159,24 +159,26 @@ func TestHandleFileContent_ReturnsFileBytes(t *testing.T) { } } -// TestHandleFileContent_Rejections covers every 400 branch of the read path. +// TestHandleFileContent_Rejections covers validation and error-status branches +// of the read path. func TestHandleFileContent_Rejections(t *testing.T) { srv, _ := newWorkspaceFileServer(t) cases := []struct { - name string - query string - wantError string + name string + query string + wantStatus int + wantError string }{ - {"missing path", "", "path is required"}, - {"unresolvable repo", "?path=one.txt&repo=nope", "repo subpath not found"}, - {"missing file", "?path=absent.txt", "no such file"}, + {"missing path", "", http.StatusBadRequest, "path is required"}, + {"unresolvable repo", "?path=one.txt&repo=nope", http.StatusBadRequest, "repo subpath not found"}, + {"missing file", "?path=absent.txt", http.StatusNotFound, "no such file"}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { rec := workspaceRequest(t, srv, http.MethodGet, "/api/v1/workspace/file/content"+tc.query, nil) - if rec.Code != http.StatusBadRequest { - t.Fatalf("status = %d, want 400 (body %s)", rec.Code, rec.Body.String()) + if rec.Code != tc.wantStatus { + t.Fatalf("status = %d, want %d (body %s)", rec.Code, tc.wantStatus, rec.Body.String()) } got := decodeWorkspaceBody[streams.FileContentResponse](t, rec) if !strings.Contains(got.Error, tc.wantError) { diff --git a/apps/backend/internal/agentctl/server/process/workspace_files.go b/apps/backend/internal/agentctl/server/process/workspace_files.go index 3aca77f290..14330f07e6 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_files.go +++ b/apps/backend/internal/agentctl/server/process/workspace_files.go @@ -35,7 +35,11 @@ const ( const maxFileSize = 10 * 1024 * 1024 // 10MB -var errPathTraversal = errors.New("path traversal detected") +var ( + // ErrFileNotFound identifies a workspace file that does not exist. + ErrFileNotFound = errors.New("file not found") + errPathTraversal = errors.New("path traversal detected") +) // workspaceMutationBarrier provides a deterministic synchronization point for // filesystem-race regression tests. It is unset in production. @@ -413,7 +417,7 @@ func (wt *WorkspaceTracker) readResolvedPath(reqPath string) (string, int64, boo // original error so they aren't mislabeled as missing. if cleaned := filepath.Clean(reqPath); filepath.IsAbs(cleaned) { if _, statErr := os.Stat(cleaned); errors.Is(statErr, fs.ErrNotExist) { - return "", 0, false, "", fmt.Errorf("file not found: %w", statErr) + return "", 0, false, "", fmt.Errorf("%w: %w", ErrFileNotFound, statErr) } } return "", 0, false, "", err @@ -457,7 +461,10 @@ func readFileContent(safePath string) (string, int64, bool, error) { // codeql[go/path-injection] safePath is canonical containment-validated by resolveSafePath; read-only external paths reach here only through absoluteReadPath validation. info, err := os.Stat(safePath) if err != nil { - return "", 0, false, fmt.Errorf("file not found: %w", err) + if errors.Is(err, fs.ErrNotExist) { + return "", 0, false, fmt.Errorf("%w: %w", ErrFileNotFound, err) + } + return "", 0, false, fmt.Errorf("failed to stat file: %w", err) } if !info.Mode().IsRegular() { diff --git a/apps/backend/internal/agentctl/server/process/workspace_files_test.go b/apps/backend/internal/agentctl/server/process/workspace_files_test.go index df33257bdb..ad611f8e75 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_files_test.go +++ b/apps/backend/internal/agentctl/server/process/workspace_files_test.go @@ -2,6 +2,7 @@ package process import ( "context" + "errors" "fmt" "math" "os" @@ -133,6 +134,32 @@ func TestResolveNonExistentPath(t *testing.T) { }) } +func TestReadFileContent_PermissionErrorIsNotMissing(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("chmod 0o000 does not block filesystem checks on Windows") + } + if os.Getuid() == 0 { + t.Skip("skipping permission test: root bypasses filesystem permission checks") + } + + restrictedDir := t.TempDir() + if err := os.Chmod(restrictedDir, 0o000); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(restrictedDir, 0o755) }) + + _, _, _, err := readFileContent(filepath.Join(restrictedDir, "missing.txt")) + if err == nil { + t.Fatal("expected permission error, got nil") + } + if errors.Is(err, ErrFileNotFound) { + t.Fatalf("error = %v, want permission error, not ErrFileNotFound", err) + } + if strings.Contains(err.Error(), "file not found") { + t.Fatalf("error = %v, want no missing-file classification", err) + } +} + func TestWorkspaceFileOperationsAllowRegisteredLinkedSource(t *testing.T) { workspace := t.TempDir() source := t.TempDir() diff --git a/apps/backend/internal/worktree/manager_state.go b/apps/backend/internal/worktree/manager_state.go index d4fad1497b..3a12b29201 100644 --- a/apps/backend/internal/worktree/manager_state.go +++ b/apps/backend/internal/worktree/manager_state.go @@ -53,9 +53,9 @@ func (m *Manager) persistAndCacheWorktree(ctx context.Context, wt *Worktree, req return nil } -// persistWorktree writes the worktree to persistent storage, logging a warning -// when session_id or a resolvable task environment is missing and cleaning up -// the git worktree directory on failure. +// persistWorktree writes the worktree to persistent storage, logging a debug +// entry when session_id or a resolvable task environment is missing and +// cleaning up the git worktree directory on failure. // // Initial materialization runs before the executor persists the task // environment, so the store cannot resolve an environment yet — that launch's @@ -72,7 +72,7 @@ func (m *Manager) persistWorktree(ctx context.Context, wt *Worktree, req CreateR } if err := m.store.CreateWorktree(ctx, wt); err != nil { if errors.Is(err, ErrEnvironmentNotResolved) { - m.logger.Warn("skipping worktree persistence: session has no task environment yet", + m.logger.Debug("skipping worktree persistence: session has no task environment yet", zap.String("task_id", req.TaskID), zap.String("session_id", req.SessionID), zap.String("worktree_id", wt.ID)) diff --git a/apps/backend/internal/worktree/manager_state_test.go b/apps/backend/internal/worktree/manager_state_test.go new file mode 100644 index 0000000000..8620a36470 --- /dev/null +++ b/apps/backend/internal/worktree/manager_state_test.go @@ -0,0 +1,89 @@ +package worktree + +import ( + "context" + "errors" + "os" + "testing" + + commonlogger "github.com/kandev/kandev/internal/common/logger" + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" +) + +type createWorktreeErrorStore struct { + *mockStore + err error +} + +func (s *createWorktreeErrorStore) CreateWorktree(context.Context, *Worktree) error { + return s.err +} + +func newObservedWorktreeLogger(t *testing.T) (*commonlogger.Logger, *observer.ObservedLogs) { + t.Helper() + core, logs := observer.New(zapcore.DebugLevel) + log, err := commonlogger.NewFromZap(zap.New(core)) + if err != nil { + t.Fatalf("create observer logger: %v", err) + } + return log, logs +} + +func TestPersistWorktree_EnvironmentNotResolved(t *testing.T) { + log, logs := newObservedWorktreeLogger(t) + store := &createWorktreeErrorStore{ + mockStore: newMockStore(), + err: ErrEnvironmentNotResolved, + } + mgr := &Manager{store: store, logger: log} + worktreePath := t.TempDir() + + err := mgr.persistWorktree(context.Background(), &Worktree{ID: "wt-1"}, CreateRequest{ + TaskID: "task-1", + SessionID: "session-1", + }, worktreePath) + if err != nil { + t.Fatalf("persistWorktree returned error: %v", err) + } + if _, err := os.Stat(worktreePath); err != nil { + t.Fatalf("expected physical worktree path to remain: %v", err) + } + + entries := logs.FilterMessage("skipping worktree persistence: session has no task environment yet").All() + if len(entries) != 1 { + t.Fatalf("observed %d deferred-persistence entries, want 1", len(entries)) + } + if entries[0].Level != zap.DebugLevel { + t.Fatalf("log level = %s, want debug", entries[0].Level) + } + warningCount := 0 + for _, entry := range logs.All() { + if entry.Level == zap.WarnLevel { + warningCount++ + } + } + if warningCount != 0 { + t.Fatalf("observed %d warning entries, want 0", warningCount) + } +} + +func TestPersistWorktree_OtherStoreError(t *testing.T) { + storeErr := errors.New("store unavailable") + log, _ := newObservedWorktreeLogger(t) + store := &createWorktreeErrorStore{ + mockStore: newMockStore(), + err: storeErr, + } + mgr := &Manager{store: store, logger: log} + + err := mgr.persistWorktree(context.Background(), &Worktree{ID: "wt-2"}, CreateRequest{ + TaskID: "task-2", + SessionID: "session-2", + RepositoryPath: t.TempDir(), + }, t.TempDir()) + if !errors.Is(err, storeErr) { + t.Fatalf("error = %v, want wrapped store error", err) + } +} diff --git a/docs/plans/expected-runtime-log-severity/plan.md b/docs/plans/expected-runtime-log-severity/plan.md new file mode 100644 index 0000000000..54ab587342 --- /dev/null +++ b/docs/plans/expected-runtime-log-severity/plan.md @@ -0,0 +1,91 @@ +--- +spec: docs/specs/platform/expected-runtime-log-severity.md +created: 2026-08-23 +status: implemented +--- + +# Implementation Plan: Expected runtime log severity + +## Overview + +Classify two confirmed normal conditions at their correct severity. First, +workspace file reads will expose missing current-checkout paths as `NotFound` +and debug evidence. Second, initial worktree persistence that races task +environment creation will remain successful but emit debug evidence instead of +a warning. Non-missing file failures and non-environment worktree failures keep +their existing behavior. + +## Backend + +### Workspace file content classification + +- Preserve the server-side `fs.ErrNotExist` distinction in + `apps/backend/internal/agentctl/server/process/workspace_files.go` and carry + it across the agentctl HTTP boundary as HTTP 404. +- Map that status to a typed client sentinel, then have + `apps/backend/internal/agent/handlers/workspace_file_handlers.go` use the + sentinel in `wsGetFileContent` to log the expected condition at debug level + and return `ws.ErrorCodeNotFound`. +- Leave non-missing errors on the current error-level and + `ws.ErrorCodeInternalError` path. +- Add focused coverage for the server status, client sentinel, handler logs, + and genuine stat-failure path. + +### Initial worktree persistence severity + +- Update `apps/backend/internal/worktree/manager_state.go` so only the + `ErrEnvironmentNotResolved` branch uses debug severity. Keep the message + fields and return behavior stable. +- Add observer-backed coverage in + `apps/backend/internal/worktree/manager_state_test.go` for the typed + environment-not-resolved branch and verify that unrelated store errors still + follow the current error path. + +## Tests + +- **What:** Missing current-checkout file returns `not_found` and emits debug, + while a non-missing dependency failure remains `internal_error` and error. + **File:** `apps/backend/internal/agent/handlers/workspace_file_handlers_test.go`. + **How:** Use the real `agentctl.Client` against an `httptest.Server` and a + zap observer logger attached to the handler. +- **What:** Initial worktree persistence is debug-only and remains successful; + other store errors remain failures. + **File:** `apps/backend/internal/worktree/manager_state_test.go`. + **How:** Call the real `Manager.persistWorktree` with a focused fake store + returning typed errors and an observer logger. Do not create a Git worktree. + +## Verification Results + +- `cd apps/backend && go test ./internal/agent/handlers -run + 'TestWorkspaceFileHandlers(Missing|NonMissing).*' -count=1` passed after the + workspace-file change and final formatting. +- `cd apps/backend && go test ./internal/worktree -run + 'TestPersistWorktree_(EnvironmentNotResolved|OtherStoreError)' -count=1` + passed after the worktree change and final formatting. +- `cd apps/backend && go test ./internal/agent/handlers ./internal/worktree + -count=1` passed: both packages green. +- PR fixup RED: the client and process tests initially failed to compile + without the new `ErrFileNotFound` sentinels; the API rejection test reported + a missing file as 400; and the handler regression classified a 400 response + containing a legacy `file not found` message as `NOT_FOUND`. +- PR fixup GREEN: + `cd apps/backend && go test ./internal/agent/runtime/agentctl -run + 'TestRequestFileContent_(Surfaces|DoesNot).*' -count=1` passed; + `cd apps/backend && go test ./internal/agentctl/server/process -run + TestReadFileContent_PermissionErrorIsNotMissing -count=1` passed; + `cd apps/backend && go test ./internal/agentctl/server/api -run + TestHandleFileContent_Rejections -count=1` passed; and + `cd apps/backend && go test ./internal/agent/handlers -run + 'TestWorkspaceFileHandlers(Missing|NonMissing).*' -count=1` passed. + +## Implementation Waves And Parallel Candidates + +Execute sequentially in the primary session. The tasks touch disjoint backend +packages, but no delegation is authorized by this plan. + +- [x] [task-01-workspace-file-severity](task-01-workspace-file-severity.md) +- [x] [task-02-worktree-initialization-severity](task-02-worktree-initialization-severity.md) + +## Open Questions + +None. diff --git a/docs/plans/expected-runtime-log-severity/task-01-workspace-file-severity.md b/docs/plans/expected-runtime-log-severity/task-01-workspace-file-severity.md new file mode 100644 index 0000000000..2f7998edc7 --- /dev/null +++ b/docs/plans/expected-runtime-log-severity/task-01-workspace-file-severity.md @@ -0,0 +1,77 @@ +--- +id: "01-workspace-file-severity" +title: "Workspace file missing-file severity" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/platform/expected-runtime-log-severity.md" +--- + +# Task 01: Workspace file missing-file severity + +## Acceptance + +- A missing current-checkout file returned by agentctl produces a + `ws.ErrorCodeNotFound` response and a debug-level handler entry. +- A non-missing file-content failure still produces + `ws.ErrorCodeInternalError` and an error-level entry. +- The existing at-ref missing-file behavior and all successful file responses + remain unchanged. + +## Verification + +```bash +cd apps/backend && go test ./internal/agent/handlers -run 'TestWorkspaceFileHandlers(Missing|NonMissing).*' -count=1 +``` + +## Files likely touched + +- `apps/backend/internal/agent/handlers/workspace_file_handlers.go` +- `apps/backend/internal/agent/handlers/workspace_file_handlers_test.go` + +## Dependencies + +None. + +## Parallelism + +Sequential. + +## Inputs + +- Spec: workspace file classification scenarios and failure modes. +- Existing `workspaceHandlerServer` test helper and the at-ref missing-file + classification in `workspace_file_handlers.go`. +- Agentctl missing-file responses use HTTP 404 and the runtime client exposes a + typed `ErrFileNotFound` sentinel; non-missing responses retain their status. + +## Output contract + +Report the classification helper or branch changed, observer assertions, exact +test result, files changed, blockers, and task/plan status updates. + +## Results + +Implemented the missing current-checkout file classification in +`wsGetFileContent`. Agentctl file-not-found responses now return +`ws.ErrorCodeNotFound` and emit one debug-level entry; non-missing failures +remain `ws.ErrorCodeInternalError` and error-level. + +- RED: `cd apps/backend && go test ./internal/agent/handlers -run + 'TestWorkspaceFileHandlers(Missing|NonMissing).*' -count=1` failed because + the missing-file response was `INTERNAL_ERROR` instead of `NOT_FOUND`. +- GREEN: the same command passed after the handler change and after `gofmt`. +- PR fixup: replaced the error-string classifier with the typed client + sentinel, added the HTTP 404 status mapping, and preserved non-ENOENT stat + failures as ordinary errors. Added client, server, and process regression + coverage for the boundary and permission path. +- PR fixup verification passed: + `cd apps/backend && go test ./internal/agent/runtime/agentctl -run + 'TestRequestFileContent_(Surfaces|DoesNot).*' -count=1`; + `cd apps/backend && go test ./internal/agentctl/server/process -run + TestReadFileContent_PermissionErrorIsNotMissing -count=1`; + `cd apps/backend && go test ./internal/agentctl/server/api -run + TestHandleFileContent_Rejections -count=1`; and + `cd apps/backend && go test ./internal/agent/handlers -run + 'TestWorkspaceFileHandlers(Missing|NonMissing).*' -count=1`. diff --git a/docs/plans/expected-runtime-log-severity/task-02-worktree-initialization-severity.md b/docs/plans/expected-runtime-log-severity/task-02-worktree-initialization-severity.md new file mode 100644 index 0000000000..087c0486a4 --- /dev/null +++ b/docs/plans/expected-runtime-log-severity/task-02-worktree-initialization-severity.md @@ -0,0 +1,61 @@ +--- +id: "02-worktree-initialization-severity" +title: "Worktree initialization log severity" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/platform/expected-runtime-log-severity.md" +--- + +# Task 02: Worktree initialization log severity + +## Acceptance + +- `ErrEnvironmentNotResolved` remains a successful persistence skip and emits + the existing structured message at debug level, not warning level. +- The manager does not remove or otherwise alter the newly materialized + physical worktree on this expected path. +- Other store errors retain their existing cleanup and error behavior. + +## Verification + +```bash +cd apps/backend && go test ./internal/worktree -run 'TestPersistWorktree_(EnvironmentNotResolved|OtherStoreError)' -count=1 +``` + +## Files likely touched + +- `apps/backend/internal/worktree/manager_state.go` +- `apps/backend/internal/worktree/manager_state_test.go` + +## Dependencies + +None. + +## Parallelism + +Sequential. + +## Inputs + +- Spec: initial materialization and persistence failure scenarios. +- Existing `ErrEnvironmentNotResolved` contract in + `apps/backend/internal/worktree/errors.go`. +- Existing `Manager.persistWorktree` cleanup boundary. + +## Output contract + +Report the severity change, persistence assertion, exact test result, files +changed, blockers, and task/plan status updates. + +## Results + +Changed only the `ErrEnvironmentNotResolved` branch in `persistWorktree` from +warning to debug severity. The branch still returns success without cleanup; +unrelated store errors remain wrapped and trigger the existing cleanup path. + +- RED: `cd apps/backend && go test ./internal/worktree -run + 'TestPersistWorktree_(EnvironmentNotResolved|OtherStoreError)' -count=1` + failed because the expected branch logged at `warn` instead of `debug`. +- GREEN: the same command passed after the severity change and after `gofmt`. diff --git a/docs/plans/merge-queue-e2e-shard-timeout/plan.md b/docs/plans/merge-queue-e2e-shard-timeout/plan.md new file mode 100644 index 0000000000..73de584afb --- /dev/null +++ b/docs/plans/merge-queue-e2e-shard-timeout/plan.md @@ -0,0 +1,60 @@ +--- +status: in_progress +created: 2026-08-23 +updated: 2026-08-23 +spec: ../../specs/e2e-duration-aware-sharding/spec.md +--- + +# Keep merge-queue E2E checks within the response budget + +## Root cause + +PR #2950 was removed from the merge queue because the required `E2E Tests +Passed` check failed. The normal shard 14 job reached its 25-minute job +timeout twice, at `14:11:29Z` and `13:46:49Z`, after the same 167-test serial +plan had run for about 24 minutes. The report job and aggregate gate then +failed because the shard was cancelled. The queue ruleset allows 60 minutes +for required checks, so the 25-minute shard cap was the smaller limit. + +The run used the deterministic count fallback because no successful `main` +timing-profile artifact existed yet. The fix gives that fallback's normal +shard enough bounded execution time to finish without changing per-test +timeouts, retry behavior, or the fail-closed aggregate gate. + +## Outcome + +Set the normal E2E matrix job timeout to 35 minutes and protect that queue +budget with the existing workflow contract test. Keep the container, desktop, +report, and Playwright timeouts unchanged. + +## Wave 1: queue-safe normal shard budget + +Task 01 updates the workflow contract, the E2E workflow, and the affected +spec/plan records. It runs the focused contract test and checks that the +worktree diff is valid. + +## Verification + +```text +python3 .github/scripts/e2e-tests-workflow-contract_test.py +git diff --check +``` + +The next merge-group run is the operational verification: shard 14 must reach +a terminal test result, the report must merge, and `E2E Tests Passed` must be +successful for the PR to remain in the queue. + +## Status + +- [x] Confirm queue removal event and failed synthetic merge-group checks. +- [x] Confirm the repeated shard timeout and ruleset response budget. +- [x] Add the regression contract and raise only the normal shard job budget. +- [ ] Push the fix to PR #2950 and re-check the current-head queue state. + +## Local implementation result + +- Added a workflow contract assertion for the normal 35-minute shard budget. +- The new assertion failed against the old 25-minute workflow, then passed + after the workflow change. +- `python3 .github/scripts/e2e-tests-workflow-contract_test.py` passed, 5 tests. +- `git diff --check` passed. diff --git a/docs/plans/merge-queue-e2e-shard-timeout/task-01-queue-safe-normal-shard.md b/docs/plans/merge-queue-e2e-shard-timeout/task-01-queue-safe-normal-shard.md new file mode 100644 index 0000000000..d25cfcd8e1 --- /dev/null +++ b/docs/plans/merge-queue-e2e-shard-timeout/task-01-queue-safe-normal-shard.md @@ -0,0 +1,47 @@ +--- +id: "01-queue-safe-normal-shard" +title: "Give the normal E2E shard a queue-safe budget" +status: completed +depends_on: [] +plan: plan.md +spec: ../../specs/e2e-duration-aware-sharding/spec.md +parallelism: sequential +--- + +# Task 01: Give the normal E2E shard a queue-safe budget + +## Acceptance + +- The normal `e2e` matrix job uses a 35-minute job timeout. +- The container, desktop, report, and individual Playwright test timeouts stay + unchanged. +- The workflow contract test fails if the normal queue budget regresses to the + old 25-minute cap. +- The aggregate gate continues to reject cancelled jobs. + +## Files + +- `.github/workflows/e2e-tests.yml` +- `.github/scripts/e2e-tests-workflow-contract_test.py` +- `docs/specs/e2e-duration-aware-sharding/spec.md` +- `docs/plans/merge-queue-e2e-shard-timeout/plan.md` + +## TDD sequence + +1. Add the contract assertion and run it to prove the old 25-minute workflow + fails the new queue-budget requirement. +2. Change only the normal matrix timeout to 35 minutes. +3. Re-run the contract test and `git diff --check`. + +## Operational verification + +The merge-group run for the pushed PR must complete the normal shard matrix, +report merge, and `E2E Tests Passed` gate without a timeout removal event. + +## Implementation result + +- Changed only the normal `e2e` matrix job from 25 to 35 minutes. +- Added the queue-budget contract assertion. +- The RED contract run failed on the old timeout; the GREEN run passed all 5 + contract tests. +- `git diff --check` passed. diff --git a/docs/specs/INDEX.md b/docs/specs/INDEX.md index 2a787fd574..f2dc7f55d6 100644 --- a/docs/specs/INDEX.md +++ b/docs/specs/INDEX.md @@ -67,6 +67,7 @@ Product-wide capabilities that are not tied to a single feature area. | [git-credential-lease-reissue](git-credential-lease-reissue/spec.md) | shipped | | [bounded-task-status-delivery](platform/bounded-task-status-delivery.md) | approved | | [diagnostic-logging](platform/diagnostic-logging.md) | approved | +| [expected runtime log severity](platform/expected-runtime-log-severity.md) | building | | [provider-error-recovery](platform/provider-error-recovery.md) | draft | | [duplicated-tab-stale-data](fix-duplicated-tab-stale-data/spec.md) | building | | [health-endpoint-version](health-endpoint-version/spec.md) | building | diff --git a/docs/specs/e2e-duration-aware-sharding/spec.md b/docs/specs/e2e-duration-aware-sharding/spec.md index b692049efd..b3bdc9ac6d 100644 --- a/docs/specs/e2e-duration-aware-sharding/spec.md +++ b/docs/specs/e2e-duration-aware-sharding/spec.md @@ -1,7 +1,7 @@ --- status: shipped created: 2026-08-10 -updated: 2026-08-18 +updated: 2026-08-23 --- # Duration-aware E2E sharding and CI reliability @@ -114,6 +114,12 @@ contract. retry summary, prediction-versus-actual summary, and plan health counters. 5. Only a successful `main` result is eligible to become the next baseline. +The normal merge-queue shard job has a 35-minute execution budget. This is a +job-capacity budget for the serial fallback plan and its setup overhead, not a +change to Playwright's 60-second per-test timeout or retry policy. The +aggregate `E2E Tests Passed` gate still treats a cancelled shard as a failure, +so a genuinely stuck shard remains visible and blocks the merge. + The desktop smoke job uses a dedicated image from `ghcr.io/kdlbs/kandev-ci`. The image contains Node.js, pnpm, Rust, Xvfb, and the Linux packages that Tauri needs. The job does not contact Ubuntu or Rust toolchain servers during setup. @@ -188,6 +194,9 @@ they improve wall time without increasing flakes. visible in the retry summary with actionable evidence. - The timing profile can be unavailable without blocking a correct, deterministic fallback plan. +- A count-fallback normal shard can finish within the merge queue's + 60-minute check-response window, including setup, report merging, and the + aggregate gate. - The desktop smoke job reaches its build and test commands without contacting Ubuntu package mirrors or Rust toolchain servers. - A contract test fails if the desktop image publisher disappears or the smoke diff --git a/docs/specs/platform/expected-runtime-log-severity.md b/docs/specs/platform/expected-runtime-log-severity.md new file mode 100644 index 0000000000..ec0b8fa73f --- /dev/null +++ b/docs/specs/platform/expected-runtime-log-severity.md @@ -0,0 +1,80 @@ +--- +status: building +created: 2026-08-23 +owner: kandev +--- + +# Expected runtime log severity + +## Why + +Normal review and first-launch timing conditions currently look like backend +failures in the logs. This obscures actionable errors and makes diagnostic +bundles harder to triage. + +## What + +- A `workspace.file.get` request for a path that is absent from the current + checkout returns the existing `not_found` WebSocket error code and records a + debug entry. It does not record an error entry. +- A `workspace.file.get` failure that is not a missing-file condition keeps the + existing `internal_error` response and error-level log entry. +- When initial worktree materialization runs before its task environment exists, + the typed `ErrEnvironmentNotResolved` condition records a debug entry. The + physical worktree remains available and the existing launch transaction still + owns persistence of its record. +- Other worktree persistence failures keep their existing return, cleanup, and + logging behavior. + +## API surface + +The existing `workspace.file.get` WebSocket action keeps its request and +response shapes. Only the response classification for a missing current file +is made explicit as `not_found`; no new action or payload field is added. The +agentctl file-content endpoint carries this classification with HTTP 404 and a +typed client sentinel. Other file-content failures keep their existing error +status and message path. + +## Failure modes + +- Missing files are expected during deleted-file review and stale diff + requests. They are handled as not-found results and are not promoted to + application failures. +- Permission, transport, malformed-response, and other non-missing failures + remain internal errors and continue to be logged at error level. +- A worktree store that reports `ErrEnvironmentNotResolved` is a normal launch + ordering condition. The manager skips that persistence attempt and does not + remove the newly created physical worktree. +- Any other worktree store failure remains an actual persistence failure and + follows the current cleanup and retry boundary. + +## Persistence guarantees + +This repair changes log severity and missing-file classification only. The +agentctl file-content transport now distinguishes missing files with HTTP 404; +it does not change task-environment ownership, worktree creation, launch +persistence, or cleanup behavior across backend restarts. + +## Scenarios + +- **GIVEN** an active session and an absent current-checkout path, **WHEN** + `workspace.file.get` requests that path, **THEN** the response has the + `not_found` error code and exactly one debug-level missing-file entry is + emitted. +- **GIVEN** an active session and a dependency failure unrelated to a missing + file, **WHEN** `workspace.file.get` requests a path, **THEN** the response + has the `internal_error` error code and an error-level entry is emitted. +- **GIVEN** a physical worktree has been materialized before its task + environment row exists, **WHEN** worktree persistence receives + `ErrEnvironmentNotResolved`, **THEN** the call succeeds, emits a debug-level + deferred-persistence entry, and does not remove the physical worktree. +- **GIVEN** worktree persistence receives any other store error, **WHEN** the + manager handles it, **THEN** the existing error and cleanup behavior remains + unchanged. + +## Out of scope + +- Changing frontend diff rendering or review-file status behavior. +- Changing worktree ownership, task-environment transactions, cleanup policy, + or startup reconciliation. +- Reclassifying unrelated integration, ACP, or startup warnings.