Skip to content

Commit 8b9843b

Browse files
authored
mcp tests: keep the real Zulip adapter out of above-port tests (#9)
## What Audits the above-the-port unit tests (`server.test.ts`, `server.integration.test.ts`, tools tests) and the `disconnect-exit` fixture to confirm the **real Zulip adapter does not appear** in them — they drive the substrate through hand-rolled port fakes and the in-memory adapter only. Documents the rule and concentrates the residual Zulip coupling into a single seam. ## Audit finding These tests were already compliant *behaviourally*: only `server.live.test.ts` constructs `zulipAdapter()` (the live suite, out of scope), and `bootstrap.test.ts` wires the real adapter *from config* (it tests wiring, not I/O — exempt). The above-port targets exercise only `buildFakeAdapter` / harness overrides and the real `memoryAdapter()`. The residual `@commy/zulip` references were the `ZulipAdapter` *type* (the `SubstrateAdapter` port is currently typed as `ZulipAdapter`) and the `UserUploadPath` brand (`decodeUserUploadPathSync`) — type/brand coupling, not adapter behaviour — duplicated across three sites that each completed an `AgentComms` core to the Zulip-shaped aggregate. ## Change - **`packages/mcp/memory-substrate.ts`** (new) — `completeAsSubstrate(base, overrides?)`, the single seam that completes an `AgentComms` core (in-memory adapter or a port fake) to the `ZulipAdapter` shape. It owns the lone `UserUploadPath` brand mint and re-exports the `ZulipAdapter` type, so `@commy/zulip` now appears in exactly **one** test-side module. - The three completion sites (`server.test.ts` ×2, `server.integration.test.ts`, `disconnect-exit.fixture.ts`) now call the helper instead of inlining the Zulip-shaped members. - **`docs/architecture.md`** — new *Test architecture* section documenting the rule (above-port unit tests use the memory adapter / port fakes only, never the real Zulip adapter), why the memory adapter is a *proven* contract-equivalent, and the two legitimate exceptions. Substrate-neutralising the `SubstrateAdapter` port itself (so above-port code names no Zulip type at all) is epic-level and tracked separately; this keeps the boundary self-enforcing in the meantime. Closes comms-e5vm.4 (epic comms-e5vm).
1 parent 4f923db commit 8b9843b

5 files changed

Lines changed: 150 additions & 73 deletions

File tree

‎docs/architecture.md‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,38 @@ The plugin README documents the tool surface, the inbound `<channel>` event
3636
format, the boot/identity model, and troubleshooting. Read it for anything about
3737
running commy *inside Claude Code* specifically.
3838

39+
## Test architecture
40+
41+
The ports are the seam, so tests sit on one side of them or the other:
42+
43+
- **Port contract tests** (`@commy/testing`) pin the behaviour every adapter
44+
must honour. They run against *both* the real Zulip adapter and the in-memory
45+
adapter, which is how `@commy/memory` earns the right to stand in for Zulip
46+
elsewhere — it is a *proven* contract-equivalent, not a hopeful mock.
47+
48+
- **Above-the-port unit tests** (in `@commy/mcp` — `server.test.ts`,
49+
`server.integration.test.ts`, tools tests) exercise the driving adapter:
50+
bootstrap, identity lifecycle, the event pump, tool dispatch. **They use the
51+
in-memory adapter (or a hand-rolled port fake) only — never the real Zulip
52+
adapter.** A boot or tool-dispatch test that needs the real Zulip adapter to
53+
pass is testing the wrong thing: the contract suite already owns Zulip's
54+
behaviour, so above the port we depend on the *contract*, served by the fast
55+
in-memory double. This keeps these tests realm-free, fast, and immune to
56+
Zulip rate limits.
57+
58+
Two narrow exceptions are legitimate and are **not** real-adapter usage:
59+
`bootstrap.test.ts` wires the real adapter *from config* (it tests the wiring,
60+
not the adapter's I/O), and the live suite (`*.live.test.ts`) deliberately
61+
hits a real realm and is excluded from default discovery.
62+
63+
Note: `server.test.ts` / `server.integration.test.ts` still reference
64+
`@commy/zulip` for the `ZulipAdapter` *type* and the `UserUploadPath` brand
65+
(`decodeUserUploadPathSync`). That is type/brand coupling, not behaviour — the
66+
`SubstrateAdapter` port the driving adapter depends on is currently *typed as*
67+
`ZulipAdapter`, so a provided in-memory double must be completed to that shape.
68+
Substrate-neutralising that port (so above-port code names no Zulip type at
69+
all) is tracked separately; it does not change the rule above.
70+
3971
## Substrate rationale and contracts
4072

4173
- [Why Zulip](why-zulip.md) — why the V1 driven adapter is backed by a Zulip realm.

‎packages/mcp/disconnect-exit.fixture.ts‎

Lines changed: 13 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -17,14 +17,14 @@
1717
* reproduction of the very race under test).
1818
*/
1919
import { stderrLoggerLayer } from '@commy/core/logging'
20-
import { type MemoryAdapter, memoryAdapter } from '@commy/memory/adapter'
21-
import type { ZulipAdapter } from '@commy/zulip/adapter'
20+
import { memoryAdapter } from '@commy/memory/adapter'
2221
import { FetchHttpClient } from '@effect/platform'
2322
import { BunFileSystem, BunRuntime } from '@effect/platform-bun'
2423
import { StdioServerTransport } from '@modelcontextprotocol/sdk/server/stdio.js'
2524
import { ConfigProvider, Effect, Layer, Option } from 'effect'
2625
import { substrateAdapterLayer } from './bootstrap.ts'
2726
import { CursorStoreTag } from './cursor-store.ts'
27+
import { completeAsSubstrate } from './memory-substrate.ts'
2828
import { clientDisconnect, makeProgram } from './server.ts'
2929

3030
const inMemoryCursorStore = {
@@ -33,23 +33,20 @@ const inMemoryCursorStore = {
3333
}
3434

3535
/**
36-
* The in-memory substrate is a `MemoryAdapter`; complete it to the
37-
* `ZulipAdapter` shape the program expects. `reconcileMinterSubscriptions`
38-
* (boot) and `close` (shutdown finalizer) are exercised, so they are real
39-
* no-ops; `uploadFile`/`downloadFile` are never reached without an MCP
40-
* client driving tools.
36+
* Complete the in-memory substrate to the `ZulipAdapter` shape the program
37+
* expects. `reconcileMinterSubscriptions` (boot) and `close` (shutdown
38+
* finalizer) are exercised, so the helper's inert no-ops suffice;
39+
* `uploadFile`/`downloadFile` are never reached without an MCP client driving
40+
* tools, so they die loudly if anything calls them.
4141
*/
42-
const asZulipAdapter = (adapter: Effect.Effect<MemoryAdapter>): Effect.Effect<ZulipAdapter> =>
43-
Effect.map(
44-
adapter,
45-
(base): ZulipAdapter => ({
46-
...base,
47-
reconcileMinterSubscriptions: () => Effect.succeed({ added: [], error: undefined }),
42+
const substrate = memoryAdapter().pipe(
43+
Effect.map((base) =>
44+
completeAsSubstrate(base, {
4845
uploadFile: () => Effect.die(new Error('disconnect-exit fixture: uploadFile unused')),
4946
downloadFile: () => Effect.die(new Error('disconnect-exit fixture: downloadFile unused')),
50-
close: async () => {},
5147
}),
52-
)
48+
),
49+
)
5350

5451
let attached = 0
5552
const armedStdin = {
@@ -71,7 +68,7 @@ BunRuntime.runMain(
7168
Effect.provide(
7269
Layer.provideMerge(
7370
Layer.mergeAll(
74-
substrateAdapterLayer(asZulipAdapter(memoryAdapter())),
71+
substrateAdapterLayer(substrate),
7572
Layer.succeed(CursorStoreTag, inMemoryCursorStore),
7673
stderrLoggerLayer,
7774
),

‎packages/mcp/memory-substrate.ts‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
import type { AgentComms } from '@commy/core/ports'
2+
import type { ZulipAdapter } from '@commy/zulip/adapter'
3+
import { decodeUserUploadPathSync } from '@commy/zulip/http'
4+
import { Effect } from 'effect'
5+
6+
/**
7+
* The single seam where above-the-port tests touch a Zulip type or brand.
8+
*
9+
* Above-port unit tests (`server.test.ts`, `server.integration.test.ts`, the
10+
* `disconnect-exit` fixture) drive the substrate through the in-memory adapter
11+
* or a hand-rolled port fake — never the real Zulip adapter (see
12+
* docs/architecture.md § Test architecture). But the `SubstrateAdapter` port
13+
* those programs depend on is currently *typed as* {@link ZulipAdapter}, so any
14+
* provided double must be completed from the universal {@link AgentComms} core
15+
* to that Zulip-shaped aggregate: `reconcileMinterSubscriptions`,
16+
* `downloadFile`, `uploadFile`, `close`. Concentrating that completion — and the
17+
* lone `UserUploadPath` brand mint it needs — here keeps the rule self-enforcing:
18+
* `@commy/zulip` appears in exactly one test-side module, this one.
19+
*
20+
* `ZulipAdapter` is re-exported so callers annotate their doubles without
21+
* naming `@commy/zulip` themselves.
22+
*/
23+
export type { ZulipAdapter } from '@commy/zulip/adapter'
24+
25+
/** The four Zulip-shaped members that complete `AgentComms` to a `ZulipAdapter`. */
26+
type SubstrateExtras = Pick<
27+
ZulipAdapter,
28+
'reconcileMinterSubscriptions' | 'downloadFile' | 'uploadFile' | 'close'
29+
>
30+
31+
/**
32+
* Per-member overrides. Anything omitted falls back to an inert default: a
33+
* no-op reconcile report, an empty download, a stub upload result, a no-op
34+
* close. Tests override only the member whose behaviour they actually assert.
35+
*/
36+
type SubstrateExtrasOverrides = Partial<SubstrateExtras>
37+
38+
const inertExtras: SubstrateExtras = {
39+
reconcileMinterSubscriptions: () => Effect.succeed({ added: [], error: undefined }),
40+
downloadFile: () =>
41+
Effect.succeed({ data: new Uint8Array([]), contentType: 'application/octet-stream' }),
42+
uploadFile: () =>
43+
Effect.succeed({ url: decodeUserUploadPathSync('/user_uploads/0/stub'), filename: 'stub' }),
44+
close: async () => {},
45+
}
46+
47+
/**
48+
* Complete an {@link AgentComms} core (the in-memory adapter, or a hand-rolled
49+
* port fake) to the `ZulipAdapter` shape the `SubstrateAdapter` port expects.
50+
* Pass overrides for the Zulip-shaped members a given test asserts on (e.g. a
51+
* counting `close`); the rest stay inert.
52+
*/
53+
export const completeAsSubstrate = (
54+
base: AgentComms,
55+
overrides: SubstrateExtrasOverrides = {},
56+
): ZulipAdapter => ({
57+
...base,
58+
...inertExtras,
59+
...overrides,
60+
})

‎packages/mcp/server.integration.test.ts‎

Lines changed: 18 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -28,14 +28,17 @@ import {
2828
InboxError,
2929
} from '@commy/core/ports'
3030
import { memoryAdapter } from '@commy/memory/adapter'
31-
import type { ZulipAdapter } from '@commy/zulip/adapter'
32-
import { decodeUserUploadPathSync } from '@commy/zulip/http'
3331
import { Client } from '@modelcontextprotocol/sdk/client/index.js'
3432
import { InMemoryTransport } from '@modelcontextprotocol/sdk/inMemory.js'
3533
import { Deferred, Effect, FiberId, Layer, Option, Stream } from 'effect'
3634
import { parseEnv, substrateAdapterLayer } from './bootstrap.ts'
3735
import type { CursorStore } from './cursor-store.ts'
3836
import { CursorStoreTag } from './cursor-store.ts'
37+
// Above-port unit tests drive the substrate through the in-memory adapter only —
38+
// never the real Zulip adapter (see docs/architecture.md § Test architecture).
39+
// `completeAsSubstrate` is the single seam that completes it to the Zulip-shaped
40+
// `SubstrateAdapter` port; no Zulip type or brand is named directly here.
41+
import { completeAsSubstrate } from './memory-substrate.ts'
3942
import { makeProgram } from './server.ts'
4043
import { testPlatformLayer } from './test-platform.ts'
4144

@@ -271,24 +274,20 @@ const buildHarness = async (overrides: AdapterOverrides = {}): Promise<Harness>
271274
}
272275
const composedDirectory: Directory = { ...base.directory, ...overrides.directoryOverrides }
273276

274-
const adapter: ZulipAdapter = {
275-
identity: composedIdentity,
276-
publisher: composedPublisher,
277-
inbox: composedInbox,
278-
history: composedHistory,
279-
directory: composedDirectory,
280-
reconcileMinterSubscriptions: () => Effect.succeed({ added: [], error: undefined }),
281-
downloadFile: () =>
282-
Effect.succeed({
283-
data: new Uint8Array([]),
284-
contentType: 'application/octet-stream',
285-
}),
286-
uploadFile: () =>
287-
Effect.succeed({ url: decodeUserUploadPathSync('/user_uploads/0/stub'), filename: 'stub' }),
288-
close: async () => {
289-
closes.count += 1
277+
const adapter = completeAsSubstrate(
278+
{
279+
identity: composedIdentity,
280+
publisher: composedPublisher,
281+
inbox: composedInbox,
282+
history: composedHistory,
283+
directory: composedDirectory,
290284
},
291-
}
285+
{
286+
close: async () => {
287+
closes.count += 1
288+
},
289+
},
290+
)
292291

293292
const [serverTransport, clientTransport] = InMemoryTransport.createLinkedPair()
294293
const client = new Client(

‎packages/mcp/server.test.ts‎

Lines changed: 27 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,6 @@ import {
2626
InboxError,
2727
} from '@commy/core/ports'
2828
import { memoryAdapter } from '@commy/memory/adapter'
29-
import type { ZulipAdapter } from '@commy/zulip/adapter'
30-
import { decodeUserUploadPathSync } from '@commy/zulip/http'
3129
import {
3230
Cause,
3331
Duration,
@@ -44,6 +42,13 @@ import { EnvConfigError, NotInRepo, parseEnv, substrateAdapterLayer } from './bo
4442
import type { CursorStore } from './cursor-store.ts'
4543
import { CursorStoreTag } from './cursor-store.ts'
4644
import type { IdentityCache } from './identity-cache.ts'
45+
// Above-port unit tests drive the substrate through hand-rolled port fakes and
46+
// the in-memory adapter only — never the real Zulip adapter (see
47+
// docs/architecture.md § Test architecture). `completeAsSubstrate` is the single
48+
// seam that completes either to the Zulip-shaped `SubstrateAdapter` port, and it
49+
// re-exports the `ZulipAdapter` type so this file names no `@commy/zulip` module
50+
// directly.
51+
import { completeAsSubstrate, type ZulipAdapter } from './memory-substrate.ts'
4752
import { clientDisconnect, forkIdleSweep, makeProgram, type ProgramParams } from './server.ts'
4853
import { testPlatformLayer } from './test-platform.ts'
4954

@@ -178,29 +183,20 @@ const buildFakeAdapter = (
178183
added: [] as ReadonlyArray<ChannelName>,
179184
error: undefined as string | undefined,
180185
}
181-
const adapter: ZulipAdapter = {
182-
identity: identityPort,
183-
publisher,
184-
inbox,
185-
history,
186-
directory,
187-
reconcileMinterSubscriptions: () =>
188-
Effect.sync(() => {
189-
events.push('reconcile')
190-
reconcileCalls.count += 1
191-
return options.reconcileReport ?? defaultReconcileReport
192-
}),
193-
downloadFile: () =>
194-
Effect.succeed({
195-
data: new Uint8Array([]),
196-
contentType: 'application/octet-stream',
197-
}),
198-
uploadFile: () =>
199-
Effect.succeed({ url: decodeUserUploadPathSync('/user_uploads/0/stub'), filename: 'stub' }),
200-
close: async () => {
201-
closes.count += 1
186+
const adapter = completeAsSubstrate(
187+
{ identity: identityPort, publisher, inbox, history, directory },
188+
{
189+
reconcileMinterSubscriptions: () =>
190+
Effect.sync(() => {
191+
events.push('reconcile')
192+
reconcileCalls.count += 1
193+
return options.reconcileReport ?? defaultReconcileReport
194+
}),
195+
close: async () => {
196+
closes.count += 1
197+
},
202198
},
203-
}
199+
)
204200
return { adapter, calls: { acquired, closes, subscribed, reconcileCalls, events } }
205201
}
206202

@@ -314,21 +310,14 @@ test('main drives a real memory adapter through acquire + env subscribe + close'
314310
}).pipe(Effect.flatMap(() => realSubscribe(target)))
315311
let closes = 0
316312
const oneShotEvents: MessageInbox['events'] = () => Stream.empty
317-
const memoryAdapterAsZulipShape: ZulipAdapter = {
318-
...adapter,
319-
inbox: { ...adapter.inbox, subscribe: spy, events: oneShotEvents },
320-
reconcileMinterSubscriptions: () => Effect.succeed({ added: [], error: undefined }),
321-
downloadFile: () =>
322-
Effect.succeed({
323-
data: new Uint8Array([]),
324-
contentType: 'application/octet-stream',
325-
}),
326-
uploadFile: () =>
327-
Effect.succeed({ url: decodeUserUploadPathSync('/user_uploads/0/stub'), filename: 'stub' }),
328-
close: async () => {
329-
closes += 1
313+
const memoryAdapterAsZulipShape = completeAsSubstrate(
314+
{ ...adapter, inbox: { ...adapter.inbox, subscribe: spy, events: oneShotEvents } },
315+
{
316+
close: async () => {
317+
closes += 1
318+
},
330319
},
331-
}
320+
)
332321
const env = {
333322
...validEnv,
334323
COMMY_SUBSCRIBE: 'channel:home,thread:home/payments,mentions',

0 commit comments

Comments
 (0)