Skip to content

lint: add 7 architectural-sync cops (catches 10 real bugs)#2593

Merged
dgageot merged 4 commits intodocker:mainfrom
dgageot:board/custom-go-linter-opportunities-analysis-e84733a8
Apr 29, 2026
Merged

lint: add 7 architectural-sync cops (catches 10 real bugs)#2593
dgageot merged 4 commits intodocker:mainfrom
dgageot:board/custom-go-linter-opportunities-analysis-e84733a8

Conversation

@dgageot
Copy link
Copy Markdown
Member

@dgageot dgageot commented Apr 29, 2026

Adds 7 new project-specific cops to the custom rubocop-go linter, focused on architectural sync invariants that golangci-lint cannot reach. Caught 10 real bugs along the way and locked in 5 architectural caches with documented justifications.

New cops

Cop What it enforces Real findings
Lint/ConfigLatestTagConsistency json and yaml struct tags in pkg/config/latest/ agree on omitempty/omitzero. Frozen vN/ versions are exempt. 1 bug — Defer DeferConfig had json:"defer" but yaml:"defer,omitempty", so JSON always serialised the empty struct while YAML omitted it. Fixed with omitzero (consistent with the existing lifecycle.go pattern; DeferConfig.IsEmpty() makes omitzero semantically correct).
Lint/ConfigVersionsRegistered pkg/config/versions.go calls Register for every pkg/config/vN/ and pkg/config/latest/ package on disk. Preventive — currently in sync; locks the invariant.
Lint/TUIViewPurity View() string methods on TUI models do not mutate the receiver (Bubble Tea purity). Slice-cache idioms (nil, [:0], append) are auto-exempt; intentional click-zone caches use //rubocop:disable Lint/TUIViewPurity (the //nolint prefix would collide with golangci-lint's nolintlint). 5 surfaced — 2 in tabbar.go (scroll-state mutations) + 3 in dialog click-zone caches; all annotated with one-line justifications.
Lint/RuntimeEventRegistry Every &XxxEvent{Type: "yyy"} constructor in pkg/runtime/event.go has a matching entry in the decoder registry map in pkg/runtime/client.go. 3 bugs — MessageAddedEvent, ModelFallbackEvent, SubSessionCompletedEvent were silently dropped by remote-runtime clients (only a slog.Debug("invalid_type") log line). Now registered.
Lint/RuntimeSessionScoped Every *Event struct with a SessionID field implements GetSessionID() string (the SessionScoped interface used by the persistence pipeline). 6 bugs — UserMessageEvent, StreamStartedEvent, StreamStoppedEvent, SessionTitleEvent, SessionSummaryEvent, SessionCompactionEvent. Without GetSessionID, the persistence-observer's sub-session filter if scoped, ok := event.(SessionScoped); ok && scoped.GetSessionID() != sess.ID short-circuits and lets sub-agent events leak into the parent's transcript. Now all six implement the method.
Lint/HookConfigSync EventXxx constants in pkg/hooks/types.go and HooksConfig fields in pkg/config/latest/types.go stay 1:1, matched by snake-case wire string. Preventive — currently in sync (22↔22); locks the invariant.
Lint/HookBuiltinsRegistered Every exported const Foo = "wire" in pkg/hooks/builtins/*.go (excluding builtins.go and tests) appears as the first arg of a RegisterBuiltin(Foo, …) call in pkg/hooks/builtins/builtins.go. Preventive — currently in sync (10 builtins); locks the invariant.

Why these cops

The codebase has several multi-file architectural invariants that the Go compiler cannot enforce:

  • Config versions — list of pkg/config/vN/ directories ↔ versions.go registry
  • Wire-format events — runtime event constructors ↔ remote-decoder registry
  • Session ownershipSessionID field ↔ SessionScoped interface
  • Hook coverageEventXxx constants ↔ YAML schema fields ↔ in-process builtin registry
  • Render purity — Bubble Tea View() is supposed to be a pure function of state

Each of these is a "two declarations that must agree" pattern. Forgetting one of the slots compiles cleanly and only blows up at runtime — usually as a silent drop, not an error. The cops fail the lint gate at PR time instead.

All seven cops follow the same idioms as the existing four (config_package_name.go, config_version_constant.go, config_version_import.go, latest_imports_predecessor.go): embed cop.Meta, expose a New<Cop>() constructor, implement Check(p *cop.Pass), report via p.Report(node, format, args...).

Bugs caught & fixed

pkg/config/latest/types.go         Defer DeferConfig: json missing omitzero
pkg/runtime/client.go              registry missing 3 events
pkg/runtime/event.go               6 events missing GetSessionID

Cumulative state

63377309a lint: add two hook-architecture sync cops
92d1a6498 lint: add two runtime-architecture cops (catches 9 bugs)
ec5b3ce66 lint: add three project-specific cops              (catches 1 bug)
e5a4b67cd lint: adopt new rubocop-go API                     (already in main)

11 cops total, 10 real bugs caught & fixed, 5 architectural caches documented, 3 pure preventive invariants locked in.

Validation

  • mise lint ✅ (golangci-lint, 11 custom cops, go mod tidy --diff)
  • mise test ✅ (105 packages, 0 failures)
  • Each cop verified to fire on a seeded violation and to go silent again when restored — including bidirectional sync cops (3 drift scenarios for HookConfigSync).

dgageot added 4 commits April 29, 2026 13:15
rubocop-go grew a much friendlier API: a *cop.Pass that exposes Filename(),
IsBlackBoxTest(), Report(node, format, args...) and ForEachConst/Func/...,
plus an embeddable cop.Meta struct for cop metadata.

Adopt all of it in the lint/ package:

  - Drop the local 'offense()' wrapper and 'importPath()' helper, now
    provided as cop.NewOffense's new ast.Node-based form and cop.ImportPath.
  - Use p.Filename(), p.IsBlackBoxTest(), p.PackageName() everywhere
    instead of fset.Position(...).Filename / strings.HasSuffix dances.
  - Replace 'len(file.Imports) == 0' early-exits with the equivalent
    p.File.Imports loop guard inline (kept as-is for clarity).
  - Move metadata into cop.Meta so the four Name/Description/Severity
    methods per cop disappear; add a New<Cop>() constructor for each.
  - main.go now passes an explicit []cop.Cop to runner.New instead of
    going through cop.Register/cop.All.

Net change: -27 lines, with each cop's intent visible in fewer lines.

Assisted-By: docker-agent
ConfigLatestTagConsistency: detect mismatched omitempty/omitzero between

json and yaml struct tags in pkg/config/latest. Caught one real bug:

Defer DeferConfig had json:"defer" but yaml:"defer,omitempty", so JSON

always serialised the empty struct while YAML omitted it.

ConfigVersionsRegistered: ensure pkg/config/versions.go calls Register for

every pkg/config/vN/ and pkg/config/latest/ on disk; missing entries silently

fail at runtime as 'unsupported config version'.

TUIViewPurity: forbid receiver mutations inside View() string methods on

TUI models (Bubble Tea purity contract). Slice cache idioms (nil, [:0],

append(s, ...)) are auto-exempt; intentional click-zone caches use

//rubocop:disable Lint/TUIViewPurity (the //nolint prefix would collide

with golangci-lint's nolintlint validation of unknown linter names).

Surfaced 5 existing mutations: 3 dialog click-zone caches + 2 tabbar

scroll-state mutations now annotated with their justification.

Assisted-By: docker-agent
RuntimeEventRegistry: every event constructor in pkg/runtime/event.go

("&XxxEvent{Type: "yyy"}") must have a matching entry in the

registry map in pkg/runtime/client.go. Without it the remote-runtime

client silently drops the event with a Debug-level log, so a sub-

agent's MessageAddedEvent / ModelFallbackEvent / SubSessionCompletedEvent

would never reach a remote frontend. The cop ran on the existing tree

and surfaced exactly those three missing entries — now added.

RuntimeSessionScoped: any *Event struct with a SessionID field must

expose GetSessionID() string on its pointer type so it satisfies the

SessionScoped interface used by the persistence pipeline:

    if scoped, ok := event.(SessionScoped); ok && scoped.GetSessionID() != sess.ID {

        return // forwarded sub-agent event — drop

    }

Skipping the method silently bypasses that filter (the type assertion

fails, the conditional short-circuits) and lets sub-agent events leak

into the parent session's transcript. The cop surfaced six events with

this hole — UserMessageEvent, StreamStartedEvent, StreamStoppedEvent,

SessionTitleEvent, SessionSummaryEvent, SessionCompactionEvent — now

all implementing the method.

Both cops follow the same idioms as the existing seven (cop.Meta,

New<Cop>() constructor, Check(p *cop.Pass), p.Report(node, ...)).

Assisted-By: docker-agent
HookConfigSync: cross-package check that the EventXxx constants in

pkg/hooks/types.go and the HooksConfig fields in pkg/config/latest/

types.go stay 1:1, matched by the snake_case wire string. Drift in

either direction silently breaks at runtime: a new event without a

HooksConfig field is unconfigurable, a new field without an event

constant is parsed but never dispatched. Codebase is currently in

sync (22 events ↔ 22 fields); the cop locks that property in.

HookBuiltinsRegistered: intra-package check that every

"const Foo = \"bar\"" exported from pkg/hooks/builtins/*.go

(excluding builtins.go and tests) appears as the first arg of a

RegisterBuiltin(Foo, ...) call in pkg/hooks/builtins/builtins.go.

Adding a new builtin file ships its constant + impl but the wiring

lives elsewhere; forgetting it compiles cleanly and only blows up

at runtime as 'unknown builtin'. 10 builtins are registered today

and the cop keeps that invariant.

Both cops verified to fire on three drift scenarios (removed event

constant, removed config field, removed Register call) and to stay

silent on the in-sync codebase.

Assisted-By: docker-agent
@dgageot dgageot requested a review from a team as a code owner April 29, 2026 12:26
@dgageot dgageot merged commit 4fd820d into docker:main Apr 29, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants