From a581dfd90a8b69abd30943c16138c0775f5ac31d Mon Sep 17 00:00:00 2001 From: Snider Date: Sat, 8 Aug 2026 13:45:11 +0100 Subject: [PATCH] fix(lint): measure the Go lint debt honestly, and clear the production half MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reported debt was never the debt. golangci-lint defaults to max-issues-per-linter: 50 and max-same-issues: 3, so every count taken from it here — 101 with --tests=false, 90 without — was a display cap. Uncapped, the same code reports 839. Fixing eleven errcheck findings moved the total by zero, because eleven suppressed ones surfaced to replace them, which is what exposed it. A capped number is not a measurement, so the config now sets both to 0. Policy moves out of a CI argument into .golangci.yml, and --tests=false goes. That flag was doing two harmful things at once. It made `unused` report 21 production symbols as dead because their only callers live in _test.go — syncPull, mcpInitialize, mcpCall, readSSEData, newCoreAgent, pokeCh and the rest, every one a deliberate test-injection seam, since a linter cannot see a caller it has been told not to read. And it hid the dead scaffolding actually inside the test files, which is where all 28 real `unused` findings live. errcheck is excluded for tests instead: the narrower, honest cut, because an unchecked Close() in test setup is noise while an unchecked write-close in production loses data. Two of those production cases were losing data. chathistory's CopyTo and ExportJSONL both closed their destination file with a bare defer, so a close that failed to flush returned nil and reported a complete copy of a truncated file. Both now close explicitly on the success path and report the error, with the defer kept as the net for early returns. The rest are decided per site rather than silenced in bulk. Read cursors and response bodies say so and drop the error. Notifications — Core.ACTION, NotifySession, ResourceUpdated, session.Log — are best-effort by design, and say why: a listener that has gone away must not fail the work that produced the event. Status writes get the opposite treatment and now report through core.Warn, the idiom already used in those files, because the status file is what the monitor polls and a silent failure leaves a workspace looking stuck forever. Same for the runner losing track of a workspace, and for a shutdown that cannot complete. Two signatures I had assumed wrong and the compiler caught: core.WriteString returns a Result, not (int, error), and ServiceShutdown returns a Result too. Remaining, and now countable: 59 production errcheck, 22 staticcheck (three in production, all SA1019 deprecations — session.Log against MCP SEP-2577, and httputil Director against Go 1.26), 28 unused, every one of them dead scaffolding in test files. 109 real findings, against a number that used to read 90 and meant nothing. go build, go vet and go test all pass; gofmt clean. Co-Authored-By: Virgil --- .github/workflows/ci.yml | 9 ++++++-- go/.golangci.yml | 35 ++++++++++++++++++++++++++++++ go/cmd/core-agent/commands_chat.go | 12 ++++++++-- go/cmd/core-agent/lemma_mcp.go | 8 ++++++- go/cmd/core-agent/main.go | 9 +++++++- go/pkg/agentic/auto_pr.go | 4 +++- go/pkg/agentic/dispatch.go | 13 +++++++---- go/pkg/brain/messaging.go | 4 +++- go/pkg/chathistory/chathistory.go | 2 +- go/pkg/chathistory/export.go | 28 +++++++++++++++++++----- go/pkg/lemma/admin.go | 2 +- go/pkg/lemma/lemma.go | 2 +- go/pkg/monitor/harvest.go | 7 ++++-- go/pkg/monitor/monitor.go | 16 ++++++++++---- go/pkg/runner/runner.go | 6 +++-- 15 files changed, 128 insertions(+), 29 deletions(-) create mode 100644 go/.golangci.yml 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) })