refactor: share hook spool, bounded JSON parser, and memory kinds by ZeR020 · Pull Request #1473 · kunickiaj/codemem

ZeR020 · 2026-08-17T12:38:28Z

First stacked PR for the pi integration review on #1430. No pi client code in this layer.

Why

#1430 asked that spool/locking be shared instead of adding a third copy, that new memory-tool POSTs reuse the bounded JSON parser, and that MCP/viewer memory kinds share a core catalog. This PR extracts those primitives on current main so later pi layers can sit on them.

What

Parameterized createHookIngestSpool used by claude and codex wrappers (same export names, same TTL/attempt numbers)
Claude HTTP-success path drains the backlog before the boundary flush
parseJsonObjectBody exported from packages/viewer-server/src/helpers.ts
Core memory-kinds.ts: seven remember kinds + session_summary; MCP re-exports the seven

Test plan

vitest run on claude-hook-ingest-spool, claude-hook-ingest, codex-hook-ingest, helpers, memory-kinds — 63/63 passed
tsc -b packages/cli --force passed

Stacked follow-ups (not in this PR): core pi adapter → /api/pi-hooks alias → observer auth → CLI → extension → tools → setup/docs.

Related: #1430, #1429

chatgpt-codex-connector · 2026-08-17T12:38:33Z

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

Copilot

Pull request overview

This PR refactors shared primitives that multiple integrations depend on (CLI hook-ingest durability, viewer-server bounded JSON parsing, and a canonical memory-kind catalog) so follow-up “pi” integration layers can reuse the same building blocks without duplicating logic.

Changes:

Extracts a shared createHookIngestSpool durability layer (lock + spool + recovery + drain) and wires Claude/Codex wrappers to it.
Promotes the bounded JSON object body parser into packages/viewer-server/src/helpers.ts and reuses it from the raw-events routes.
Introduces a core memory-kinds catalog + validation, and re-exports the MCP catalog from core to prevent drift.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
packages/viewer-server/src/routes/raw-events.ts	Switches to using the shared `parseJsonObjectBody` helper for bounded JSON parsing.
packages/viewer-server/src/helpers.ts	Exports `parseJsonObjectBody` so routes can share a single bounded JSON parser implementation.
packages/viewer-server/src/helpers.test.ts	Adds unit tests covering `parseJsonObjectBody` size bounds and shape validation.
packages/mcp-server/src/memory-kinds.ts	Re-exports the MCP memory-kind descriptions from `@codemem/core` to prevent catalog drift.
packages/core/src/store.ts	Replaces local memory-kind validation with `validateMemoryKind` imported from the new core catalog.
packages/core/src/memory-kinds.ts	Adds canonical memory-kind catalog + allowed set + `validateMemoryKind`.
packages/core/src/memory-kinds.test.ts	Adds tests to lock down the catalog shape and validation behavior.
packages/core/src/index.ts	Re-exports `memory-kinds` from core’s public entrypoint.
packages/core/src/filters.ts	Hardens the “kind” filter against non-string JSON inputs to avoid malformed bound parameters.
packages/cli/src/commands/hook-ingest-spool.ts	Adds the shared hook-ingest lock/spool/recovery/drain implementation used by multiple CLI commands.
packages/cli/src/commands/codex-hook-ingest-spool.ts	Replaces duplicated codex lock/spool code with configuration over the shared spool implementation.
packages/cli/src/commands/claude-hook-ingest.ts	Changes HTTP-success flow to drain any backlog before running boundary flush.
packages/cli/src/commands/claude-hook-ingest.test.ts	Adds a regression test asserting backlog drain occurs before boundary flush on HTTP-success.
packages/cli/src/commands/claude-hook-ingest-spool.ts	Replaces duplicated claude lock/spool code with configuration over the shared spool implementation (retaining flush predicate).

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

kunickiaj

Thanks for splitting this foundation out of #1430—the shared spool factory and bounded JSON helper are much easier to review in this form, and the targeted tests pass locally (187 tests plus the CLI TypeScript build).

I found two items I think we should address before merging:

This branch currently conflicts with main. Recent changes added rawEventTarget/targetMismatch handling in the same Claude and Codex ingest paths touched here. Could you restack and preserve those safeguards during conflict resolution?
The new memory-kind catalog does not yet drive the MCP validation schema, so kind enforcement can still drift.

I also left a couple of focused test suggestions for the new abstraction. Once the branch is restacked and those catalog changes are addressed, I would be happy to take another pass.

kunickiaj · 2026-08-21T15:58:39Z

+ * the kinds never drift.
+ */
+
+export const MEMORY_KIND_DESCRIPTIONS: Record<string, string> = {


Nice direction—having the descriptions and store validation share one catalog removes an existing source of drift.

Could we also derive the actual MCP validator from this catalog? packages/mcp-server/src/schemas.ts still has a separate hardcoded z.enum(...), while ingest-pipeline.ts and ingest-xml-parser.ts retain similar lists.

Preserving literal keys here with something like as const satisfies Record<string, string> would let REMEMBER_MEMORY_KINDS drive the MCP schema and make the “canonical catalog” invariant enforceable rather than documentary.

memoryKindSchema is now z.enum(REMEMBER_MEMORY_KINDS). Ingest-pipeline and SUPPORTED_OBSERVATION_KINDS use the same tuple.

kunickiaj · 2026-08-21T15:58:39Z

+
+export type { SpoolDrainResult } from "./hook-ingest-spool.js";
+
+const spool = createHookIngestSpool({


Could we add a small focused test for the Codex factory configuration? The extraction moves several important behaviors into this config object—environment variable names, TTL, spool directory, retry count, and error identity—but the detailed spool tests currently exercise only the Claude instance.

At minimum, it would be useful to assert the Codex TTL/env overrides and that Claude and Codex resolve to separate spool directories. That would protect against a future configuration typo silently mixing the queues.

Added codex-hook-ingest-spool.test.ts for the 120s TTL, env override, separate spool dirs, and CodexHookLockBusyError.

kunickiaj · 2026-08-21T15:58:39Z

+		// Drain any spooled backlog before the boundary flush so the
+		// flush pass sees every queued payload of the session, not just
+		// this event.
 		await drainBacklogIfPresent();


Draining the backlog before the boundary flush makes sense and fixes the ordering issue described in the PR.

Could the regression test record both the queued HTTP handling and boundaryFlush in one sequence, then assert that the drain completes first? The current test confirms that the backlog drains, but its payload is a Stop event with flushing disabled by default, so it does not exercise the ordering this change is intended to guarantee.

The SessionEnd case already records http:queued before flush:fresh. Left that test in place.

kunickiaj · 2026-08-21T15:58:39Z

+			}
+			if (parsed == null || typeof parsed !== "object" || Array.isArray(parsed)) {
+				// Parseable but wrong shape — same problem, same fix.
+				quarantineSpoolEntry(dir, name, "wrong-shape");


This shared implementation now increments failed for wrong-shape Claude spool entries, whereas the previous Claude implementation quarantined them without incrementing the count. The new behavior looks more consistent, but could we assert the returned { processed, failed } result in the existing wrong-shape test so the intentional change is captured?

The existing non-object test now asserts { processed: 0, failed: 2 }.

ZeR020 · 2026-08-23T15:30:28Z

Restacked onto current main and kept the rawEventTarget / targetMismatch path. REMEMBER_MEMORY_KINDS now drives the MCP z.enum plus the ingest-pipeline and observer XML kind sets.

kunickiaj

Re-reviewed at f8c95d1. All prior requested changes are addressed: the branch is cleanly restacked with viewer-target safeguards preserved, MCP and observer enforcement now derive from the core memory-kind catalog, Codex spool configuration/isolation has focused coverage, the boundary-flush ordering is asserted, and wrong-shape drain accounting is covered.

Local validation passed: 202 targeted tests, workspace TypeScript build, and Biome lint. CI is also green. No merge blockers remain.

Approving this revision. Please hold the merge until after the 0.43 release so it lands in the intended release sequence.

Extract the claude/codex lock-and-spool copies into one parameterized helper so a later pi client does not add a third concurrency path. Export the viewer bounded JSON parser and a single core kind catalog (seven remember kinds plus session_summary) so HTTP and MCP cannot drift. Drain the claude HTTP-success backlog before boundary flush.

Layer-1 review: the typeof guard in buildFilterClauses had no regression test.

Keep remember-kind enforcement on one const catalog so the MCP zod enum and observer ingest lists cannot drift independently.

Cover Codex TTL/env/dir isolation plus the shared drain result for quarantined non-object spool entries.

ZeR020 · 2026-08-28T19:03:02Z

@kunickiaj 0.43 is out. This is still approved from Aug 25. Restacked onto current main.

kunickiaj · 2026-08-28T19:08:27Z

Merge activity

Aug 28, 7:08 PM UTC: Graphite couldn't merge this PR because it failed for an unknown reason (Fast-forward merges are not supported for forked repositories. Please create a branch in the target repository in order to merge).

ZeR020 · 2026-08-28T19:52:18Z

@kunickiaj Graphite couldn’t merge this: fast-forward merge isn’t supported for fork PRs. GitHub shows it mergeable with green CI. Could you merge it from the GitHub UI (Merge or Squash) instead of Graphite?

Copilot AI lite review requested due to automatic review settings August 17, 2026 12:38

Copilot started reviewing on behalf of ZeR020 August 17, 2026 12:38 View session

Copilot AI reviewed Aug 17, 2026

View reviewed changes

kunickiaj requested changes Aug 21, 2026

View reviewed changes

ZeR020 force-pushed the stack/1-shared-spool-memory branch from cfc0654 to d21ae92 Compare August 23, 2026 15:28

ZeR020 force-pushed the stack/1-shared-spool-memory branch from d21ae92 to f8c95d1 Compare August 24, 2026 19:54

kunickiaj approved these changes Aug 25, 2026

View reviewed changes

ZeR020 added 4 commits August 25, 2026 17:54

test(core): ignore non-string kind filters

dee82de

Layer-1 review: the typeof guard in buildFilterClauses had no regression test.

refactor(core): derive MCP and ingest kinds from the catalog

5f8f7c9

Keep remember-kind enforcement on one const catalog so the MCP zod enum and observer ingest lists cannot drift independently.

test(cli): lock Codex spool config and wrong-shape drain counts

55531d7

Cover Codex TTL/env/dir isolation plus the shared drain result for quarantined non-object spool entries.

ZeR020 force-pushed the stack/1-shared-spool-memory branch from f8c95d1 to 55531d7 Compare August 25, 2026 17:54

ZeR020 added 2 commits August 26, 2026 20:12

Merge branch 'main' into stack/1-shared-spool-memory

800a3d0

Merge branch 'main' into stack/1-shared-spool-memory

eec2701

kunickiaj merged commit 1410603 into kunickiaj:main Aug 28, 2026
11 checks passed

ZeR020 mentioned this pull request Aug 29, 2026

feat(core): add pi event adapter for 0.41 ingest #1546

Merged

ZeR020 mentioned this pull request Sep 10, 2026

feat(viewer-server): add /api/pi-hooks alias and memory tool routes #1678

Open


		export type { SpoolDrainResult } from "./hook-ingest-spool.js";

		const spool = createHookIngestSpool({

Conversation

ZeR020 commented Aug 17, 2026

Why

What

Test plan

Uh oh!

chatgpt-codex-connector Bot commented Aug 17, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

kunickiaj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZeR020 commented Aug 23, 2026

Uh oh!

kunickiaj left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZeR020 commented Aug 28, 2026

Uh oh!

kunickiaj commented Aug 28, 2026

Merge activity

Uh oh!

ZeR020 commented Aug 28, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kunickiaj left a comment •

edited

Loading