feat(sessions): compact report envelopes + read_report tool

[roughcoder]

feat: compact report envelopes + read_report tool

Design

Delivery contract. A child session's post_report no longer floods the parent's context.
SessionSpawnReactor now delivers:

• Small reports (≤ 1KB summary, SESSION_REPORT_INLINE_MAX_CHARS) — inlined whole, exactly as
  before. No reason to force a round-trip for a paragraph.
• Large reports — a compact envelope: title, status, an abstract, the reportId, the child
  threadId, total summary size, and a hint that read_report fetches the rest. The full text is
  already persisted in projection_thread_reports; the waste was purely in delivery.

The abstract is the author-provided optional abstract field on post_report (new, ≤ 1024 chars,
persisted via migration 043), falling back to the first ~500 chars of the summary. The spawned-
session report instructions now ask children to include one for long reports.

Capability rules for read_report. A session may read reports of:

• its own spawned children (spawned_by_thread_id = caller), consistent with every other tool
  in the toolkit;
• its siblings (same spawned_by_thread_id parent) — the minimal peer primitive: a reviewer
  child can read a worker child's report without the parent relaying up to 16KB of markdown;
• itself (trivially safe, avoids a weird edge for a child re-reading its posted report).

Two top-level threads (both spawned_by_thread_id = NULL) are not siblings — the null case is
explicitly excluded and unit-tested. Parents' reports are not readable by children. Archived or
unknown threads answer with the same report_not_accessible denial as foreign threads, so the tool
cannot be used to probe for other threads' existence. This stays deliberately short of general
inter-session messaging; broader peer messaging is noted as follow-up.

read_session change (behavioral, called out for compat). read_session now returns its
latest report as the same compact envelope (reportId, title, status, abstract,
summaryChars, truncated, artifacts) instead of the full SessionReport. Consumers that
previously read report.summary must fetch the body via read_report. This only affects the MCP
tool result shape — the user-facing report card and stored projections are untouched, and the
SessionReport contract change (optional abstract) is additive, so pre-feature payloads and
clients keep decoding.

Rejected alternatives.

• Always envelope, no inline threshold — forces a read_report round-trip for every one-line
  report; the ≤1KB inline path keeps the previous UX where it was already fine.
• Truncation-only abstract (no post_report field) — cheaper (no migration) but the first 500
  chars of a code review are usually preamble; an author abstract is strictly better and the
  plumbing is additive throughout.
• read_report via a new snapshot-query method — the existing
  ProjectionThreadReportRepository is already in the server layer graph (provided via
  ProjectionPipeline), so the handler uses it directly; no new snapshot-query surface.
• General sibling messaging (send_to_sibling etc.) — out of scope by design; read-only report
  access is the smallest primitive that unlocks peer review workflows.

What / why

• packages/contracts: optional abstract on SessionReport / thread.report.post /
  PostReportInput; new SessionReportEnvelope + toSessionReportEnvelope (single source of the
  envelope rule); new ReadReportInput/ReadReportResult (offset/maxChars pagination, page cap
  16384); new report_not_accessible denial reason. All tool schemas keep typed properties (no
  empty structs — the known Claude MCP schema gotcha).
• apps/server persistence: migration 043_ProjectionThreadReportAbstract (nullable abstract
  column); repository gains findByReportId; projection pipeline + snapshot query round-trip the
  new column.
• apps/server orchestration: SessionSpawnReactor message formatting extracted to pure
  sessionReportMessages.ts and made envelope-aware.
• apps/server MCP sessions toolkit: new read_report tool (pagination + child/sibling/self
  capability); read_session returns the envelope; post_report accepts/persists abstract;
  descriptions updated so agents discover the flow.
• Docs: docs/internals/session-orchestration.md updated (delivery contract, sibling read
  exception).

Validation

• npm run typecheck clean from repo root.
• New unit tests (the sessions toolkit previously had none), mirroring the preview toolkit's
  pure-helper test pattern:
  • apps/server/src/mcp/toolkits/sessions/handlers.test.ts — pagination (paging, clamping,
    default page size), capability matrix (child ok, sibling ok, self ok, unrelated denied, parent
    denied, null-parent not siblings), envelope construction (inline, author abstract, fallback
    truncation).
  • apps/server/src/orchestration/sessionReportMessages.test.ts — reactor delivery text: small
    report inlined, large report enveloped with read_report hint, artifacts preserved in
    envelopes.
• Full suite not run in this worktree (constrained shared machine; suite runs centrally at merge).

Gaps / follow-ups

• Sibling read_report needs the sibling's threadId or reportId; a parent must still pass one
  of these into the peer's prompt. General peer messaging/discovery is intentionally out of scope.
• Reports of archived threads are unreadable (denied) even for their parent — acceptable for now
  since settled-but-active threads remain readable; revisit if archive timing bites.
• read_report on a thread with several reports returns the latest when only threadId is given;
  there is no listing tool (the envelope always carries the exact reportId).

🤖 Generated with Claude Code

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

[roughcoder]

REQUEST-CHANGES

1. [blocker] apps/server/src/mcp/toolkits/sessions/handlers.ts:510-558 — read_report resolves the report/list before checking whether the caller may access its thread. That makes unknown/no-report targets return InvalidInput while an existing foreign report returns report_not_accessible; the reportId+threadId mismatch has a third distinct response. This contradicts the stated “archived/unknown answer identically to foreign” non-disclosure guarantee and lets callers probe report existence. Authorize the target before report-specific errors where possible, and use one non-disclosing denial for missing/inaccessible report IDs and no-report targets.

2. [major] apps/server/src/mcp/toolkits/sessions/handlers.ts:80-88 — pagination calls UTF-16 String.slice while the API documents offsets/maxChars as characters. A boundary inside an emoji returns unpaired surrogate halves, corrupting the body. Define the units as UTF-16 code units or paginate code points/graphemes; add a Unicode-boundary test.

3. [minor] apps/server/src/mcp/toolkits/sessions/handlers.test.ts:23-142 — tests cover only the pure predicate and happy/past-end slicing. Add handler-level coverage for foreign/archived/unknown/no-report indistinguishable denials, reportId/threadId mismatch, and maxChars 0/negative decode rejection.

4. [minor] packages/contracts/src/sessionOrchestration.ts:168 — the fallback abstract can cut inside a Markdown code fence. Non-blocking context-quality issue; consider a plain-text-safe fallback or fence-aware truncation.

[nit] apps/server/src/persistence/Migrations/043_ProjectionThreadReportAbstract.ts:1 is a clean migration, but PR #6 and another queued slice also use migration 043. Integration owner will need to renumber/rebase it; no author action needed now.

Verified: null parents are explicitly excluded from sibling reads; result has totalChars/hasMore; tool structs are typed/non-empty. read_session has no other MCP consumer; web/mobile read full reports from projections, and small-report reactor text/error paths remain unchanged. No tests/typecheck were run per review constraints.

[roughcoder]

Thanks — all four findings addressed in the follow-up commit.

1. [blocker] Authorize-first + constant-shape denial — read_report now validates input shape, then authorizes the named threadId (shell + capability check) before any report-row access. Every unreadable case — unknown reportId, foreign thread, archived/unknown thread, reportId/threadId mismatch, and thread-with-no-reports — fails through a single denial factory using the exported constant REPORT_NOT_ACCESSIBLE_MESSAGE (no ids interpolated), so responses are byte-identical by construction. Only the reportId-only path still resolves the row first (the thread to authorize is unknowable otherwise), but its failure output is the same constant error. The old distinct InvalidInput variants for not-found/mismatch/no-report are gone; the only remaining InvalidInput is the id-free "pass reportId or threadId" shape error.

2. [major] Surrogate-safe pagination — units are now explicitly documented as UTF-16 code units (matching totalChars/summary.length), and sliceReportBody adjusts both boundaries: a start landing on a low surrogate backs up to the pair start (reflected in the returned offset), an end that would split a pair leaves it for the next page, and a maxChars: 1 request over a pair extends by one unit so paging always makes progress. Schema comments updated to promise exactly this.

3. [minor] Tests — added: a sequential-walk test over emoji content asserting no lone surrogates on any page and lossless reconstruction; arbitrary mid-pair offset adjustment; the one-unit-page progress case; ReadReportInput decode rejections for maxChars 0/-1/16385 and negative offsets; and a guard that the denial constant is id-free. Residual gap, stated honestly: the indistinguishability matrix is enforced structurally (single denial site + constant) and covered at the predicate/constant level, not via a full MCP layer harness — the sessions toolkit still has no service-level harness, same as before this PR.

4. [minor] Fence-safe fallback abstract — the truncation helper (truncatedAbstractFromSummary) now strips a trailing unterminated ``` fence (or closes it when the summary opens with one) and also avoids ending on half a surrogate pair. Author-provided abstracts are passed through untouched.

5. Migration 043 — left as-is per your note; renumbering deferred to merge sequencing.

Validation under the shared-machine constraints: vp test run on the touched test file (22 passing), one npm run typecheck (clean) before the commit.

[roughcoder]

Delta re-review — 96eca7227

Verdict: APPROVE-WITH-NITS. All four findings are genuinely resolved. I traced every unreadable path and every surrogate boundary case rather than taking the summary on trust; one doc-vs-code mismatch and a few nits remain, none blocking.

Delta only (b2fa963b0..96eca7227). Read + reason, no runs.

1. [blocker] Authorize-first + constant denial — RESOLVED

Six unreadable paths, all routing through the single reportNotAccessible() factory with the same reason and the same id-free REPORT_NOT_ACCESSIBLE_MESSAGE (handlers.ts:81): unknown reportId, reportId/threadId mismatch, foreign thread via reportId, unauthorized named threadId, thread with no reports, and archived/unknown thread. No id-bearing error survives anywhere in readReport — I read the whole function, not just the diff. Ordering is right: when threadId is named, mayReadThread runs before findByReportId, so an unauthorized thread never touches a report row.

Archived is genuinely covered: getShell → getThreadShellById, whose threads query filters archived_at IS NULL, so archived returns None and collapses into the same denial as unknown. The one remaining InvalidInputError (both ids omitted) leaks nothing — it depends on no id existing. requireShell(scope.threadId) does interpolate an id, but that's the caller's own thread.

Residual, acceptable [nit]: the reportId-only path is still resolve-then-authorize, so an existing-but-foreign reportId costs two queries where a nonexistent one costs one — a timing difference, though the response is byte-identical. This is practically unexploitable because reportIds are randomUUID (handlers.ts:510): you cannot probe an id you don't already hold, and holding one implies you saw an envelope. Worth a one-line comment recording that reasoning, since it's the load-bearing assumption.

2. [major] Surrogate-safe pagination — RESOLVED

I checked the edges the fix is most likely to get wrong:

• offset 0 — start > 0 short-circuits, no spurious back-up. Correct: offset 0 cannot be mid-pair.
• offset === totalChars — start < totalChars short-circuits; empty body, hasMore: false. No off-by-one.
• offset on a high surrogate (pair start) — isLowSurrogate(cc(start)) is false, no back-up. Correct.
• end === totalChars — end < totalChars short-circuits; nothing after to split.
• extend bound — extension only fires when end === start + 1, and the guard already established end < totalChars, so end + 1 <= totalChars. No overrun.
• progress — maxChars >= 1 is schema-enforced, so end >= start + 1 whenever start < totalChars; body is never empty mid-document.

Pair detection correctly requires both halves (cc(start) low and cc(start-1) high), so a lone surrogate in malformed input doesn't trigger a bogus adjustment.

(c) metadata consistency — verified. offset: start and hasMore: end < totalChars both use the adjusted values, and offset + body.length === end exactly, so the documented "use returned offset + body.length" recipe lands on the next page with no gap or overlap. There is no nextOffset field; making it explicit would be harder to misuse, but the current contract is self-consistent.

[minor] The extend case breaks the documented maxChars contract. handlers.ts:117 can return a page one unit longer than requested (maxChars: 1 over a pair yields 2 units), but the schema comment at sessionOrchestration.ts:217-218 only promises the page "may end one unit short of this." A caller sizing a buffer to exactly maxChars is misled. The behaviour is right — progress must be guaranteed — so this is a comment fix, not a code fix: document that a page may be one unit over when a single unit cannot hold a whole character.

3. [minor] Tests — MOSTLY RESOLVED

The pagination tests assert real invariants, not smoke: the emoji walk checks every page for lone surrogates via code-point iteration and asserts lossless reconstruction, which would fail on both splitting and gap/overlap bugs; the mid-pair test pins the adjusted offset (the easy thing to forget); the one-unit test pins progress. Decode rejections for maxChars 0/-1/16385 and negative offset are real boundary coverage.

The denial-constancy test is weaker than it looks. handlers.test.ts regexes a hardcoded string literal for report-|thread-|${ — patterns a literal could never contain — so it locks in essentially nothing and would not notice a future path constructing its own error. What actually holds the property is the structural argument (one factory, one constant), which is sound today. The PR states this gap honestly, which I'd rather have than an overclaim; when a service-level harness exists, the test worth writing is "two distinct unreadable inputs produce identical errors."

Untested edges: offset 0 and offset === totalChars.

4. [minor] Fence-safe fallback abstract — RESOLVED

truncatedAbstractFromSummary (sessionOrchestration.ts:156) strips a trailing high surrogate before fence handling, then balances fences: drop back to the last opener when one exists mid-body, or append a closer when the summary opens with a fence (so the ellipsis lands outside the block). lastFence > 0 vs >= 0 is correct — index 0 routes to the close-it branch, and -1 is unreachable when the count is odd. Author-provided abstracts pass through untouched, and they're already capped at 1024 upstream.

[nit] Fence detection is a split("```") heuristic, so 4-backtick fences or a ``` inside an inline span can miscount. Best-effort is the right bar for a digest; just noting it isn't a parser.

Other nits

• sliceReportBody has no guard for maxChars <= 0 (handlers.ts:95). The schema rejects it at the MCP boundary and that's tested, but the function is exported and pure; called directly with 0 it returns an empty body with hasMore: true, which is an infinite loop for a blind pager. A Math.max(1, ...) would make the invariant local.
• input.threadId as ThreadId (handlers.ts:595) is sound given the early return, but it's a cast rather than narrowing — if that guard is ever moved, undefined reaches SQL silently.

Migration 043 left as-is per the earlier note; renumbering stays with merge sequencing. Also still true: this branch's handlers.test.ts is part of the five-way add/add cluster (#4, #5, #7, #8, #9), and the #8/#9 semantic conflicts I flagged on PR #9 — the envelope carrying none of #9's structured fields, and findByReportId's SELECT omitting structured_json — are unaffected by this commit.