diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1c19c6d1..fcfb145c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -124,8 +124,13 @@ jobs: with: version: latest working-directory: go - # AX: Skip test files (covered by test-go job), timeout for large codebase - args: --timeout=5m --tests=false + # Tests are linted, not skipped. --tests=false was hiding two things + # at once: it reported 21 production symbols as unused because the + # only callers are in _test.go files — every one of them a + # test-injection seam like syncPull, mcpInitialize or newCoreAgent — + # while never reporting the dead scaffolding actually in the test + # files. Linting tests swaps 21 false positives for 28 real ones. + args: --timeout=5m # Guards the one rule that has silently cost delivered features: the sibling # dappco.re/* modules are dependencies, not workspace files. diff --git a/go/.golangci.yml b/go/.golangci.yml new file mode 100644 index 00000000..76c599d4 --- /dev/null +++ b/go/.golangci.yml @@ -0,0 +1,35 @@ +# golangci-lint configuration. +# +# Policy lived in a CI argument (--tests=false) until now, which is why it was +# both wrong and invisible. Two problems it caused: +# +# * Skipping test files made `unused` report 21 production symbols as dead +# because their only callers are in _test.go — every one a deliberate +# test-injection seam (syncPull, mcpInitialize, mcpCall, readSSEData, +# newCoreAgent, pokeCh and friends). The linter cannot see a caller it has +# been told not to read. +# * It simultaneously hid the dead scaffolding actually inside the test +# files, which is where all 28 genuine `unused` findings turn out to live. +# +# So tests are linted. errcheck is excluded for them instead, which is the +# narrower and honest cut: an unchecked Close() in test setup is noise, while +# an unchecked write-close in production can silently lose data — as two did in +# pkg/chathistory before this sweep. +version: "2" + +linters: + exclusions: + rules: + # Unchecked errors in tests are conventional and not worth the churn; + # everything else still applies to them, which is the point. + - path: '_test\.go' + linters: + - errcheck + +issues: + # Report everything. The defaults are max-issues-per-linter: 50 and + # max-same-issues: 3, which silently truncate — the debt in this repo read as + # "90 issues" for exactly as long as nobody passed these, and is really 839. + # A capped number is not a measurement. + max-issues-per-linter: 0 + max-same-issues: 0 diff --git a/go/cmd/core-agent/commands_chat.go b/go/cmd/core-agent/commands_chat.go index 984b6021..e1e87ae7 100644 --- a/go/cmd/core-agent/commands_chat.go +++ b/go/cmd/core-agent/commands_chat.go @@ -53,7 +53,13 @@ func (commands applicationCommandSet) chat(opts core.Options) core.Result { applicationPrint("chat: open archive: %v", err) return core.Result{} } - defer hist.Close() + defer func() { + // Reported, not dropped: this handle is written to, so a failed + // close can mean the last turns never reached the archive. + if err := hist.Close(); err != nil { + core.Warn("chat: failed to close history archive", "reason", err) + } + }() svc := lemma.New(lemma.Config{ BaseURL: baseURL, @@ -78,7 +84,9 @@ func (commands applicationCommandSet) chat(opts core.Options) core.Result { scanner := bufio.NewScanner(core.Stdin()) scanner.Buffer(make([]byte, 64*1024), 1024*1024) // allow long prompts for { - core.WriteString(stdout, "you: ") + // Prompt only: a failed terminal write is cosmetic and there is + // nowhere to return an error to inside the loop. + _ = core.WriteString(stdout, "you: ") if !scanner.Scan() { break } diff --git a/go/cmd/core-agent/lemma_mcp.go b/go/cmd/core-agent/lemma_mcp.go index e750bd12..224388e4 100644 --- a/go/cmd/core-agent/lemma_mcp.go +++ b/go/cmd/core-agent/lemma_mcp.go @@ -113,7 +113,13 @@ func (s *lemmaSubsystem) handleSend(ctx context.Context, input LemmaSendInput) ( if err != nil { return nil, LemmaSendOutput{}, err } - defer hist.Close() + defer func() { + // Reported, not dropped: this handle is written to, so a failed + // close can mean the last turns never reached the archive. + if err := hist.Close(); err != nil { + core.Warn("chat: failed to close history archive", "reason", err) + } + }() cfg := s.cfg cfg.History = hist diff --git a/go/cmd/core-agent/main.go b/go/cmd/core-agent/main.go index 35b9f2a4..158ec15a 100644 --- a/go/cmd/core-agent/main.go +++ b/go/cmd/core-agent/main.go @@ -125,7 +125,14 @@ var runApp = func(coreApp *core.Core, cliArgs []string) error { return core.E("main.runApp", "core is required", nil) } - defer coreApp.ServiceShutdown(context.Background()) + defer func() { + // Reported on the way out: a shutdown that fails can leave a + // socket or database handle behind, and this is the last chance + // anything gets to say so. + if r := coreApp.ServiceShutdown(context.Background()); !r.OK { + core.Warn("core-agent: service shutdown failed", "reason", r.Value) + } + }() result := coreApp.ServiceStartup(coreApp.Context(), nil) if !result.OK { diff --git a/go/pkg/agentic/auto_pr.go b/go/pkg/agentic/auto_pr.go index 8bc461f8..c97e72e2 100644 --- a/go/pkg/agentic/auto_pr.go +++ b/go/pkg/agentic/auto_pr.go @@ -93,7 +93,9 @@ func (s *PrepSubsystem) autoCreatePR(workspaceDir string) { return } workspaceStatusUpdate.PRURL = pullRequestURL - writeStatusResult(workspaceDir, workspaceStatusUpdate) + if r := writeStatusResult(workspaceDir, workspaceStatusUpdate); !r.OK { + core.Warn("agentic: failed to record PR URL on status", "workspace", workspaceDir, "reason", r.Value) + } } } diff --git a/go/pkg/agentic/dispatch.go b/go/pkg/agentic/dispatch.go index a2f74e77..29311525 100644 --- a/go/pkg/agentic/dispatch.go +++ b/go/pkg/agentic/dispatch.go @@ -654,8 +654,9 @@ func (s *PrepSubsystem) broadcastComplete(agent, workspaceDir, finalStatus strin }); !result.OK { core.Warn("agentic.broadcastComplete: notification failed", "reason", result.Error()) } - // Push to MCP channel so Claude Code receives the notification - s.Core().ACTION(coremcp.ChannelPush{ + // Push to MCP channel so Claude Code receives the notification. + // Best-effort: a listener that is gone must not fail the dispatch. + _ = s.Core().ACTION(coremcp.ChannelPush{ Channel: coremcp.ChannelAgentComplete, Data: map[string]any{ "agent": agent, "repo": repo, @@ -686,7 +687,9 @@ func (s *PrepSubsystem) onAgentComplete(agent, workspaceDir, outputFile string, workspaceStatus.Status = finalStatus workspaceStatus.PID = 0 workspaceStatus.Question = question - writeStatusResult(workspaceDir, workspaceStatus) + if r := writeStatusResult(workspaceDir, workspaceStatus); !r.OK { + core.Warn("agentic: failed to write blocked status", "workspace", workspaceDir, "reason", r.Value) + } s.TrackWorkspace(WorkspaceName(workspaceDir), workspaceStatus) s.trackFailureRate(agent, finalStatus, workspaceStatus.StartedAt) @@ -891,7 +894,9 @@ var dispatch = func(s *PrepSubsystem, ctx context.Context, callRequest *mcp.Call StartedAt: time.Now(), Runs: 0, } - writeStatusResult(workspaceDir, workspaceStatus) + if r := writeStatusResult(workspaceDir, workspaceStatus); !r.OK { + core.Warn("agentic: failed to write requeued status", "workspace", workspaceDir, "reason", r.Value) + } if runnerResult := s.Core().Service("runner"); runnerResult.OK { if runnerSvc, ok := runnerResult.Value.(workspaceTracker); ok { runnerSvc.TrackWorkspace(WorkspaceName(workspaceDir), workspaceStatus) diff --git a/go/pkg/brain/messaging.go b/go/pkg/brain/messaging.go index fa820cfa..ff79fff5 100644 --- a/go/pkg/brain/messaging.go +++ b/go/pkg/brain/messaging.go @@ -121,7 +121,9 @@ func (s *DirectSubsystem) notifySelf(ctx context.Context, input SendInput) { return } for session := range mcpSvc.Sessions() { - coremcp.NotifySession(ctx, session, "notifications/claude/channel", map[string]any{ + // Best-effort per session: one unreachable listener must not stop + // the loop notifying the rest. + _ = coremcp.NotifySession(ctx, session, "notifications/claude/channel", map[string]any{ "content": input.Content, "meta": map[string]string{ "from": agentic.AgentName(), diff --git a/go/pkg/chathistory/chathistory.go b/go/pkg/chathistory/chathistory.go index 6a8724bd..56342b8b 100644 --- a/go/pkg/chathistory/chathistory.go +++ b/go/pkg/chathistory/chathistory.go @@ -288,7 +288,7 @@ func (h *History) LoadTurns(conversationID string) ([]Turn, error) { if err != nil { return nil, core.E("chathistory.LoadTurns", "query", err) } - defer rows.Close() + defer func() { _ = rows.Close() }() // read cursor var out []Turn for rows.Next() { var t Turn diff --git a/go/pkg/chathistory/export.go b/go/pkg/chathistory/export.go index d3cf7fb2..8f8f912b 100644 --- a/go/pkg/chathistory/export.go +++ b/go/pkg/chathistory/export.go @@ -35,7 +35,7 @@ func (h *History) CopyTo(dest string) error { return core.E("chathistory.CopyTo", "open source", srcResult.Value.(error)) } src := srcResult.Value.(*core.OSFile) - defer src.Close() + defer func() { _ = src.Close() }() // read handle: nothing to lose on close if dir := core.PathDir(dest); dir != "" { if r := core.MkdirAll(dir, 0o755); !r.OK { return core.E("chathistory.CopyTo", "mkdir dest parent", r.Value.(error)) @@ -46,10 +46,17 @@ func (h *History) CopyTo(dest string) error { return core.E("chathistory.CopyTo", "create dest", dstResult.Value.(error)) } dst := dstResult.Value.(*core.OSFile) - defer dst.Close() + // Net for the error paths below; the success path closes explicitly. + defer func() { _ = dst.Close() }() if _, err := io.Copy(dst, src); err != nil { return core.E("chathistory.CopyTo", "copy bytes", err) } + // Checked, not deferred-and-forgotten: a write handle that fails to close + // may never have flushed, and returning nil there reports a copy that did + // not fully happen. + if err := dst.Close(); err != nil { + return core.E("chathistory.CopyTo", "close dest", err) + } return nil } @@ -107,7 +114,8 @@ func (h *History) ExportJSONL(dest string) error { return core.E("chathistory.ExportJSONL", "create dest", fResult.Value.(error)) } f := fResult.Value.(*core.OSFile) - defer f.Close() + // Net for the error paths below; the success path closes explicitly. + defer func() { _ = f.Close() }() convRows, err := h.db.Query( `SELECT id, user_id, title, started_at, ended_at, model_id, base_model, @@ -118,7 +126,7 @@ func (h *History) ExportJSONL(dest string) error { if err != nil { return core.E("chathistory.ExportJSONL", "query conversations", err) } - defer convRows.Close() + defer func() { _ = convRows.Close() }() // read cursor for convRows.Next() { var c JSONLConversation @@ -163,7 +171,7 @@ func (h *History) ExportJSONL(dest string) error { &toolCalls, &toolResults, &t.CreatedAt, &tokensIn, &tokensOut, &signal, ); err != nil { - turnRows.Close() + _ = turnRows.Close() // read cursor; the scan error is what matters return core.E("chathistory.ExportJSONL", "scan turn", err) } if toolCalls.Valid { @@ -181,7 +189,7 @@ func (h *History) ExportJSONL(dest string) error { t.Signal = signal.String c.Turns = append(c.Turns, t) } - turnRows.Close() + _ = turnRows.Close() // read cursor marshalled := core.JSONMarshal(c) if !marshalled.OK { @@ -195,5 +203,13 @@ func (h *History) ExportJSONL(dest string) error { return core.E("chathistory.ExportJSONL", "write newline", err) } } + + // Checked, not deferred-and-forgotten: every line above can be written + // successfully and still be lost if the final flush fails, which would + // report a complete export of a truncated file. + if err := f.Close(); err != nil { + return core.E("chathistory.ExportJSONL", "close dest", err) + } + return nil } diff --git a/go/pkg/lemma/admin.go b/go/pkg/lemma/admin.go index 002d37ec..339c9766 100644 --- a/go/pkg/lemma/admin.go +++ b/go/pkg/lemma/admin.go @@ -308,7 +308,7 @@ func (a *Admin) doJSON(ctx context.Context, method, path string, body, out any) if err != nil { return core.E("lemma.Admin.doJSON", "transport", err) } - defer resp.Body.Close() + defer func() { _ = resp.Body.Close() }() // read side of the response respBody, _ := io.ReadAll(io.LimitReader(resp.Body, 1<<20)) if resp.StatusCode >= 400 { diff --git a/go/pkg/lemma/lemma.go b/go/pkg/lemma/lemma.go index b50c36b8..93585f33 100644 --- a/go/pkg/lemma/lemma.go +++ b/go/pkg/lemma/lemma.go @@ -287,7 +287,7 @@ func (s *Service) callChatCompletions(ctx context.Context, messages []chatMessag if err != nil { return "", 0, 0, err } - defer resp.Body.Close() + defer func() { _ = resp.Body.Close() }() // read side of the response rawBody, err := io.ReadAll(resp.Body) if err != nil { diff --git a/go/pkg/monitor/harvest.go b/go/pkg/monitor/harvest.go index 64811039..f9f70704 100644 --- a/go/pkg/monitor/harvest.go +++ b/go/pkg/monitor/harvest.go @@ -43,12 +43,15 @@ func (m *Subsystem) harvestCompleted() string { if h.rejected != "" { parts = append(parts, core.Sprintf("%s: REJECTED (%s)", h.repo, h.rejected)) if m.ServiceRuntime != nil { - m.Core().ACTION(messages.HarvestRejected{Repo: h.repo, Branch: h.branch, Reason: h.rejected}) + // Best-effort notification: a failed dispatch must not + // abort the harvest that produced it. + _ = m.Core().ACTION(messages.HarvestRejected{Repo: h.repo, Branch: h.branch, Reason: h.rejected}) } } else { parts = append(parts, core.Sprintf("%s: ready-for-review %s (%d files)", h.repo, h.branch, h.files)) if m.ServiceRuntime != nil { - m.Core().ACTION(messages.HarvestComplete{Repo: h.repo, Branch: h.branch, Files: h.files}) + // Best-effort notification, as above. + _ = m.Core().ACTION(messages.HarvestComplete{Repo: h.repo, Branch: h.branch, Files: h.files}) } } } diff --git a/go/pkg/monitor/monitor.go b/go/pkg/monitor/monitor.go index 6a3d2446..343f8b0b 100644 --- a/go/pkg/monitor/monitor.go +++ b/go/pkg/monitor/monitor.go @@ -245,7 +245,9 @@ func (m *Subsystem) checkIdleAfterDelay() { running, queued := m.countLiveWorkspaces() if running == 0 && queued == 0 { - m.Core().ACTION(messages.QueueDrained{Completed: 0}) + // Best-effort: a listener that has gone away must not stop the + // monitor noticing the queue drained. + _ = m.Core().ACTION(messages.QueueDrained{Completed: 0}) } } @@ -333,7 +335,9 @@ func (m *Subsystem) check(ctx context.Context) { m.notify(ctx, combinedMessage) if m.svc != nil { - m.svc.Server().ResourceUpdated(ctx, &mcp.ResourceUpdatedNotificationParams{ + // Best-effort: a subscriber that cannot be notified must not + // stop the status update itself. + _ = m.svc.Server().ResourceUpdated(ctx, &mcp.ResourceUpdatedNotificationParams{ URI: "status://agents", }) } @@ -403,7 +407,9 @@ func (m *Subsystem) checkCompletions() string { liveRunning, liveQueued := m.countLiveWorkspaces() if m.ServiceRuntime != nil && liveRunning == 0 && liveQueued == 0 { - m.Core().ACTION(messages.QueueDrained{Completed: len(newlyCompleted)}) + // Best-effort: a listener that has gone away must not stop the + // monitor noticing the queue drained. + _ = m.Core().ACTION(messages.QueueDrained{Completed: len(newlyCompleted)}) } msg := core.Sprintf("%d agent(s) completed", len(newlyCompleted)) @@ -505,7 +511,9 @@ func (m *Subsystem) notify(ctx context.Context, message string) { } for session := range m.svc.Server().Sessions() { - session.Log(ctx, &mcp.LoggingMessageParams{ + // Best-effort: one unreachable session must not stop the loop + // logging to the others. + _ = session.Log(ctx, &mcp.LoggingMessageParams{ Level: "info", Logger: "monitor", Data: message, diff --git a/go/pkg/runner/runner.go b/go/pkg/runner/runner.go index d2f01001..4a24c93e 100644 --- a/go/pkg/runner/runner.go +++ b/go/pkg/runner/runner.go @@ -284,8 +284,10 @@ func (s *Service) TrackWorkspace(name string, status any) { if workspaceStatus == nil { return } - s.workspaces.Set(name, workspaceStatus) - s.workspaces.Delete(core.Concat("pending/", workspaceStatus.Repo)) + if r := s.workspaces.Set(name, workspaceStatus); !r.OK { + core.Warn("runner: failed to track workspace", "workspace", name, "reason", r.Value) + } + _ = s.workspaces.Delete(core.Concat("pending/", workspaceStatus.Repo)) // best-effort: the pending marker may already be gone } // s.Workspaces().Each(func(name string, workspaceStatus *WorkspaceStatus) { core.Println(name, workspaceStatus.Status) })