Page back to a bounded read's window instead of filtering one page (comms-rldp)

docs/agent-experience.md

@@ -134,6 +134,12 @@ Places the current implementation fails this reference.
   `read_thread` and `list_channels`; a human member also gets search and
   unread state. Principle 1, prospectively — this is a gap to fill, not
   something to cut.
+- **A capped read does not say it was capped.** A human scrolling back sees
+  where they stopped. An agent whose read hits `limit` gets a page that looks
+  the same as a complete one, with the old end of its window silently missing.
+  The bounds themselves are honest — a window is read out of history rather
+  than filtered off the newest page — but the cap is not. Principle 1, and a
+  gap to fill.
 
 ## Worked example: lazy acquire
 

packages/mcp/tools.ts

@@ -426,7 +426,11 @@ const parseRange = (range: {
 const rangeSchemaFields = {
   since: { type: 'number', description: 'Inclusive lower bound in epoch seconds' },
   until: { type: 'number', description: 'Inclusive upper bound in epoch seconds' },
-  limit: { type: 'number', description: 'Hard cap on returned messages' },
+  limit: {
+    type: 'number',
+    description:
+      "Hard cap on returned messages, cut from the window's old end. A result holding exactly this many may omit older messages inside the window — compare the oldest message returned against since to tell one from the other.",
+  },
 } as const
 
 /**
@@ -996,7 +1000,7 @@ const buildToolDefs = (deps: RegisterToolsDeps, cache: InternalCache): ReadonlyA
     {
       name: 'read_channel',
       description:
-        'Read recent messages from a channel by name. Returns {messages: Message[]} bounded by optional since/until/limit. Each message carries a clickable permalink (and channel.permalink / thread.permalink) — when you cite one of these to a human, render it as that permalink, not a bare name or id.',
+        "Read messages from a channel by name. Returns {messages: Message[]}. since/until name a window and are read out of history, so a window further back than the newest messages still comes back; limit then caps the result and cuts from the window's old end. Each message carries a clickable permalink (and channel.permalink / thread.permalink) — when you cite one of these to a human, render it as that permalink, not a bare name or id.",
       inputSchema: {
         type: 'object',
         properties: {
@@ -1025,7 +1029,7 @@ const buildToolDefs = (deps: RegisterToolsDeps, cache: InternalCache): ReadonlyA
     {
       name: 'read_thread',
       description:
-        'Read recent messages from a thread (topic) within a channel. Returns {messages: Message[]} bounded by optional since/until/limit. Each message carries a clickable permalink (and channel.permalink / thread.permalink) — when you cite one of these to a human, render it as that permalink, not a bare name or id.',
+        "Read messages from a thread (topic) within a channel. Returns {messages: Message[]}. since/until name a window and are read out of history, so a window further back than the newest messages still comes back; limit then caps the result and cuts from the window's old end. Each message carries a clickable permalink (and channel.permalink / thread.permalink) — when you cite one of these to a human, render it as that permalink, not a bare name or id.",
       inputSchema: {
         type: 'object',
         properties: {

packages/zulip/adapter.test.ts

@@ -2169,6 +2169,166 @@ effectTest('history.readChannel filters by range.until (epoch seconds, inclusive
   }),
 )
 
+const historyRow = (id: number, subject = 'a'): Record<string, unknown> => ({
+  id,
+  sender_id: 5,
+  sender_full_name: 'Robin Reyes',
+  stream_id: 1234,
+  display_recipient: 'general',
+  subject,
+  content: `m${id}`,
+  timestamp: 1714000000 + id * 100,
+})
+
+const rowTs = (id: number) => decodeTimestampSync(1714000000 + id * 100)
+
+/**
+ * Answer consecutive `/messages` reads with the given pages, then with an
+ * empty one. Zulip's anchor is inclusive, so a page anchored on the id below
+ * the last one re-offers that row — the overlap these pages reproduce.
+ */
+const seedMessagePages = (
+  stub: StubHttpClient,
+  pages: ReadonlyArray<ReadonlyArray<Record<string, unknown>>>,
+): Effect.Effect<void> =>
+  stub
+    .respond('GET', '/api/v1/messages', messagesPage([]))
+    .pipe(Effect.zipRight(stub.respondSequence('GET', '/api/v1/messages', pages.map(messagesPage))))
+
+const historyAnchors = (stub: StubHttpClient) =>
+  stub.captured.pipe(
+    Effect.map((reqs) =>
+      reqs
+        .filter((r) => r.method === 'GET' && r.url.pathname === '/api/v1/messages')
+        .map((r) => r.url.searchParams.get('anchor')),
+    ),
+  )
+
+// The defect this fix removes. Every message in the window sits below a page
+// selected purely by recency, so filtering that page empties it and the read
+// returns an authoritative-looking nothing.
+effectTest('history.readChannel pages back to a window that lies below the newest page', () =>
+  Effect.gen(function* () {
+    const stub = yield* makeStubHttpClient
+    const adapter = yield* buildAdapter(stub)
+    yield* seedUsers(stub, [HERMES, MAINTAINER])
+    yield* seedMessagePages(stub, [
+      [7, 8, 9].map((id) => historyRow(id)),
+      [4, 5, 6, 7].map((id) => historyRow(id)),
+      [1, 2, 3, 4].map((id) => historyRow(id)),
+    ])
+    const messages = yield* adapter.history.readChannel(generalChannel.name, {
+      until: rowTs(3),
+      limit: 3,
+    })
+    expect(messages.map((m) => m.body)).toEqual([
+      decodeMessageBodySync('m1'),
+      decodeMessageBodySync('m2'),
+      decodeMessageBodySync('m3'),
+    ])
+    expect(yield* historyAnchors(stub)).toEqual(['newest', '7', '4'])
+  }),
+)
+
+effectTest('history.readThread pages back to a window that lies below the newest page', () =>
+  Effect.gen(function* () {
+    const stub = yield* makeStubHttpClient
+    const adapter = yield* buildAdapter(stub)
+    yield* seedUsers(stub, [HERMES, MAINTAINER])
+    yield* seedMessagePages(stub, [
+      [7, 8, 9].map((id) => historyRow(id, 'planning')),
+      [4, 5, 6, 7].map((id) => historyRow(id, 'planning')),
+      [1, 2, 3, 4].map((id) => historyRow(id, 'planning')),
+    ])
+    const messages = yield* adapter.history.readThread(
+      generalChannel.name,
+      decodeThreadNameSync('planning'),
+      { until: rowTs(3), limit: 3 },
+    )
+    expect(messages.map((m) => m.body)).toEqual([
+      decodeMessageBodySync('m1'),
+      decodeMessageBodySync('m2'),
+      decodeMessageBodySync('m3'),
+    ])
+  }),
+)
+
+// A row Zulip re-offers on the next page because the anchor is inclusive must
+// not be counted twice, and the window must come back in ascending order.
+effectTest('history.readChannel collects a row that spans two pages exactly once', () =>
+  Effect.gen(function* () {
+    const stub = yield* makeStubHttpClient
+    const adapter = yield* buildAdapter(stub)
+    yield* seedUsers(stub, [HERMES, MAINTAINER])
+    yield* seedMessagePages(stub, [
+      [7, 8, 9].map((id) => historyRow(id)),
+      [4, 5, 6, 7].map((id) => historyRow(id)),
+      [1, 2, 3, 4].map((id) => historyRow(id)),
+    ])
+    const messages = yield* adapter.history.readChannel(generalChannel.name, {
+      since: rowTs(1),
+      limit: 50,
+    })
+    expect(messages.map((m) => m.body)).toEqual(
+      [1, 2, 3, 4, 5, 6, 7, 8, 9].map((id) => decodeMessageBodySync(`m${id}`)),
+    )
+  }),
+)
+
+// A read with no bounds is answered by the newest page and nothing more —
+// the walk exists to reach a window, and there is no window to reach.
+effectTest('history.readChannel with no bounds reads a single page', () =>
+  Effect.gen(function* () {
+    const stub = yield* makeStubHttpClient
+    const adapter = yield* buildAdapter(stub)
+    yield* seedUsers(stub, [HERMES, MAINTAINER])
+    yield* seedMessagePages(stub, [[7, 8, 9].map((id) => historyRow(id))])
+    const messages = yield* adapter.history.readChannel(generalChannel.name, { limit: 3 })
+    expect(messages).toHaveLength(3)
+    expect(yield* historyAnchors(stub)).toEqual(['newest'])
+  }),
+)
+
+// `limit` stays a cap that truncates from the old end: the walk stops once it
+// holds that many, and keeps the newest of them.
+effectTest('history.readChannel keeps the newest messages when the window overruns limit', () =>
+  Effect.gen(function* () {
+    const stub = yield* makeStubHttpClient
+    const adapter = yield* buildAdapter(stub)
+    yield* seedUsers(stub, [HERMES, MAINTAINER])
+    yield* seedMessagePages(stub, [
+      [7, 8, 9].map((id) => historyRow(id)),
+      [4, 5, 6, 7].map((id) => historyRow(id)),
+    ])
+    const messages = yield* adapter.history.readChannel(generalChannel.name, {
+      since: rowTs(1),
+      limit: 4,
+    })
+    expect(messages.map((m) => m.body)).toEqual(
+      [6, 7, 8, 9].map((id) => decodeMessageBodySync(`m${id}`)),
+    )
+  }),
+)
+
+// The directory read runs alongside the walk, so a window that turns out
+// empty never consumes it. It must still be waited on, or a realm that has
+// stopped answering /users reads as a channel with nothing in the window.
+effectTest('history.readChannel surfaces a directory failure on an empty window', () =>
+  Effect.gen(function* () {
+    const stub = yield* makeStubHttpClient
+    const adapter = yield* buildAdapter(stub)
+    yield* seedMessagePages(stub, [])
+    yield* stub.respond('GET', '/api/v1/users', {
+      body: { result: 'error', msg: 'realm unavailable' },
+      status: 503,
+    })
+    const err = yield* Effect.flip(
+      adapter.history.readChannel(generalChannel.name, { since: rowTs(1), limit: 10 }),
+    )
+    expect(err._tag).toBe('HistoryError')
+  }),
+)
+
 effectTest('history.readThread narrows by both channel and topic', () =>
   Effect.gen(function* () {
     const stub = yield* makeStubHttpClient
@@ -2640,6 +2800,26 @@ effectTest('inbox.replay(since) returns message-posted events for messages with
   }),
 )
 
+// Boot catch-up carries the same defect independently: a seat offline across
+// more realm traffic than one page holds silently lost the overflow.
+effectTest('inbox.replay pages back until it reaches the catch-up bound', () =>
+  Effect.gen(function* () {
+    const stub = yield* makeStubHttpClient
+    const adapter = yield* buildAdapter(stub)
+    yield* seedUsers(stub, [HERMES, MAINTAINER])
+    yield* seedMessagePages(stub, [
+      [7, 8, 9].map((id) => historyRow(id, 'lobby')),
+      [4, 5, 6, 7].map((id) => historyRow(id, 'lobby')),
+      [1, 2, 3, 4].map((id) => historyRow(id, 'lobby')),
+    ])
+    const events = yield* adapter.inbox.replay(rowTs(2))
+    expect(events.flatMap((e) => (e.kind === 'message-posted' ? [e.message.body] : []))).toEqual(
+      [2, 3, 4, 5, 6, 7, 8, 9].map((id) => decodeMessageBodySync(`m${id}`)),
+    )
+    expect(yield* historyAnchors(stub)).toEqual(['newest', '7', '4'])
+  }),
+)
+
 effectTest('inbox.replay calls /messages with anchor=newest and a generous num_before', () =>
   Effect.gen(function* () {
     const stub = yield* makeStubHttpClient
@@ -2730,6 +2910,9 @@ effectTest(
           history_limited: false,
         },
       })
+      // Catch-up walks back until a page adds nothing new, so the realm has
+      // to keep answering past the raw page and its rendered twin.
+      yield* stub.respond('GET', '/api/v1/messages', messagesPage([]))
       yield* stub.respondSequence('GET', '/api/v1/messages', [
         replayPage('@**hermes-agent** wake up'),
         replayPage(