Skip to content

Commit 6554515

Browse files
committed
refactor(mcp): flatten Handler god-object, promote session state to entity
Closes TASKS.md:45 ("Move 6 grandfathered cross-package MCP types to entity/") by eliminating the Handler struct entirely and reducing the grandfathered cross-package types count from 6 to 0. ## Phase 1 — sub-task moves - `def/prompt.EntrySpec`/`EntryField` -> `entity.PromptEntrySpec`/ `PromptEntryField` (pure data, natural entity fit) - `internal/mcp/session/*` collapsed into `internal/mcp/handler/*` (sole consumer; eliminates the sibling crossing) - `internal/mcp/server/poll/` -> `internal/mcp/server/dispatch/poll/` (ancestor->descendant exemption closes the last sibling crossing before Phase 2) ## Phase 2 — flatten the Handler god-object The Handler struct was a parameter bundle masquerading as domain logic. 13 of its 17 methods only read ContextDir; 2 mutated session state; the other 2 did not touch the receiver at all. TokenBudget was a pass-through passenger consumed only by dispatch.go. - New `entity.MCPDeps { ContextDir, TokenBudget, *MCPSession }` parameter object, held by the server and threaded through dispatch - New `entity.MCPSession` holding the former session.State fields with pure Record*/Increment* mutation methods (no I/O) - New `entity.PendingUpdate` (pure data) - All 17 `(h *Handler) Foo(...)` methods converted to free functions `handler.Foo(d *entity.MCPDeps, ...)` - `handler.CheckGovernance` kept as a free function in handler/ because it still reads .context/state/violations.json (Phase 3 absorbed into Phase 2 naturally) - `server.Server.handler *handler.Handler` replaced with `server.Server.deps *entity.MCPDeps`; route/tool/* and route/prompt/* updated to take `*entity.MCPDeps` - `handler.Handler` struct deleted entirely; `handler/handler.go` deleted; `handler/state.go` deleted - `governance_test.go` rewritten to construct `*entity.MCPDeps` ## Grandfather purge (TestTypeFileConvention) The grandfatheredTypes map was cleared to zero entries. All 35 underlying violations across 18 packages were fixed by moving type declarations into per-package types.go files; methods remain in their original files, which become type-decl-free and are skipped by the audit. - New types.go created in 13 packages (cli/dep/core/{golang,node, python,rust}, cli/drift/core/{fix,out}, cli/guide/core/skill, cli/remind/core/store, cli/system/core/nudge, cli/why/core/data, config/memory, mcp/server/catalog, mcp/server/io) - fm.go renamed to types.go (frontmatter) and atom.go renamed to types.go (rss) since they were pure type definition files - session.go regenerated with only the Session interface - parser.{ClaudeCode,Copilot,CopilotCLI,MarkdownSession} and lookup.commandEntry absorbed into their packages' existing types.go Hardened the grandfatheredTypes comment with an explicit agent directive: do not add entries here under any circumstance. Any re-addition requires a dedicated pull request with per-entry justification and maintainer approval. Drive-by additions by any assistant are unauthorized. ## Interface segregation Interfaces are behavioral contracts, not data blueprints, so they live in their own <name>.go files rather than types.go. - TestTypeFileConvention updated: interface type declarations are now exempt from the "must have exported receiver" phase 4 check. Interfaces cannot carry receivers; a file containing only an interface is a valid pure type file. - `parser.Session` interface moved to journal/parser/session.go - `builder.GraphBuilder` interface moved to cli/dep/core/builder/graph_builder.go ## Audit state - TestCrossPackageTypes: grandfathered 6 -> 0 - TestTypeFileConvention: grandfathered map 34 -> 0 - TestDocCommentStructure: 0 (unchanged) - make test: all pass - make lint: 0 issues ## Documentation Updated .context/DETAILED_DESIGN-mcp.md, .context/ARCHITECTURE.md, .context/DANGER-ZONES.md, and .context/DETAILED_DESIGN.md to reflect the new package layout (no more mcp/session, poll lives under dispatch, Handler replaced by entity.MCPDeps). Also sweeps in two pre-existing .context/ entries that were uncommitted at session start (architecture skill pipeline decision, pad index shifting learning). Spec: specs/mcp-handler-flatten.md Signed-off-by: Jose Alekhinne <jose@ctx.ist>
1 parent 4a2a9cb commit 6554515

77 files changed

Lines changed: 1244 additions & 920 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

.context/ARCHITECTURE.md

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,6 @@ graph TD
9191
9292
MCP --> HANDLER[mcp/handler]
9393
MCP --> PROTO[mcp/proto]
94-
MCP --> SESSION[mcp/session]
9594
HANDLER --> DOMAIN
9695
9796
CORE --> DOMAIN[domain packages]
@@ -163,12 +162,12 @@ graph TD
163162
| `mcp/proto` | JSON-RPC 2.0 message types, MCP constants |
164163
| `mcp/server` | Main loop: stdin read, dispatch, stdout write |
165164
| `mcp/server/dispatch` | Method-based request routing |
165+
| `mcp/server/dispatch/poll` | File mtime polling for change notifications |
166166
| `mcp/server/catalog` | URI-to-file resource mapping (9 resources) |
167-
| `mcp/server/poll` | File mtime polling for change notifications |
168167
| `mcp/server/route/*` | Handlers: initialize, ping, tool, prompt, resource |
169168
| `mcp/server/def/*` | Tool (11) and prompt (5) definitions |
170-
| `mcp/handler` | Domain logic (testable without JSON-RPC) |
171-
| `mcp/session` | Per-session advisory state and governance |
169+
| `mcp/handler` | Domain logic as free functions taking `*entity.MCPDeps` |
170+
| `entity.MCPSession` | Per-session advisory state (pure data + mutations) |
172171

173172
### CLI Commands (`internal/cli/*`)
174173

.context/DANGER-ZONES.md

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,8 @@ Enriched 2026-04-03 via GitNexus (blast radius verified)._
2525
| journal/parser | 1MB buffer limit | MEDIUM | n/a | n/a | Large tool results truncated without warning |
2626
| memory | Slug format dependency | MEDIUM | 7 | 7 | Claude Code naming convention change breaks discovery |
2727
| drift | Path ref false positives | MEDIUM | n/a | n/a | Code examples in markdown trigger false warnings |
28-
| mcp/server/poll | Mtime granularity | MEDIUM | n/a | n/a | Sub-second changes between polls are missed |
29-
| mcp/session | In-memory only state | MEDIUM | n/a | n/a | Server restart loses governance tracking |
28+
| mcp/server/dispatch/poll | Mtime granularity | MEDIUM | n/a | n/a | Sub-second changes between polls are missed |
29+
| entity.MCPSession | In-memory only state | MEDIUM | n/a | n/a | Server restart loses governance tracking |
3030
| mcp/handler | Fuzzy task matching | MEDIUM | n/a | n/a | Word overlap threshold (2) causes false positives |
3131
| rc | sync.Once lock-in | MEDIUM | n/a | n/a | First RC() call locks config for process lifetime |
3232
| bootstrap | PersistentPreRunE | MEDIUM | n/a | n/a | New commands without SkipInit fail pre-.context/ |
@@ -41,7 +41,7 @@ Enriched 2026-04-03 via GitNexus (blast radius verified)._
4141
layer: MCP handler (all 11 tool methods), format (TimeAgo,
4242
Duration, Tokens), index (GenerateTable, UpdateDecisions,
4343
UpdateLearnings), tidy, trace, memory, sysinfo, io (SafeFprintf),
44-
mcp/session (CheckGovernance), mcp/server (Serve).
44+
mcp/handler (CheckGovernance), mcp/server (Serve).
4545
Participates in 53 execution flows.
4646
- Blast radius: d=1: 30+, flows: 53
4747
- Risk: CRITICAL (enriched 2026-04-03 via GitNexus)
@@ -165,12 +165,14 @@ Enriched 2026-04-03 via GitNexus (blast radius verified)._
165165
between polls are coalesced.
166166
- Risk: MEDIUM
167167

168-
### internal/mcp/session
168+
### entity.MCPSession / mcp/handler CheckGovernance
169169

170170
1. **In-memory only state** - Session governance lost on restart.
171-
CheckGovernance has clean call chain (d=1: 1 -> appendGovernance
172-
-> DispatchCall -> Do) but the advisory data it tracks is
173-
ephemeral.
171+
`handler.CheckGovernance` has clean call chain (d=1: 1 ->
172+
appendGovernance -> DispatchCall -> Do) but the advisory data
173+
it tracks is ephemeral. Data lives in `entity.MCPSession`;
174+
the I/O-touching CheckGovernance free function lives in
175+
`mcp/handler` because it drains `.context/state/violations.json`.
174176
- Blast radius: d=1: 1, d=2: 1, d=3: 1 (clean chain)
175177
- Risk: MEDIUM (enriched 2026-04-03 via GitNexus)
176178

.context/DECISIONS.md

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
<!-- INDEX:START -->
44
| Date | Decision |
55
|----|--------|
6+
| 2026-04-09 | Architecture skill pipeline is a triad not a quartet |
67
| 2026-04-08 | Remove #done tag convention, simplify task archival |
78
| 2026-04-06 | Use hook relay for session provenance instead of JSONL parsing or env vars |
89
| 2026-04-04 | TestNoMagicStrings and TestNoMagicValues no longer exempt const/var definitions outside config/ |
@@ -118,6 +119,20 @@ For significant decisions:
118119
119120
-->
120121

122+
## [2026-04-09-001332] Architecture skill pipeline is a triad not a quartet
123+
124+
**Status**: Accepted
125+
126+
**Context**: Had a proposed ctx-architecture-extend for extension point mapping, making four skills
127+
128+
**Decision**: Architecture skill pipeline is a triad not a quartet
129+
130+
**Rationale**: Extension points already covered per-module in DETAILED_DESIGN and by registration site discovery in enrich. Fourth skill fragments pipeline without distinct value
131+
132+
**Consequence**: Pipeline is map enrich hunt. Three skills three questions: how does it work, how well does it connect, where will it break
133+
134+
---
135+
121136
## [2026-04-08-013731] Remove #done tag convention, simplify task archival
122137

123138
**Status**: Accepted

.context/DETAILED_DESIGN-mcp.md

Lines changed: 74 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
# Detailed Design: MCP Server
22

3-
Modules: mcp/proto, mcp/handler, mcp/server/*, mcp/session
3+
Modules: mcp/proto, mcp/handler, mcp/server/*
44

55
## Overview
66

@@ -54,7 +54,7 @@ dispatch, writes responses to stdout.
5454
**Key types**:
5555
```
5656
Server {
57-
handler *handler.Handler
57+
deps *entity.MCPDeps // ContextDir, TokenBudget, Session
5858
version string
5959
out *mcpIO.Writer // mutex-protected stdout
6060
in io.Reader // stdin
@@ -85,10 +85,13 @@ URI-to-file resource mapping. 9 resources: 8 individual context
8585
files + 1 assembled agent packet (`ctx://context/agent`).
8686
`Init()` builds lookup map once; `ToList()` returns immutable list.
8787

88-
### server/poll
88+
### server/dispatch/poll
8989
File mtime-based polling (5s interval). Lazy goroutine lifecycle:
9090
starts on first Subscribe(), stops when all unsubscribed.
91-
Emits `notifications/resources/updated` via callback.
91+
Emits `notifications/resources/updated` via callback. Lives as a
92+
descendant of `server/dispatch` so both server (ancestor) and
93+
dispatch (parent) can consume `Poller` without triggering the
94+
sibling cross-package-type check.
9295

9396
### server/route/*
9497
Method-specific handlers:
@@ -152,36 +155,48 @@ Lightweight analytics: `TotalAdds(m)` sums entry add counts.
152155
- Consider concurrent tool execution for read-only tools
153156
- Resource change detection could use fsnotify instead of polling
154157

155-
**Dependencies**: handler, proto, session, config/mcp/*
158+
**Dependencies**: handler, proto, entity, config/mcp/*
156159

157160
---
158161

159162
## mcp/handler
160163

161164
**Purpose**: Domain logic implementation, testable without JSON-RPC
162-
coupling. All tool and prompt functionality lives here.
165+
coupling. All tool and prompt functionality lives here as free
166+
functions that take a `*entity.MCPDeps` as the first argument.
163167

164-
**Key types**:
168+
**Runtime bundle** (defined in `entity/mcp_deps.go`):
165169
```
166-
Handler {
170+
MCPDeps {
167171
ContextDir string
168172
TokenBudget int
169-
Session *session.State
173+
Session *entity.MCPSession
170174
}
171175
```
172176

173-
**Exported API**:
174-
- `Status()`: context health summary
175-
- `Add(type, content, opts)`: validate boundary, write entry
176-
- `Complete(query)`: mark task done by number/text match
177-
- `Drift()`: detect violations/warnings
178-
- `Recall(limit, since)`: query session history
179-
- `WatchUpdate(type, content, opts)`: write + queue pending update
180-
- `Compact(archive)`: move completed tasks to archive
181-
- `Next()`: next pending task
182-
- `CheckTaskCompletion(recentAction)`: match action to tasks
183-
- `SessionEvent(eventType, caller)`: start/end lifecycle
184-
- `Remind()`: list pending reminders
177+
The server holds a single `*MCPDeps`, threaded through
178+
dispatch into each handler function.
179+
180+
**Exported API** (all free functions — no receiver):
181+
- `Status(d)`: context health summary
182+
- `Add(d, type, content, opts)`: validate boundary, write entry
183+
- `Complete(d, query)`: mark task done by number/text match
184+
- `Drift(d)`: detect violations/warnings
185+
- `Recall(d, limit, since)`: query session history
186+
- `WatchUpdate(d, type, content, opts)`: write + queue pending update
187+
- `Compact(d, archive)`: move completed tasks to archive
188+
- `Next(d)`: next pending task
189+
- `CheckTaskCompletion(d, recentAction)`: match action to tasks
190+
- `SessionEvent(d, eventType, caller)`: start/end lifecycle
191+
- `Remind(d)`: list pending reminders
192+
- `SteeringGet(d, prompt)`: applicable steering files
193+
- `Search(d, query)`: full-text search across context files
194+
- `SessionStartHooks(d)` / `SessionEndHooks(d, summary)`:
195+
run session-lifecycle triggers
196+
- `CheckGovernance(d, toolName)`: compute advisory warnings
197+
for the current tool response (does violations-file I/O,
198+
which is why it stays in `handler/` rather than being a
199+
method on `entity.MCPSession`)
185200

186201
### handler/task
187202
Task list parsing for MCP: `ForEachPending(lines, fn)` iterates
@@ -201,32 +216,51 @@ entity, io, rc
201216

202217
---
203218

204-
## mcp/session
219+
## entity.MCPSession
205220

206-
**Purpose**: Per-session advisory state and governance warnings.
221+
**Purpose**: Per-MCP-run advisory state. Lives in `internal/entity/`
222+
because it is pure data + pure mutation methods, with no I/O.
223+
Formerly the `mcp/session.State` type; promoted to entity when
224+
the `mcp/session` package was collapsed into `mcp/handler` and
225+
the god-object Handler was dissolved.
207226

208-
**Key types**:
227+
**Key type**:
209228
```
210-
State {
229+
MCPSession {
211230
ToolCalls int
212231
AddsPerformed map[string]int
232+
SessionStartedAt time.Time
213233
PendingFlush []PendingUpdate
214-
sessionStarted bool
215-
contextLoaded bool
216-
lastDriftCheck time.Time
217-
callsSinceWrite int
234+
SessionStarted bool
235+
ContextLoaded bool
236+
LastDriftCheck time.Time
237+
LastContextWrite time.Time
238+
CallsSinceWrite int
218239
}
219240
```
220241

221-
**Governance warnings** (appended to tool responses):
222-
1. Session not started (if sessionStarted=false)
223-
2. Context not loaded (if contextLoaded=false)
242+
**Pure methods** (all in `entity/mcp_session.go`):
243+
- `NewMCPSession() *MCPSession`
244+
- `RecordToolCall()`, `RecordAdd(type)`
245+
- `QueuePendingUpdate(u)`, `PendingCount()`
246+
- `RecordSessionStart()`, `RecordContextLoaded()`
247+
- `RecordDriftCheck()`, `RecordContextWrite()`
248+
- `IncrementCallsSinceWrite()`
249+
250+
**Governance warnings** (computed by `handler.CheckGovernance`,
251+
which lives in `mcp/handler` because it does I/O on the
252+
violations file):
253+
1. Session not started (if SessionStarted=false)
254+
2. Context not loaded (if ContextLoaded=false)
224255
3. Drift not checked (after interval or min calls)
225-
4. Persist nudge (after callsSinceWrite threshold)
226-
5. Violations from extension (reads violations.json)
256+
4. Persist nudge (after CallsSinceWrite threshold)
257+
5. Violations from extension (reads violations.json — the one
258+
bit of I/O that forced splitting CheckGovernance out of the
259+
entity-side methods)
227260

228-
**Data flow**: Each tool call -> RecordToolCall() ->
229-
CheckGovernance() -> warnings appended to response text.
261+
**Data flow**: Each tool call -> `d.Session.RecordToolCall()` ->
262+
`handler.CheckGovernance(d, toolName)` -> warnings appended to
263+
response text.
230264

231265
**Edge cases**:
232266
- violations.json is read-and-cleared (one-shot delivery)
@@ -239,10 +273,12 @@ CheckGovernance() -> warnings appended to response text.
239273
config/mcp/governance — not user-configurable.
240274

241275
**Extension points**:
242-
- Add new governance rules in CheckGovernance()
276+
- Add new governance rules in `handler.CheckGovernance`
243277
- Violations file format is extensible
244278

245-
**Dependencies**: config/mcp/governance, proto
279+
**Dependencies**: time (entity side); config/mcp/governance,
280+
proto, assets/read/desc, config/format, config/token,
281+
config/mcp/tool (handler side)
246282

247283
---
248284

.context/DETAILED_DESIGN.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ edge cases, danger zones, extension points).
88
|--------|------|---------|---------|
99
| Foundation | [DETAILED_DESIGN-foundation.md](DETAILED_DESIGN-foundation.md) | config/*, assets/*, io, format, parse, sanitize, validate, inspect, flagbind, exec/*, log/*, crypto, sysinfo, rc | Constants, I/O, formatting, external commands |
1010
| Domain | [DETAILED_DESIGN-domain.md](DETAILED_DESIGN-domain.md) | entity, entry, context/*, drift, index, task, tidy, trace, journal/*, memory, notify, claude | Core business logic and data types |
11-
| MCP | [DETAILED_DESIGN-mcp.md](DETAILED_DESIGN-mcp.md) | mcp/proto, mcp/handler, mcp/server/*, mcp/session | JSON-RPC 2.0 server, tools, resources, prompts |
11+
| MCP | [DETAILED_DESIGN-mcp.md](DETAILED_DESIGN-mcp.md) | mcp/proto, mcp/handler, mcp/server/*, entity.MCPSession | JSON-RPC 2.0 server, tools, resources, prompts |
1212
| CLI | [DETAILED_DESIGN-cli.md](DETAILED_DESIGN-cli.md) | bootstrap, cli/parent, 34 command packages | Command registration, groups, taxonomy |
1313
| Output | [DETAILED_DESIGN-output.md](DETAILED_DESIGN-output.md) | write/*, err/*, assets/read/* | Terminal formatting, error constructors, text lookup |
1414

.context/LEARNINGS.md

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ DO NOT UPDATE FOR:
1717
<!-- INDEX:START -->
1818
| Date | Learning |
1919
|----|--------|
20+
| 2026-04-09 | Pad index shifting is a real UX bug in batch operations |
2021
| 2026-04-08 | fmt.Fprintf to strings.Builder silently discards errors |
2122
| 2026-04-08 | AST audit tests must cover unexported functions too |
2223
| 2026-04-06 | Agents ignore system-reminder content without explicit relay instructions |
@@ -111,6 +112,16 @@ DO NOT UPDATE FOR:
111112

112113
---
113114

115+
## [2026-04-09-001323] Pad index shifting is a real UX bug in batch operations
116+
117+
**Context**: ctx pad rm 10; rm 11; rm 12 deleted wrong entries because indices shifted after each deletion
118+
119+
**Lesson**: Any ID-based system where users chain operations needs stable IDs. Look-then-act is safe for single ops; look-then-batch-act breaks with shifting indices
120+
121+
**Application**: Both pad and remind now use stable IDs with batch delete and range support. Apply same pattern to any future numbered-list subsystem
122+
123+
---
124+
114125
## [2026-04-08-074612] fmt.Fprintf to strings.Builder silently discards errors
115126

116127
**Context**: golangci-lint errcheck allows fmt.Fprintf to strings.Builder because Write never fails, but project convention says zero silent discard

.context/TASKS.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ TASK STATUS LABELS:
4242

4343
### Phase -2: Task completion nudge:
4444

45-
- [ ] Move 6 grandfathered cross-package MCP types to entity/ #session:cc97cb0d #branch:main #commit:e8d5c60a #added:2026-04-08-074620
45+
- [x] Move 6 grandfathered cross-package MCP types to entity/ #session:cc97cb0d #branch:main #commit:e8d5c60a #added:2026-04-08-074620
4646

4747
- [ ] Design UserPromptSubmit hook that runs `make audit` at
4848
session start and surfaces failures as a consolidation-debt

internal/assets/read/lookup/state.go

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9,13 +9,6 @@ package lookup
99
// stopWordsMap holds words excluded from search indexing.
1010
var stopWordsMap map[string]bool
1111

12-
// commandEntry is a single YAML-backed description with
13-
// short and optional long forms.
14-
type commandEntry struct {
15-
Short string `yaml:"short"`
16-
Long string `yaml:"long"`
17-
}
18-
1912
var (
2013
// CommandsMap maps command description keys to their YAML entries.
2114
CommandsMap map[string]commandEntry

internal/assets/read/lookup/types.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,3 +15,10 @@ type ConfigPattern struct {
1515
Pattern string
1616
Topic string
1717
}
18+
19+
// commandEntry is a single YAML-backed description with
20+
// short and optional long forms.
21+
type commandEntry struct {
22+
Short string `yaml:"short"`
23+
Long string `yaml:"long"`
24+
}

internal/audit/cross_package_types_test.go

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,11 @@ import (
1515
)
1616

1717
// grandfatheredCrossPackageTypes is the count of pre-existing
18-
// cross-package type violations in mcp/ sibling subpackages.
19-
// These were exposed by hardening the sameModule heuristic
20-
// to stop blanket-exempting intra-module sibling sharing.
21-
// Reduce as violations are moved to entity/.
22-
const grandfatheredCrossPackageTypes = 6
18+
// cross-package type violations. Kept at zero: every new
19+
// violation must be addressed by moving the type to entity/
20+
// or restructuring the package boundary to eliminate the
21+
// crossing.
22+
const grandfatheredCrossPackageTypes = 0
2323

2424
// DO NOT add entries here to make tests pass. New code must
2525
// conform to the check. Widening requires a dedicated PR with
@@ -247,7 +247,7 @@ func sameModule(a, b string) bool {
247247
}
248248
}
249249
// Parent consuming its own child subpackage
250-
// (e.g. mcp/server using mcp/server/poll.Poller).
250+
// (e.g. mcp/server using mcp/server/dispatch/poll.Poller).
251251
if isChildPackage(a, b) || isChildPackage(b, a) {
252252
return true
253253
}

0 commit comments

Comments
 (0)